Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: restrict private key permissions #1016

Merged
merged 3 commits into from
Mar 27, 2023
Merged

fix: restrict private key permissions #1016

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1490513
fix: restrict private file permissions by default
buchdag Mar 27, 2023
ce16975
fix: check perms of /etc/acme.sh private keys
buchdag Mar 27, 2023
54a37dd
fix: typo
buchdag Mar 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions app/functions.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"
Expand Down
2 changes: 2 additions & 0 deletions app/letsencrypt_service
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' \
Expand Down
24 changes: 6 additions & 18 deletions docs/Persistent-data.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
17 changes: 9 additions & 8 deletions test/tests/permissions_custom/run.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

files_uid=1000
files_gid=1001
files_perms=640
files_perms=644
folders_perms=750

if [[ -z $GITHUB_ACTIONS ]]; then
Expand Down Expand Up @@ -56,18 +56,19 @@ 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=( \
[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
Expand Down
19 changes: 10 additions & 9 deletions test/tests/permissions_default/run.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,25 +50,26 @@ 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=( \
[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
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

Expand Down