From 0b300cbe4263293b6d1e7b207baef22f02919c58 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 10 Mar 2021 10:30:06 -0800 Subject: [PATCH] Use TokenCredentialRequest instead of base64 token with impersonator To make an impersonation request, first make a TokenCredentialRequest to get a certificate. That cert will either be issued by the Kube API server's CA or by a new CA specific to the impersonator. Either way, you can then make a request to the impersonator and present that client cert for auth and the impersonator will accept it and make the impesonation call on your behalf. The impersonator http handler now borrows some Kube library code to handle request processing. This will allow us to more closely mimic the behavior of a real API server, e.g. the client cert auth will work exactly like the real API server. Signed-off-by: Monis Khan --- deploy/concierge/deployment.yaml | 1 + go.mod | 2 +- internal/certauthority/certauthority.go | 11 +- .../dynamiccertauthority.go | 9 +- internal/concierge/apiserver/apiserver.go | 3 +- .../concierge/impersonator/impersonator.go | 380 ++++++----- .../impersonator/impersonator_test.go | 417 +++++------- internal/concierge/server/server.go | 54 +- internal/config/concierge/config.go | 3 + internal/config/concierge/config_test.go | 35 +- internal/config/concierge/types.go | 1 + .../controller/apicerts/apiservice_updater.go | 4 +- internal/controller/apicerts/certs_expirer.go | 18 +- .../controller/apicerts/certs_expirer_test.go | 22 +- internal/controller/apicerts/certs_manager.go | 52 +- .../controller/apicerts/certs_manager_test.go | 46 +- .../controller/apicerts/certs_observer.go | 4 +- .../impersonatorconfig/impersonator_config.go | 255 ++++--- .../impersonator_config_test.go | 633 ++++++++++++++---- .../controllermanager/prepare_controllers.go | 54 +- internal/dynamiccert/provider.go | 29 +- internal/issuer/issuer.go | 42 ++ internal/registry/credentialrequest/rest.go | 9 +- .../registry/credentialrequest/rest_test.go | 9 +- .../impersonationtoken/impersonationtoken.go | 65 -- .../concierge_credentialrequest_test.go | 51 +- .../concierge_impersonation_proxy_test.go | 151 +++-- test/library/credential_request.go | 27 + 28 files changed, 1486 insertions(+), 901 deletions(-) create mode 100644 internal/issuer/issuer.go delete mode 100644 internal/testutil/impersonationtoken/impersonationtoken.go create mode 100644 test/library/credential_request.go diff --git a/deploy/concierge/deployment.yaml b/deploy/concierge/deployment.yaml index 41119243..223b7fa8 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 0d15fb2a..de809464 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 64fa2ecb..806d6498 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 a2c97898..c15e4cc0 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 e5fc8da9..f64e0104 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 bbbcda39..f943db80 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 7a7124b2..31e9aae9 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 cc623393..d485ee19 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 b6720cea..b4d4c9a5 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 5b8177ff..0034d06b 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 2a151274..cfec621e 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 4e28e519..3cfe4fcc 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 336f619f..2b643c3e 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 f8f16595..7e82819a 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 22f0f6df..f0c11d0d 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 f70f8500..b32b9228 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 7f05d243..c6380741 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 c1e491aa..8d741a4f 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 65ea68ff..66cab3e7 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 d727a1bc..5af528e4 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 a4ca6ad5..77687fb1 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 00000000..94e2f235 --- /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 a8906676..a1846a40 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 8542b99e..6b71ec20 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 64b65378..00000000 --- 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 de48bf85..196670d4 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 06f0a55d..a48e3203 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 00000000..44aeaff2 --- /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{}, + ) +}