Add dns-01 challenge support to the ACME client #2061
15 participants
alex
and 2 others
commented on an outdated diff
| + return False | ||
| + | ||
| + validation_domain_name = chall.validation_domain_name(domain) | ||
| + validation = chall.validation(account_public_key) | ||
| + logger.debug("Verifying %s at %s...", chall.typ, validation_domain_name) | ||
| + try: | ||
| + dns_response = dns.resolver.query(validation_domain_name, 'TXT') | ||
| + txt_records = sum([rdata.strings for rdata in dns_response], []) | ||
| + except dns.exception.DNSException as error: | ||
| + logger.error("Unable to resolve %s: %s", validation_domain_name, | ||
| + error) | ||
| + return False | ||
| + | ||
| + for txt_record in txt_records: | ||
| + if txt_record == validation: | ||
| + return True |
|
I think this code can be simplified to: try:
dns_response = dns.resolver.query(validation_domain_name, 'TXT')
except dns.exception.DNSException as error:
logger.error(...)
return False
for rdata in dns_response:
for txt_record in rdata.strings:
if txt_record == validation:
return TrueI factored out the dns query, I'd rather contain the dnspython objects as much as I can. And I think it gives some extra structure that helps. Agreed about containing exists = validation in txt_records
if not exists:
logger.debug(...)
return exists
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
alex
and 2 others
commented on an outdated diff
| + | ||
| + LABEL = "_acme-challenge" | ||
| + """Label clients prepend to the domain name being validated.""" | ||
| + | ||
| + # FIXME: Remove extra parameter once #2052 is integrated | ||
| + def validation(self, account_key, dns01_hexdigit_response=True, **unused_kwargs): | ||
| + """Generate validation. | ||
| + | ||
| + :param JWK account_key: | ||
| + :rtype: unicode | ||
| + | ||
| + """ | ||
| + key_authorization = self.key_authorization(account_key) | ||
| + if dns01_hexdigit_response: | ||
| + return hashlib.sha256(key_authorization).hexdigest() | ||
| + return base64.urlsafe_b64encode(hashlib.sha256(key_authorization).digest()) |
|
On reading the discussion on letsencrypt/boulder#1295 I think this maybe should be I think it makes sense to use one common encoding, but the spec needs to be updated for that (the last drat I looked at did not even specify urlsafe base64). The discussion cites passages related to JSON, but this is not a JSON based field so that does not clarify the issue. It is an implementation detail that the server does a byte-to-byte comparison of the encoded values, so it is not necessary (although preferred) that the DNS record and the JSON objects use identical encoding. @wteiken See this issue in the acme draft repository ietf-wg-acme/acme#64
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
Not sure if it's a bug here or a bug in boulder or a bug in my code, but running https://github.com/alex/letsencrypt-aws/ I pass simple_verify but get an error back from the staging server.
alex
and 1 other
commented on an outdated diff
| + performed! | ||
| + :param JWK account_public_key: | ||
| + | ||
| + :returns: ``True`` iff validation is successful, ``False`` | ||
| + otherwise. | ||
| + :rtype: bool | ||
| + | ||
| + """ | ||
| + if not self.verify(chall, account_public_key): | ||
| + logger.debug("Verification of key authorization in response failed") | ||
| + return False | ||
| + | ||
| + validation_domain_name = chall.validation_domain_name(domain) | ||
| + validation = chall.validation(account_public_key) | ||
| + logger.debug("Verifying %s at %s...", chall.typ, validation_domain_name) | ||
| + for txt_record in self.txt_records_for_domain(validation_domain_name): |
|
If there's an error in DNS querying now it'll fail with a
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
alex
commented on an outdated diff
| + :param JWK account_public_key: | ||
| + | ||
| + :returns: ``True`` iff validation is successful, ``False`` | ||
| + otherwise. | ||
| + :rtype: bool | ||
| + | ||
| + """ | ||
| + if not self.verify(chall, account_public_key): | ||
| + logger.debug("Verification of key authorization in response failed") | ||
| + return False | ||
| + | ||
| + validation_domain_name = chall.validation_domain_name(domain) | ||
| + validation = chall.validation(account_public_key) | ||
| + logger.debug("Verifying %s at %s...", chall.typ, validation_domain_name) | ||
| + txt_records = self.txt_records_for_name(validation_domain_name) | ||
| + if txt_records == None: |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
onovy
and 1 other
commented on an outdated diff
| + except dns.exception.DNSException as error: | ||
| + logger.error("Unable to resolve %s: %s", name, error) | ||
| + return None | ||
| + return sum([rdata.strings for rdata in dns_response], []) | ||
| + | ||
| + def simple_verify(self, chall, domain, account_public_key): | ||
| + """Simple verify. | ||
| + | ||
| + :param challenges.DNS01 chall: Corresponding challenge. | ||
| + :param unicode domain: Domain name being verified. | ||
| + :param account_public_key: Public key for the key pair | ||
| + being authorized. If ``None`` key verification is not | ||
| + performed! | ||
| + :param JWK account_public_key: | ||
| + | ||
| + :returns: ``True`` iff validation is successful, ``False`` |
|
I'd rather remove the superfluous "False otherwise" part. Same in rest of doc.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
alex
and 2 others
commented on an outdated diff
| + typ = response_cls.typ | ||
| + | ||
| + LABEL = "_acme-challenge" | ||
| + """Label clients prepend to the domain name being validated.""" | ||
| + | ||
| + # FIXME: Remove extra parameter once #2052 is integrated | ||
| + def validation(self, account_key, dns01_hexdigit_response=True, **unused_kwargs): | ||
| + """Generate validation. | ||
| + | ||
| + :param JWK account_key: | ||
| + :rtype: unicode | ||
| + | ||
| + """ | ||
| + key_authorization = self.key_authorization(account_key) | ||
| + if dns01_hexdigit_response: | ||
| + return hashlib.sha256(key_authorization).hexdigest() |
|
The code was landed in Very much agreed that we remove that fallback! Boulder will be fixed Real Soon Now and abiding by Postel's Law is a quick way to build insecure systems. There's also a ticket to clarify the spec on this matter. Yes, it was only in there in case a server gets released with the old implementation. I will remove the various references (and replace the encoding with jose.b64encode). (To be clear, the DNS challenge in boulder is fixed in master and will soon be deployed to staging.)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
kuba
and 1 other
commented on an outdated diff
| @@ -12,6 +12,7 @@ | ||
| 'cryptography>=0.8', | ||
| # Connection.set_tlsext_host_name (>=0.13), X509Req.get_extensions (>=0.15) | ||
| 'PyOpenSSL>=0.15', | ||
| + 'dnspython', |
|
Unfortunately, this adds additional dependency to the project. Moreover, some of the clients using this library (e.g. https://github.com/kuba/simp_le) will never use DNS challenge. Unless there are some technical arguments against, I would strongly prefer to move I'll have a look at that (new to python). I see your point, but I feel it's a little odd to have something covered by the core standard require an "extras" library. And given the core standard includes DNS queries some DNS library dependency is to be expected. Basic DNS queries can by performed by stdlib. On the other hand, official client (rightfully) gets a lot of critique due to too many dependencies (see #1301). :(
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
kuba
and 1 other
commented on an outdated diff
| @@ -1,5 +1,7 @@ | ||
| """ACME Identifier Validation Challenges.""" | ||
| import abc | ||
| +import dns.resolver | ||
| +import dns.exception |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
kuba
and 1 other
commented on an outdated diff
| + | ||
| + def txt_records_for_name(self, name): | ||
| + """Resolve the name and return the TXT records. | ||
| + | ||
| + :param unicode name: Domain name being verified. | ||
| + | ||
| + :returns: A list of txt records, or None if the name could not be resolved | ||
| + :rtype: list of unicode | ||
| + | ||
| + """ | ||
| + try: | ||
| + dns_response = dns.resolver.query(name, 'TXT') | ||
| + except dns.exception.DNSException as error: | ||
| + logger.error("Unable to resolve %s: %s", name, error) | ||
| + return None | ||
| + return sum([rdata.strings for rdata in dns_response], []) |
|
Why do you use this is a list of lists (multiple rdata objects with possibly multiple strings), so this flattens that structure. Please use
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
kuba
and 1 other
commented on an outdated diff
| @@ -216,6 +218,90 @@ def response_and_validation(self, account_key, *args, **kwargs): | ||
| @ChallengeResponse.register | ||
| +class DNS01Response(KeyAuthorizationChallengeResponse): | ||
| + """ACME "dns-01" challenge response.""" | ||
| + typ = "dns-01" | ||
| + | ||
| + def txt_records_for_name(self, name): |
|
@pde I still feel like a robot :( Yep, it passes the tests. Will change and move it out of the class.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
kuba
and 1 other
commented on an outdated diff
| + typ = "dns-01" | ||
| + | ||
| + def txt_records_for_name(self, name): | ||
| + """Resolve the name and return the TXT records. | ||
| + | ||
| + :param unicode name: Domain name being verified. | ||
| + | ||
| + :returns: A list of txt records, or None if the name could not be resolved | ||
| + :rtype: list of unicode | ||
| + | ||
| + """ | ||
| + try: | ||
| + dns_response = dns.resolver.query(name, 'TXT') | ||
| + except dns.exception.DNSException as error: | ||
| + logger.error("Unable to resolve %s: %s", name, error) | ||
| + return None |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
kuba
commented on an outdated diff
| + :param JWK account_public_key: | ||
| + | ||
| + :returns: ``True`` iff validation with the TXT records resolved from a | ||
| + DNS server is successful. | ||
| + :rtype: bool | ||
| + | ||
| + """ | ||
| + if not self.verify(chall, account_public_key): | ||
| + logger.debug("Verification of key authorization in response failed") | ||
| + return False | ||
| + | ||
| + validation_domain_name = chall.validation_domain_name(domain) | ||
| + validation = chall.validation(account_public_key) | ||
| + logger.debug("Verifying %s at %s...", chall.typ, validation_domain_name) | ||
| + txt_records = self.txt_records_for_name(validation_domain_name) | ||
| + if txt_records is None: |
|
I would drop this test in favour of returning
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
kuba
commented on an outdated diff
| + """ | ||
| + try: | ||
| + dns_response = dns.resolver.query(name, 'TXT') | ||
| + except dns.exception.DNSException as error: | ||
| + logger.error("Unable to resolve %s: %s", name, error) | ||
| + return None | ||
| + return sum([rdata.strings for rdata in dns_response], []) | ||
| + | ||
| + def simple_verify(self, chall, domain, account_public_key): | ||
| + """Simple verify. | ||
| + | ||
| + :param challenges.DNS01 chall: Corresponding challenge. | ||
| + :param unicode domain: Domain name being verified. | ||
| + :param account_public_key: Public key for the key pair | ||
| + being authorized. If ``None`` key verification is not | ||
| + performed! |
|
Comment about
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
kuba
and 1 other
commented on an outdated diff
| + txt_records = self.txt_records_for_name(validation_domain_name) | ||
| + if txt_records is None: | ||
| + return False | ||
| + | ||
| + for txt_record in txt_records: | ||
| + if txt_record == validation: | ||
| + return True | ||
| + | ||
| + logger.debug("Key authorization from response (%r) doesn't match any " | ||
| + "DNS response in %r", self.key_authorization, txt_records) | ||
| + return False | ||
| + | ||
| +@Challenge.register # pylint: disable=too-many-ancestors | ||
| +class DNS01(KeyAuthorizationChallenge): | ||
| + """ACME "dns-01" challenge.""" | ||
| + |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
kuba
commented on an outdated diff
| + | ||
| + response_cls = DNS01Response | ||
| + typ = response_cls.typ | ||
| + | ||
| + LABEL = "_acme-challenge" | ||
| + """Label clients prepend to the domain name being validated.""" | ||
| + | ||
| + def validation(self, account_key, **unused_kwargs): | ||
| + """Generate validation. | ||
| + | ||
| + :param JWK account_key: | ||
| + :rtype: unicode | ||
| + | ||
| + """ | ||
| + key_authorization = self.key_authorization(account_key) | ||
| + return jose.b64encode(hashlib.sha256(key_authorization).digest()) |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
kuba
and 1 other
commented on an outdated diff
| @@ -76,6 +77,113 @@ def test_verify_wrong_form(self): | ||
| key_authorization='.foo.oKGqedy-b-acd5eoybm2f-NVFxvyOoET5CNy3xnv8WY') | ||
| self.assertFalse(response.verify(self.chall, KEY.public_key())) | ||
| +class DNS01ResponseTest(unittest.TestCase): |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
kuba
and 1 other
commented on an outdated diff
| + self.chall.validation_domain_name("local"), ["!"]) | ||
| + self.assertFalse(self.response.simple_verify( | ||
| + self.chall, "local", KEY.public_key())) | ||
| + | ||
| + @mock.patch("acme.challenges.dns.resolver.query") | ||
| + def test_simple_verify_connection_error(self, mock_dns): | ||
| + mock_dns.side_effect = dns.exception.DNSException | ||
| + self.assertFalse(self.response.simple_verify( | ||
| + self.chall, "local", KEY.public_key())) | ||
| + | ||
| +class DNS01Test(unittest.TestCase): | ||
| + | ||
| + def setUp(self): | ||
| + from acme.challenges import DNS01 | ||
| + self.msg = DNS01( | ||
| + token=jose.decode_b64jose( |
|
Forgot to push commits? To be clear, I meant: Hah, I accidentally fixed another line instead that was similar. Will be fixed in the next merge.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
Nice job, thanks! :) Couple of nits to fix, couple of idiomatic improvements and possibly-tricky-to-implement extras_require approach of declaring dnspython dependency before next round of review.
Successfully tested the new library against the "Boulder=( +34c1b83 Tue Jan 5 22:16:57 UTC 2016)" staging server.
I have made some progress isolating dnspython, it is now in a separate module which means challenges and challenges_test have no direct dependency on dnspython anymore (in fact, only calling simple_verify will have any issues without it). I moved the dependency to extras_require (via testing_extras).
The functional tests on travis-ci succeed, but lint and coverage fail (as they don't install the extras_require for 'testing'), see https://travis-ci.org/wteiken/letsencrypt/builds/100522564. So problem one will be to make these two get the right dependency.
Another question is how the tests for developers running the tests locally, both as part of the client install and as part of using the ACME client e.g., via PIP. I don't know enough about that part to make any predictions...
It seems that you haven't pushed commits with isolated dnspython. Intended?
For Travis, you will need to update tox.ini to call pip install -e acme[dns,testing] instead of just pip install -e acme[testing].
Yeah, we should have some kind of integration testing for the DNS challenge. All the other tests are at the core client (letsencrypt) level, rather than acme library (this might change in future, in order to get acme library separated from this repo, cf. #1958), so I just assumed you were going to create a follow-up PR with new DNS IPlugin and integration tests including deployment of local temporary DNS server.
Yes, I am trying to have it pass checks first :-) I assume otherwise the chatter will be a lot higher for you.
I do the travis-cl tests from my fork, once that is done I will merge the change to the branch linked to the PR.
K, this should work.
I was more wondering how the tests are run outside of travis. The new dns_resolver has a unit test that will of course fail if dnsresolver is not installed.... so there is probably some configuration to translate the option (dns) into which tests to include.
kuba
and 1 other
commented on an outdated diff
| +import dns.exception | ||
| + | ||
| +logger = logging.getLogger(__name__) | ||
| + | ||
| +def txt_records_for_name(name): | ||
| + """Resolve the name and return the TXT records. | ||
| + | ||
| + :param unicode name: Domain name being verified. | ||
| + | ||
| + :returns: A list of txt records, if empty the name could not be resolved | ||
| + :rtype: list of unicode | ||
| + | ||
| + """ | ||
| + try: | ||
| + dns_response = dns.resolver.query(name, 'TXT') | ||
| + except ImportError as error: # pragma: no cover |
|
Yep, leftover from an earlier version. It seems the import must be global so the mocking works properly.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
kuba
and 1 other
commented on an outdated diff
| + | ||
| +logger = logging.getLogger(__name__) | ||
| + | ||
| +def txt_records_for_name(name): | ||
| + """Resolve the name and return the TXT records. | ||
| + | ||
| + :param unicode name: Domain name being verified. | ||
| + | ||
| + :returns: A list of txt records, if empty the name could not be resolved | ||
| + :rtype: list of unicode | ||
| + | ||
| + """ | ||
| + try: | ||
| + dns_response = dns.resolver.query(name, 'TXT') | ||
| + except ImportError as error: # pragma: no cover | ||
| + raise ImportError("Local validation for 'dns-01' challenges requires " |
|
I wouldn't necessarily hide one import error using another import error,
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
NB One of the standard ways of dealing with optional deps I've seen around in other Python projects is:
try:
import foo
except ImportError:
# optionally logger.warning('all function depending on foo will not work because it's not importable')
foo = None
...
def bar():
if foo is None:
raise Error('foo is necessary for bar to work, please install it')Does anyone else have any ideas, suggestions?
kuba
and 1 other
commented on an outdated diff
| + :rtype: bool | ||
| + | ||
| + """ | ||
| + if not self.verify(chall, account_public_key): | ||
| + logger.debug("Verification of key authorization in response failed") | ||
| + return False | ||
| + | ||
| + validation_domain_name = chall.validation_domain_name(domain) | ||
| + validation = chall.validation(account_public_key) | ||
| + logger.debug("Verifying %s at %s...", chall.typ, validation_domain_name) | ||
| + | ||
| + try: | ||
| + from acme import dns_resolver | ||
| + txt_records = dns_resolver.txt_records_for_name( | ||
| + validation_domain_name) | ||
| + except ImportError: # pragma: no cover |
|
In general, please restrict However, it's still not clear whether Assuming in the long run the DNS interaction may be extended (e.g., checking DANE records for some future validation, or explicitly validating DNSSEC) I figured we might as well move the DNS specific code into a separate module anyway. I am more wondering if a little more should be pushed to the DNS specific file. The other option of course would be to modularize this code and have challenge-specific files. That way the whole challenge handling could be loaded via an options file. But even then the DNS dependency only applies to simple_verify, which is not strictly needed even if DNS challenges are supported by the client.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
BTW, if you follow the method I suggested above (catching ImportError), and mock Answer using mock.Mock rather than dns.rrset it should be possible to run tests in environments where dnspython is missing - which answers your question from #2061 (comment) and #2061 (comment)
Given DNS has landed in production (https://twitter.com/letsencrypt/status/689919523164721152) we probably want to have this merged ASAP. Will look into that after I come back from FOSDEM.
Works like a charm for me. Can't wait to have it merged in master :)
I didn't find this PR before. I'm writing a plugin that needs this challenge, and started working on the ACME challenge myself in PR #2461. I'll test this code with my plugin and close my PR as it isn't needed.
| + DNS server is successful. | ||
| + :rtype: bool | ||
| + | ||
| + """ | ||
| + if not self.verify(chall, account_public_key): | ||
| + logger.debug("Verification of key authorization in response failed") | ||
| + return False | ||
| + | ||
| + validation_domain_name = chall.validation_domain_name(domain) | ||
| + validation = chall.validation(account_public_key) | ||
| + logger.debug("Verifying %s at %s...", chall.typ, validation_domain_name) | ||
| + | ||
| + try: | ||
| + from acme import dns_resolver | ||
| + except ImportError: # pragma: no cover | ||
| + raise errors.Error("Local validation for 'dns-01' challenges " |
|
I'm not sure this should raise an error. Not sure. My idea was that if you do call self-verification you should have the extra library installed (i.e., code using the client should ensure the lib is installed). Returning false means "I tried the verification and it failed". That is not the case if the library is missing, in that case a precondition for the call is missing. A "typical" client would have to abort trying to answer the challenge (assuming it will fail). And the client would not be able to determine if it is a problem that is solvable by creating a new challenge and trying to verify it. Thoughts? I agree that "failed to do verification" is seperate from "verification failed". I'm not sure how to best indicate this... raising an error here looks good to me, we might want to have a different class of errors, like
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
| + return False | ||
| + | ||
| + validation_domain_name = chall.validation_domain_name(domain) | ||
| + validation = chall.validation(account_public_key) | ||
| + logger.debug("Verifying %s at %s...", chall.typ, validation_domain_name) | ||
| + | ||
| + try: | ||
| + from acme import dns_resolver | ||
| + except ImportError: # pragma: no cover | ||
| + raise errors.Error("Local validation for 'dns-01' challenges " | ||
| + "requires 'dnspython'") | ||
| + txt_records = dns_resolver.txt_records_for_name(validation_domain_name) | ||
| + exists = validation in txt_records | ||
| + if not exists: | ||
| + logger.debug("Key authorization from response (%r) doesn't match " | ||
| + "any DNS response in %r", self.key_authorization, |
|
I see your argument, but this matches all the other responses in the module. So I'd rather keep it this way (and maybe change it uniformly later, because I do agree that it would be helpful to see the actual expected string). Agreed with @wteiken, we could potentially fix this in follow-up PR, but not here. NB there is no security issue here as the challenge response is verified at the beginning of the function, so
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
| + """ACME "dns-01" challenge.""" | ||
| + response_cls = DNS01Response | ||
| + typ = response_cls.typ | ||
| + | ||
| + LABEL = "_acme-challenge" | ||
| + """Label clients prepend to the domain name being validated.""" | ||
| + | ||
| + def validation(self, account_key, **unused_kwargs): | ||
| + """Generate validation. | ||
| + | ||
| + :param JWK account_key: | ||
| + :rtype: unicode | ||
| + | ||
| + """ | ||
| + return jose.b64encode(hashlib.sha256(self.key_authorization( | ||
| + account_key).encode("utf-8")).digest()).decode() |
|
Why return unicode here? DNS TXT records are not encoding aware so I think a string would be better. The return value from this method becomes available in Authenticator plugins, and this way they will always need to encode it back to a string. My thought that a string might be better mostly comes from the docstring of the abstract method:
I am tempted to say that even the validation_domain_name and other constants defined by the ACME specification could be returned here. (Thats what I did myself in 83fe5e8)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
moshevds
and 1 other
commented on an outdated diff
| + | ||
| +logger = logging.getLogger(__name__) | ||
| + | ||
| +def txt_records_for_name(name): | ||
| + """Resolve the name and return the TXT records. | ||
| + | ||
| + :param unicode name: Domain name being verified. | ||
| + | ||
| + :returns: A list of txt records, if empty the name could not be resolved | ||
| + :rtype: list of unicode | ||
| + | ||
| + """ | ||
| + try: | ||
| + dns_response = dns.resolver.query(name, 'TXT') | ||
| + except dns.exception.DNSException as error: | ||
| + logger.error("Unable to resolve %s: %s", name, str(error)) |
|
If the subdomain does not exist NXDOMAIN is raised here, and that's not really an error, just the signal for "no result". Maybe add "except dns.resolver.NXDOMAIN: return []"?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
There is a small merge conflict with 86d6d27 but this code works great. Thanks.
I reviewed the code and it looks good to me. I hope this is merged soon :-)
I uploaded my plugin that uses this pull request to https://github.com/moshevds/le_dnsupdate.
@kuba When do you think you will be able to look at this pullrequest?
Also, is there any interest in having a dnsupdate/RFC2136 plugin directly in the letsencrypt repository?
@kuba Any idea when you'll have a chance to take another look to get this merged?
One I noticed while playing around with #2236 was that a dependency needs adding in setup.py for dnspython.
Hi @mitchellrj,
The dnspython dependency is actually already added (you can see it at d842f26). It is only used for the simple_verify method, but there is no need to use that method in the client itself. #2505 is actually created to remove the one place the client currently uses it (the http manual plugin, your new dns manual plugin does not need to use simple_verify either).
I'm currently writing integration tests with Boulder to get this merged soon.
FYI my WIP branch which builds off @wteiken's code: https://github.com/kuba/letsencrypt/tree/dns-01.
Couple of PRs and bug fixing later, bd4a07a has all green (https://travis-ci.org/kuba/letsencrypt/builds/122090126) integration testing for dns-01. At least I know this PR is working... Will look at the code once again and send a new PR soon, stay tuned.
how will it handle hooks to dns provider?
does it compatible to letsencrypt.sh or https://github.com/AnalogJ/lexicon ?
This is not at all about any client support but only the python library. If you program your tooling against the acme python library you can integrate with your DNS in any way you like.
It's really bad, consider implement it in CLI ?
if need a "standard" dns update method, here is a "nsupdate" program. although most of DNS service providers don't support RFC2136, There will be nsupdate-compatible program to do it, like AnalogJ/lexicon . It's much better than let every dns-01 user develop them own client.
Hi @zh99998,
I have already implemented a RFC2136 plugin that works with this patch: https://github.com/moshevds/le_dnsupdate
When this DNS-01 support is merged I intend to create a pull request to get RFC2136 support into the standard client. So what you are suggesting is already underway. I have also seen efforts to create a "manual" plugin that works with DNS-01.
What happened with this PR? Is something blocking this from being merged?
Kuba almost single-handedly wrote the acme module so he was reviewing and building off this PR, however, he hasn't pushed any commits to his branch in over two months.
While most of the rest of the client team is focusing on nginx support. We'll see if we can get this reviewed for the next release.
Sorry for the delay everyone!
@bmw we talked about whether adding a dnspython dependency would undermine our medium-term goal of having certbot-auto not require gcc and compilation steps. It appears that dnspython is purely Pythonic, so that's not a concern for merging this PR.
It might be a good idea to adjust the simple_verify() method to query the authoritative nameserver(s) of a domain, instead of the system configured ones. If there's for some reason delay in publishing the newly added records (due to zone transfers for example) the authoritative nameserver will respond with a NXDOMAIN which will be cached.
@dropje86, Would you require the simple_verify method for something? If not, I now think any effort to improve on this particular method should not delay this PR any further. simple_verify should not be used by certbot anyway (but could be used by other users of the acme package), as you can read about here: #1586 (comment)
I thought about the suggestion you now make myself and I haven't decided about my opinion on it. Sure any sane ACME-based CA would do as you suggest, but that is not the (only) use-case for the acme package. Also: Any implementation improvement for this method is obviously a good idea, but creating a simple_verify that is fully hardened against potential external configurations seems to go counter to the very name of the method.
I am, among many others it seems, impatiently awaiting this PR to be land. I really hope that happens soon!
@moshevds totally agree that it shouldn't block the PR. Was just mentioning the downside of the current verification method implementation. For now I've packaged a local version with this PR merged, as it is explicitly what I need.
bmw
commented on an outdated diff
| @@ -207,6 +206,74 @@ def response_and_validation(self, account_key, *args, **kwargs): | ||
| @ChallengeResponse.register | ||
| +class DNS01Response(KeyAuthorizationChallengeResponse): | ||
| + """ACME "dns-01" challenge response.""" |
|
nit: Maybe we should remove the quotes around `"dns-01" to be consistent with the other challenge types.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
bmw
commented on an outdated diff
| + from acme import dns_resolver | ||
| + except ImportError: # pragma: no cover | ||
| + raise errors.Error("Local validation for 'dns-01' challenges " | ||
| + "requires 'dnspython'") | ||
| + txt_records = dns_resolver.txt_records_for_name(validation_domain_name) | ||
| + exists = validation in txt_records | ||
| + if not exists: | ||
| + logger.debug("Key authorization from response (%r) doesn't match " | ||
| + "any DNS response in %r", self.key_authorization, | ||
| + txt_records) | ||
| + return exists | ||
| + | ||
| + | ||
| +@Challenge.register # pylint: disable=too-many-ancestors | ||
| +class DNS01(KeyAuthorizationChallenge): | ||
| + """ACME "dns-01" challenge.""" |
|
nit: Maybe we should remove the quotes around `"dns-01" to be consistent with the other challenge types.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
bmw
commented on an outdated diff
| @@ -207,6 +206,74 @@ def response_and_validation(self, account_key, *args, **kwargs): | ||
| @ChallengeResponse.register | ||
| +class DNS01Response(KeyAuthorizationChallengeResponse): | ||
| + """ACME "dns-01" challenge response.""" | ||
| + typ = "dns-01" | ||
| + | ||
| + def simple_verify(self, chall, domain, account_public_key): | ||
| + """Simple verify. | ||
| + | ||
| + :param challenges.DNS01 chall: Corresponding challenge. | ||
| + :param unicode domain: Domain name being verified. | ||
| + :param account_public_key: Public key for the key pair |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
bmw
commented on an outdated diff
| @@ -35,6 +35,15 @@ | ||
| else: | ||
| install_requires.append('mock') | ||
| +if sys.version_info < (3, 0): |
|
This kind of code makes me nervous. My understanding is that we'd have to start building two wheels of our packages, one for Python 2 and one for Python 3. It'd be nice to not have to do that if possible. Looks like we're in luck though. Since version 1.12, EDIT: Another reason to do this is dnspython has continued to get updates while dnspython3 has not.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
Other than a few tiny nits above, this LGTM. Looks like there's a merge conflict that needs to be fixed as well, but once these changes are made, I'll merge. Sorry again for the delay!
EDIT: Appending "[needs minor revision]" to the title. Please remove it when you'd like a final review.
bmw
changed the title from Add dns-01 challenge support to the ACME client to Add dns-01 challenge support to the ACME client [needs minor revision]
Thanks for the review, I'll try to address the open items this weekend.
Changes Unknown when pulling b495d7e on wteiken:add_dns01_challenge into ** on certbot:master**.
The latest update should address the comments and resolve the merge conflict.
wteiken
changed the title from Add dns-01 challenge support to the ACME client [needs minor revision] to Add dns-01 challenge support to the ACME client
Tested against a local dev version of the non-spec server, should also work against the soon-to-be-updated staging server.