Panic on ZeroSSL API Issuer when no `Storage` is set

mbardelmeijer commented 1 month ago

What version of the package are you using?

v0.21.2

What are you trying to do?

We're trying to implement the newly added ZeroSSL API issuer, to allow SSL certificates for IP addresses.

What steps did you take?

We've implemented the issuer conform to the documentation and PR changes. In it's most basic form:

magic.Issuers = append(magic.Issuers, &certmagic.ZeroSSLIssuer{
            APIKey: config.Certificates.ZeroSSLAPIKey,
            Logger: logger.With(zap.String("component", "zerossl_api_issuer")),
})

We handle the distributed HTTP challenge like:

func (s *httpServer) handleCertificateHttpValidation(res http.ResponseWriter, req *http.Request) bool {
    if certmagic.LooksLikeHTTPChallenge(req) {
        for _, issuer := range *s.issuers {
            if am, ok := issuer.(*certmagic.ACMEIssuer); ok {
                if am.HandleHTTPChallenge(res, req) {
                    log.Debugf("handled ACME HTTP challenge for %s", req.Host)

                    return true
                }
            }
        }

        return false
    }

    if certmagic.LooksLikeZeroSSLHTTPValidation(req) {
        for _, issuer := range *s.issuers {
            if am, ok := issuer.(*certmagic.ZeroSSLIssuer); ok {
                if am.HandleZeroSSLHTTPValidation(res, req) {
                    log.Debugf("handled ZeroSSL HTTP challenge for %s", req.Host)

                    return true
                }
            }
        }

        return false
    }

    return false
}

What did you expect to happen, and what actually happened instead?

2024-05-24T19:08:52.443Z        INFO    obtain  obtaining certificate   {"component": "acme", "identifier": "IPADDRESS"}
2024-05-24T19:08:52.445Z        DEBUG   obtain  trying issuer 1/2       {"component": "acme", "issuer": "acme-v02.api.letsencrypt.org-directory"}
2024-05-24T19:08:52.446Z        DEBUG   obtain  trying issuer 2/2       {"component": "acme", "issuer": "zerossl"}
2024-05-24T19:08:52.446Z        INFO    creating certificate    {"component": "zerossl_api_issuer", "identifiers": ["IPADDRESS"]}
2024-05-24T19:08:53.642Z        INFO    created certificate     {"component": "zerossl_api_issuer", "identifiers": ["IPADDRESS"], "cert_id": "CERTID"}
2024-05-24T19:08:53.643Z        INFO    validating identifiers  {"component": "zerossl_api_issuer", "identifiers": ["IPADDRESS"], "cert_id": "CERTID", "verification_method": "HTTP_CSR_HASH"}
2024/05/24 19:08:54 notifying bugsnag: runtime error: invalid memory address or nil pointer dereference
2024-05-24T19:08:54.224Z        DEBUG   stdlib  http/server.go:3411     http: panic serving 34.194.10.63:27608: runtime error: invalid memory address or nil pointer dereference
goroutine 306 [running]:
net/http.(*conn).serve.func1()
        /usr/local/go/src/net/http/server.go:1898 +0xbe
panic({0x114c640?, 0x1c6ccb0?})
        /usr/local/go/src/runtime/panic.go:770 +0x132
github.com/bugsnag/bugsnag-go/v2.(*Notifier).AutoNotify(0xc007bd80c0, {0xc000a791d8, 0x2, 0x2})
        /home/michel/go/pkg/mod/github.com/bugsnag/bugsnag-go/v2@v2.4.0/notifier.go:102 +0x545
panic({0x114c640?, 0x1c6ccb0?})
        /usr/local/go/src/runtime/panic.go:770 +0x132
github.com/caddyserver/certmagic.(*ZeroSSLIssuer).getDistributedValidationInfo(0xc007a72f00, {0x14bb350, 0xc000491590}, {0xc004f85bb0, 0xe})
        /home/michel/go/pkg/mod/github.com/caddyserver/certmagic@v0.21.2/zerosslissuer.go:275 +0x112
github.com/caddyserver/certmagic.(*ZeroSSLIssuer).distributedHTTPValidationAnswer(0xc007a72f00, {0x14b9cc8, 0xc007bd29a0}, 0xc00221afc0)
        /home/michel/go/pkg/mod/github.com/caddyserver/certmagic@v0.21.2/httphandlers.go:166 +0x158
