From 0ef98f05584f4a30fb8cef7eee78a0ad45354ce9 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Fri, 19 Apr 2024 11:15:59 -0700 Subject: [PATCH] Use new helpers to assert that all webhook dials use ptls settings --- .../jwtcachefiller/jwtcachefiller.go | 1 - .../webhookcachefiller_test.go | 27 +++++++----- internal/testutil/tlsserver/tlsserver.go | 43 ++++++++++++++++--- 3 files changed, 54 insertions(+), 17 deletions(-) diff --git a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go index 951db2bfa..eda1fd78c 100644 --- a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go +++ b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go @@ -66,7 +66,6 @@ const ( reasonInvalidTLSConfiguration = "InvalidTLSConfiguration" reasonInvalidDiscoveryProbe = "InvalidDiscoveryProbe" reasonInvalidAuthenticator = "InvalidAuthenticator" - reasonInvalidTokenSigningFailure = "InvalidTokenSigningFailure" reasonInvalidCouldNotFetchJWKS = "InvalidCouldNotFetchJWKS" msgUnableToValidate = "unable to validate; see other conditions for details" diff --git a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go index 3664e1ef4..a846c9577 100644 --- a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go +++ b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go @@ -92,18 +92,21 @@ func TestController(t *testing.T) { // only expecting dials, which will not get into handler func }), func(s *httptest.Server) { s.TLS.Certificates = []tls.Certificate{*hostAsLocalhostServingCert} + tlsserver.AssertEveryTLSHello(t, s, ptls.Default) // assert on every hello because we are only expecting dials }) hostAs127001WebhookServer, _ := tlsserver.TestServerIPv4(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // only expecting dials, which will not get into handler func }), func(s *httptest.Server) { s.TLS.Certificates = []tls.Certificate{*hostAs127001ServingCert} + tlsserver.AssertEveryTLSHello(t, s, ptls.Default) // assert on every hello because we are only expecting dials }) hostLocalWithExampleDotComCertServer, _ := tlsserver.TestServerIPv4(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // only expecting dials, which will not get into handler func }), func(s *httptest.Server) { s.TLS.Certificates = []tls.Certificate{*localButExampleDotComServerCert} + tlsserver.AssertEveryTLSHello(t, s, ptls.Default) // assert on every hello because we are only expecting dials }) hostLocalIPv6Server, ipv6CA := tlsserver.TestServerIPv6(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}), tlsserver.RecordTLSHello) @@ -116,7 +119,9 @@ func TestController(t *testing.T) { })) hostGoodDefaultServingCertServer, _ := tlsserver.TestServerIPv4(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { mux.ServeHTTP(w, r) - }), nil) + }), func(s *httptest.Server) { + tlsserver.AssertEveryTLSHello(t, s, ptls.Default) // assert on every hello because we are only expecting dials + }) goodWebhookDefaultServingCertEndpoint := hostGoodDefaultServingCertServer.URL goodWebhookDefaultServingCertEndpointBut404 := goodWebhookDefaultServingCertEndpoint + "/nothing/here" @@ -1398,14 +1403,14 @@ func TestNewWebhookAuthenticator(t *testing.T) { ) tests := []struct { - name string - endpoint string - pemBytes []byte - prereqOk bool - wantConditions []*metav1.Condition - wantWebhook bool - wantErr string - userCreatedWebhookClientToCallWebhookEndpoint bool + name string + endpoint string + pemBytes []byte + prereqOk bool + wantConditions []*metav1.Condition + wantErr string + wantWebhook bool // When true, we want a webhook client to have been successfully created. + callWebhook bool // When true, really call the webhook endpoint using the created webhook client. }{ { name: "prerequisites not ready, cannot create webhook authenticator", @@ -1455,7 +1460,7 @@ func TestNewWebhookAuthenticator(t *testing.T) { Message: "authenticator initialized", }}, wantWebhook: true, - userCreatedWebhookClientToCallWebhookEndpoint: true, + callWebhook: true, }, } @@ -1480,7 +1485,7 @@ func TestNewWebhookAuthenticator(t *testing.T) { require.NoError(t, err) } - if tt.userCreatedWebhookClientToCallWebhookEndpoint { + if tt.callWebhook { authResp, isAuthenticated, err := webhook.AuthenticateToken(context.Background(), "test-token") require.NoError(t, err) require.True(t, isAuthenticated) diff --git a/internal/testutil/tlsserver/tlsserver.go b/internal/testutil/tlsserver/tlsserver.go index b1ad27494..a6b3f3457 100644 --- a/internal/testutil/tlsserver/tlsserver.go +++ b/internal/testutil/tlsserver/tlsserver.go @@ -103,6 +103,9 @@ func TLSTestServerWithCert(t *testing.T, handler http.HandlerFunc, certificate * return l.Addr().String() } +// RecordTLSHello configures the server to record client TLS negotiation info onto each incoming request, +// so that the details of the client's TLS settings can be asserted upon during request handling +// using AssertTLS. func RecordTLSHello(server *httptest.Server) { server.Config.ConnContext = func(ctx context.Context, _ net.Conn) context.Context { return context.WithValue(ctx, mapKey, &sync.Map{}) @@ -120,6 +123,29 @@ func RecordTLSHello(server *httptest.Server) { } } +// AssertEveryTLSHello can be used to make assertions about the client's TLS configuration +// when a test expects the server to be dialed by the client, but does not expect that http +// requests will be performed (and thus assertions cannot be made during request handling). +// For example, a test could use this when the production code is only going to perform a +// tls.Dial to test the connection to the server, without making any actual requests. +func AssertEveryTLSHello(t *testing.T, server *httptest.Server, clientTLSConfigFunc ptls.ConfigFunc) { + server.TLS.GetConfigForClient = func(info *tls.ClientHelloInfo) (*tls.Config, error) { + t.Helper() + + // We don't know yet at this point if the request will be an upgrade request, so as a shortcut, + // we won't assert on that. When that a concern, use AssertTLS in the request handler instead. + assertionsPassed := assertTLSOnHello(t, info, false, clientTLSConfigFunc) + + if !assertionsPassed { + t.Errorf("insecure client TLS hello detected during TLS negotiation") + } + + return nil, nil + } +} + +// AssertTLS makes assertions on a particular incoming http request, assuming that the server +// was configured using RecordTLSHello. func AssertTLS(t *testing.T, r *http.Request, clientTLSConfigFunc ptls.ConfigFunc) { t.Helper() @@ -132,6 +158,16 @@ func AssertTLS(t *testing.T, r *http.Request, clientTLSConfigFunc ptls.ConfigFun actualClientHello, ok := h.(*tls.ClientHelloInfo) require.True(t, ok) + assertionsPassed := assertTLSOnHello(t, actualClientHello, httpstream.IsUpgradeRequest(r), clientTLSConfigFunc) + + if !assertionsPassed { + t.Errorf("insecure TLS detected for %q %q %q upgrade=%v", r.Proto, r.Method, r.URL.String(), httpstream.IsUpgradeRequest(r)) + } +} + +func assertTLSOnHello(t *testing.T, actualClientHello *tls.ClientHelloInfo, isUpgradeRequest bool, clientTLSConfigFunc ptls.ConfigFunc) bool { + t.Helper() + clientTLSConfig := clientTLSConfigFunc(nil) var wantClientSupportedVersions []uint16 @@ -155,7 +191,7 @@ func AssertTLS(t *testing.T, r *http.Request, clientTLSConfigFunc ptls.ConfigFun } wantClientProtos := clientTLSConfig.NextProtos - if httpstream.IsUpgradeRequest(r) { + if isUpgradeRequest { wantClientProtos = clientTLSConfig.NextProtos[1:] } @@ -164,10 +200,7 @@ func AssertTLS(t *testing.T, r *http.Request, clientTLSConfigFunc ptls.ConfigFun ok2 := assert.Equal(t, cipherSuiteIDsToStrings(wantClientSupportedCiphers), cipherSuiteIDsToStrings(actualClientHello.CipherSuites)) ok3 := assert.Equal(t, wantClientProtos, actualClientHello.SupportedProtos) - if all := ok1 && ok2 && ok3; !all { - t.Errorf("insecure TLS detected for %q %q %q upgrade=%v wantClientSupportedVersions=%v wantClientSupportedCiphers=%v wantClientProtos=%v", - r.Proto, r.Method, r.URL.String(), httpstream.IsUpgradeRequest(r), ok1, ok2, ok3) - } + return ok1 && ok2 && ok3 } // appendIfNotAlreadyIncluded only adds the newItems to the list if they are not already included