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 <mok@vmware.com>

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-") @)

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

internal/certauthority/certauthority.go

@@ -192,6 +192,7 @@ func (c *CA) Issue(subject pkix.Name, dnsNames []string, ips []net.IP, ttl time.
 		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,

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,
 	}

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

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/apimachinery/pkg/runtime/serializer"
-	"k8s.io/apiserver/pkg/authentication/request/bearertoken"
+	"k8s.io/apimachinery/pkg/util/sets"
-	"k8s.io/apiserver/pkg/authentication/user"
+	"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 {
+// FactoryFunc is a function which can create an impersonator server.
-	cache       *authncache.Cache
+// It returns a function which will start the impersonator server.
-	jsonDecoder runtime.Decoder
+// That start function takes a stopCh which can be used to stop the server.
-	proxy       *httputil.ReverseProxy
+// Once a server has been stopped, don't start it again using the start function.
-	log         logr.Logger
+// 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) {
+func newInternal( //nolint:funlen // yeah, it's kind of long.
-	return newInternal(cache, jsonDecoder, log, func() (*rest.Config, error) {
+	port int,
-		client, err := kubeclient.New()
+	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)
-	})
-}
-
-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)
+			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)
+
+		if recOpts != nil {
+			recOpts(recommendedOptions)
 		}
 
-	serverURL, err := url.Parse(kubeconfig.Host)
+		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
+	}
+
+	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)
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-	reverseProxy.Transport = kubeRoundTripper
+		// TODO integration test using a bearer token
-	reverseProxy.FlushInterval = 200 * time.Millisecond // the "watch" verb will not work without this line
+		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",
-	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,
 			)
+			http.Error(w, "invalid authorization header", http.StatusInternalServerError)
+			return
+		}
 
 		if err := ensureNoImpersonationHeaders(r); err != nil {
-		log.Error(err, "impersonation header already exists")
+			plog.Error("noImpersonationAuthorizer logic did not prevent nested impersonation but it is always supposed to do so",
-		http.Error(w, "impersonation header already exists", http.StatusBadRequest)
+				err,
+				"url", r.URL.String(),
+				"method", r.Method,
+			)
+			http.Error(w, "invalid impersonation", http.StatusInternalServerError)
 			return
 		}
 
