From 980ada4f6719488ff2f27259edf6a813db4009ef Mon Sep 17 00:00:00 2001 From: Daniel Date: Sat, 30 Jun 2018 21:01:33 -0400 Subject: [PATCH 01/10] DNS Providers: Add ACME-DNS provider. 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. --- cli.go | 1 + providers/dns/acmedns/acmedns.go | 161 ++++++++++++++ providers/dns/acmedns/acmedns_test.go | 301 ++++++++++++++++++++++++++ providers/dns/dns_providers.go | 3 + 4 files changed, 466 insertions(+) create mode 100644 providers/dns/acmedns/acmedns.go create mode 100644 providers/dns/acmedns/acmedns_test.go diff --git a/cli.go b/cli.go index a499dc76c5..901778318b 100644 --- a/cli.go +++ b/cli.go @@ -198,6 +198,7 @@ Here is an example bash command using the CloudFlare DNS provider: w := tabwriter.NewWriter(os.Stdout, 0, 8, 1, '\t', 0) fmt.Fprintln(w, "Valid providers and their associated credential environment variables:") fmt.Fprintln(w) + fmt.Fprintln(w, "\tacme-dns:\tACME_DNS_API_BASE, ACME_DNS_STORAGE_PATH") fmt.Fprintln(w, "\tazure:\tAZURE_CLIENT_ID, AZURE_CLIENT_SECRET, AZURE_SUBSCRIPTION_ID, AZURE_TENANT_ID, AZURE_RESOURCE_GROUP") fmt.Fprintln(w, "\tauroradns:\tAURORA_USER_ID, AURORA_KEY, AURORA_ENDPOINT") fmt.Fprintln(w, "\tbluecat:\tBLUECAT_SERVER_URL, BLUECAT_USER_NAME, BLUECAT_PASSWORD, BLUECAT_CONFIG_NAME, BLUECAT_DNS_VIEW") diff --git a/providers/dns/acmedns/acmedns.go b/providers/dns/acmedns/acmedns.go new file mode 100644 index 0000000000..ff4860f22a --- /dev/null +++ b/providers/dns/acmedns/acmedns.go @@ -0,0 +1,161 @@ +// Package acmedns implements a DNS provider for solving DNS-01 challenges using +// Joohoi's acme-dns project. For more information see the ACME-DNS homepage: +// https://github.com/joohoi/acme-dns +package acmedns + +import ( + "errors" + "fmt" + "os" + + "github.com/cpu/goacmedns" + "github.com/xenolf/lego/acme" +) + +const ( + // envNamespace is the prefix for ACME-DNS environment variables. + envNamespace = "ACME_DNS_" + // apiBaseEnvVar is the environment variable name for the ACME-DNS API address + // (e.g. https://acmedns.your-domain.com). + apiBaseEnvVar = envNamespace + "API_BASE" + // storagePathEnvVar is the environment variable name for the ACME-DNS JSON + // account data file. A per-domain account will be registered/persisted to + // this file and used for TXT updates. + storagePathEnvVar = envNamespace + "STORAGE_PATH" +) + +// acmeDNSClient is an interface describing the goacmedns.Client functions +// the DNSProvider uses. It makes it easier for tests to shim a mock Client into +// the DNSProvider. +type acmeDNSClient interface { + // UpdateTXTRecord updates the provided account's TXT record to the given + // value or returns an error. + UpdateTXTRecord(goacmedns.Account, string) error + // RegisterAccount registers and returns a new account with the given + // allowFrom restriction or returns an error. + RegisterAccount([]string) (goacmedns.Account, error) +} + +// DNSProvider is an implementation of the acme.ChallengeProvider interface for +// an ACME-DNS server. +type DNSProvider struct { + apiBase string + client acmeDNSClient + storage goacmedns.Storage +} + +// NewDNSProvider creates an ACME-DNS provider. Its configuration is loaded from +// the environment by reading apiBaseEnvVar and storagePathEnvVar. +func NewDNSProvider() (*DNSProvider, error) { + endpoint := os.Getenv(apiBaseEnvVar) + storagePath := os.Getenv(storagePathEnvVar) + return NewDNSProviderClient(endpoint, storagePath) +} + +// NewDNSProviderClient creates an ACME-DNS DNSProvider configured to +// communicate with the provided ACME-DNS API endpoint, and to save/load account +// credentials in a file found at the provided storage path. +func NewDNSProviderClient(apiBase string, storagePath string) (*DNSProvider, error) { + if apiBase == "" { + return nil, errors.New("ACME-DNS API base must not be empty") + } + if storagePath == "" { + return nil, errors.New("ACME-DNS Storage path must not be empty") + } + + return &DNSProvider{ + apiBase: apiBase, + client: goacmedns.NewClient(apiBase), + storage: goacmedns.NewFileStorage(storagePath, 0600), + }, nil +} + +// ErrCNAMERequired is returned by Present when the Domain indicated had no +// existing ACME-DNS account in the Storage and additional setup is required. +// The user must create a CNAME in the DNS zone for Domain that aliases FQDN +// to Target in order to complete setup for the ACME-DNS account that was +// created. +type ErrCNAMERequired struct { + // The Domain that is being issued for. + Domain string + // The alias of the CNAME (left hand DNS label). + FQDN string + // The RDATA of the CNAME (right hand side, canonical name). + Target string +} + +// Error returns a descriptive message for the ErrCNAMERequired instance telling +// the user that a CNAME needs to be added to the DNS zone of c.Domain before +// the ACME-DNS hook will work. The CNAME to be created should be of the form: +// {{ c.FQDN }} CNAME {{ c.Target }} +func (e ErrCNAMERequired) Error() string { + return fmt.Sprintf("acme-dns: new account created for %q. "+ + "To complete setup for %q you must provision the following "+ + "CNAME in your DNS zone and re-run this provider when it is "+ + "in place:\n"+ + "%s CNAME %s.", + e.Domain, e.Domain, e.FQDN, e.Target) +} + +// Present creates a TXT record to fulfil the DNS-01 challenge. If there is an +// existing account for the domain in the provider's storage then it will be +// used to set the challenge response TXT record with the ACME-DNS server and +// issuance will continue. If there is not an account for the given domain +// present in the DNSProvider storage one will be created and registered with +// the ACME DNS server and an ErrCNAMERequired error is returned. This will halt +// issuance and indicate to the user that a one-time manual setup is required +// for the domain. +func (d *DNSProvider) Present(domain, _, keyAuth string) error { + // Compute the challenge response FQDN and TXT value for the domain based + // on the keyAuth. + fqdn, value, _ := acme.DNS01Record(domain, keyAuth) + // Check if credentials were previously saved for this domain. + account, err := d.storage.Fetch(domain) + // Errors other than goacmeDNS.ErrDomainNotFound are unexpected. + if err != nil && err != goacmedns.ErrDomainNotFound { + return err + } else 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) + } + // Update the acme-dns TXT record. + return d.client.UpdateTXTRecord(account, value) +} + +// CleanUp removes the record matching the specified parameters. It is not +// implemented for the ACME-DNS provider. +func (d *DNSProvider) CleanUp(_, _, _ string) error { + // ACME-DNS doesn't support the notion of removing a record. For users of + // ACME-DNS it is expected the stale records remain in-place. + return nil +} + +// register creates a new ACME-DNS account for the given domain. If account +// creation works as expected a ErrCNAMERequired error is returned describing +// the one-time manual CNAME setup required to complete setup of the ACME-DNS +// hook for the domain. If any other error occurs it is returned as-is. +func (d *DNSProvider) register(domain, fqdn string) error { + // TODO(@cpu): Read CIDR whitelists from the environment + newAcct, err := d.client.RegisterAccount(nil) + if err != nil { + return err + } + // Store the new account in the storage and call save to persist the data. + err = d.storage.Put(domain, newAcct) + if err != nil { + return err + } + err = d.storage.Save() + if err != nil { + return err + } + // Stop issuance by returning an error. The user needs to perform a manual + // one-time CNAME setup in their DNS zone to complete the setup of the new + // account we created. + return ErrCNAMERequired{ + Domain: domain, + FQDN: fqdn, + Target: newAcct.FullDomain, + } +} diff --git a/providers/dns/acmedns/acmedns_test.go b/providers/dns/acmedns/acmedns_test.go new file mode 100644 index 0000000000..ed0136b6d7 --- /dev/null +++ b/providers/dns/acmedns/acmedns_test.go @@ -0,0 +1,301 @@ +package acmedns + +import ( + "errors" + "testing" + + "github.com/cpu/goacmedns" +) + +var ( + // errorClientErr is used by the Client mocks that return an error. + errorClientErr = errors.New("errorClient always errors") + // errorStorageErr is used by the Storage mocks that return an error. + errorStorageErr = errors.New("errorStorage always errors") + + // Fixed test data for unit tests. + egDomain = "threeletter.agency" + egFQDN = "_acme-challenge." + egDomain + "." + egKeyAuth = "⚷" + egAccount = goacmedns.Account{ + FullDomain: "acme-dns." + egDomain, + SubDomain: "random-looking-junk." + egDomain, + Username: "spooky.mulder", + Password: "trustno1", + } +) + +// mockClient is a mock implementing the acmeDNSClient interface that always +// returns a fixed goacmedns.Account from calls to Register. +type mockClient struct { + mockAccount goacmedns.Account +} + +// UpdateTXTRecord does nothing. +func (c mockClient) UpdateTXTRecord(_ goacmedns.Account, _ string) error { + return nil +} + +// RegisterAccount returns c.mockAccount and no errors. +func (c mockClient) RegisterAccount(_ []string) (goacmedns.Account, error) { + return c.mockAccount, nil +} + +// mockUpdateClient is a mock implementing the acmeDNSClient interface that +// tracks the calls to UpdateTXTRecord in the records map. +type mockUpdateClient struct { + mockClient + records map[goacmedns.Account]string +} + +// UpdateTXTRecord saves a record value to c.records for the given acct. +func (c mockUpdateClient) UpdateTXTRecord(acct goacmedns.Account, value string) error { + c.records[acct] = value + return nil +} + +// errorRegisterClient is a mock implementing the acmeDNSClient interface that always +// returns errors from errorUpdateClient. +type errorUpdateClient struct { + mockClient +} + +// UpdateTXTRecord always returns an error. +func (c errorUpdateClient) UpdateTXTRecord(_ goacmedns.Account, _ string) error { + return errorClientErr +} + +// errorRegisterClient is a mock implementing the acmeDNSClient interface that always +// returns errors from RegisterAccount. +type errorRegisterClient struct { + mockClient +} + +// RegisterAccount always returns an error. +func (c errorRegisterClient) RegisterAccount(_ []string) (goacmedns.Account, error) { + return goacmedns.Account{}, errorClientErr +} + +// mockStorage is a mock implementing the goacmedns.Storage interface that +// returns static account data and ignores Save. +type mockStorage struct { + accounts map[string]goacmedns.Account +} + +// Save does nothing. +func (m mockStorage) Save() error { + return nil +} + +// Put stores an account for the given domain in m.accounts. +func (m mockStorage) Put(domain string, acct goacmedns.Account) error { + m.accounts[domain] = acct + return nil +} + +// Fetch retrieves an account for the given domain from m.accounts or returns +// goacmedns.ErrDomainNotFound. +func (m mockStorage) Fetch(domain string) (goacmedns.Account, error) { + if acct, ok := m.accounts[domain]; ok { + return acct, nil + } + return goacmedns.Account{}, goacmedns.ErrDomainNotFound +} + +// errorPutStorage is a mock implementing the goacmedns.Storage interface that +// always returns errors from Put. +type errorPutStorage struct { + mockStorage +} + +// Put always errors. +func (e errorPutStorage) Put(_ string, _ goacmedns.Account) error { + return errorStorageErr +} + +// errorSaveStoragr is a mock implementing the goacmedns.Storage interface that +// always returns errors from Save. +type errorSaveStorage struct { + mockStorage +} + +// Save always errors. +func (e errorSaveStorage) Save() error { + return errorStorageErr +} + +// errorFetchStorage is a mock implementing the goacmedns.Storage interface that +// always returns errors from Fetch. +type errorFetchStorage struct { + mockStorage +} + +// Fetch always errors. +func (e errorFetchStorage) Fetch(_ string) (goacmedns.Account, error) { + return goacmedns.Account{}, errorStorageErr +} + +// TestPresent tests that the ACME-DNS Present function for updating a DNS-01 +// challenge response TXT record works as expected. +func TestPresent(t *testing.T) { + // validAccountStorage is a mockStorage configured to return the egAccount. + validAccountStorage := mockStorage{ + map[string]goacmedns.Account{ + egDomain: egAccount, + }, + } + // validUpdateClient is a mockClient configured with the egAccount that will + // track TXT updates in a map. + validUpdateClient := mockUpdateClient{ + mockClient{egAccount}, + make(map[goacmedns.Account]string), + } + + testCases := []struct { + Name string + Client acmeDNSClient + Storage goacmedns.Storage + ExpectedError error + }{ + { + Name: "present when client storage returns unexpected error", + Client: mockClient{egAccount}, + Storage: errorFetchStorage{}, + ExpectedError: errorStorageErr, + }, + { + Name: "present when client storage returns ErrDomainNotFound", + Client: mockClient{egAccount}, + ExpectedError: ErrCNAMERequired{ + Domain: egDomain, + FQDN: egFQDN, + Target: egAccount.FullDomain, + }, + }, + { + Name: "present when client UpdateTXTRecord returns unexpected error", + Client: errorUpdateClient{}, + Storage: validAccountStorage, + ExpectedError: errorClientErr, + }, + { + Name: "present when everything works", + Storage: validAccountStorage, + Client: validUpdateClient, + }, + } + + for _, tc := range testCases { + t.Run(tc.Name, func(t *testing.T) { + // create the DNS provider using. The API base address and the storage + // 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 { + t.Fatalf("unexpected error creating NewDNSProviderClient: %v", err) + } + // mock the client. + dp.client = tc.Client + // mock the storage. + dp.storage = mockStorage{make(map[string]goacmedns.Account)} + // override the storage mock if required by the testcase. + if tc.Storage != nil { + dp.storage = tc.Storage + } + // call Present. The token argument can be garbage because the ACME-DNS + // provider does not use it. + err = dp.Present(egDomain, "foo", egKeyAuth) + if tc.ExpectedError != nil && err == nil { + t.Errorf("expected present to return error %v, got nil", tc.ExpectedError) + } else if tc.ExpectedError == nil && err != nil { + t.Errorf("expected present to return no error, got %v", err) + } else if tc.ExpectedError != nil && err != nil && tc.ExpectedError != err { + t.Errorf("expected present to return error %v, got %v", tc.ExpectedError, err) + } + }) + } + + // Check that the success testcase set a record. + if len(validUpdateClient.records) != 1 { + t.Fatalf("expected present to successfully set a record on the mock, got none") + } + + // Check that the success testcase set the right record for the right account. + if value, ok := validUpdateClient.records[egAccount]; !ok { + t.Errorf("expected present to successfully set a record for the egAccount, got none") + } else if len(value) != 43 { + t.Errorf("expected present to successfully set a record with 43 characters "+ + "for the egAccount. Got value %q, len %d", + value, len(value)) + } +} + +// TestRegister tests that the ACME-DNS register function works correctly. +func TestRegister(t *testing.T) { + testCases := []struct { + Name string + Client acmeDNSClient + Storage goacmedns.Storage + Domain string + FQDN string + ExpectedError error + }{ + { + Name: "register when acme-dns client returns an error", + Client: errorRegisterClient{}, + ExpectedError: errorClientErr, + }, + { + Name: "register when acme-dns storage put returns an error", + Client: mockClient{egAccount}, + Storage: errorPutStorage{mockStorage{make(map[string]goacmedns.Account)}}, + ExpectedError: errorStorageErr, + }, + { + Name: "register when acme-dns storage save returns an error", + Client: mockClient{egAccount}, + Storage: errorSaveStorage{mockStorage{make(map[string]goacmedns.Account)}}, + ExpectedError: errorStorageErr, + }, + { + Name: "register when everything works", + Client: mockClient{egAccount}, + ExpectedError: ErrCNAMERequired{ + Domain: egDomain, + FQDN: egFQDN, + Target: egAccount.FullDomain, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.Name, func(t *testing.T) { + // create the DNS provider using. The API base address and the storage + // 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 { + t.Fatalf("unexpected error creating NewDNSProviderClient: %v", err) + } + // mock the client. + dp.client = tc.Client + // mock the storage. + dp.storage = mockStorage{ + make(map[string]goacmedns.Account), + } + // override the storage mock if required by the testcase. + if tc.Storage != nil { + dp.storage = tc.Storage + } + // Call register for the example domain/fqdn. + err = dp.register(egDomain, egFQDN) + if tc.ExpectedError != nil && err == nil { + t.Errorf("expected register to return error %v, got nil", tc.ExpectedError) + } else if tc.ExpectedError == nil && err != nil { + t.Errorf("expected register to return no error, got %v", err) + } else if tc.ExpectedError != nil && err != nil && tc.ExpectedError != err { + t.Errorf("expected register to return error %v, got %v", tc.ExpectedError, err) + } + }) + } +} diff --git a/providers/dns/dns_providers.go b/providers/dns/dns_providers.go index 2a4c80e30e..8c5cbd5c32 100644 --- a/providers/dns/dns_providers.go +++ b/providers/dns/dns_providers.go @@ -4,6 +4,7 @@ import ( "fmt" "github.com/xenolf/lego/acme" + "github.com/xenolf/lego/providers/dns/acmedns" "github.com/xenolf/lego/providers/dns/auroradns" "github.com/xenolf/lego/providers/dns/azure" "github.com/xenolf/lego/providers/dns/bluecat" @@ -41,6 +42,8 @@ import ( // NewDNSChallengeProviderByName Factory for DNS providers func NewDNSChallengeProviderByName(name string) (acme.ChallengeProvider, error) { switch name { + case "acme-dns": + return acmedns.NewDNSProvider() case "azure": return azure.NewDNSProvider() case "auroradns": From 8d3e310895c981e7bb67ec397553763d4ef66c3b Mon Sep 17 00:00:00 2001 From: Daniel Date: Sun, 1 Jul 2018 15:14:14 -0400 Subject: [PATCH 02/10] Add goacmedns to the Gopkg.lock --- Gopkg.lock | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/Gopkg.lock b/Gopkg.lock index 355b7c4367..8d3bc278fe 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -82,6 +82,12 @@ revision = "398d14696895d68a3409bb3ccb1cfe8abc2d4376" version = "v1.13.57" +[[projects]] + branch = "master" + name = "github.com/cpu/goacmedns" + packages = ["."] + revision = "f232997f461a5a58982d536108cfc382e512481e" + [[projects]] name = "github.com/davecgh/go-spew" packages = ["spew"] @@ -374,6 +380,6 @@ [solve-meta] analyzer-name = "dep" analyzer-version = 1 - inputs-digest = "d93e6f0825f1f7c1c3c2ccd421af4d99b77b390f3346ca751968af0609119c96" + inputs-digest = "5d63911ee02dd9fe3f9699420a28c50e069c6dbc0d9fe2ed4e0598fd1ec28845" solver-name = "gps-cdcl" solver-version = 1 From e5fc7bf05c15c8a40c4f0a61fb89dc7e75180163 Mon Sep 17 00:00:00 2001 From: Daniel Date: Mon, 2 Jul 2018 11:34:27 -0400 Subject: [PATCH 03/10] acme-dns: use env.Get like other providers --- providers/dns/acmedns/acmedns.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/providers/dns/acmedns/acmedns.go b/providers/dns/acmedns/acmedns.go index ff4860f22a..07a359f7bb 100644 --- a/providers/dns/acmedns/acmedns.go +++ b/providers/dns/acmedns/acmedns.go @@ -6,10 +6,10 @@ package acmedns import ( "errors" "fmt" - "os" "github.com/cpu/goacmedns" "github.com/xenolf/lego/acme" + "github.com/xenolf/lego/platform/config/env" ) const ( @@ -47,9 +47,11 @@ type DNSProvider struct { // NewDNSProvider creates an ACME-DNS provider. Its configuration is loaded from // the environment by reading apiBaseEnvVar and storagePathEnvVar. func NewDNSProvider() (*DNSProvider, error) { - endpoint := os.Getenv(apiBaseEnvVar) - storagePath := os.Getenv(storagePathEnvVar) - return NewDNSProviderClient(endpoint, storagePath) + values, err := env.Get(apiBaseEnvVar, storagePathEnvVar) + if err != nil { + return nil, fmt.Errorf("acme-dns: %v", err) + } + return NewDNSProviderClient(values[apiBaseEnvVar], values[storagePathEnvVar]) } // NewDNSProviderClient creates an ACME-DNS DNSProvider configured to From f5e122fdc5dbf27712cc0fc6a6738958d34c1fc3 Mon Sep 17 00:00:00 2001 From: Daniel Date: Mon, 2 Jul 2018 11:35:50 -0400 Subject: [PATCH 04/10] acme-dns: use if not else if --- providers/dns/acmedns/acmedns.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/providers/dns/acmedns/acmedns.go b/providers/dns/acmedns/acmedns.go index 07a359f7bb..026c153eb1 100644 --- a/providers/dns/acmedns/acmedns.go +++ b/providers/dns/acmedns/acmedns.go @@ -116,7 +116,8 @@ func (d *DNSProvider) Present(domain, _, keyAuth string) error { // Errors other than goacmeDNS.ErrDomainNotFound are unexpected. if err != nil && err != goacmedns.ErrDomainNotFound { return err - } else if err == goacmedns.ErrDomainNotFound { + } + 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) From 48d0f5b4997a734891fb8e3362554b9f9d13fca6 Mon Sep 17 00:00:00 2001 From: Daniel Date: Mon, 2 Jul 2018 11:47:13 -0400 Subject: [PATCH 05/10] acme-dns: use testify assertions --- providers/dns/acmedns/acmedns_test.go | 48 +++++++++++---------------- 1 file changed, 20 insertions(+), 28 deletions(-) diff --git a/providers/dns/acmedns/acmedns_test.go b/providers/dns/acmedns/acmedns_test.go index ed0136b6d7..188c4a0798 100644 --- a/providers/dns/acmedns/acmedns_test.go +++ b/providers/dns/acmedns/acmedns_test.go @@ -2,6 +2,7 @@ package acmedns import ( "errors" + "github.com/stretchr/testify/assert" "testing" "github.com/cpu/goacmedns" @@ -187,13 +188,13 @@ func TestPresent(t *testing.T) { for _, tc := range testCases { t.Run(tc.Name, func(t *testing.T) { + assert := assert.New(t) // create the DNS provider using. The API base address and the storage // 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 { - t.Fatalf("unexpected error creating NewDNSProviderClient: %v", err) - } + assert.Nil(err) + // mock the client. dp.client = tc.Client // mock the storage. @@ -205,29 +206,20 @@ func TestPresent(t *testing.T) { // call Present. The token argument can be garbage because the ACME-DNS // provider does not use it. err = dp.Present(egDomain, "foo", egKeyAuth) - if tc.ExpectedError != nil && err == nil { - t.Errorf("expected present to return error %v, got nil", tc.ExpectedError) - } else if tc.ExpectedError == nil && err != nil { - t.Errorf("expected present to return no error, got %v", err) - } else if tc.ExpectedError != nil && err != nil && tc.ExpectedError != err { - t.Errorf("expected present to return error %v, got %v", tc.ExpectedError, err) + if tc.ExpectedError != nil { + assert.NotNil(err) + assert.Equal(err, tc.ExpectedError) + } else { + assert.Nil(err) } }) } // Check that the success testcase set a record. - if len(validUpdateClient.records) != 1 { - t.Fatalf("expected present to successfully set a record on the mock, got none") - } + assert.Equal(t, len(validUpdateClient.records), 1) // Check that the success testcase set the right record for the right account. - if value, ok := validUpdateClient.records[egAccount]; !ok { - t.Errorf("expected present to successfully set a record for the egAccount, got none") - } else if len(value) != 43 { - t.Errorf("expected present to successfully set a record with 43 characters "+ - "for the egAccount. Got value %q, len %d", - value, len(value)) - } + assert.Equal(t, len(validUpdateClient.records[egAccount]), 43) } // TestRegister tests that the ACME-DNS register function works correctly. @@ -270,13 +262,13 @@ func TestRegister(t *testing.T) { for _, tc := range testCases { t.Run(tc.Name, func(t *testing.T) { + assert := assert.New(t) // create the DNS provider using. The API base address and the storage // 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 { - t.Fatalf("unexpected error creating NewDNSProviderClient: %v", err) - } + assert.Nil(err) + // mock the client. dp.client = tc.Client // mock the storage. @@ -287,14 +279,14 @@ func TestRegister(t *testing.T) { if tc.Storage != nil { dp.storage = tc.Storage } + // Call register for the example domain/fqdn. err = dp.register(egDomain, egFQDN) - if tc.ExpectedError != nil && err == nil { - t.Errorf("expected register to return error %v, got nil", tc.ExpectedError) - } else if tc.ExpectedError == nil && err != nil { - t.Errorf("expected register to return no error, got %v", err) - } else if tc.ExpectedError != nil && err != nil && tc.ExpectedError != err { - t.Errorf("expected register to return error %v, got %v", tc.ExpectedError, err) + if tc.ExpectedError != nil { + assert.NotNil(err) + assert.Equal(tc.ExpectedError, err) + } else { + assert.Nil(err) } }) } From f4b2b6bb6694a6432461b08fcf6221e028faff16 Mon Sep 17 00:00:00 2001 From: Daniel Date: Tue, 3 Jul 2018 18:44:47 -0400 Subject: [PATCH 06/10] ACME-DNS: Add exported constructor to specify Storage impl. 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. --- providers/dns/acmedns/acmedns.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/providers/dns/acmedns/acmedns.go b/providers/dns/acmedns/acmedns.go index 026c153eb1..fdcf7e64dc 100644 --- a/providers/dns/acmedns/acmedns.go +++ b/providers/dns/acmedns/acmedns.go @@ -72,6 +72,26 @@ func NewDNSProviderClient(apiBase string, storagePath string) (*DNSProvider, err }, nil } +// NewDNSProviderClientStorage creates an ACME-DNS DNSProvider configured to +// communicate with the provided ACME-DNS API endpoint, and to save/load account +// credentials using the provided acmedns.Storage implementation. This is useful +// for programmatic usage of the ACME-DNS provider where fine-grained control +// over data persistance is required. +func NewDNSProviderClientStorage(apiBase string, storage goacmedns.Storage) (*DNSProvider, error) { + if apiBase == "" { + return nil, errors.New("ACME-DNS API base must not be empty") + } + if storage == nil { + return nil, errors.New("ACME-DNS Storage must not be nil") + } + + return &DNSProvider{ + apiBase: apiBase, + client: goacmedns.NewClient(apiBase), + storage: storage, + }, nil +} + // ErrCNAMERequired is returned by Present when the Domain indicated had no // existing ACME-DNS account in the Storage and additional setup is required. // The user must create a CNAME in the DNS zone for Domain that aliases FQDN From 555485fa005b1efbca0f8af4b3c426ab69bbc51e Mon Sep 17 00:00:00 2001 From: Daniel Date: Tue, 3 Jul 2018 19:43:41 -0400 Subject: [PATCH 07/10] ACME-DNS: fix spelling error --- providers/dns/acmedns/acmedns.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/providers/dns/acmedns/acmedns.go b/providers/dns/acmedns/acmedns.go index fdcf7e64dc..7c77afc0f3 100644 --- a/providers/dns/acmedns/acmedns.go +++ b/providers/dns/acmedns/acmedns.go @@ -76,7 +76,7 @@ func NewDNSProviderClient(apiBase string, storagePath string) (*DNSProvider, err // communicate with the provided ACME-DNS API endpoint, and to save/load account // credentials using the provided acmedns.Storage implementation. This is useful // for programmatic usage of the ACME-DNS provider where fine-grained control -// over data persistance is required. +// over data persistence is required. func NewDNSProviderClientStorage(apiBase string, storage goacmedns.Storage) (*DNSProvider, error) { if apiBase == "" { return nil, errors.New("ACME-DNS API base must not be empty") From d9b03727da571be7fb399d32f7883fb77a240187 Mon Sep 17 00:00:00 2001 From: Daniel Date: Thu, 5 Jul 2018 11:32:22 -0400 Subject: [PATCH 08/10] ACME-DNS: Use one NewDNSProvider func --- providers/dns/acmedns/acmedns.go | 45 +++++++-------------------- providers/dns/acmedns/acmedns_test.go | 20 ++---------- 2 files changed, 14 insertions(+), 51 deletions(-) diff --git a/providers/dns/acmedns/acmedns.go b/providers/dns/acmedns/acmedns.go index 7c77afc0f3..540e4ab5e8 100644 --- a/providers/dns/acmedns/acmedns.go +++ b/providers/dns/acmedns/acmedns.go @@ -39,55 +39,34 @@ type acmeDNSClient interface { // DNSProvider is an implementation of the acme.ChallengeProvider interface for // an ACME-DNS server. type DNSProvider struct { - apiBase string client acmeDNSClient storage goacmedns.Storage } -// NewDNSProvider creates an ACME-DNS provider. Its configuration is loaded from -// the environment by reading apiBaseEnvVar and storagePathEnvVar. +// NewDNSProvider creates an ACME-DNS provider using file based account storage. +// Its configuration is loaded from the environment by reading apiBaseEnvVar and +// storagePathEnvVar. func NewDNSProvider() (*DNSProvider, error) { values, err := env.Get(apiBaseEnvVar, storagePathEnvVar) if err != nil { return nil, fmt.Errorf("acme-dns: %v", err) } - return NewDNSProviderClient(values[apiBaseEnvVar], values[storagePathEnvVar]) -} - -// NewDNSProviderClient creates an ACME-DNS DNSProvider configured to -// communicate with the provided ACME-DNS API endpoint, and to save/load account -// credentials in a file found at the provided storage path. -func NewDNSProviderClient(apiBase string, storagePath string) (*DNSProvider, error) { - if apiBase == "" { + if values[apiBaseEnvVar] == "" { return nil, errors.New("ACME-DNS API base must not be empty") } - if storagePath == "" { + if values[storagePathEnvVar] == "" { return nil, errors.New("ACME-DNS Storage path must not be empty") } - - return &DNSProvider{ - apiBase: apiBase, - client: goacmedns.NewClient(apiBase), - storage: goacmedns.NewFileStorage(storagePath, 0600), - }, nil + client := goacmedns.NewClient(values[apiBaseEnvVar]) + storage := goacmedns.NewFileStorage(values[storagePathEnvVar], 0600) + return NewDNSProviderClient(client, storage) } -// NewDNSProviderClientStorage creates an ACME-DNS DNSProvider configured to -// communicate with the provided ACME-DNS API endpoint, and to save/load account -// 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) { - if apiBase == "" { - return nil, errors.New("ACME-DNS API base must not be empty") - } - if storage == nil { - return nil, errors.New("ACME-DNS Storage must not be nil") - } - +// NewDNSProviderClient creates an ACME-DNS DNSProvider with the given +// acmeDNSClient and goacmedns.Storage. +func NewDNSProviderClient(client acmeDNSClient, storage goacmedns.Storage) (*DNSProvider, error) { return &DNSProvider{ - apiBase: apiBase, - client: goacmedns.NewClient(apiBase), + client: client, storage: storage, }, nil } diff --git a/providers/dns/acmedns/acmedns_test.go b/providers/dns/acmedns/acmedns_test.go index 188c4a0798..df74e1847d 100644 --- a/providers/dns/acmedns/acmedns_test.go +++ b/providers/dns/acmedns/acmedns_test.go @@ -189,16 +189,9 @@ func TestPresent(t *testing.T) { for _, tc := range testCases { t.Run(tc.Name, func(t *testing.T) { assert := assert.New(t) - // create the DNS provider using. The API base address and the storage - // 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") + dp, err := NewDNSProviderClient(tc.Client, mockStorage{make(map[string]goacmedns.Account)}) assert.Nil(err) - // mock the client. - dp.client = tc.Client - // mock the storage. - dp.storage = mockStorage{make(map[string]goacmedns.Account)} // override the storage mock if required by the testcase. if tc.Storage != nil { dp.storage = tc.Storage @@ -263,18 +256,9 @@ func TestRegister(t *testing.T) { for _, tc := range testCases { t.Run(tc.Name, func(t *testing.T) { assert := assert.New(t) - // create the DNS provider using. The API base address and the storage - // 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") + dp, err := NewDNSProviderClient(tc.Client, mockStorage{make(map[string]goacmedns.Account)}) assert.Nil(err) - // mock the client. - dp.client = tc.Client - // mock the storage. - dp.storage = mockStorage{ - make(map[string]goacmedns.Account), - } // override the storage mock if required by the testcase. if tc.Storage != nil { dp.storage = tc.Storage From 49c96899cdbd265354c6fff5b2fd8851da9169a5 Mon Sep 17 00:00:00 2001 From: Daniel Date: Thu, 5 Jul 2018 11:36:14 -0400 Subject: [PATCH 09/10] ACME-DNS: Better testify usage --- providers/dns/acmedns/acmedns_test.go | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/providers/dns/acmedns/acmedns_test.go b/providers/dns/acmedns/acmedns_test.go index df74e1847d..376ca25390 100644 --- a/providers/dns/acmedns/acmedns_test.go +++ b/providers/dns/acmedns/acmedns_test.go @@ -3,6 +3,7 @@ package acmedns import ( "errors" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "testing" "github.com/cpu/goacmedns" @@ -188,9 +189,8 @@ func TestPresent(t *testing.T) { for _, tc := range testCases { t.Run(tc.Name, func(t *testing.T) { - assert := assert.New(t) dp, err := NewDNSProviderClient(tc.Client, mockStorage{make(map[string]goacmedns.Account)}) - assert.Nil(err) + require.NoError(t, err) // override the storage mock if required by the testcase. if tc.Storage != nil { @@ -200,19 +200,18 @@ func TestPresent(t *testing.T) { // provider does not use it. err = dp.Present(egDomain, "foo", egKeyAuth) if tc.ExpectedError != nil { - assert.NotNil(err) - assert.Equal(err, tc.ExpectedError) + assert.Equal(t, tc.ExpectedError, err) } else { - assert.Nil(err) + require.NoError(t, err) } }) } // Check that the success testcase set a record. - assert.Equal(t, len(validUpdateClient.records), 1) + assert.Len(t, 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) } // TestRegister tests that the ACME-DNS register function works correctly. @@ -255,9 +254,8 @@ func TestRegister(t *testing.T) { for _, tc := range testCases { t.Run(tc.Name, func(t *testing.T) { - assert := assert.New(t) dp, err := NewDNSProviderClient(tc.Client, mockStorage{make(map[string]goacmedns.Account)}) - assert.Nil(err) + require.NoError(t, err) // override the storage mock if required by the testcase. if tc.Storage != nil { @@ -267,10 +265,9 @@ func TestRegister(t *testing.T) { // Call register for the example domain/fqdn. err = dp.register(egDomain, egFQDN) if tc.ExpectedError != nil { - assert.NotNil(err) - assert.Equal(tc.ExpectedError, err) + assert.Equal(t, tc.ExpectedError, err) } else { - assert.Nil(err) + require.NoError(t, err) } }) } From 2ca17642c44d68e25a3c34e729cc916272c0bd12 Mon Sep 17 00:00:00 2001 From: Fernandez Ludovic Date: Mon, 9 Jul 2018 17:27:06 +0200 Subject: [PATCH 10/10] review: minor changes. --- providers/dns/acmedns/acmedns.go | 19 +++++++++++++------ providers/dns/acmedns/acmedns_test.go | 4 +++- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/providers/dns/acmedns/acmedns.go b/providers/dns/acmedns/acmedns.go index 540e4ab5e8..cce0d8d876 100644 --- a/providers/dns/acmedns/acmedns.go +++ b/providers/dns/acmedns/acmedns.go @@ -51,12 +51,7 @@ func NewDNSProvider() (*DNSProvider, error) { if err != nil { return nil, fmt.Errorf("acme-dns: %v", err) } - if values[apiBaseEnvVar] == "" { - return nil, errors.New("ACME-DNS API base must not be empty") - } - if values[storagePathEnvVar] == "" { - return nil, errors.New("ACME-DNS Storage path must not be empty") - } + client := goacmedns.NewClient(values[apiBaseEnvVar]) storage := goacmedns.NewFileStorage(values[storagePathEnvVar], 0600) return NewDNSProviderClient(client, storage) @@ -65,6 +60,14 @@ func NewDNSProvider() (*DNSProvider, error) { // NewDNSProviderClient creates an ACME-DNS DNSProvider with the given // acmeDNSClient and goacmedns.Storage. func NewDNSProviderClient(client acmeDNSClient, storage goacmedns.Storage) (*DNSProvider, error) { + if client == nil { + return nil, errors.New("ACME-DNS Client must be not nil") + } + + if storage == nil { + return nil, errors.New("ACME-DNS Storage must be not nil") + } + return &DNSProvider{ client: client, storage: storage, @@ -110,6 +113,7 @@ func (d *DNSProvider) Present(domain, _, keyAuth string) error { // Compute the challenge response FQDN and TXT value for the domain based // on the keyAuth. fqdn, value, _ := acme.DNS01Record(domain, keyAuth) + // Check if credentials were previously saved for this domain. account, err := d.storage.Fetch(domain) // Errors other than goacmeDNS.ErrDomainNotFound are unexpected. @@ -121,6 +125,7 @@ func (d *DNSProvider) Present(domain, _, keyAuth string) error { // indicating the required one-time manual CNAME setup. return d.register(domain, fqdn) } + // Update the acme-dns TXT record. return d.client.UpdateTXTRecord(account, value) } @@ -143,6 +148,7 @@ func (d *DNSProvider) register(domain, fqdn string) error { if err != nil { return err } + // Store the new account in the storage and call save to persist the data. err = d.storage.Put(domain, newAcct) if err != nil { @@ -152,6 +158,7 @@ func (d *DNSProvider) register(domain, fqdn string) error { if err != nil { return err } + // Stop issuance by returning an error. The user needs to perform a manual // one-time CNAME setup in their DNS zone to complete the setup of the new // account we created. diff --git a/providers/dns/acmedns/acmedns_test.go b/providers/dns/acmedns/acmedns_test.go index 376ca25390..d1131b10ab 100644 --- a/providers/dns/acmedns/acmedns_test.go +++ b/providers/dns/acmedns/acmedns_test.go @@ -2,9 +2,10 @@ package acmedns import ( "errors" + "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "testing" "github.com/cpu/goacmedns" ) @@ -196,6 +197,7 @@ func TestPresent(t *testing.T) { if tc.Storage != nil { dp.storage = tc.Storage } + // call Present. The token argument can be garbage because the ACME-DNS // provider does not use it. err = dp.Present(egDomain, "foo", egKeyAuth)