diff --git a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go index d1bc470a8..80915ae9f 100644 --- a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go +++ b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go @@ -10,7 +10,7 @@ import ( "crypto/x509" "fmt" "net/url" - "os" + "time" k8sauthv1beta1 "k8s.io/api/authentication/v1beta1" "k8s.io/apimachinery/pkg/api/equality" @@ -19,10 +19,8 @@ import ( errorsutil "k8s.io/apimachinery/pkg/util/errors" k8snetutil "k8s.io/apimachinery/pkg/util/net" "k8s.io/apiserver/pkg/authentication/authenticator" - webhookutil "k8s.io/apiserver/pkg/util/webhook" "k8s.io/apiserver/plugin/pkg/authenticator/token/webhook" - "k8s.io/client-go/tools/clientcmd" - clientcmdapi "k8s.io/client-go/tools/clientcmd/api" + "k8s.io/client-go/rest" "k8s.io/klog/v2" "k8s.io/utils/clock" @@ -36,6 +34,7 @@ import ( "go.pinniped.dev/internal/controllerlib" "go.pinniped.dev/internal/crypto/ptls" "go.pinniped.dev/internal/endpointaddr" + "go.pinniped.dev/internal/kubeclient" "go.pinniped.dev/internal/plog" ) @@ -49,9 +48,7 @@ const ( reasonSuccess = "Success" reasonNotReady = "NotReady" reasonUnableToValidate = "UnableToValidate" - reasonUnableToCreateTempFile = "UnableToCreateTempFile" - reasonUnableToMarshallKubeconfig = "UnableToMarshallKubeconfig" - reasonUnableToLoadKubeconfig = "UnableToLoadKubeconfig" + reasonUnableToCreateClient = "UnableToCreateClient" reasonUnableToInstantiateWebhook = "UnableToInstantiateWebhook" reasonInvalidTLSConfiguration = "InvalidTLSConfiguration" reasonInvalidEndpointURL = "InvalidEndpointURL" @@ -95,7 +92,7 @@ type webhookCacheFillerController struct { client conciergeclientset.Interface clock clock.Clock log plog.Logger - tlsDialerFunc func(network string, addr string, config *tls.Config) (*tls.Conn, error) + tlsDialerFunc func(network string, addr string, config *tls.Config) (*tls.Conn, error) // for mocking, use tls.Dial in prod } // Sync implements controllerlib.Syncer. @@ -106,25 +103,25 @@ func (c *webhookCacheFillerController) Sync(ctx controllerlib.Context) error { return nil } if err != nil { + // no unit test for this failure return fmt.Errorf("failed to get WebhookAuthenticator %s/%s: %w", ctx.Key.Namespace, ctx.Key.Name, err) } conditions := make([]*metav1.Condition, 0) - specCopy := obj.Spec.DeepCopy() var errs []error - certPool, pemBytes, conditions, tlsBundleOk := c.validateTLSBundle(specCopy.TLS, conditions) - endpointHostPort, conditions, endpointOk := c.validateEndpoint(specCopy.Endpoint, conditions) + certPool, pemBytes, conditions, tlsBundleOk := c.validateTLSBundle(obj.Spec.TLS, conditions) + endpointHostPort, conditions, endpointOk := c.validateEndpoint(obj.Spec.Endpoint, conditions) okSoFar := tlsBundleOk && endpointOk conditions, tlsNegotiateErr := c.validateConnection(certPool, endpointHostPort, conditions, okSoFar) errs = append(errs, tlsNegotiateErr) okSoFar = okSoFar && tlsNegotiateErr == nil webhookAuthenticator, conditions, err := newWebhookAuthenticator( - specCopy.Endpoint, + // Note that we use the whole URL when constructing the webhook client, + // not just the host and port that ew validated above. We need the path, etc. + obj.Spec.Endpoint, pemBytes, - os.CreateTemp, - clientcmd.WriteToFile, conditions, okSoFar, ) @@ -153,10 +150,8 @@ func (c *webhookCacheFillerController) Sync(ctx controllerlib.Context) error { // newWebhookAuthenticator creates a webhook from the provided API server url and caBundle // used to validate TLS connections. func newWebhookAuthenticator( - endpoint string, + endpointURL string, pemBytes []byte, - tempfileFunc func(string, string) (*os.File, error), - marshalFunc func(clientcmdapi.Config, string) error, conditions []*metav1.Condition, prereqOk bool, ) (*webhook.WebhookTokenAuthenticator, []*metav1.Condition, error) { @@ -169,39 +164,6 @@ func newWebhookAuthenticator( }) return nil, conditions, nil } - temp, err := tempfileFunc("", "pinniped-webhook-kubeconfig-*") - if err != nil { - errText := "unable to create temporary file" - msg := fmt.Sprintf("%s: %s", errText, err.Error()) - conditions = append(conditions, &metav1.Condition{ - Type: typeAuthenticatorValid, - Status: metav1.ConditionFalse, - Reason: reasonUnableToCreateTempFile, - Message: msg, - }) - return nil, conditions, fmt.Errorf("%s: %w", errText, err) - } - defer func() { _ = os.Remove(temp.Name()) }() - - cluster := &clientcmdapi.Cluster{Server: endpoint} - cluster.CertificateAuthorityData = pemBytes - - kubeconfig := clientcmdapi.NewConfig() - kubeconfig.Clusters["anonymous-cluster"] = cluster - kubeconfig.Contexts["anonymous"] = &clientcmdapi.Context{Cluster: "anonymous-cluster"} - kubeconfig.CurrentContext = "anonymous" - - if err := marshalFunc(*kubeconfig, temp.Name()); err != nil { - errText := "unable to marshal kubeconfig" - msg := fmt.Sprintf("%s: %s", errText, err.Error()) - conditions = append(conditions, &metav1.Condition{ - Type: typeAuthenticatorValid, - Status: metav1.ConditionFalse, - Reason: reasonUnableToMarshallKubeconfig, - Message: msg, - }) - return nil, conditions, fmt.Errorf("%s: %w", errText, err) - } // We use v1beta1 instead of v1 since v1beta1 is more prevalent in our desired // integration points. @@ -216,40 +178,30 @@ func newWebhookAuthenticator( // custom proxy stuff used by the API server. var customDial k8snetutil.DialFunc - // TODO refactor this code to directly construct the rest.Config - // ideally we would keep rest config generation contained to the kubeclient package - // but this will require some form of a new WithTLSConfigFunc kubeclient.Option - // ex: - // _, caBundle, err := pinnipedauthenticator.CABundle(spec.TLS) - // ... - // restConfig := &rest.Config{ - // Host: spec.Endpoint, - // TLSClientConfig: rest.TLSClientConfig{CAData: caBundle}, - // // copied from k8s.io/apiserver/pkg/util/webhook - // Timeout: 30 * time.Second, - // QPS: -1, - // } - // client, err := kubeclient.New(kubeclient.WithConfig(restConfig), kubeclient.WithTLSConfigFunc(ptls.Default)) - // ... - // then use client.JSONConfig as clientConfig - clientConfig, err := webhookutil.LoadKubeconfig(temp.Name(), customDial) + restConfig := &rest.Config{ + Host: endpointURL, + TLSClientConfig: rest.TLSClientConfig{CAData: pemBytes}, + + // The remainder of these settings are copied from webhookutil.LoadKubeconfig in k8s.io/apiserver/pkg/util/webhook. + Dial: customDial, + Timeout: 30 * time.Second, + QPS: -1, + } + + client, err := kubeclient.New(kubeclient.WithConfig(restConfig), kubeclient.WithTLSConfigFunc(ptls.Default)) if err != nil { - // no unit test for this failure. - errText := "unable to load kubeconfig" + errText := "unable to create client for this webhook" msg := fmt.Sprintf("%s: %s", errText, err.Error()) conditions = append(conditions, &metav1.Condition{ Type: typeAuthenticatorValid, Status: metav1.ConditionFalse, - Reason: reasonUnableToLoadKubeconfig, + Reason: reasonUnableToCreateClient, Message: msg, }) return nil, conditions, fmt.Errorf("%s: %w", errText, err) } - // this uses a http client that does not honor our TLS config - // TODO: fix when we pick up https://github.com/kubernetes/kubernetes/pull/106155 - // NOTE: looks like the above was merged on Mar 18, 2022 - webhookA, err := webhook.New(clientConfig, version, implicitAuds, *webhook.DefaultRetryBackoff()) + webhookAuthenticator, err := webhook.New(client.JSONConfig, version, implicitAuds, *webhook.DefaultRetryBackoff()) if err != nil { // no unit test for this failure. errText := "unable to instantiate webhook" @@ -262,6 +214,7 @@ func newWebhookAuthenticator( }) return nil, conditions, fmt.Errorf("%s: %w", errText, err) } + msg := "authenticator initialized" conditions = append(conditions, &metav1.Condition{ Type: typeAuthenticatorValid, @@ -269,7 +222,8 @@ func newWebhookAuthenticator( Reason: reasonSuccess, Message: msg, }) - return webhookA, conditions, nil + + return webhookAuthenticator, conditions, nil } func (c *webhookCacheFillerController) validateConnection(certPool *x509.CertPool, endpointHostPort *endpointaddr.HostPort, conditions []*metav1.Condition, prereqOk bool) ([]*metav1.Condition, error) { @@ -300,6 +254,7 @@ func (c *webhookCacheFillerController) validateConnection(certPool *x509.CertPoo // this error should never be significant err = conn.Close() if err != nil { + // no unit test for this failure c.log.Error("error closing dialer", err) } @@ -307,7 +262,7 @@ func (c *webhookCacheFillerController) validateConnection(certPool *x509.CertPoo Type: typeWebhookConnectionValid, Status: metav1.ConditionTrue, Reason: reasonSuccess, - Message: "tls verified", + Message: "successfully dialed webhook server", }) return conditions, nil } @@ -378,7 +333,7 @@ func (c *webhookCacheFillerController) validateEndpoint(endpoint string, conditi Type: typeEndpointURLValid, Status: metav1.ConditionTrue, Reason: reasonSuccess, - Message: "endpoint is a valid URL", + Message: "spec.endpoint is a valid URL", }) return &endpointHostPort, conditions, true } diff --git a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go index 21c53dba3..67038f2df 100644 --- a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go +++ b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go @@ -16,19 +16,17 @@ import ( "net/http" "net/http/httptest" "net/url" - "os" "testing" "time" "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + authenticationv1beta1 "k8s.io/api/authentication/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" coretesting "k8s.io/client-go/testing" - "k8s.io/client-go/tools/clientcmd" - clientcmdapi "k8s.io/client-go/tools/clientcmd/api" clocktesting "k8s.io/utils/clock/testing" auth1alpha1 "go.pinniped.dev/generated/latest/apis/concierge/authentication/v1alpha1" @@ -91,56 +89,33 @@ func TestController(t *testing.T) { ) require.NoError(t, err) - hostAsLocalhostMux := http.NewServeMux() hostAsLocalhostWebhookServer := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - tlsserver.AssertTLS(t, r, ptls.Default) - hostAsLocalhostMux.ServeHTTP(w, r) - }), func(thisServer *httptest.Server) { - thisTLSConfig := ptls.Default(nil) - thisTLSConfig.Certificates = []tls.Certificate{ - *hostAsLocalhostServingCert, - } - thisServer.TLS = thisTLSConfig + // only expecting dials, which will not get into handler func + }), func(s *httptest.Server) { + s.TLS.Certificates = []tls.Certificate{*hostAsLocalhostServingCert} }) - hostAs127001Mux := http.NewServeMux() hostAs127001WebhookServer := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - tlsserver.AssertTLS(t, r, ptls.Default) - hostAs127001Mux.ServeHTTP(w, r) - }), func(thisServer *httptest.Server) { - thisTLSConfig := ptls.Default(nil) - thisTLSConfig.Certificates = []tls.Certificate{ - *hostAs127001ServingCert, - } - thisServer.TLS = thisTLSConfig + // only expecting dials, which will not get into handler func + }), func(s *httptest.Server) { + s.TLS.Certificates = []tls.Certificate{*hostAs127001ServingCert} }) - localWithExampleDotComMux := http.NewServeMux() hostLocalWithExampleDotComCertServer := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - tlsserver.AssertTLS(t, r, ptls.Default) - localWithExampleDotComMux.ServeHTTP(w, r) - }), func(thisServer *httptest.Server) { - thisTLSConfig := ptls.Default(nil) - thisTLSConfig.Certificates = []tls.Certificate{ - *localButExampleDotComServerCert, - } - thisServer.TLS = thisTLSConfig + // only expecting dials, which will not get into handler func + }), func(s *httptest.Server) { + s.TLS.Certificates = []tls.Certificate{*localButExampleDotComServerCert} }) - goodMux := http.NewServeMux() - hostGoodDefaultServingCertServer := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - tlsserver.AssertTLS(t, r, ptls.Default) - goodMux.ServeHTTP(w, r) - }), tlsserver.RecordTLSHello) - goodMux.Handle("/some/webhook", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - _, err := fmt.Fprintf(w, `{"something": "%s"}`, "something-for-response") - require.NoError(t, err) - })) - goodMux.Handle("/nothing/here", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mux := http.NewServeMux() + mux.Handle("/nothing/here", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // note that we are only dialing, so we shouldn't actually get here w.WriteHeader(http.StatusNotFound) - fmt.Fprint(w, "404 nothing here") + _, _ = fmt.Fprint(w, "404 nothing here") })) + hostGoodDefaultServingCertServer := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mux.ServeHTTP(w, r) + }), nil) goodWebhookDefaultServingCertEndpoint := hostGoodDefaultServingCertServer.URL goodWebhookDefaultServingCertEndpointBut404 := goodWebhookDefaultServingCertEndpoint + "/nothing/here" @@ -270,7 +245,7 @@ func TestController(t *testing.T) { ObservedGeneration: observedGeneration, LastTransitionTime: time, Reason: "Success", - Message: "tls verified", + Message: "successfully dialed webhook server", } } unknownWebhookConnectionValid := func(time metav1.Time, observedGeneration int64) metav1.Condition { @@ -321,7 +296,7 @@ func TestController(t *testing.T) { ObservedGeneration: observedGeneration, LastTransitionTime: time, Reason: "Success", - Message: "endpoint is a valid URL", + Message: "spec.endpoint is a valid URL", } } sadEndpointURLValid := func(issuer string, time metav1.Time, observedGeneration int64) metav1.Condition { @@ -390,7 +365,7 @@ func TestController(t *testing.T) { wantCacheEntries int }{ { - name: "404: WebhookAuthenticator not found will abort sync loop, no status conditions", + name: "Sync: WebhookAuthenticator not found will abort sync loop, no status conditions", syncKey: controllerlib.Key{Name: "test-name"}, wantLogs: []map[string]any{ { @@ -859,14 +834,14 @@ func TestController(t *testing.T) { Name: "test-name", }, Spec: auth1alpha1.WebhookAuthenticatorSpec{ - Endpoint: fmt.Sprintf("%s:%s", "https://localhost", localhostURL.Port()), + Endpoint: fmt.Sprintf("https://localhost:%s", localhostURL.Port()), TLS: &auth1alpha1.TLSSpec{ // CA Bundle for validating the server's certs CertificateAuthorityData: base64.StdEncoding.EncodeToString(caForLocalhostAsHostname.Bundle()), }, }, Status: auth1alpha1.WebhookAuthenticatorStatus{ - Conditions: allHappyConditionsSuccess(fmt.Sprintf("%s:%s", "https://localhost", localhostURL.Port()), frozenMetav1Now, 0), + Conditions: allHappyConditionsSuccess(fmt.Sprintf("https://localhost:%s", localhostURL.Port()), frozenMetav1Now, 0), Phase: "Ready", }, }, @@ -877,7 +852,7 @@ func TestController(t *testing.T) { "timestamp": "2099-08-08T13:57:36.123456Z", "logger": "webhookcachefiller-controller", "message": "added new webhook authenticator", - "endpoint": fmt.Sprintf("%s:%s", "https://localhost", localhostURL.Port()), + "endpoint": fmt.Sprintf("https://localhost:%s", localhostURL.Port()), "webhook": map[string]interface{}{ "name": "test-name", }, @@ -1379,35 +1354,53 @@ func TestController(t *testing.T) { } func TestNewWebhookAuthenticator(t *testing.T) { - goodEndpoint := "https://example.com" + server := tlsserver.TLSTestServer(t, + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Webhook clients should always use ptls.Default when making requests to the webhook. Assert that here. + tlsserver.AssertTLS(t, r, ptls.Default) - testServerCABundle, testServerURL := testutil.TLSTestServer(t, func(w http.ResponseWriter, r *http.Request) { - body, err := io.ReadAll(r.Body) - require.NoError(t, err) - require.Contains(t, string(body), "test-token") - _, err = w.Write([]byte(`{}`)) - require.NoError(t, err) - }) + // Loosely assert on the request body. + body, err := io.ReadAll(r.Body) + require.NoError(t, err) + require.Contains(t, string(body), "test-token") + + // Write a realistic looking fake response for a successfully authenticated user, so we can tell that + // this endpoint was actually called by the test below where it asserts on the fake user and group names. + w.Header().Add("Content-Type", "application/json") + responseBody := authenticationv1beta1.TokenReview{ + TypeMeta: metav1.TypeMeta{ + Kind: "TokenReview", + APIVersion: authenticationv1beta1.SchemeGroupVersion.String(), + }, + Status: authenticationv1beta1.TokenReviewStatus{ + Authenticated: true, + User: authenticationv1beta1.UserInfo{ + Username: "fake-username-from-server", + Groups: []string{"fake-group-from-server-1", "fake-group-from-server-2"}, + }, + }, + } + err = json.NewEncoder(w).Encode(responseBody) + require.NoError(t, err) + }), + tlsserver.RecordTLSHello, + ) tests := []struct { - name string - endpoint string - pemBytes []byte - tempFileFunc func(dir string, pattern string) (*os.File, error) - marshallFunc func(config clientcmdapi.Config, filename string) error - prereqOk bool - wantConditions []*metav1.Condition - wantWebhook bool - wantErr string - testCreatedWebhookWithFakeToken bool + name string + endpoint string + pemBytes []byte + prereqOk bool + wantConditions []*metav1.Condition + wantWebhook bool + wantErr string + userCreatedWebhookClientToCallWebhookEndpoint bool }{ { - name: "prerequisites not ready, cannot create webhook authenticator", - endpoint: "", - pemBytes: []byte("irrelevant pem bytes"), - tempFileFunc: os.CreateTemp, - marshallFunc: clientcmd.WriteToFile, - wantErr: "", + name: "prerequisites not ready, cannot create webhook authenticator", + endpoint: "", + pemBytes: []byte("irrelevant pem bytes"), + wantErr: "", wantConditions: []*metav1.Condition{{ Type: "AuthenticatorValid", Status: "Unknown", @@ -1416,58 +1409,22 @@ func TestNewWebhookAuthenticator(t *testing.T) { }}, prereqOk: false, }, { - name: "temp file failure, cannot create webhook authenticator", - endpoint: "", - pemBytes: []byte("irrelevant pem bytes"), - tempFileFunc: func(_ string, _ string) (*os.File, error) { - return nil, fmt.Errorf("some temp file error") - }, - marshallFunc: clientcmd.WriteToFile, - prereqOk: true, - wantConditions: []*metav1.Condition{{ - Type: "AuthenticatorValid", - Status: "False", - Reason: "UnableToCreateTempFile", - Message: "unable to create temporary file: some temp file error", - }}, - wantErr: "unable to create temporary file: some temp file error", - }, { - name: "marshal failure, cannot create webhook authenticator", - endpoint: "", - pemBytes: []byte("irrelevant pem bytes"), - tempFileFunc: os.CreateTemp, - marshallFunc: func(_ clientcmdapi.Config, _ string) error { - return fmt.Errorf("some marshal error") - }, + name: "invalid pem data, unable to parse bytes as PEM block", + endpoint: "https://does-not-matter-will-not-be-used", + pemBytes: []byte("invalid-bas64"), prereqOk: true, wantConditions: []*metav1.Condition{{ Type: "AuthenticatorValid", Status: "False", - Reason: "UnableToMarshallKubeconfig", - Message: "unable to marshal kubeconfig: some marshal error", + Reason: "UnableToCreateClient", + Message: "unable to create client for this webhook: could not create secure client config: unable to load root certificates: unable to parse bytes as PEM block", }}, - wantErr: "unable to marshal kubeconfig: some marshal error", + wantErr: "unable to create client for this webhook: could not create secure client config: unable to load root certificates: unable to parse bytes as PEM block", }, { - name: "invalid pem data, unable to parse bytes as PEM block", - endpoint: goodEndpoint, - pemBytes: []byte("invalid-bas64"), - tempFileFunc: os.CreateTemp, - marshallFunc: clientcmd.WriteToFile, - prereqOk: true, - wantConditions: []*metav1.Condition{{ - Type: "AuthenticatorValid", - Status: "False", - Reason: "UnableToInstantiateWebhook", - Message: "unable to instantiate webhook: unable to load root certificates: unable to parse bytes as PEM block", - }}, - wantErr: "unable to instantiate webhook: unable to load root certificates: unable to parse bytes as PEM block", - }, { - name: "valid config with no TLS spec, webhook authenticator created", - endpoint: goodEndpoint, - pemBytes: nil, - tempFileFunc: os.CreateTemp, - marshallFunc: clientcmd.WriteToFile, - prereqOk: true, + name: "valid config with no PEM bytes, webhook authenticator created", + endpoint: "https://does-not-matter-will-not-be-used", + pemBytes: nil, + prereqOk: true, wantConditions: []*metav1.Condition{{ Type: "AuthenticatorValid", Status: "True", @@ -1476,19 +1433,18 @@ func TestNewWebhookAuthenticator(t *testing.T) { }}, wantWebhook: true, }, { - name: "success, webhook authenticator created", - endpoint: testServerURL, - pemBytes: []byte(testServerCABundle), - tempFileFunc: os.CreateTemp, - marshallFunc: clientcmd.WriteToFile, - prereqOk: true, + name: "valid config, webhook authenticator created, and test calling webhook server", + endpoint: server.URL, + pemBytes: tlsserver.TLSTestServerCA(server), + prereqOk: true, wantConditions: []*metav1.Condition{{ Type: "AuthenticatorValid", Status: "True", Reason: "Success", Message: "authenticator initialized", }}, - testCreatedWebhookWithFakeToken: true, + wantWebhook: true, + userCreatedWebhookClientToCallWebhookEndpoint: true, }, } @@ -1497,12 +1453,14 @@ func TestNewWebhookAuthenticator(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() var conditions []*metav1.Condition - webhook, conditions, err := newWebhookAuthenticator(tt.endpoint, tt.pemBytes, tt.tempFileFunc, tt.marshallFunc, conditions, tt.prereqOk) + webhook, conditions, err := newWebhookAuthenticator(tt.endpoint, tt.pemBytes, conditions, tt.prereqOk) require.Equal(t, tt.wantConditions, conditions) if tt.wantWebhook { require.NotNil(t, webhook) + } else { + require.Nil(t, webhook) } if tt.wantErr != "" { @@ -1511,11 +1469,12 @@ func TestNewWebhookAuthenticator(t *testing.T) { require.NoError(t, err) } - if tt.testCreatedWebhookWithFakeToken { + if tt.userCreatedWebhookClientToCallWebhookEndpoint { authResp, isAuthenticated, err := webhook.AuthenticateToken(context.Background(), "test-token") require.NoError(t, err) - require.Nil(t, authResp) - require.False(t, isAuthenticated) + require.True(t, isAuthenticated) + require.Equal(t, "fake-username-from-server", authResp.User.GetName()) + require.Equal(t, []string{"fake-group-from-server-1", "fake-group-from-server-2"}, authResp.User.GetGroups()) } }) } diff --git a/internal/kubeclient/kubeclient.go b/internal/kubeclient/kubeclient.go index 1f5c43220..e1ba1681f 100644 --- a/internal/kubeclient/kubeclient.go +++ b/internal/kubeclient/kubeclient.go @@ -1,4 +1,4 @@ -// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package kubeclient @@ -36,7 +36,9 @@ type Client struct { } func New(opts ...Option) (*Client, error) { - c := &clientConfig{} + c := &clientConfig{ + tlsConfigFunc: ptls.Secure, // Set a default value. Can be overridden by an Option. + } for _, opt := range opts { opt(c) @@ -51,16 +53,16 @@ func New(opts ...Option) (*Client, error) { WithConfig(inClusterConfig)(c) // make sure all writes to clientConfig flow through one code path } - secureKubeConfig, err := createSecureKubeConfig(c.config) + ptlsKubeConfig, err := createPTLSKubeConfig(c.config, c.tlsConfigFunc) if err != nil { return nil, fmt.Errorf("could not create secure client config: %w", err) } // explicitly use json when talking to CRD APIs - jsonKubeConfig := createJSONKubeConfig(secureKubeConfig) + jsonKubeConfig := createJSONKubeConfig(ptlsKubeConfig) // explicitly use protobuf when talking to built-in kube APIs - protoKubeConfig := createProtoKubeConfig(secureKubeConfig) + protoKubeConfig := createProtoKubeConfig(ptlsKubeConfig) // Connect to the core Kubernetes API. k8sClient, err := kubernetes.NewForConfig(configWithWrapper(protoKubeConfig, kubescheme.Scheme, kubescheme.Codecs, c.middlewares, c.transportWrapper)) @@ -120,25 +122,25 @@ func createProtoKubeConfig(kubeConfig *restclient.Config) *restclient.Config { return protoKubeConfig } -// createSecureKubeConfig returns a copy of the input config with the WrapTransport +// createPTLSKubeConfig returns a copy of the input config with the WrapTransport // enhanced to use the secure TLS configuration of the ptls / phttp packages. -func createSecureKubeConfig(kubeConfig *restclient.Config) (*restclient.Config, error) { - secureKubeConfig := restclient.CopyConfig(kubeConfig) +func createPTLSKubeConfig(kubeConfig *restclient.Config, tlsConfigFunc ptls.ConfigFunc) (*restclient.Config, error) { + ptlsKubeConfig := restclient.CopyConfig(kubeConfig) // by setting proxy to always be non-nil, we bust the client-go global TLS config cache. // this is required to make our wrapper function work without data races. the unit tests // associated with this code run in parallel to assert that we are not using the cache. // see k8s.io/client-go/transport.tlsConfigKey - if secureKubeConfig.Proxy == nil { - secureKubeConfig.Proxy = net.NewProxierWithNoProxyCIDR(http.ProxyFromEnvironment) + if ptlsKubeConfig.Proxy == nil { + ptlsKubeConfig.Proxy = net.NewProxierWithNoProxyCIDR(http.ProxyFromEnvironment) } // make sure restclient.TLSConfigFor always returns a non-nil TLS config - if len(secureKubeConfig.NextProtos) == 0 { - secureKubeConfig.NextProtos = ptls.Secure(nil).NextProtos + if len(ptlsKubeConfig.NextProtos) == 0 { + ptlsKubeConfig.NextProtos = tlsConfigFunc(nil).NextProtos } - tlsConfigTest, err := restclient.TLSConfigFor(secureKubeConfig) + tlsConfigTest, err := restclient.TLSConfigFor(ptlsKubeConfig) if err != nil { return nil, err // should never happen because our input config should always be valid } @@ -146,10 +148,10 @@ func createSecureKubeConfig(kubeConfig *restclient.Config) (*restclient.Config, return nil, fmt.Errorf("unexpected empty TLS config") // should never happen because we set NextProtos above } - secureKubeConfig.Wrap(func(rt http.RoundTripper) http.RoundTripper { + ptlsKubeConfig.Wrap(func(rt http.RoundTripper) http.RoundTripper { defer func() { - if err := AssertSecureTransport(rt); err != nil { - panic(err) // not sure what the point of this function would be if it failed to make the config secure + if err := assertTransport(rt, tlsConfigFunc); err != nil { + panic(err) // not sure what the point of this function would be if it failed to make the config use the ptls settings } }() @@ -167,23 +169,23 @@ func createSecureKubeConfig(kubeConfig *restclient.Config) (*restclient.Config, } // mutate the TLS config into our desired state before it is used - ptls.Merge(ptls.Secure, tlsConfig) + ptls.Merge(tlsConfigFunc, tlsConfig) return rt // return the input transport since we mutated it in-place }) - if err := AssertSecureConfig(secureKubeConfig); err != nil { + if err := assertConfig(ptlsKubeConfig, tlsConfigFunc); err != nil { return nil, err // not sure what the point of this function would be if it failed to make the config secure } - return secureKubeConfig, nil + return ptlsKubeConfig, nil } // SecureAnonymousClientConfig has the same properties as restclient.AnonymousClientConfig // while still enforcing the secure TLS configuration of the ptls / phttp packages. func SecureAnonymousClientConfig(kubeConfig *restclient.Config) *restclient.Config { kubeConfig = restclient.AnonymousClientConfig(kubeConfig) - secureKubeConfig, err := createSecureKubeConfig(kubeConfig) + secureKubeConfig, err := createPTLSKubeConfig(kubeConfig, ptls.Secure) if err != nil { panic(err) // should never happen as this would only fail on invalid CA data, which would never work anyway } @@ -194,22 +196,30 @@ func SecureAnonymousClientConfig(kubeConfig *restclient.Config) *restclient.Conf } func AssertSecureConfig(kubeConfig *restclient.Config) error { + return assertConfig(kubeConfig, ptls.Secure) +} + +func assertConfig(kubeConfig *restclient.Config, tlsConfigFunc ptls.ConfigFunc) error { rt, err := restclient.TransportFor(kubeConfig) if err != nil { return fmt.Errorf("failed to build transport: %w", err) } - return AssertSecureTransport(rt) + return assertTransport(rt, tlsConfigFunc) } func AssertSecureTransport(rt http.RoundTripper) error { + return assertTransport(rt, ptls.Secure) +} + +func assertTransport(rt http.RoundTripper, tlsConfigFunc ptls.ConfigFunc) error { tlsConfig, err := net.TLSClientConfig(rt) if err != nil { return fmt.Errorf("failed to get TLS config: %w", err) } tlsConfigCopy := tlsConfig.Clone() - ptls.Merge(ptls.Secure, tlsConfigCopy) // only mutate the copy + ptls.Merge(tlsConfigFunc, tlsConfigCopy) // only mutate the copy //nolint:gosec // the empty TLS config here is not used if diff := cmp.Diff(tlsConfigCopy, tlsConfig, diff --git a/internal/kubeclient/kubeclient_test.go b/internal/kubeclient/kubeclient_test.go index f880118bf..a4e95313f 100644 --- a/internal/kubeclient/kubeclient_test.go +++ b/internal/kubeclient/kubeclient_test.go @@ -1,4 +1,4 @@ -// Copyright 2021-2023 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package kubeclient @@ -957,7 +957,7 @@ func TestUnwrap(t *testing.T) { regularClient := makeClient(t, restConfig, func(_ *rest.Config) {}) - testUnwrap(t, regularClient, serverSubjects) + testUnwrap(t, regularClient, serverSubjects, ptls.Secure) }) t.Run("exec client", func(t *testing.T) { @@ -972,7 +972,7 @@ func TestUnwrap(t *testing.T) { } }) - testUnwrap(t, execClient, serverSubjects) + testUnwrap(t, execClient, serverSubjects, ptls.Secure) }) t.Run("oidc client", func(t *testing.T) { @@ -988,90 +988,149 @@ func TestUnwrap(t *testing.T) { } }) - testUnwrap(t, oidcClient, serverSubjects) + testUnwrap(t, oidcClient, serverSubjects, ptls.Secure) + }) + + t.Run("regular client with ptls.Default", func(t *testing.T) { + t.Parallel() // make sure to run in parallel to confirm that our client-go TLS cache busting works (i.e. assert no data races) + + regularClient := makeClient(t, restConfig, func(_ *rest.Config) {}, WithTLSConfigFunc(ptls.Default)) + + testUnwrap(t, regularClient, serverSubjects, ptls.Default) + }) + + t.Run("exec client with ptls.Default", func(t *testing.T) { + t.Parallel() // make sure to run in parallel to confirm that our client-go TLS cache busting works (i.e. assert no data races) + + execClient := makeClient(t, restConfig, func(config *rest.Config) { + config.ExecProvider = &clientcmdapi.ExecConfig{ + Command: "echo", + Args: []string{"pandas are awesome"}, + APIVersion: clientauthenticationv1.SchemeGroupVersion.String(), + InteractiveMode: clientcmdapi.NeverExecInteractiveMode, + } + }, WithTLSConfigFunc(ptls.Default)) + + testUnwrap(t, execClient, serverSubjects, ptls.Default) + }) + + t.Run("oidc client with ptls.Default", func(t *testing.T) { + t.Parallel() // make sure to run in parallel to confirm that our client-go TLS cache busting works (i.e. assert no data races) + + oidcClient := makeClient(t, restConfig, func(config *rest.Config) { + config.AuthProvider = &clientcmdapi.AuthProviderConfig{ + Name: "oidc", + Config: map[string]string{ + "idp-issuer-url": "https://pandas.local", + "client-id": "walrus", + }, + } + }, WithTLSConfigFunc(ptls.Default)) + + testUnwrap(t, oidcClient, serverSubjects, ptls.Default) }) } -func testUnwrap(t *testing.T, client *Client, serverSubjects [][]byte) { +func testUnwrap(t *testing.T, client *Client, serverSubjects [][]byte, tlsConfigFuncForExpectedValues ptls.ConfigFunc) { tests := []struct { - name string - rt http.RoundTripper + name string + rt http.RoundTripper + wantConfigFunc ptls.ConfigFunc }{ { - name: "core v1", - rt: extractTransport(client.Kubernetes.CoreV1()), + name: "core v1", + rt: extractTransport(client.Kubernetes.CoreV1()), + wantConfigFunc: tlsConfigFuncForExpectedValues, }, { - name: "coordination v1", - rt: extractTransport(client.Kubernetes.CoordinationV1()), + name: "coordination v1", + rt: extractTransport(client.Kubernetes.CoordinationV1()), + wantConfigFunc: tlsConfigFuncForExpectedValues, }, { - name: "api registration v1", - rt: extractTransport(client.Aggregation.ApiregistrationV1()), + name: "api registration v1", + rt: extractTransport(client.Aggregation.ApiregistrationV1()), + wantConfigFunc: tlsConfigFuncForExpectedValues, }, { - name: "concierge login", - rt: extractTransport(client.PinnipedConcierge.LoginV1alpha1()), + name: "concierge login", + rt: extractTransport(client.PinnipedConcierge.LoginV1alpha1()), + wantConfigFunc: tlsConfigFuncForExpectedValues, }, { - name: "concierge config", - rt: extractTransport(client.PinnipedConcierge.ConfigV1alpha1()), + name: "concierge config", + rt: extractTransport(client.PinnipedConcierge.ConfigV1alpha1()), + wantConfigFunc: tlsConfigFuncForExpectedValues, }, { - name: "supervisor idp", - rt: extractTransport(client.PinnipedSupervisor.IDPV1alpha1()), + name: "supervisor idp", + rt: extractTransport(client.PinnipedSupervisor.IDPV1alpha1()), + wantConfigFunc: tlsConfigFuncForExpectedValues, }, { - name: "supervisor config", - rt: extractTransport(client.PinnipedSupervisor.ConfigV1alpha1()), + name: "supervisor config", + rt: extractTransport(client.PinnipedSupervisor.ConfigV1alpha1()), + wantConfigFunc: tlsConfigFuncForExpectedValues, }, { - name: "json config", - rt: configToTransport(t, client.JSONConfig), + name: "json config", + rt: configToTransport(t, client.JSONConfig), + wantConfigFunc: tlsConfigFuncForExpectedValues, }, { - name: "proto config", - rt: configToTransport(t, client.ProtoConfig), + name: "proto config", + rt: configToTransport(t, client.ProtoConfig), + wantConfigFunc: tlsConfigFuncForExpectedValues, }, { - name: "anonymous json config", - rt: configToTransport(t, SecureAnonymousClientConfig(client.JSONConfig)), + name: "anonymous json config", + rt: configToTransport(t, SecureAnonymousClientConfig(client.JSONConfig)), + wantConfigFunc: ptls.Secure, // SecureAnonymousClientConfig is always ptls.Secure }, { - name: "anonymous proto config", - rt: configToTransport(t, SecureAnonymousClientConfig(client.ProtoConfig)), + name: "anonymous proto config", + rt: configToTransport(t, SecureAnonymousClientConfig(client.ProtoConfig)), + wantConfigFunc: ptls.Secure, // SecureAnonymousClientConfig is always ptls.Secure }, { - name: "json config - no cache", - rt: configToTransport(t, bustTLSCache(client.JSONConfig)), + name: "json config - no cache", + rt: configToTransport(t, bustTLSCache(client.JSONConfig)), + wantConfigFunc: tlsConfigFuncForExpectedValues, }, { - name: "proto config - no cache", - rt: configToTransport(t, bustTLSCache(client.ProtoConfig)), + name: "proto config - no cache", + rt: configToTransport(t, bustTLSCache(client.ProtoConfig)), + wantConfigFunc: tlsConfigFuncForExpectedValues, }, { - name: "anonymous json config - no cache, inner bust", - rt: configToTransport(t, SecureAnonymousClientConfig(bustTLSCache(client.JSONConfig))), + name: "anonymous json config - no cache, inner bust", + rt: configToTransport(t, SecureAnonymousClientConfig(bustTLSCache(client.JSONConfig))), + wantConfigFunc: ptls.Secure, // SecureAnonymousClientConfig is always ptls.Secure }, { - name: "anonymous proto config - no cache, inner bust", - rt: configToTransport(t, SecureAnonymousClientConfig(bustTLSCache(client.ProtoConfig))), + name: "anonymous proto config - no cache, inner bust", + rt: configToTransport(t, SecureAnonymousClientConfig(bustTLSCache(client.ProtoConfig))), + wantConfigFunc: ptls.Secure, // SecureAnonymousClientConfig is always ptls.Secure }, { - name: "anonymous json config - no cache, double bust", - rt: configToTransport(t, bustTLSCache(SecureAnonymousClientConfig(bustTLSCache(client.JSONConfig)))), + name: "anonymous json config - no cache, double bust", + rt: configToTransport(t, bustTLSCache(SecureAnonymousClientConfig(bustTLSCache(client.JSONConfig)))), + wantConfigFunc: ptls.Secure, // SecureAnonymousClientConfig is always ptls.Secure }, { - name: "anonymous proto config - no cache, double bust", - rt: configToTransport(t, bustTLSCache(SecureAnonymousClientConfig(bustTLSCache(client.ProtoConfig)))), + name: "anonymous proto config - no cache, double bust", + rt: configToTransport(t, bustTLSCache(SecureAnonymousClientConfig(bustTLSCache(client.ProtoConfig)))), + wantConfigFunc: ptls.Secure, // SecureAnonymousClientConfig is always ptls.Secure }, { - name: "anonymous json config - no cache, outer bust", - rt: configToTransport(t, bustTLSCache(SecureAnonymousClientConfig(client.JSONConfig))), + name: "anonymous json config - no cache, outer bust", + rt: configToTransport(t, bustTLSCache(SecureAnonymousClientConfig(client.JSONConfig))), + wantConfigFunc: ptls.Secure, // SecureAnonymousClientConfig is always ptls.Secure }, { - name: "anonymous proto config - no cache, outer bust", - rt: configToTransport(t, bustTLSCache(SecureAnonymousClientConfig(client.ProtoConfig))), + name: "anonymous proto config - no cache, outer bust", + rt: configToTransport(t, bustTLSCache(SecureAnonymousClientConfig(client.ProtoConfig))), + wantConfigFunc: ptls.Secure, // SecureAnonymousClientConfig is always ptls.Secure }, } for _, tt := range tests { @@ -1083,11 +1142,11 @@ func testUnwrap(t *testing.T, client *Client, serverSubjects [][]byte) { require.NoError(t, err) require.NotNil(t, tlsConfig) - secureTLSConfig := ptls.Secure(nil) + ptlsConfig := tt.wantConfigFunc(nil) - require.Equal(t, secureTLSConfig.MinVersion, tlsConfig.MinVersion) - require.Equal(t, secureTLSConfig.CipherSuites, tlsConfig.CipherSuites) - require.Equal(t, secureTLSConfig.NextProtos, tlsConfig.NextProtos) + require.Equal(t, ptlsConfig.MinVersion, tlsConfig.MinVersion) + require.Equal(t, ptlsConfig.CipherSuites, tlsConfig.CipherSuites) + require.Equal(t, ptlsConfig.NextProtos, tlsConfig.NextProtos) // x509.CertPool has some embedded functions that make it hard to compare so just look at the subjects //nolint:staticcheck // since we're not using .Subjects() to access the system pool @@ -1120,14 +1179,14 @@ func bustTLSCache(config *rest.Config) *rest.Config { return c } -func makeClient(t *testing.T, restConfig *rest.Config, f func(*rest.Config)) *Client { +func makeClient(t *testing.T, restConfig *rest.Config, f func(*rest.Config), opts ...Option) *Client { t.Helper() restConfig = rest.CopyConfig(restConfig) f(restConfig) - client, err := New(WithConfig(restConfig)) + client, err := New(append([]Option{WithConfig(restConfig)}, opts...)...) require.NoError(t, err) return client diff --git a/internal/kubeclient/option.go b/internal/kubeclient/option.go index 7c10bda98..0099ebd3c 100644 --- a/internal/kubeclient/option.go +++ b/internal/kubeclient/option.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package kubeclient @@ -6,12 +6,15 @@ package kubeclient import ( restclient "k8s.io/client-go/rest" "k8s.io/client-go/transport" + + "go.pinniped.dev/internal/crypto/ptls" ) type Option func(*clientConfig) type clientConfig struct { config *restclient.Config + tlsConfigFunc ptls.ConfigFunc middlewares []Middleware transportWrapper transport.WrapperFunc } @@ -22,6 +25,15 @@ func WithConfig(config *restclient.Config) Option { } } +// WithTLSConfigFunc will cause the client to use the provided configuration from the ptls package when the +// client makes requests. For example, pass ptls.Default or ptls.Secure as the argument. When this Option +// is not used, the client will default to using ptls.Secure. +func WithTLSConfigFunc(tlsConfigFunc ptls.ConfigFunc) Option { + return func(c *clientConfig) { + c.tlsConfigFunc = tlsConfigFunc + } +} + func WithMiddleware(middleware Middleware) Option { return func(c *clientConfig) { if middleware == nil {