AnalogJ / lexicon

Manipulate DNS records on various DNS providers in a standardized way.
MIT License
1.47k stars 303 forks source link

Support multiple records of the same type on an identifier #182

Closed bmw closed 6 years ago

bmw commented 6 years ago

In certbot, we use lexicon to create TXT records for a domain to prove to a SSL/TLS certificate authority that the user has control over that domain so the CA will give you a certificate which you can use to set up HTTPS. Due to the way the protocol works, we sometimes have to set multiple TXT records for a single domain.

I'm not sure if each of the providers we use have problems with creating multiple TXT records on a domain (although I know this works with your code for dnsimple), but they all have problems deleting it and immediately error out if there is more than 1 record for the domain:

Can support for this be added?

AnalogJ commented 6 years ago

Hey,

Yeah, there were some general assumptions made when lexicon was first developed. It may be time to take another look at those and handle record types differently.

I can work on adding record type specific tests, however I'll need to get in touch with some of the original provider developers to get them updated and and tested. I'll try to get this started tonight/tomorrow.

references #179

trinopoty commented 6 years ago

I've taken a look at the code of the mentioned providers and I believe I can fix the deletion issue in them. The question of whether I can test them on the respective platforms is another question.

bmw commented 6 years ago

@AnalogJ, let me know how you'd prefer I document problems with creating multiple records as we find them (e.g. create a new issue, add a new post here, modify my original post, etc.), but I got a detailed bug report from @Fmstrat at https://github.com/certbot/certbot/issues/5735 saying that creating multiple records doesn't work with nsone. I've copied the relevant bits here for convenience:

Notice the "false" at the end of this section here:

Starting new HTTPS connection (1): api.nsone.net
https://api.nsone.net:443 "GET /v1/zones/MYDOMAIN.com HTTP/1.1" 200 None
Starting new HTTPS connection (1): api.nsone.net
https://api.nsone.net:443 "PUT /v1/zones/MYDOMAIN.com/_acme-challenge.MYDOMAIN.com/TXT HTTP/1.1" 200 None
create_record: True
Starting new HTTPS connection (1): api.nsone.net
https://api.nsone.net:443 "GET /v1/zones/MYDOMAIN.com HTTP/1.1" 200 None
Starting new HTTPS connection (1): api.nsone.net
https://api.nsone.net:443 "PUT /v1/zones/MYDOMAIN.com/_acme-challenge.MYDOMAIN.com/TXT HTTP/1.1" 400 36
create_record: False

I think what is happening is that, unlike separate subdomains, with wildcard certs you need two answers for one TXT record, instead of multiple answers for multiple TXT records.

For example from https://ns1.com/api we can see trying to create a TXT record by calling the API twice fails:

curl -X PUT -H 'X-NSONE-Key: [...APIKEY...]' -d '{"zone":"DOMAIN.com", "domain":"_txt-test2.DOMAIN.com", "type":"TXT", "answers":[{"answer":["testA"]}]}' https://api.nsone.net/v1/zones/DOMAIN.com/_txt-test2.DOMAIN.com/TXT
{"domain":"_txt-test2.DOMAIN.com","zone":"DOMAIN.com","use_client_subnet":true,"answers":[{"answer":["testA"],"id":"5aaa[...]cda51"}],"id":"5aaa[...]cda51","regions":{},"meta":{},"link":null,"filters":[],"ttl":3600,"tier":1,"type":"TXT","networks":[0]}
curl -X PUT -H 'X-NSONE-Key: [...APIKEY...]' -d '{"zone":"DOMAIN.com", "domain":"_txt-test2.DOMAIN.com", "type":"TXT", "answers":[{"answer":["testB"]}]}' https://api.nsone.net/v1/zones/DOMAIN.com/_txt-test2.DOMAIN.com/TXT
{"message":"record already exists"}

This means only "testA" is shown as an answer, vs "testA" and "testB". From that API doc, to have two answers, this should be ran instead of the above twice:

curl -X PUT -H 'X-NSONE-Key: [...APIKEY...]' -d '{"zone":"DOMAIN.com", "domain":"_txt-test2.DOMAIN.com", "type":"TXT", "answers":[{"answer":["testA"]},{"answer":["testB"]}]}' https://api.nsone.net/v1/zones/DOMAIN.com/_txt-test2.DOMAIN.com/TXT

