Executing DNS challenges through a DNS zone delegate (CNAME approach)#6644
Executing DNS challenges through a DNS zone delegate (CNAME approach)#6644adferrand wants to merge 29 commits intocertbot:mainfrom
Conversation
Support a new option --dns-<plugin>-override-challenge to instruct certbot to set up the TXT record for a DNS-01 challenge at an alternate location instead of the default _acme-challenge.<domain>. Independently of this option, this change also incidentally fixes the possibly uncommon use case where _acme-challenge is itself a delegation apex (previously the RFC2136 plugin would generate an update for the wrong zone in that case). Refactor common code across test modules for the various DNS plugins into a new method in dns_test_common.BaseAuthenticatorTest. As a consequence, bump the oldest requirement of all DNS plugins to the latest certbot core. As a consequence, bump their oldest requirement for acme to 0.25.0, as current certbot requires acme.magic_typing.
Also improve testing of challenge overrides using actual argument parser instead of mocked configuration.
Provide a docstring, and explain necessity of using ** syntax.
The zone containing the TXT record to update may be different from the one of the requested certificate domain, in case that record is a CNAME for some other location (in which case the target of the CNAME is specified using --dns-<plugin>-override-challenge).
Previously, the requested certificate domain and the corresponding validation domain name (by default _acme-challenge.<domain>) were passed to DNS plugins. However, if a CNAME is installed at the standard validation location, and the validation domain name is overridden accordingly, plugins must disregard the original domain, and install the challenge TXT record at the overridden domain. To avoid any confusion, pass the validation domain name (not the original domain) as first argument to _perform and _cleanup.
# Conflicts: # certbot-dns-cloudxns/local-oldest-requirements.txt # certbot-dns-dnsimple/local-oldest-requirements.txt # certbot-dns-dnsmadeeasy/local-oldest-requirements.txt # certbot-dns-linode/local-oldest-requirements.txt # certbot-dns-luadns/local-oldest-requirements.txt # certbot-dns-nsone/local-oldest-requirements.txt # certbot-dns-ovh/local-oldest-requirements.txt
|
@adferrand, someone needs to review your changes, but are you happy with the code here? If so, I think we can just review any changes you made and treat the previous code as code reviewed by you. From IRC, this still needs testing on:
|
|
Besides merging master, and correcting a threshold for code coverage, I did not do any functional changes here. So from the code written by @quinot, I am happy with it. However @joohoi was a little concerned by the verbosity of the help for the new cli option. So he may review here. Appart that, yes 3 providers to test left. I will update the PR when tests are done. |
# Conflicts: # certbot-dns-dnsimple/local-oldest-requirements.txt
|
Proposal: cloudxns, gehrin, and linode are all based on Lexicon. The changes that are needed for one of our Lexicon based plugins to support the features added in this PR are extremely small. If the changes made to these three plugins match the ones made to other Lexicon based DNS plugins which we have successfully tested, they should also work, the chance of a bug is little to non-existent, and we should merge this PR. Thoughts? |
|
I have been following this PR as its very needed for a project. And IF they are all Lexicon based, and other Lexicon based DNS plugins are indeed tested, I would agree with bmw that it should be pretty safe to merge. |
|
And as part of the maintainers team for Lexicon, I can positively assert that these providers are fully covered for tests about TXT records creation/deletion, that are required to make functional the relevant Certbot DNS plugins. So I think also that the feature is covered and I am ready to take the responsability to merge it. |
bmw
left a comment
There was a problem hiding this comment.
The changes made by Adrien and to the three plugins we haven't manually tested LGTM.
In addition to the couple of inline threads I started about the UX here, the CHANGELOG needs to be updated.
| 'expand to the domain being requested, and the corresponding ' | ||
| 'standard ACME challenge location. For instance, when ' | ||
| 'requesting "example.com", {domain} is "example.com", and ' | ||
| '{acme} is "_acme-challenge.example.com".') |
There was a problem hiding this comment.
I see the concern about the help text now. Not only is how to use this feature with CNAMEs somewhat complex, but this flag works like --webroot-path and includes a templating system.
I personally think we can't just say something like:
This flag is complex. Go read <url> to learn how to use it.
I think we should try to:
- Provide a succinct but sufficient explanation about what this flag does for an experienced, technical user.
- Maybe provide a link at the bottom to learn more.
@schoen, if you agree with me, are you interested in trying to propose text for (1) here? I think you would do a much better job at this than me.
There was a problem hiding this comment.
It is also what I proposed to @joohoi on the last discussion we had: just put a link to the web documentation in the CLI help, because I do not see how to make a short summary of this feature that would be understandable.
The main concern in the web documentation on which we agree, is that the documentation must not give to users a false feeling of security just because they use a delegate CNAME. There is more to do to build a really secured way that protect DNS credentials, and some work and study need to be done on the users side.
There was a problem hiding this comment.
I think I'd personally rather have long help text about this flag than no help text other than a link. What harm is there in including an explanation of whatever length is necessary?
I think this help text should focus on what the flag does and we can put a link if we want explaining who, what, where, when, why, and how someone might want to use this feature.
Do people disagree? If so, why?
There was a problem hiding this comment.
Oh I misunderstood your last response. You want a description. Still, I do not see how to write something moderately short, as it involves a lot of context to understand what is its usage. Even by limiting to what it is doing technically.
Length can be an issue. Certbot has already a rather long inline help. Making it worse is harmful for usability in my opinion, as it makes the inline help less likely to be read correctly by someone. Basically, the tl;dr is a threat here, argparse (absence of) formatting is not suitable for long descriptions.
|
@adferrand, @joohoi: As noted above, this flag as complex. Have one of the two of you tested various cases on it:
|
897f514 to
d38f6d6
Compare
|
Unfortunately the initial rounds of review were conducted so long time ago, that I honestly cannot remember all the possible corner cases I tested. I did a lot of testing on this however. |
|
Most of the blockers on this PR in my mind are due to how complicated the flag being added is. It's perhaps a bit unfortunate to revisit this decision now, but I wonder if we shouldn't reconsider the idea of automatically figuring out where to set the TXT record by following CNAMEs as was initially done in #5350. Doing this removes the complicated flag completely and avoids duplicating information that is already present in DNS. Is the main concern here adding a new dependency? Are there any other reasons people thing we shouldn't do this? |
|
Regarding the initial review @joohoi made, I think indeed that we can mitigate some points. I think the biggest problem about adding a new dependency is that for OS packages, it requires to add another OS dependency. That said, Concerning the credentials, it is true that finally, the implicit CNAME resolution will make another credential be required despite the "visible" provider of the hostname requested. That said, this feature is an advanced one, and a given user needs to know what it is doing anyway. Also, credentials is already a problem if, for instance, you try to make a certificate composed of hostnames from different provider. On the complexity on itself, well, it is the case anyway. Either with the original implementation, or the current one. At least with the original one, the explanation to give on the flag is easier to sum up. |
|
While we should definitely factor in the non-zero cost of adding a new dependencies (not doing this resulted in the creation of issues like #1301 which was created in response to backlash from some users), it's always a tradeoff and sometimes adding new dependencies is worth it for us. If we're worried about existing OS packages, the thing to check is if new enough versions of the package we want to depend on is present in the various distros where we're packaged and receiving updates. It seems to me like the same credentials problem exists regardless of the choice made here. We can certainly solve the relatively small UX/testing problems in this PR, but quickly revisiting this issue, automatic CNAME resolution seems like a better approach to me. It results in less things to explain so docs and I'm curious what other people think though. |
# Conflicts: # certbot/plugins/dns_common_test.py
# Conflicts: # certbot-dns-cloudflare/local-oldest-requirements.txt # certbot-dns-cloudxns/local-oldest-requirements.txt # certbot-dns-digitalocean/local-oldest-requirements.txt # certbot-dns-dnsimple/local-oldest-requirements.txt # certbot-dns-dnsmadeeasy/local-oldest-requirements.txt # certbot-dns-gehirn/local-oldest-requirements.txt # certbot-dns-google/local-oldest-requirements.txt # certbot-dns-linode/local-oldest-requirements.txt # certbot-dns-luadns/local-oldest-requirements.txt # certbot-dns-nsone/local-oldest-requirements.txt # certbot-dns-ovh/local-oldest-requirements.txt # certbot-dns-rfc2136/local-oldest-requirements.txt # certbot-dns-route53/local-oldest-requirements.txt # certbot-dns-sakuracloud/local-oldest-requirements.txt # certbot/plugins/dns_test_common.py
|
Yes, automatic CNAME resolution is a better approach and no need for duplication of information already in DNS. |
|
Any updates here? |
|
Will this PR be ever finished and merged? |
|
I am not sure it will. We are currently working on #7244 that proposes basically a superior solution because it is automated, and is able to resolve more cases. However, it is for rfc2136 plugin only. Once it is merged, I would like to make it generic for all dns plugins, if possible. |
# Conflicts: # certbot-dns-cloudflare/local-oldest-requirements.txt # certbot-dns-cloudxns/local-oldest-requirements.txt # certbot-dns-cloudxns/tests/dns_cloudxns_test.py # certbot-dns-digitalocean/local-oldest-requirements.txt # certbot-dns-dnsimple/local-oldest-requirements.txt # certbot-dns-dnsmadeeasy/local-oldest-requirements.txt # certbot-dns-gehirn/local-oldest-requirements.txt # certbot-dns-google/local-oldest-requirements.txt # certbot-dns-linode/local-oldest-requirements.txt # certbot-dns-luadns/local-oldest-requirements.txt # certbot-dns-nsone/local-oldest-requirements.txt # certbot-dns-ovh/local-oldest-requirements.txt # certbot-dns-rfc2136/local-oldest-requirements.txt # certbot-dns-route53/local-oldest-requirements.txt # certbot-dns-sakuracloud/local-oldest-requirements.txt # certbot/certbot/plugins/dns_test_common.py # certbot/cli.py # certbot/tests/plugins/dns_common_test.py
c4374b8 to
b3b9da3
Compare
This PR integrates the work of @quinot from #5350, and updates it towards current master. Use case solved by this PR is described in #6566.
The state of PR #5350 was a code review validated by @joohoi, except end-to-end tests for Google DNS plugin, and potentially a refactoring of the text help for
--dns-*-override-challenge.On top of that, I confirm from live tests that the Google DNS plugin works as expected. Text help seems complete for me, so I let reviewers to propose modifications if it seems to be necessary.
Fixes #6566.
cc @bmw @joohoi @quinot