-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
letsencrypt: Fix and extend Gandi DNS challenge #3581
Conversation
Fix Gandi DNS challenge using an API key. Also add support for token authentication. Fixes: #3383
bashio::log.info "Use Gandi gandi_token" | ||
echo "dns_gandi_token = $(bashio::config 'dns.gandi_token')" >> "/data/dnsapikey" | ||
fi | ||
if bashio::config.exists 'dns.gandi_api_key'; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be an 'else' to avoid strange behavior? (Or an error if both are provided.) It looks like token is preferred according to https://api.gandi.net/docs/authentication/. Alternately maybe these should be reversed to give the token precedence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we could be smarter here, but not sure what the correct smartness is. E.g. the sharing id I think is only valid when an API key is present.
The plug-in just seem to read all of them and forward: https://github.com/obynio/certbot-plugin-gandi/blob/master/certbot_plugin_gandi/main.py#L82-L85. So why not doing it here too 🤷♂️ 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It only forwards one here. Both clauses point to the same target, and if both are set the deprecated one is used because it is last.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean, if all three are set, it forwards all three, at least with this change:
We then just rely on the upstream implementation to pick what it feels right 🤷♂️
Maybe it is easiest if you open a PR what you'd suggest to change, then we can discuss an actual suggestion.
Fix Gandi DNS challenge using an API key. Also add support for token authentication. Fixes: home-assistant#3383
Fix Gandi DNS challenge using an API key. Also add support for token authentication.
Fixes: #3383