-	// Never mutate request (see http.Handler docs).
+		userInfo, ok := request.UserFrom(r.Context())
-	newR := r.Clone(r.Context())
+		if !ok {
-
+			plog.Warning("aggregated API server logic did not set user info but it is always supposed to do so",
-	authentication, authenticated, err := bearertoken.New(authenticator.TokenFunc(func(ctx context.Context, token string) (*authenticator.Response, bool, error) {
+				"url", r.URL.String(),
-		tokenCredentialReq, err := extractToken(token, p.jsonDecoder)
+				"method", r.Method,
-		if err != nil {
+			)
-			log.Error(err, "invalid token encoding")
+			http.Error(w, "invalid user", http.StatusInternalServerError)
-			return nil, false, &httpError{message: "invalid token encoding", code: http.StatusBadRequest}
+			return
 		}
 
-		log = log.WithValues(
+		if len(userInfo.GetUID()) > 0 {
-			"authenticator", tokenCredentialReq.Spec.Authenticator,
+			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)
+		reverseProxy := httputil.NewSingleHostReverseProxy(serverURL)
-		if err != nil {
-			log.Error(err, "received invalid token")
-			return nil, false, &httpError{message: "invalid token", code: http.StatusUnauthorized}
-		}
-		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)
-}
-
-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) {
-			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) {
+		reverseProxy.Transport = transport.NewImpersonatingRoundTripper(impersonateConfig, kubeRoundTripper)
-		for headerKey, headerValues := range r.Header {
+		reverseProxy.FlushInterval = 200 * time.Millisecond // the "watch" verb will not work without this line
-			if isImpersonationHeader(headerKey) {
+		// transport.NewImpersonatingRoundTripper clones the request before setting headers
-				for _, headerValue := range headerValues {
+		// so this call will not accidentally mutate the input request (see http.Handler docs)
-					newHeaders.Add(headerKey, headerValue)
+		reverseProxy.ServeHTTP(w, r)
-				}
+	}), nil
-			}
-		}
-		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 {
+func ensureNoImpersonationHeaders(r *http.Request) error {
-	return strings.HasPrefix(http.CanonicalHeaderKey(header), "Impersonate")
+	for key := range r.Header {
+		if strings.HasPrefix(key, "Impersonate") {
+			return fmt.Errorf("%q header already exists", key)
+		}
 	}
 
-func extractToken(token string, jsonDecoder runtime.Decoder) (*login.TokenCredentialRequest, error) {
+	return nil
-	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
 }

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/internal/certauthority"
-	"go.pinniped.dev/generated/latest/apis/concierge/login"
+	"go.pinniped.dev/internal/kubeclient"
-	conciergescheme "go.pinniped.dev/internal/concierge/scheme"
-	"go.pinniped.dev/internal/controller/authenticator/authncache"
-	"go.pinniped.dev/internal/mocks/mocktokenauthenticator"
 	"go.pinniped.dev/internal/testutil"
-	"go.pinniped.dev/internal/testutil/impersonationtoken"
-	"go.pinniped.dev/internal/testutil/testlogger"
 )
 
+func TestNew(t *testing.T) {
+	const port = 8444
+
+	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 (
+	const testUser = "test-user"
-		defaultAPIGroup = "pinniped.dev"
-		customAPIGroup  = "walrus.tld"
-
-		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 {
+	newRequest := func(h http.Header, userInfo user.Info) *http.Request {
-		r, err := http.NewRequestWithContext(context.Background(), http.MethodGet, validURL.String(), nil)
+		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
+		restConfig                      *rest.Config
-		getKubeconfig                   func() (*rest.Config, error)
 		wantCreationErr                 string
 		request                         *http.Request
 		wantHTTPBody                    string
 		wantHTTPStatus                  int
-		wantLogs                        []string
 		wantKubeAPIServerRequestHeaders http.Header
-		wantKubeAPIServerStatusCode     int
+		kubeAPIServerStatusCode         int
-		expectMockToken                 func(*testing.T, *mocktokenauthenticator.MockTokenMockRecorder)
 	}{
-		{
-			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) {
+			restConfig:      &rest.Config{Host: ":"},
-				return &rest.Config{Host: ":"}, nil
-			},
 			wantCreationErr: "could not parse host URL from in-cluster config: parse \":\": missing protocol scheme",
 		},
 		{
 			name: "invalid transport config",
-			getKubeconfig: func() (*rest.Config, error) {
+			restConfig: &rest.Config{
-				return &rest.Config{
 				Host:         "pinniped.dev/blah",
 				ExecProvider: &api.ExecConfig{},
 				AuthProvider: &api.AuthProviderConfig{},
-				}, nil
 			},
 			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) {
+			restConfig: &rest.Config{
-				return &rest.Config{
 				Host:            "pinniped.dev/blah",
 				BearerToken:     "test-bearer-token",
 				Transport:       http.DefaultTransport,
 				TLSClientConfig: rest.TLSClientConfig{Insecure: true},
-				}, nil
 			},
 			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"}}),
+			request:        newRequest(map[string][]string{"Impersonate-User": {"some-user"}}, nil),
-			wantHTTPBody:   "impersonation header already exists\n",
+			wantHTTPBody:   "invalid impersonation\n",
-			wantHTTPStatus: http.StatusBadRequest,
+			wantHTTPStatus: http.StatusInternalServerError,
-			wantLogs:       []string{"\"msg\"=\"impersonation header already exists\" \"error\"=\"\\\"Impersonate-User\\\" header already exists\" \"method\"=\"GET\" \"url\"=\"http://pinniped.dev/blah\""},
 		},
 		{
 			name:           "Impersonate-Group header already in request",
-			request:        newRequest(map[string][]string{"Impersonate-Group": {"some-group"}}),
+			request:        newRequest(map[string][]string{"Impersonate-Group": {"some-group"}}, nil),
-			wantHTTPBody:   "impersonation header already exists\n",
+			wantHTTPBody:   "invalid impersonation\n",
-			wantHTTPStatus: http.StatusBadRequest,
+			wantHTTPStatus: http.StatusInternalServerError,
-			wantLogs:       []string{"\"msg\"=\"impersonation header already exists\" \"error\"=\"\\\"Impersonate-Group\\\" header already exists\" \"method\"=\"GET\" \"url\"=\"http://pinniped.dev/blah\""},
 		},
 		{
 			name:           "Impersonate-Extra header already in request",
-			request:        newRequest(map[string][]string{"Impersonate-Extra-something": {"something"}}),
+			request:        newRequest(map[string][]string{"Impersonate-Extra-something": {"something"}}, nil),
-			wantHTTPBody:   "impersonation header already exists\n",
+			wantHTTPBody:   "invalid impersonation\n",
-			wantHTTPStatus: http.StatusBadRequest,
+			wantHTTPStatus: http.StatusInternalServerError,
-			wantLogs:       []string{"\"msg\"=\"impersonation header already exists\" \"error\"=\"\\\"Impersonate-Extra-something\\\" header already exists\" \"method\"=\"GET\" \"url\"=\"http://pinniped.dev/blah\""},
 		},
 		{
 			name:           "Impersonate-* header already in request",
-			request: newRequest(map[string][]string{
+			request:        newRequest(map[string][]string{"Impersonate-Something": {"some-newfangled-impersonate-header"}}, nil),
-				"Impersonate-Something": {"some-newfangled-impersonate-header"},
+			wantHTTPBody:   "invalid impersonation\n",
-			}),
+			wantHTTPStatus: http.StatusInternalServerError,
-			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:           "missing authorization header",
+			name:           "unexpected authorization header",
-			request:        newRequest(map[string][]string{}),
+			request:        newRequest(map[string][]string{"Authorization": {"panda"}}, nil),
-			wantHTTPBody:   "invalid token encoding\n",
+			wantHTTPBody:   "invalid authorization header\n",
-			wantHTTPStatus: http.StatusBadRequest,
+			wantHTTPStatus: http.StatusInternalServerError,
-			wantLogs:       []string{"\"msg\"=\"invalid token encoding\" \"error\"=\"token authenticator did not find token\" \"method\"=\"GET\" \"url\"=\"http://pinniped.dev/blah\""},
 		},
 		{
-			name:           "authorization header missing bearer prefix",
+			name:           "missing user",
-			request:        newRequest(map[string][]string{"Authorization": {impersonationtoken.Make(t, "test-token", &goodAuthenticator, defaultAPIGroup)}}),
+			request:        newRequest(map[string][]string{}, nil),
-			wantHTTPBody:   "invalid token encoding\n",
+			wantHTTPBody:   "invalid user\n",
-			wantHTTPStatus: http.StatusBadRequest,
+			wantHTTPStatus: http.StatusInternalServerError,
-			wantLogs:       []string{"\"msg\"=\"invalid token encoding\" \"error\"=\"token authenticator did not find token\" \"method\"=\"GET\" \"url\"=\"http://pinniped.dev/blah\""},
 		},
 		{
-			name:           "token is not base64 encoded",
+			name:           "unexpected UID",
-			request:        newRequest(map[string][]string{"Authorization": {"Bearer !!!"}}),
+			request:        newRequest(map[string][]string{}, &user.DefaultInfo{UID: "007"}),
-			wantHTTPBody:   "invalid token encoding\n",
+			wantHTTPBody:   "unexpected uid\n",
-			wantHTTPStatus: http.StatusBadRequest,
+			wantHTTPStatus: http.StatusUnprocessableEntity,
-			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\""},
 		},
 		// 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{
-			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.DefaultInfo{
-			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}
+			kubeAPIServerStatusCode: http.StatusNotFound,
-				recorder.AuthenticateToken(gomock.Any(), "test-token").Return(response, true, nil)
-			},
-			wantKubeAPIServerStatusCode: 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 tt.kubeAPIServerStatusCode == 0 {
-				if t.Failed() {
+				tt.kubeAPIServerStatusCode = http.StatusOK
-					for i, line := range testLog.Lines() {
-						t.Logf("testLog line %d: %q", i+1, line)
-					}
-				}
-			}()
-
-			if tt.wantKubeAPIServerStatusCode == 0 {
-				tt.wantKubeAPIServerStatusCode = 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 {
+				if tt.kubeAPIServerStatusCode != http.StatusOK {
-					w.WriteHeader(tt.wantKubeAPIServerStatusCode)
+					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 {
+			if tt.restConfig == nil {
-				tt.getKubeconfig = func() (*rest.Config, error) {
+				tt.restConfig = &testServerKubeconfig
-					return &testServerKubeconfig, nil
-				}
 			}
 
-			// stole this from cache_test, hopefully it is sufficient
+			proxy, err := newImpersonationReverseProxy(tt.restConfig)
-			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)
 			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,
-	})
-}

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)
@@ -136,21 +140,26 @@ func (a *App) runServer(ctx context.Context) error {
 			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,
-			LoginJSONDecoder:           getLoginJSONDecoder(loginGV.Group, scheme),
 		},
 	)
 	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,
-	})
-}

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, ", "))
 	}

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])?)*')",
 		},

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

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)
 	}
 

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,
+		return fmt.Errorf("failed to get cert bounds for secret %q with key %q: %w", secret.Name, c.secretKey, err)
-		// 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
 	}
 
 	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) {
+func (c *certsExpirerController) getCertBounds(secret *corev1.Secret) (time.Time, time.Time, error) {
-	certPEM := secret.Data[tlsCertificateChainSecretKey]
+	certPEM := secret.Data[c.secretKey]
 	if certPEM == nil {
 		return time.Time{}, time.Time{}, constable.Error("failed to find certificate")
 	}

internal/controller/apicerts/certs_expirer_test.go

@@ -99,6 +99,7 @@ func TestExpirerControllerFilters(t *testing.T) {
 				secretsInformer,
 				withInformer.WithInformer,
 				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[fakeTestKey] = certPEM
-				m["tlsCertificateChain"] = certPEM
 			},
 			wantDelete: false,
 		},
@@ -158,8 +160,7 @@ func TestExpirerControllerSync(t *testing.T) {
 				)
 				require.NoError(t, err)
 
-				// See certs_manager.go for this constant.
+				m[fakeTestKey] = certPEM
-				m["tlsCertificateChain"] = certPEM
 			},
 			wantDelete: true,
 		},
