Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple TestServerBehaviour tests fail with Go 1.21 #187

Closed
dswarbrick opened this issue Dec 12, 2023 · 7 comments
Closed

Multiple TestServerBehaviour tests fail with Go 1.21 #187

dswarbrick opened this issue Dec 12, 2023 · 7 comments

Comments

@dswarbrick
Copy link
Contributor

Tested latest tag v0.11.0 with Go 1.21.1 on Ubuntu 23.10, and Go 1.21.5 on Debian sid.

$ go test ./...
?       github.com/prometheus/exporter-toolkit/web/kingpinflag  [no test files]
2023/12/12 18:33:28 http: TLS handshake error from 127.0.0.1:51088: EOF
2023/12/12 18:33:29 http: TLS handshake error from 127.0.0.1:51114: tls: no cipher suite supported by both client and server
2023/12/12 18:33:30 http: TLS handshake error from 127.0.0.1:51160: tls: no ECDHE curve supported by both client and server
2023/12/12 18:33:30 http: TLS handshake error from 127.0.0.1:38214: tls: client didn't provide a certificate
2023/12/12 18:33:30 http: TLS handshake error from 127.0.0.1:38202: tls: client didn't provide a certificate
2023/12/12 18:33:32 http: TLS handshake error from 127.0.0.1:38260: tls: client didn't provide a certificate
2023/12/12 18:33:32 http: TLS handshake error from 127.0.0.1:38288: tls: failed to verify certificate: x509: certificate signed by unknown authority
2023/12/12 18:33:33 http: TLS handshake error from 127.0.0.1:38306: could not find allowed SANs in client cert, found: [bad]
--- FAIL: TestServerBehaviour (6.29s)
    --- FAIL: TestServerBehaviour/valid_tls_config_yml_and_tls_client_with_RequireAnyClientCert (0.26s)
        tls_config_test.go:572: Expected error matching regular expression: bad certificate
        tls_config_test.go:573: Got: Get "https://localhost:41327": remote error: tls: certificate required
    --- FAIL: TestServerBehaviour/valid_tls_config_yml_and_tls_client_with_RequireAndVerifyClientCert (0.27s)
        tls_config_test.go:572: Expected error matching regular expression: bad certificate
        tls_config_test.go:573: Got: Get "https://localhost:41327": remote error: tls: certificate required
    --- FAIL: TestServerBehaviour/valid_tls_config_yml_and_tls_client_with_RequireAndVerifyClientCert_(present_wrong_certificate) (0.26s)
        tls_config_test.go:572: Expected error matching regular expression: bad certificate
        tls_config_test.go:573: Got: Get "https://localhost:41327": remote error: tls: unknown certificate authority
2023/12/12 18:33:33 http: TLS handshake error from 127.0.0.1:38318: tls: client didn't provide a certificate
FAIL
FAIL    github.com/prometheus/exporter-toolkit/web      10.339s
FAIL
@dswarbrick
Copy link
Contributor Author

I think this is the same as #171.

@dswarbrick dswarbrick changed the title Multiple TestServerBehaviour tests fail Multiple TestServerBehaviour tests fail with Go 1.21 Dec 12, 2023
@dswarbrick
Copy link
Contributor Author

All tests pass successfully when tested with Go 1.20.8.

@SuperQ
Copy link
Member

SuperQ commented Dec 12, 2023

Yea, I noticed that in #183.

@dswarbrick
Copy link
Contributor Author

@SuperQ I'll take a look at refactoring the tests as I described in #171 (comment) and submit a PR.

@dswarbrick
Copy link
Contributor Author

The code responsible for the change in behaviour in Go 1.21 is in the processCertsFromClient function in the crypto/tls package. The Go 1.21 code is:

        chains, err := certs[0].Verify(opts)
        if err != nil {
            var errCertificateInvalid x509.CertificateInvalidError
            if errors.As(err, &x509.UnknownAuthorityError{}) {
                c.sendAlert(alertUnknownCA)
            } else if errors.As(err, &errCertificateInvalid) && errCertificateInvalid.Reason == x509.Expired {
                c.sendAlert(alertCertificateExpired)
            } else {
                c.sendAlert(alertBadCertificate)
            }   
            return &CertificateVerificationError{UnverifiedCertificates: certs, Err: err}
        }

... compared to the more simplistic error handling in Go 1.20:

        chains, err := certs[0].Verify(opts)
        if err != nil {
            c.sendAlert(alertBadCertificate)
            return &CertificateVerificationError{UnverifiedCertificates: certs, Err: err}
        }

In theory, we should be able to use errors.As(err, &cvErr) where cvErr is a *tls.CertificateVerificationError. However, the error type returned in the following test failure is a *url.Error:

    --- FAIL: TestServerBehaviour/valid_tls_config_yml_and_tls_client_with_RequireAndVerifyClientCert_(present_wrong_certificate) (0.26s)
        tls_config_test.go:572: Expected error matching regular expression: bad certificate
        tls_config_test.go:573: Got: Get "https://localhost:42333": remote error: tls: unknown certificate authority

Unwrapping that we get *tls.permanentError, then *net.OpError, and finally tls.alert (from whence the "unknown certificate authority" string comes). Since tls.alert is not an exported type, we can't use it in a errors.As(...) test.

I haven't dug deep enough yet to see whether the actual test code is somehow munging the original CertificateVerificationError that should be in the error chain.

@dswarbrick
Copy link
Contributor Author

dswarbrick commented Dec 12, 2023

A little more investigation reveals that these errors are originating from client.Do(req) in the ClientConnection function in tls_config_test.go. Go docs say this about Client.Do():

Any returned error will be of type *url.Error. ...

So the CertificateVerificationError would appear to be on the server side, and not the client side, which is where we are capturing the error.

I'm not sure there is much we can do to improve this, other than implement Go version-specific error message patterns to match in the tests.

dswarbrick added a commit to dswarbrick/exporter-toolkit that referenced this issue Dec 12, 2023
Go 1.21 introduced more granular / specific error strings for certain
"bad certificate" scenarios[1]. As such, we need to split these tests
and conditionally compile based on build tags.

Fixes: prometheus#171, prometheus#187

[1]: golang/go@62a9948

Signed-off-by: Daniel Swarbrick <daniel.swarbrick@gmail.com>
SuperQ added a commit that referenced this issue Dec 20, 2023
* Split TLS client tests to handle Go 1.21+ error strings

Go 1.21 introduced more granular / specific error strings for certain
"bad certificate" scenarios[1]. As such, we need to split these tests
and conditionally compile based on build tags.

Fixes: #171, #187

[1]: golang/go@62a9948

---------

Signed-off-by: Daniel Swarbrick <daniel.swarbrick@gmail.com>
Co-authored-by: Ben Kochie <superq@gmail.com>
@dswarbrick
Copy link
Contributor Author

Closed via #188

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants