[deSEC] nil pointer dereference on http.Client error

[JenswBE]

NOTE: Have a general question? You'll get a better response on the dnscontrol-discuss email list!

Describe the bug The HTTP response is accessed before checking the error response at: https://github.com/StackExchange/dnscontrol/blob/355c0fe6a1edb2c04fe56f2fc482bb6f90fabebc/providers/desec/protocol.go#L71-L79

Due to this, if the request fails at the http.Client inside c.get (e.g. net/http: invalid header field value), you get a nil pointer dereference instead of graceful handling.

Full trace

``` panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x10 pc=0xd1690b] goroutine 1 [running]: github.com/StackExchange/dnscontrol/v4/providers/desec.(*desecProvider).authenticate(0x1781f40?) /home/runner/work/dnscontrol/dnscontrol/providers/desec/protocol.go:74 +0x2b github.com/StackExchange/dnscontrol/v4/providers/desec.NewDeSec(0xc000fdccc0, {0xc0006b42a0?, 0xc000fdae5a?, 0x5?}) /home/runner/work/dnscontrol/dnscontrol/providers/desec/desecProvider.go:28 +0x85 github.com/StackExchange/dnscontrol/v4/providers.CreateDNSProvider({0xc000fdae5a?, 0xc000fdcc00?}, 0xc000fdccc0, {0x0, 0x0, 0x0}) /home/runner/work/dnscontrol/dnscontrol/providers/providers.go:102 +0x88 github.com/StackExchange/dnscontrol/v4/commands.InitializeProviders(0xc000e618c0, 0xc000fdcc00, 0x0?) /home/runner/work/dnscontrol/dnscontrol/commands/previewPush.go:366 +0x891 github.com/StackExchange/dnscontrol/v4/commands.run({{{{0x1b0daed, 0xc}, {0x0, 0x0}, 0x0, {{...}, {...}, 0x0, 0x0}}, {0x0, ...}}, ...}, ...) /home/runner/work/dnscontrol/dnscontrol/commands/previewPush.go:163 +0x1a5 github.com/StackExchange/dnscontrol/v4/commands.Preview(...) /home/runner/work/dnscontrol/dnscontrol/commands/previewPush.go:134 github.com/StackExchange/dnscontrol/v4/commands.init.func8.1(0xc0008dfe00?) /home/runner/work/dnscontrol/dnscontrol/commands/previewPush.go:32 +0xa8 github.com/urfave/cli/v2.(*Command).Run(0xc00016fb80, 0xc0008dfe00, {0xc0008f24e0, 0x1, 0x1}) /home/runner/go/pkg/mod/github.com/urfave/cli/v2@v2.27.2/command.go:276 +0x97d github.com/urfave/cli/v2.(*Command).Run(0xc00078ab00, 0xc0008dfbc0, {0xc00003e040, 0x2, 0x2}) /home/runner/go/pkg/mod/github.com/urfave/cli/v2@v2.27.2/command.go:269 +0xbb7 github.com/urfave/cli/v2.(*App).RunContext(0xc0008e0400, {0x1deba50, 0x29c3fa0}, {0xc00003e040, 0x2, 0x2}) /home/runner/go/pkg/mod/github.com/urfave/cli/v2@v2.27.2/app.go:333 +0x58b github.com/urfave/cli/v2.(*App).Run(...) /home/runner/go/pkg/mod/github.com/urfave/cli/v2@v2.27.2/app.go:307 github.com/StackExchange/dnscontrol/v4/commands.Run({0xc00011c480?, 0x1b1a24d?}) /home/runner/work/dnscontrol/dnscontrol/commands/commands.go:105 +0x445 main.main() /home/runner/work/dnscontrol/dnscontrol/main.go:30 +0xad ```

To Reproduce Steps to reproduce the behavior:

1. Set a deSEC token value ending with a newline
2. Run dnscontrol preview

Expected behavior

The returned error of c.get should be checked before accessing resp.

DNS Provider

• deSEC

Additional context Add any other context about the problem here.

[JenswBE]

As a secondary question, is validation of the token really needed in NewDeSec? Because in other "simple" providers like Hetzner and SimpleDNS, they don't seem to validate the token upfront. As I assume it will fail anyway when doing the first API request.

If upfront validation is really required, could it be worth to consider checking an endpoint which is accessible by any token? E.g. https://desec.io/api/v1/domains/. But likely it's the first call being executed anyway to get the current status. Circling back to above question on the need of the c.authenticate() function.

[tlimoncelli]

CC @D3luxee maintainer of DESEC

[tlimoncelli]

On the nil issue: I agree. Those should be reversed.

On the token validation:

You are correct. Providers shouldn't talk to the network in a setup function like NewDeSec(). I guess I missed that when reviewing the original PR.

c.authenticate() and c.initializeDomainIndex() shouldn't be called in NewDeSec(). Removing them will probably require more than just simply deleting them.

[JenswBE]

Thanks for the validation of the issue! I would be happy to submit a PR in the coming days (depending on the time the integration tests take for deSEC).

[tlimoncelli]

A PR would be appreciated!