terricain / certbot-dns-azure

AzureDNS Certbot plugin
Other
33 stars 16 forks source link

Not all TXT records entries in cleanup should be overwritten with "-" #42

Closed felixZdi closed 3 months ago

felixZdi commented 4 months ago

We are currently implementing a wrapper library for DNS challenge based on certbot for Azure and were facing a small issue.

Use case: We want to use a "shared" TXT record for CName delegation, so that multiple users/identities can gather a Certficate with only one TXT record, instead of creating a dedicated TXT record for every user/identity.

The DNS Validation sometimes fails, if the challenges take place in parallel. I believe this section is the problem:

https://github.com/terrycain/certbot-dns-azure/blob/5a4f7e310e01715a5762875b5bf126cff734d248/certbot_dns_azure/_internal/dns_azure.py#L298-L304

Proposal: Only remove TXT record entry that was validated and only set TXT record to '-' if that entry was the last one of this record

felixZdi commented 4 months ago

Did you have a chance to look into this? Do you need additional information? I could also raise a PR and contribute.

terricain commented 3 months ago

Sorry, I read this a while back, and it completely slipped my mind.

So if i'm understanding you correctly, this is somewhat a race condition that multiple people update a singular record, adding validation txt enties as they go, and one of the validation pieces sets it to - wiping out the other ones?

felixZdi commented 3 months ago

Yes exactly - this only can happen when a TXT record is modified by multiple users. We´d like to only use one TXT and e.g. enable multiple user-assigned identities to aquire a certificate for the related CName.

That´s what I meant with the proposal - I guess to support this one would need to add the according checks before deleting/updating the TXT record.

terricain commented 3 months ago

So I had a quick look as there's usually (if you've designed the api well) mechanisms to only apply changes based on a precondition, to prevent these kind of read, then write issues. Azure seems to allow you to specify an etag on the update (which you'd get from the get record) which if provided then would fail if the record'd been modified since it was read.

I'll have a look this weekend to see if I can get that working.

terricain commented 3 months ago

Can you test the changes on the fix-parallel-dns-access branch, this should fix your issues.

felixZdi commented 3 months ago

Thanks for the fix - I will test it and give you feedback

felixZdi commented 3 months ago

@terrycain Sorry for the delay - I can confirm this is fixed. Thanks a lot!

terricain commented 3 months ago

Cool, I'll work on getting it released officially either this weekend or next.

terricain commented 3 months ago

This is now out on PyPI as version 2.5.0, is currently in the snapstore on the edge channel and will be promoted to stable assuming there are no complaints in a week.