So, I believe the nsone DNS plugin needs to be updated to check for a wildcard request, and use the multi-answer API call instead of multiple single-answer API calls if that is the case.

It's best for us (and probably other existing Lexicon users), if Lexicon can keep the same API and query for existing records to support this, but if the API has to change, life goes on, just try to give me a heads up!

Fmstrat commented 6 years ago

EDIT Sorry, thought this was in the certbot repo. And it's wrong. So... ignore please.

I think this can be altered without changing Lexicon. Take a look here for nsone:

https://github.com/certbot/certbot/blob/master/certbot-dns-nsone/certbot_dns_nsone/dns_nsone.py#L51

And here:

https://github.com/certbot/certbot/blob/7da53819682fe1b320e0e2890d4cc4be0a637303/certbot/plugins/dns_common_lexicon.py#L33

In one of those places (probably nsone if others are working), we could add to that section of the plugin a check:

We could also delay all of the TXT calls by storing them in variables and checking, which would cut down on API calls, but may require changes outside of the plugin.

This series of logic would result in a two answer TXT record with no changes to Lexicon.

bmw commented 6 years ago

Since this is talking about a way to solve the problem in Certbot rather than Lexicon, I responded back on the Certbot repo at https://github.com/certbot/certbot/issues/5735#issuecomment-373427137.

Fmstrat commented 6 years ago

Hi everyone. I've started a fix that works for ns1, and should work for others while keeping all providers independent to ensure no conflicts. I've got a commit that handles create/delete, but haven't had time to look into update yet. Since I'm new to Lexicon, I'd love if someone could take a quick peek.

Basically, I'm overriding the "--content" option with one that allows you to do:

--content="Answer1" --content="Answer2" --content="Answer3" ...

This requires one change in the main function (allow overrides), and the rest is all done in the provider file.

Here's the commit: https://github.com/Fmstrat/lexicon/commit/7f2670fa61df7e2eb4e25c8c793a861154ede97b

AnalogJ commented 6 years ago

Hey guys,

Sorry its taken so long to get a working solution going. I've been a bit busy, but it's mostly because I kept coming up with completely over-engineered solutions that did too many other things.

Instead I've decided to just to this properly, piece by piece. I've created a new PR #190 that's tracking my changes.

If there's any additional tests you think I should add related to record sets, I'm more than happy to add them before I start making recordings. tests/providers/integration_tests.py

bmw commented 6 years ago

Thanks so much @AnalogJ.

While the Certbot team is also quite busy, if you don't have credentials for any of the providers listed in my initial post and the other developers who have worked on them aren't able to quickly respond, let me know. We're obviously especially interested in seeing this resolved for the providers we're using and can try and pitch in.

AnalogJ commented 6 years ago

Hey @bmw Looks like I got all the providers you mentioned except nsone. Honestly I'm not sure how I got recordings for it in the first place, as it requires a paid account. Any chance someone on your team has an account and can create recordings in a PR against the record_set_support branch?

Fmstrat commented 6 years ago

@AnalogJ, I'd be happy to but I won't be back from vacation until Monday. However, if you want your own testing account, nsone does offer a free account with 50 hosts and 500k queries a month. You have to put in a CC, but they don't charge you unless you go over those limits.

AnalogJ commented 6 years ago

With @trinopoty 's PR for nsone fixes + recordings, all the providers mentioned in @bmw 's list of Certbot plugins are now fixed.

Since it looks like it'll be a while yet until we get all the other providers up-to-spec with the new record set tests, I'm planning on ignoring the new tests on the remaining (unfixed) providers in #190, and releasing a new "minor" version.

Unless you guys have any objections?

bmw commented 6 years ago

Thanks @trinopoty :heart:

@AnalogJ, releasing a new minor version now would be great. Thanks for all your work on this.

trinopoty commented 6 years ago

I have to mention that update_record for nsone and a few other providers are broken as mentioned in #200 . It's especially broken for nsone due to the way nsone handles records. Please refrain from using update_record if you can.

AnalogJ commented 6 years ago

Reopening this, to continue tracking until all other providers are fixed as well.

The above mentioned providers & others I have credentials for have been fixed, and released in lexicon v2.2.0

AnalogJ commented 6 years ago

Actually I'm going to keep this closed. All certbot plugin providers have been fixed, but there's alot of other lexicon providers that need to be fixed and have updated test recordings. This work is happening in #203 which is Part 2 of this issue.