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.

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

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

Conversation

stuartbirrell
Copy link
Contributor

@stuartbirrell stuartbirrell commented Sep 26, 2024

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.

adding steps to convert the environment variable ACMESH_${cid}_DNS_API_CONFIG from json to assoc array so it can be properly parsed
@stuartbirrell stuartbirrell changed the title Update letsencrypt_service Bugfix: Standalone DNS_API_CONFIG env variable is not properly formatted to be parsed correctly. Sep 26, 2024
@stuartbirrell stuartbirrell changed the title Bugfix: Standalone DNS_API_CONFIG env variable is not properly formatted to be parsed correctly. Bugfix: Standalone ACMESH_${cid}_DNS_API_CONFIG env variable is not properly formatted to be parsed correctly. Sep 26, 2024
@buchdag
Copy link
Member

buchdag commented Sep 26, 2024

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) ?

@buchdag buchdag added the type/fix PR for a bug fix label Sep 26, 2024
@stuartbirrell
Copy link
Contributor Author

stuartbirrell commented Sep 26, 2024

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
Copy link
Member

buchdag commented Sep 27, 2024

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
Copy link
Contributor Author

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 😄

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants