diff --git a/deploy/concierge/deployment.yaml b/deploy/concierge/deployment.yaml index 41119243f..223b7fa80 100644 --- a/deploy/concierge/deployment.yaml +++ b/deploy/concierge/deployment.yaml @@ -46,6 +46,7 @@ data: impersonationLoadBalancerService: (@= defaultResourceNameWithSuffix("impersonation-proxy-load-balancer") @) impersonationTLSCertificateSecret: (@= defaultResourceNameWithSuffix("impersonation-proxy-tls-serving-certificate") @) impersonationCACertificateSecret: (@= defaultResourceNameWithSuffix("impersonation-proxy-ca-certificate") @) + impersonationSignerSecret: (@= defaultResourceNameWithSuffix("impersonation-proxy-signer-ca-certificate") @) labels: (@= json.encode(labels()).rstrip() @) kubeCertAgent: namePrefix: (@= defaultResourceNameWithSuffix("kube-cert-agent-") @) diff --git a/go.mod b/go.mod index 0d15fb2a4..de8094645 100644 --- a/go.mod +++ b/go.mod @@ -26,7 +26,7 @@ require ( github.com/spf13/pflag v1.0.5 github.com/stretchr/testify v1.7.0 golang.org/x/crypto v0.0.0-20201217014255-9d1352758620 - golang.org/x/net v0.0.0-20210226172049-e18ecbb05110 // indirect + golang.org/x/net v0.0.0-20210226172049-e18ecbb05110 golang.org/x/oauth2 v0.0.0-20200107190931-bf48bf16ab8d golang.org/x/sync v0.0.0-20201207232520-09787c993a3a golang.org/x/tools v0.0.0-20200825202427-b303f430e36d // indirect diff --git a/internal/certauthority/certauthority.go b/internal/certauthority/certauthority.go index 64fa2ecbe..806d6498b 100644 --- a/internal/certauthority/certauthority.go +++ b/internal/certauthority/certauthority.go @@ -187,11 +187,12 @@ func (c *CA) Issue(subject pkix.Name, dnsNames []string, ips []net.IP, ttl time. // Sign a cert, getting back the DER-encoded certificate bytes. template := x509.Certificate{ - SerialNumber: serialNumber, - Subject: subject, - NotBefore: notBefore, - NotAfter: notAfter, - KeyUsage: x509.KeyUsageDigitalSignature, + SerialNumber: serialNumber, + Subject: subject, + NotBefore: notBefore, + NotAfter: notAfter, + KeyUsage: x509.KeyUsageDigitalSignature, + // TODO split this function into two funcs that handle client and serving certs differently ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth}, BasicConstraintsValid: true, IsCA: false, diff --git a/internal/certauthority/dynamiccertauthority/dynamiccertauthority.go b/internal/certauthority/dynamiccertauthority/dynamiccertauthority.go index a2c978983..c15e4cc0d 100644 --- a/internal/certauthority/dynamiccertauthority/dynamiccertauthority.go +++ b/internal/certauthority/dynamiccertauthority/dynamiccertauthority.go @@ -1,4 +1,4 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package dynamiccertauthority implements a x509 certificate authority capable of issuing @@ -9,18 +9,19 @@ import ( "crypto/x509/pkix" "time" + "k8s.io/apiserver/pkg/server/dynamiccertificates" + "go.pinniped.dev/internal/certauthority" - "go.pinniped.dev/internal/dynamiccert" ) // CA is a type capable of issuing certificates. type CA struct { - provider dynamiccert.Provider + provider dynamiccertificates.CertKeyContentProvider } // New creates a new CA, ready to issue certs whenever the provided provider has a keypair to // provide. -func New(provider dynamiccert.Provider) *CA { +func New(provider dynamiccertificates.CertKeyContentProvider) *CA { return &CA{ provider: provider, } diff --git a/internal/concierge/apiserver/apiserver.go b/internal/concierge/apiserver/apiserver.go index e5fc8da9e..f64e0104c 100644 --- a/internal/concierge/apiserver/apiserver.go +++ b/internal/concierge/apiserver/apiserver.go @@ -15,6 +15,7 @@ import ( genericapiserver "k8s.io/apiserver/pkg/server" "k8s.io/client-go/pkg/version" + "go.pinniped.dev/internal/issuer" "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/registry/credentialrequest" "go.pinniped.dev/internal/registry/whoamirequest" @@ -27,7 +28,7 @@ type Config struct { type ExtraConfig struct { Authenticator credentialrequest.TokenCredentialRequestAuthenticator - Issuer credentialrequest.CertIssuer + Issuer issuer.CertIssuer StartControllersPostStartHook func(ctx context.Context) Scheme *runtime.Scheme NegotiatedSerializer runtime.NegotiatedSerializer diff --git a/internal/concierge/impersonator/impersonator.go b/internal/concierge/impersonator/impersonator.go index bbbcda390..f943db808 100644 --- a/internal/concierge/impersonator/impersonator.go +++ b/internal/concierge/impersonator/impersonator.go @@ -4,58 +4,202 @@ package impersonator import ( - "context" - "encoding/base64" "fmt" + "net" "net/http" "net/http/httputil" "net/url" "strings" "time" - "github.com/go-logr/logr" + "k8s.io/apimachinery/pkg/util/errors" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apiserver/pkg/authentication/authenticator" - "k8s.io/apiserver/pkg/authentication/request/bearertoken" - "k8s.io/apiserver/pkg/authentication/user" + "k8s.io/apimachinery/pkg/runtime/serializer" + "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/apiserver/pkg/authorization/authorizer" + "k8s.io/apiserver/pkg/endpoints/request" + genericapiserver "k8s.io/apiserver/pkg/server" + "k8s.io/apiserver/pkg/server/dynamiccertificates" + genericoptions "k8s.io/apiserver/pkg/server/options" "k8s.io/client-go/rest" "k8s.io/client-go/transport" - "go.pinniped.dev/generated/latest/apis/concierge/login" "go.pinniped.dev/internal/constable" - "go.pinniped.dev/internal/controller/authenticator/authncache" + "go.pinniped.dev/internal/httputil/securityheader" "go.pinniped.dev/internal/kubeclient" + "go.pinniped.dev/internal/plog" ) -type proxy struct { - cache *authncache.Cache - jsonDecoder runtime.Decoder - proxy *httputil.ReverseProxy - log logr.Logger +// FactoryFunc is a function which can create an impersonator server. +// It returns a function which will start the impersonator server. +// That start function takes a stopCh which can be used to stop the server. +// Once a server has been stopped, don't start it again using the start function. +// Instead, call the factory function again to get a new start function. +type FactoryFunc func( + port int, + dynamicCertProvider dynamiccertificates.CertKeyContentProvider, + impersonationProxySignerCA dynamiccertificates.CAContentProvider, +) (func(stopCh <-chan struct{}) error, error) + +func New( + port int, + dynamicCertProvider dynamiccertificates.CertKeyContentProvider, // TODO: we need to check those optional interfaces and see what we need to implement + impersonationProxySignerCA dynamiccertificates.CAContentProvider, // TODO: we need to check those optional interfaces and see what we need to implement +) (func(stopCh <-chan struct{}) error, error) { + return newInternal(port, dynamicCertProvider, impersonationProxySignerCA, nil, nil) } -func New(cache *authncache.Cache, jsonDecoder runtime.Decoder, log logr.Logger) (http.Handler, error) { - return newInternal(cache, jsonDecoder, log, func() (*rest.Config, error) { - client, err := kubeclient.New() +func newInternal( //nolint:funlen // yeah, it's kind of long. + port int, + dynamicCertProvider dynamiccertificates.CertKeyContentProvider, + impersonationProxySignerCA dynamiccertificates.CAContentProvider, + clientOpts []kubeclient.Option, // for unit testing, should always be nil in production + recOpts func(*genericoptions.RecommendedOptions), // for unit testing, should always be nil in production +) (func(stopCh <-chan struct{}) error, error) { + var listener net.Listener + + constructServer := func() (func(stopCh <-chan struct{}) error, error) { + // bare minimum server side scheme to allow for status messages to be encoded + scheme := runtime.NewScheme() + metav1.AddToGroupVersion(scheme, metav1.Unversioned) + codecs := serializer.NewCodecFactory(scheme) + + // this is unused for now but it is a safe value that we could use in the future + defaultEtcdPathPrefix := "/pinniped-impersonation-proxy-registry" + + recommendedOptions := genericoptions.NewRecommendedOptions( + defaultEtcdPathPrefix, + codecs.LegacyCodec(), + ) + recommendedOptions.Etcd = nil // turn off etcd storage because we don't need it yet + recommendedOptions.SecureServing.ServerCert.GeneratedCert = dynamicCertProvider // serving certs (end user facing) + recommendedOptions.SecureServing.BindPort = port + + // wire up the impersonation proxy signer CA as a valid authenticator for client cert auth + // TODO fix comments + kubeClient, err := kubeclient.New(clientOpts...) if err != nil { return nil, err } - return client.JSONConfig, nil - }) -} + kubeClientCA, err := dynamiccertificates.NewDynamicCAFromConfigMapController("client-ca", metav1.NamespaceSystem, "extension-apiserver-authentication", "client-ca-file", kubeClient.Kubernetes) + if err != nil { + return nil, err + } + recommendedOptions.Authentication.ClientCert.ClientCA = "---irrelevant-but-needs-to-be-non-empty---" // drop when we pick up https://github.com/kubernetes/kubernetes/pull/100055 + recommendedOptions.Authentication.ClientCert.CAContentProvider = dynamiccertificates.NewUnionCAContentProvider(impersonationProxySignerCA, kubeClientCA) -func newInternal(cache *authncache.Cache, jsonDecoder runtime.Decoder, log logr.Logger, getConfig func() (*rest.Config, error)) (*proxy, error) { - kubeconfig, err := getConfig() - if err != nil { - return nil, fmt.Errorf("could not get in-cluster config: %w", err) + if recOpts != nil { + recOpts(recommendedOptions) + } + + serverConfig := genericapiserver.NewRecommendedConfig(codecs) + + // Note that ApplyTo is going to create a network listener and bind to the requested port. + // It puts this listener into serverConfig.SecureServing.Listener. + err = recommendedOptions.ApplyTo(serverConfig) + if serverConfig.SecureServing != nil { + // Set the pointer from the outer function to allow the outer function to close the listener in case + // this function returns an error for any reason anywhere below here. + listener = serverConfig.SecureServing.Listener + } + if err != nil { + return nil, err + } + + // loopback authentication to this server does not really make sense since we just proxy everything to KAS + // thus we replace loopback connection config with one that does direct connections to KAS + // loopback config is mainly used by post start hooks, so this is mostly future proofing + serverConfig.LoopbackClientConfig = rest.CopyConfig(kubeClient.ProtoConfig) // assume proto is safe (hooks can override) + // remove the bearer token so our authorizer does not get stomped on by AuthorizeClientBearerToken + // see sanity checks at the end of this function + serverConfig.LoopbackClientConfig.BearerToken = "" + + // assume proto config is safe because transport level configs do not use rest.ContentConfig + // thus if we are interacting with actual APIs, they should be using pre-built clients + impersonationProxy, err := newImpersonationReverseProxy(rest.CopyConfig(kubeClient.ProtoConfig)) + if err != nil { + return nil, err + } + + defaultBuildHandlerChainFunc := serverConfig.BuildHandlerChainFunc + serverConfig.BuildHandlerChainFunc = func(_ http.Handler, c *genericapiserver.Config) http.Handler { + // we ignore the passed in handler because we never have any REST APIs to delegate to + handler := defaultBuildHandlerChainFunc(impersonationProxy, c) + handler = securityheader.Wrap(handler) + return handler + } + + // TODO integration test this authorizer logic with system:masters + double impersonation + // overwrite the delegating authorizer with one that only cares about impersonation + // empty string is disallowed because request info has had bugs in the past where it would leave it empty + disallowedVerbs := sets.NewString("", "impersonate") + noImpersonationAuthorizer := &comparableAuthorizer{ + AuthorizerFunc: func(a authorizer.Attributes) (authorizer.Decision, string, error) { + // supporting impersonation is not hard, it would just require a bunch of testing + // and configuring the audit layer (to preserve the caller) which we can do later + // we would also want to delete the incoming impersonation headers + // instead of overwriting the delegating authorizer, we would + // actually use it to make the impersonation authorization checks + if disallowedVerbs.Has(a.GetVerb()) { + return authorizer.DecisionDeny, "impersonation is not allowed or invalid verb", nil + } + + return authorizer.DecisionAllow, "deferring authorization to kube API server", nil + }, + } + // TODO write a big comment explaining wth this is doing + serverConfig.Authorization.Authorizer = noImpersonationAuthorizer + + impersonationProxyServer, err := serverConfig.Complete().New("impersonation-proxy", genericapiserver.NewEmptyDelegate()) + if err != nil { + return nil, err + } + + preparedRun := impersonationProxyServer.PrepareRun() + + // wait until the very end to do sanity checks + + if preparedRun.Authorizer != noImpersonationAuthorizer { + return nil, constable.Error("invalid mutation of impersonation authorizer detected") + } + + // assert that we have a functioning token file to use and no bearer token + if len(preparedRun.LoopbackClientConfig.BearerToken) != 0 || len(preparedRun.LoopbackClientConfig.BearerTokenFile) == 0 { + return nil, constable.Error("invalid impersonator loopback rest config has wrong bearer token semantics") + } + + // TODO make sure this is closed on error + _ = preparedRun.SecureServingInfo.Listener + + return preparedRun.Run, nil } - serverURL, err := url.Parse(kubeconfig.Host) + result, err := constructServer() + // If there was any error during construction, then we would like to close the listener to free up the port. + if err != nil { + errs := []error{err} + if listener != nil { + errs = append(errs, listener.Close()) + } + return nil, errors.NewAggregate(errs) + } + return result, nil +} + +// No-op wrapping around AuthorizerFunc to allow for comparisons. +type comparableAuthorizer struct { + authorizer.AuthorizerFunc +} + +func newImpersonationReverseProxy(restConfig *rest.Config) (http.Handler, error) { + serverURL, err := url.Parse(restConfig.Host) if err != nil { return nil, fmt.Errorf("could not parse host URL from in-cluster config: %w", err) } - kubeTransportConfig, err := kubeconfig.TransportConfig() + kubeTransportConfig, err := restConfig.TransportConfig() if err != nil { return nil, fmt.Errorf("could not get in-cluster transport config: %w", err) } @@ -66,148 +210,72 @@ func newInternal(cache *authncache.Cache, jsonDecoder runtime.Decoder, log logr. return nil, fmt.Errorf("could not get in-cluster transport: %w", err) } - reverseProxy := httputil.NewSingleHostReverseProxy(serverURL) - reverseProxy.Transport = kubeRoundTripper - reverseProxy.FlushInterval = 200 * time.Millisecond // the "watch" verb will not work without this line - - return &proxy{ - cache: cache, - jsonDecoder: jsonDecoder, - proxy: reverseProxy, - log: log, - }, nil -} - -func (p *proxy) ServeHTTP(w http.ResponseWriter, r *http.Request) { - log := p.log.WithValues( - "url", r.URL.String(), - "method", r.Method, - ) - - if err := ensureNoImpersonationHeaders(r); err != nil { - log.Error(err, "impersonation header already exists") - http.Error(w, "impersonation header already exists", http.StatusBadRequest) - return - } - - // Never mutate request (see http.Handler docs). - newR := r.Clone(r.Context()) - - authentication, authenticated, err := bearertoken.New(authenticator.TokenFunc(func(ctx context.Context, token string) (*authenticator.Response, bool, error) { - tokenCredentialReq, err := extractToken(token, p.jsonDecoder) - if err != nil { - log.Error(err, "invalid token encoding") - return nil, false, &httpError{message: "invalid token encoding", code: http.StatusBadRequest} + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // TODO integration test using a bearer token + if len(r.Header.Values("Authorization")) != 0 { + plog.Warning("aggregated API server logic did not delete authorization header but it is always supposed to do so", + "url", r.URL.String(), + "method", r.Method, + ) + http.Error(w, "invalid authorization header", http.StatusInternalServerError) + return } - log = log.WithValues( - "authenticator", tokenCredentialReq.Spec.Authenticator, + if err := ensureNoImpersonationHeaders(r); err != nil { + plog.Error("noImpersonationAuthorizer logic did not prevent nested impersonation but it is always supposed to do so", + err, + "url", r.URL.String(), + "method", r.Method, + ) + http.Error(w, "invalid impersonation", http.StatusInternalServerError) + return + } + + userInfo, ok := request.UserFrom(r.Context()) + if !ok { + plog.Warning("aggregated API server logic did not set user info but it is always supposed to do so", + "url", r.URL.String(), + "method", r.Method, + ) + http.Error(w, "invalid user", http.StatusInternalServerError) + return + } + + if len(userInfo.GetUID()) > 0 { + plog.Warning("rejecting request with UID since we cannot impersonate UIDs", + "url", r.URL.String(), + "method", r.Method, + ) + http.Error(w, "unexpected uid", http.StatusUnprocessableEntity) + return + } + + plog.Trace("proxying authenticated request", + "url", r.URL.String(), + "method", r.Method, + "username", userInfo.GetName(), // this info leak seems fine for trace level logs ) - userInfo, err := p.cache.AuthenticateTokenCredentialRequest(newR.Context(), tokenCredentialReq) - if err != nil { - log.Error(err, "received invalid token") - return nil, false, &httpError{message: "invalid token", code: http.StatusUnauthorized} + reverseProxy := httputil.NewSingleHostReverseProxy(serverURL) + impersonateConfig := transport.ImpersonationConfig{ + UserName: userInfo.GetName(), + Groups: userInfo.GetGroups(), + Extra: userInfo.GetExtra(), } - if userInfo == nil { - log.Info("received token that did not authenticate") - return nil, false, &httpError{message: "not authenticated", code: http.StatusUnauthorized} - } - log = log.WithValues("userID", userInfo.GetUID()) - - return &authenticator.Response{User: userInfo}, true, nil - })).AuthenticateRequest(newR) - if err != nil { - httpErr, ok := err.(*httpError) - if !ok { - log.Error(err, "unrecognized error") - http.Error(w, "unrecognized error", http.StatusInternalServerError) - } - http.Error(w, httpErr.message, httpErr.code) - return - } - if !authenticated { - log.Error(constable.Error("token authenticator did not find token"), "invalid token encoding") - http.Error(w, "invalid token encoding", http.StatusBadRequest) - return - } - - newR.Header = getProxyHeaders(authentication.User, r.Header) - - log.Info("proxying authenticated request") - p.proxy.ServeHTTP(w, newR) + reverseProxy.Transport = transport.NewImpersonatingRoundTripper(impersonateConfig, kubeRoundTripper) + reverseProxy.FlushInterval = 200 * time.Millisecond // the "watch" verb will not work without this line + // transport.NewImpersonatingRoundTripper clones the request before setting headers + // so this call will not accidentally mutate the input request (see http.Handler docs) + reverseProxy.ServeHTTP(w, r) + }), nil } -type httpError struct { - message string - code int -} - -func (e *httpError) Error() string { return e.message } - func ensureNoImpersonationHeaders(r *http.Request) error { for key := range r.Header { - if isImpersonationHeader(key) { + if strings.HasPrefix(key, "Impersonate") { return fmt.Errorf("%q header already exists", key) } } + return nil } - -type roundTripperFunc func(*http.Request) (*http.Response, error) - -func (f roundTripperFunc) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) } - -func getProxyHeaders(userInfo user.Info, requestHeaders http.Header) http.Header { - // Copy over all headers except the Authorization header from the original request to the new request. - newHeaders := requestHeaders.Clone() - newHeaders.Del("Authorization") - - // Leverage client-go's impersonation RoundTripper to set impersonation headers for us in the new - // request. The client-go RoundTripper not only sets all of the impersonation headers for us, but - // it also does some helpful escaping of characters that can't go into an HTTP header. To do this, - // we make a fake call to the impersonation RoundTripper with a fake HTTP request and a delegate - // RoundTripper that captures the impersonation headers set on the request. - impersonateConfig := transport.ImpersonationConfig{ - UserName: userInfo.GetName(), - Groups: userInfo.GetGroups(), - Extra: userInfo.GetExtra(), - } - impersonateHeaderSpy := roundTripperFunc(func(r *http.Request) (*http.Response, error) { - for headerKey, headerValues := range r.Header { - if isImpersonationHeader(headerKey) { - for _, headerValue := range headerValues { - newHeaders.Add(headerKey, headerValue) - } - } - } - return nil, nil - }) - fakeReq, _ := http.NewRequestWithContext(context.Background(), "", "", nil) - //nolint:bodyclose // We return a nil http.Response above, so there is nothing to close. - _, _ = transport.NewImpersonatingRoundTripper(impersonateConfig, impersonateHeaderSpy).RoundTrip(fakeReq) - return newHeaders -} - -func isImpersonationHeader(header string) bool { - return strings.HasPrefix(http.CanonicalHeaderKey(header), "Impersonate") -} - -func extractToken(token string, jsonDecoder runtime.Decoder) (*login.TokenCredentialRequest, error) { - tokenCredentialRequestJSON, err := base64.StdEncoding.DecodeString(token) - if err != nil { - return nil, fmt.Errorf("invalid base64 in encoded bearer token: %w", err) - } - - obj, err := runtime.Decode(jsonDecoder, tokenCredentialRequestJSON) - if err != nil { - return nil, fmt.Errorf("invalid object encoded in bearer token: %w", err) - } - - tokenCredentialRequest, ok := obj.(*login.TokenCredentialRequest) - if !ok { - return nil, fmt.Errorf("invalid TokenCredentialRequest encoded in bearer token: got %T", obj) - } - - return tokenCredentialRequest, nil -} diff --git a/internal/concierge/impersonator/impersonator_test.go b/internal/concierge/impersonator/impersonator_test.go index 7a7124b2c..31e9aae93 100644 --- a/internal/concierge/impersonator/impersonator_test.go +++ b/internal/concierge/impersonator/impersonator_test.go @@ -5,40 +5,125 @@ package impersonator import ( "context" - "fmt" + "crypto/x509/pkix" + "net" "net/http" "net/http/httptest" "net/url" + "strconv" "testing" + "time" - "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/apimachinery/pkg/runtime/serializer" - "k8s.io/apiserver/pkg/authentication/authenticator" "k8s.io/apiserver/pkg/authentication/user" + "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/apiserver/pkg/features" + "k8s.io/apiserver/pkg/server/dynamiccertificates" + genericoptions "k8s.io/apiserver/pkg/server/options" + utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd/api" + featuregatetesting "k8s.io/component-base/featuregate/testing" - authenticationv1alpha1 "go.pinniped.dev/generated/latest/apis/concierge/authentication/v1alpha1" - "go.pinniped.dev/generated/latest/apis/concierge/login" - conciergescheme "go.pinniped.dev/internal/concierge/scheme" - "go.pinniped.dev/internal/controller/authenticator/authncache" - "go.pinniped.dev/internal/mocks/mocktokenauthenticator" + "go.pinniped.dev/internal/certauthority" + "go.pinniped.dev/internal/kubeclient" "go.pinniped.dev/internal/testutil" - "go.pinniped.dev/internal/testutil/impersonationtoken" - "go.pinniped.dev/internal/testutil/testlogger" ) -func TestImpersonator(t *testing.T) { - const ( - defaultAPIGroup = "pinniped.dev" - customAPIGroup = "walrus.tld" +func TestNew(t *testing.T) { + const port = 8444 - testUser = "test-user" - ) + ca, err := certauthority.New(pkix.Name{CommonName: "ca"}, time.Hour) + require.NoError(t, err) + cert, key, err := ca.IssuePEM(pkix.Name{CommonName: "example.com"}, []string{"example.com"}, time.Hour) + require.NoError(t, err) + certKeyContent, err := dynamiccertificates.NewStaticCertKeyContent("cert-key", cert, key) + require.NoError(t, err) + caContent, err := dynamiccertificates.NewStaticCAContent("ca", ca.Bundle()) + require.NoError(t, err) + + // Punch out just enough stuff to make New actually run without error. + recOpts := func(options *genericoptions.RecommendedOptions) { + options.Authentication.RemoteKubeConfigFileOptional = true + options.Authorization.RemoteKubeConfigFileOptional = true + options.CoreAPI = nil + options.Admission = nil + } + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.APIPriorityAndFairness, false)() + + tests := []struct { + name string + clientOpts []kubeclient.Option + wantErr string + }{ + { + name: "happy path", + clientOpts: []kubeclient.Option{ + kubeclient.WithConfig(&rest.Config{ + BearerToken: "should-be-ignored", + BearerTokenFile: "required-to-be-set", + }), + }, + }, + { + name: "no bearer token file", + clientOpts: []kubeclient.Option{ + kubeclient.WithConfig(&rest.Config{ + BearerToken: "should-be-ignored", + }), + }, + wantErr: "invalid impersonator loopback rest config has wrong bearer token semantics", + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + // This is a serial test because the production code binds to the port. + runner, constructionErr := newInternal(port, certKeyContent, caContent, tt.clientOpts, recOpts) + + if len(tt.wantErr) != 0 { + require.EqualError(t, constructionErr, tt.wantErr) + require.Nil(t, runner) + } else { + require.NoError(t, constructionErr) + require.NotNil(t, runner) + + stopCh := make(chan struct{}) + errCh := make(chan error) + go func() { + stopErr := runner(stopCh) + errCh <- stopErr + }() + + select { + case unexpectedExit := <-errCh: + t.Errorf("unexpected exit, err=%v (even nil error is failure)", unexpectedExit) + case <-time.After(10 * time.Second): + } + + close(stopCh) + exitErr := <-errCh + require.NoError(t, exitErr) + } + + // assert listener is closed is both cases above by trying to make another one on the same port + ln, _, listenErr := genericoptions.CreateListener("", "0.0.0.0:"+strconv.Itoa(port), net.ListenConfig{}) + defer func() { + if ln == nil { + return + } + require.NoError(t, ln.Close()) + }() + require.NoError(t, listenErr) + + // TODO: create some client certs and assert the authorizer works correctly with system:masters + // and nested impersonation - we could also try to test what headers are sent to KAS + }) + } +} + +func TestImpersonator(t *testing.T) { + const testUser = "test-user" testGroups := []string{"test-group-1", "test-group-2"} testExtra := map[string][]string{ @@ -47,167 +132,97 @@ func TestImpersonator(t *testing.T) { } validURL, _ := url.Parse("http://pinniped.dev/blah") - newRequest := func(h http.Header) *http.Request { - r, err := http.NewRequestWithContext(context.Background(), http.MethodGet, validURL.String(), nil) + newRequest := func(h http.Header, userInfo user.Info) *http.Request { + ctx := context.Background() + if userInfo != nil { + ctx = request.WithUser(ctx, userInfo) + } + r, err := http.NewRequestWithContext(ctx, http.MethodGet, validURL.String(), nil) require.NoError(t, err) r.Header = h return r } - goodAuthenticator := corev1.TypedLocalObjectReference{ - Name: "authenticator-one", - APIGroup: stringPtr(authenticationv1alpha1.GroupName), - } - badAuthenticator := corev1.TypedLocalObjectReference{ - Name: "", - APIGroup: stringPtr(authenticationv1alpha1.GroupName), - } - tests := []struct { name string - apiGroupOverride string - getKubeconfig func() (*rest.Config, error) + restConfig *rest.Config wantCreationErr string request *http.Request wantHTTPBody string wantHTTPStatus int - wantLogs []string wantKubeAPIServerRequestHeaders http.Header - wantKubeAPIServerStatusCode int - expectMockToken func(*testing.T, *mocktokenauthenticator.MockTokenMockRecorder) + kubeAPIServerStatusCode int }{ { - name: "fail to get in-cluster config", - getKubeconfig: func() (*rest.Config, error) { - return nil, fmt.Errorf("some kubernetes error") - }, - wantCreationErr: "could not get in-cluster config: some kubernetes error", - }, - { - name: "invalid kubeconfig host", - getKubeconfig: func() (*rest.Config, error) { - return &rest.Config{Host: ":"}, nil - }, + name: "invalid kubeconfig host", + restConfig: &rest.Config{Host: ":"}, wantCreationErr: "could not parse host URL from in-cluster config: parse \":\": missing protocol scheme", }, { name: "invalid transport config", - getKubeconfig: func() (*rest.Config, error) { - return &rest.Config{ - Host: "pinniped.dev/blah", - ExecProvider: &api.ExecConfig{}, - AuthProvider: &api.AuthProviderConfig{}, - }, nil + restConfig: &rest.Config{ + Host: "pinniped.dev/blah", + ExecProvider: &api.ExecConfig{}, + AuthProvider: &api.AuthProviderConfig{}, }, wantCreationErr: "could not get in-cluster transport config: execProvider and authProvider cannot be used in combination", }, { name: "fail to get transport from config", - getKubeconfig: func() (*rest.Config, error) { - return &rest.Config{ - Host: "pinniped.dev/blah", - BearerToken: "test-bearer-token", - Transport: http.DefaultTransport, - TLSClientConfig: rest.TLSClientConfig{Insecure: true}, - }, nil + restConfig: &rest.Config{ + Host: "pinniped.dev/blah", + BearerToken: "test-bearer-token", + Transport: http.DefaultTransport, + TLSClientConfig: rest.TLSClientConfig{Insecure: true}, }, wantCreationErr: "could not get in-cluster transport: using a custom transport with TLS certificate options or the insecure flag is not allowed", }, { name: "Impersonate-User header already in request", - request: newRequest(map[string][]string{"Impersonate-User": {"some-user"}}), - wantHTTPBody: "impersonation header already exists\n", - wantHTTPStatus: http.StatusBadRequest, - wantLogs: []string{"\"msg\"=\"impersonation header already exists\" \"error\"=\"\\\"Impersonate-User\\\" header already exists\" \"method\"=\"GET\" \"url\"=\"http://pinniped.dev/blah\""}, + request: newRequest(map[string][]string{"Impersonate-User": {"some-user"}}, nil), + wantHTTPBody: "invalid impersonation\n", + wantHTTPStatus: http.StatusInternalServerError, }, { name: "Impersonate-Group header already in request", - request: newRequest(map[string][]string{"Impersonate-Group": {"some-group"}}), - wantHTTPBody: "impersonation header already exists\n", - wantHTTPStatus: http.StatusBadRequest, - wantLogs: []string{"\"msg\"=\"impersonation header already exists\" \"error\"=\"\\\"Impersonate-Group\\\" header already exists\" \"method\"=\"GET\" \"url\"=\"http://pinniped.dev/blah\""}, + request: newRequest(map[string][]string{"Impersonate-Group": {"some-group"}}, nil), + wantHTTPBody: "invalid impersonation\n", + wantHTTPStatus: http.StatusInternalServerError, }, { name: "Impersonate-Extra header already in request", - request: newRequest(map[string][]string{"Impersonate-Extra-something": {"something"}}), - wantHTTPBody: "impersonation header already exists\n", - wantHTTPStatus: http.StatusBadRequest, - wantLogs: []string{"\"msg\"=\"impersonation header already exists\" \"error\"=\"\\\"Impersonate-Extra-something\\\" header already exists\" \"method\"=\"GET\" \"url\"=\"http://pinniped.dev/blah\""}, + request: newRequest(map[string][]string{"Impersonate-Extra-something": {"something"}}, nil), + wantHTTPBody: "invalid impersonation\n", + wantHTTPStatus: http.StatusInternalServerError, }, { - name: "Impersonate-* header already in request", - request: newRequest(map[string][]string{ - "Impersonate-Something": {"some-newfangled-impersonate-header"}, - }), - wantHTTPBody: "impersonation header already exists\n", - wantHTTPStatus: http.StatusBadRequest, - wantLogs: []string{"\"msg\"=\"impersonation header already exists\" \"error\"=\"\\\"Impersonate-Something\\\" header already exists\" \"method\"=\"GET\" \"url\"=\"http://pinniped.dev/blah\""}, + name: "Impersonate-* header already in request", + request: newRequest(map[string][]string{"Impersonate-Something": {"some-newfangled-impersonate-header"}}, nil), + wantHTTPBody: "invalid impersonation\n", + wantHTTPStatus: http.StatusInternalServerError, }, { - name: "missing authorization header", - request: newRequest(map[string][]string{}), - wantHTTPBody: "invalid token encoding\n", - wantHTTPStatus: http.StatusBadRequest, - wantLogs: []string{"\"msg\"=\"invalid token encoding\" \"error\"=\"token authenticator did not find token\" \"method\"=\"GET\" \"url\"=\"http://pinniped.dev/blah\""}, + name: "unexpected authorization header", + request: newRequest(map[string][]string{"Authorization": {"panda"}}, nil), + wantHTTPBody: "invalid authorization header\n", + wantHTTPStatus: http.StatusInternalServerError, }, { - name: "authorization header missing bearer prefix", - request: newRequest(map[string][]string{"Authorization": {impersonationtoken.Make(t, "test-token", &goodAuthenticator, defaultAPIGroup)}}), - wantHTTPBody: "invalid token encoding\n", - wantHTTPStatus: http.StatusBadRequest, - wantLogs: []string{"\"msg\"=\"invalid token encoding\" \"error\"=\"token authenticator did not find token\" \"method\"=\"GET\" \"url\"=\"http://pinniped.dev/blah\""}, + name: "missing user", + request: newRequest(map[string][]string{}, nil), + wantHTTPBody: "invalid user\n", + wantHTTPStatus: http.StatusInternalServerError, }, { - name: "token is not base64 encoded", - request: newRequest(map[string][]string{"Authorization": {"Bearer !!!"}}), - wantHTTPBody: "invalid token encoding\n", - wantHTTPStatus: http.StatusBadRequest, - wantLogs: []string{"\"msg\"=\"invalid token encoding\" \"error\"=\"invalid base64 in encoded bearer token: illegal base64 data at input byte 0\" \"method\"=\"GET\" \"url\"=\"http://pinniped.dev/blah\""}, - }, - { - name: "base64 encoded token is not valid json", - request: newRequest(map[string][]string{"Authorization": {"Bearer aGVsbG8gd29ybGQK"}}), // aGVsbG8gd29ybGQK is "hello world" base64 encoded - wantHTTPBody: "invalid token encoding\n", - wantHTTPStatus: http.StatusBadRequest, - wantLogs: []string{"\"msg\"=\"invalid token encoding\" \"error\"=\"invalid object encoded in bearer token: couldn't get version/kind; json parse error: invalid character 'h' looking for beginning of value\" \"method\"=\"GET\" \"url\"=\"http://pinniped.dev/blah\""}, - }, - { - name: "base64 encoded token is encoded with default api group but we are expecting custom api group", - apiGroupOverride: customAPIGroup, - request: newRequest(map[string][]string{"Authorization": {"Bearer " + impersonationtoken.Make(t, "test-token", &goodAuthenticator, defaultAPIGroup)}}), - wantHTTPBody: "invalid token encoding\n", - wantHTTPStatus: http.StatusBadRequest, - wantLogs: []string{"\"msg\"=\"invalid token encoding\" \"error\"=\"invalid object encoded in bearer token: no kind \\\"TokenCredentialRequest\\\" is registered for version \\\"login.concierge.pinniped.dev/v1alpha1\\\" in scheme \\\"pkg/runtime/scheme.go:100\\\"\" \"method\"=\"GET\" \"url\"=\"http://pinniped.dev/blah\""}, - }, - { - name: "base64 encoded token is encoded with custom api group but we are expecting default api group", - request: newRequest(map[string][]string{"Authorization": {"Bearer " + impersonationtoken.Make(t, "test-token", &goodAuthenticator, customAPIGroup)}}), - wantHTTPBody: "invalid token encoding\n", - wantHTTPStatus: http.StatusBadRequest, - wantLogs: []string{"\"msg\"=\"invalid token encoding\" \"error\"=\"invalid object encoded in bearer token: no kind \\\"TokenCredentialRequest\\\" is registered for version \\\"login.concierge.walrus.tld/v1alpha1\\\" in scheme \\\"pkg/runtime/scheme.go:100\\\"\" \"method\"=\"GET\" \"url\"=\"http://pinniped.dev/blah\""}, - }, - { - name: "token could not be authenticated", - request: newRequest(map[string][]string{"Authorization": {"Bearer " + impersonationtoken.Make(t, "", &badAuthenticator, defaultAPIGroup)}}), - wantHTTPBody: "invalid token\n", - wantHTTPStatus: http.StatusUnauthorized, - wantLogs: []string{"\"msg\"=\"received invalid token\" \"error\"=\"no such authenticator\" \"authenticator\"={\"apiGroup\":\"authentication.concierge.pinniped.dev\",\"kind\":\"\",\"name\":\"\"} \"method\"=\"GET\" \"url\"=\"http://pinniped.dev/blah\""}, - }, - { - name: "token authenticates as nil", - request: newRequest(map[string][]string{"Authorization": {"Bearer " + impersonationtoken.Make(t, "test-token", &goodAuthenticator, defaultAPIGroup)}}), - expectMockToken: func(t *testing.T, recorder *mocktokenauthenticator.MockTokenMockRecorder) { - recorder.AuthenticateToken(gomock.Any(), "test-token").Return(nil, false, nil) - }, - wantHTTPBody: "not authenticated\n", - wantHTTPStatus: http.StatusUnauthorized, - wantLogs: []string{"\"level\"=0 \"msg\"=\"received token that did not authenticate\" \"authenticator\"={\"apiGroup\":\"authentication.concierge.pinniped.dev\",\"kind\":\"\",\"name\":\"authenticator-one\"} \"method\"=\"GET\" \"url\"=\"http://pinniped.dev/blah\""}, + name: "unexpected UID", + request: newRequest(map[string][]string{}, &user.DefaultInfo{UID: "007"}), + wantHTTPBody: "unexpected uid\n", + wantHTTPStatus: http.StatusUnprocessableEntity, }, // happy path { - name: "token validates", + name: "authenticated user", request: newRequest(map[string][]string{ - "Authorization": {"Bearer " + impersonationtoken.Make(t, "test-token", &goodAuthenticator, defaultAPIGroup)}, "User-Agent": {"test-user-agent"}, "Accept": {"some-accepted-format"}, "Accept-Encoding": {"some-accepted-encoding"}, @@ -216,17 +231,11 @@ func TestImpersonator(t *testing.T) { "Content-Type": {"some-type"}, "Content-Length": {"some-length"}, "Other-Header": {"test-header-value-1"}, // this header will be passed through + }, &user.DefaultInfo{ + Name: testUser, + Groups: testGroups, + Extra: testExtra, }), - expectMockToken: func(t *testing.T, recorder *mocktokenauthenticator.MockTokenMockRecorder) { - userInfo := user.DefaultInfo{ - Name: testUser, - Groups: testGroups, - UID: "test-uid", - Extra: testExtra, - } - response := &authenticator.Response{User: &userInfo} - recorder.AuthenticateToken(gomock.Any(), "test-token").Return(response, true, nil) - }, wantKubeAPIServerRequestHeaders: map[string][]string{ "Authorization": {"Bearer some-service-account-token"}, "Impersonate-Extra-Extra-1": {"some", "extra", "stuff"}, @@ -243,25 +252,17 @@ func TestImpersonator(t *testing.T) { }, wantHTTPBody: "successful proxied response", wantHTTPStatus: http.StatusOK, - wantLogs: []string{"\"level\"=0 \"msg\"=\"proxying authenticated request\" \"authenticator\"={\"apiGroup\":\"authentication.concierge.pinniped.dev\",\"kind\":\"\",\"name\":\"authenticator-one\"} \"method\"=\"GET\" \"url\"=\"http://pinniped.dev/blah\" \"userID\"=\"test-uid\""}, }, { - name: "token validates and the kube API request returns an error", + name: "user is authenticated but the kube API request returns an error", request: newRequest(map[string][]string{ - "Authorization": {"Bearer " + impersonationtoken.Make(t, "test-token", &goodAuthenticator, defaultAPIGroup)}, - "User-Agent": {"test-user-agent"}, + "User-Agent": {"test-user-agent"}, + }, &user.DefaultInfo{ + Name: testUser, + Groups: testGroups, + Extra: testExtra, }), - expectMockToken: func(t *testing.T, recorder *mocktokenauthenticator.MockTokenMockRecorder) { - userInfo := user.DefaultInfo{ - Name: testUser, - Groups: testGroups, - UID: "test-uid", - Extra: testExtra, - } - response := &authenticator.Response{User: &userInfo} - recorder.AuthenticateToken(gomock.Any(), "test-token").Return(response, true, nil) - }, - wantKubeAPIServerStatusCode: http.StatusNotFound, + kubeAPIServerStatusCode: http.StatusNotFound, wantKubeAPIServerRequestHeaders: map[string][]string{ "Accept-Encoding": {"gzip"}, // because the rest client used in this test does not disable compression "Authorization": {"Bearer some-service-account-token"}, @@ -272,56 +273,14 @@ func TestImpersonator(t *testing.T) { "User-Agent": {"test-user-agent"}, }, wantHTTPStatus: http.StatusNotFound, - wantLogs: []string{"\"level\"=0 \"msg\"=\"proxying authenticated request\" \"authenticator\"={\"apiGroup\":\"authentication.concierge.pinniped.dev\",\"kind\":\"\",\"name\":\"authenticator-one\"} \"method\"=\"GET\" \"url\"=\"http://pinniped.dev/blah\" \"userID\"=\"test-uid\""}, - }, - { - name: "token validates with custom api group", - apiGroupOverride: customAPIGroup, - request: newRequest(map[string][]string{ - "Authorization": {"Bearer " + impersonationtoken.Make(t, "test-token", &goodAuthenticator, customAPIGroup)}, - "Other-Header": {"test-header-value-1"}, - "User-Agent": {"test-user-agent"}, - }), - expectMockToken: func(t *testing.T, recorder *mocktokenauthenticator.MockTokenMockRecorder) { - userInfo := user.DefaultInfo{ - Name: testUser, - Groups: testGroups, - UID: "test-uid", - Extra: testExtra, - } - response := &authenticator.Response{User: &userInfo} - recorder.AuthenticateToken(gomock.Any(), "test-token").Return(response, true, nil) - }, - wantKubeAPIServerRequestHeaders: map[string][]string{ - "Accept-Encoding": {"gzip"}, // because the rest client used in this test does not disable compression - "Authorization": {"Bearer some-service-account-token"}, - "Impersonate-Extra-Extra-1": {"some", "extra", "stuff"}, - "Impersonate-Extra-Extra-2": {"some", "more", "extra", "stuff"}, - "Impersonate-Group": {"test-group-1", "test-group-2"}, - "Impersonate-User": {"test-user"}, - "User-Agent": {"test-user-agent"}, - "Other-Header": {"test-header-value-1"}, - }, - wantHTTPBody: "successful proxied response", - wantHTTPStatus: http.StatusOK, - wantLogs: []string{"\"level\"=0 \"msg\"=\"proxying authenticated request\" \"authenticator\"={\"apiGroup\":\"authentication.concierge.pinniped.dev\",\"kind\":\"\",\"name\":\"authenticator-one\"} \"method\"=\"GET\" \"url\"=\"http://pinniped.dev/blah\" \"userID\"=\"test-uid\""}, }, } for _, tt := range tests { tt := tt - testLog := testlogger.New(t) t.Run(tt.name, func(t *testing.T) { - defer func() { - if t.Failed() { - for i, line := range testLog.Lines() { - t.Logf("testLog line %d: %q", i+1, line) - } - } - }() - - if tt.wantKubeAPIServerStatusCode == 0 { - tt.wantKubeAPIServerStatusCode = http.StatusOK + if tt.kubeAPIServerStatusCode == 0 { + tt.kubeAPIServerStatusCode = http.StatusOK } serverWasCalled := false @@ -329,8 +288,8 @@ func TestImpersonator(t *testing.T) { testServerCA, testServerURL := testutil.TLSTestServer(t, func(w http.ResponseWriter, r *http.Request) { serverWasCalled = true serverSawHeaders = r.Header - if tt.wantKubeAPIServerStatusCode != http.StatusOK { - w.WriteHeader(tt.wantKubeAPIServerStatusCode) + if tt.kubeAPIServerStatusCode != http.StatusOK { + w.WriteHeader(tt.kubeAPIServerStatusCode) } else { _, _ = w.Write([]byte("successful proxied response")) } @@ -340,30 +299,11 @@ func TestImpersonator(t *testing.T) { BearerToken: "some-service-account-token", TLSClientConfig: rest.TLSClientConfig{CAData: []byte(testServerCA)}, } - if tt.getKubeconfig == nil { - tt.getKubeconfig = func() (*rest.Config, error) { - return &testServerKubeconfig, nil - } + if tt.restConfig == nil { + tt.restConfig = &testServerKubeconfig } - // stole this from cache_test, hopefully it is sufficient - cacheWithMockAuthenticator := authncache.New() - ctrl := gomock.NewController(t) - defer ctrl.Finish() - key := authncache.Key{Name: "authenticator-one", APIGroup: *goodAuthenticator.APIGroup} - mockToken := mocktokenauthenticator.NewMockToken(ctrl) - cacheWithMockAuthenticator.Store(key, mockToken) - - if tt.expectMockToken != nil { - tt.expectMockToken(t, mockToken.EXPECT()) - } - - apiGroup := defaultAPIGroup - if tt.apiGroupOverride != "" { - apiGroup = tt.apiGroupOverride - } - - proxy, err := newInternal(cacheWithMockAuthenticator, makeDecoder(t, apiGroup), testLog, tt.getKubeconfig) + proxy, err := newImpersonationReverseProxy(tt.restConfig) if tt.wantCreationErr != "" { require.EqualError(t, err, tt.wantCreationErr) return @@ -380,11 +320,8 @@ func TestImpersonator(t *testing.T) { if tt.wantHTTPBody != "" { require.Equal(t, tt.wantHTTPBody, w.Body.String()) } - if tt.wantLogs != nil { - require.Equal(t, tt.wantLogs, testLog.Lines()) - } - if tt.wantHTTPStatus == http.StatusOK || tt.wantKubeAPIServerStatusCode != http.StatusOK { + if tt.wantHTTPStatus == http.StatusOK || tt.kubeAPIServerStatusCode != http.StatusOK { require.True(t, serverWasCalled, "Should have proxied the request to the Kube API server, but didn't") require.Equal(t, tt.wantKubeAPIServerRequestHeaders, serverSawHeaders) } else { @@ -393,19 +330,3 @@ func TestImpersonator(t *testing.T) { }) } } - -func stringPtr(s string) *string { return &s } - -func makeDecoder(t *testing.T, apiGroupSuffix string) runtime.Decoder { - t.Helper() - - scheme, loginGV, _ := conciergescheme.New(apiGroupSuffix) - codecs := serializer.NewCodecFactory(scheme) - respInfo, ok := runtime.SerializerInfoForMediaType(codecs.SupportedMediaTypes(), runtime.ContentTypeJSON) - require.True(t, ok, "couldn't find serializer info for media type") - - return codecs.DecoderToVersion(respInfo.Serializer, schema.GroupVersion{ - Group: loginGV.Group, - Version: login.SchemeGroupVersion.Version, - }) -} diff --git a/internal/concierge/server/server.go b/internal/concierge/server/server.go index cc6233933..d485ee199 100644 --- a/internal/concierge/server/server.go +++ b/internal/concierge/server/server.go @@ -17,7 +17,6 @@ import ( genericapiserver "k8s.io/apiserver/pkg/server" genericoptions "k8s.io/apiserver/pkg/server/options" - loginapi "go.pinniped.dev/generated/latest/apis/concierge/login" "go.pinniped.dev/internal/certauthority/dynamiccertauthority" "go.pinniped.dev/internal/concierge/apiserver" conciergescheme "go.pinniped.dev/internal/concierge/scheme" @@ -27,6 +26,7 @@ import ( "go.pinniped.dev/internal/downward" "go.pinniped.dev/internal/dynamiccert" "go.pinniped.dev/internal/here" + "go.pinniped.dev/internal/issuer" "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/registry/credentialrequest" ) @@ -116,10 +116,14 @@ func (a *App) runServer(ctx context.Context) error { // keep incoming requests fast. dynamicServingCertProvider := dynamiccert.New() - // This cert provider will be used to provide a signing key to the + // This cert provider will be used to provide the Kube signing key to the // cert issuer used to issue certs to Pinniped clients wishing to login. dynamicSigningCertProvider := dynamiccert.New() + // This cert provider will be used to provide the impersonation proxy signing key to the + // cert issuer used to issue certs to Pinniped clients wishing to login. + impersonationProxySigningCertProvider := dynamiccert.New() + // Get the "real" name of the login concierge API group (i.e., the API group name with the // injected suffix). scheme, loginGV, identityGV := conciergescheme.New(*cfg.APIGroupSuffix) @@ -128,29 +132,34 @@ func (a *App) runServer(ctx context.Context) error { // post start hook of the aggregated API server. startControllersFunc, err := controllermanager.PrepareControllers( &controllermanager.Config{ - ServerInstallationInfo: podInfo, - APIGroupSuffix: *cfg.APIGroupSuffix, - NamesConfig: &cfg.NamesConfig, - Labels: cfg.Labels, - KubeCertAgentConfig: &cfg.KubeCertAgentConfig, - DiscoveryURLOverride: cfg.DiscoveryInfo.URL, - DynamicServingCertProvider: dynamicServingCertProvider, - DynamicSigningCertProvider: dynamicSigningCertProvider, - ServingCertDuration: time.Duration(*cfg.APIConfig.ServingCertificateConfig.DurationSeconds) * time.Second, - ServingCertRenewBefore: time.Duration(*cfg.APIConfig.ServingCertificateConfig.RenewBeforeSeconds) * time.Second, - AuthenticatorCache: authenticators, - LoginJSONDecoder: getLoginJSONDecoder(loginGV.Group, scheme), + ServerInstallationInfo: podInfo, + APIGroupSuffix: *cfg.APIGroupSuffix, + NamesConfig: &cfg.NamesConfig, + Labels: cfg.Labels, + KubeCertAgentConfig: &cfg.KubeCertAgentConfig, + DiscoveryURLOverride: cfg.DiscoveryInfo.URL, + DynamicServingCertProvider: dynamicServingCertProvider, + DynamicSigningCertProvider: dynamicSigningCertProvider, + ImpersonationSigningCertProvider: impersonationProxySigningCertProvider, + ServingCertDuration: time.Duration(*cfg.APIConfig.ServingCertificateConfig.DurationSeconds) * time.Second, + ServingCertRenewBefore: time.Duration(*cfg.APIConfig.ServingCertificateConfig.RenewBeforeSeconds) * time.Second, + AuthenticatorCache: authenticators, }, ) if err != nil { return fmt.Errorf("could not prepare controllers: %w", err) } + certIssuer := issuer.CertIssuers{ + dynamiccertauthority.New(dynamicSigningCertProvider), // attempt to use the real Kube CA if possible + dynamiccertauthority.New(impersonationProxySigningCertProvider), // fallback to our internal CA if we need to + } + // Get the aggregated API server config. aggregatedAPIServerConfig, err := getAggregatedAPIServerConfig( dynamicServingCertProvider, authenticators, - dynamiccertauthority.New(dynamicSigningCertProvider), + certIssuer, startControllersFunc, *cfg.APIGroupSuffix, scheme, @@ -175,7 +184,7 @@ func (a *App) runServer(ctx context.Context) error { func getAggregatedAPIServerConfig( dynamicCertProvider dynamiccert.Provider, authenticator credentialrequest.TokenCredentialRequestAuthenticator, - issuer credentialrequest.CertIssuer, + issuer issuer.CertIssuer, startControllersPostStartHook func(context.Context), apiGroupSuffix string, scheme *runtime.Scheme, @@ -222,16 +231,3 @@ func getAggregatedAPIServerConfig( } return apiServerConfig, nil } - -func getLoginJSONDecoder(loginConciergeAPIGroup string, loginConciergeScheme *runtime.Scheme) runtime.Decoder { - scheme := loginConciergeScheme - codecs := serializer.NewCodecFactory(scheme) - respInfo, ok := runtime.SerializerInfoForMediaType(codecs.SupportedMediaTypes(), runtime.ContentTypeJSON) - if !ok { - panic(fmt.Errorf("unknown content type: %s ", runtime.ContentTypeJSON)) // static input, programmer error - } - return codecs.DecoderToVersion(respInfo.Serializer, schema.GroupVersion{ - Group: loginConciergeAPIGroup, - Version: loginapi.SchemeGroupVersion.Version, - }) -} diff --git a/internal/config/concierge/config.go b/internal/config/concierge/config.go index b6720cea4..b4d4c9a5e 100644 --- a/internal/config/concierge/config.go +++ b/internal/config/concierge/config.go @@ -119,6 +119,9 @@ func validateNames(names *NamesConfigSpec) error { if names.ImpersonationCACertificateSecret == "" { missingNames = append(missingNames, "impersonationCACertificateSecret") } + if names.ImpersonationSignerSecret == "" { + missingNames = append(missingNames, "impersonationSignerSecret") + } if len(missingNames) > 0 { return constable.Error("missing required names: " + strings.Join(missingNames, ", ")) } diff --git a/internal/config/concierge/config_test.go b/internal/config/concierge/config_test.go index 5b8177ff4..0034d06b4 100644 --- a/internal/config/concierge/config_test.go +++ b/internal/config/concierge/config_test.go @@ -41,6 +41,8 @@ func TestFromPath(t *testing.T) { impersonationLoadBalancerService: impersonationLoadBalancerService-value impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value impersonationCACertificateSecret: impersonationCACertificateSecret-value + impersonationSignerSecret: impersonationSignerSecret-value + impersonationSignerSecret: impersonationSignerSecret-value labels: myLabelKey1: myLabelValue1 myLabelKey2: myLabelValue2 @@ -69,6 +71,7 @@ func TestFromPath(t *testing.T) { ImpersonationLoadBalancerService: "impersonationLoadBalancerService-value", ImpersonationTLSCertificateSecret: "impersonationTLSCertificateSecret-value", ImpersonationCACertificateSecret: "impersonationCACertificateSecret-value", + ImpersonationSignerSecret: "impersonationSignerSecret-value", }, Labels: map[string]string{ "myLabelKey1": "myLabelValue1", @@ -94,6 +97,7 @@ func TestFromPath(t *testing.T) { impersonationLoadBalancerService: impersonationLoadBalancerService-value impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value impersonationCACertificateSecret: impersonationCACertificateSecret-value + impersonationSignerSecret: impersonationSignerSecret-value `), wantConfig: &Config{ DiscoveryInfo: DiscoveryInfoSpec{ @@ -114,6 +118,7 @@ func TestFromPath(t *testing.T) { ImpersonationLoadBalancerService: "impersonationLoadBalancerService-value", ImpersonationTLSCertificateSecret: "impersonationTLSCertificateSecret-value", ImpersonationCACertificateSecret: "impersonationCACertificateSecret-value", + ImpersonationSignerSecret: "impersonationSignerSecret-value", }, Labels: map[string]string{}, KubeCertAgentConfig: KubeCertAgentSpec{ @@ -127,7 +132,8 @@ func TestFromPath(t *testing.T) { yaml: here.Doc(``), wantError: "validate names: missing required names: servingCertificateSecret, credentialIssuer, " + "apiService, impersonationConfigMap, impersonationLoadBalancerService, " + - "impersonationTLSCertificateSecret, impersonationCACertificateSecret", + "impersonationTLSCertificateSecret, impersonationCACertificateSecret, " + + "impersonationSignerSecret", }, { name: "Missing apiService name", @@ -140,6 +146,7 @@ func TestFromPath(t *testing.T) { impersonationLoadBalancerService: impersonationLoadBalancerService-value impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value impersonationCACertificateSecret: impersonationCACertificateSecret-value + impersonationSignerSecret: impersonationSignerSecret-value `), wantError: "validate names: missing required names: apiService", }, @@ -154,6 +161,7 @@ func TestFromPath(t *testing.T) { impersonationLoadBalancerService: impersonationLoadBalancerService-value impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value impersonationCACertificateSecret: impersonationCACertificateSecret-value + impersonationSignerSecret: impersonationSignerSecret-value `), wantError: "validate names: missing required names: credentialIssuer", }, @@ -168,6 +176,7 @@ func TestFromPath(t *testing.T) { impersonationLoadBalancerService: impersonationLoadBalancerService-value impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value impersonationCACertificateSecret: impersonationCACertificateSecret-value + impersonationSignerSecret: impersonationSignerSecret-value `), wantError: "validate names: missing required names: servingCertificateSecret", }, @@ -182,6 +191,7 @@ func TestFromPath(t *testing.T) { impersonationLoadBalancerService: impersonationLoadBalancerService-value impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value impersonationCACertificateSecret: impersonationCACertificateSecret-value + impersonationSignerSecret: impersonationSignerSecret-value `), wantError: "validate names: missing required names: impersonationConfigMap", }, @@ -196,6 +206,7 @@ func TestFromPath(t *testing.T) { impersonationConfigMap: impersonationConfigMap-value impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value impersonationCACertificateSecret: impersonationCACertificateSecret-value + impersonationSignerSecret: impersonationSignerSecret-value `), wantError: "validate names: missing required names: impersonationLoadBalancerService", }, @@ -210,6 +221,7 @@ func TestFromPath(t *testing.T) { impersonationConfigMap: impersonationConfigMap-value impersonationLoadBalancerService: impersonationLoadBalancerService-value impersonationCACertificateSecret: impersonationCACertificateSecret-value + impersonationSignerSecret: impersonationSignerSecret-value `), wantError: "validate names: missing required names: impersonationTLSCertificateSecret", }, @@ -224,9 +236,25 @@ func TestFromPath(t *testing.T) { impersonationConfigMap: impersonationConfigMap-value impersonationLoadBalancerService: impersonationLoadBalancerService-value impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value + impersonationSignerSecret: impersonationSignerSecret-value `), wantError: "validate names: missing required names: impersonationCACertificateSecret", }, + { + name: "Missing impersonationSignerSecret name", + yaml: here.Doc(` + --- + names: + servingCertificateSecret: pinniped-concierge-api-tls-serving-certificate + credentialIssuer: pinniped-config + apiService: pinniped-api + impersonationConfigMap: impersonationConfigMap-value + impersonationLoadBalancerService: impersonationLoadBalancerService-value + impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value + impersonationCACertificateSecret: impersonationCACertificateSecret-value + `), + wantError: "validate names: missing required names: impersonationSignerSecret", + }, { name: "Missing several required names", yaml: here.Doc(` @@ -236,6 +264,7 @@ func TestFromPath(t *testing.T) { credentialIssuer: pinniped-config apiService: pinniped-api impersonationLoadBalancerService: impersonationLoadBalancerService-value + impersonationSignerSecret: impersonationSignerSecret-value `), wantError: "validate names: missing required names: impersonationConfigMap, " + "impersonationTLSCertificateSecret, impersonationCACertificateSecret", @@ -256,6 +285,7 @@ func TestFromPath(t *testing.T) { impersonationLoadBalancerService: impersonationLoadBalancerService-value impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value impersonationCACertificateSecret: impersonationCACertificateSecret-value + impersonationSignerSecret: impersonationSignerSecret-value `), wantError: "validate api: durationSeconds cannot be smaller than renewBeforeSeconds", }, @@ -275,6 +305,7 @@ func TestFromPath(t *testing.T) { impersonationLoadBalancerService: impersonationLoadBalancerService-value impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value impersonationCACertificateSecret: impersonationCACertificateSecret-value + impersonationSignerSecret: impersonationSignerSecret-value `), wantError: "validate api: renewBefore must be positive", }, @@ -294,6 +325,7 @@ func TestFromPath(t *testing.T) { impersonationLoadBalancerService: impersonationLoadBalancerService-value impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value impersonationCACertificateSecret: impersonationCACertificateSecret-value + impersonationSignerSecret: impersonationSignerSecret-value `), wantError: "validate api: renewBefore must be positive", }, @@ -314,6 +346,7 @@ func TestFromPath(t *testing.T) { impersonationLoadBalancerService: impersonationLoadBalancerService-value impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value impersonationCACertificateSecret: impersonationCACertificateSecret-value + impersonationSignerSecret: impersonationSignerSecret-value `), wantError: "validate apiGroupSuffix: a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')", }, diff --git a/internal/config/concierge/types.go b/internal/config/concierge/types.go index 2a151274c..cfec621ef 100644 --- a/internal/config/concierge/types.go +++ b/internal/config/concierge/types.go @@ -40,6 +40,7 @@ type NamesConfigSpec struct { ImpersonationLoadBalancerService string `json:"impersonationLoadBalancerService"` ImpersonationTLSCertificateSecret string `json:"impersonationTLSCertificateSecret"` ImpersonationCACertificateSecret string `json:"impersonationCACertificateSecret"` + ImpersonationSignerSecret string `json:"impersonationSignerSecret"` } // ServingCertificateConfigSpec contains the configuration knobs for the API's diff --git a/internal/controller/apicerts/apiservice_updater.go b/internal/controller/apicerts/apiservice_updater.go index 4e28e519f..3cfe4fcc6 100644 --- a/internal/controller/apicerts/apiservice_updater.go +++ b/internal/controller/apicerts/apiservice_updater.go @@ -1,4 +1,4 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package apicerts @@ -64,7 +64,7 @@ func (c *apiServiceUpdaterController) Sync(ctx controllerlib.Context) error { } // Update the APIService to give it the new CA bundle. - if err := UpdateAPIService(ctx.Context, c.aggregatorClient, c.apiServiceName, c.namespace, certSecret.Data[caCertificateSecretKey]); err != nil { + if err := UpdateAPIService(ctx.Context, c.aggregatorClient, c.apiServiceName, c.namespace, certSecret.Data[CACertificateSecretKey]); err != nil { return fmt.Errorf("could not update the API service: %w", err) } diff --git a/internal/controller/apicerts/certs_expirer.go b/internal/controller/apicerts/certs_expirer.go index 336f619f7..2b643c3e3 100644 --- a/internal/controller/apicerts/certs_expirer.go +++ b/internal/controller/apicerts/certs_expirer.go @@ -1,4 +1,4 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package apicerts @@ -30,6 +30,8 @@ type certsExpirerController struct { // renewBefore is the amount of time after the cert's issuance where // this controller will start to try to rotate it. renewBefore time.Duration + + secretKey string } // NewCertsExpirerController returns a controllerlib.Controller that will delete a @@ -42,6 +44,7 @@ func NewCertsExpirerController( secretInformer corev1informers.SecretInformer, withInformer pinnipedcontroller.WithInformerOptionFunc, renewBefore time.Duration, + secretKey string, ) controllerlib.Controller { return controllerlib.New( controllerlib.Config{ @@ -52,6 +55,7 @@ func NewCertsExpirerController( k8sClient: k8sClient, secretInformer: secretInformer, renewBefore: renewBefore, + secretKey: secretKey, }, }, withInformer( @@ -74,13 +78,9 @@ func (c *certsExpirerController) Sync(ctx controllerlib.Context) error { return nil } - notBefore, notAfter, err := getCertBounds(secret) + notBefore, notAfter, err := c.getCertBounds(secret) if err != nil { - // If we can't read the cert, then really all we can do is log something, - // since if we returned an error then the controller lib would just call us - // again and again, which would probably yield the same results. - klog.Warningf("certsExpirerController Sync found that the secret is malformed: %s", err.Error()) - return nil + return fmt.Errorf("failed to get cert bounds for secret %q with key %q: %w", secret.Name, c.secretKey, err) } certAge := time.Since(notBefore) @@ -105,8 +105,8 @@ func (c *certsExpirerController) Sync(ctx controllerlib.Context) error { // certificate in the provided secret, or an error. Not that it expects the // provided secret to contain the well-known data keys from this package (see // certs_manager.go). -func getCertBounds(secret *corev1.Secret) (time.Time, time.Time, error) { - certPEM := secret.Data[tlsCertificateChainSecretKey] +func (c *certsExpirerController) getCertBounds(secret *corev1.Secret) (time.Time, time.Time, error) { + certPEM := secret.Data[c.secretKey] if certPEM == nil { return time.Time{}, time.Time{}, constable.Error("failed to find certificate") } diff --git a/internal/controller/apicerts/certs_expirer_test.go b/internal/controller/apicerts/certs_expirer_test.go index f8f165956..7e82819a8 100644 --- a/internal/controller/apicerts/certs_expirer_test.go +++ b/internal/controller/apicerts/certs_expirer_test.go @@ -98,7 +98,8 @@ func TestExpirerControllerFilters(t *testing.T) { nil, // k8sClient, not needed secretsInformer, withInformer.WithInformer, - 0, // renewBefore, not needed + 0, // renewBefore, not needed + "", // not needed ) unrelated := corev1.Secret{} @@ -115,6 +116,7 @@ func TestExpirerControllerSync(t *testing.T) { t.Parallel() const certsSecretResourceName = "some-resource-name" + const fakeTestKey = "some-awesome-key" tests := []struct { name string @@ -132,6 +134,7 @@ func TestExpirerControllerSync(t *testing.T) { name: "secret missing key", fillSecretData: func(t *testing.T, m map[string][]byte) {}, wantDelete: false, + wantError: `failed to get cert bounds for secret "some-resource-name" with key "some-awesome-key": failed to find certificate`, }, { name: "lifetime below threshold", @@ -143,8 +146,7 @@ func TestExpirerControllerSync(t *testing.T) { ) require.NoError(t, err) - // See certs_manager.go for this constant. - m["tlsCertificateChain"] = certPEM + m[fakeTestKey] = certPEM }, wantDelete: false, }, @@ -158,8 +160,7 @@ func TestExpirerControllerSync(t *testing.T) { ) require.NoError(t, err) - // See certs_manager.go for this constant. - m["tlsCertificateChain"] = certPEM + m[fakeTestKey] = certPEM }, wantDelete: true, }, @@ -173,8 +174,7 @@ func TestExpirerControllerSync(t *testing.T) { ) require.NoError(t, err) - // See certs_manager.go for this constant. - m["tlsCertificateChain"] = certPEM + m[fakeTestKey] = certPEM }, wantDelete: true, }, @@ -188,8 +188,7 @@ func TestExpirerControllerSync(t *testing.T) { ) require.NoError(t, err) - // See certs_manager.go for this constant. - m["tlsCertificateChain"] = certPEM + m[fakeTestKey] = certPEM }, configKubeAPIClient: func(c *kubernetesfake.Clientset) { c.PrependReactor("delete", "secrets", func(_ kubetesting.Action) (bool, runtime.Object, error) { @@ -204,11 +203,11 @@ func TestExpirerControllerSync(t *testing.T) { privateKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) require.NoError(t, err) - // See certs_manager.go for this constant. - m["tlsCertificateChain"], err = x509.MarshalPKCS8PrivateKey(privateKey) + m[fakeTestKey], err = x509.MarshalPKCS8PrivateKey(privateKey) require.NoError(t, err) }, wantDelete: false, + wantError: `failed to get cert bounds for secret "some-resource-name" with key "some-awesome-key": failed to decode certificate PEM`, }, } for _, test := range tests { @@ -253,6 +252,7 @@ func TestExpirerControllerSync(t *testing.T) { kubeInformers.Core().V1().Secrets(), controllerlib.WithInformer, test.renewBefore, + fakeTestKey, ) // Must start informers before calling TestRunSynchronously(). diff --git a/internal/controller/apicerts/certs_manager.go b/internal/controller/apicerts/certs_manager.go index 22f0f6df7..f0c11d0de 100644 --- a/internal/controller/apicerts/certs_manager.go +++ b/internal/controller/apicerts/certs_manager.go @@ -21,9 +21,10 @@ import ( ) const ( - caCertificateSecretKey = "caCertificate" - tlsPrivateKeySecretKey = "tlsPrivateKey" - tlsCertificateChainSecretKey = "tlsCertificateChain" + CACertificateSecretKey = "caCertificate" + CACertificatePrivateKeySecretKey = "caCertificatePrivateKey" + tlsPrivateKeySecretKey = "tlsPrivateKey" + TLSCertificateChainSecretKey = "tlsCertificateChain" ) type certsManagerController struct { @@ -98,23 +99,11 @@ func (c *certsManagerController) Sync(ctx controllerlib.Context) error { return fmt.Errorf("could not initialize CA: %w", err) } - // Using the CA from above, create a TLS server cert. - serviceEndpoint := c.serviceNameForGeneratedCertCommonName + "." + c.namespace + ".svc" - tlsCert, err := ca.Issue( - pkix.Name{CommonName: serviceEndpoint}, - []string{serviceEndpoint}, - nil, - c.certDuration, - ) + caPrivateKeyPEM, err := ca.PrivateKeyToPEM() if err != nil { - return fmt.Errorf("could not issue serving certificate: %w", err) + return fmt.Errorf("could not get CA private key: %w", err) } - // Write the CA's public key bundle and the serving certs to a secret. - tlsCertChainPEM, tlsPrivateKeyPEM, err := certauthority.ToPEM(tlsCert) - if err != nil { - return fmt.Errorf("could not PEM encode serving certificate: %w", err) - } secret := corev1.Secret{ TypeMeta: metav1.TypeMeta{}, ObjectMeta: metav1.ObjectMeta{ @@ -123,11 +112,34 @@ func (c *certsManagerController) Sync(ctx controllerlib.Context) error { Labels: c.certsSecretLabels, }, StringData: map[string]string{ - caCertificateSecretKey: string(ca.Bundle()), - tlsPrivateKeySecretKey: string(tlsPrivateKeyPEM), - tlsCertificateChainSecretKey: string(tlsCertChainPEM), + CACertificateSecretKey: string(ca.Bundle()), + CACertificatePrivateKeySecretKey: string(caPrivateKeyPEM), }, } + + // Using the CA from above, create a TLS server cert if we have service name. + if len(c.serviceNameForGeneratedCertCommonName) != 0 { + serviceEndpoint := c.serviceNameForGeneratedCertCommonName + "." + c.namespace + ".svc" + tlsCert, err := ca.Issue( + pkix.Name{CommonName: serviceEndpoint}, + []string{serviceEndpoint}, + nil, + c.certDuration, + ) + if err != nil { + return fmt.Errorf("could not issue serving certificate: %w", err) + } + + // Write the CA's public key bundle and the serving certs to a secret. + tlsCertChainPEM, tlsPrivateKeyPEM, err := certauthority.ToPEM(tlsCert) + if err != nil { + return fmt.Errorf("could not PEM encode serving certificate: %w", err) + } + + secret.StringData[tlsPrivateKeySecretKey] = string(tlsPrivateKeyPEM) + secret.StringData[TLSCertificateChainSecretKey] = string(tlsCertChainPEM) + } + _, err = c.k8sClient.CoreV1().Secrets(c.namespace).Create(ctx.Context, &secret, metav1.CreateOptions{}) if err != nil { return fmt.Errorf("could not create secret: %w", err) diff --git a/internal/controller/apicerts/certs_manager_test.go b/internal/controller/apicerts/certs_manager_test.go index f70f85001..b32b9228e 100644 --- a/internal/controller/apicerts/certs_manager_test.go +++ b/internal/controller/apicerts/certs_manager_test.go @@ -49,7 +49,7 @@ func TestManagerControllerOptions(t *testing.T) { observableWithInitialEventOption.WithInitialEvent, 0, "Pinniped CA", - "pinniped-api", + "ignored", ) secretsInformerFilter = observableWithInformerOption.GetFilterForInformer(secretsInformer) }) @@ -118,6 +118,7 @@ func TestManagerControllerSync(t *testing.T) { const installedInNamespace = "some-namespace" const certsSecretResourceName = "some-resource-name" const certDuration = 12345678 * time.Second + const defaultServiceName = "pinniped-api" var r *require.Assertions @@ -131,7 +132,7 @@ func TestManagerControllerSync(t *testing.T) { // Defer starting the informers until the last possible moment so that the // nested Before's can keep adding things to the informer caches. - var startInformersAndController = func() { + var startInformersAndController = func(serviceName string) { // Set this at the last second to allow for injection of server override. subject = NewCertsManagerController( installedInNamespace, @@ -146,7 +147,7 @@ func TestManagerControllerSync(t *testing.T) { controllerlib.WithInitialEvent, certDuration, "Pinniped CA", - "pinniped-api", + serviceName, ) // Set this at the last second to support calling subject.Name(). @@ -191,7 +192,7 @@ func TestManagerControllerSync(t *testing.T) { }) it("creates the serving cert Secret", func() { - startInformersAndController() + startInformersAndController(defaultServiceName) err := controllerlib.TestSync(t, subject, *syncContext) r.NoError(err) @@ -208,14 +209,17 @@ func TestManagerControllerSync(t *testing.T) { "myLabelKey2": "myLabelValue2", }, actualSecret.Labels) actualCACert := actualSecret.StringData["caCertificate"] + actualCAPrivateKey := actualSecret.StringData["caCertificatePrivateKey"] actualPrivateKey := actualSecret.StringData["tlsPrivateKey"] actualCertChain := actualSecret.StringData["tlsCertificateChain"] r.NotEmpty(actualCACert) + r.NotEmpty(actualCAPrivateKey) r.NotEmpty(actualPrivateKey) r.NotEmpty(actualCertChain) + r.Len(actualSecret.StringData, 4) - // Validate the created CA's lifetime. validCACert := testutil.ValidateCertificate(t, actualCACert, actualCACert) + validCACert.RequireMatchesPrivateKey(actualCAPrivateKey) validCACert.RequireLifetime(time.Now(), time.Now().Add(certDuration), 6*time.Minute) // Validate the created cert using the CA, and also validate the cert's hostname @@ -225,6 +229,34 @@ func TestManagerControllerSync(t *testing.T) { validCert.RequireMatchesPrivateKey(actualPrivateKey) }) + it("creates the CA but not service when the service name is empty", func() { + startInformersAndController("") + err := controllerlib.TestSync(t, subject, *syncContext) + r.NoError(err) + + // Check all the relevant fields from the create Secret action + r.Len(kubeAPIClient.Actions(), 1) + actualAction := kubeAPIClient.Actions()[0].(coretesting.CreateActionImpl) + r.Equal(schema.GroupVersionResource{Group: "", Version: "v1", Resource: "secrets"}, actualAction.GetResource()) + r.Equal(installedInNamespace, actualAction.GetNamespace()) + actualSecret := actualAction.GetObject().(*corev1.Secret) + r.Equal(certsSecretResourceName, actualSecret.Name) + r.Equal(installedInNamespace, actualSecret.Namespace) + r.Equal(map[string]string{ + "myLabelKey1": "myLabelValue1", + "myLabelKey2": "myLabelValue2", + }, actualSecret.Labels) + actualCACert := actualSecret.StringData["caCertificate"] + actualCAPrivateKey := actualSecret.StringData["caCertificatePrivateKey"] + r.NotEmpty(actualCACert) + r.NotEmpty(actualCAPrivateKey) + r.Len(actualSecret.StringData, 2) + + validCACert := testutil.ValidateCertificate(t, actualCACert, actualCACert) + validCACert.RequireMatchesPrivateKey(actualCAPrivateKey) + validCACert.RequireLifetime(time.Now(), time.Now().Add(certDuration), 6*time.Minute) + }) + when("creating the Secret fails", func() { it.Before(func() { kubeAPIClient.PrependReactor( @@ -237,7 +269,7 @@ func TestManagerControllerSync(t *testing.T) { }) it("returns the create error", func() { - startInformersAndController() + startInformersAndController(defaultServiceName) err := controllerlib.TestSync(t, subject, *syncContext) r.EqualError(err, "could not create secret: create failed") }) @@ -257,7 +289,7 @@ func TestManagerControllerSync(t *testing.T) { }) it("does not need to make any API calls with its API client", func() { - startInformersAndController() + startInformersAndController(defaultServiceName) err := controllerlib.TestSync(t, subject, *syncContext) r.NoError(err) r.Empty(kubeAPIClient.Actions()) diff --git a/internal/controller/apicerts/certs_observer.go b/internal/controller/apicerts/certs_observer.go index 7f05d243f..c6380741b 100644 --- a/internal/controller/apicerts/certs_observer.go +++ b/internal/controller/apicerts/certs_observer.go @@ -1,4 +1,4 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package apicerts @@ -62,7 +62,7 @@ func (c *certsObserverController) Sync(_ controllerlib.Context) error { } // Mutate the in-memory cert provider to update with the latest cert values. - c.dynamicCertProvider.Set(certSecret.Data[tlsCertificateChainSecretKey], certSecret.Data[tlsPrivateKeySecretKey]) + c.dynamicCertProvider.Set(certSecret.Data[TLSCertificateChainSecretKey], certSecret.Data[tlsPrivateKeySecretKey]) klog.Info("certsObserverController Sync updated certs in the dynamic cert provider") return nil } diff --git a/internal/controller/impersonatorconfig/impersonator_config.go b/internal/controller/impersonatorconfig/impersonator_config.go index c1e491aab..8d741a4f8 100644 --- a/internal/controller/impersonatorconfig/impersonator_config.go +++ b/internal/controller/impersonatorconfig/impersonator_config.go @@ -10,19 +10,18 @@ import ( "crypto/x509/pkix" "encoding/base64" "encoding/pem" - "errors" "fmt" "net" - "net/http" "strings" - "sync" "time" v1 "k8s.io/api/core/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/clock" + "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/apimachinery/pkg/util/sets" corev1informers "k8s.io/client-go/informers/core/v1" "k8s.io/client-go/kubernetes" @@ -31,14 +30,17 @@ import ( "go.pinniped.dev/internal/certauthority" "go.pinniped.dev/internal/clusterhost" "go.pinniped.dev/internal/concierge/impersonator" + "go.pinniped.dev/internal/constable" pinnipedcontroller "go.pinniped.dev/internal/controller" + "go.pinniped.dev/internal/controller/apicerts" "go.pinniped.dev/internal/controller/issuerconfig" "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/dynamiccert" "go.pinniped.dev/internal/plog" ) const ( - impersonationProxyPort = "8444" + impersonationProxyPort = 8444 defaultHTTPSPort = 443 oneHundredYears = 100 * 365 * 24 * time.Hour caCommonName = "Pinniped Impersonation Proxy CA" @@ -54,6 +56,7 @@ type impersonatorConfigController struct { generatedLoadBalancerServiceName string tlsSecretName string caSecretName string + impersonationSignerSecretName string k8sClient kubernetes.Interface pinnipedAPIClient pinnipedclientset.Interface @@ -62,19 +65,17 @@ type impersonatorConfigController struct { servicesInformer corev1informers.ServiceInformer secretsInformer corev1informers.SecretInformer - labels map[string]string - clock clock.Clock - startTLSListenerFunc StartTLSListenerFunc - httpHandlerFactory func() (http.Handler, error) + labels map[string]string + clock clock.Clock + impersonationSigningCertProvider dynamiccert.Provider + impersonatorFunc impersonator.FactoryFunc - server *http.Server - hasControlPlaneNodes *bool - tlsCert *tls.Certificate // always read/write using tlsCertMutex - tlsCertMutex sync.RWMutex + hasControlPlaneNodes *bool + serverStopCh chan struct{} + errorCh chan error + tlsServingCertDynamicCertProvider dynamiccert.Provider } -type StartTLSListenerFunc func(network, listenAddress string, config *tls.Config) (net.Listener, error) - func NewImpersonatorConfigController( namespace string, configMapResourceName string, @@ -91,28 +92,32 @@ func NewImpersonatorConfigController( caSecretName string, labels map[string]string, clock clock.Clock, - startTLSListenerFunc StartTLSListenerFunc, - httpHandlerFactory func() (http.Handler, error), + impersonatorFunc impersonator.FactoryFunc, + impersonationSignerSecretName string, + impersonationSigningCertProvider dynamiccert.Provider, ) controllerlib.Controller { + secretNames := sets.NewString(tlsSecretName, caSecretName, impersonationSignerSecretName) return controllerlib.New( controllerlib.Config{ Name: "impersonator-config-controller", Syncer: &impersonatorConfigController{ - namespace: namespace, - configMapResourceName: configMapResourceName, - credentialIssuerResourceName: credentialIssuerResourceName, - generatedLoadBalancerServiceName: generatedLoadBalancerServiceName, - tlsSecretName: tlsSecretName, - caSecretName: caSecretName, - k8sClient: k8sClient, - pinnipedAPIClient: pinnipedAPIClient, - configMapsInformer: configMapsInformer, - servicesInformer: servicesInformer, - secretsInformer: secretsInformer, - labels: labels, - clock: clock, - startTLSListenerFunc: startTLSListenerFunc, - httpHandlerFactory: httpHandlerFactory, + namespace: namespace, + configMapResourceName: configMapResourceName, + credentialIssuerResourceName: credentialIssuerResourceName, + generatedLoadBalancerServiceName: generatedLoadBalancerServiceName, + tlsSecretName: tlsSecretName, + caSecretName: caSecretName, + impersonationSignerSecretName: impersonationSignerSecretName, + k8sClient: k8sClient, + pinnipedAPIClient: pinnipedAPIClient, + configMapsInformer: configMapsInformer, + servicesInformer: servicesInformer, + secretsInformer: secretsInformer, + labels: labels, + clock: clock, + impersonationSigningCertProvider: impersonationSigningCertProvider, + impersonatorFunc: impersonatorFunc, + tlsServingCertDynamicCertProvider: dynamiccert.New(), }, }, withInformer( @@ -128,7 +133,7 @@ func NewImpersonatorConfigController( withInformer( secretsInformer, pinnipedcontroller.SimpleFilter(func(obj metav1.Object) bool { - return (obj.GetName() == tlsSecretName || obj.GetName() == caSecretName) && obj.GetNamespace() == namespace + return obj.GetNamespace() == namespace && secretNames.Has(obj.GetName()) }, nil), controllerlib.InformerOption{}, ), @@ -138,13 +143,14 @@ func NewImpersonatorConfigController( Namespace: namespace, Name: configMapResourceName, }), + // TODO fix these controller options to make this a singleton queue ) } func (c *impersonatorConfigController) Sync(syncCtx controllerlib.Context) error { plog.Debug("Starting impersonatorConfigController Sync") - strategy, err := c.doSync(syncCtx.Context) + strategy, err := c.doSync(syncCtx) if err != nil { strategy = &v1alpha1.CredentialIssuerStrategy{ @@ -154,6 +160,8 @@ func (c *impersonatorConfigController) Sync(syncCtx controllerlib.Context) error Message: err.Error(), LastUpdateTime: metav1.NewTime(c.clock.Now()), } + // The impersonator is not ready, so clear the signer CA from the dynamic provider. + c.clearSignerCA() } updateStrategyErr := c.updateStrategy(syncCtx.Context, strategy) @@ -186,7 +194,9 @@ type certNameInfo struct { clientEndpoint string } -func (c *impersonatorConfigController) doSync(ctx context.Context) (*v1alpha1.CredentialIssuerStrategy, error) { +func (c *impersonatorConfigController) doSync(syncCtx controllerlib.Context) (*v1alpha1.CredentialIssuerStrategy, error) { + ctx := syncCtx.Context + config, err := c.loadImpersonationProxyConfiguration() if err != nil { return nil, err @@ -206,12 +216,12 @@ func (c *impersonatorConfigController) doSync(ctx context.Context) (*v1alpha1.Cr } if c.shouldHaveImpersonator(config) { - if err = c.ensureImpersonatorIsStarted(); err != nil { + if err = c.ensureImpersonatorIsStarted(syncCtx); err != nil { return nil, err } } else { - if err = c.ensureImpersonatorIsStopped(); err != nil { - return nil, err + if err = c.ensureImpersonatorIsStopped(true); err != nil { + return nil, err // TODO write unit test that errors during stopping the server are returned by sync } } @@ -227,6 +237,8 @@ func (c *impersonatorConfigController) doSync(ctx context.Context) (*v1alpha1.Cr nameInfo, err := c.findDesiredTLSCertificateName(config) if err != nil { + // Unexpected error while determining the name that should go into the certs, so clear any existing certs. + c.tlsServingCertDynamicCertProvider.Set(nil, nil) return nil, err } @@ -242,7 +254,13 @@ func (c *impersonatorConfigController) doSync(ctx context.Context) (*v1alpha1.Cr return nil, err } - return c.doSyncResult(nameInfo, config, impersonationCA), nil + credentialIssuerStrategyResult := c.doSyncResult(nameInfo, config, impersonationCA) + + if err = c.loadSignerCA(credentialIssuerStrategyResult.Status); err != nil { + return nil, err + } + + return credentialIssuerStrategyResult, nil } func (c *impersonatorConfigController) loadImpersonationProxyConfiguration() (*impersonator.Config, error) { @@ -325,50 +343,73 @@ func (c *impersonatorConfigController) tlsSecretExists() (bool, *v1.Secret, erro return true, secret, nil } -func (c *impersonatorConfigController) ensureImpersonatorIsStarted() error { - if c.server != nil { - return nil - } - - handler, err := c.httpHandlerFactory() - if err != nil { - return err - } - - listener, err := c.startTLSListenerFunc("tcp", ":"+impersonationProxyPort, &tls.Config{ - MinVersion: tls.VersionTLS12, // Allow v1.2 because clients like the default `curl` on MacOS don't support 1.3 yet. - GetCertificate: func(info *tls.ClientHelloInfo) (*tls.Certificate, error) { - return c.getTLSCert(), nil - }, - }) - if err != nil { - return err - } - - c.server = &http.Server{Handler: handler} - - go func() { - plog.Info("Starting impersonation proxy", "port", impersonationProxyPort) - err = c.server.Serve(listener) - if errors.Is(err, http.ErrServerClosed) { - plog.Info("The impersonation proxy server has shut down") - } else { - plog.Error("Unexpected shutdown of the impersonation proxy server", err) +func (c *impersonatorConfigController) ensureImpersonatorIsStarted(syncCtx controllerlib.Context) error { + if c.serverStopCh != nil { + // The server was already started, but it could have died in the background, so make a non-blocking + // check to see if it has sent any errors on the errorCh. + select { + case runningErr := <-c.errorCh: + if runningErr == nil { + // The server sent a nil error, meaning that it shutdown without reporting any particular + // error for some reason. We would still like to report this as an error for logging purposes. + runningErr = constable.Error("unexpected shutdown of proxy server") + } + // The server has stopped, so finish shutting it down. + // If that fails too, return both errors for logging purposes. + // By returning an error, the sync function will be called again + // and we'll have a change to restart the server. + close(c.errorCh) // We don't want ensureImpersonatorIsStopped to block on reading this channel. + stoppingErr := c.ensureImpersonatorIsStopped(false) + return errors.NewAggregate([]error{runningErr, stoppingErr}) + default: + // Seems like it is still running, so nothing to do. + return nil } + } + + plog.Info("Starting impersonation proxy", "port", impersonationProxyPort) + startImpersonatorFunc, err := c.impersonatorFunc( + impersonationProxyPort, + c.tlsServingCertDynamicCertProvider, + dynamiccert.NewCAProvider(c.impersonationSigningCertProvider), + ) + if err != nil { + return err + } + + c.serverStopCh = make(chan struct{}) + c.errorCh = make(chan error) + + // startImpersonatorFunc will block until the server shuts down (or fails to start), so run it in the background. + go func() { + startOrStopErr := startImpersonatorFunc(c.serverStopCh) + // The server has stopped, so enqueue ourselves for another sync, so we can + // try to start the server again as quickly as possible. + syncCtx.Queue.AddRateLimited(syncCtx.Key) // TODO this a race because the main controller go routine could run and complete before we send on the err chan + // Forward any errors returned by startImpersonatorFunc on the errorCh. + c.errorCh <- startOrStopErr }() + return nil } -func (c *impersonatorConfigController) ensureImpersonatorIsStopped() error { - if c.server != nil { - plog.Info("Stopping impersonation proxy", "port", impersonationProxyPort) - err := c.server.Close() - c.server = nil - if err != nil { - return err - } +func (c *impersonatorConfigController) ensureImpersonatorIsStopped(shouldCloseErrChan bool) error { + if c.serverStopCh == nil { + return nil } - return nil + + plog.Info("Stopping impersonation proxy", "port", impersonationProxyPort) + close(c.serverStopCh) + stopErr := <-c.errorCh + + if shouldCloseErrChan { + close(c.errorCh) + } + + c.serverStopCh = nil + c.errorCh = nil + + return stopErr } func (c *impersonatorConfigController) ensureLoadBalancerIsStarted(ctx context.Context) error { @@ -385,7 +426,7 @@ func (c *impersonatorConfigController) ensureLoadBalancerIsStarted(ctx context.C Type: v1.ServiceTypeLoadBalancer, Ports: []v1.ServicePort{ { - TargetPort: intstr.Parse(impersonationProxyPort), + TargetPort: intstr.FromInt(impersonationProxyPort), Port: defaultHTTPSPort, Protocol: v1.ProtocolTCP, }, @@ -614,19 +655,19 @@ func (c *impersonatorConfigController) createCASecret(ctx context.Context) (*cer func (c *impersonatorConfigController) findDesiredTLSCertificateName(config *impersonator.Config) (*certNameInfo, error) { if config.HasEndpoint() { - return c.findTLSCertificateNameFromEndpointConfig(config) + return c.findTLSCertificateNameFromEndpointConfig(config), nil } return c.findTLSCertificateNameFromLoadBalancer() } -func (c *impersonatorConfigController) findTLSCertificateNameFromEndpointConfig(config *impersonator.Config) (*certNameInfo, error) { +func (c *impersonatorConfigController) findTLSCertificateNameFromEndpointConfig(config *impersonator.Config) *certNameInfo { endpointMaybeWithPort := config.Endpoint endpointWithoutPort := strings.Split(endpointMaybeWithPort, ":")[0] parsedAsIP := net.ParseIP(endpointWithoutPort) if parsedAsIP != nil { - return &certNameInfo{ready: true, selectedIP: parsedAsIP, clientEndpoint: endpointMaybeWithPort}, nil + return &certNameInfo{ready: true, selectedIP: parsedAsIP, clientEndpoint: endpointMaybeWithPort} } - return &certNameInfo{ready: true, selectedHostname: endpointWithoutPort, clientEndpoint: endpointMaybeWithPort}, nil + return &certNameInfo{ready: true, selectedHostname: endpointWithoutPort, clientEndpoint: endpointMaybeWithPort} } func (c *impersonatorConfigController) findTLSCertificateNameFromLoadBalancer() (*certNameInfo, error) { @@ -707,16 +748,16 @@ func (c *impersonatorConfigController) createNewTLSSecret(ctx context.Context, c func (c *impersonatorConfigController) loadTLSCertFromSecret(tlsSecret *v1.Secret) error { certPEM := tlsSecret.Data[v1.TLSCertKey] keyPEM := tlsSecret.Data[v1.TLSPrivateKeyKey] - tlsCert, err := tls.X509KeyPair(certPEM, keyPEM) + _, err := tls.X509KeyPair(certPEM, keyPEM) if err != nil { - c.setTLSCert(nil) + c.tlsServingCertDynamicCertProvider.Set(nil, nil) return fmt.Errorf("could not parse TLS cert PEM data from Secret: %w", err) } plog.Info("Loading TLS certificates for impersonation proxy", "certPEM", string(certPEM), "secret", c.tlsSecretName, "namespace", c.namespace) - c.setTLSCert(&tlsCert) + c.tlsServingCertDynamicCertProvider.Set(certPEM, keyPEM) return nil } @@ -736,11 +777,43 @@ func (c *impersonatorConfigController) ensureTLSSecretIsRemoved(ctx context.Cont return err } - c.setTLSCert(nil) + c.tlsServingCertDynamicCertProvider.Set(nil, nil) return nil } +func (c *impersonatorConfigController) loadSignerCA(status v1alpha1.StrategyStatus) error { + // Clear it when the impersonator is not completely ready. + if status != v1alpha1.SuccessStrategyStatus { + c.clearSignerCA() + return nil + } + + signingCertSecret, err := c.secretsInformer.Lister().Secrets(c.namespace).Get(c.impersonationSignerSecretName) + if err != nil { + return fmt.Errorf("could not load the impersonator's credential signing secret: %w", err) + } + + certPEM := signingCertSecret.Data[apicerts.CACertificateSecretKey] + keyPEM := signingCertSecret.Data[apicerts.CACertificatePrivateKeySecretKey] + _, err = tls.X509KeyPair(certPEM, keyPEM) + if err != nil { + return fmt.Errorf("could not load the impersonator's credential signing secret: %w", err) + } + + plog.Info("Loading credential signing certificate for impersonation proxy", + "certPEM", string(certPEM), + "fromSecret", c.impersonationSignerSecretName, + "namespace", c.namespace) + c.impersonationSigningCertProvider.Set(certPEM, keyPEM) + return nil +} + +func (c *impersonatorConfigController) clearSignerCA() { + plog.Info("Clearing credential signing certificate for impersonation proxy") + c.impersonationSigningCertProvider.Set(nil, nil) +} + func (c *impersonatorConfigController) doSyncResult(nameInfo *certNameInfo, config *impersonator.Config, ca *certauthority.CA) *v1alpha1.CredentialIssuerStrategy { switch { case c.disabledExplicitly(config): @@ -784,15 +857,3 @@ func (c *impersonatorConfigController) doSyncResult(nameInfo *certNameInfo, conf } } } - -func (c *impersonatorConfigController) setTLSCert(cert *tls.Certificate) { - c.tlsCertMutex.Lock() - defer c.tlsCertMutex.Unlock() - c.tlsCert = cert -} - -func (c *impersonatorConfigController) getTLSCert() *tls.Certificate { - c.tlsCertMutex.RLock() - defer c.tlsCertMutex.RUnlock() - return c.tlsCert -} diff --git a/internal/controller/impersonatorconfig/impersonator_config_test.go b/internal/controller/impersonatorconfig/impersonator_config_test.go index 65ea68ff8..66cab3e7e 100644 --- a/internal/controller/impersonatorconfig/impersonator_config_test.go +++ b/internal/controller/impersonatorconfig/impersonator_config_test.go @@ -17,7 +17,7 @@ import ( "net/http" "reflect" "regexp" - "strings" + "sync" "testing" "time" @@ -30,6 +30,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/clock" + "k8s.io/apiserver/pkg/server/dynamiccertificates" kubeinformers "k8s.io/client-go/informers" kubernetesfake "k8s.io/client-go/kubernetes/fake" coretesting "k8s.io/client-go/testing" @@ -37,33 +38,13 @@ import ( "go.pinniped.dev/generated/latest/apis/concierge/config/v1alpha1" pinnipedfake "go.pinniped.dev/generated/latest/client/concierge/clientset/versioned/fake" "go.pinniped.dev/internal/certauthority" + "go.pinniped.dev/internal/controller/apicerts" "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/dynamiccert" "go.pinniped.dev/internal/kubeclient" "go.pinniped.dev/internal/testutil" ) -type tlsListenerWrapper struct { - listener net.Listener - closeError error -} - -func (t *tlsListenerWrapper) Accept() (net.Conn, error) { - return t.listener.Accept() -} - -func (t *tlsListenerWrapper) Close() error { - if t.closeError != nil { - // Really close the connection and then "pretend" that there was an error during close. - _ = t.listener.Close() - return t.closeError - } - return t.listener.Close() -} - -func (t *tlsListenerWrapper) Addr() net.Addr { - return t.listener.Addr() -} - func TestImpersonatorConfigControllerOptions(t *testing.T) { spec.Run(t, "options", func(t *testing.T, when spec.G, it spec.S) { const installedInNamespace = "some-namespace" @@ -71,6 +52,7 @@ func TestImpersonatorConfigControllerOptions(t *testing.T) { const generatedLoadBalancerServiceName = "some-service-resource-name" const tlsSecretName = "some-tls-secret-name" //nolint:gosec // this is not a credential const caSecretName = "some-ca-secret-name" + const caSignerName = "some-ca-signer-name" var r *require.Assertions var observableWithInformerOption *testutil.ObservableWithInformerOption @@ -105,6 +87,7 @@ func TestImpersonatorConfigControllerOptions(t *testing.T) { nil, nil, nil, + caSignerName, nil, ) configMapsInformerFilter = observableWithInformerOption.GetFilterForInformer(configMapsInformer) @@ -210,12 +193,13 @@ func TestImpersonatorConfigControllerOptions(t *testing.T) { when("watching Secret objects", func() { var subject controllerlib.Filter - var target1, target2, wrongNamespace1, wrongNamespace2, wrongName, unrelated *corev1.Secret + var target1, target2, target3, wrongNamespace1, wrongNamespace2, wrongName, unrelated *corev1.Secret it.Before(func() { subject = secretsInformerFilter target1 = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: tlsSecretName, Namespace: installedInNamespace}} target2 = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: caSecretName, Namespace: installedInNamespace}} + target3 = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: caSignerName, Namespace: installedInNamespace}} wrongNamespace1 = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: tlsSecretName, Namespace: "wrong-namespace"}} wrongNamespace2 = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: caSecretName, Namespace: "wrong-namespace"}} wrongName = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "wrong-name", Namespace: installedInNamespace}} @@ -232,6 +216,10 @@ func TestImpersonatorConfigControllerOptions(t *testing.T) { r.True(subject.Update(target2, unrelated)) r.True(subject.Update(unrelated, target2)) r.True(subject.Delete(target2)) + r.True(subject.Add(target3)) + r.True(subject.Update(target3, unrelated)) + r.True(subject.Update(unrelated, target3)) + r.True(subject.Delete(target3)) }) }) @@ -285,6 +273,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { const loadBalancerServiceName = "some-service-resource-name" const tlsSecretName = "some-tls-secret-name" //nolint:gosec // this is not a credential const caSecretName = "some-ca-secret-name" + const caSignerName = "some-ca-signer-name" const localhostIP = "127.0.0.1" const httpsPort = ":443" const fakeServerResponseBody = "hello, world!" @@ -300,44 +289,139 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { var cancelContext context.Context var cancelContextCancelFunc context.CancelFunc var syncContext *controllerlib.Context - var startTLSListenerFuncWasCalled int - var startTLSListenerFuncError error - var startTLSListenerUponCloseError error - var httpHandlerFactoryFuncError error + var impersonatorFuncWasCalled int + var impersonatorFuncError error + var impersonatorFuncReturnedFuncError error var startedTLSListener net.Listener var frozenNow time.Time + var signingCertProvider dynamiccert.Provider + var signingCACertPEM, signingCAKeyPEM []byte + var signingCASecret *corev1.Secret + var testHTTPServer *http.Server + var testHTTPServerMutex sync.RWMutex + var testHTTPServerInterruptCh chan struct{} + var queue *testQueue + var validClientCert *tls.Certificate - var startTLSListenerFunc = func(network, listenAddress string, config *tls.Config) (net.Listener, error) { - startTLSListenerFuncWasCalled++ - r.Equal("tcp", network) - r.Equal(":8444", listenAddress) - r.Equal(uint16(tls.VersionTLS12), config.MinVersion) - if startTLSListenerFuncError != nil { - return nil, startTLSListenerFuncError + var impersonatorFunc = func( + port int, + dynamicCertProvider dynamiccertificates.CertKeyContentProvider, + impersonationProxySignerCAProvider dynamiccertificates.CAContentProvider, + ) (func(stopCh <-chan struct{}) error, error) { + impersonatorFuncWasCalled++ + r.Equal(8444, port) + r.NotNil(dynamicCertProvider) + r.NotNil(impersonationProxySignerCAProvider) + + if impersonatorFuncError != nil { + return nil, impersonatorFuncError } - var err error - startedTLSListener, err = tls.Listen(network, localhostIP+":0", config) // automatically choose the port for unit tests - r.NoError(err) - return &tlsListenerWrapper{listener: startedTLSListener, closeError: startTLSListenerUponCloseError}, nil + + // Return a func that starts a fake server when called, and shuts down the fake server when stopCh is closed. + // This fake server is enough like the real impersonation proxy server for this unit test because it + // uses the supplied providers to serve TLS. The goal of this unit test is to make sure that the server + // was started/stopped/configured correctly, not to test the actual impersonation behavior. + return func(stopCh <-chan struct{}) error { + if impersonatorFuncReturnedFuncError != nil { + return impersonatorFuncReturnedFuncError + } + + var err error + // automatically choose the port for unit tests + startedTLSListener, err = tls.Listen("tcp", localhostIP+":0", &tls.Config{ + MinVersion: tls.VersionTLS12, + GetCertificate: func(info *tls.ClientHelloInfo) (*tls.Certificate, error) { + certPEM, keyPEM := dynamicCertProvider.CurrentCertKeyContent() + if certPEM != nil && keyPEM != nil { + tlsCert, err := tls.X509KeyPair(certPEM, keyPEM) + r.NoError(err) + return &tlsCert, nil + } + return nil, nil // no cached TLS certs + }, + ClientAuth: tls.RequestClientCert, + VerifyPeerCertificate: func(rawCerts [][]byte, _ [][]*x509.Certificate) error { + // Docs say that this will always be called in tls.RequestClientCert mode + // and that the second parameter will always be nil in that case. + // rawCerts will be raw ASN.1 certificates provided by the peer. + if rawCerts == nil || len(rawCerts) != 1 { + return fmt.Errorf("expected to get one client cert on incoming request to test server") + } + clientCert := rawCerts[0] + currentClientCertCA := impersonationProxySignerCAProvider.CurrentCABundleContent() + if currentClientCertCA == nil { + return fmt.Errorf("impersonationProxySignerCAProvider does not have a current CA certificate") + } + // Assert that the client's cert was signed by the CA cert that the controller put into + // the CAContentProvider that was passed in. + parsed, err := x509.ParseCertificate(clientCert) + require.NoError(t, err) + t.Log("PARSED CLIENT CERT") + roots := x509.NewCertPool() + require.True(t, roots.AppendCertsFromPEM(currentClientCertCA)) + opts := x509.VerifyOptions{Roots: roots} + _, err = parsed.Verify(opts) + require.NoError(t, err) + return nil + }, + }) + r.NoError(err) + + testHTTPServerMutex.Lock() // this is to satisfy the race detector + testHTTPServer = &http.Server{Handler: http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + _, err := fmt.Fprint(w, fakeServerResponseBody) + r.NoError(err) + })} + testHTTPServerMutex.Unlock() + + // Start serving requests in the background. + go func() { + err := testHTTPServer.Serve(startedTLSListener) + if !errors.Is(err, http.ErrServerClosed) { + t.Log("Got an unexpected error while starting the fake http server!") + r.NoError(err) // causes the test to crash, which is good enough because this should never happen + } + }() + + if testHTTPServerInterruptCh == nil { + // Wait in the foreground for the stopCh to be closed, and kill the server when that happens. + // This is similar to the behavior of the real impersonation server. + <-stopCh + } else { + // The test supplied an interrupt channel because it wants to test unexpected termination + // of the server, so wait for that channel to close instead of waiting for the one that + // was passed in from the production code. + <-testHTTPServerInterruptCh + } + + err = testHTTPServer.Close() + t.Log("Got an unexpected error while stopping the fake http server!") + r.NoError(err) // causes the test to crash, which is good enough because this should never happen + + return nil + }, nil } var testServerAddr = func() string { + require.Eventually(t, func() bool { + return startedTLSListener != nil + }, 20*time.Second, 50*time.Millisecond, "TLS listener never became not nil") + return startedTLSListener.Addr().String() } - var closeTLSListener = func() { - if startedTLSListener != nil { - err := startedTLSListener.Close() - // Ignore when the production code has already closed the server because there is nothing to - // clean up in that case. - if err != nil && !strings.Contains(err.Error(), "use of closed network connection") { - r.NoError(err) - } + var closeTestHTTPServer = func() { + // If a test left it running, then close it. + testHTTPServerMutex.RLock() // this is to satisfy the race detector + defer testHTTPServerMutex.RUnlock() + if testHTTPServer != nil { + err := testHTTPServer.Close() + r.NoError(err) } } var requireTLSServerIsRunning = func(caCrt []byte, addr string, dnsOverrides map[string]string) { - r.Greater(startTLSListenerFuncWasCalled, 0) + r.Greater(impersonatorFuncWasCalled, 0) realDialer := &net.Dialer{} overrideDialContext := func(ctx context.Context, network, addr string) (net.Conn, error) { @@ -361,8 +445,13 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { rootCAs := x509.NewCertPool() rootCAs.AppendCertsFromPEM(caCrt) tr = &http.Transport{ - TLSClientConfig: &tls.Config{RootCAs: rootCAs}, - DialContext: overrideDialContext, + TLSClientConfig: &tls.Config{ + // Server's TLS serving cert CA + RootCAs: rootCAs, + // Client cert which is supposed to work against the server's dynamic CAContentProvider + Certificates: []tls.Certificate{*validClientCert}, + }, + DialContext: overrideDialContext, } } client := &http.Client{Transport: tr} @@ -385,7 +474,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { } var requireTLSServerIsRunningWithoutCerts = func() { - r.Greater(startTLSListenerFuncWasCalled, 0) + r.Greater(impersonatorFuncWasCalled, 0) tr := &http.Transport{ TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, //nolint:gosec } @@ -406,14 +495,14 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { } var requireTLSServerIsNoLongerRunning = func() { - r.Greater(startTLSListenerFuncWasCalled, 0) + r.Greater(impersonatorFuncWasCalled, 0) var err error expectedErrorRegex := "dial tcp .*: connect: connection refused" expectedErrorRegexCompiled, err := regexp.Compile(expectedErrorRegex) r.NoError(err) assert.Eventually(t, func() bool { _, err = tls.Dial( - startedTLSListener.Addr().Network(), + "tcp", testServerAddr(), &tls.Config{InsecureSkipVerify: true}, //nolint:gosec ) @@ -424,7 +513,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { } var requireTLSServerWasNeverStarted = func() { - r.Equal(0, startTLSListenerFuncWasCalled) + r.Equal(0, impersonatorFuncWasCalled) } // Defer starting the informers until the last possible moment so that the @@ -447,13 +536,9 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { caSecretName, labels, clock.NewFakeClock(frozenNow), - startTLSListenerFunc, - func() (http.Handler, error) { - return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - _, err := fmt.Fprint(w, fakeServerResponseBody) - r.NoError(err) - }), httpHandlerFactoryFuncError - }, + impersonatorFunc, + caSignerName, + signingCertProvider, ) // Set this at the last second to support calling subject.Name(). @@ -464,6 +549,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { Namespace: installedInNamespace, Name: configMapResourceName, }, + Queue: queue, } // Must start informers before calling TestRunSynchronously() @@ -536,6 +622,13 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { return newSecretWithData(resourceName, newTLSCertSecretData(ca, []string{"foo", "bar"}, ip)) } + var newSigningKeySecret = func(resourceName string, certPEM, keyPEM []byte) *corev1.Secret { + return newSecretWithData(resourceName, map[string][]byte{ + apicerts.CACertificateSecretKey: certPEM, + apicerts.CACertificatePrivateKeySecretKey: keyPEM, + }) + } + var newLoadBalancerService = func(resourceName string, status corev1.ServiceStatus) *corev1.Service { return &corev1.Service{ ObjectMeta: metav1.ObjectMeta{ @@ -842,12 +935,26 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { validCert.RequireLifetime(time.Now().Add(-10*time.Second), time.Now().Add(100*time.Hour*24*365), 10*time.Second) } + var requireSigningCertProviderHasLoadedCerts = func(certPEM, keyPEM []byte) { + actualCert, actualKey := signingCertProvider.CurrentCertKeyContent() + // Cast to string for better failure messages. + r.Equal(string(certPEM), string(actualCert)) + r.Equal(string(keyPEM), string(actualKey)) + } + + var requireSigningCertProviderIsEmpty = func() { + actualCert, actualKey := signingCertProvider.CurrentCertKeyContent() + r.Nil(actualCert) + r.Nil(actualKey) + } + var runControllerSync = func() error { return controllerlib.TestSync(t, subject, *syncContext) } it.Before(func() { r = require.New(t) + queue = &testQueue{} cancelContext, cancelContextCancelFunc = context.WithCancel(context.Background()) kubeInformerClient = kubernetesfake.NewSimpleClientset() kubeInformers = kubeinformers.NewSharedInformerFactoryWithOptions(kubeInformerClient, 0, @@ -856,15 +963,26 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { kubeAPIClient = kubernetesfake.NewSimpleClientset() pinnipedAPIClient = pinnipedfake.NewSimpleClientset() frozenNow = time.Date(2021, time.March, 2, 7, 42, 0, 0, time.Local) + signingCertProvider = dynamiccert.New() + + ca := newCA() + signingCACertPEM = ca.Bundle() + var err error + signingCAKeyPEM, err = ca.PrivateKeyToPEM() + r.NoError(err) + signingCASecret = newSigningKeySecret(caSignerName, signingCACertPEM, signingCAKeyPEM) + validClientCert, err = ca.Issue(pkix.Name{}, nil, nil, time.Hour) + r.NoError(err) }) it.After(func() { cancelContextCancelFunc() - closeTLSListener() + closeTestHTTPServer() }) when("the ConfigMap does not yet exist in the installation namespace or it was deleted (defaults to auto mode)", func() { it.Before(func() { + addSecretToTrackers(signingCASecret, kubeInformerClient) addImpersonatorConfigMapToTracker("some-other-unrelated-configmap", "foo: bar", kubeInformerClient) }) @@ -880,6 +998,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { r.Len(kubeAPIClient.Actions(), 1) requireNodesListed(kubeAPIClient.Actions()[0]) requireCredentialIssuer(newAutoDisabledStrategy()) + requireSigningCertProviderIsEmpty() }) }) @@ -900,6 +1019,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireLoadBalancerWasDeleted(kubeAPIClient.Actions()[1]) requireTLSSecretWasDeleted(kubeAPIClient.Actions()[2]) requireCredentialIssuer(newAutoDisabledStrategy()) + requireSigningCertProviderIsEmpty() }) }) @@ -917,6 +1037,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireLoadBalancerWasCreated(kubeAPIClient.Actions()[1]) requireCASecretWasCreated(kubeAPIClient.Actions()[2]) requireCredentialIssuer(newPendingStrategy()) + requireSigningCertProviderIsEmpty() }) }) @@ -935,6 +1056,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireNodesListed(kubeAPIClient.Actions()[0]) requireCASecretWasCreated(kubeAPIClient.Actions()[1]) requireCredentialIssuer(newPendingStrategy()) + requireSigningCertProviderIsEmpty() }) }) @@ -953,6 +1075,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireNodesListed(kubeAPIClient.Actions()[0]) requireCASecretWasCreated(kubeAPIClient.Actions()[1]) requireCredentialIssuer(newPendingStrategy()) + requireSigningCertProviderIsEmpty() }) }) @@ -970,6 +1093,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { r.Len(kubeAPIClient.Actions(), 1) requireNodesListed(kubeAPIClient.Actions()[0]) requireCredentialIssuer(newErrorStrategy("could not find valid IP addresses or hostnames from load balancer some-namespace/some-service-resource-name")) + requireSigningCertProviderIsEmpty() }) }) @@ -990,6 +1114,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireTLSSecretWasCreated(kubeAPIClient.Actions()[2], ca) requireTLSServerIsRunning(ca, fakeIP, map[string]string{fakeIP + ":443": testServerAddr()}) requireCredentialIssuer(newSuccessStrategy(fakeIP, ca)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) // Simulate the informer cache's background update from its watch. addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[1], kubeInformers.Core().V1().Secrets()) @@ -999,6 +1124,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { r.NoError(runControllerSync()) r.Len(kubeAPIClient.Actions(), 3) // nothing changed requireCredentialIssuer(newSuccessStrategy(fakeIP, ca)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) }) }) @@ -1019,6 +1145,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireTLSSecretWasCreated(kubeAPIClient.Actions()[2], ca) requireTLSServerIsRunning(ca, firstHostname, map[string]string{firstHostname + httpsPort: testServerAddr()}) requireCredentialIssuer(newSuccessStrategy(firstHostname, ca)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) // Simulate the informer cache's background update from its watch. addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[1], kubeInformers.Core().V1().Secrets()) @@ -1028,6 +1155,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { r.NoError(runControllerSync()) r.Len(kubeAPIClient.Actions(), 3) // nothing changed requireCredentialIssuer(newSuccessStrategy(firstHostname, ca)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) }) }) @@ -1048,6 +1176,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireTLSSecretWasCreated(kubeAPIClient.Actions()[2], ca) requireTLSServerIsRunning(ca, firstHostname, map[string]string{firstHostname + httpsPort: testServerAddr()}) requireCredentialIssuer(newSuccessStrategy(firstHostname, ca)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) // Simulate the informer cache's background update from its watch. addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[1], kubeInformers.Core().V1().Secrets()) @@ -1057,6 +1186,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { r.NoError(runControllerSync()) r.Len(kubeAPIClient.Actions(), 3) // nothing changed requireCredentialIssuer(newSuccessStrategy(firstHostname, ca)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) }) }) @@ -1082,6 +1212,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireTLSSecretWasCreated(kubeAPIClient.Actions()[2], caCrt) requireTLSServerIsRunning(caCrt, testServerAddr(), nil) requireCredentialIssuer(newSuccessStrategy(localhostIP, caCrt)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) }) }) @@ -1106,6 +1237,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireTLSSecretWasDeleted(kubeAPIClient.Actions()[1]) requireTLSServerIsRunningWithoutCerts() requireCredentialIssuer(newErrorStrategy("error on delete")) + requireSigningCertProviderIsEmpty() }) }) @@ -1123,7 +1255,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { addSecretToTrackers(tlsSecret, kubeAPIClient, kubeInformerClient) }) - it("returns an error and keeps running the proxy with the old cert", func() { + it("returns an error and keeps the proxy running but now without certs", func() { startInformersAndController() r.NoError(runControllerSync()) r.Len(kubeAPIClient.Actions(), 1) @@ -1135,13 +1267,18 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { errString := "could not find valid IP addresses or hostnames from load balancer some-namespace/some-service-resource-name" r.EqualError(runControllerSync(), errString) r.Len(kubeAPIClient.Actions(), 1) // no new actions - requireTLSServerIsRunning(caCrt, testServerAddr(), nil) + requireTLSServerIsRunningWithoutCerts() requireCredentialIssuer(newErrorStrategy(errString)) + requireSigningCertProviderIsEmpty() }) }) }) when("the ConfigMap is already in the installation namespace", func() { + it.Before(func() { + addSecretToTrackers(signingCASecret, kubeInformerClient) + }) + when("the configuration is auto mode with an endpoint", func() { it.Before(func() { configMapYAML := fmt.Sprintf("{mode: auto, endpoint: %s}", localhostIP) @@ -1160,6 +1297,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireNodesListed(kubeAPIClient.Actions()[0]) r.Len(kubeAPIClient.Actions(), 1) requireCredentialIssuer(newAutoDisabledStrategy()) + requireSigningCertProviderIsEmpty() }) }) @@ -1177,6 +1315,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireTLSSecretWasCreated(kubeAPIClient.Actions()[2], ca) requireTLSServerIsRunning(ca, testServerAddr(), nil) requireCredentialIssuer(newSuccessStrategy(localhostIP, ca)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) }) }) }) @@ -1194,6 +1333,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireNodesListed(kubeAPIClient.Actions()[0]) r.Len(kubeAPIClient.Actions(), 1) requireCredentialIssuer(newManuallyDisabledStrategy()) + requireSigningCertProviderIsEmpty() }) }) @@ -1213,13 +1353,15 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireCASecretWasCreated(kubeAPIClient.Actions()[2]) requireTLSServerIsRunningWithoutCerts() requireCredentialIssuer(newPendingStrategy()) + requireSigningCertProviderIsEmpty() }) - it("returns an error when the tls listener fails to start", func() { - startTLSListenerFuncError = errors.New("tls error") + it("returns an error when the impersonation TLS server fails to start", func() { + impersonatorFuncError = errors.New("impersonation server start error") startInformersAndController() - r.EqualError(runControllerSync(), "tls error") - requireCredentialIssuer(newErrorStrategy("tls error")) + r.EqualError(runControllerSync(), "impersonation server start error") + requireCredentialIssuer(newErrorStrategy("impersonation server start error")) + requireSigningCertProviderIsEmpty() }) }) @@ -1239,13 +1381,15 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireCASecretWasCreated(kubeAPIClient.Actions()[1]) requireTLSServerIsRunningWithoutCerts() requireCredentialIssuer(newPendingStrategy()) + requireSigningCertProviderIsEmpty() }) - it("returns an error when the tls listener fails to start", func() { - startTLSListenerFuncError = errors.New("tls error") + it("returns an error when the impersonation TLS server fails to start", func() { + impersonatorFuncError = errors.New("impersonation server start error") startInformersAndController() - r.EqualError(runControllerSync(), "tls error") - requireCredentialIssuer(newErrorStrategy("tls error")) + r.EqualError(runControllerSync(), "impersonation server start error") + requireCredentialIssuer(newErrorStrategy("impersonation server start error")) + requireSigningCertProviderIsEmpty() }) }) @@ -1271,6 +1415,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireNodesListed(kubeAPIClient.Actions()[0]) requireTLSServerIsRunning(caCrt, testServerAddr(), nil) requireCredentialIssuer(newSuccessStrategy(localhostIP, caCrt)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) }) }) @@ -1292,6 +1437,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { // Check that the server is running and that TLS certs that are being served are are for fakeHostname. requireTLSServerIsRunning(ca, fakeHostname, map[string]string{fakeHostname + httpsPort: testServerAddr()}) requireCredentialIssuer(newSuccessStrategy(fakeHostname, ca)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) }) }) @@ -1313,6 +1459,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { // Check that the server is running and that TLS certs that are being served are are for fakeIPWithPort. requireTLSServerIsRunning(ca, fakeIPWithPort, map[string]string{fakeIPWithPort: testServerAddr()}) requireCredentialIssuer(newSuccessStrategy(fakeIPWithPort, ca)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) }) }) @@ -1334,6 +1481,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { // Check that the server is running and that TLS certs that are being served are are for fakeHostnameWithPort. requireTLSServerIsRunning(ca, fakeHostnameWithPort, map[string]string{fakeHostnameWithPort: testServerAddr()}) requireCredentialIssuer(newSuccessStrategy(fakeHostnameWithPort, ca)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) }) }) @@ -1357,6 +1505,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { // Check that the server is running and that TLS certs that are being served are are for fakeIP. requireTLSServerIsRunning(ca, fakeIP, map[string]string{fakeIP + httpsPort: testServerAddr()}) requireCredentialIssuer(newSuccessStrategy(fakeIP, ca)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) // Simulate the informer cache's background update from its watch. addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[1], kubeInformers.Core().V1().Secrets()) @@ -1372,9 +1521,11 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { // Check that the server is running and that TLS certs that are being served are are for fakeHostname. requireTLSServerIsRunning(ca, fakeHostname, map[string]string{fakeHostname + httpsPort: testServerAddr()}) requireCredentialIssuer(newSuccessStrategy(fakeHostname, ca)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) // Simulate the informer cache's background update from its watch. deleteSecretFromTracker(tlsSecretName, kubeInformerClient) + waitForObjectToBeDeletedFromInformer(tlsSecretName, kubeInformers.Core().V1().Secrets()) addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[4], kubeInformers.Core().V1().Secrets()) // Switch the endpoint config back to an IP. @@ -1387,6 +1538,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { // Check that the server is running and that TLS certs that are being served are are for fakeIP. requireTLSServerIsRunning(ca, fakeIP, map[string]string{fakeIP + httpsPort: testServerAddr()}) requireCredentialIssuer(newSuccessStrategy(fakeIP, ca)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) }) }) @@ -1408,8 +1560,9 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { // Check that the server is running and that TLS certs that are being served are are for fakeHostname. requireTLSServerIsRunning(ca, fakeHostname, map[string]string{fakeHostname + httpsPort: testServerAddr()}) requireCredentialIssuer(newSuccessStrategy(fakeHostname, ca)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) - // Simulate the informer cache's background update from its watch for the CA Secret. + // Simulate the informer cache's background update from its watch. addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[1], kubeInformers.Core().V1().Secrets()) // Delete the TLS Secret that was just created from the Kube API server. Note that we never @@ -1423,6 +1576,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { // Check that the server is running and that TLS certs that are being served are are for fakeHostname. requireTLSServerIsRunning(ca, fakeHostname, map[string]string{fakeHostname + httpsPort: testServerAddr()}) requireCredentialIssuer(newSuccessStrategy(fakeHostname, ca)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) }) }) @@ -1444,8 +1598,9 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { // Check that the server is running and that TLS certs that are being served are are for fakeHostname. requireTLSServerIsRunning(ca, fakeHostname, map[string]string{fakeHostname + httpsPort: testServerAddr()}) requireCredentialIssuer(newSuccessStrategy(fakeHostname, ca)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) - // Simulate the informer cache's background update from its watch for the CA Secret. + // Simulate the informer cache's background update from its watch. addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[2], kubeInformers.Core().V1().Secrets()) // Delete the CA Secret that was just created from the Kube API server. Note that we never @@ -1461,6 +1616,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { // Check that the server is running and that TLS certs that are being served are are for fakeHostname. requireTLSServerIsRunning(ca, fakeHostname, map[string]string{fakeHostname + httpsPort: testServerAddr()}) requireCredentialIssuer(newSuccessStrategy(fakeHostname, ca)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) }) }) @@ -1480,8 +1636,9 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { // Check that the server is running and that TLS certs that are being served are are for fakeHostname. requireTLSServerIsRunning(ca, fakeHostname, map[string]string{fakeHostname + httpsPort: testServerAddr()}) requireCredentialIssuer(newSuccessStrategy(fakeHostname, ca)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) - // Simulate the informer cache's background update from its watch for the CA Secret. + // Simulate the informer cache's background update from its watch. addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[2], kubeInformers.Core().V1().Secrets()) // Simulate someone updating the CA Secret out of band, e.g. when a human edits it with kubectl. @@ -1505,6 +1662,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { // Check that the server is running and that TLS certs that are being served are are for fakeHostname. requireTLSServerIsRunning(caCrt, fakeHostname, map[string]string{fakeHostname + httpsPort: testServerAddr()}) requireCredentialIssuer(newSuccessStrategy(fakeHostname, caCrt)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) }) when("deleting the TLS cert due to mismatched CA results in an error", func() { @@ -1522,6 +1680,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { r.Len(kubeAPIClient.Actions(), 4) requireTLSSecretWasDeleted(kubeAPIClient.Actions()[3]) // tried to delete cert but failed requireCredentialIssuer(newErrorStrategy("error on tls secret delete")) + requireSigningCertProviderIsEmpty() }) }) }) @@ -1543,6 +1702,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireLoadBalancerWasCreated(kubeAPIClient.Actions()[1]) requireCASecretWasCreated(kubeAPIClient.Actions()[2]) requireCredentialIssuer(newPendingStrategy()) + requireSigningCertProviderIsEmpty() // Simulate the informer cache's background update from its watch. addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[1], kubeInformers.Core().V1().Services()) @@ -1556,6 +1716,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { r.Len(kubeAPIClient.Actions(), 4) requireLoadBalancerWasDeleted(kubeAPIClient.Actions()[3]) requireCredentialIssuer(newManuallyDisabledStrategy()) + requireSigningCertProviderIsEmpty() deleteServiceFromTracker(loadBalancerServiceName, kubeInformerClient) waitForObjectToBeDeletedFromInformer(loadBalancerServiceName, kubeInformers.Core().V1().Services()) @@ -1568,25 +1729,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { r.Len(kubeAPIClient.Actions(), 5) requireLoadBalancerWasCreated(kubeAPIClient.Actions()[4]) requireCredentialIssuer(newPendingStrategy()) - }) - - when("there is an error while shutting down the server", func() { - it.Before(func() { - startTLSListenerUponCloseError = errors.New("fake server close error") - }) - - it("returns the error from the sync function", func() { - startInformersAndController() - r.NoError(runControllerSync()) - requireTLSServerIsRunningWithoutCerts() - - // Update the configmap. - updateImpersonatorConfigMapInInformerAndWait(configMapResourceName, "mode: disabled", kubeInformers.Core().V1().ConfigMaps()) - - r.EqualError(runControllerSync(), "fake server close error") - requireTLSServerIsNoLongerRunning() - requireCredentialIssuer(newErrorStrategy("fake server close error")) - }) + requireSigningCertProviderIsEmpty() }) }) @@ -1608,6 +1751,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireTLSSecretWasCreated(kubeAPIClient.Actions()[2], ca) requireTLSServerIsRunning(ca, testServerAddr(), nil) requireCredentialIssuer(newSuccessStrategy(localhostIP, ca)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) // Simulate the informer cache's background update from its watch. addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[1], kubeInformers.Core().V1().Secrets()) @@ -1622,6 +1766,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireTLSSecretWasDeleted(kubeAPIClient.Actions()[4]) // the Secret was deleted because it contained a cert with the wrong IP requireTLSServerIsRunningWithoutCerts() requireCredentialIssuer(newPendingStrategy()) + requireSigningCertProviderIsEmpty() // Simulate the informer cache's background update from its watch. addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[3], kubeInformers.Core().V1().Services()) @@ -1633,6 +1778,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { r.Len(kubeAPIClient.Actions(), 5) // no new actions while it is waiting for the load balancer's ingress requireTLSServerIsRunningWithoutCerts() requireCredentialIssuer(newPendingStrategy()) + requireSigningCertProviderIsEmpty() // Update the ingress of the LB in the informer's client and run Sync again. fakeIP := "127.0.0.123" @@ -1643,6 +1789,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { // Check that the server is running and that TLS certs that are being served are are for fakeIP. requireTLSServerIsRunning(ca, fakeIP, map[string]string{fakeIP + httpsPort: testServerAddr()}) requireCredentialIssuer(newSuccessStrategy(fakeIP, ca)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) // Simulate the informer cache's background update from its watch. addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[5], kubeInformers.Core().V1().Secrets()) @@ -1658,12 +1805,14 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireTLSSecretWasCreated(kubeAPIClient.Actions()[8], ca) // recreated because the endpoint was updated, reused the old CA requireTLSServerIsRunning(ca, testServerAddr(), nil) requireCredentialIssuer(newSuccessStrategy(localhostIP, ca)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) }) }) }) when("sync is called more than once", func() { it.Before(func() { + addSecretToTrackers(signingCASecret, kubeInformerClient) addNodeWithRoleToTracker("worker", kubeAPIClient) }) @@ -1676,15 +1825,17 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireCASecretWasCreated(kubeAPIClient.Actions()[2]) requireTLSServerIsRunningWithoutCerts() requireCredentialIssuer(newPendingStrategy()) + requireSigningCertProviderIsEmpty() // Simulate the informer cache's background update from its watch. addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[1], kubeInformers.Core().V1().Services()) addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[2], kubeInformers.Core().V1().Secrets()) r.NoError(runControllerSync()) - r.Equal(1, startTLSListenerFuncWasCalled) // wasn't started a second time - requireTLSServerIsRunningWithoutCerts() // still running + r.Equal(1, impersonatorFuncWasCalled) // wasn't started a second time + requireTLSServerIsRunningWithoutCerts() // still running requireCredentialIssuer(newPendingStrategy()) + requireSigningCertProviderIsEmpty() r.Len(kubeAPIClient.Actions(), 3) // no new API calls }) @@ -1697,6 +1848,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { ca := requireCASecretWasCreated(kubeAPIClient.Actions()[2]) requireTLSServerIsRunningWithoutCerts() requireCredentialIssuer(newPendingStrategy()) + requireSigningCertProviderIsEmpty() // Simulate the informer cache's background update from its watch. addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[1], kubeInformers.Core().V1().Services()) @@ -1705,20 +1857,22 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { updateLoadBalancerServiceInInformerAndWait(loadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: localhostIP}}, kubeInformers.Core().V1().Services()) r.NoError(runControllerSync()) - r.Equal(1, startTLSListenerFuncWasCalled) // wasn't started a second time + r.Equal(1, impersonatorFuncWasCalled) // wasn't started a second time r.Len(kubeAPIClient.Actions(), 4) requireTLSSecretWasCreated(kubeAPIClient.Actions()[3], ca) // uses the ca from last time requireTLSServerIsRunning(ca, testServerAddr(), nil) // running with certs now requireCredentialIssuer(newSuccessStrategy(localhostIP, ca)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) // Simulate the informer cache's background update from its watch. addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[3], kubeInformers.Core().V1().Secrets()) r.NoError(runControllerSync()) - r.Equal(1, startTLSListenerFuncWasCalled) // wasn't started again + r.Equal(1, impersonatorFuncWasCalled) // wasn't started again r.Len(kubeAPIClient.Actions(), 4) // no more actions requireTLSServerIsRunning(ca, testServerAddr(), nil) // still running requireCredentialIssuer(newSuccessStrategy(localhostIP, ca)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) }) it("creates certs from the hostname listed on the load balancer", func() { @@ -1731,6 +1885,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { ca := requireCASecretWasCreated(kubeAPIClient.Actions()[2]) requireTLSServerIsRunningWithoutCerts() requireCredentialIssuer(newPendingStrategy()) + requireSigningCertProviderIsEmpty() // Simulate the informer cache's background update from its watch. addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[1], kubeInformers.Core().V1().Services()) @@ -1739,20 +1894,56 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { updateLoadBalancerServiceInInformerAndWait(loadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: localhostIP, Hostname: hostname}}, kubeInformers.Core().V1().Services()) r.NoError(runControllerSync()) - r.Equal(1, startTLSListenerFuncWasCalled) // wasn't started a second time + r.Equal(1, impersonatorFuncWasCalled) // wasn't started a second time r.Len(kubeAPIClient.Actions(), 4) requireTLSSecretWasCreated(kubeAPIClient.Actions()[3], ca) // uses the ca from last time requireTLSServerIsRunning(ca, hostname, map[string]string{hostname + httpsPort: testServerAddr()}) // running with certs now requireCredentialIssuer(newSuccessStrategy(hostname, ca)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) // Simulate the informer cache's background update from its watch. addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[3], kubeInformers.Core().V1().Secrets()) r.NoError(runControllerSync()) - r.Equal(1, startTLSListenerFuncWasCalled) // wasn't started a third time + r.Equal(1, impersonatorFuncWasCalled) // wasn't started a third time r.Len(kubeAPIClient.Actions(), 4) // no more actions requireTLSServerIsRunning(ca, hostname, map[string]string{hostname + httpsPort: testServerAddr()}) // still running requireCredentialIssuer(newSuccessStrategy(hostname, ca)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) + }) + }) + + when("there is already a CredentialIssuer", func() { + preExistingStrategy := v1alpha1.CredentialIssuerStrategy{ + Type: v1alpha1.KubeClusterSigningCertificateStrategyType, + Status: v1alpha1.SuccessStrategyStatus, + Reason: v1alpha1.FetchedKeyStrategyReason, + Message: "happy other unrelated strategy", + LastUpdateTime: metav1.NewTime(frozenNow), + Frontend: &v1alpha1.CredentialIssuerFrontend{ + Type: v1alpha1.TokenCredentialRequestAPIFrontendType, + }, + } + + it.Before(func() { + addSecretToTrackers(signingCASecret, kubeInformerClient) + r.NoError(pinnipedAPIClient.Tracker().Add(&v1alpha1.CredentialIssuer{ + ObjectMeta: metav1.ObjectMeta{Name: credentialIssuerResourceName}, + Status: v1alpha1.CredentialIssuerStatus{Strategies: []v1alpha1.CredentialIssuerStrategy{preExistingStrategy}}, + })) + addNodeWithRoleToTracker("worker", kubeAPIClient) + }) + + it("merges into the existing strategy array on the CredentialIssuer", func() { + startInformersAndController() + r.NoError(runControllerSync()) + requireTLSServerIsRunningWithoutCerts() + r.Len(kubeAPIClient.Actions(), 3) + requireNodesListed(kubeAPIClient.Actions()[0]) + requireLoadBalancerWasCreated(kubeAPIClient.Actions()[1]) + requireCASecretWasCreated(kubeAPIClient.Actions()[2]) + credentialIssuer := getCredentialIssuer() + r.Equal([]v1alpha1.CredentialIssuerStrategy{preExistingStrategy, newPendingStrategy()}, credentialIssuer.Status.Strategies) }) }) @@ -1761,21 +1952,110 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { startInformersAndController() r.EqualError(runControllerSync(), "no nodes found") requireCredentialIssuer(newErrorStrategy("no nodes found")) + requireSigningCertProviderIsEmpty() requireTLSServerWasNeverStarted() }) }) - when("the http handler factory function returns an error", func() { + when("the impersonator start function returned by the impersonatorFunc returns an error immediately", func() { it.Before(func() { addNodeWithRoleToTracker("worker", kubeAPIClient) - httpHandlerFactoryFuncError = errors.New("some factory error") + impersonatorFuncReturnedFuncError = errors.New("some immediate impersonator startup error") }) - it("returns an error", func() { + it("causes an immediate resync, returns an error on that next sync, and then restarts the server in a following sync", func() { startInformersAndController() - r.EqualError(runControllerSync(), "some factory error") - requireCredentialIssuer(newErrorStrategy("some factory error")) - requireTLSServerWasNeverStarted() + // The failure happens in a background goroutine, so the first sync succeeds. + r.NoError(runControllerSync()) + // Eventually the server is not really running, because the startup failed. + r.Nil(startedTLSListener) + r.Equal(impersonatorFuncWasCalled, 1) + r.Len(kubeAPIClient.Actions(), 3) + requireNodesListed(kubeAPIClient.Actions()[0]) + requireLoadBalancerWasCreated(kubeAPIClient.Actions()[1]) + requireCASecretWasCreated(kubeAPIClient.Actions()[2]) + requireCredentialIssuer(newPendingStrategy()) + requireSigningCertProviderIsEmpty() + + // Simulate the informer cache's background update from its watch. + addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[1], kubeInformers.Core().V1().Services()) + addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[2], kubeInformers.Core().V1().Secrets()) + + // The controller's first sync should have started a background routine which, when the server dies, + // requests to re-enqueue the original sync key to cause its sync method to get called again in the near future. + r.Eventually(func() bool { + queue.mutex.RLock() // this is to satisfy the race detector + defer queue.mutex.RUnlock() + return syncContext.Key == queue.key + }, 10*time.Second, 10*time.Millisecond) + + // The next sync should error because the server died in the background. This second + // sync should be able to detect the error and return it. + r.EqualError(runControllerSync(), "some immediate impersonator startup error") + requireCredentialIssuer(newErrorStrategy("some immediate impersonator startup error")) + requireSigningCertProviderIsEmpty() + + // Next time the controller starts the server, the server will start successfully. + impersonatorFuncReturnedFuncError = nil + + // One more sync and the controller should try to restart the server. + // Now everything should be working correctly. + r.NoError(runControllerSync()) + requireTLSServerIsRunningWithoutCerts() + requireCredentialIssuer(newPendingStrategy()) + requireSigningCertProviderIsEmpty() + }) + }) + + when("the impersonator server dies for no apparent reason after running for a while", func() { + it.Before(func() { + addNodeWithRoleToTracker("worker", kubeAPIClient) + }) + + it("causes an immediate resync, returns an error on that next sync, and then restarts the server in a following sync", func() { + // Prepare to be able to cause the server to die for no apparent reason. + testHTTPServerInterruptCh = make(chan struct{}) + + startInformersAndController() + r.NoError(runControllerSync()) + r.Len(kubeAPIClient.Actions(), 3) + requireNodesListed(kubeAPIClient.Actions()[0]) + requireLoadBalancerWasCreated(kubeAPIClient.Actions()[1]) + requireCASecretWasCreated(kubeAPIClient.Actions()[2]) + requireCredentialIssuer(newPendingStrategy()) + requireSigningCertProviderIsEmpty() + requireTLSServerIsRunningWithoutCerts() + + // Simulate the informer cache's background update from its watch. + addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[1], kubeInformers.Core().V1().Services()) + addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[2], kubeInformers.Core().V1().Secrets()) + + // Simulate that impersonation server dies for no apparent reason. + close(testHTTPServerInterruptCh) + + // The controller's first sync should have started a background routine which, when the server dies, + // requests to re-enqueue the original sync key to cause its sync method to get called again in the near future. + r.Eventually(func() bool { + queue.mutex.RLock() // this is to satisfy the race detector + defer queue.mutex.RUnlock() + return syncContext.Key == queue.key + }, 10*time.Second, 10*time.Millisecond) + + // The next sync should error because the server died in the background. This second + // sync should be able to detect the error and return it. + r.EqualError(runControllerSync(), "unexpected shutdown of proxy server") + requireCredentialIssuer(newErrorStrategy("unexpected shutdown of proxy server")) + requireSigningCertProviderIsEmpty() + + // Next time the controller starts the server, the server should behave as normal. + testHTTPServerInterruptCh = nil + + // One more sync and the controller should try to restart the server. + // Now everything should be working correctly. + r.NoError(runControllerSync()) + requireTLSServerIsRunningWithoutCerts() + requireCredentialIssuer(newPendingStrategy()) + requireSigningCertProviderIsEmpty() }) }) @@ -1789,6 +2069,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { errString := "invalid impersonator configuration: decode yaml: error unmarshaling JSON: while decoding JSON: json: cannot unmarshal string into Go value of type impersonator.Config" r.EqualError(runControllerSync(), errString) requireCredentialIssuer(newErrorStrategy(errString)) + requireSigningCertProviderIsEmpty() requireTLSServerWasNeverStarted() }) }) @@ -1805,6 +2086,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { startInformersAndController() r.EqualError(runControllerSync(), "error on create") requireCredentialIssuer(newErrorStrategy("error on create")) + requireSigningCertProviderIsEmpty() requireTLSServerIsRunningWithoutCerts() }) }) @@ -1826,6 +2108,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { startInformersAndController() r.EqualError(runControllerSync(), "error on tls secret create") requireCredentialIssuer(newErrorStrategy("error on tls secret create")) + requireSigningCertProviderIsEmpty() requireTLSServerIsRunningWithoutCerts() r.Len(kubeAPIClient.Actions(), 3) requireNodesListed(kubeAPIClient.Actions()[0]) @@ -1851,6 +2134,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { startInformersAndController() r.EqualError(runControllerSync(), "error on ca secret create") requireCredentialIssuer(newErrorStrategy("error on ca secret create")) + requireSigningCertProviderIsEmpty() requireTLSServerIsRunningWithoutCerts() r.Len(kubeAPIClient.Actions(), 2) requireNodesListed(kubeAPIClient.Actions()[0]) @@ -1870,6 +2154,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { errString := "could not load CA: tls: failed to find any PEM data in certificate input" r.EqualError(runControllerSync(), errString) requireCredentialIssuer(newErrorStrategy(errString)) + requireSigningCertProviderIsEmpty() requireTLSServerIsRunningWithoutCerts() r.Len(kubeAPIClient.Actions(), 1) requireNodesListed(kubeAPIClient.Actions()[0]) @@ -1891,6 +2176,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { it("does not start the impersonator, deletes the loadbalancer, returns an error", func() { r.EqualError(runControllerSync(), "error on delete") requireCredentialIssuer(newErrorStrategy("error on delete")) + requireSigningCertProviderIsEmpty() requireTLSServerWasNeverStarted() r.Len(kubeAPIClient.Actions(), 3) requireNodesListed(kubeAPIClient.Actions()[0]) @@ -1901,6 +2187,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { when("the PEM formatted data in the TLS Secret is not a valid cert", func() { it.Before(func() { + addSecretToTrackers(signingCASecret, kubeInformerClient) configMapYAML := fmt.Sprintf("{mode: enabled, endpoint: %s}", localhostIP) addImpersonatorConfigMapToTracker(configMapResourceName, configMapYAML, kubeInformerClient) addNodeWithRoleToTracker("worker", kubeAPIClient) @@ -1927,6 +2214,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireTLSSecretWasCreated(kubeAPIClient.Actions()[3], ca) requireTLSServerIsRunning(ca, testServerAddr(), nil) requireCredentialIssuer(newSuccessStrategy(localhostIP, ca)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) }) when("there is an error while the invalid cert is being deleted", func() { @@ -1941,6 +2229,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { errString := "PEM data represented an invalid cert, but got error while deleting it: error on delete" r.EqualError(runControllerSync(), errString) requireCredentialIssuer(newErrorStrategy(errString)) + requireSigningCertProviderIsEmpty() requireTLSServerIsRunningWithoutCerts() r.Len(kubeAPIClient.Actions(), 3) requireNodesListed(kubeAPIClient.Actions()[0]) @@ -1954,6 +2243,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { when("a tls secret already exists but it is not valid", func() { var caCrt []byte it.Before(func() { + addSecretToTrackers(signingCASecret, kubeInformerClient) addImpersonatorConfigMapToTracker(configMapResourceName, "mode: enabled", kubeInformerClient) addNodeWithRoleToTracker("worker", kubeAPIClient) ca := newCA() @@ -1974,6 +2264,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireTLSSecretWasCreated(kubeAPIClient.Actions()[2], caCrt) requireTLSServerIsRunning(caCrt, testServerAddr(), nil) requireCredentialIssuer(newSuccessStrategy(localhostIP, caCrt)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) }) when("there is an error while the invalid cert is being deleted", func() { @@ -1988,6 +2279,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { errString := "found missing or not PEM-encoded data in TLS Secret, but got error while deleting it: error on delete" r.EqualError(runControllerSync(), errString) requireCredentialIssuer(newErrorStrategy(errString)) + requireSigningCertProviderIsEmpty() requireTLSServerIsRunningWithoutCerts() r.Len(kubeAPIClient.Actions(), 2) requireNodesListed(kubeAPIClient.Actions()[0]) @@ -2000,6 +2292,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { when("a tls secret already exists but the private key is not valid", func() { var caCrt []byte it.Before(func() { + addSecretToTrackers(signingCASecret, kubeInformerClient) addImpersonatorConfigMapToTracker(configMapResourceName, "mode: enabled", kubeInformerClient) addNodeWithRoleToTracker("worker", kubeAPIClient) ca := newCA() @@ -2022,6 +2315,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireTLSSecretWasCreated(kubeAPIClient.Actions()[2], caCrt) requireTLSServerIsRunning(caCrt, testServerAddr(), nil) requireCredentialIssuer(newSuccessStrategy(localhostIP, caCrt)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) }) when("there is an error while the invalid cert is being deleted", func() { @@ -2036,6 +2330,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { errString := "cert had an invalid private key, but got error while deleting it: error on delete" r.EqualError(runControllerSync(), errString) requireCredentialIssuer(newErrorStrategy(errString)) + requireSigningCertProviderIsEmpty() requireTLSServerIsRunningWithoutCerts() r.Len(kubeAPIClient.Actions(), 2) requireNodesListed(kubeAPIClient.Actions()[0]) @@ -2047,6 +2342,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { when("there is an error while creating or updating the CredentialIssuer status", func() { it.Before(func() { + addSecretToTrackers(signingCASecret, kubeInformerClient) addNodeWithRoleToTracker("worker", kubeAPIClient) pinnipedAPIClient.PrependReactor("create", "credentialissuers", func(action coretesting.Action) (handled bool, ret runtime.Object, err error) { return true, nil, fmt.Errorf("error on create") @@ -2072,37 +2368,110 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { }) }) - when("there is already a CredentialIssuer", func() { - preExistingStrategy := v1alpha1.CredentialIssuerStrategy{ - Type: v1alpha1.KubeClusterSigningCertificateStrategyType, - Status: v1alpha1.SuccessStrategyStatus, - Reason: v1alpha1.FetchedKeyStrategyReason, - Message: "happy other unrelated strategy", - LastUpdateTime: metav1.NewTime(frozenNow), - Frontend: &v1alpha1.CredentialIssuerFrontend{ - Type: v1alpha1.TokenCredentialRequestAPIFrontendType, - }, - } - + when("the impersonator is ready but there is a problem with the signing secret, which should be created by another controller", func() { + const fakeHostname = "foo.example.com" it.Before(func() { - r.NoError(pinnipedAPIClient.Tracker().Add(&v1alpha1.CredentialIssuer{ - ObjectMeta: metav1.ObjectMeta{Name: credentialIssuerResourceName}, - Status: v1alpha1.CredentialIssuerStatus{Strategies: []v1alpha1.CredentialIssuerStrategy{preExistingStrategy}}, - })) + configMapYAML := fmt.Sprintf("{mode: enabled, endpoint: %s}", fakeHostname) + addImpersonatorConfigMapToTracker(configMapResourceName, configMapYAML, kubeInformerClient) addNodeWithRoleToTracker("worker", kubeAPIClient) }) - it("merges into the existing strategy array on the CredentialIssuer", func() { - startInformersAndController() - r.NoError(runControllerSync()) - requireTLSServerIsRunningWithoutCerts() - r.Len(kubeAPIClient.Actions(), 3) - requireNodesListed(kubeAPIClient.Actions()[0]) - requireLoadBalancerWasCreated(kubeAPIClient.Actions()[1]) - requireCASecretWasCreated(kubeAPIClient.Actions()[2]) - credentialIssuer := getCredentialIssuer() - r.Equal([]v1alpha1.CredentialIssuerStrategy{preExistingStrategy, newPendingStrategy()}, credentialIssuer.Status.Strategies) + when("it does not exist in the informers", func() { + it("returns the error", func() { + startInformersAndController() + errString := `could not load the impersonator's credential signing secret: secret "some-ca-signer-name" not found` + r.EqualError(runControllerSync(), errString) + requireCredentialIssuer(newErrorStrategy(errString)) + requireSigningCertProviderIsEmpty() + }) + }) + + when("it does not have the expected fields", func() { + it.Before(func() { + addSecretToTrackers(newEmptySecret(caSignerName), kubeInformerClient) + }) + + it("returns the error", func() { + startInformersAndController() + errString := `could not load the impersonator's credential signing secret: tls: failed to find any PEM data in certificate input` + r.EqualError(runControllerSync(), errString) + requireCredentialIssuer(newErrorStrategy(errString)) + requireSigningCertProviderIsEmpty() + }) + }) + + when("the cert is invalid", func() { + it.Before(func() { + signingCASecret.Data[apicerts.CACertificateSecretKey] = []byte("not a valid PEM formatted cert") + addSecretToTrackers(signingCASecret, kubeInformerClient) + }) + + it("returns the error", func() { + startInformersAndController() + errString := `could not load the impersonator's credential signing secret: tls: failed to find any PEM data in certificate input` + r.EqualError(runControllerSync(), errString) + requireCredentialIssuer(newErrorStrategy(errString)) + requireSigningCertProviderIsEmpty() + }) + }) + + when("the cert goes from being valid to being invalid", func() { + const fakeHostname = "foo.example.com" + it.Before(func() { + addSecretToTrackers(signingCASecret, kubeInformerClient) + }) + + it("returns the error and clears the dynamic provider", func() { + startInformersAndController() + r.NoError(runControllerSync()) + r.Len(kubeAPIClient.Actions(), 3) + requireNodesListed(kubeAPIClient.Actions()[0]) + ca := requireCASecretWasCreated(kubeAPIClient.Actions()[1]) + requireTLSSecretWasCreated(kubeAPIClient.Actions()[2], ca) + // Check that the server is running and that TLS certs that are being served are are for fakeHostname. + requireTLSServerIsRunning(ca, fakeHostname, map[string]string{fakeHostname + httpsPort: testServerAddr()}) + requireCredentialIssuer(newSuccessStrategy(fakeHostname, ca)) + requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM) + + // Simulate the informer cache's background update from its watch. + addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[1], kubeInformers.Core().V1().Secrets()) + addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[2], kubeInformers.Core().V1().Secrets()) + + // Now update the signer CA to something invalid. + deleteSecretFromTracker(caSignerName, kubeInformerClient) + waitForObjectToBeDeletedFromInformer(caSignerName, kubeInformers.Core().V1().Secrets()) + updatedSigner := newEmptySecret(caSignerName) + addSecretToTrackers(updatedSigner, kubeInformerClient) + waitForObjectToAppearInInformer(updatedSigner, kubeInformers.Core().V1().Secrets()) + + errString := `could not load the impersonator's credential signing secret: tls: failed to find any PEM data in certificate input` + r.EqualError(runControllerSync(), errString) + requireCredentialIssuer(newErrorStrategy(errString)) + requireSigningCertProviderIsEmpty() + }) }) }) }, spec.Parallel(), spec.Report(report.Terminal{})) } + +type testQueue struct { + key controllerlib.Key + mutex sync.RWMutex + + controllerlib.Queue +} + +func (q *testQueue) AddRateLimited(key controllerlib.Key) { + q.mutex.Lock() // this is to satisfy the race detector + defer q.mutex.Unlock() + + if q.key != (controllerlib.Key{}) { + panic("called more than once") + } + + if key == (controllerlib.Key{}) { + panic("unexpected empty key") + } + + q.key = key +} diff --git a/internal/controllermanager/prepare_controllers.go b/internal/controllermanager/prepare_controllers.go index d727a1bc8..5af528e41 100644 --- a/internal/controllermanager/prepare_controllers.go +++ b/internal/controllermanager/prepare_controllers.go @@ -7,12 +7,9 @@ package controllermanager import ( "context" - "crypto/tls" "fmt" - "net/http" "time" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/clock" k8sinformers "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes" @@ -67,9 +64,11 @@ type Config struct { // DynamicServingCertProvider provides a setter and a getter to the Pinniped API's serving cert. DynamicServingCertProvider dynamiccert.Provider - // DynamicSigningCertProvider provides a setter and a getter to the Pinniped API's + // DynamicSigningCertProvider provides a setter and a getter to the Pinniped API's // TODO fix comment // signing cert, i.e., the cert that it uses to sign certs for Pinniped clients wishing to login. DynamicSigningCertProvider dynamiccert.Provider + // TODO fix comment + ImpersonationSigningCertProvider dynamiccert.Provider // ServingCertDuration is the validity period, in seconds, of the API serving certificate. ServingCertDuration time.Duration @@ -81,10 +80,6 @@ type Config struct { // AuthenticatorCache is a cache of authenticators shared amongst various authenticated-related controllers. AuthenticatorCache *authncache.Cache - // LoginJSONDecoder can decode login.concierge.pinniped.dev types (e.g., TokenCredentialRequest) - // into their internal representation. - LoginJSONDecoder runtime.Decoder - // Labels are labels that should be added to any resources created by the controllers. Labels map[string]string } @@ -188,6 +183,7 @@ func PrepareControllers(c *Config) (func(ctx context.Context), error) { informers.installationNamespaceK8s.Core().V1().Secrets(), controllerlib.WithInformer, c.ServingCertRenewBefore, + apicerts.TLSCertificateChainSecretKey, ), singletonWorker, ). @@ -295,18 +291,36 @@ func PrepareControllers(c *Config) (func(ctx context.Context), error) { c.NamesConfig.ImpersonationCACertificateSecret, c.Labels, clock.RealClock{}, - tls.Listen, - func() (http.Handler, error) { - impersonationProxyHandler, err := impersonator.New( - c.AuthenticatorCache, - c.LoginJSONDecoder, - klogr.New().WithName("impersonation-proxy"), - ) - if err != nil { - return nil, fmt.Errorf("could not create impersonation proxy: %w", err) - } - return impersonationProxyHandler, nil - }, + impersonator.New, + c.NamesConfig.ImpersonationSignerSecret, + c.ImpersonationSigningCertProvider, + ), + singletonWorker, + ). + WithController( + apicerts.NewCertsManagerController( + c.ServerInstallationInfo.Namespace, + c.NamesConfig.ImpersonationSignerSecret, + c.Labels, + client.Kubernetes, + informers.installationNamespaceK8s.Core().V1().Secrets(), + controllerlib.WithInformer, + controllerlib.WithInitialEvent, + 365*24*time.Hour, // 1 year hard coded value + "Pinniped Impersonation Proxy CA", + "", // optional, means do not give me a serving cert + ), + singletonWorker, + ). + WithController( + apicerts.NewCertsExpirerController( + c.ServerInstallationInfo.Namespace, + c.NamesConfig.ImpersonationSignerSecret, + client.Kubernetes, + informers.installationNamespaceK8s.Core().V1().Secrets(), + controllerlib.WithInformer, + c.ServingCertRenewBefore, + apicerts.CACertificateSecretKey, ), singletonWorker, ) diff --git a/internal/dynamiccert/provider.go b/internal/dynamiccert/provider.go index a4ca6ad5d..77687fb14 100644 --- a/internal/dynamiccert/provider.go +++ b/internal/dynamiccert/provider.go @@ -1,9 +1,10 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package dynamiccert import ( + "crypto/x509" "sync" "k8s.io/apiserver/pkg/server/dynamiccertificates" @@ -13,6 +14,8 @@ import ( // certificate and matching key. type Provider interface { dynamiccertificates.CertKeyContentProvider + // TODO dynamiccertificates.Notifier + // TODO dynamiccertificates.ControllerRunner ??? Set(certPEM, keyPEM []byte) } @@ -43,3 +46,27 @@ func (p *provider) CurrentCertKeyContent() (cert []byte, key []byte) { defer p.mutex.RUnlock() return p.certPEM, p.keyPEM } + +func NewCAProvider(delegate dynamiccertificates.CertKeyContentProvider) dynamiccertificates.CAContentProvider { + return &caContentProvider{delegate: delegate} +} + +type caContentProvider struct { + delegate dynamiccertificates.CertKeyContentProvider +} + +func (c *caContentProvider) Name() string { + return "DynamicCAProvider" +} + +func (c *caContentProvider) CurrentCABundleContent() []byte { + ca, _ := c.delegate.CurrentCertKeyContent() + return ca +} + +func (c *caContentProvider) VerifyOptions() (x509.VerifyOptions, bool) { + return x509.VerifyOptions{}, false // assume we are unioned via dynamiccertificates.NewUnionCAContentProvider +} + +// TODO look at both the serving side union struct and the ca side union struct for all optional interfaces +// and then implement everything that makes sense for us to implement diff --git a/internal/issuer/issuer.go b/internal/issuer/issuer.go new file mode 100644 index 000000000..94e2f2352 --- /dev/null +++ b/internal/issuer/issuer.go @@ -0,0 +1,42 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package issuer + +import ( + "crypto/x509/pkix" + "time" + + "k8s.io/apimachinery/pkg/util/errors" + + "go.pinniped.dev/internal/constable" +) + +const defaultCertIssuerErr = constable.Error("failed to issue cert") + +type CertIssuer interface { + IssuePEM(subject pkix.Name, dnsNames []string, ttl time.Duration) (certPEM, keyPEM []byte, err error) +} + +var _ CertIssuer = CertIssuers{} + +type CertIssuers []CertIssuer + +func (c CertIssuers) IssuePEM(subject pkix.Name, dnsNames []string, ttl time.Duration) ([]byte, []byte, error) { + var errs []error + + for _, issuer := range c { + certPEM, keyPEM, err := issuer.IssuePEM(subject, dnsNames, ttl) + if err != nil { + errs = append(errs, err) + continue + } + return certPEM, keyPEM, nil + } + + if err := errors.NewAggregate(errs); err != nil { + return nil, nil, err + } + + return nil, nil, defaultCertIssuerErr +} diff --git a/internal/registry/credentialrequest/rest.go b/internal/registry/credentialrequest/rest.go index a89066761..a1846a406 100644 --- a/internal/registry/credentialrequest/rest.go +++ b/internal/registry/credentialrequest/rest.go @@ -22,20 +22,17 @@ import ( "k8s.io/utils/trace" loginapi "go.pinniped.dev/generated/latest/apis/concierge/login" + "go.pinniped.dev/internal/issuer" ) // clientCertificateTTL is the TTL for short-lived client certificates returned by this API. const clientCertificateTTL = 5 * time.Minute -type CertIssuer interface { - IssuePEM(subject pkix.Name, dnsNames []string, ttl time.Duration) ([]byte, []byte, error) -} - type TokenCredentialRequestAuthenticator interface { AuthenticateTokenCredentialRequest(ctx context.Context, req *loginapi.TokenCredentialRequest) (user.Info, error) } -func NewREST(authenticator TokenCredentialRequestAuthenticator, issuer CertIssuer, resource schema.GroupResource) *REST { +func NewREST(authenticator TokenCredentialRequestAuthenticator, issuer issuer.CertIssuer, resource schema.GroupResource) *REST { return &REST{ authenticator: authenticator, issuer: issuer, @@ -45,7 +42,7 @@ func NewREST(authenticator TokenCredentialRequestAuthenticator, issuer CertIssue type REST struct { authenticator TokenCredentialRequestAuthenticator - issuer CertIssuer + issuer issuer.CertIssuer tableConvertor rest.TableConvertor } diff --git a/internal/registry/credentialrequest/rest_test.go b/internal/registry/credentialrequest/rest_test.go index 8542b99e8..6b71ec20f 100644 --- a/internal/registry/credentialrequest/rest_test.go +++ b/internal/registry/credentialrequest/rest_test.go @@ -24,6 +24,7 @@ import ( "k8s.io/klog/v2" loginapi "go.pinniped.dev/generated/latest/apis/concierge/login" + "go.pinniped.dev/internal/issuer" "go.pinniped.dev/internal/mocks/credentialrequestmocks" "go.pinniped.dev/internal/testutil" ) @@ -353,12 +354,12 @@ func requireSuccessfulResponseWithAuthenticationFailureMessage(t *testing.T, err }) } -func successfulIssuer(ctrl *gomock.Controller) CertIssuer { - issuer := credentialrequestmocks.NewMockCertIssuer(ctrl) - issuer.EXPECT(). +func successfulIssuer(ctrl *gomock.Controller) issuer.CertIssuer { + certIssuer := credentialrequestmocks.NewMockCertIssuer(ctrl) + certIssuer.EXPECT(). IssuePEM(gomock.Any(), gomock.Any(), gomock.Any()). Return([]byte("test-cert"), []byte("test-key"), nil) - return issuer + return certIssuer } func stringPtr(s string) *string { diff --git a/internal/testutil/impersonationtoken/impersonationtoken.go b/internal/testutil/impersonationtoken/impersonationtoken.go deleted file mode 100644 index 64b653785..000000000 --- a/internal/testutil/impersonationtoken/impersonationtoken.go +++ /dev/null @@ -1,65 +0,0 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. -// SPDX-License-Identifier: Apache-2.0 - -// Package impersonationtoken contains a test utility to generate a token to be used against our -// impersonation proxy. -// -// It is its own package to fix import cycles involving concierge/scheme, testutil, and groupsuffix. -package impersonationtoken - -import ( - "encoding/base64" - "testing" - - "github.com/stretchr/testify/require" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/serializer" - - loginv1alpha1 "go.pinniped.dev/generated/latest/apis/concierge/login/v1alpha1" - conciergescheme "go.pinniped.dev/internal/concierge/scheme" - "go.pinniped.dev/internal/groupsuffix" -) - -func Make( - t *testing.T, - token string, - authenticator *corev1.TypedLocalObjectReference, - apiGroupSuffix string, -) string { - t.Helper() - - // The impersonation test token should be a base64-encoded TokenCredentialRequest object. The API - // group of the TokenCredentialRequest object, and its Spec.Authenticator, should match whatever - // is installed on the cluster. This API group is usually replaced by the kubeclient middleware, - // but this object is not touched by the middleware since it is in a HTTP header. Therefore, we - // need to make a manual edit here. - scheme, loginGV, _ := conciergescheme.New(apiGroupSuffix) - tokenCredentialRequest := loginv1alpha1.TokenCredentialRequest{ - TypeMeta: metav1.TypeMeta{ - Kind: "TokenCredentialRequest", - APIVersion: loginGV.Group + "/v1alpha1", - }, - Spec: loginv1alpha1.TokenCredentialRequestSpec{ - Token: token, - Authenticator: *authenticator.DeepCopy(), - }, - } - - // It is assumed that the provided authenticator uses the default pinniped.dev API group, since - // this is usually replaced by the kubeclient middleware. Since we are not going through the - // kubeclient middleware, we need to make this replacement ourselves. - require.NotNil(t, tokenCredentialRequest.Spec.Authenticator.APIGroup, "expected authenticator to have non-nil API group") - authenticatorAPIGroup, ok := groupsuffix.Replace(*tokenCredentialRequest.Spec.Authenticator.APIGroup, apiGroupSuffix) - require.True(t, ok, "couldn't replace suffix of %q with %q", *tokenCredentialRequest.Spec.Authenticator.APIGroup, apiGroupSuffix) - tokenCredentialRequest.Spec.Authenticator.APIGroup = &authenticatorAPIGroup - - codecs := serializer.NewCodecFactory(scheme) - respInfo, ok := runtime.SerializerInfoForMediaType(codecs.SupportedMediaTypes(), runtime.ContentTypeJSON) - require.True(t, ok, "couldn't find serializer info for media type") - - reqJSON, err := runtime.Encode(respInfo.PrettySerializer, &tokenCredentialRequest) - require.NoError(t, err) - return base64.StdEncoding.EncodeToString(reqJSON) -} diff --git a/test/integration/concierge_credentialrequest_test.go b/test/integration/concierge_credentialrequest_test.go index de48bf85f..196670d4b 100644 --- a/test/integration/concierge_credentialrequest_test.go +++ b/test/integration/concierge_credentialrequest_test.go @@ -30,11 +30,13 @@ func TestUnsuccessfulCredentialRequest(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), time.Minute) defer cancel() - response, err := makeRequest(ctx, t, validCredentialRequestSpecWithRealToken(t, corev1.TypedLocalObjectReference{ - APIGroup: &auth1alpha1.SchemeGroupVersion.Group, - Kind: "WebhookAuthenticator", - Name: "some-webhook-that-does-not-exist", - })) + response, err := library.CreateTokenCredentialRequest(ctx, t, + validCredentialRequestSpecWithRealToken(t, corev1.TypedLocalObjectReference{ + APIGroup: &auth1alpha1.SchemeGroupVersion.Group, + Kind: "WebhookAuthenticator", + Name: "some-webhook-that-does-not-exist", + }), + ) require.NoError(t, err) require.Nil(t, response.Status.Credential) require.NotNil(t, response.Status.Message) @@ -88,10 +90,9 @@ func TestSuccessfulCredentialRequest(t *testing.T) { var response *loginv1alpha1.TokenCredentialRequest successfulResponse := func() bool { var err error - response, err = makeRequest(ctx, t, loginv1alpha1.TokenCredentialRequestSpec{ - Token: token, - Authenticator: authenticator, - }) + response, err = library.CreateTokenCredentialRequest(ctx, t, + loginv1alpha1.TokenCredentialRequestSpec{Token: token, Authenticator: authenticator}, + ) require.NoError(t, err, "the request should never fail at the HTTP level") return response.Status.Credential != nil } @@ -141,10 +142,9 @@ func TestFailedCredentialRequestWhenTheRequestIsValidButTheTokenDoesNotAuthentic defer cancel() testWebhook := library.CreateTestWebhookAuthenticator(ctx, t) - response, err := makeRequest(context.Background(), t, loginv1alpha1.TokenCredentialRequestSpec{ - Token: "not a good token", - Authenticator: testWebhook, - }) + response, err := library.CreateTokenCredentialRequest(context.Background(), t, + loginv1alpha1.TokenCredentialRequestSpec{Token: "not a good token", Authenticator: testWebhook}, + ) require.NoError(t, err) @@ -164,10 +164,9 @@ func TestCredentialRequest_ShouldFailWhenRequestDoesNotIncludeToken(t *testing.T defer cancel() testWebhook := library.CreateTestWebhookAuthenticator(ctx, t) - response, err := makeRequest(context.Background(), t, loginv1alpha1.TokenCredentialRequestSpec{ - Token: "", - Authenticator: testWebhook, - }) + response, err := library.CreateTokenCredentialRequest(context.Background(), t, + loginv1alpha1.TokenCredentialRequestSpec{Token: "", Authenticator: testWebhook}, + ) require.Error(t, err) statusError, isStatus := err.(*errors.StatusError) @@ -193,7 +192,7 @@ func TestCredentialRequest_OtherwiseValidRequestWithRealTokenShouldFailWhenTheCl testWebhook := library.CreateTestWebhookAuthenticator(ctx, t) - response, err := makeRequest(ctx, t, validCredentialRequestSpecWithRealToken(t, testWebhook)) + response, err := library.CreateTokenCredentialRequest(ctx, t, validCredentialRequestSpecWithRealToken(t, testWebhook)) require.NoError(t, err) @@ -202,22 +201,6 @@ func TestCredentialRequest_OtherwiseValidRequestWithRealTokenShouldFailWhenTheCl require.Equal(t, stringPtr("authentication failed"), response.Status.Message) } -func makeRequest(ctx context.Context, t *testing.T, spec loginv1alpha1.TokenCredentialRequestSpec) (*loginv1alpha1.TokenCredentialRequest, error) { - t.Helper() - env := library.IntegrationEnv(t) - - client := library.NewAnonymousConciergeClientset(t) - - ctx, cancel := context.WithTimeout(ctx, time.Minute) - defer cancel() - - return client.LoginV1alpha1().TokenCredentialRequests().Create(ctx, &loginv1alpha1.TokenCredentialRequest{ - TypeMeta: metav1.TypeMeta{}, - ObjectMeta: metav1.ObjectMeta{Namespace: env.ConciergeNamespace}, - Spec: spec, - }, metav1.CreateOptions{}) -} - func validCredentialRequestSpecWithRealToken(t *testing.T, authenticator corev1.TypedLocalObjectReference) loginv1alpha1.TokenCredentialRequestSpec { return loginv1alpha1.TokenCredentialRequestSpec{ Token: library.IntegrationEnv(t).TestUser.Token, diff --git a/test/integration/concierge_impersonation_proxy_test.go b/test/integration/concierge_impersonation_proxy_test.go index 06f0a55de..a48e32033 100644 --- a/test/integration/concierge_impersonation_proxy_test.go +++ b/test/integration/concierge_impersonation_proxy_test.go @@ -10,6 +10,7 @@ import ( "crypto/x509" "encoding/base64" "encoding/json" + "encoding/pem" "fmt" "io/ioutil" "net/http" @@ -21,9 +22,8 @@ import ( "testing" "time" - "golang.org/x/net/websocket" - "github.com/stretchr/testify/require" + "golang.org/x/net/websocket" v1 "k8s.io/api/authorization/v1" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" @@ -38,10 +38,10 @@ import ( "sigs.k8s.io/yaml" "go.pinniped.dev/generated/latest/apis/concierge/config/v1alpha1" + loginv1alpha1 "go.pinniped.dev/generated/latest/apis/concierge/login/v1alpha1" "go.pinniped.dev/generated/latest/client/concierge/clientset/versioned" "go.pinniped.dev/internal/concierge/impersonator" "go.pinniped.dev/internal/testutil" - "go.pinniped.dev/internal/testutil/impersonationtoken" "go.pinniped.dev/test/library" ) @@ -49,7 +49,7 @@ import ( // - load balancers not supported, has squid proxy (e.g. kind) // - load balancers supported, has squid proxy (e.g. EKS) // - load balancers supported, no squid proxy (e.g. GKE) -func TestImpersonationProxy(t *testing.T) { +func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's complex. env := library.IntegrationEnv(t) ctx, cancel := context.WithTimeout(context.Background(), 20*time.Minute) @@ -64,14 +64,67 @@ func TestImpersonationProxy(t *testing.T) { // The address of the ClusterIP service that points at the impersonation proxy's port (used when there is no load balancer). proxyServiceEndpoint := fmt.Sprintf("%s-proxy.%s.svc.cluster.local", env.ConciergeAppName, env.ConciergeNamespace) + expectedProxyServiceEndpointURL := "https://" + proxyServiceEndpoint // The error message that will be returned by squid when the impersonation proxy port inside the cluster is not listening. serviceUnavailableViaSquidError := fmt.Sprintf(`Get "https://%s/api/v1/namespaces": Service Unavailable`, proxyServiceEndpoint) - impersonationProxyRestConfig := func(host string, caData []byte, doubleImpersonateUser string) *rest.Config { + credentialAlmostExpired := func(credential *loginv1alpha1.TokenCredentialRequest) bool { + pemBlock, _ := pem.Decode([]byte(credential.Status.Credential.ClientCertificateData)) + parsedCredential, err := x509.ParseCertificate(pemBlock.Bytes) + require.NoError(t, err) + timeRemaining := time.Until(parsedCredential.NotAfter) + if timeRemaining < 2*time.Minute { + t.Logf("The TokenCredentialRequest cred is almost expired and needs to be refreshed. Expires in %s.", timeRemaining) + return true + } + t.Logf("The TokenCredentialRequest cred is good for some more time (%s) so using it.", timeRemaining) + return false + } + + var tokenCredentialRequestResponse *loginv1alpha1.TokenCredentialRequest + refreshCredential := func() *loginv1alpha1.ClusterCredential { + if tokenCredentialRequestResponse == nil || credentialAlmostExpired(tokenCredentialRequestResponse) { + var err error + // Make a TokenCredentialRequest. This can either return a cert signed by the Kube API server's CA (e.g. on kind) + // or a cert signed by the impersonator's signing CA (e.g. on GKE). Either should be accepted by the impersonation + // proxy server as a valid authentication. + // + // However, we issue short-lived certs, so this cert will only be valid for a few minutes. + // Cache it until it is almost expired and then refresh it whenever it is close to expired. + tokenCredentialRequestResponse, err = library.CreateTokenCredentialRequest(ctx, t, loginv1alpha1.TokenCredentialRequestSpec{ + Token: env.TestUser.Token, + Authenticator: authenticator, + }) + require.NoError(t, err) + + require.Empty(t, tokenCredentialRequestResponse.Status.Message) // no error message + require.NotEmpty(t, tokenCredentialRequestResponse.Status.Credential.ClientCertificateData) + require.NotEmpty(t, tokenCredentialRequestResponse.Status.Credential.ClientKeyData) + + // At the moment the credential request should not have returned a token. In the future, if we make it return + // tokens, we should revisit this test's rest config below. + require.Empty(t, tokenCredentialRequestResponse.Status.Credential.Token) + } + return tokenCredentialRequestResponse.Status.Credential + } + + impersonationProxyRestConfig := func(credential *loginv1alpha1.ClusterCredential, host string, caData []byte, doubleImpersonateUser string) *rest.Config { config := rest.Config{ - Host: host, - TLSClientConfig: rest.TLSClientConfig{Insecure: caData == nil, CAData: caData}, - BearerToken: impersonationtoken.Make(t, env.TestUser.Token, &authenticator, env.APIGroupSuffix), + Host: host, + TLSClientConfig: rest.TLSClientConfig{ + Insecure: caData == nil, + CAData: caData, + CertData: []byte(credential.ClientCertificateData), + KeyData: []byte(credential.ClientKeyData), + }, + // kubectl would set both the client cert and the token, so we'll do that too. + // The Kube API server will ignore the token if the client cert successfully authenticates. + // Only if the client cert is not present or fails to authenticate will it use the token. + // Historically, it works that way because some web browsers will always send your + // corporate-assigned client cert even if it is not valid, and it doesn't want to treat + // that as a failure if you also sent a perfectly good token. + // We would like the impersonation proxy to imitate that behavior, so we test it here. + BearerToken: "this is not valid", } if doubleImpersonateUser != "" { config.Impersonate = rest.ImpersonationConfig{UserName: doubleImpersonateUser} @@ -79,9 +132,9 @@ func TestImpersonationProxy(t *testing.T) { return &config } - impersonationProxyViaSquidClient := func(caData []byte, doubleImpersonateUser string) kubernetes.Interface { + impersonationProxyViaSquidClient := func(proxyURL string, caData []byte, doubleImpersonateUser string) kubernetes.Interface { t.Helper() - kubeconfig := impersonationProxyRestConfig("https://"+proxyServiceEndpoint, caData, doubleImpersonateUser) + kubeconfig := impersonationProxyRestConfig(refreshCredential(), proxyURL, caData, doubleImpersonateUser) kubeconfig.Proxy = func(req *http.Request) (*url.URL, error) { proxyURL, err := url.Parse(env.Proxy) require.NoError(t, err) @@ -93,10 +146,17 @@ func TestImpersonationProxy(t *testing.T) { impersonationProxyViaLoadBalancerClient := func(proxyURL string, caData []byte, doubleImpersonateUser string) kubernetes.Interface { t.Helper() - kubeconfig := impersonationProxyRestConfig(proxyURL, caData, doubleImpersonateUser) + kubeconfig := impersonationProxyRestConfig(refreshCredential(), proxyURL, caData, doubleImpersonateUser) return library.NewKubeclient(t, kubeconfig).Kubernetes } + newImpersonationProxyClient := func(proxyURL string, impersonationProxyCACertPEM []byte, doubleImpersonateUser string) kubernetes.Interface { + if env.HasCapability(library.HasExternalLoadBalancerProvider) { + return impersonationProxyViaLoadBalancerClient(proxyURL, impersonationProxyCACertPEM, doubleImpersonateUser) + } + return impersonationProxyViaSquidClient(proxyURL, impersonationProxyCACertPEM, doubleImpersonateUser) + } + oldConfigMap, err := adminClient.CoreV1().ConfigMaps(env.ConciergeNamespace).Get(ctx, impersonationProxyConfigMapName(env), metav1.GetOptions{}) if !k8serrors.IsNotFound(err) { require.NoError(t, err) // other errors aside from NotFound are unexpected @@ -142,7 +202,7 @@ func TestImpersonationProxy(t *testing.T) { }, 10*time.Second, 500*time.Millisecond) // Check that we can't use the impersonation proxy to execute kubectl commands yet. - _, err = impersonationProxyViaSquidClient(nil, "").CoreV1().Namespaces().List(ctx, metav1.ListOptions{}) + _, err = impersonationProxyViaSquidClient(expectedProxyServiceEndpointURL, nil, "").CoreV1().Namespaces().List(ctx, metav1.ListOptions{}) require.EqualError(t, err, serviceUnavailableViaSquidError) // Create configuration to make the impersonation proxy turn on with a hard coded endpoint (without a load balancer). @@ -161,29 +221,30 @@ func TestImpersonationProxy(t *testing.T) { // in the strategies array or it may be included in an error state. It can be in an error state for // awhile when it is waiting for the load balancer to be assigned an ip/hostname. impersonationProxyURL, impersonationProxyCACertPEM := performImpersonatorDiscovery(ctx, t, env, adminConciergeClient) - - // Create an impersonation proxy client with that CA data to use for the rest of this test. - // This client performs TLS checks, so it also provides test coverage that the impersonation proxy server is generating TLS certs correctly. - var impersonationProxyClient kubernetes.Interface - if env.HasCapability(library.HasExternalLoadBalancerProvider) { - impersonationProxyClient = impersonationProxyViaLoadBalancerClient(impersonationProxyURL, impersonationProxyCACertPEM, "") - } else { + if !env.HasCapability(library.HasExternalLoadBalancerProvider) { // In this case, we specified the endpoint in the configmap, so check that it was reported correctly in the CredentialIssuer. require.Equal(t, "https://"+proxyServiceEndpoint, impersonationProxyURL) - impersonationProxyClient = impersonationProxyViaSquidClient(impersonationProxyCACertPEM, "") + } + + // Because our credentials expire so quickly, we'll always use a new client, to give us a chance to refresh our + // credentials before they expire. Create a closure to capture the arguments to newImpersonationProxyClient + // so we don't have to keep repeating them. + // This client performs TLS checks, so it also provides test coverage that the impersonation proxy server is generating TLS certs correctly. + impersonationProxyClient := func() kubernetes.Interface { + return newImpersonationProxyClient(impersonationProxyURL, impersonationProxyCACertPEM, "") } // Test that the user can perform basic actions through the client with their username and group membership // influencing RBAC checks correctly. t.Run( "access as user", - library.AccessAsUserTest(ctx, env.TestUser.ExpectedUsername, impersonationProxyClient), + library.AccessAsUserTest(ctx, env.TestUser.ExpectedUsername, impersonationProxyClient()), ) for _, group := range env.TestUser.ExpectedGroups { group := group t.Run( "access as group "+group, - library.AccessAsGroupTest(ctx, group, impersonationProxyClient), + library.AccessAsGroupTest(ctx, group, impersonationProxyClient()), ) } @@ -201,7 +262,6 @@ func TestImpersonationProxy(t *testing.T) { // Try more Kube API verbs through the impersonation proxy. t.Run("watching all the basic verbs", func(t *testing.T) { - // Create an RBAC rule to allow this user to read/write everything. library.CreateTestClusterRoleBinding(t, rbacv1.Subject{Kind: rbacv1.UserKind, APIGroup: rbacv1.GroupName, Name: env.TestUser.ExpectedUsername}, @@ -214,7 +274,7 @@ func TestImpersonationProxy(t *testing.T) { // Create and start informer to exercise the "watch" verb for us. informerFactory := k8sinformers.NewSharedInformerFactoryWithOptions( - impersonationProxyClient, + impersonationProxyClient(), 0, k8sinformers.WithNamespace(namespace.Name)) informer := informerFactory.Core().V1().ConfigMaps() @@ -234,17 +294,17 @@ func TestImpersonationProxy(t *testing.T) { } // Test "create" verb through the impersonation proxy. - _, err = impersonationProxyClient.CoreV1().ConfigMaps(namespace.Name).Create(ctx, + _, err = impersonationProxyClient().CoreV1().ConfigMaps(namespace.Name).Create(ctx, &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "configmap-1", Labels: configMapLabels}}, metav1.CreateOptions{}, ) require.NoError(t, err) - _, err = impersonationProxyClient.CoreV1().ConfigMaps(namespace.Name).Create(ctx, + _, err = impersonationProxyClient().CoreV1().ConfigMaps(namespace.Name).Create(ctx, &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "configmap-2", Labels: configMapLabels}}, metav1.CreateOptions{}, ) require.NoError(t, err) - _, err = impersonationProxyClient.CoreV1().ConfigMaps(namespace.Name).Create(ctx, + _, err = impersonationProxyClient().CoreV1().ConfigMaps(namespace.Name).Create(ctx, &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "configmap-3", Labels: configMapLabels}}, metav1.CreateOptions{}, ) @@ -260,11 +320,11 @@ func TestImpersonationProxy(t *testing.T) { }, 10*time.Second, 50*time.Millisecond) // Test "get" verb through the impersonation proxy. - configMap3, err := impersonationProxyClient.CoreV1().ConfigMaps(namespace.Name).Get(ctx, "configmap-3", metav1.GetOptions{}) + configMap3, err := impersonationProxyClient().CoreV1().ConfigMaps(namespace.Name).Get(ctx, "configmap-3", metav1.GetOptions{}) require.NoError(t, err) // Test "list" verb through the impersonation proxy. - listResult, err := impersonationProxyClient.CoreV1().ConfigMaps(namespace.Name).List(ctx, metav1.ListOptions{ + listResult, err := impersonationProxyClient().CoreV1().ConfigMaps(namespace.Name).List(ctx, metav1.ListOptions{ LabelSelector: configMapLabels.String(), }) require.NoError(t, err) @@ -272,7 +332,7 @@ func TestImpersonationProxy(t *testing.T) { // Test "update" verb through the impersonation proxy. configMap3.Data = map[string]string{"foo": "bar"} - updateResult, err := impersonationProxyClient.CoreV1().ConfigMaps(namespace.Name).Update(ctx, configMap3, metav1.UpdateOptions{}) + updateResult, err := impersonationProxyClient().CoreV1().ConfigMaps(namespace.Name).Update(ctx, configMap3, metav1.UpdateOptions{}) require.NoError(t, err) require.Equal(t, "bar", updateResult.Data["foo"]) @@ -283,7 +343,7 @@ func TestImpersonationProxy(t *testing.T) { }, 10*time.Second, 50*time.Millisecond) // Test "patch" verb through the impersonation proxy. - patchResult, err := impersonationProxyClient.CoreV1().ConfigMaps(namespace.Name).Patch(ctx, + patchResult, err := impersonationProxyClient().CoreV1().ConfigMaps(namespace.Name).Patch(ctx, "configmap-3", types.MergePatchType, []byte(`{"data":{"baz":"42"}}`), @@ -300,7 +360,7 @@ func TestImpersonationProxy(t *testing.T) { }, 10*time.Second, 50*time.Millisecond) // Test "delete" verb through the impersonation proxy. - err = impersonationProxyClient.CoreV1().ConfigMaps(namespace.Name).Delete(ctx, "configmap-3", metav1.DeleteOptions{}) + err = impersonationProxyClient().CoreV1().ConfigMaps(namespace.Name).Delete(ctx, "configmap-3", metav1.DeleteOptions{}) require.NoError(t, err) // Make sure that the deleted ConfigMap shows up in the informer's cache. @@ -311,7 +371,7 @@ func TestImpersonationProxy(t *testing.T) { }, 10*time.Second, 50*time.Millisecond) // Test "deletecollection" verb through the impersonation proxy. - err = impersonationProxyClient.CoreV1().ConfigMaps(namespace.Name).DeleteCollection(ctx, metav1.DeleteOptions{}, metav1.ListOptions{}) + err = impersonationProxyClient().CoreV1().ConfigMaps(namespace.Name).DeleteCollection(ctx, metav1.DeleteOptions{}, metav1.ListOptions{}) require.NoError(t, err) // Make sure that the deleted ConfigMaps shows up in the informer's cache. @@ -321,7 +381,7 @@ func TestImpersonationProxy(t *testing.T) { }, 10*time.Second, 50*time.Millisecond) // There should be no ConfigMaps left. - listResult, err = impersonationProxyClient.CoreV1().ConfigMaps(namespace.Name).List(ctx, metav1.ListOptions{ + listResult, err = impersonationProxyClient().CoreV1().ConfigMaps(namespace.Name).List(ctx, metav1.ListOptions{ LabelSelector: configMapLabels.String(), }) require.NoError(t, err) @@ -341,24 +401,22 @@ func TestImpersonationProxy(t *testing.T) { // Make a client which will send requests through the impersonation proxy and will also add // impersonate headers to the request. - var doubleImpersonationClient kubernetes.Interface - if env.HasCapability(library.HasExternalLoadBalancerProvider) { - doubleImpersonationClient = impersonationProxyViaLoadBalancerClient(impersonationProxyURL, impersonationProxyCACertPEM, "other-user-to-impersonate") - } else { - doubleImpersonationClient = impersonationProxyViaSquidClient(impersonationProxyCACertPEM, "other-user-to-impersonate") - } + doubleImpersonationClient := newImpersonationProxyClient(impersonationProxyURL, impersonationProxyCACertPEM, "other-user-to-impersonate") // Check that we can get some resource through the impersonation proxy without any impersonation headers on the request. // We could use any resource for this, but we happen to know that this one should exist. - _, err = impersonationProxyClient.CoreV1().Secrets(env.ConciergeNamespace).Get(ctx, impersonationProxyTLSSecretName(env), metav1.GetOptions{}) + _, err = impersonationProxyClient().CoreV1().Secrets(env.ConciergeNamespace).Get(ctx, impersonationProxyTLSSecretName(env), metav1.GetOptions{}) require.NoError(t, err) // Now we'll see what happens when we add an impersonation header to the request. This should generate a // request similar to the one above, except that it will have an impersonation header. _, err = doubleImpersonationClient.CoreV1().Secrets(env.ConciergeNamespace).Get(ctx, impersonationProxyTLSSecretName(env), metav1.GetOptions{}) // Double impersonation is not supported yet, so we should get an error. - expectedErr := fmt.Sprintf("the server rejected our request for an unknown reason (get secrets %s)", impersonationProxyTLSSecretName(env)) - require.EqualError(t, err, expectedErr) + require.EqualError(t, err, fmt.Sprintf( + `users "other-user-to-impersonate" is forbidden: `+ + `User "%s" cannot impersonate resource "users" in API group "" at the cluster scope: `+ + `impersonation is not allowed or invalid verb`, + env.TestUser.ExpectedUsername)) }) t.Run("kubectl as a client", func(t *testing.T) { @@ -495,7 +553,8 @@ func TestImpersonationProxy(t *testing.T) { rootCAs := x509.NewCertPool() rootCAs.AppendCertsFromPEM(impersonationProxyCACertPEM) tlsConfig := &tls.Config{ - RootCAs: rootCAs, + MinVersion: tls.VersionTLS12, + RootCAs: rootCAs, } websocketConfig := websocket.Config{ @@ -563,7 +622,7 @@ func TestImpersonationProxy(t *testing.T) { require.Eventually(t, func() bool { // It's okay if this returns RBAC errors because this user has no role bindings. // What we want to see is that the proxy eventually shuts down entirely. - _, err = impersonationProxyViaSquidClient(nil, "").CoreV1().Namespaces().List(ctx, metav1.ListOptions{}) + _, err = impersonationProxyViaSquidClient(expectedProxyServiceEndpointURL, nil, "").CoreV1().Namespaces().List(ctx, metav1.ListOptions{}) return err.Error() == serviceUnavailableViaSquidError }, 20*time.Second, 500*time.Millisecond) } @@ -705,7 +764,7 @@ func credentialIssuerName(env *library.TestEnv) string { return env.ConciergeAppName + "-config" } -// watchJSON defines the expected JSON wire equivalent of watch.Event +// watchJSON defines the expected JSON wire equivalent of watch.Event. type watchJSON struct { Type watch.EventType `json:"type,omitempty"` Object json.RawMessage `json:"object,omitempty"` diff --git a/test/library/credential_request.go b/test/library/credential_request.go new file mode 100644 index 000000000..44aeaff2a --- /dev/null +++ b/test/library/credential_request.go @@ -0,0 +1,27 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package library + +import ( + "context" + "testing" + "time" + + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "go.pinniped.dev/generated/latest/apis/concierge/login/v1alpha1" +) + +func CreateTokenCredentialRequest(ctx context.Context, t *testing.T, spec v1alpha1.TokenCredentialRequestSpec) (*v1alpha1.TokenCredentialRequest, error) { + t.Helper() + + client := NewAnonymousConciergeClientset(t) + + ctx, cancel := context.WithTimeout(ctx, time.Minute) + defer cancel() + + return client.LoginV1alpha1().TokenCredentialRequests().Create(ctx, + &v1alpha1.TokenCredentialRequest{Spec: spec}, v1.CreateOptions{}, + ) +}