Merge pull request #1549 from vmware-tanzu/jtc/tiny-fixups-to-support…

…-1548

Tiny fixups to support #1548

internal/certauthority/certauthority.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package certauthority implements a simple x509 certificate authority suitable for use in an aggregated API service.
@@ -179,13 +179,13 @@ func (c *CA) IssueServerCert(dnsNames []string, ips []net.IP, ttl time.Duration)
 	return c.issueCert(x509.ExtKeyUsageServerAuth, pkix.Name{}, dnsNames, ips, ttl)
 }
 
-// Similar to IssueClientCert, but returning the new cert as a pair of PEM-formatted byte slices
+// IssueClientCertPEM is similar to IssueClientCert, but returns the new cert as a pair of PEM-formatted byte slices
 // for the certificate and private key.
 func (c *CA) IssueClientCertPEM(username string, groups []string, ttl time.Duration) ([]byte, []byte, error) {
 	return toPEM(c.IssueClientCert(username, groups, ttl))
 }
 
-// Similar to IssueServerCert, but returning the new cert as a pair of PEM-formatted byte slices
+// IssueServerCertPEM is similar to IssueServerCert, but returns the new cert as a pair of PEM-formatted byte slices
 // for the certificate and private key.
 func (c *CA) IssueServerCertPEM(dnsNames []string, ips []net.IP, ttl time.Duration) ([]byte, []byte, error) {
 	return toPEM(c.IssueServerCert(dnsNames, ips, ttl))
@@ -260,7 +260,7 @@ func toPEM(cert *tls.Certificate, err error) ([]byte, []byte, error) {
 	return certPEM, keyPEM, nil
 }
 
-// Encode a tls.Certificate into a private key PEM and a cert chain PEM.
+// ToPEM encodes a tls.Certificate into a private key PEM and a cert chain PEM.
 func ToPEM(cert *tls.Certificate) ([]byte, []byte, error) {
 	// Encode the certificate(s) to PEM.
 	certPEMBlocks := make([][]byte, 0, len(cert.Certificate))

internal/certauthority/certauthority_test.go

@@ -7,10 +7,10 @@ import (
 	"crypto"
 	"crypto/tls"
 	"crypto/x509"
+	_ "embed"
 	"fmt"
 	"io"
 	"net"
-	"os"
 	"strings"
 	"testing"
 	"time"
@@ -20,60 +20,65 @@ import (
 	"go.pinniped.dev/internal/testutil"
 )
 
-func loadFromFiles(t *testing.T, certPath string, keyPath string) (*CA, error) {
-	t.Helper()
-
-	certPEM, err := os.ReadFile(certPath)
-	require.NoError(t, err)
-
-	keyPEM, err := os.ReadFile(keyPath)
-	require.NoError(t, err)
-
-	ca, err := Load(string(certPEM), string(keyPEM))
-	return ca, err
-}
+var (
+	//go:embed testdata/empty
+	empty string
+	//go:embed testdata/invalid
+	invalid string
+	//go:embed testdata/multiple.crt
+	multiple string
+	//go:embed testdata/test.crt
+	testCert string
+	//go:embed testdata/test.key
+	testKey string
+	//go:embed testdata/test2.key
+	testKey2 string
+)
 
 func TestLoad(t *testing.T) {
 	tests := []struct {
-		name     string
-		certPath string
-		keyPath  string
-		wantErr  string
+		name    string
+		cert    string
+		key     string
+		wantErr string
+		test    []byte
 	}{
 		{
-			name:     "empty key",
-			certPath: "./testdata/test.crt",
-			keyPath:  "./testdata/empty",
-			wantErr:  "could not load CA: tls: failed to find any PEM data in key input",
+			name:    "empty key",
+			cert:    testCert,
+			key:     empty,
+			wantErr: "could not load CA: tls: failed to find any PEM data in key input",
 		},
 		{
-			name:     "invalid key",
-			certPath: "./testdata/test.crt",
-			keyPath:  "./testdata/invalid",
-			wantErr:  "could not load CA: tls: failed to find any PEM data in key input",
+			name:    "invalid key",
+			cert:    testCert,
+			key:     invalid,
+			wantErr: "could not load CA: tls: failed to find any PEM data in key input",
 		},
 		{
-			name:     "mismatched cert and key",
-			certPath: "./testdata/test.crt",
-			keyPath:  "./testdata/test2.key",
-			wantErr:  "could not load CA: tls: private key does not match public key",
+			name:    "mismatched cert and key",
+			cert:    testCert,
+			key:     testKey2,
+			wantErr: "could not load CA: tls: private key does not match public key",
 		},
 		{
-			name:     "multiple certs",
-			certPath: "./testdata/multiple.crt",
-			keyPath:  "./testdata/test.key",
-			wantErr:  "invalid CA certificate: expected a single certificate, found 2 certificates",
+			name:    "multiple certs",
+			cert:    multiple,
+			key:     testKey,
+			wantErr: "invalid CA certificate: expected a single certificate, found 2 certificates",
 		},
 		{
-			name:     "success",
-			certPath: "./testdata/test.crt",
-			keyPath:  "./testdata/test.key",
+			name: "success",
+			cert: testCert,
+			key:  testKey,
 		},
 	}
 	for _, tt := range tests {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
-			ca, err := loadFromFiles(t, tt.certPath, tt.keyPath)
+			t.Parallel()
+
+			ca, err := Load(tt.cert, tt.key)
 			if tt.wantErr != "" {
 				require.EqualError(t, err, tt.wantErr)
 				return
@@ -226,7 +231,7 @@ func TestIssue(t *testing.T) {
 
 	now := time.Date(2020, 7, 10, 12, 41, 12, 1234, time.UTC)
 
-	realCA, err := loadFromFiles(t, "./testdata/test.crt", "./testdata/test.key")
+	realCA, err := Load(testCert, testKey)
 	require.NoError(t, err)
 
 	tests := []struct {

internal/concierge/server/server.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package server is the command line entry point for pinniped-concierge.
@@ -118,17 +118,17 @@ func (a *App) runServer(ctx context.Context) error {
 
 	// This cert provider will provide certs to the API server and will
 	// be mutated by a controller to keep the certs up to date with what
-	// is stored in a k8s Secret. Therefore it also effectively acting as
-	// an in-memory cache of what is stored in the k8s Secret, helping to
-	// keep incoming requests fast.
+	// is stored in a k8s Secret. Therefore, it acts as an in-memory cache
+	// of what is stored in the k8s Secret, helping to keep incoming requests
+	// fast.
 	dynamicServingCertProvider := dynamiccert.NewServingCert("concierge-serving-cert")
 
 	// 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.
+	// cert issuer used to issue certs to Pinniped clients wishing to log in.
 	dynamicSigningCertProvider := dynamiccert.NewCA("concierge-kube-signing-cert")
 
 	// 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.
+	// cert issuer used to issue certs to Pinniped clients wishing to log in.
 	impersonationProxySigningCertProvider := dynamiccert.NewCA("impersonation-proxy-signing-cert")
 
 	// Get the "real" name of the login concierge API group (i.e., the API group name with the
@@ -256,7 +256,8 @@ func getAggregatedAPIServerConfig(
 	return apiServerConfig, nil
 }
 
-func main() error { // return an error instead of plog.Fatal to allow defer statements to run
+// main returns an error instead of calling plog.Fatal to allow defer statements to run.
+func main() error {
 	defer plog.Setup()()
 
 	// Dump out the time since compile (mostly useful for benchmarking our local development cycle latency).

internal/config/concierge/config.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package concierge contains functionality to load/store Config's from/to
@@ -35,7 +35,7 @@ const (
 	impersonationProxyPortDefault = 8444
 )
 
-// FromPath loads an Config from a provided local file path, inserts any
+// FromPath loads a Config from a provided local file path, inserts any
 // defaults (from the Config documentation), and verifies that the config is
 // valid (per the Config documentation).
 //

internal/config/concierge/types.go

@@ -1,11 +1,11 @@
-// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package concierge
 
 import "go.pinniped.dev/internal/plog"
 
-// Config contains knobs to setup an instance of the Pinniped Concierge.
+// Config contains knobs to set up an instance of the Pinniped Concierge.
 type Config struct {
 	DiscoveryInfo                DiscoveryInfoSpec `json:"discovery"`
 	APIConfig                    APIConfigSpec     `json:"api"`
@@ -21,7 +21,7 @@ type Config struct {
 }
 
 // DiscoveryInfoSpec contains configuration knobs specific to
-// pinniped's publishing of discovery information. These values can be
+// Pinniped's publishing of discovery information. These values can be
 // viewed as overrides, i.e., if these are set, then Pinniped will
 // publish these values in its discovery document instead of the ones it finds.
 type DiscoveryInfoSpec struct {

internal/controller/impersonatorconfig/impersonator_config.go

@@ -1,4 +1,4 @@
-// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved.
+// Copyright 2021-2023 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package impersonatorconfig
@@ -285,12 +285,10 @@ func (c *impersonatorConfigController) doSync(syncCtx controllerlib.Context, cre
 		return nil, err
 	}
 
-	var impersonationCA *certauthority.CA
+	var impersonationCABundle []byte
 	if c.shouldHaveImpersonator(impersonationSpec) {
-		if impersonationCA, err = c.ensureCASecretIsCreated(ctx); err != nil {
-			return nil, err
-		}
-		if err = c.ensureTLSSecret(ctx, nameInfo, impersonationCA); err != nil {
+		impersonationCABundle, err = c.ensureCAAndTLSSecrets(ctx, nameInfo)
+		if err != nil {
 			return nil, err
 		}
 	} else {
@@ -300,7 +298,7 @@ func (c *impersonatorConfigController) doSync(syncCtx controllerlib.Context, cre
 		c.clearTLSSecret()
 	}
 
-	credentialIssuerStrategyResult := c.doSyncResult(nameInfo, impersonationSpec, impersonationCA)
+	credentialIssuerStrategyResult := c.doSyncResult(nameInfo, impersonationSpec, impersonationCABundle)
 
 	if c.shouldHaveImpersonator(impersonationSpec) {
 		if err = c.loadSignerCA(); err != nil {
@@ -313,6 +311,25 @@ func (c *impersonatorConfigController) doSync(syncCtx controllerlib.Context, cre
 	return credentialIssuerStrategyResult, nil
 }
 
+func (c *impersonatorConfigController) ensureCAAndTLSSecrets(ctx context.Context, nameInfo *certNameInfo) ([]byte, error) {
+	var (
+		impersonationCA *certauthority.CA
+		err             error
+	)
+	if impersonationCA, err = c.ensureCASecretIsCreated(ctx); err != nil {
+		return nil, err
+	}
+	if err = c.ensureTLSSecret(ctx, nameInfo, impersonationCA); err != nil {
+		return nil, err
+	}
+
+	if impersonationCA != nil {
+		return impersonationCA.Bundle(), nil
+	}
+
+	return nil, nil
+}
+
 func (c *impersonatorConfigController) loadImpersonationProxyConfiguration(credIssuer *v1alpha1.CredentialIssuer) (*v1alpha1.ImpersonationProxySpec, error) {
 	// Make a copy of the spec since we got this object from informer cache.
 	spec := credIssuer.Spec.DeepCopy().ImpersonationProxy
@@ -707,7 +724,7 @@ func (c *impersonatorConfigController) deleteTLSSecretWhenCertificateDoesNotMatc
 	}
 
 	if !nameInfo.ready {
-		// We currently have a secret but we are waiting for a load balancer to be assigned an ingress, so
+		// We currently have a secret, but we are waiting for a load balancer to be assigned an ingress, so
 		// our current secret must be old/unwanted.
 		if err = c.ensureTLSSecretIsRemoved(ctx); err != nil {
 			return false, err
@@ -1018,7 +1035,7 @@ func (c *impersonatorConfigController) clearSignerCA() {
 	c.impersonationSigningCertProvider.UnsetCertKeyContent()
 }
 
-func (c *impersonatorConfigController) doSyncResult(nameInfo *certNameInfo, config *v1alpha1.ImpersonationProxySpec, ca *certauthority.CA) *v1alpha1.CredentialIssuerStrategy {
+func (c *impersonatorConfigController) doSyncResult(nameInfo *certNameInfo, config *v1alpha1.ImpersonationProxySpec, caBundle []byte) *v1alpha1.CredentialIssuerStrategy {
 	switch {
 	case c.disabledExplicitly(config):
 		return &v1alpha1.CredentialIssuerStrategy{
@@ -1055,7 +1072,7 @@ func (c *impersonatorConfigController) doSyncResult(nameInfo *certNameInfo, conf
 				Type: v1alpha1.ImpersonationProxyFrontendType,
 				ImpersonationProxyInfo: &v1alpha1.ImpersonationProxyInfo{
 					Endpoint:                 "https://" + nameInfo.clientEndpoint,
-					CertificateAuthorityData: base64.StdEncoding.EncodeToString(ca.Bundle()),
+					CertificateAuthorityData: base64.StdEncoding.EncodeToString(caBundle),
 				},
 			},
 		}

internal/controllerlib/manager.go

@@ -1,4 +1,4 @@
-// Copyright 2020 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package controllerlib
@@ -39,8 +39,8 @@ func (c *controllerManager) WithController(controller Controller, workers int) M
 	return c
 }
 
-// Start will run all managed controllers and block until all controllers shutdown.
-// When the context passed is cancelled, all controllers are signalled to shutdown.
+// Start will run all managed controllers and block until all controllers have shut down.
+// When the context passed is cancelled, all controllers are signalled to shut down.
 func (c *controllerManager) Start(ctx context.Context) {
 	var wg sync.WaitGroup
 	wg.Add(len(c.controllers))

internal/issuer/issuer.go

@@ -1,4 +1,4 @@
-// Copyright 2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2021-2023 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package issuer
@@ -38,15 +38,14 @@ func (c ClientCertIssuers) Name() string {
 }
 
 func (c ClientCertIssuers) IssueClientCertPEM(username string, groups []string, ttl time.Duration) ([]byte, []byte, error) {
-	var errs []error
+	errs := make([]error, 0, len(c))
 
 	for _, issuer := range c {
 		certPEM, keyPEM, err := issuer.IssueClientCertPEM(username, groups, ttl)
-		if err != nil {
-			errs = append(errs, fmt.Errorf("%s failed to issue client cert: %w", issuer.Name(), err))
-			continue
+		if err == nil {
+			return certPEM, keyPEM, nil
 		}
-		return certPEM, keyPEM, nil
+		errs = append(errs, fmt.Errorf("%s failed to issue client cert: %w", issuer.Name(), err))
 	}
 
 	if err := errors.NewAggregate(errs); err != nil {