Fix DuckDNS Lets Encrypt certificate creation/renewal failing with "Incorrect TXT record" error #2662
Conversation
This fixes the problem where the output included a newline just before all the aliases were listed
This adds a bit more context to the OK lines seen in the output
Fixes home-assistant#2505. This includes a bit of logic already performed by dehydrated, however we need it to ensure we don't unnecessarily cause certificates to be reissued everytime we check.
Having null results in an empty domain automatically added when first configuring the addon
|
can this be merged? |
|
@homeassistant merge 👍 |
|
ping for merge... |
|
I'm not a member of this project, I have no authority to merge, I just threw my review into the mix because I had the same issue this will solve and looked at the code and it seemed sound to my eyes. I was also waiting on a merge up until last month when I moved away from duckdns as a solution and managed my certificates using the core_letsencrypt addon which had support for my new registrar I've recently migrated to. |
|
ah. my apologies. |
|
Will it merge... |
|
@SimplyLinn Thanks for approving the PR. However this is still not merged. Do you know who to contact to make this happen? |
|
@diamant-x One if the top contributors would be my best guess, I am a user of HA, not a dev |
|
I have the same problem. |
|
Hi @pvizeli, Could this passed and signed PR be merged to solve a multi-year long standing issue with DuckDNS official Addon? |
|
This is ridiculous! Dealing with a Home Assistant outage every couple of months just because of this! Please merge into master and release before I lose my mind! |
| domains: | ||
| - null | ||
| domains: [] |
There was a problem hiding this comment.
Actually this has to stay this way for now. The reason is because setting something to null is the only way the addon currently has to tell users they must put their own value here. If you install the addon and immediately try to click start you see this right now:
If you remove the null and just set it to empty array then it won't do this anymore. It'll just allow the user to save and start after they fill in the token.
What's really needed is home-assistant/supervisor#2640 but until that's done this has to stay as is.
There was a problem hiding this comment.
🤔 I'm sure this worked when I initially implemented this waaaay back in Sep last year, but it's possible I'm mis-remembering... it has been a long time. Thanks for pointing this out.
| if [ -n "${issue_cert:-}" ]; then | ||
| # Order is important as we want the duckdns.org domain to be first | ||
| for domain in $(echo "${domains}" "${uniq_sorted_aliases}" | tr ' ' '\n' | uniq | tr '\n' ' '); do | ||
| domain_args+=("--domain" "${domain}") | ||
| dehydrated --cron --algo "${ALGO}" --hook ./hooks.sh --challenge dns-01 "${domain_args[@]}" --out "${CERT_DIR}" --config "${WORK_DIR}/config" || true | ||
| done | ||
| else | ||
| bashio::log.info "Certificate still valid. Skipping renew!" | ||
| fi |
There was a problem hiding this comment.
The problem with this strategy is you are overwriting the cert each time. I tested your branch and it works with this config:
domains:
- x.duckdns.org
aliases:
- domain: x.my-domain.com
alias: x.duckdns.orgBut not with this config:
domains:
- x.duckdns.org
aliases:
- domain: x.my-domain.com
alias: x.duckdns.org
- domain: x.my-other-domain.com
alias: x.duckdns.orgWith this config what seems to happen is it does successfully get two certificates but you only end up with one certificate at the filepath specified with the names x.duckdns.org x.my-other-domain.com. The cert that was valid for x.duckdns.org x.my-domain.com is overwritten and lost.
Which is actually a big problem because then every 300 seconds (or whatever the user changed that value to) it sees the names on the cert does not match the name and all the aliases specified. So it gets two new certificates again, all but ensuring you'll hit the rate limit of 5 identical requests per 168 hours.
TBH I don't really know how to handle this with the dns-01 challenge. You are correct, this addon does not work right now with anything in aliases for the reasons you laid out in the comment and this PR. But this change breaks the multiple alias situation even worse. We might need to consider using an alternate challenge if there are multiple aliases. Or removing the ability to specify more then one alias.
I should note that I also tested multiple domains each with an alias and that didn't work at all. Both the addon as is and after the PR resulted in a 403 error in the logs.
There was a problem hiding this comment.
One thing I noticed while playing with the add-on was that the format for the aliases isn't what you might think it would be. For multiple domains, try
domains:
- x.duckdns.org
aliases:
- domain: x.my-domain.com x.my-other-domain.com
alias: x.duckdns.org
and see if it works better for you. At least that worked for me for two domains. I tried a third domain as well, but that got the "Incorrect TXT record" error.
For some reason, it also seems to sort all the domains and aliases alphabetically when it creates the certificate so when I look at the certificate details, the Alternative Name field has all of the domains and aliases listed alphabetically and whichever is the first in that list is also listed as the common name. It doesn't matter what order I listed them in the YAML. I don't know if that matters in any way, but I thought it was strange.
There was a problem hiding this comment.
TBH I don't really know how to handle this with the dns-01 challenge. You are correct, this addon does not work right now with anything in aliases for the reasons you laid out in the comment and this PR. But this change breaks the multiple alias situation even worse. We might need to consider using an alternate challenge if there are multiple aliases. Or removing the ability to specify more then one alias.
I follow your reasoning and believe you are right. This PR would help some users, and make it even worse for other users.
Bash scripting is great, but I am starting to get the feeling that the operations which would have to take place here, are becoming a bit too complex for the programming language used. Sure you can do everything with Bash as well... but sometimes other languages are more appropriate and benefit to logic implementation, readability and flow.
Maybe it is time to port to Python?
There was a problem hiding this comment.
@mdegat01 @sbussinger and @thomasgeens thanks for finally taking a look at this and providing feedback. 🙇
The problem with this strategy is you are overwriting the cert each time.
Correct. There is no other way around this because of the limitation with DuckDNS this PR is trying to workaround.
With this config what seems to happen is it does successfully get two certificates but you only end up with one certificate at the filepath specified with the names
x.duckdns.org x.my-other-domain.com. The cert that was valid forx.duckdns.org x.my-domain.comis overwritten and lost.
Getting only one certificate at the end is expected. Having only two of the three alt names within it isn't. I'm pretty sure I tested this, but it's possible I'm mis-remembering as I've since moved onto a slightly different implementation which uses a different configuration method like this (I think the current configuration is unnecessarily confusing):
domains:
- x.duckdns.org
aliases:
- x.my-domain.com
- x.my-other-domain.comIt uses an almost identical method that is being suggested here which works a treat. I didn't include those changes in this PR as it's a breaking change in the configuration and I couldn't find a documented stance on how to approach breaking changes in addons at the time.
Which is actually a big problem because then every 300 seconds (or whatever the user changed that value to) it sees the names on the cert does not match the name and all the aliases specified. So it gets two new certificates again, all but ensuring you'll hit the rate limit of 5 identical requests per 168 hours.
Yeah, it shouldn't if all the names were successfully contained in the final certificate... ie if the problem you mentioned didn't happen.
But this change breaks the multiple alias situation even worse.
It's possible this is why I opted for the change in my other fork, but I can't remember.
We might need to consider using an alternate challenge if there are multiple aliases. Or removing the ability to specify more then one alias.
Yeah, I think this is definitely worth considering, but once again this would be a breaking change in admittedly broken functionality and I still don't know what the stance is with regards to handling breaking changes.
For some reason, it also seems to sort all the domains and aliases alphabetically when it creates the certificate
Yes, this was deliberate to simplify the checking of the certificate against alt names in the configuration. The order doesn't matter in the grand scheme of things.
Maybe it is time to port to Python?
I don't think changing the code this is written in is going to help. We're still limited by DuckDNS so will still need to parse the config and make multiple requests to DuckDNS and LetsEncrypt.
If someone has a link to an official stance on how to introduce breaking changes in addons, I'm happy to close this PR and instead submit a PR which introduces all the changes in my other fork which includes a simplified configuration and support for ZeroSSL and the LetsEncrypt staging environment and a few other changes.
There was a problem hiding this comment.
domains:
- x.duckdns.org
aliases:
- x.my-domain.com
- x.my-other-domain.comThe reason the config is as it is currently is because domains can contain a list of duckdns domains. And then for each listed domain you can provide one or more aliases. With this suggested config you're dropping that.
After talking it over with other HA developers I think we've come to the conclusion that the aliases option itself was a mistake. It really overcomplicates this addon for most users that simply want a DuckDNS domain. And for those that do own their own domain on the side there's still really no reason they have to use aliases. Instead they can just do this:
- Set up the existing duckdns integration in HA to keep their duckdns domain up to date with their IP address
- CNAME one or more subdomains of their personal domain to their DuckDNS domain (already a mandatory step for using aliases in this addon)
- Use the Let's Encrypt addon to get a certificate for the subdomain(s) of their personal domain they use.
Then they have their working https setup at https://my-domain.personal.com and HA is keeping their IP address up to date. Everything working.
Fwiw I just tested and the existing addon has no issues with multiple DuckDNS domains being listed, it is only if aliases are listed that it breaks. So if aliases is deprecated and removed then all existing features of the addon work fine.
There was a problem hiding this comment.
The reason the config is as it is currently is because
domainscan contain a list of duckdns domains. And then for each listed domain you can provide one or more aliases. With this suggested config you're dropping that.
Doh! I didn't consider using multiple DuckDNS domains. 🤦
After talking it over with other HA developers I think we've come to the conclusion that the aliases option itself was a mistake. It really overcomplicates this addon for most users that simply want a DuckDNS domain.
[...]
So ifaliasesis deprecated and removed then all existing features of the addon work fine.
🆒 Makes sense to me. I'm happy to do this (in another PR), however it would be a breaking change.
How are these situations announced and handled?
Would it be sufficient to remove the functionality, change the config, and add a notice to the README.md and updating the DOCS.md to remove references to the aliasing and maybe add a section proposing using the Lets Encrypt addon you've suggested for those that want to keep this behaviour?
There was a problem hiding this comment.
Would it be sufficient to remove the functionality, change the config, and add a notice to the README.md and updating the DOCS.md to remove references to the aliasing and maybe add a section proposing using the Lets Encrypt addon you've suggested for those that want to keep this behaviour?
Yes to removing the info from the docs. Don't need to update the README.md, just note it in the changelog. The blurb in the changelog for the current version is shown to the user as update notes so then they'll see it prior to update. That's also where I would put migration info so users see what they need to do prior to update. Also bump the version to a new major version (so 2.0.0 in this case)
If you want to make the PR here that would be greatly appreciated. Just @ me and I'll review and we can work on the changlog notes. 👍
There was a problem hiding this comment.
The reason the config is as it is currently is because
domainscan contain a list of duckdns domains. And then for each listed domain you can provide one or more aliases. With this suggested config you're dropping that.
[...]
Fwiw I just tested and the existing addon has no issues with multiple DuckDNS domains being listed, it is only ifaliasesare listed that it breaks. So ifaliasesis deprecated and removed then all existing features of the addon work fine.
Hi,
Can you share an example where a single instance of this addons can keep up to date different duckdns domains, each with an alias to another personal domain?
In that case, the addon should output more than 1 pair of privkey+certs if I'm not mistaken?
Thanks!
|
|
||
| domains=$(bashio::config 'domains') | ||
|
|
||
| # Prepare domain for Let's Encrypt | ||
| for domain in ${domains}; do | ||
| for alias in $(jq --raw-output --exit-status "[.aliases[]|{(.alias):.domain}]|add.\"${domain}\" | select(. != null)" /data/options.json) ; do | ||
| aliases="${aliases} ${alias}" | ||
| aliases+=("${alias}") |
There was a problem hiding this comment.
Can we make it work, if we will collect aliases separately for each domain?
So, we can move inside for-loop querying for aliases and sorting them:
function le_renew() {
local domain_args=()
local domains=''
local aliases=''
domains=$(bashio::config 'domains')
# Prepare domain for Let's Encrypt
for domain in ${domains}; do
for alias in $(jq --raw-output --exit-status "[.aliases[]|select(.alias == \"${domain}\")|.domain+\" \"]|add | select(. != null)" /data/options.json) ; do
aliases+=("${alias}")
done
uniq_sorted_aliases="$(echo "${aliases[@]}" | tr ' ' '\n' | sort | uniq | tr '\n' ' ')"
bashio::log.info "Renew certificate for domain: ${domain} and aliases: $(echo -n "${uniq_sorted_aliases}")"
cert="${CERT_DIR}/${domain}/cert.pem"
if [ -f "${cert}" ]; then
bashio::log.info "Checking existing cert..."
certnames="$(openssl x509 -in "${cert}" -text -noout | grep DNS: | sed 's/DNS://g' | tr -d ' ' | tr ',' '\n' | sort | tr '\n' ' ' | sed 's/ $//')"
givennames="$(echo "${domain}" "${uniq_sorted_aliases}"| tr ' ' '\n' | sort | tr '\n' ' ' | sed 's/ $//' | sed 's/^ //')"
if [ "${certnames}" != "${givennames}" ]; then
bashio::log.info "Certificate names do not match, requesting new certificate."
issue_cert=1
fi
if ! (openssl x509 -checkend $((30 * 86400)) -noout -in "${cert}" > /dev/null 2>&1); then
bashio::log.info "Certificate is due for renewal, auto-renewing."
issue_cert=1
fi
else
bashio::log.info "No certificate found for ${domain}, requesting new certificate."
issue_cert=1
fi
if [ -n "${issue_cert:-}" ]; then
# Order is important as we want the duckdns.org domain to be first
for domain in $(echo "${domain}" "${uniq_sorted_aliases}" | tr ' ' '\n' | uniq | tr '\n' ' '); do
domain_args+=("--domain" "${domain}")
dehydrated --cron --algo "${ALGO}" --hook ./hooks.sh --challenge dns-01 "${domain_args[@]}" --out "${CERT_DIR}" --config "${WORK_DIR}/config" || true
done
else
bashio::log.info "Certificate still valid. Skipping renew!"
fi
done
LE_UPDATE="$(date +%s)"
}I don't have enough knowledge yet to test everything, but looks like it should works when I look for separate pars.
As detailed in #2505, the DuckDNS extension will fail to issue or renew a certificate and fail with the error "Incorrect TXT record"
As I've detailed in the issue #2505 (comment):
This PR fixes that by requesting the certificates sequentially for each of the aliases. This isn't ideal, but it does the trick and should be fine for most users. Users with a lot of domains or aliases might need to try a few times because of Lets Encrypt's rate limits, but I suspect there won't be many people hitting these.
Whilst I'm at it, I've also made the
OKlines in the output clearer by adding more context to what the hooks are actually doing so the output now looks like this:And a renew attempt for an already valid certificate will look like this:
I've also fixed an annoyance where first configuring the extension would always have an empty invalid domain name added.
Fixes #2505