Repository: xenolf/lego (go)

Issue:
dnsimple: DNS-01 challenge for wildcard: NXDOMAIN then panic (nil pointer dereference)
On commit ad20bf90ff5479cb8c63aef7e0f773b3c7e35835 (current master HEAD).

When obtaining a wildcard certificate, I get a panic using the dnsimple DNS provider:

```
 2018/09/26 04:25:13 [INFO] [*.subdomain.mydomain.net] acme: Obtaining bundled SAN certificate
 2018/09/26 04:25:13 [INFO] [*.subdomain.mydomain.net] AuthURL: https://acme-v02.api.letsencrypt.org/acme/authz/wRRYQyK5NOtKPgWJ64WraUKp3Uwa...
 2018/09/26 04:25:13 [INFO] [subdomain.mydomain.net] acme: Preparing to solve DNS-01
Sep 26 04:25:13 [ERROR] Server returned error NXDOMAIN, mitigating potential DNS violation DVE-2018-0001, retrying transaction with reduced feature level UDP.
Sep 26 04:25:13 [ERROR] Server returned error NXDOMAIN, mitigating potential DNS violation DVE-2018-0001, retrying transaction with reduced feature level UDP.
 panic: runtime error: invalid memory address or nil pointer dereference
 [signal SIGSEGV: segmentation violation code=0x1 addr=0x30 pc=0x8d779f]
 goroutine 1 [running]:
 github.com/xenolf/lego/providers/dns/dnsimple.(*DNSProvider).Present(0xc00017e660, 0xc00053c4e0, 0x18, 0xc0003b0060, 0x2b, 0xc0000606c0, 0x57, 0x0, 0xc00019ef60)
         /Users/matt/Dev/src/github.com/xenolf/lego/providers/dns/dnsimple/dnsimple.go:99 +0x13f
 github.com/xenolf/lego/acme.(*dnsChallenge).PreSolve(0xc000544240, 0xc000054930, 0x6a, 0xc0005283c0, 0x6, 0xc0005283c6, 0x7, 0xc0003b0060, 0x2b, 0x0, ...)
         /Users/matt/Dev/src/github.com/xenolf/lego/acme/dns_challenge.go:97 +0x17d
 github.com/xenolf/lego/acme.(*Client).solveChallengeForAuthz(0xc0001320c0, 0xc0001f83c0, 0x1, 0x1, 0x0, 0x0)
         /Users/matt/Dev/src/github.com/xenolf/lego/acme/client.go:601 +0x3c6
 github.com/xenolf/lego/acme.(*Client).ObtainCertificate(0xc0001320c0, 0xc000521020, 0x1, 0x1, 0x4b5a01, 0x0, 0x0, 0x0, 0x0, 0xc00001d980, ...)
         /Users/matt/Dev/src/github.com/xenolf/lego/acme/client.go:434 +0x2b3
```

The NXDOMAIN error is strange, since the domain definitely exists, and in fact this same code was running in production just fine before moving from an earlier commit to this latest one.

