fix: Setting gid on certificates (#62)

.gitattributes

@@ -1,5 +1,6 @@
 * text=auto
 *.sh            eol=lf
 **/run          eol=lf
+*/usr/sbin/*    eol=lf
 */services.d/*  eol=lf
 cron-tick       eol=lf

Dockerfile

@@ -1,6 +1,6 @@
 FROM certbot/certbot:v1.18.0 as certbot
 
-FROM homecentr/cron-base:2.0.4
+FROM homecentr/cron-base:2.0.6
 
 ARG CERTBOT_PIP_VERSION="1.17.0"
 
@@ -48,8 +48,8 @@ RUN apk add --no-cache \
 
 COPY ./fs/ /
 
-RUN mkdir /logs && chmod 0777 /logs
+RUN chmod a+x /usr/sbin/check-dirs-writable
 
-VOLUME "/etc/letsencrypt"
-VOLUME "/data"
+VOLUME "/state"
+VOLUME "/certs"
 VOLUME "/logs"

README.md

@@ -35,7 +35,7 @@ services:
 | PUID | 7077 | UID of the user certbot be running as. |
 | PGID | 7077 | GID of the user certbot be running as. |
 | CERTBOT_ARGS | | Additional arguments passed to certbot's `certonly` command. The argument `--agree-tos` is passed automatically, but you have to provide the `--email` argument. |
-| CERTS_GID | 7077 | GID of a group which set as group owner of the certificates in the `/data` directory. This is to simplify sharing the certificates with other containers/components. |
+| CERTS_GID | 7077 | GID of a group which set as group owner of the certificates in the `/certs` directory. This is to simplify sharing the certificates with other containers/components. |
 
 ## Exposed ports
 
@@ -45,9 +45,9 @@ This image does not expose any ports.
 
 | Container path | Description |
 |------------|---------------|
-| /etc/letsencrypt | Directory where certbot keeps its state. This directory should be persisted to avoid issuing the same certificate multiple times. |
-| /data | The output certificates will be placed in this directory. This is the directory you can/want share with other components. The certificates are standard files, not symlinks. |
-| /logs | Certbot will output detailed logs into this directory. Make sure the PUID user has write permissions in this directory. |
+| /state | Directory where certbot keeps its state. This directory should be persisted to avoid issuing the same certificate multiple times. This directory must be **writable** by PUID or PGID. |
+| /certs | The output certificates will be placed in this directory. This is the directory you can/want share with other components. The certificates are standard files, not symlinks. This directory must be **writable** by PUID or PGID. |
+| /logs | Certbot will output detailed logs into this directory. Make sure the PUID user has write permissions in this directory. This directory must be **writable** by PUID or PGID. |
 
 ## Security
 The container is regularly scanned for vulnerabilities and updated. Further info can be found in the [Security tab](https://github.com/homecentr/docker-certbot).

fs/cron/cron-tick

@@ -1,17 +1,15 @@
 #!/usr/bin/env ash
 
 # Execute certbot
-certbot certonly --agree-tos --non-interactive --logs-dir /logs $CERTBOT_ARGS
+certbot certonly --agree-tos --non-interactive --work-dir /state/work --config-dir /state/config --logs-dir /logs $CERTBOT_ARGS
 
 # Fix file permissions
-echo "Updating certs ownership to $PUID:${CERTS_GID:-$PGID}"
+echo "Updating certs group ownership to gid=${CERTS_GID:-$PGID}"
 
-chown "$PUID" -R /etc/letsencrypt
-chgrp "${CERTS_GID:-$PGID}" -R /etc/letsencrypt
-chmod "0750" -R /etc/letsencrypt
+# Chmod etc. usually do not follow symlinks when in recursive mode. All files in .../live/...
+# are symlinks and hence the target files' permissions must be changed directly.
+chgrp "${CERTS_GID:-$PGID}" -R /state/config/archive/*/*.pem
+chmod "0640" -R /state/config/archive/*/*.pem
 
-# Copy files over to /data while preserving the file permissions
-cp -p /etc/letsencrypt/live/*/*.pem /data
-
-# Output the log files to stdout
-cat /logs/*
+# Copy files over to /certs while preserving the file permissions
+cp -p /state/config/live/*/*.pem /certs

fs/etc/cont-init.d/15-readers-group.sh

@@ -1,29 +1,21 @@
 #!/usr/bin/with-contenv ash
 
+source homecentr_create_group
+source homecentr_set_s6_env_var
+source homecentr_get_s6_env_var
+
 EXEC_USER=$(cat /var/run/s6/container_environment/EXEC_USER)
 
 if [ "$CERTS_GID" == "0" ] || [ "$CERTS_GID" == "" ]
 then
-  # User doesn't want special group ownership, the group owner of the files will be PGID, skip creating the group
   return
 fi
 
-if [ "$CERTS_GID" == "$PGID" ]
-then
-  CERTS_GID=$PGID
-fi
-
-# Check if the group already exists
-cat /etc/group | grep ^ssl-readers: > /dev/null
-
-if [ $? == 0 ]
-then
-  # Group already exists, delete it
-  delgroup ssl-readers
-fi
-
 # Make sure we really need to create a separate group
 if [ "$PGID" != "$CERTS_GID" ]
 then
-  addgroup -g $CERTS_GID ssl-readers
+  homecentr_create_group "$CERTS_GID" "ssl-readers"
+
+  # Add executing user to the group
+  addgroup "$EXEC_USER" ssl-readers
 fi

fs/etc/cont-init.d/60-check-file-permissions.sh

@@ -0,0 +1,4 @@
+#!/usr/bin/with-contenv ash
+
+# Check if directory writable as PUID/PGID, this is why it's in a separate script because it's running PUID/PGID context
+runas check-dirs-writable

fs/etc/cont-init.d/98-verify-args.sh

@@ -2,4 +2,6 @@
 
 if [[ "$CERTBOT_ARGS" != *"--email"* ]]; then
   echo "The CERTBOT_ARGS variable must contain '--email' as certbot is executed in a non-interactive way."
+
+  exit 1
 fi

fs/etc/cont-init.d/99-first-execution.sh

@@ -1,4 +1,4 @@
 #!/usr/bin/with-contenv ash
 
 echo "Executing the certbot immediately to ensure the certificate exists..."
-cron-tick-execute
+runas cron-tick-execute

fs/usr/sbin/check-dirs-writable

@@ -0,0 +1,28 @@
+#!/usr/bin/env ash
+
+ls -l /
+
+SHOULD_EXIT=0
+
+if ! test -w "/certs"
+then
+    echo "Directory /certs is not writable by the user $(whoami)!"
+    SHOULD_EXIT=1
+fi
+
+if ! test -w "/logs"
+then
+    echo "Directory /logs is not writable by the user $(whoami)!"
+    SHOULD_EXIT=1
+fi
+
+if ! test -w "/state"
+then
+    echo "Directory /state is not writable by the user $(whoami)!"
+    SHOULD_EXIT=1
+fi
+
+if [ "$SHOULD_EXIT" != "0" ]
+then
+    exit 2
+fi

tests/build.gradle

@@ -23,7 +23,7 @@ repositories {
 dependencies {
     testImplementation group: 'junit', name: 'junit', version: '4.13.2'
     testImplementation group: 'org.testcontainers', name: 'testcontainers', version: '1.16.0'
-    testImplementation group: 'io.homecentr', name: 'testcontainers-extensions', version: '1.6.0'
+    testImplementation group: 'io.homecentr', name: 'testcontainers-extensions', version: '1.7.0'
     testImplementation group: 'org.slf4j', name: 'slf4j-api', version: '1.7.32'
     testImplementation group: 'org.slf4j', name: 'slf4j-simple', version: '1.7.32'
 }

tests/src/test/java/CertbotContainerShould.java

@@ -25,14 +25,16 @@ public class CertbotContainerShould {
     @BeforeClass
     public static void before() throws Exception {
         _testConfig = TestConfiguration.create();
-        _testConfig.createCredentialsSecretFile();
 
         _certbotContainer = new GenericContainerEx<>(new CertbotDockerTagResolver())
                 .withEnv("PUID", "9001")
                 .withEnv("PGID", "9002")
                 .withEnv("CRON_SCHEDULE", "* * * * *")
                 .withEnv("CERTBOT_ARGS", _testConfig.getCertbotArgs())
-                .withFileSystemBind(TestConfiguration.cloudflareCredentialsHostPath, TestConfiguration.cloudflareCredentialsContainerPath)
+                .withFileSystemBind(_testConfig.getCloudflareCredentialFilePath(), TestConfiguration.cloudflareCredentialsContainerPath)
+                .withTempDirectoryBind("/certs", 9002)
+                .withTempDirectoryBind("/logs", 9002)
+                .withTempDirectoryBind("/state", 9002)
                 .withImagePullPolicy(PullPolicyEx.never())
                 .waitingFor(WaitEx.forS6OverlayStart());
 
@@ -49,34 +51,34 @@ public static void after() {
 
     @Test
     public void createCertificateFullChainFile() throws IOException, InterruptedException {
-        assertTrue(fileExists("/data/fullchain.pem"));
+        assertTrue(fileExists("/certs/fullchain.pem"));
 
-        assertEquals((Integer)9001, _certbotContainer.getFileOwnerUid("/data/fullchain.pem"));
-        assertEquals((Integer)9002, _certbotContainer.getFileOwningGid("/data/fullchain.pem"));
+        assertEquals((Integer)9001, _certbotContainer.getFileOwnerUid("/certs/fullchain.pem"));
+        assertEquals((Integer)9002, _certbotContainer.getFileOwningGid("/certs/fullchain.pem"));
     }
 
     @Test
     public void createCertificateChainFile() throws IOException, InterruptedException {
-        assertTrue(fileExists("/data/chain.pem"));
+        assertTrue(fileExists("/certs/chain.pem"));
 
-        assertEquals((Integer)9001, _certbotContainer.getFileOwnerUid("/data/chain.pem"));
-        assertEquals((Integer)9002, _certbotContainer.getFileOwningGid("/data/chain.pem"));
+        assertEquals((Integer)9001, _certbotContainer.getFileOwnerUid("/certs/chain.pem"));
+        assertEquals((Integer)9002, _certbotContainer.getFileOwningGid("/certs/chain.pem"));
     }
 
     @Test
     public void createPrivateKeyFile() throws IOException, InterruptedException {
-        assertTrue(fileExists("/data/privkey.pem"));
+        assertTrue(fileExists("/certs/privkey.pem"));
 
-        assertEquals((Integer)9001, _certbotContainer.getFileOwnerUid("/data/privkey.pem"));
-        assertEquals((Integer)9002, _certbotContainer.getFileOwningGid("/data/privkey.pem"));
+        assertEquals((Integer)9001, _certbotContainer.getFileOwnerUid("/certs/privkey.pem"));
+        assertEquals((Integer)9002, _certbotContainer.getFileOwningGid("/certs/privkey.pem"));
     }
 
     @Test
     public void createPublicKeyFile() throws IOException, InterruptedException {
-        assertTrue(fileExists("/data/cert.pem"));
+        assertTrue(fileExists("/certs/cert.pem"));
 
-        assertEquals((Integer)9001, _certbotContainer.getFileOwnerUid("/data/cert.pem"));
-        assertEquals((Integer)9002, _certbotContainer.getFileOwningGid("/data/cert.pem"));
+        assertEquals((Integer)9001, _certbotContainer.getFileOwnerUid("/certs/cert.pem"));
+        assertEquals((Integer)9002, _certbotContainer.getFileOwningGid("/certs/cert.pem"));
     }
 
     private boolean fileExists(String fileNamePattern) throws IOException, InterruptedException {