@@ -173,8 +174,7 @@ func TestExpirerControllerSync(t *testing.T) {
 				)
 				require.NoError(t, err)
 
-				// See certs_manager.go for this constant.
+				m[fakeTestKey] = certPEM
-				m["tlsCertificateChain"] = certPEM
 			},
 			wantDelete: true,
 		},
@@ -188,8 +188,7 @@ func TestExpirerControllerSync(t *testing.T) {
 				)
 				require.NoError(t, err)
 
-				// See certs_manager.go for this constant.
+				m[fakeTestKey] = certPEM
-				m["tlsCertificateChain"] = 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[fakeTestKey], err = x509.MarshalPKCS8PrivateKey(privateKey)
-				m["tlsCertificateChain"], 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().

internal/controller/apicerts/certs_manager.go

@@ -21,9 +21,10 @@ import (
 )
 
 const (
-	caCertificateSecretKey       = "caCertificate"
+	CACertificateSecretKey           = "caCertificate"
+	CACertificatePrivateKeySecretKey = "caCertificatePrivateKey"
 	tlsPrivateKeySecretKey           = "tlsPrivateKey"
-	tlsCertificateChainSecretKey = "tlsCertificateChain"
+	TLSCertificateChainSecretKey     = "tlsCertificateChain"
 )
 
 type certsManagerController struct {
@@ -98,7 +99,26 @@ 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.
+	caPrivateKeyPEM, err := ca.PrivateKeyToPEM()
+	if err != nil {
+		return fmt.Errorf("could not get CA private key: %w", err)
+	}
+
+	secret := corev1.Secret{
+		TypeMeta: metav1.TypeMeta{},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      c.certsSecretResourceName,
+			Namespace: c.namespace,
+			Labels:    c.certsSecretLabels,
+		},
+		StringData: map[string]string{
+			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},
@@ -115,19 +135,11 @@ func (c *certsManagerController) Sync(ctx controllerlib.Context) error {
 		if err != nil {
 			return fmt.Errorf("could not PEM encode serving certificate: %w", err)
 		}
-	secret := corev1.Secret{
+
-		TypeMeta: metav1.TypeMeta{},
+		secret.StringData[tlsPrivateKeySecretKey] = string(tlsPrivateKeyPEM)
-		ObjectMeta: metav1.ObjectMeta{
+		secret.StringData[TLSCertificateChainSecretKey] = string(tlsCertChainPEM)
-			Name:      c.certsSecretResourceName,
-			Namespace: c.namespace,
-			Labels:    c.certsSecretLabels,
-		},
-		StringData: map[string]string{
-			caCertificateSecretKey:       string(ca.Bundle()),
-			tlsPrivateKeySecretKey:       string(tlsPrivateKeyPEM),
-			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)

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())

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
 }

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
@@ -64,17 +67,15 @@ type impersonatorConfigController struct {
 
 	labels                           map[string]string
 	clock                            clock.Clock
-	startTLSListenerFunc StartTLSListenerFunc
+	impersonationSigningCertProvider dynamiccert.Provider
-	httpHandlerFactory   func() (http.Handler, error)
+	impersonatorFunc                 impersonator.FactoryFunc
 
-	server               *http.Server
 	hasControlPlaneNodes              *bool
-	tlsCert              *tls.Certificate // always read/write using tlsCertMutex
+	serverStopCh                      chan struct{}
-	tlsCertMutex         sync.RWMutex
+	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,9 +92,11 @@ func NewImpersonatorConfigController(
 	caSecretName string,
 	labels map[string]string,
 	clock clock.Clock,
-	startTLSListenerFunc StartTLSListenerFunc,
+	impersonatorFunc impersonator.FactoryFunc,
-	httpHandlerFactory func() (http.Handler, error),
+	impersonationSignerSecretName string,
+	impersonationSigningCertProvider dynamiccert.Provider,
 ) controllerlib.Controller {
+	secretNames := sets.NewString(tlsSecretName, caSecretName, impersonationSignerSecretName)
 	return controllerlib.New(
 		controllerlib.Config{
 			Name: "impersonator-config-controller",
@@ -104,6 +107,7 @@ func NewImpersonatorConfigController(
 				generatedLoadBalancerServiceName:  generatedLoadBalancerServiceName,
 				tlsSecretName:                     tlsSecretName,
 				caSecretName:                      caSecretName,
+				impersonationSignerSecretName:     impersonationSignerSecretName,
 				k8sClient:                         k8sClient,
 				pinnipedAPIClient:                 pinnipedAPIClient,
 				configMapsInformer:                configMapsInformer,
@@ -111,8 +115,9 @@ func NewImpersonatorConfigController(
 				secretsInformer:                   secretsInformer,
 				labels:                            labels,
 				clock:                             clock,
-				startTLSListenerFunc:             startTLSListenerFunc,
+				impersonationSigningCertProvider:  impersonationSigningCertProvider,
-				httpHandlerFactory:               httpHandlerFactory,
+				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 {
+		if err = c.ensureImpersonatorIsStopped(true); err != nil {
-			return nil, err
+			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,52 +343,75 @@ func (c *impersonatorConfigController) tlsSecretExists() (bool, *v1.Secret, erro
 	return true, secret, nil
 }
 
-func (c *impersonatorConfigController) ensureImpersonatorIsStarted() error {
+func (c *impersonatorConfigController) ensureImpersonatorIsStarted(syncCtx controllerlib.Context) error {
-	if c.server != nil {
+	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
 		}
-
-	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)
+	startImpersonatorFunc, err := c.impersonatorFunc(
-		if errors.Is(err, http.ErrServerClosed) {
+		impersonationProxyPort,
-			plog.Info("The impersonation proxy server has shut down")
+		c.tlsServingCertDynamicCertProvider,
-		} else {
+		dynamiccert.NewCAProvider(c.impersonationSigningCertProvider),
-			plog.Error("Unexpected shutdown of the impersonation proxy server", err)
+	)
-		}
-	}()
-	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
 	}
-	}
+
+	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(shouldCloseErrChan bool) error {
+	if c.serverStopCh == 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 {
 	running, err := c.loadBalancerExists()
 	if err != nil {
@@ -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
-}

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,
+				impersonator.New,
-				func() (http.Handler, error) {
+				c.NamesConfig.ImpersonationSignerSecret,
-					impersonationProxyHandler, err := impersonator.New(
+				c.ImpersonationSigningCertProvider,
-						c.AuthenticatorCache,
+			),
-						c.LoginJSONDecoder,
+			singletonWorker,
-						klogr.New().WithName("impersonation-proxy"),
+		).
-					)
+		WithController(
-					if err != nil {
+			apicerts.NewCertsManagerController(
-						return nil, fmt.Errorf("could not create impersonation proxy: %w", err)
+				c.ServerInstallationInfo.Namespace,
-					}
+				c.NamesConfig.ImpersonationSignerSecret,
-					return impersonationProxyHandler, nil
+				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,
 		)

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

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
+}

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
 }
 

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 {
+func successfulIssuer(ctrl *gomock.Controller) issuer.CertIssuer {
-	issuer := credentialrequestmocks.NewMockCertIssuer(ctrl)
+	certIssuer := credentialrequestmocks.NewMockCertIssuer(ctrl)
-	issuer.EXPECT().
+	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 {

internal/testutil/impersonationtoken/impersonationtoken.go

@@ -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)
-}

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{
+	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{
+				response, err = library.CreateTokenCredentialRequest(ctx, t,
-					Token:         token,
+					loginv1alpha1.TokenCredentialRequestSpec{Token: token, Authenticator: authenticator},
-					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{
+	response, err := library.CreateTokenCredentialRequest(context.Background(), t,
-		Token:         "not a good token",
+		loginv1alpha1.TokenCredentialRequestSpec{Token: "not a good token", Authenticator: testWebhook},
-		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{
+	response, err := library.CreateTokenCredentialRequest(context.Background(), t,
-		Token:         "",
+		loginv1alpha1.TokenCredentialRequestSpec{Token: "", Authenticator: testWebhook},
-		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,

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},
+			TLSClientConfig: rest.TLSClientConfig{
-			BearerToken:     impersonationtoken.Make(t, env.TestUser.Token, &authenticator, env.APIGroupSuffix),
+				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)
-
+	if !env.HasCapability(library.HasExternalLoadBalancerProvider) {
-	// 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 {
 		// 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
+		doubleImpersonationClient := newImpersonationProxyClient(impersonationProxyURL, impersonationProxyCACertPEM, "other-user-to-impersonate")
-		if env.HasCapability(library.HasExternalLoadBalancerProvider) {
-			doubleImpersonationClient = impersonationProxyViaLoadBalancerClient(impersonationProxyURL, impersonationProxyCACertPEM, "other-user-to-impersonate")
-		} else {
-			doubleImpersonationClient = impersonationProxyViaSquidClient(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, fmt.Sprintf(
-		require.EqualError(t, err, expectedErr)
+			`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,6 +553,7 @@ func TestImpersonationProxy(t *testing.T) {
 		rootCAs := x509.NewCertPool()
 		rootCAs.AppendCertsFromPEM(impersonationProxyCACertPEM)
 		tlsConfig := &tls.Config{
+			MinVersion: tls.VersionTLS12,
 			RootCAs:    rootCAs,
 		}
 
@@ -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"`

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{},
+	)
+}