nginx-proxy / acme-companion

Automated ACME SSL certificate generation for nginx-proxy
MIT License
7.42k stars 825 forks source link

Bugfix: Standalone ACMESH_${cid}_DNS_API_CONFIG env variable is not properly formatted to be parsed correctly. #1154

Closed stuartbirrell closed 2 months ago

stuartbirrell commented 2 months ago

The standalone ACMESH_${cid}_DNS_API_CONFIG env variable is not properly formatted when read and is unable to be properly parsed. So it always falls back to the global DEFAULT_ACMESH_DNS_API_CONFIG.

If you attempt to define a standalone DNS_API_CONFIG variable, the script will error.

nginx-acme-acme-companion  | [Thu Sep 26 12:55:40 UTC 2024] Domains not changed.
nginx-acme-acme-companion  | [Thu Sep 26 12:55:40 UTC 2024] Skipping. Next renewal time is: 2024-11-18T14:10:56Z
nginx-acme-acme-companion  | [Thu Sep 26 12:55:40 UTC 2024] Add '--force' to force renewal.
nginx-acme-acme-companion  | /app/letsencrypt_service: line 217: local: `0={ 'DNS_API': 'dns_cf', 'CF_Token': '<REDACTED>', 'CF_Account_ID': '<REDACTED>', 'CF_Zone_ID':'<REDACTED>'}': not a valid identifier
nginx-acme-acme-companion  | Info: DNS challenge using { 'DNS_API': 'dns_cf', 'CF_Token': '<REDACTED>', 'CF_Account_ID': '<REDACTED>', 'CF_Zone_ID':'<REDACTED>'} DNS API with the following keys: 0 (container config)
nginx-acme-acme-companion  | Creating/renewal <REDACTED.DOMAIN.COM> certificates... (<REDACTED.DOMAIN.COM>)

This prevents you from having multiple standalone domains that require different API settings for DNS-01. i.e one domain using Cloudflare and another using Dreamhost, for example.

This change will properly format the environment variable into an assoc array once it is read in and you can now successfully apply multiple DNS_API_CONFIG settings in standalone config.

buchdag commented 2 months ago

What the hell, I remember specifically testing that before merging #1137 😭

Do you happen to have the config that caused the error (with secrets and tokens sanitized, of course) ?

stuartbirrell commented 2 months ago

It was funny/weird when I was looking at it - it was obvious you clearly had intent on allow it to work, checked the docs to make sure I wasnt missing anything weird.

On this rollout I have a default solver set up against the acme-companion container - which works as expected of course

export ACME_CHALLENGE=DNS-01
export ACMESH_DNS_API_CONFIG="{ 'DNS_API': 'dns_dreamhost', 'DH_API_KEY': '<REDACTED>' }"

I also have a separate standalone configuration like so:

LETSENCRYPT_STANDALONE_CERTS=('altio')

LETSENCRYPT_altio_HOST=('my.host.com')
ACME_altio_CHALLENGE=DNS-01
ACMESH_altio_DNS_API_CONFIG="{ 'DNS_API': 'dns_cf', 'CF_Token': '<REDACTED>', 'CF_Account_ID': '<REDACTED>', 'CF_Zone_ID':'<REDACTED>'}"

It would be great if I missed a trick here, let me know please.

FWIW: I got a build of the container with my branch running on my system at the moment, works a treat.

buchdag commented 2 months ago

Ok, I understood what's happening there.

Contrary to all the other environment variables, the ACMESH_DNS_API_CONFIG is handled very differently by acme-companion's docker-gen template.

All other variables are first trimed and given a default empty value

{{ $FOO := trim (coalesce $container.Env.ACME_FOO "") }}

then rendered like this (the {{- "\n" }} is just for formatting purpose):

{{- "\n" }}ACME_{{ $cid }}_FOO="{{ $FOO }}"

which result in this in /app/letsencrypt_service_data

ACME_containerid_FOO="someValue"

ACMESH_DNS_API_CONFIG however is first parsed from YAML / JSON to a Go data structure directly by docker-gen

{{ $ACMESH_DNS_API_CONFIG := fromYaml (coalesce $container.Env.ACMESH_DNS_API_CONFIG "") }}

then rendered as a Bash associative array

{{- if $ACMESH_DNS_API_CONFIG }}
    {{- "\n" }}declare -A ACMESH_{{ $cid }}_DNS_API_CONFIG=(
        {{- range $key, $value := $ACMESH_DNS_API_CONFIG }}
            {{- "\n\t" }}['{{ $key }}']='{{ $value }}'
        {{- end }}
    {{- "\n" }})
{{- end }}

which result in this in /app/letsencrypt_service_data

declare -A ACMESH_containerid_DNS_API_CONFIG=(
    ['DNS_API']='dns_cf'
    ['CF_Token']='<REDACTED>'
    ['CF_Account_ID']='<REDACTED>'
    ['CF_Zone_ID']='<REDACTED>'
)

Writing straight JSON to the standalone config file won't work since it's supposed to have already been parsed, you must follow the same syntax if you wish to use ACMESH_DNS_API_CONFIG in the letsencrypt_user_data file, and the standalone certificates documentation was not updated to reflect that. A doc update PR would be more than welcome, if you're motivated.

stuartbirrell commented 2 months ago

Good shout.

I didnt expect the structure of DNS_API_CONFIG to have to be in a different format when trying to use it in the /app/letsencrypt_user_data file. My solution does have merit here, it will provide a common interface at least with that parameter :smile:

But I agree this is nothing a bit of doco can't mitigate instead of adding in more jq