fix: restrict private key permissions (#1016)

* fix: restrict private file permissions by default

* fix: check perms of /etc/acme.sh private keys

* fix: typo

app/functions.sh

@@ -344,10 +344,10 @@ function reload_nginx {
 
 function set_ownership_and_permissions {
   local path="${1:?}"
-  # The default ownership is root:root, with 755 permissions for folders and 644 for files.
+  # The default ownership is root:root, with 755 permissions for folders and 600 for private files.
   local user="${FILES_UID:-root}"
   local group="${FILES_GID:-$user}"
-  local f_perms="${FILES_PERMS:-644}"
+  local f_perms="${FILES_PERMS:-600}"
   local d_perms="${FOLDERS_PERMS:-755}"
 
   if [[ ! "$f_perms" =~ ^[0-7]{3,4}$ ]]; then
@@ -406,7 +406,7 @@ function set_ownership_and_permissions {
     # If the path is a file, check and modify permissions if required.
     elif [[ -f "$path" ]]; then
       # Use different permissions for private files (private keys and ACME account files) ...
-      if [[ "$path" =~ ^.*(default\.key|key\.pem|\.json)$ ]]; then
+      if [[ "$path" =~ ^.*(key\.pem|\.key)$ ]]; then
         if [[ "$(stat -c %a "$path")" != "$f_perms" ]]; then
           [[ "$DEBUG" == 1 ]] && echo "Debug: setting $path permissions to $f_perms."
           chmod "$f_perms" "$path"

app/letsencrypt_service

@@ -374,6 +374,8 @@ function update_cert {
             local file_path="${certificate_dir}/${file}"
             [[ -e "$file_path" ]] && set_ownership_and_permissions "$file_path"
         done
+        local acme_private_key="$(find /etc/acme.sh -name "*.key" -path "*${hosts_array[0]}*")"
+        [[ -e "$acme_private_key" ]] && set_ownership_and_permissions "$acme_private_key"
         # Queue nginx reload if a certificate was issued or renewed
         [[ $acmesh_return -eq 0 ]] \
             && should_reload_nginx='true' \

docs/Persistent-data.md

@@ -68,19 +68,14 @@ By default, the **acme-companion** container will enforce the following ownershi
 
 ```
 [drwxr-xr-x]  /etc/nginx/certs
-├── [drwxr-xr-x root root]  accounts
-│   └── [drwxr-xr-x root root]  acme-v02.api.letsencrypt.org
-│       └── [drwxr-xr-x root root]  directory
-│           └── [-rw-r--r-- root root]  default.json
 ├── [-rw-r--r-- root root]  dhparam.pem
 ├── [-rw-r--r-- root root]  default.crt
-├── [-rw-r--r-- root root]  default.key
+├── [-rw------- root root]  default.key
 ├── [drwxr-xr-x root root]  domain.tld
-│   ├── [lrwxrwxrwx root root]  account_key.json -> ../accounts/acme-v02.api.letsencrypt.org/directory/default.json
 │   ├── [-rw-r--r-- root root]  cert.pem
 │   ├── [-rw-r--r-- root root]  chain.pem
 │   ├── [-rw-r--r-- root root]  fullchain.pem
-│   └── [-rw-r--r-- root root]  key.pem
+│   └── [-rw------- root root]  key.pem
 ├── [lrwxrwxrwx root root]  domain.tld.chain.pem -> ./domain.tld/chain.pem
 ├── [lrwxrwxrwx root root]  domain.tld.crt -> ./domain.tld/fullchain.pem
 ├── [lrwxrwxrwx root root]  domain.tld.dhparam.pem -> ./dhparam.pem
@@ -91,30 +86,23 @@ This behavior can be customized using the following environment variable on the
 
 * `FILES_UID` - Set the user owning the files and folders managed by the container. The variable can be either a user name if this user exists inside the container or a user numeric ID. Default to `root` (user ID `0`).
 * `FILES_GID` - Set the group owning the files and folders managed by the container. The variable can be either a group name if this group exists inside the container or a group numeric ID. Default to the same value as `FILES_UID`.
-* `FILES_PERMS` - Set the permissions of the private keys and ACME account keys. The variable must be a valid octal permission setting and defaults to `644`.
+* `FILES_PERMS` - Set the permissions of the private keys. The variable must be a valid octal permission setting and defaults to `600`.
 * `FOLDERS_PERMS` - Set the permissions of the folders managed by the container. The variable must be a valid octal permission setting and defaults to `755`.
 
-For example, `FILES_UID=1000`, `FILES_PERMS=600` and `FOLDERS_PERMS=700` will result in the following:
+For example, `FILES_UID=1000`, `FILES_PERMS=644` and `FOLDERS_PERMS=700` will result in the following:
 
 ```
 [drwxr-xr-x]  /etc/nginx/certs
-├── [drwx------ 1000 1000]  accounts
-│   └── [drwx------ 1000 1000]  acme-v02.api.letsencrypt.org
-│       └── [drwx------ 1000 1000]  directory
-│           └── [-rw------- 1000 1000]  default.json
 ├── [-rw-r--r-- 1000 1000]  dhparam.pem
 ├── [-rw-r--r-- 1000 1000]  default.crt
-├── [-rw------- 1000 1000]  default.key
+├── [-rw-r--r-- 1000 1000]  default.key
 ├── [drwx------ 1000 1000]  domain.tld
-│   ├── [lrwxrwxrwx 1000 1000]  account_key.json -> ../accounts/acme-v02.api.letsencrypt.org/directory/default.json
 │   ├── [-rw-r--r-- 1000 1000]  cert.pem
 │   ├── [-rw-r--r-- 1000 1000]  chain.pem
 │   ├── [-rw-r--r-- 1000 1000]  fullchain.pem
-│   └── [-rw------- 1000 1000]  key.pem
+│   └── [-rw-r--r-- 1000 1000]  key.pem
 ├── [lrwxrwxrwx 1000 1000]  domain.tld.chain.pem -> ./domain.tld/chain.pem
 ├── [lrwxrwxrwx 1000 1000]  domain.tld.crt -> ./domain.tld/fullchain.pem
 ├── [lrwxrwxrwx 1000 1000]  domain.tld.dhparam.pem -> ./dhparam.pem
 └── [lrwxrwxrwx 1000 1000]  domain.tld.key -> ./domain.tld/key.pem
 ```
-
-If you just want to make the most sensitive files (private keys and ACME account keys) root readable only, set the environment variable `FILES_PERMS` to `600` on your **acme-companion** container.

test/tests/permissions_custom/run.sh

@@ -4,7 +4,7 @@
 
 files_uid=1000
 files_gid=1001
-files_perms=640
+files_perms=644
 folders_perms=750
 
 if [[ -z $GITHUB_ACTIONS ]]; then
@@ -56,18 +56,19 @@ symlinks=( \
   [3]="/etc/nginx/certs/${domains[0]}.dhparam.pem" \
   )
 
-  # Test symlinks paths
+# Test symlinks paths
-  for symlink in  "${symlinks[@]}"; do
+for symlink in  "${symlinks[@]}"; do
   ownership="$(docker exec "$le_container_name" stat -c %u:%g "$symlink")"
   if [[ "$ownership" != ${files_uid}:${files_gid} ]]; then
     echo "Expected ${files_uid}:${files_gid} on ${symlink}, found ${ownership}."
   fi
-  done
+done
 
 # Array of private file paths to test
 private_files=( \
   [0]="/etc/nginx/certs/default.key" \
   [1]="/etc/nginx/certs/${domains[0]}/key.pem" \
+  [2]="/etc/acme.sh/default/${domains[0]}/${domains[0]}.key" \
   )
 
 # Test private file paths

test/tests/permissions_default/run.sh

@@ -50,25 +50,26 @@ symlinks=( \
   [3]="/etc/nginx/certs/${domains[0]}.dhparam.pem" \
   )
 
-  # Test symlinks paths
+# Test symlinks paths
-  for symlink in  "${symlinks[@]}"; do
+for symlink in  "${symlinks[@]}"; do
   ownership="$(docker exec "$le_container_name" stat -c %u:%g "$symlink")"
   if [[ "$ownership" != 0:0 ]]; then
     echo "Expected 0:0 on ${symlink}, found ${ownership}."
   fi
-  done
+done
 
 # Array of private file paths to test
 private_files=( \
   [0]="/etc/nginx/certs/default.key" \
   [1]="/etc/nginx/certs/${domains[0]}/key.pem" \
+  [2]="/etc/acme.sh/default/${domains[0]}/${domains[0]}.key" \
   )
 
 # Test private file paths
 for file in  "${private_files[@]}"; do
   ownership_and_permissions="$(docker exec "$le_container_name" stat -c %u:%g:%a "$file")"
-  if [[ "$ownership_and_permissions" != 0:0:644 ]]; then
+  if [[ "$ownership_and_permissions" != 0:0:600 ]]; then
-    echo "Expected 0:0:644 on ${file}, found ${ownership_and_permissions}."
+    echo "Expected 0:0:600 on ${file}, found ${ownership_and_permissions}."
   fi
 done