github.com/caddyserver/certmagic.(*ZeroSSLIssuer).HandleZeroSSLHTTPValidation(0xc007a72f00?, {0x14b9cc8?, 0xc007bd29a0?}, 0xc00221afc0?)
        /home/michel/go/pkg/mod/github.com/caddyserver/certmagic@v0.21.2/httphandlers.go:154 +0xa9
main.(*httpServer).handleCertificateHttpValidation(0xc007a59440, {0x14b9cc8, 0xc007bd29a0}, 0xc00221afc0)
        /home/michel/code/http.go:236 +0x166
main.(*httpServer).serveRequest(0xc007a59440, {0x14b9cc8, 0xc007bd29a0}, 0xc00221afc0)
        /home/michel/code/http.go:255 +0xb4
main.newHttpServerRateLimiter.func1.1({0x14b9cc8, 0xc007bd29a0}, 0xc00221afc0)
        /home/michel/code/http.go:157 +0xeb
net/http.HandlerFunc.ServeHTTP(0x30?, {0x14b9cc8?, 0xc007bd29a0?}, 0xc000071008?)
        /usr/local/go/src/net/http/server.go:2166 +0x29
net/http.HandlerFunc.ServeHTTP(0xc007a65380?, {0x14b9cc8?, 0xc007bd29a0?}, 0xc007acda78?)
        /usr/local/go/src/net/http/server.go:2166 +0x29
net/http.(*ServeMux).ServeHTTP(0x14bb388?, {0x14b9cc8, 0xc007bd29a0}, 0xc00221afc0)
        /usr/local/go/src/net/http/server.go:2683 +0x1ad
github.com/bugsnag/bugsnag-go/v2.Handler.func1({0x14b9cc8, 0xc007bd29a0}, 0xc00221aea0)
        /home/michel/go/pkg/mod/github.com/bugsnag/bugsnag-go/v2@v2.4.0/bugsnag.go:188 +0x238
net/http.HandlerFunc.ServeHTTP(0x473779?, {0x14b9cc8?, 0xc007bd29a0?}, 0xc007acdb68?)
        /usr/local/go/src/net/http/server.go:2166 +0x29
net/http.serverHandler.ServeHTTP({0xc0004914d0?}, {0x14b9cc8?, 0xc007bd29a0?}, 0x6?)
        /usr/local/go/src/net/http/server.go:3137 +0x8e
net/http.(*conn).serve(0xc002501a70, {0x14bb350, 0xc007bd6870})
        /usr/local/go/src/net/http/server.go:2039 +0x5e8
created by net/http.(*Server).Serve in goroutine 116
        /usr/local/go/src/net/http/server.go:3285 +0x4b4
2024-05-24T19:08:55.449Z        INFO    canceled certificate    {"component": "zerossl_api_issuer", "identifiers": ["IPADDRESS"], "cert_id": "CERTID", "verification_method": "HTTP_CSR_HASH"}

How do you think this should be fixed?

Setting the Storage on the issuer solves the issue. I would expect the magic.Storage which is set globally to be used, like the ACME issuers.

Please link to any related issues, pull requests, and/or discussion

https://github.com/caddyserver/certmagic/pull/279

mbardelmeijer commented 1 month ago

From what I can see debugging further, the ACME issuer's are created with certmagic.NewACMEIssuer, which forward some parameters to the underlying issuer. For ZeroSSL there isn't such method from what I can see.

What's recommend, setting the Storage itself, or would a certmagic.NewZeroSSLIssuer helper method be preferred?

mholt commented 1 month ago

Ohh good find. Set the storage. I'll update things when I'm back at my desk. The package doesn't have a default ZeroSSL issuer like it does ACME issuer since you'll always need to provide an API key, but that kind of thing isn't required to use ACME. So setting a default storage for ZeroSSL may not make sense.

And I hate constructor functions when we can avoid them 😅

mholt commented 1 month ago

For now, I just made the Storage field required (and documented it as such) since it's simpler to store the verification info in storage whether distributed or not. In the future if I have more time (or a sponsor needs it), I can work on an implementation that doesn't require a Storage value.

Thanks for the issue!

caddyserver / certmagic