However, the panic happens even without NXDOMAIN errors (here's another try from this morning):

```
2018/09/26 16:31:42 [INFO] [subdomain1.mydomain.net] The server validated our request
2018/09/26 16:31:42 [INFO] [subdomain1.mydomain.net] acme: Validations succeeded; requesting certificates
2018/09/26 16:31:42 http: TLS handshake error from 70.176.85.69:63561: remote error: tls: bad certificate
2018/09/26 16:31:42 [INFO] [subdomain1.mydomain.net] Server responded with a certificate.
2018/09/26 16:31:42 [INFO] [*.subdomain2.mydomain.net] acme: Obtaining bundled SAN certificate
2018/09/26 16:31:43 [INFO] [*.subdomain2.mydomain.net] AuthURL: https://acme-staging-v02.api.letsencrypt.org/acme/authz/mP0GDa8a6Kh4dNn2Tri4SybZlu0TV...
2018/09/26 16:31:43 [INFO] [subdomain2.mydomain.net] acme: Preparing to solve DNS-01
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x30 pc=0x8d7a9f]

goroutine 1 [running]:
github.com/xenolf/lego/providers/dns/dnsimple.(*DNSProvider).Present(0xc000180660, 0xc00057c4a0, 0x18, 0xc0002d0090, 0x2b, 0xc0005201e0, 0x57, 0x0, 0xc000658f60)
	/Users/matt/Dev/src/github.com/xenolf/lego/providers/dns/dnsimple/dnsimple.go:99 +0x13f
github.com/xenolf/lego/acme.(*dnsChallenge).PreSolve(0xc00050e220, 0xc000068100, 0x71, 0xc000556628, 0x6, 0xc0005566c0, 0x7, 0xc0002d0090, 0x2b, 0x0, ...)
	/Users/matt/Dev/src/github.com/xenolf/lego/acme/dns_challenge.go:100 +0x17d
github.com/xenolf/lego/acme.(*Client).solveChallengeForAuthz(0xc0001360c0, 0xc000406600, 0x1, 0x1, 0x0, 0x0)
	/Users/matt/Dev/src/github.com/xenolf/lego/acme/client.go:603 +0x3c6
github.com/xenolf/lego/acme.(*Client).ObtainCertificate(0xc0001360c0, 0xc00056e4f0, 0x1, 0x1, 0x4b5a01, 0x0, 0x0, 0x0, 0xc00054c1e0, 0x0, ...)
	/Users/matt/Dev/src/github.com/xenolf/lego/acme/client.go:437 +0x2b3
```

I'm configuring the DNS provider like this:

```go
dnsimpleClient, err := dnsimple.NewDNSProviderCredentials(Config.DNS.Provider.DNSimpleOauthToken, Config.DNS.Provider.DNSimpleBaseURL)
if err != nil {
	return fmt.Errorf("constructing DNS provider: %v", err)
}
```

With no error.

Somehow, the config appears to be set to nil at some point.

**Here's my workaround/fix:**  reverting to an older commit before de3accf531a7dd4eff65ae20e9787fc62045b674, such as 6588bc6fa5b9d47abae1b6c40ef73e166f44fa95, resolves both errors: no NXDOMAIN and no panic:

```
2018/09/26 10:42:02 [INFO] [*.subdomain2.mydomain.net] acme: Obtaining bundled SAN certificate
2018/09/26 10:42:03 [INFO] [*.subdomain2.mydomain.net] AuthURL: https://acme-v02.api.letsencrypt.org/acme/authz/wRRYQyK5NOtKPgWJ64WraUKp3UwaF...
2018/09/26 10:42:03 [INFO] [subdomain2.mydomain.net] acme: Trying to solve DNS-01
2018/09/26 10:42:03 [INFO] [subdomain2.mydomain.net] Checking DNS record propagation using [127.0.0.53:53]
2018/09/26 10:42:15 [INFO] [subdomain2.mydomain.net] The server validated our request
2018/09/26 10:42:15 [INFO] [*.subdomain2.mydomain.net] acme: Validations succeeded; requesting certificates
2018/09/26 10:42:16 [INFO] [*.subdomain2.mydomain.net] Server responded with a certificate.
```

I suspect de3accf531a7dd4eff65ae20e9787fc62045b674 may be the problem (though I haven't verified this) because it introduces the the log message that is output just before the panic.

I haven't had time to narrow down which commit broke it since I have to recompile, re-upload, and re-run it all, which takes some time (I have very slow uplink). So some help or input from people with familiarity with the code and its recent changes would be appreciated!

Thanks for your great work on lego -- really awesome stuff. Probably one of the most useful libraries out there.

/cc @captncraig 

Pull request description:
dnsimple: fix challenge.


Fixes #649

and 
```
time="2018-10-08T15:49:23Z" level=error msg="Error in Go routine: runtime error: invalid memory address or nil pointer dereference"
goroutine 35 [running]:
runtime/debug.Stack(0x76b3c7, 0xc00009c280, 0x25bda3d)
   /usr/local/go/src/runtime/debug/stack.go:24 +0xa7
runtime/debug.PrintStack()
   /usr/local/go/src/runtime/debug/stack.go:16 +0x22
github.com/containous/traefik/safe.defaultRecoverGoroutine(0x215a8c0, 0x4435a20)
   /go/src/github.com/containous/traefik/safe/routine.go:148 +0x85
github.com/containous/traefik/safe.GoWithRecover.func1.1(0x266a948)
   /go/src/github.com/containous/traefik/safe/routine.go:139 +0x57
panic(0x215a8c0, 0x4435a20)
   /usr/local/go/src/runtime/panic.go:513 +0x1b9
github.com/containous/traefik/vendor/github.com/xenolf/lego/providers/dns/dnsimple.(*DNSProvider).Timeout(0xc0008beb30, 0x289b5a0, 0xc0008beb30)
   /go/src/github.com/containous/traefik/vendor/github.com/xenolf/lego/providers/dns/dnsimple/dnsimple.go:135 +0x8
github.com/containous/traefik/provider/acme.(*Provider).getClient(0xc000214cf0, 0x0, 0x0, 0x0)
   /go/src/github.com/containous/traefik/provider/acme/provider.go:280 +0x86e
github.com/containous/traefik/provider/acme.(*Provider).resolveCertificate(0xc000214cf0, 0xc000886f40, 0xd, 0xc0004f3230, 0x1, 0x1, 0xf43a01, 0x0, 0x0, 0x0)
   /go/src/github.com/containous/traefik/provider/acme/provider.go:398 +0x259
github.com/containous/traefik/provider/acme.(*Provider).Provide.func1()
   /go/src/github.com/containous/traefik/provider/acme/provider.go:184 +0x72
github.com/containous/traefik/safe.GoWithRecover.func1(0x266a948, 0xc000300f80)
   /go/src/github.com/containous/traefik/safe/routine.go:142 +0x4d
created by github.com/containous/traefik/safe.GoWithRecover
   /go/src/github.com/containous/traefik/safe/routine.go:136 +0x49
```

Patch:
diff --git a/providers/dns/dnsimple/dnsimple.go b/providers/dns/dnsimple/dnsimple.go
index 3b40dc5e..8f52cac6 100644
--- a/providers/dns/dnsimple/dnsimple.go
+++ b/providers/dns/dnsimple/dnsimple.go
@@ -78,7 +78,7 @@ func NewDNSProviderConfig(config *Config) (*DNSProvider, error) {
 		client.BaseURL = config.BaseURL
 	}
 
-	return &DNSProvider{client: client}, nil
+	return &DNSProvider{client: client, config: config}, nil
 }
 
 // Present creates a TXT record to fulfill the dns-01 challenge.
@@ -87,18 +87,18 @@ func (d *DNSProvider) Present(domain, token, keyAuth string) error {
 
 	zoneName, err := d.getHostedZone(domain)
 	if err != nil {
-		return err
+		return fmt.Errorf("dnsimple: %v", err)
 	}
 
 	accountID, err := d.getAccountID()
 	if err != nil {
-		return err
+		return fmt.Errorf("dnsimple: %v", err)
 	}
 
-	recordAttributes := d.newTxtRecord(zoneName, fqdn, value, d.config.TTL)
+	recordAttributes := newTxtRecord(zoneName, fqdn, value, d.config.TTL)
 	_, err = d.client.Zones.CreateRecord(accountID, zoneName, recordAttributes)
 	if err != nil {
-		return fmt.Errorf("API call failed: %v", err)
+		return fmt.Errorf("dnsimple: API call failed: %v", err)
 	}
 
 	return nil
@@ -110,22 +110,23 @@ func (d *DNSProvider) CleanUp(domain, token, keyAuth string) error {
 
 	records, err := d.findTxtRecords(domain, fqdn)
 	if err != nil {
-		return err
+		return fmt.Errorf("dnsimple: %v", err)
 	}
 
 	accountID, err := d.getAccountID()
 	if err != nil {
-		return err
+		return fmt.Errorf("dnsimple: %v", err)
 	}
 
+	var lastErr error
 	for _, rec := range records {
 		_, err := d.client.Zones.DeleteRecord(accountID, rec.ZoneID, rec.ID)
 		if err != nil {
-			return err
+			lastErr = fmt.Errorf("dnsimple: %v", err)
 		}
 	}
 
-	return nil
+	return lastErr
 }
 
 // Timeout returns the timeout and interval to use when checking for DNS propagation.
@@ -177,7 +178,7 @@ func (d *DNSProvider) findTxtRecords(domain, fqdn string) ([]dnsimple.ZoneRecord
 		return nil, err
 	}
 
-	recordName := d.extractRecordName(fqdn, zoneName)
+	recordName := extractRecordName(fqdn, zoneName)
 
 	result, err := d.client.Zones.ListRecords(accountID, zoneName, &dnsimple.ZoneRecordListOptions{Name: recordName, Type: "TXT", ListOptions: dnsimple.ListOptions{}})
 	if err != nil {
@@ -187,8 +188,8 @@ func (d *DNSProvider) findTxtRecords(domain, fqdn string) ([]dnsimple.ZoneRecord
 	return result.Data, nil
 }
 
-func (d *DNSProvider) newTxtRecord(zoneName, fqdn, value string, ttl int) dnsimple.ZoneRecord {
-	name := d.extractRecordName(fqdn, zoneName)
+func newTxtRecord(zoneName, fqdn, value string, ttl int) dnsimple.ZoneRecord {
+	name := extractRecordName(fqdn, zoneName)
 
 	return dnsimple.ZoneRecord{
 		Type:    "TXT",
@@ -198,7 +199,7 @@ func (d *DNSProvider) newTxtRecord(zoneName, fqdn, value string, ttl int) dnsimp
 	}
 }
 
-func (d *DNSProvider) extractRecordName(fqdn, domain string) string {
+func extractRecordName(fqdn, domain string) string {
 	name := acme.UnFqdn(fqdn)
 	if idx := strings.Index(name, "."+domain); idx != -1 {
 		return name[:idx]


Test patch:
diff --git a/providers/dns/dnsimple/dnsimple_test.go b/providers/dns/dnsimple/dnsimple_test.go
index c3780c0c..652a1832 100644
--- a/providers/dns/dnsimple/dnsimple_test.go
+++ b/providers/dns/dnsimple/dnsimple_test.go
@@ -38,84 +38,125 @@ func restoreEnv() {
 	os.Setenv("DNSIMPLE_BASE_URL", dnsimpleBaseURL)
 }
 
-//
-// NewDNSProvider
-//
-
-func TestNewDNSProviderValid(t *testing.T) {
-	defer restoreEnv()
-	os.Setenv("DNSIMPLE_OAUTH_TOKEN", "123")
-
-	acme.UserAgent = "lego"
-
-	provider, err := NewDNSProvider()
-
-	assert.NotNil(t, provider)
-	assert.Equal(t, "lego", provider.client.UserAgent)
-	assert.NoError(t, err)
-}
-
-func TestNewDNSProviderValidWithBaseUrl(t *testing.T) {
-	defer restoreEnv()
-	os.Setenv("DNSIMPLE_OAUTH_TOKEN", "123")
-	os.Setenv("DNSIMPLE_BASE_URL", "https://api.dnsimple.test")
-
-	provider, err := NewDNSProvider()
-
-	assert.NotNil(t, provider)
-	assert.NoError(t, err)
-
-	assert.Equal(t, provider.client.BaseURL, "https://api.dnsimple.test")
-}
-
-func TestNewDNSProviderInvalidWithMissingOauthToken(t *testing.T) {
-	defer restoreEnv()
-	os.Setenv("DNSIMPLE_OAUTH_TOKEN", "")
-
-	provider, err := NewDNSProvider()
+func TestNewDNSProvider(t *testing.T) {
+	testCases := []struct {
+		desc      string
+		userAgent string
+		envVars   map[string]string
+		expected  string
+	}{
+		{
+			desc:      "success",
+			userAgent: "lego",
+			envVars: map[string]string{
+				"DNSIMPLE_OAUTH_TOKEN": "my_token",
+			},
+		},
+		{
+			desc: "success: base url",
+			envVars: map[string]string{
+				"DNSIMPLE_OAUTH_TOKEN": "my_token",
+				"DNSIMPLE_BASE_URL":    "https://api.dnsimple.test",
+			},
+		},
+		{
+			desc: "missing oauth token",
+			envVars: map[string]string{
+				"DNSIMPLE_OAUTH_TOKEN": "",
+			},
+			expected: "dnsimple: OAuth token is missing",
+		},
+	}
 
-	assert.Nil(t, provider)
-	assert.EqualError(t, err, "dnsimple: OAuth token is missing")
+	for _, test := range testCases {
+		t.Run(test.desc, func(t *testing.T) {
+			defer restoreEnv()
+			for key, value := range test.envVars {
+				if len(value) == 0 {
+					os.Unsetenv(key)
+				} else {
+					os.Setenv(key, value)
+				}
+			}
+
+			if test.userAgent != "" {
+				acme.UserAgent = test.userAgent
+			}
+
+			p, err := NewDNSProvider()
+
+			if len(test.expected) == 0 {
+				require.NoError(t, err)
+				require.NotNil(t, p)
+				require.NotNil(t, p.config)
+				require.NotNil(t, p.client)
+
+				baseURL := os.Getenv("DNSIMPLE_BASE_URL")
+				if baseURL != "" {
+					assert.Equal(t, baseURL, p.client.BaseURL)
+				}
+
+				if test.userAgent != "" {
+					assert.Equal(t, "lego", p.client.UserAgent)
+				}
+
+			} else {
+				require.EqualError(t, err, test.expected)
+			}
+		})
+	}
 }
 
-//
-// NewDNSProviderCredentials
-//
-
-func TestNewDNSProviderCredentialsValid(t *testing.T) {
-	config := NewDefaultConfig()
-	config.AccessToken = "123"
-	config.BaseURL = ""
-
-	provider, err := NewDNSProviderConfig(config)
-	require.NoError(t, err)
-	require.NotNil(t, provider)
-
-	assert.Equal(t, "lego", provider.client.UserAgent)
-	assert.NoError(t, err)
-}
+func TestNewDNSProviderConfig(t *testing.T) {
+	testCases := []struct {
+		desc        string
+		accessToken string
+		baseURL     string
+		expected    string
+	}{
+		{
+			desc:        "success",
+			accessToken: "my_token",
+			baseURL:     "",
+		},
+		{
+			desc:        "success: base url",
+			accessToken: "my_token",
+			baseURL:     "https://api.dnsimple.test",
+		},
+		{
+			desc:     "missing oauth token",
+			expected: "dnsimple: OAuth token is missing",
+		},
+	}
 
-func TestNewDNSProviderCredentialsValidWithBaseUrl(t *testing.T) {
-	config := NewDefaultConfig()
-	config.AccessToken = "123"
-	config.BaseURL = "https://api.dnsimple.test"
+	for _, test := range testCases {
+		t.Run(test.desc, func(t *testing.T) {
+			defer restoreEnv()
+			os.Unsetenv("DNSIMPLE_OAUTH_TOKEN")
+			os.Unsetenv("DNSIMPLE_BASE_URL")
 
-	provider, err := NewDNSProviderConfig(config)
-	require.NoError(t, err)
-	require.NotNil(t, provider)
+			config := NewDefaultConfig()
+			config.AccessToken = test.accessToken
+			config.BaseURL = test.baseURL
 
-	assert.Equal(t, provider.client.BaseURL, "https://api.dnsimple.test")
-}
+			p, err := NewDNSProviderConfig(config)
 
-func TestNewDNSProviderCredentialsInvalidWithMissingOauthToken(t *testing.T) {
-	config := NewDefaultConfig()
-	config.AccessToken = ""
-	config.BaseURL = ""
+			if len(test.expected) == 0 {
+				require.NoError(t, err)
+				require.NotNil(t, p)
+				require.NotNil(t, p.config)
+				require.NotNil(t, p.client)
 
-	provider, err := NewDNSProviderConfig(config)
+				if test.baseURL != "" {
+					assert.Equal(t, test.baseURL, p.client.BaseURL)
+				}
 
-	assert.Nil(t, provider)
-	assert.EqualError(t, err, "dnsimple: OAuth token is missing")
+			} else {
+				require.EqualError(t, err, test.expected)
+			}
+		})
+	}
 }
 
 //