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

Add Gandi DNS challenge provider #133

Merged
merged 2 commits into from Mar 17, 2016
Merged

Add Gandi DNS challenge provider #133

merged 2 commits into from Mar 17, 2016

Conversation

xi2
Copy link
Contributor

@xi2 xi2 commented Feb 16, 2016

This commit adds support for a Gandi DNS ChallengeProvider, making use
of Gandi's XML-RPC API described at http://doc.rpc.gandi.net/index.html. It
has no dependencies other than the Go standard library.

I realise that due to #100 and #111 there may not be opportunity to
look at this for a while but I thought I'd upload it anyway. Once they
are resolved, if it is deemed suitable for lego/acme, I can easily
rework it into its own package or whatever else is required.

Some issues that I came across were:

  • Gandi has a long delay until the TXT record appears. In my tests it
    seemed to take 20-30 minutes on average, but the TXT record does
    appear eventually! Therefore I had to change the waitFor timeout
    call following the "Checking DNS record propagation..." from 30
    seconds to 60 minutes. However, users of other ChallengeProviders
    are not likely to be happy about this new timeout. Do we need a
    per-provider timeout? What's the best way? A new interface method to
    be implemented for ChallengeProvider such as Timeout() time.Duration?
  • Because of the long DNS propagation time users are more likely to
    use CTRL-C to abort the running program than with other DNS
    providers, in which case CleanUp does not run and you are left with
    a useless but harmless TXT record and zone. In the CLI program, do
    you think we should trap that signal (on unix) and attempt to run
    CleanUps for unfinished challenges, or is this a non-issue?
  • I had trouble thinking of a suitable way to make tests. In the end I
    recorded the XML going back and forth from a successful session,
    anonymized it, and used it as the basis for a fake RPC server. I
    hope that is enough, but any better suggestions are welcome.

Cheers,

@xenolf
Copy link
Member

xenolf commented Feb 18, 2016

Thank you for providing this :)

Is it really take 30 minutes for a DNS record to appear? That seems very excessive.

@xi2
Copy link
Contributor Author

xi2 commented Feb 18, 2016

Hi xenolf

Gandi themselves say it takes up to 20 minutes (see question one of this faq: https://wiki.gandi.net/en/dns/faq). However, I found it could take slightly longer (50%-ish) for lego to notice. There was a couple of occasions during testing when I typed:

dig TXT example.com @a.dns.gandi.net

(and the same for b.dns.gandi.net and c.dns.gandi.net) and dig picked it up, but then lego didn't notice for a further 5/10 minutes. That seemed odd, but I didn't look into it any further since lego did always pick it up without fail. The longest it took after around 10 tests was about 31 minutes as I recall (hence the 60 minute timeout). But it remains unexplained to me as to why it should ever be longer than 20 minutes as stated in Gandi's official docs. If I get a chance I will have another look at this to see if there is a bug in our DNS code, but I couldn't see anything at a quick glance.

Thanks for considering this patch.

@xi2
Copy link
Contributor Author

xi2 commented Feb 19, 2016

The >20min thing was bugging me so I stuck in a few Printfs to see what was going on, and I think I have indeed found a bug within checkDNSPropagation. What is happening is that within this function the code never queries the authoritative nameservers until the TXT record shows up on the recursive nameserver. Hence the extra 5-10 minutes wait that should not be there. As far as I can gather the recursive nameserver should not be holding things up like this. I will submit a PR to fix, once I am 100% sure of things (unless someone beats me to it).

EDIT: See #136

@xenolf xenolf added this to the v0.3.0 milestone Mar 7, 2016
@xi2
Copy link
Contributor Author

xi2 commented Mar 8, 2016

Rebased to current master and altered to use the ChallengeProviderTimeout type from #148. Other DNS providers no longer have their timeouts altered.

Awaiting merge of #144, following which this PR will be altered to use the new package structure given there.

@xi2
Copy link
Contributor Author

xi2 commented Mar 12, 2016

I have rebased this PR to now sit on top of both #144 and #148 in their present state.

@xenolf
Copy link
Member

xenolf commented Mar 14, 2016

Something isn't right with this PR. There are changes in here which are supposed to only be in #148

@xi2
Copy link
Contributor Author

xi2 commented Mar 14, 2016

@xenolf. Sorry about that. I forgot to update this PR when I changed #148. It should be fixed now. Thanks for merging #144!

@xi2
Copy link
Contributor Author

xi2 commented Mar 14, 2016

There are changes listed here that are from #148, but that is because this PR depends on #148. When #148 Is finally resolved and merged I'll remove them and rebase to master at that point. I hope that is the correct way to use git in this project? If not, corrections appreciated.

Michael Cross added 2 commits March 16, 2016 18:17
This type allows for implementing DNS ChallengeProviders that require
an unsually long timeout when checking for record propagation.
xenolf added a commit that referenced this pull request Mar 17, 2016
@xenolf xenolf merged commit 8c3023d into go-acme:master Mar 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants