Add DNS Provider for ACME-DNS. #591

cpu · 2018-07-01T19:00:46Z

This PR adds a new "acme-dns" DNS provider for acme-dns to allow Lego to set DNS-01 challenge response TXT records with an ACME-DNS server automatically. ACME-DNS allows ceding minimal zone editing permissions to the ACME client and can be useful when the primary DNS provider for the zone does not allow scripting/API access but can set a CNAME to an ACME-DNS server.

The acme-dns provider accepts an ACME-DNS API server address in the ACME_DNS_API_BASE environment variable. The provider's lower level ACME-DNS API calls & account loading/storing is handled by the github.com/cpu/goacmedns library. This is roughly modelled after the acme-dns-certbot-joohoi hook for Certbot and the https://github.com/joohoi/pyacmedns Python library.

The acme-dns provider accepts a JSON storage file for ACME-DNS account information in the ACME_DNS_STORAGE_PATH environment variable. The provider loads existing ACME-DNS accounts/domains from this file at runtime. When required, the provider handles registering new accounts with the ACME-DNS server and saves the details to the storage path. This will halt issuance with an error prompting the user to set the one-time manual CNAME required to delegate the DNS-01 challenge record to the ACME-DNS server. Subsequent runs will use the previously registered account from disk and assume the CNAME is in-place. Library users of Lego can use the NewDNSProviderClientStorage constructor to provide their own goacmedns.Storage implementation if more advanced account management is required.

You can avoid having Lego fail on the first run by pre-registering the domains with ACME-DNS using the goacmedns-register command line utility and providing the -storage path you will later use as the ACME_DNS_STORAGE_PATH value. This will let you perform the initial one-time CNAME delegation ahead of using lego.

Usage example:

ACME-DNS is running at http://10.0.0.8:4443 with a configuration for pki.threeletter.agency. We're going to issue a certificate for *.legotest.xkeyscore.club with this setup by delegating the _acme-challenge.legotest.xkeyscore.club record to the pki.threeletter.agency ACME-DNS server. Account information will be saved in /root/.lego-acme-dns-accounts.json.

Since for this first run ACME_DNS_STORAGE_PATH is empty (we did not pre-register the domain with goacmedns-register) and the ACME-DNS CNAME delegation hasn't been done yet it will fail with an error about setting up the initial CNAME delegation:

root@example:~# ACME_DNS_API_BASE=http://10.0.0.8:4443 ACME_DNS_STORAGE_PATH=/root/.lego-acme-dns-accounts.json lego --server https://acme-staging-v02.api.letsencrypt.org/directory --email daniel@binaryparadox.net -a --dns acme-dns --domains "*.legotest.xkeyscore.club" run
2018/07/01 14:31:05 No key found for account daniel@binaryparadox.net. Generating a curve P384 EC key.
2018/07/01 14:31:05 Saved key to /root/.lego/accounts/acme-staging-v02.api.letsencrypt.org/daniel@binaryparadox.net/keys/daniel@binaryparadox.net.key
2018/07/01 14:31:05 [INFO] acme: Registering account for daniel@binaryparadox.net
2018/07/01 14:31:05 !!!! HEADS UP !!!!
2018/07/01 14:31:05 
		Your account credentials have been saved in your Let's Encrypt
		configuration directory at "/root/.lego/accounts/acme-staging-v02.api.letsencrypt.org/daniel@binaryparadox.net".
		You should make a secure backup	of this folder now. This
		configuration directory will also contain certificates and
		private keys obtained from Let's Encrypt so making regular
		backups of this folder is ideal.
2018/07/01 14:31:05 [INFO] [*.legotest.xkeyscore.club] acme: Obtaining bundled SAN certificate
2018/07/01 14:31:06 [INFO] [*.legotest.xkeyscore.club] AuthURL: https://acme-staging-v02.api.letsencrypt.org/acme/authz/qRUiCZ99r1ZGkeJXnQuZwnsSyunr8HDp_LNwfyXdmpk
2018/07/01 14:31:06 [INFO] [legotest.xkeyscore.club] acme: Trying to solve DNS-01
2018/07/01 14:31:06 Could not obtain certificates
	acme: Error -> One or more domains had a problem:
[legotest.xkeyscore.club] error presenting token: acme-dns: new account created for "legotest.xkeyscore.club". To complete setup for "legotest.xkeyscore.club" you must provision the following CNAME in your DNS zone and re-run this provider when it is in place:
_acme-challenge.legotest.xkeyscore.club. CNAME a9b4649d-113f-4449-886b-28dd534cc599.pki.threeletter.agency.

After adding the _acme-challenge.legotest.xkeyscore.club. CNAME a9b4649d-113f-4449-886b-28dd534cc599.pki.threeletter.agency. record to the xkeyscore.club DNS zone and making sure all of the secondary DNS servers have the record too we can run Lego again and it will issue the certificate successfully by updating the ACME-DNS TXT record:

root@example:~# ACME_DNS_API_BASE=http://10.0.0.8:4443 ACME_DNS_STORAGE_PATH=/root/.lego-acme-dns-accounts.json lego --server https://acme-staging-v02.api.letsencrypt.org/directory --email daniel@binaryparadox.net -a --dns acme-dns --domains "*.legotest.xkeyscore.club" run
2018/07/01 14:35:56 [INFO] [*.legotest.xkeyscore.club] acme: Obtaining bundled SAN certificate
2018/07/01 14:35:56 [INFO] [*.legotest.xkeyscore.club] AuthURL: https://acme-staging-v02.api.letsencrypt.org/acme/authz/qRUiCZ99r1ZGkeJXnQuZwnsSyunr8HDp_LNwfyXdmpk
2018/07/01 14:35:56 [INFO] [legotest.xkeyscore.club] acme: Trying to solve DNS-01
2018/07/01 14:35:57 [INFO] [legotest.xkeyscore.club] Checking DNS record propagation using [8.8.8.8:53]
2018/07/01 14:36:02 [INFO] [legotest.xkeyscore.club] The server validated our request
2018/07/01 14:36:02 [INFO] [*.legotest.xkeyscore.club] acme: Validations succeeded; requesting certificates
2018/07/01 14:36:03 [INFO] [*.legotest.xkeyscore.club] Server responded with a certificate.

Woohoo! 🎉

This commit adds a new DNS provider for [acme-dns](https://github.com/joohoi/acme-dns) to allow Lego to set DNS-01 challenge response TXT with an ACME-DNS server automatically. ACME-DNS allows ceding minimal zone editing permissions to the ACME client and can be useful when the primary DNS provider for the zone does not allow scripting/API access but can set a CNAME to an ACME-DNS server. Lower level ACME-DNS API calls & account loading/storing is handled by the `github.com/cpu/goacmedns` library. The provider loads existing ACME-DNS accounts from the specified JSON file on disk. Any accounts the provider registers on behalf of the user will also be saved to this JSON file. When required, the provider handles registering accounts with the ACME-DNS server domains that do not already have an ACME-DNS account. This will halt issuance with an error prompting the user to set the one-time manual CNAME required to delegate the DNS-01 challenge record to the ACME-DNS server. Subsequent runs will use the account from disk and assume the CNAME is in-place.

cpu · 2018-07-01T19:13:12Z

I didn't vendor the goacmedns library correctly and CI is unhappy:

Lock inputs-digest mismatch due to the following packages missing from the lock:
PROJECT                   MISSING PACKAGES
github.com/cpu/goacmedns  [github.com/cpu/goacmedns]  
This happens when a new import is added. Run `dep ensure` to install the missing packages.
input-digest mismatch
The command "dep status -v" failed and exited with 1 during .

I'm running dep ensure now as instructed by the error message. I think it must be fetching a number of dependencies from the network because it hasn't returned yet in 2+ min (I am on a very slow internet connection!).

I'll fix ASAP. Reviews most welcome in the meantime!

cpu · 2018-07-01T19:21:17Z

I'll fix ASAP. Reviews most welcome in the meantime!

As of d9bdd50 this should be resolved

ldez

I did not know https://github.com/joohoi/acme-dns, it looks great.

ldez · 2018-07-01T19:40:32Z

providers/dns/acmedns/acmedns.go

+func NewDNSProvider() (*DNSProvider, error) {
+	endpoint := os.Getenv(apiBaseEnvVar)
+	storagePath := os.Getenv(storagePathEnvVar)
+	return NewDNSProviderClient(endpoint, storagePath)


In order to be homogeneous with the other DNS providers, can you rewrite like the following example:
https://github.com/xenolf/lego/blob/c4bbb4b819c087d02066fe745d7ca5b60a480d36/providers/dns/cloudflare/cloudflare.go#L32-L39

But you can keep NewDNSProviderClient function name.

Fixed in e5fc7bf

ldez · 2018-07-01T19:55:30Z

providers/dns/acmedns/acmedns_test.go

+			// path can be made up because we mock the client and storage instead of
+			// making real API calls and writing JSON to disk.
+			dp, err := NewDNSProviderClient("foo", "bar")
+			if err != nil {


In order to be homogeneous with the other DNS providers, can you replace the use of the std-lib by Testify.

I took a crack at this in 48d0f5b Let me know if I'm still writing my tests too stdlib focused.

ldez · 2018-07-01T22:48:52Z

providers/dns/acmedns/acmedns.go

+	// Errors other than goacmeDNS.ErrDomainNotFound are unexpected.
+	if err != nil && err != goacmedns.ErrDomainNotFound {
+		return err
+	} else if err == goacmedns.ErrDomainNotFound {


The else does not seem necessary.

if err != nil && err != goacmedns.ErrDomainNotFound { return err } if err == goacmedns.ErrDomainNotFound { // The account did not exist. Create a new one and return an error // indicating the required one-time manual CNAME setup. return d.register(domain, fqdn) }

Good catch, fixed in f5e122f

shupp · 2018-07-02T16:10:11Z

This looks cool, thanks @cpu!. We actually use lego as a package, and don't use the filesystem at all. It would be sweet if there was an option to not be bound to the file system for storage of the account info. Optionally getting it from environment variables could work for us.

cpu · 2018-07-02T16:15:47Z

@shupp Thanks for the feedback

We actually use lego as a package, and don't use the filesystem at all. It would be sweet if there was an option to not be bound to the file system for storage of the account info. Optionally getting it from environment variables could work for us.

I would be open to refactoring slightly so that you could provide your own goacmedns.Storage implementation to the DNSProvider. In the effort of keeping this PR moving forward I'd slightly prefer to do that as a follow-up unless the maintainers would rather otherwise.

Edit: the required change was very small so I included it in this branch: 0c01b0e

cpu · 2018-07-02T16:17:24Z

@ldez Ready for another 🔍 on this PR when you have a chance. Thanks!

shupp · 2018-07-02T16:27:24Z

@cpu cool. Yeah, a follow-up would be more appropriate. Thanks!

For users that programmatically use Lego there may be demand for using the ACME-DNS provider with a custom goacmedns.Storage implementation that is more complex than the "read/save JSON from disk" default storage.

cpu · 2018-07-03T22:48:56Z

@shupp I don't have experience using Lego as a package, would cpu@f4b2b6b meet your needs?

shupp · 2018-07-03T23:05:32Z

@cpu Yeah, that's perfect. Thanks!

cpu · 2018-07-03T23:38:51Z

I think the CI failure for b43a443 is a flake unrelated to the branch:

unable to deduce repository and source type for "cloud.google.com/go": unable to read metadata: unable to fetch raw metadata: failed HTTP request to URL "http://cloud.google.com/go?go-get=1": Get https://cloud.google.com/go?go-get=1: net/http: TLS handshake timeout

…-provider

cpu · 2018-07-03T23:40:43Z

@cpu Yeah, that's perfect. Thanks!

Ok! Since it was a really small change and there hasn't been another 🔍 iteration from the maintainers yet I pushed that change into this PR in 0c01b0e

ldez

Some remarks

ldez · 2018-07-04T20:55:06Z

providers/dns/acmedns/acmedns.go

+// credentials using the provided acmedns.Storage implementation. This is useful
+// for programmatic usage of the ACME-DNS provider where fine-grained control
+// over data persistence is required.
+func NewDNSProviderClientStorage(apiBase string, storage goacmedns.Storage) (*DNSProvider, error) {


I think you can keep only 2 NewDNSProvider functions:

func NewDNSProvider() (*DNSProvider, error) { values, err := env.Get(apiBaseEnvVar, storagePathEnvVar) if err != nil { return nil, fmt.Errorf("acme-dns: %v", err) } client := goacmedns.NewClient(values[apiBaseEnvVar]) storage := goacmedns.NewFileStorage(values[storagePathEnvVar], 0600) return NewDNSProviderClient(client, storage) } func NewDNSProviderClient(client acmeDNSClient, storage goacmedns.Storage) (*DNSProvider, error) { // .... }

WFM! Much cleaner :-) I was a little bit shy about changing NewDNSProvider after you had reviewed it. Fixed in d9b0372

ldez · 2018-07-04T20:56:24Z

providers/dns/acmedns/acmedns.go

+// DNSProvider is an implementation of the acme.ChallengeProvider interface for
+// an ACME-DNS server.
+type DNSProvider struct {
+	apiBase string


apiBase is not used, it can be remove

ldez · 2018-07-04T20:58:45Z

providers/dns/acmedns/acmedns_test.go

+
+	for _, tc := range testCases {
+		t.Run(tc.Name, func(t *testing.T) {
+			assert := assert.New(t)


you can delete this line, see below

ldez · 2018-07-04T20:59:18Z

providers/dns/acmedns/acmedns_test.go

+			// path can be made up because we mock the client and storage instead of
+			// making real API calls and writing JSON to disk.
+			dp, err := NewDNSProviderClient("foo", "bar")
+			assert.Nil(err)


require.NoError(t, err)

ldez · 2018-07-04T21:00:51Z

providers/dns/acmedns/acmedns_test.go

+			err = dp.register(egDomain, egFQDN)
+			if tc.ExpectedError != nil {
+				assert.NotNil(err)
+				assert.Equal(tc.ExpectedError, err)


you can remove assert.NotNil(err) and rewrite the equals:

assert.Equal(t, tc.ExpectedError, err)

ldez · 2018-07-04T21:03:45Z

providers/dns/acmedns/acmedns_test.go

+			// path can be made up because we mock the client and storage instead of
+			// making real API calls and writing JSON to disk.
+			dp, err := NewDNSProviderClient("foo", "bar")
+			assert.Nil(err)


require.NoError(t, err)

ldez · 2018-07-04T21:05:06Z

providers/dns/acmedns/acmedns_test.go

+			err = dp.Present(egDomain, "foo", egKeyAuth)
+			if tc.ExpectedError != nil {
+				assert.NotNil(err)
+				assert.Equal(err, tc.ExpectedError)


you can remove assert.NotNil(err) and rewrite the equals:

assert.Equal(t, tc.ExpectedError, err)

ldez · 2018-07-04T21:05:47Z

providers/dns/acmedns/acmedns_test.go

+				assert.NotNil(err)
+				assert.Equal(err, tc.ExpectedError)
+			} else {
+				assert.Nil(err)


require.NoError(t, err)

ldez · 2018-07-04T21:06:45Z

providers/dns/acmedns/acmedns_test.go

+	}
+
+	// Check that the success testcase set a record.
+	assert.Equal(t, len(validUpdateClient.records), 1)


assert.Len(t, validUpdateClient.records, 1)

ldez · 2018-07-04T21:06:59Z

providers/dns/acmedns/acmedns_test.go

+	assert.Equal(t, len(validUpdateClient.records), 1)
+
+	// Check that the success testcase set the right record for the right account.
+	assert.Equal(t, len(validUpdateClient.records[egAccount]), 43)


assert.Len(t, validUpdateClient.records[egAccount], 43)

cpu · 2018-07-05T15:45:33Z

@ldez Ready for another 🔍 when you have a chance. Thanks for your patience with the testify tests. I should have read the API docs a little more :-)

cpu · 2018-07-05T15:46:55Z

@ldez Another question: Do you want me to rebase this branch before it's merged or do you folks prefer to squash merge? I'm happy to cleanup the commit history if you'd like.

ldez

LGTM 👍

cpu · 2018-07-09T17:36:32Z

Thanks @ldez!

cpu mentioned this pull request Jul 1, 2018

README: Add goacmedns, a Go acme-dns client library joohoi/acme-dns#92

Merged

cpu added 2 commits July 1, 2018 15:14

Add goacmedns to the Gopkg.lock

8d3e310

Merge remote-tracking branch 'origin/master' into cpu-acme-dns-provider

d9bdd50

ldez added the area/dnsprovider label Jul 1, 2018

ldez requested changes Jul 1, 2018

View reviewed changes

cpu added 4 commits July 2, 2018 11:28

Merge remote-tracking branch 'origin/master' into cpu-acme-dns-provider

7e383e0

acme-dns: use env.Get like other providers

e5fc7bf

acme-dns: use if not else if

f5e122f

acme-dns: use testify assertions

48d0f5b

ACME-DNS: Add exported constructor to specify Storage impl.

f4b2b6b

For users that programmatically use Lego there may be demand for using the ACME-DNS provider with a custom goacmedns.Storage implementation that is more complex than the "read/save JSON from disk" default storage.

Merge remote-tracking branch 'origin/master' into cpu-acme-dns-provider

b43a443

Merge branch 'cpu-acme-dns-provider-custom-storage' into cpu-acme-dns…

0c01b0e

…-provider

ACME-DNS: fix spelling error

555485f

ldez requested changes Jul 4, 2018

View reviewed changes

Daniel added 2 commits July 5, 2018 11:32

ACME-DNS: Use one NewDNSProvider func

d9b0372

ACME-DNS: Better testify usage

49c9689

review: minor changes.

2ca1764

ldez approved these changes Jul 9, 2018

View reviewed changes

ldez merged commit 04e2d74 into go-acme:master Jul 9, 2018

cpu deleted the cpu-acme-dns-provider branch July 9, 2018 17:35

ldez changed the title ~~DNS Providers: Add ACME-DNS provider.~~ Add DNS Provider for ACME-DNS. Aug 4, 2018

ldez added the enhancement label Sep 29, 2018

ldez added this to the v1.1 milestone Dec 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DNS Provider for ACME-DNS. #591

Add DNS Provider for ACME-DNS. #591

cpu commented Jul 1, 2018 •

edited

Loading

cpu commented Jul 1, 2018 •

edited

Loading

cpu commented Jul 1, 2018

ldez left a comment

ldez Jul 1, 2018

cpu Jul 2, 2018

ldez Jul 1, 2018

cpu Jul 2, 2018 •

edited

Loading

ldez Jul 1, 2018

cpu Jul 2, 2018

shupp commented Jul 2, 2018

cpu commented Jul 2, 2018 •

edited

Loading

cpu commented Jul 2, 2018

shupp commented Jul 2, 2018

cpu commented Jul 3, 2018 •

edited

Loading

shupp commented Jul 3, 2018

cpu commented Jul 3, 2018

cpu commented Jul 3, 2018

ldez left a comment

ldez Jul 4, 2018

cpu Jul 5, 2018 •

edited

Loading

ldez Jul 4, 2018

ldez Jul 4, 2018

ldez Jul 4, 2018

ldez Jul 4, 2018

ldez Jul 4, 2018

ldez Jul 4, 2018

ldez Jul 4, 2018

ldez Jul 4, 2018

ldez Jul 4, 2018

cpu commented Jul 5, 2018

cpu commented Jul 5, 2018 •

edited

Loading

ldez left a comment

cpu commented Jul 9, 2018

Add DNS Provider for ACME-DNS. #591

Add DNS Provider for ACME-DNS. #591

Conversation

cpu commented Jul 1, 2018 • edited Loading

Usage example:

cpu commented Jul 1, 2018 • edited Loading

cpu commented Jul 1, 2018

ldez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpu Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shupp commented Jul 2, 2018

cpu commented Jul 2, 2018 • edited Loading

cpu commented Jul 2, 2018

shupp commented Jul 2, 2018

cpu commented Jul 3, 2018 • edited Loading

shupp commented Jul 3, 2018

cpu commented Jul 3, 2018

cpu commented Jul 3, 2018

ldez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpu Jul 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpu commented Jul 5, 2018

cpu commented Jul 5, 2018 • edited Loading

ldez left a comment

Choose a reason for hiding this comment

cpu commented Jul 9, 2018

cpu commented Jul 1, 2018 •

edited

Loading

cpu commented Jul 1, 2018 •

edited

Loading

cpu Jul 2, 2018 •

edited

Loading

cpu commented Jul 2, 2018 •

edited

Loading

cpu commented Jul 3, 2018 •

edited

Loading

cpu Jul 5, 2018 •

edited

Loading

cpu commented Jul 5, 2018 •

edited

Loading