From 1490513e6f39434a7ee8ccde9141b12065e1ad79 Mon Sep 17 00:00:00 2001 From: Nicolas Duchon Date: Mon, 27 Mar 2023 08:23:18 +0200 Subject: [PATCH 1/3] fix: restrict private file permissions by default --- app/functions.sh | 6 +++--- docs/Persistent-data.md | 24 ++++++------------------ test/tests/permissions_custom/run.sh | 16 ++++++++-------- test/tests/permissions_default/run.sh | 18 +++++++++--------- 4 files changed, 26 insertions(+), 38 deletions(-) diff --git a/app/functions.sh b/app/functions.sh index 540a25dd..6ba73b0c 100644 --- a/app/functions.sh +++ b/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" diff --git a/docs/Persistent-data.md b/docs/Persistent-data.md index 971f91e7..16ff94b6 100644 --- a/docs/Persistent-data.md +++ b/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. diff --git a/test/tests/permissions_custom/run.sh b/test/tests/permissions_custom/run.sh index b9f99123..72665b29 100755 --- a/test/tests/permissions_custom/run.sh +++ b/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,13 +56,13 @@ symlinks=( \ [3]="/etc/nginx/certs/${domains[0]}.dhparam.pem" \ ) - # Test symlinks paths - 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 +# Test symlinks paths +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 # Array of private file paths to test private_files=( \ diff --git a/test/tests/permissions_default/run.sh b/test/tests/permissions_default/run.sh index 2d816b1e..30272204 100755 --- a/test/tests/permissions_default/run.sh +++ b/test/tests/permissions_default/run.sh @@ -50,13 +50,13 @@ symlinks=( \ [3]="/etc/nginx/certs/${domains[0]}.dhparam.pem" \ ) - # Test symlinks paths - 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 +# Test symlinks paths +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 # Array of private file paths to test private_files=( \ @@ -67,8 +67,8 @@ private_files=( \ # 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 - echo "Expected 0:0:644 on ${file}, found ${ownership_and_permissions}." + if [[ "$ownership_and_permissions" != 0:0:600 ]]; then + echo "Expected 0:0:600 on ${file}, found ${ownership_and_permissions}." fi done From ce169752ce6dbda11a5373890aa9782b2574259a Mon Sep 17 00:00:00 2001 From: Nicolas Duchon Date: Mon, 27 Mar 2023 08:28:43 +0200 Subject: [PATCH 2/3] fix: check perms of /etc/acme.sh private keys --- app/letsencrypt_service | 2 ++ test/tests/permissions_custom/run.sh | 1 + test/tests/permissions_default/run.sh | 1 + 3 files changed, 4 insertions(+) diff --git a/app/letsencrypt_service b/app/letsencrypt_service index edc2f9b3..7e165449 100755 --- a/app/letsencrypt_service +++ b/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' \ diff --git a/test/tests/permissions_custom/run.sh b/test/tests/permissions_custom/run.sh index 72665b29..01a5f56f 100755 --- a/test/tests/permissions_custom/run.sh +++ b/test/tests/permissions_custom/run.sh @@ -68,6 +68,7 @@ done 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 diff --git a/test/tests/permissions_default/run.sh b/test/tests/permissions_default/run.sh index 30272204..21bc1381 100755 --- a/test/tests/permissions_default/run.sh +++ b/test/tests/permissions_default/run.sh @@ -62,6 +62,7 @@ done 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 From 54a37dde467cc5829f2575ad8c8bbf1a16233e32 Mon Sep 17 00:00:00 2001 From: Nicolas Duchon Date: Mon, 27 Mar 2023 08:47:18 +0200 Subject: [PATCH 3/3] fix: typo --- app/letsencrypt_service | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/letsencrypt_service b/app/letsencrypt_service index 7e165449..ea9c4864 100755 --- a/app/letsencrypt_service +++ b/app/letsencrypt_service @@ -374,7 +374,7 @@ 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]}*") + 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 ]] \