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

duckdns: Support wildcard certs for subdomains in DuckDNS addon (resolves #336) #3244

Closed
wants to merge 1 commit into from

Conversation

kyle-cackett
Copy link

This PR resolves the issue described in this comment on issue #336 (copied below for convenience) which occurs when the domains array contains entries of the form "*.xxx.duckdns.org > xxx.duckdns.org". This type of entry is used to obtain wildcard certificates from Let's Encrypt / dehydrated.

There's a quick workaround if you only need to use subdomains of "xxx.duckdns.org". Configure the addon with:
"domains": [
".xxx.duckdns.org > xxx.duckdns.org"
]
This will create a wildcard certificate for "
.xxx.duckdns.org" with "xxx.duckdns.org" as an alias (required by dehydrated). The certificate will not be valid for "xxx.duckdns.org", but only for its subdomains.

It doesn't work propertly. It generate wildcard certificate but doesn't update IP on DuckDNS.org.
In log only empty warnings:
[18:52:58] WARNING:

The sed substitution 's/[^,]*[ > ]//g' matches all occurrences of strings which do not contain a , (so that after the join(",") at most one match is found per entry in the domain array) that end with the characters > and removes them leaving only valid domains according to the DuckDNS API. Note that this transformation will not impact Let's Encrypt because the $DOMAINS variable is only used by DuckDNS. The le_renew function grabs the domains from the config directly on line 23:

https://github.com/home-assistant/addons/blob/72fac56dae57fe3573a1e568b248fa304cca2b1b/duckdns/data/run.sh#L23C1-L24C1

Testing

  1. Wrote the following script to test the sed substitution:
#!/usr/bin/with-contenv bashio

TEST_DOMAINS=$(bashio::config 'test_domains | join(",")' | sed 's/[^,]*[ > ]//g')
echo $TEST_DOMAINS

Added the following field to the json object in /tmp/.bashio/addons.self.options.config.cache

"test_domains": ["*.example.duckdns.org > example.duckdns.org","test.duckdns.org","*.sample.duckdns.org > sample.duckdns.org","subdomain.duckdns.org","demo.duckdns.org"],

Ran the script and received the following output:

example.duckdns.org,test.duckdns.org,sample.duckdns.org,subdomain.duckdns.org,demo.duckdns.org
  1. Modified my IP address on duckdns.org and ran the new version of this run.sh from within the add-on. IP was correctly reset to my actual IP.

Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @kyle-cackett

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@home-assistant
Copy link

home-assistant bot commented Oct 5, 2023

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@home-assistant home-assistant bot marked this pull request as draft October 5, 2023 02:34
@kyle-cackett kyle-cackett marked this pull request as ready for review October 5, 2023 02:36
@kyle-cackett kyle-cackett changed the title Support wildcard certs for subdomains in DuckDNS addon (resolves #336) duckdns: Support wildcard certs for subdomains in DuckDNS addon (resolves #336) Oct 5, 2023
@agners
Copy link
Member

agners commented Nov 24, 2023

Is this still required since #3152 added DuckDNS support in the Let's Encrypt add-on?

From what I can see from #2662 (comment) the preferred way is to use Let's Encrypt for these type of situation 🤔

@kyle-cackett
Copy link
Author

kyle-cackett commented Nov 24, 2023

Looks like #3152 was merged ~30 minutes ago so I haven't yet had a chance to evaluate whether switching add-ons and using the latest build of Let's Encrypt would be an alternative solution to the current limitation in the DuckDNS add-on described in this PR.

Is this still required since #3152 added DuckDNS support in the Let's Encrypt add-on?

Following this line of questioning, if DuckDNS support is now included in the Let's Encrypt add-on does that make the DuckDNS add-on redundant? If so perhaps the DuckDNS add-on should be deprecated and eventually discontinued entirely and users should be migrated to Let's Encrypt.

If the DuckDNS plugin is here to stay, including the fix proposed in this PR would be nice since based on #336 some people are using this configuration which is leading to some frustration.

Unrelated note: this fix could also be implemented without sed by using bash parameter expansion. E.g. ${DOMAINS//[^,]*[ > ]/}

@vdemidov
Copy link

vdemidov commented Nov 24, 2023

> Is this still required since #3152 added DuckDNS support in the Let's Encrypt add-on?

I'm not sure. Will Let's Encrypt add-on support domains like *.xxx.duckdns.org? I will check after Let's Encrypt add-on release.
Anyway I will prefer one DuckDNS add-on instead two addons.

@iranl
Copy link
Contributor

iranl commented Nov 24, 2023

dehydrated-io/dehydrated#554 and the dehydrated troubleshooting guide both specifically mention that API/DNS implementations such as employed by DuckDNS are considered broken and are not properly supported by dehyrated.

As such I consider the Let's encrypt implementation using dehydrated in the DuckDNS addon inherently broken and agree with @mdegat01 in #2662 (comment) that support for aliases (including custom domains and wildcard DuckDNS domains) should be removed from the DuckDNS addon.

I would even go one further and remove SSL support from the DuckDNS addon entirely and use the addon only for keeping a dynamic IP up to date, but maybe it's more sensible to keep the implementation only for the most basic of use cases (only for xxx.duckdns.org certs).

I created #3152 in an effort to move DuckDNS DNS challenges to the Let's Encrypt addon as certbot and the certbot_duckdns plugin properly support the (bad) API implementation at DuckDNS. It should support both domains such as (*.)xxx.duckdns.org and (*.)custom.com

Now that #3152 is merged maybe it's time to close this and reopen #2964?

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

Successfully merging this pull request may close these issues.

4 participants