From c82f568b2c14c16eb918f66ed7b4235db52b87e3 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Fri, 12 Mar 2021 16:09:16 -0800 Subject: [PATCH] certauthority.go: Refactor issuing client versus server certs We were previously issuing both client certs and server certs with both extended key usages included. Split the Issue*() methods into separate methods for issuing server certs versus client certs so they can have different extended key usages tailored for each use case. Also took the opportunity to clean up the parameters of the Issue*() methods and New() methods to more closely match how we prefer to call them. We were always only passing the common name part of the pkix.Name to New(), so now the New() method just takes the common name as a string. When making a server cert, we don't need to set the deprecated common name field, so remove that param. When making a client cert, we're always making it in the format expected by the Kube API server, so just accept the username and group as parameters directly. --- cmd/local-user-authenticator/main_test.go | 5 +- cmd/pinniped/cmd/flag_types_test.go | 3 +- cmd/pinniped/cmd/kubeconfig_test.go | 5 +- cmd/pinniped/cmd/login_oidc_test.go | 3 +- cmd/pinniped/cmd/login_static_test.go | 3 +- internal/certauthority/certauthority.go | 57 ++++--- internal/certauthority/certauthority_test.go | 146 ++++++++++++++---- .../dynamiccertauthority.go | 7 +- .../dynamiccertauthority_test.go | 18 +-- internal/concierge/apiserver/apiserver.go | 2 +- .../impersonator/impersonator_test.go | 11 +- internal/concierge/server/server.go | 4 +- internal/controller/apicerts/certs_manager.go | 10 +- .../controller/apicerts/certs_manager_test.go | 6 +- .../impersonatorconfig/impersonator_config.go | 19 ++- .../impersonator_config_test.go | 14 +- internal/issuer/issuer.go | 13 +- .../credentialrequestmocks.go | 43 +----- .../mocks/credentialrequestmocks/generate.go | 4 +- internal/mocks/issuermocks/generate.go | 6 + internal/mocks/issuermocks/issuermocks.go | 55 +++++++ internal/registry/credentialrequest/rest.go | 22 +-- .../registry/credentialrequest/rest_test.go | 32 ++-- internal/testutil/certs.go | 54 ++++++- pkg/conciergeclient/conciergeclient_test.go | 3 +- test/integration/e2e_test.go | 10 +- test/integration/supervisor_discovery_test.go | 5 +- test/integration/supervisor_login_test.go | 10 +- 28 files changed, 346 insertions(+), 224 deletions(-) create mode 100644 internal/mocks/issuermocks/generate.go create mode 100644 internal/mocks/issuermocks/issuermocks.go diff --git a/cmd/local-user-authenticator/main_test.go b/cmd/local-user-authenticator/main_test.go index 632fbde0..5a6fb0bd 100644 --- a/cmd/local-user-authenticator/main_test.go +++ b/cmd/local-user-authenticator/main_test.go @@ -8,7 +8,6 @@ import ( "context" "crypto/tls" "crypto/x509" - "crypto/x509/pkix" "encoding/json" "fmt" "io" @@ -464,10 +463,10 @@ func newCertProvider(t *testing.T) (dynamiccert.Provider, []byte, string) { serverName := "local-user-authenticator" - ca, err := certauthority.New(pkix.Name{CommonName: serverName + " CA"}, time.Hour*24) + ca, err := certauthority.New(serverName+" CA", time.Hour*24) require.NoError(t, err) - cert, err := ca.Issue(pkix.Name{CommonName: serverName}, []string{serverName}, nil, time.Hour*24) + cert, err := ca.IssueServerCert([]string{serverName}, nil, time.Hour*24) require.NoError(t, err) certPEM, keyPEM, err := certauthority.ToPEM(cert) diff --git a/cmd/pinniped/cmd/flag_types_test.go b/cmd/pinniped/cmd/flag_types_test.go index 6d967969..101191d5 100644 --- a/cmd/pinniped/cmd/flag_types_test.go +++ b/cmd/pinniped/cmd/flag_types_test.go @@ -5,7 +5,6 @@ package cmd import ( "bytes" - "crypto/x509/pkix" "fmt" "io/ioutil" "path/filepath" @@ -51,7 +50,7 @@ func TestConciergeModeFlag(t *testing.T) { } func TestCABundleFlag(t *testing.T) { - testCA, err := certauthority.New(pkix.Name{CommonName: "Test CA"}, 1*time.Hour) + testCA, err := certauthority.New("Test CA", 1*time.Hour) require.NoError(t, err) tmpdir := testutil.TempDir(t) emptyFilePath := filepath.Join(tmpdir, "empty") diff --git a/cmd/pinniped/cmd/kubeconfig_test.go b/cmd/pinniped/cmd/kubeconfig_test.go index f3b27d5d..879e2d71 100644 --- a/cmd/pinniped/cmd/kubeconfig_test.go +++ b/cmd/pinniped/cmd/kubeconfig_test.go @@ -5,7 +5,6 @@ package cmd import ( "bytes" - "crypto/x509/pkix" "encoding/base64" "fmt" "io/ioutil" @@ -30,13 +29,13 @@ import ( ) func TestGetKubeconfig(t *testing.T) { - testOIDCCA, err := certauthority.New(pkix.Name{CommonName: "Test CA"}, 1*time.Hour) + testOIDCCA, err := certauthority.New("Test CA", 1*time.Hour) require.NoError(t, err) tmpdir := testutil.TempDir(t) testOIDCCABundlePath := filepath.Join(tmpdir, "testca.pem") require.NoError(t, ioutil.WriteFile(testOIDCCABundlePath, testOIDCCA.Bundle(), 0600)) - testConciergeCA, err := certauthority.New(pkix.Name{CommonName: "Test Concierge CA"}, 1*time.Hour) + testConciergeCA, err := certauthority.New("Test Concierge CA", 1*time.Hour) require.NoError(t, err) testConciergeCABundlePath := filepath.Join(tmpdir, "testconciergeca.pem") require.NoError(t, ioutil.WriteFile(testConciergeCABundlePath, testConciergeCA.Bundle(), 0600)) diff --git a/cmd/pinniped/cmd/login_oidc_test.go b/cmd/pinniped/cmd/login_oidc_test.go index 976e3adf..2b1e8469 100644 --- a/cmd/pinniped/cmd/login_oidc_test.go +++ b/cmd/pinniped/cmd/login_oidc_test.go @@ -6,7 +6,6 @@ package cmd import ( "bytes" "context" - "crypto/x509/pkix" "encoding/base64" "fmt" "io/ioutil" @@ -29,7 +28,7 @@ import ( func TestLoginOIDCCommand(t *testing.T) { cfgDir := mustGetConfigDir() - testCA, err := certauthority.New(pkix.Name{CommonName: "Test CA"}, 1*time.Hour) + testCA, err := certauthority.New("Test CA", 1*time.Hour) require.NoError(t, err) tmpdir := testutil.TempDir(t) testCABundlePath := filepath.Join(tmpdir, "testca.pem") diff --git a/cmd/pinniped/cmd/login_static_test.go b/cmd/pinniped/cmd/login_static_test.go index 1d097123..caf41df5 100644 --- a/cmd/pinniped/cmd/login_static_test.go +++ b/cmd/pinniped/cmd/login_static_test.go @@ -6,7 +6,6 @@ package cmd import ( "bytes" "context" - "crypto/x509/pkix" "fmt" "io/ioutil" "path/filepath" @@ -24,7 +23,7 @@ import ( ) func TestLoginStaticCommand(t *testing.T) { - testCA, err := certauthority.New(pkix.Name{CommonName: "Test CA"}, 1*time.Hour) + testCA, err := certauthority.New("Test CA", 1*time.Hour) require.NoError(t, err) tmpdir := testutil.TempDir(t) testCABundlePath := filepath.Join(tmpdir, "testca.pem") diff --git a/internal/certauthority/certauthority.go b/internal/certauthority/certauthority.go index ad81e38a..b64bf0fa 100644 --- a/internal/certauthority/certauthority.go +++ b/internal/certauthority/certauthority.go @@ -89,13 +89,13 @@ func Load(certPEM string, keyPEM string) (*CA, error) { }, nil } -// New generates a fresh certificate authority with the given subject and ttl. -func New(subject pkix.Name, ttl time.Duration) (*CA, error) { - return newInternal(subject, ttl, secureEnv()) +// New generates a fresh certificate authority with the given Common Name and TTL. +func New(commonName string, ttl time.Duration) (*CA, error) { + return newInternal(commonName, ttl, secureEnv()) } // newInternal is the internal guts of New, broken out for easier testing. -func newInternal(subject pkix.Name, ttl time.Duration, env env) (*CA, error) { +func newInternal(commonName string, ttl time.Duration, env env) (*CA, error) { ca := CA{env: env} // Generate a random serial for the CA serialNumber, err := randomSerial(env.serialRNG) @@ -118,7 +118,7 @@ func newInternal(subject pkix.Name, ttl time.Duration, env env) (*CA, error) { // Create CA cert template caTemplate := x509.Certificate{ SerialNumber: serialNumber, - Subject: subject, + Subject: pkix.Name{CommonName: commonName}, NotBefore: notBefore, NotAfter: notAfter, IsCA: true, @@ -160,8 +160,31 @@ func (c *CA) Pool() *x509.CertPool { return pool } -// Issue a new server certificate for the given identity and duration. -func (c *CA) Issue(subject pkix.Name, dnsNames []string, ips []net.IP, ttl time.Duration) (*tls.Certificate, error) { +// IssueClientCert issues a new client certificate with username and groups included in the Kube-style +// certificate subject for the given identity and duration. +func (c *CA) IssueClientCert(username string, groups []string, ttl time.Duration) (*tls.Certificate, error) { + return c.issueCert(x509.ExtKeyUsageClientAuth, pkix.Name{CommonName: username, Organization: groups}, nil, nil, ttl) +} + +// IssueServerCert issues a new server certificate for the given identity and duration. +// The dnsNames and ips are each optional, but at least one of them should be specified. +func (c *CA) IssueServerCert(dnsNames []string, ips []net.IP, ttl time.Duration) (*tls.Certificate, error) { + 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 +// 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 +// 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)) +} + +func (c *CA) issueCert(extKeyUsage x509.ExtKeyUsage, subject pkix.Name, dnsNames []string, ips []net.IP, ttl time.Duration) (*tls.Certificate, error) { // Choose a random 128 bit serial number. serialNumber, err := randomSerial(c.env.serialRNG) if err != nil { @@ -187,13 +210,11 @@ func (c *CA) Issue(subject pkix.Name, dnsNames []string, ips []net.IP, ttl time. // Sign a cert, getting back the DER-encoded certificate bytes. template := x509.Certificate{ - SerialNumber: serialNumber, - Subject: subject, - NotBefore: notBefore, - NotAfter: notAfter, - KeyUsage: x509.KeyUsageDigitalSignature, - // TODO split this function into two funcs that handle client and serving certs differently - ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth}, + SerialNumber: serialNumber, + Subject: subject, + NotBefore: notBefore, + NotAfter: notAfter, + ExtKeyUsage: []x509.ExtKeyUsage{extKeyUsage}, BasicConstraintsValid: true, IsCA: false, DNSNames: dnsNames, @@ -218,14 +239,8 @@ func (c *CA) Issue(subject pkix.Name, dnsNames []string, ips []net.IP, ttl time. }, nil } -// IssuePEM issues a new server certificate for the given identity and duration, returning it as a pair of -// PEM-formatted byte slices for the certificate and private key. -func (c *CA) IssuePEM(subject pkix.Name, dnsNames []string, ips []net.IP, ttl time.Duration) ([]byte, []byte, error) { - return toPEM(c.Issue(subject, dnsNames, ips, ttl)) -} - func toPEM(cert *tls.Certificate, err error) ([]byte, []byte, error) { - // If the wrapped Issue() returned an error, pass it back. + // If the wrapped IssueServerCert() returned an error, pass it back. if err != nil { return nil, nil, err } diff --git a/internal/certauthority/certauthority_test.go b/internal/certauthority/certauthority_test.go index 7f9e3571..80948a77 100644 --- a/internal/certauthority/certauthority_test.go +++ b/internal/certauthority/certauthority_test.go @@ -7,7 +7,6 @@ import ( "crypto" "crypto/tls" "crypto/x509" - "crypto/x509/pkix" "fmt" "io" "io/ioutil" @@ -17,6 +16,8 @@ import ( "time" "github.com/stretchr/testify/require" + + "go.pinniped.dev/internal/testutil" ) func loadFromFiles(t *testing.T, certPath string, keyPath string) (*CA, error) { @@ -87,7 +88,7 @@ func TestLoad(t *testing.T) { func TestNew(t *testing.T) { now := time.Now() - ca, err := New(pkix.Name{CommonName: "Test CA"}, time.Minute) + ca, err := New("Test CA", time.Minute) require.NoError(t, err) require.NotNil(t, ca) @@ -158,7 +159,7 @@ func TestNewInternal(t *testing.T) { for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { - got, err := newInternal(pkix.Name{CommonName: "Test CA"}, tt.ttl, tt.env) + got, err := newInternal("Test CA", tt.ttl, tt.env) if tt.wantErr != "" { require.EqualError(t, err, tt.wantErr) require.Nil(t, got) @@ -184,7 +185,7 @@ func TestBundle(t *testing.T) { } func TestPrivateKeyToPEM(t *testing.T) { - ca, err := New(pkix.Name{CommonName: "Test CA"}, time.Hour) + ca, err := New("Test CA", time.Hour) require.NoError(t, err) keyPEM, err := ca.PrivateKeyToPEM() require.NoError(t, err) @@ -201,7 +202,7 @@ func TestPrivateKeyToPEM(t *testing.T) { } func TestPool(t *testing.T) { - ca, err := New(pkix.Name{CommonName: "test"}, 1*time.Hour) + ca, err := New("test", 1*time.Hour) require.NoError(t, err) pool := ca.Pool() @@ -220,6 +221,8 @@ func (e *errSigner) Sign(_ io.Reader, _ []byte, _ crypto.SignerOpts) ([]byte, er } func TestIssue(t *testing.T) { + const numRandBytes = 64 * 2 // each call to issue a cert will consume 64 bytes from the reader + now := time.Date(2020, 7, 10, 12, 41, 12, 1234, time.UTC) realCA, err := loadFromFiles(t, "./testdata/test.crt", "./testdata/test.key") @@ -243,7 +246,7 @@ func TestIssue(t *testing.T) { name: "failed to generate keypair", ca: CA{ env: env{ - serialRNG: strings.NewReader(strings.Repeat("x", 64)), + serialRNG: strings.NewReader(strings.Repeat("x", numRandBytes)), keygenRNG: strings.NewReader(""), }, }, @@ -253,8 +256,8 @@ func TestIssue(t *testing.T) { name: "invalid CA certificate", ca: CA{ env: env{ - serialRNG: strings.NewReader(strings.Repeat("x", 64)), - keygenRNG: strings.NewReader(strings.Repeat("x", 64)), + serialRNG: strings.NewReader(strings.Repeat("x", numRandBytes)), + keygenRNG: strings.NewReader(strings.Repeat("x", numRandBytes)), clock: func() time.Time { return now }, }, }, @@ -264,8 +267,8 @@ func TestIssue(t *testing.T) { name: "signing error", ca: CA{ env: env{ - serialRNG: strings.NewReader(strings.Repeat("x", 64)), - keygenRNG: strings.NewReader(strings.Repeat("x", 64)), + serialRNG: strings.NewReader(strings.Repeat("x", numRandBytes)), + keygenRNG: strings.NewReader(strings.Repeat("x", numRandBytes)), clock: func() time.Time { return now }, }, caCertBytes: realCA.caCertBytes, @@ -277,11 +280,11 @@ func TestIssue(t *testing.T) { wantErr: "could not sign certificate: some signer error", }, { - name: "success", + name: "parse certificate error", ca: CA{ env: env{ - serialRNG: strings.NewReader(strings.Repeat("x", 64)), - keygenRNG: strings.NewReader(strings.Repeat("x", 64)), + serialRNG: strings.NewReader(strings.Repeat("x", numRandBytes)), + keygenRNG: strings.NewReader(strings.Repeat("x", numRandBytes)), clock: func() time.Time { return now }, parseCert: func(_ []byte) (*x509.Certificate, error) { return nil, fmt.Errorf("some parse certificate error") @@ -296,8 +299,8 @@ func TestIssue(t *testing.T) { name: "success", ca: CA{ env: env{ - serialRNG: strings.NewReader(strings.Repeat("x", 64)), - keygenRNG: strings.NewReader(strings.Repeat("x", 64)), + serialRNG: strings.NewReader(strings.Repeat("x", numRandBytes)), + keygenRNG: strings.NewReader(strings.Repeat("x", numRandBytes)), clock: func() time.Time { return now }, parseCert: x509.ParseCertificate, }, @@ -309,28 +312,26 @@ func TestIssue(t *testing.T) { for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { - got, err := tt.ca.Issue(pkix.Name{CommonName: "Test Server"}, []string{"example.com"}, []net.IP{net.IPv4(1, 2, 3, 4)}, 10*time.Minute) + got, err := tt.ca.IssueServerCert([]string{"example.com"}, []net.IP{net.IPv4(1, 2, 3, 4)}, 10*time.Minute) if tt.wantErr != "" { require.EqualError(t, err, tt.wantErr) require.Nil(t, got) - return + } else { + require.NoError(t, err) + require.NotNil(t, got) + } + got, err = tt.ca.IssueClientCert("test-user", []string{"group1", "group2"}, 10*time.Minute) + if tt.wantErr != "" { + require.EqualError(t, err, tt.wantErr) + require.Nil(t, got) + } else { + require.NoError(t, err) + require.NotNil(t, got) } - require.NoError(t, err) - require.NotNil(t, got) }) } } -func TestIssuePEM(t *testing.T) { - realCA, err := loadFromFiles(t, "./testdata/test.crt", "./testdata/test.key") - require.NoError(t, err) - - certPEM, keyPEM, err := realCA.IssuePEM(pkix.Name{CommonName: "Test Server"}, []string{"example.com"}, nil, 10*time.Minute) - require.NoError(t, err) - require.NotEmpty(t, certPEM) - require.NotEmpty(t, keyPEM) -} - func TestToPEM(t *testing.T) { realCert, err := tls.LoadX509KeyPair("./testdata/test.crt", "./testdata/test.key") require.NoError(t, err) @@ -358,3 +359,90 @@ func TestToPEM(t *testing.T) { require.NotEmpty(t, keyPEM) }) } + +func TestIssueMethods(t *testing.T) { + // One CA can be used to issue both kinds of certs. + ca, err := New("Test CA", time.Hour) + require.NoError(t, err) + + ttl := 121 * time.Hour + + t.Run("client certs", func(t *testing.T) { + user := "test-username" + groups := []string{"group1", "group2"} + + clientCert, err := ca.IssueClientCert(user, groups, ttl) + require.NoError(t, err) + certPEM, keyPEM, err := ToPEM(clientCert) + require.NoError(t, err) + validateClientCert(t, ca.Bundle(), certPEM, keyPEM, user, groups, ttl) + + certPEM, keyPEM, err = ca.IssueClientCertPEM(user, groups, ttl) + require.NoError(t, err) + validateClientCert(t, ca.Bundle(), certPEM, keyPEM, user, groups, ttl) + + certPEM, keyPEM, err = ca.IssueClientCertPEM(user, nil, ttl) + require.NoError(t, err) + validateClientCert(t, ca.Bundle(), certPEM, keyPEM, user, nil, ttl) + + certPEM, keyPEM, err = ca.IssueClientCertPEM(user, []string{}, ttl) + require.NoError(t, err) + validateClientCert(t, ca.Bundle(), certPEM, keyPEM, user, nil, ttl) + + certPEM, keyPEM, err = ca.IssueClientCertPEM("", []string{}, ttl) + require.NoError(t, err) + validateClientCert(t, ca.Bundle(), certPEM, keyPEM, "", nil, ttl) + }) + + t.Run("server certs", func(t *testing.T) { + dnsNames := []string{"example.com", "pinniped.dev"} + ips := []net.IP{net.ParseIP("127.0.0.1"), net.ParseIP("1.2.3.4")} + + serverCert, err := ca.IssueServerCert(dnsNames, ips, ttl) + require.NoError(t, err) + certPEM, keyPEM, err := ToPEM(serverCert) + require.NoError(t, err) + validateServerCert(t, ca.Bundle(), certPEM, keyPEM, dnsNames, ips, ttl) + + certPEM, keyPEM, err = ca.IssueServerCertPEM(dnsNames, ips, ttl) + require.NoError(t, err) + validateServerCert(t, ca.Bundle(), certPEM, keyPEM, dnsNames, ips, ttl) + + certPEM, keyPEM, err = ca.IssueServerCertPEM(nil, ips, ttl) + require.NoError(t, err) + validateServerCert(t, ca.Bundle(), certPEM, keyPEM, nil, ips, ttl) + + certPEM, keyPEM, err = ca.IssueServerCertPEM(dnsNames, nil, ttl) + require.NoError(t, err) + validateServerCert(t, ca.Bundle(), certPEM, keyPEM, dnsNames, nil, ttl) + + certPEM, keyPEM, err = ca.IssueServerCertPEM([]string{}, ips, ttl) + require.NoError(t, err) + validateServerCert(t, ca.Bundle(), certPEM, keyPEM, nil, ips, ttl) + + certPEM, keyPEM, err = ca.IssueServerCertPEM(dnsNames, []net.IP{}, ttl) + require.NoError(t, err) + validateServerCert(t, ca.Bundle(), certPEM, keyPEM, dnsNames, nil, ttl) + }) +} + +func validateClientCert(t *testing.T, caBundle []byte, certPEM []byte, keyPEM []byte, expectedUser string, expectedGroups []string, expectedTTL time.Duration) { + const fudgeFactor = 10 * time.Second + v := testutil.ValidateClientCertificate(t, string(caBundle), string(certPEM)) + v.RequireLifetime(time.Now(), time.Now().Add(expectedTTL), certBackdate+fudgeFactor) + v.RequireMatchesPrivateKey(string(keyPEM)) + v.RequireCommonName(expectedUser) + v.RequireOrganizations(expectedGroups) + v.RequireEmptyDNSNames() + v.RequireEmptyIPs() +} + +func validateServerCert(t *testing.T, caBundle []byte, certPEM []byte, keyPEM []byte, expectedDNSNames []string, expectedIPs []net.IP, expectedTTL time.Duration) { + const fudgeFactor = 10 * time.Second + v := testutil.ValidateServerCertificate(t, string(caBundle), string(certPEM)) + v.RequireLifetime(time.Now(), time.Now().Add(expectedTTL), certBackdate+fudgeFactor) + v.RequireMatchesPrivateKey(string(keyPEM)) + v.RequireCommonName("") + v.RequireDNSNames(expectedDNSNames) + v.RequireIPs(expectedIPs) +} diff --git a/internal/certauthority/dynamiccertauthority/dynamiccertauthority.go b/internal/certauthority/dynamiccertauthority/dynamiccertauthority.go index 3c0c4b72..78eb97c6 100644 --- a/internal/certauthority/dynamiccertauthority/dynamiccertauthority.go +++ b/internal/certauthority/dynamiccertauthority/dynamiccertauthority.go @@ -6,7 +6,6 @@ package dynamiccertauthority import ( - "crypto/x509/pkix" "time" "k8s.io/apiserver/pkg/server/dynamiccertificates" @@ -27,14 +26,14 @@ func New(provider dynamiccertificates.CertKeyContentProvider) *CA { } } -// IssuePEM issues a new server certificate for the given identity and duration, returning it as a +// IssueClientCertPEM issues a new client certificate for the given identity and duration, returning it as a // pair of PEM-formatted byte slices for the certificate and private key. -func (c *CA) IssuePEM(subject pkix.Name, dnsNames []string, ttl time.Duration) ([]byte, []byte, error) { +func (c *CA) IssueClientCertPEM(username string, groups []string, ttl time.Duration) ([]byte, []byte, error) { caCrtPEM, caKeyPEM := c.provider.CurrentCertKeyContent() ca, err := certauthority.Load(string(caCrtPEM), string(caKeyPEM)) if err != nil { return nil, nil, err } - return ca.IssuePEM(subject, dnsNames, nil, ttl) + return ca.IssueClientCertPEM(username, groups, ttl) } diff --git a/internal/certauthority/dynamiccertauthority/dynamiccertauthority_test.go b/internal/certauthority/dynamiccertauthority/dynamiccertauthority_test.go index 41e0f291..47bc6539 100644 --- a/internal/certauthority/dynamiccertauthority/dynamiccertauthority_test.go +++ b/internal/certauthority/dynamiccertauthority/dynamiccertauthority_test.go @@ -1,10 +1,9 @@ -// 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 import ( - "crypto/x509/pkix" "testing" "time" @@ -106,10 +105,9 @@ func TestCAIssuePEM(t *testing.T) { require.NotEmpty(t, keyPEM) caCrtPEM, _ := provider.CurrentCertKeyContent() - crtAssertions := testutil.ValidateCertificate(t, string(caCrtPEM), string(crtPEM)) - crtAssertions.RequireCommonName("some-common-name") - crtAssertions.RequireDNSName("some-dns-name") - crtAssertions.RequireDNSName("some-other-dns-name") + crtAssertions := testutil.ValidateClientCertificate(t, string(caCrtPEM), string(crtPEM)) + crtAssertions.RequireCommonName("some-username") + crtAssertions.RequireOrganizations([]string{"some-group1", "some-group2"}) crtAssertions.RequireLifetime(time.Now(), time.Now().Add(time.Hour*24), time.Minute*10) crtAssertions.RequireMatchesPrivateKey(string(keyPEM)) } @@ -126,11 +124,5 @@ func issuePEM(provider dynamiccert.Provider, ca *CA, caCrt, caKey []byte) ([]byt } // otherwise check to see if their is an issuing error - return ca.IssuePEM( - pkix.Name{ - CommonName: "some-common-name", - }, - []string{"some-dns-name", "some-other-dns-name"}, - time.Hour*24, - ) + return ca.IssueClientCertPEM("some-username", []string{"some-group1", "some-group2"}, time.Hour*24) } diff --git a/internal/concierge/apiserver/apiserver.go b/internal/concierge/apiserver/apiserver.go index f64e0104..f1bce95b 100644 --- a/internal/concierge/apiserver/apiserver.go +++ b/internal/concierge/apiserver/apiserver.go @@ -28,7 +28,7 @@ type Config struct { type ExtraConfig struct { Authenticator credentialrequest.TokenCredentialRequestAuthenticator - Issuer issuer.CertIssuer + Issuer issuer.ClientCertIssuer StartControllersPostStartHook func(ctx context.Context) Scheme *runtime.Scheme NegotiatedSerializer runtime.NegotiatedSerializer diff --git a/internal/concierge/impersonator/impersonator_test.go b/internal/concierge/impersonator/impersonator_test.go index 9d1e231c..66985781 100644 --- a/internal/concierge/impersonator/impersonator_test.go +++ b/internal/concierge/impersonator/impersonator_test.go @@ -5,7 +5,6 @@ package impersonator import ( "context" - "crypto/x509/pkix" "net" "net/http" "net/http/httptest" @@ -36,7 +35,7 @@ import ( func TestImpersonator(t *testing.T) { const port = 9444 - ca, err := certauthority.New(pkix.Name{CommonName: "ca"}, time.Hour) + ca, err := certauthority.New("ca", time.Hour) require.NoError(t, err) caKey, err := ca.PrivateKeyToPEM() require.NoError(t, err) @@ -44,13 +43,13 @@ func TestImpersonator(t *testing.T) { err = caContent.SetCertKeyContent(ca.Bundle(), caKey) require.NoError(t, err) - cert, key, err := ca.IssuePEM(pkix.Name{}, nil, []net.IP{net.ParseIP("127.0.0.1")}, time.Hour) + cert, key, err := ca.IssueServerCertPEM(nil, []net.IP{net.ParseIP("127.0.0.1")}, time.Hour) require.NoError(t, err) certKeyContent := dynamiccert.New("cert-key") err = certKeyContent.SetCertKeyContent(cert, key) require.NoError(t, err) - unrelatedCA, err := certauthority.New(pkix.Name{CommonName: "ca"}, time.Hour) + unrelatedCA, err := certauthority.New("ca", time.Hour) require.NoError(t, err) // Punch out just enough stuff to make New actually run without error. @@ -486,9 +485,7 @@ type clientCert struct { } func newClientCert(t *testing.T, ca *certauthority.CA, username string, groups []string) *clientCert { - certPEM, keyPEM, err := ca.IssuePEM( - pkix.Name{CommonName: username, Organization: groups}, nil, nil, time.Hour, - ) + certPEM, keyPEM, err := ca.IssueClientCertPEM(username, groups, time.Hour) require.NoError(t, err) return &clientCert{ certPEM: certPEM, diff --git a/internal/concierge/server/server.go b/internal/concierge/server/server.go index 8602d07d..4a4415a5 100644 --- a/internal/concierge/server/server.go +++ b/internal/concierge/server/server.go @@ -150,7 +150,7 @@ func (a *App) runServer(ctx context.Context) error { return fmt.Errorf("could not prepare controllers: %w", err) } - certIssuer := issuer.CertIssuers{ + certIssuer := issuer.ClientCertIssuers{ dynamiccertauthority.New(dynamicSigningCertProvider), // attempt to use the real Kube CA if possible dynamiccertauthority.New(impersonationProxySigningCertProvider), // fallback to our internal CA if we need to } @@ -184,7 +184,7 @@ func (a *App) runServer(ctx context.Context) error { func getAggregatedAPIServerConfig( dynamicCertProvider dynamiccert.Provider, authenticator credentialrequest.TokenCredentialRequestAuthenticator, - issuer issuer.CertIssuer, + issuer issuer.ClientCertIssuer, startControllersPostStartHook func(context.Context), apiGroupSuffix string, scheme *runtime.Scheme, diff --git a/internal/controller/apicerts/certs_manager.go b/internal/controller/apicerts/certs_manager.go index f0c11d0d..c3d8bf36 100644 --- a/internal/controller/apicerts/certs_manager.go +++ b/internal/controller/apicerts/certs_manager.go @@ -4,7 +4,6 @@ package apicerts import ( - "crypto/x509/pkix" "fmt" "time" @@ -94,7 +93,7 @@ func (c *certsManagerController) Sync(ctx controllerlib.Context) error { } // Create a CA. - ca, err := certauthority.New(pkix.Name{CommonName: c.generatedCACommonName}, c.certDuration) + ca, err := certauthority.New(c.generatedCACommonName, c.certDuration) if err != nil { return fmt.Errorf("could not initialize CA: %w", err) } @@ -120,12 +119,7 @@ func (c *certsManagerController) Sync(ctx controllerlib.Context) error { // Using the CA from above, create a TLS server cert if we have service name. if len(c.serviceNameForGeneratedCertCommonName) != 0 { serviceEndpoint := c.serviceNameForGeneratedCertCommonName + "." + c.namespace + ".svc" - tlsCert, err := ca.Issue( - pkix.Name{CommonName: serviceEndpoint}, - []string{serviceEndpoint}, - nil, - c.certDuration, - ) + tlsCert, err := ca.IssueServerCert([]string{serviceEndpoint}, nil, c.certDuration) if err != nil { return fmt.Errorf("could not issue serving certificate: %w", err) } diff --git a/internal/controller/apicerts/certs_manager_test.go b/internal/controller/apicerts/certs_manager_test.go index b32b9228..092f504b 100644 --- a/internal/controller/apicerts/certs_manager_test.go +++ b/internal/controller/apicerts/certs_manager_test.go @@ -218,12 +218,12 @@ func TestManagerControllerSync(t *testing.T) { r.NotEmpty(actualCertChain) r.Len(actualSecret.StringData, 4) - validCACert := testutil.ValidateCertificate(t, actualCACert, actualCACert) + validCACert := testutil.ValidateServerCertificate(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 - validCert := testutil.ValidateCertificate(t, actualCACert, actualCertChain) + validCert := testutil.ValidateServerCertificate(t, actualCACert, actualCertChain) validCert.RequireDNSName("pinniped-api." + installedInNamespace + ".svc") validCert.RequireLifetime(time.Now(), time.Now().Add(certDuration), 6*time.Minute) validCert.RequireMatchesPrivateKey(actualPrivateKey) @@ -252,7 +252,7 @@ func TestManagerControllerSync(t *testing.T) { r.NotEmpty(actualCAPrivateKey) r.Len(actualSecret.StringData, 2) - validCACert := testutil.ValidateCertificate(t, actualCACert, actualCACert) + validCACert := testutil.ValidateServerCertificate(t, actualCACert, actualCACert) validCACert.RequireMatchesPrivateKey(actualCAPrivateKey) validCACert.RequireLifetime(time.Now(), time.Now().Add(certDuration), 6*time.Minute) }) diff --git a/internal/controller/impersonatorconfig/impersonator_config.go b/internal/controller/impersonatorconfig/impersonator_config.go index 6f81901d..1733d9da 100644 --- a/internal/controller/impersonatorconfig/impersonator_config.go +++ b/internal/controller/impersonatorconfig/impersonator_config.go @@ -7,7 +7,6 @@ import ( "context" "crypto/tls" "crypto/x509" - "crypto/x509/pkix" "encoding/base64" "encoding/pem" "fmt" @@ -40,13 +39,13 @@ import ( ) const ( - impersonationProxyPort = 8444 - defaultHTTPSPort = 443 - oneHundredYears = 100 * 365 * 24 * time.Hour - caCommonName = "Pinniped Impersonation Proxy CA" - caCrtKey = "ca.crt" - caKeyKey = "ca.key" - appLabelKey = "app" + impersonationProxyPort = 8444 + defaultHTTPSPort = 443 + approximatelyOneHundredYears = 100 * 365 * 24 * time.Hour + caCommonName = "Pinniped Impersonation Proxy CA" + caCrtKey = "ca.crt" + caKeyKey = "ca.key" + appLabelKey = "app" ) type impersonatorConfigController struct { @@ -622,7 +621,7 @@ func (c *impersonatorConfigController) ensureCASecretIsCreated(ctx context.Conte } func (c *impersonatorConfigController) createCASecret(ctx context.Context) (*certauthority.CA, error) { - impersonationCA, err := certauthority.New(pkix.Name{CommonName: caCommonName}, oneHundredYears) + impersonationCA, err := certauthority.New(caCommonName, approximatelyOneHundredYears) if err != nil { return nil, fmt.Errorf("could not create impersonation CA: %w", err) } @@ -716,7 +715,7 @@ func (c *impersonatorConfigController) createNewTLSSecret(ctx context.Context, c ips = []net.IP{ip} } - impersonationCert, err := ca.Issue(pkix.Name{}, hostnames, ips, oneHundredYears) + impersonationCert, err := ca.IssueServerCert(hostnames, ips, approximatelyOneHundredYears) if err != nil { return nil, fmt.Errorf("could not create impersonation cert: %w", err) } diff --git a/internal/controller/impersonatorconfig/impersonator_config_test.go b/internal/controller/impersonatorconfig/impersonator_config_test.go index f926208b..f4996184 100644 --- a/internal/controller/impersonatorconfig/impersonator_config_test.go +++ b/internal/controller/impersonatorconfig/impersonator_config_test.go @@ -7,7 +7,6 @@ import ( "context" "crypto/tls" "crypto/x509" - "crypto/x509/pkix" "encoding/base64" "encoding/pem" "errors" @@ -352,7 +351,10 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { require.NoError(t, err) roots := x509.NewCertPool() require.True(t, roots.AppendCertsFromPEM(currentClientCertCA)) - opts := x509.VerifyOptions{Roots: roots} + opts := x509.VerifyOptions{ + Roots: roots, + KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, + } _, err = parsed.Verify(opts) require.NoError(t, err) return nil @@ -594,7 +596,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { } var newCA = func() *certauthority.CA { - ca, err := certauthority.New(pkix.Name{CommonName: "test CA"}, 24*time.Hour) + ca, err := certauthority.New("test CA", 24*time.Hour) r.NoError(err) return ca } @@ -609,7 +611,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { } var newTLSCertSecretData = func(ca *certauthority.CA, dnsNames []string, ip string) map[string][]byte { - impersonationCert, err := ca.Issue(pkix.Name{}, dnsNames, []net.IP{net.ParseIP(ip)}, 24*time.Hour) + impersonationCert, err := ca.IssueServerCert(dnsNames, []net.IP{net.ParseIP(ip)}, 24*time.Hour) r.NoError(err) certPEM, keyPEM, err := certauthority.ToPEM(impersonationCert) r.NoError(err) @@ -939,7 +941,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { createdKeyPEM := createdSecret.Data[corev1.TLSPrivateKeyKey] r.NotNil(createdKeyPEM) r.NotNil(createdCertPEM) - validCert := testutil.ValidateCertificate(t, string(caCert), string(createdCertPEM)) + validCert := testutil.ValidateServerCertificate(t, string(caCert), string(createdCertPEM)) validCert.RequireMatchesPrivateKey(string(createdKeyPEM)) validCert.RequireLifetime(time.Now().Add(-10*time.Second), time.Now().Add(100*time.Hour*24*365), 10*time.Second) } @@ -980,7 +982,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { signingCAKeyPEM, err = ca.PrivateKeyToPEM() r.NoError(err) signingCASecret = newSigningKeySecret(caSignerName, signingCACertPEM, signingCAKeyPEM) - validClientCert, err = ca.Issue(pkix.Name{}, nil, nil, time.Hour) + validClientCert, err = ca.IssueClientCert("username", nil, time.Hour) r.NoError(err) }) diff --git a/internal/issuer/issuer.go b/internal/issuer/issuer.go index 94e2f235..88ac1538 100644 --- a/internal/issuer/issuer.go +++ b/internal/issuer/issuer.go @@ -4,7 +4,6 @@ package issuer import ( - "crypto/x509/pkix" "time" "k8s.io/apimachinery/pkg/util/errors" @@ -14,19 +13,19 @@ import ( 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) +type ClientCertIssuer interface { + IssueClientCertPEM(username string, groups []string, ttl time.Duration) (certPEM, keyPEM []byte, err error) } -var _ CertIssuer = CertIssuers{} +var _ ClientCertIssuer = ClientCertIssuers{} -type CertIssuers []CertIssuer +type ClientCertIssuers []ClientCertIssuer -func (c CertIssuers) IssuePEM(subject pkix.Name, dnsNames []string, ttl time.Duration) ([]byte, []byte, error) { +func (c ClientCertIssuers) IssueClientCertPEM(username string, groups []string, ttl time.Duration) ([]byte, []byte, error) { var errs []error for _, issuer := range c { - certPEM, keyPEM, err := issuer.IssuePEM(subject, dnsNames, ttl) + certPEM, keyPEM, err := issuer.IssueClientCertPEM(username, groups, ttl) if err != nil { errs = append(errs, err) continue diff --git a/internal/mocks/credentialrequestmocks/credentialrequestmocks.go b/internal/mocks/credentialrequestmocks/credentialrequestmocks.go index 58bd134d..68ff7f2e 100644 --- a/internal/mocks/credentialrequestmocks/credentialrequestmocks.go +++ b/internal/mocks/credentialrequestmocks/credentialrequestmocks.go @@ -3,61 +3,20 @@ // // Code generated by MockGen. DO NOT EDIT. -// Source: go.pinniped.dev/internal/registry/credentialrequest (interfaces: CertIssuer,TokenCredentialRequestAuthenticator) +// Source: go.pinniped.dev/internal/registry/credentialrequest (interfaces: TokenCredentialRequestAuthenticator) // Package credentialrequestmocks is a generated GoMock package. package credentialrequestmocks import ( context "context" - pkix "crypto/x509/pkix" reflect "reflect" - time "time" gomock "github.com/golang/mock/gomock" login "go.pinniped.dev/generated/latest/apis/concierge/login" user "k8s.io/apiserver/pkg/authentication/user" ) -// MockCertIssuer is a mock of CertIssuer interface. -type MockCertIssuer struct { - ctrl *gomock.Controller - recorder *MockCertIssuerMockRecorder -} - -// MockCertIssuerMockRecorder is the mock recorder for MockCertIssuer. -type MockCertIssuerMockRecorder struct { - mock *MockCertIssuer -} - -// NewMockCertIssuer creates a new mock instance. -func NewMockCertIssuer(ctrl *gomock.Controller) *MockCertIssuer { - mock := &MockCertIssuer{ctrl: ctrl} - mock.recorder = &MockCertIssuerMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockCertIssuer) EXPECT() *MockCertIssuerMockRecorder { - return m.recorder -} - -// IssuePEM mocks base method. -func (m *MockCertIssuer) IssuePEM(arg0 pkix.Name, arg1 []string, arg2 time.Duration) ([]byte, []byte, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "IssuePEM", arg0, arg1, arg2) - ret0, _ := ret[0].([]byte) - ret1, _ := ret[1].([]byte) - ret2, _ := ret[2].(error) - return ret0, ret1, ret2 -} - -// IssuePEM indicates an expected call of IssuePEM. -func (mr *MockCertIssuerMockRecorder) IssuePEM(arg0, arg1, arg2 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IssuePEM", reflect.TypeOf((*MockCertIssuer)(nil).IssuePEM), arg0, arg1, arg2) -} - // MockTokenCredentialRequestAuthenticator is a mock of TokenCredentialRequestAuthenticator interface. type MockTokenCredentialRequestAuthenticator struct { ctrl *gomock.Controller diff --git a/internal/mocks/credentialrequestmocks/generate.go b/internal/mocks/credentialrequestmocks/generate.go index c22ec978..ce19cf3f 100644 --- a/internal/mocks/credentialrequestmocks/generate.go +++ b/internal/mocks/credentialrequestmocks/generate.go @@ -1,6 +1,6 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package credentialrequestmocks -//go:generate go run -v github.com/golang/mock/mockgen -destination=credentialrequestmocks.go -package=credentialrequestmocks -copyright_file=../../../hack/header.txt go.pinniped.dev/internal/registry/credentialrequest CertIssuer,TokenCredentialRequestAuthenticator +//go:generate go run -v github.com/golang/mock/mockgen -destination=credentialrequestmocks.go -package=credentialrequestmocks -copyright_file=../../../hack/header.txt go.pinniped.dev/internal/registry/credentialrequest TokenCredentialRequestAuthenticator diff --git a/internal/mocks/issuermocks/generate.go b/internal/mocks/issuermocks/generate.go new file mode 100644 index 00000000..7d0c9937 --- /dev/null +++ b/internal/mocks/issuermocks/generate.go @@ -0,0 +1,6 @@ +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package issuermocks + +//go:generate go run -v github.com/golang/mock/mockgen -destination=issuermocks.go -package=issuermocks -copyright_file=../../../hack/header.txt go.pinniped.dev/internal/issuer ClientCertIssuer diff --git a/internal/mocks/issuermocks/issuermocks.go b/internal/mocks/issuermocks/issuermocks.go new file mode 100644 index 00000000..99dce2c5 --- /dev/null +++ b/internal/mocks/issuermocks/issuermocks.go @@ -0,0 +1,55 @@ +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 +// + +// Code generated by MockGen. DO NOT EDIT. +// Source: go.pinniped.dev/internal/issuer (interfaces: ClientCertIssuer) + +// Package issuermocks is a generated GoMock package. +package issuermocks + +import ( + reflect "reflect" + time "time" + + gomock "github.com/golang/mock/gomock" +) + +// MockClientCertIssuer is a mock of ClientCertIssuer interface. +type MockClientCertIssuer struct { + ctrl *gomock.Controller + recorder *MockClientCertIssuerMockRecorder +} + +// MockClientCertIssuerMockRecorder is the mock recorder for MockClientCertIssuer. +type MockClientCertIssuerMockRecorder struct { + mock *MockClientCertIssuer +} + +// NewMockClientCertIssuer creates a new mock instance. +func NewMockClientCertIssuer(ctrl *gomock.Controller) *MockClientCertIssuer { + mock := &MockClientCertIssuer{ctrl: ctrl} + mock.recorder = &MockClientCertIssuerMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockClientCertIssuer) EXPECT() *MockClientCertIssuerMockRecorder { + return m.recorder +} + +// IssueClientCertPEM mocks base method. +func (m *MockClientCertIssuer) IssueClientCertPEM(arg0 string, arg1 []string, arg2 time.Duration) ([]byte, []byte, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "IssueClientCertPEM", arg0, arg1, arg2) + ret0, _ := ret[0].([]byte) + ret1, _ := ret[1].([]byte) + ret2, _ := ret[2].(error) + return ret0, ret1, ret2 +} + +// IssueClientCertPEM indicates an expected call of IssueClientCertPEM. +func (mr *MockClientCertIssuerMockRecorder) IssueClientCertPEM(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IssueClientCertPEM", reflect.TypeOf((*MockClientCertIssuer)(nil).IssueClientCertPEM), arg0, arg1, arg2) +} diff --git a/internal/registry/credentialrequest/rest.go b/internal/registry/credentialrequest/rest.go index a1846a40..9c9165d5 100644 --- a/internal/registry/credentialrequest/rest.go +++ b/internal/registry/credentialrequest/rest.go @@ -6,7 +6,6 @@ package credentialrequest import ( "context" - "crypto/x509/pkix" "fmt" "time" @@ -32,7 +31,7 @@ type TokenCredentialRequestAuthenticator interface { AuthenticateTokenCredentialRequest(ctx context.Context, req *loginapi.TokenCredentialRequest) (user.Info, error) } -func NewREST(authenticator TokenCredentialRequestAuthenticator, issuer issuer.CertIssuer, resource schema.GroupResource) *REST { +func NewREST(authenticator TokenCredentialRequestAuthenticator, issuer issuer.ClientCertIssuer, resource schema.GroupResource) *REST { return &REST{ authenticator: authenticator, issuer: issuer, @@ -42,7 +41,7 @@ func NewREST(authenticator TokenCredentialRequestAuthenticator, issuer issuer.Ce type REST struct { authenticator TokenCredentialRequestAuthenticator - issuer issuer.CertIssuer + issuer issuer.ClientCertIssuer tableConvertor rest.TableConvertor } @@ -97,30 +96,23 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation return nil, err } - user, err := r.authenticator.AuthenticateTokenCredentialRequest(ctx, credentialRequest) + userInfo, err := r.authenticator.AuthenticateTokenCredentialRequest(ctx, credentialRequest) if err != nil { traceFailureWithError(t, "token authentication", err) return failureResponse(), nil } - if user == nil || user.GetName() == "" { - traceSuccess(t, user, false) + if userInfo == nil || userInfo.GetName() == "" { + traceSuccess(t, userInfo, false) return failureResponse(), nil } - certPEM, keyPEM, err := r.issuer.IssuePEM( - pkix.Name{ - CommonName: user.GetName(), - Organization: user.GetGroups(), - }, - []string{}, - clientCertificateTTL, - ) + certPEM, keyPEM, err := r.issuer.IssueClientCertPEM(userInfo.GetName(), userInfo.GetGroups(), clientCertificateTTL) if err != nil { traceFailureWithError(t, "cert issuer", err) return failureResponse(), nil } - traceSuccess(t, user, true) + traceSuccess(t, userInfo, true) return &loginapi.TokenCredentialRequest{ Status: loginapi.TokenCredentialRequestStatus{ diff --git a/internal/registry/credentialrequest/rest_test.go b/internal/registry/credentialrequest/rest_test.go index 6b71ec20..9c05c2bb 100644 --- a/internal/registry/credentialrequest/rest_test.go +++ b/internal/registry/credentialrequest/rest_test.go @@ -5,7 +5,6 @@ package credentialrequest import ( "context" - "crypto/x509/pkix" "errors" "fmt" "testing" @@ -26,6 +25,7 @@ import ( loginapi "go.pinniped.dev/generated/latest/apis/concierge/login" "go.pinniped.dev/internal/issuer" "go.pinniped.dev/internal/mocks/credentialrequestmocks" + "go.pinniped.dev/internal/mocks/issuermocks" "go.pinniped.dev/internal/testutil" ) @@ -89,16 +89,14 @@ func TestCreate(t *testing.T) { Groups: []string{"test-group-1", "test-group-2"}, }, nil) - issuer := credentialrequestmocks.NewMockCertIssuer(ctrl) - issuer.EXPECT().IssuePEM( - pkix.Name{ - CommonName: "test-user", - Organization: []string{"test-group-1", "test-group-2"}}, - []string{}, + clientCertIssuer := issuermocks.NewMockClientCertIssuer(ctrl) + clientCertIssuer.EXPECT().IssueClientCertPEM( + "test-user", + []string{"test-group-1", "test-group-2"}, 5*time.Minute, ).Return([]byte("test-cert"), []byte("test-key"), nil) - storage := NewREST(requestAuthenticator, issuer, schema.GroupResource{}) + storage := NewREST(requestAuthenticator, clientCertIssuer, schema.GroupResource{}) response, err := callCreate(context.Background(), storage, req) @@ -132,12 +130,12 @@ func TestCreate(t *testing.T) { Groups: []string{"test-group-1", "test-group-2"}, }, nil) - issuer := credentialrequestmocks.NewMockCertIssuer(ctrl) - issuer.EXPECT(). - IssuePEM(gomock.Any(), gomock.Any(), gomock.Any()). + clientCertIssuer := issuermocks.NewMockClientCertIssuer(ctrl) + clientCertIssuer.EXPECT(). + IssueClientCertPEM(gomock.Any(), gomock.Any(), gomock.Any()). Return(nil, nil, fmt.Errorf("some certificate authority error")) - storage := NewREST(requestAuthenticator, issuer, schema.GroupResource{}) + storage := NewREST(requestAuthenticator, clientCertIssuer, schema.GroupResource{}) response, err := callCreate(context.Background(), storage, req) requireSuccessfulResponseWithAuthenticationFailureMessage(t, err, response) @@ -354,12 +352,12 @@ func requireSuccessfulResponseWithAuthenticationFailureMessage(t *testing.T, err }) } -func successfulIssuer(ctrl *gomock.Controller) issuer.CertIssuer { - certIssuer := credentialrequestmocks.NewMockCertIssuer(ctrl) - certIssuer.EXPECT(). - IssuePEM(gomock.Any(), gomock.Any(), gomock.Any()). +func successfulIssuer(ctrl *gomock.Controller) issuer.ClientCertIssuer { + clientCertIssuer := issuermocks.NewMockClientCertIssuer(ctrl) + clientCertIssuer.EXPECT(). + IssueClientCertPEM(gomock.Any(), gomock.Any(), gomock.Any()). Return([]byte("test-cert"), []byte("test-key"), nil) - return certIssuer + return clientCertIssuer } func stringPtr(s string) *string { diff --git a/internal/testutil/certs.go b/internal/testutil/certs.go index 79792195..9d6b41d3 100644 --- a/internal/testutil/certs.go +++ b/internal/testutil/certs.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 testutil @@ -12,6 +12,7 @@ import ( "crypto/x509/pkix" "encoding/pem" "math/big" + "net" "testing" "time" @@ -25,10 +26,18 @@ type ValidCert struct { parsed *x509.Certificate } -// ValidateCertificate validates a certificate and provides an object for asserting properties of the certificate. -func ValidateCertificate(t *testing.T, caPEM string, certPEM string) *ValidCert { +// ValidateServerCertificate validates a certificate and provides an object for asserting properties of the certificate. +func ValidateServerCertificate(t *testing.T, caPEM string, certPEM string) *ValidCert { t.Helper() + return validateCertificate(t, x509.ExtKeyUsageServerAuth, caPEM, certPEM) +} +func ValidateClientCertificate(t *testing.T, caPEM string, certPEM string) *ValidCert { + t.Helper() + return validateCertificate(t, x509.ExtKeyUsageClientAuth, caPEM, certPEM) +} + +func validateCertificate(t *testing.T, extKeyUsage x509.ExtKeyUsage, caPEM string, certPEM string) *ValidCert { block, _ := pem.Decode([]byte(certPEM)) require.NotNil(t, block) parsed, err := x509.ParseCertificate(block.Bytes) @@ -37,7 +46,10 @@ func ValidateCertificate(t *testing.T, caPEM string, certPEM string) *ValidCert // Validate the created cert using the CA. roots := x509.NewCertPool() require.True(t, roots.AppendCertsFromPEM([]byte(caPEM))) - opts := x509.VerifyOptions{Roots: roots} + opts := x509.VerifyOptions{ + Roots: roots, + KeyUsages: []x509.ExtKeyUsage{extKeyUsage}, + } _, err = parsed.Verify(opts) require.NoError(t, err) @@ -61,6 +73,35 @@ func (v *ValidCert) RequireDNSName(expectDNSName string) { require.Contains(v.t, v.parsed.DNSNames, expectDNSName, "expected an explicit DNS SAN, not just Common Name") } +func (v *ValidCert) RequireDNSNames(names []string) { + v.t.Helper() + require.Equal(v.t, names, v.parsed.DNSNames) +} + +func (v *ValidCert) RequireEmptyDNSNames() { + v.t.Helper() + require.Empty(v.t, v.parsed.DNSNames) +} + +func (v *ValidCert) RequireIPs(ips []net.IP) { + v.t.Helper() + actualIPs := v.parsed.IPAddresses + actualIPsStrings := make([]string, len(actualIPs)) + for i := range actualIPs { + actualIPsStrings[i] = actualIPs[i].String() + } + expectedIPsStrings := make([]string, len(ips)) + for i := range ips { + expectedIPsStrings[i] = ips[i].String() + } + require.Equal(v.t, expectedIPsStrings, actualIPsStrings) +} + +func (v *ValidCert) RequireEmptyIPs() { + v.t.Helper() + require.Empty(v.t, v.parsed.IPAddresses) +} + // RequireLifetime asserts that the lifetime of the certificate matches the expected timestamps. func (v *ValidCert) RequireLifetime(expectNotBefore time.Time, expectNotAfter time.Time, delta time.Duration) { v.t.Helper() @@ -81,6 +122,11 @@ func (v *ValidCert) RequireCommonName(commonName string) { require.Equal(v.t, commonName, v.parsed.Subject.CommonName) } +func (v *ValidCert) RequireOrganizations(orgs []string) { + v.t.Helper() + require.Equal(v.t, orgs, v.parsed.Subject.Organization) +} + // CreateCertificate creates a certificate with the provided time bounds, and returns the PEM // representation of the certificate and its private key. The returned certificate is capable of // signing child certificates. diff --git a/pkg/conciergeclient/conciergeclient_test.go b/pkg/conciergeclient/conciergeclient_test.go index 162ecfee..6787cc92 100644 --- a/pkg/conciergeclient/conciergeclient_test.go +++ b/pkg/conciergeclient/conciergeclient_test.go @@ -5,7 +5,6 @@ package conciergeclient import ( "context" - "crypto/x509/pkix" "encoding/base64" "encoding/json" "fmt" @@ -26,7 +25,7 @@ import ( func TestNew(t *testing.T) { t.Parallel() - testCA, err := certauthority.New(pkix.Name{}, 1*time.Hour) + testCA, err := certauthority.New("Test CA", 1*time.Hour) require.NoError(t, err) tests := []struct { diff --git a/test/integration/e2e_test.go b/test/integration/e2e_test.go index dfa4e2a0..b382d85f 100644 --- a/test/integration/e2e_test.go +++ b/test/integration/e2e_test.go @@ -6,7 +6,6 @@ import ( "bufio" "bytes" "context" - "crypto/x509/pkix" "encoding/base64" "errors" "fmt" @@ -69,7 +68,7 @@ func TestE2EFullIntegration(t *testing.T) { // Generate a CA bundle with which to serve this provider. t.Logf("generating test CA") - ca, err := certauthority.New(pkix.Name{CommonName: "Downstream Test CA"}, 1*time.Hour) + ca, err := certauthority.New("Downstream Test CA", 1*time.Hour) require.NoError(t, err) // Save that bundle plus the one that signs the upstream issuer, for test purposes. @@ -80,12 +79,7 @@ func TestE2EFullIntegration(t *testing.T) { // Use the CA to issue a TLS server cert. t.Logf("issuing test certificate") - tlsCert, err := ca.Issue( - pkix.Name{CommonName: issuerURL.Hostname()}, - []string{issuerURL.Hostname()}, - nil, - 1*time.Hour, - ) + tlsCert, err := ca.IssueServerCert([]string{issuerURL.Hostname()}, nil, 1*time.Hour) require.NoError(t, err) certPEM, keyPEM, err := certauthority.ToPEM(tlsCert) require.NoError(t, err) diff --git a/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go index 8712a316..d076f828 100644 --- a/test/integration/supervisor_discovery_test.go +++ b/test/integration/supervisor_discovery_test.go @@ -7,7 +7,6 @@ import ( "context" "crypto/tls" "crypto/x509" - "crypto/x509/pkix" "encoding/json" "fmt" "io/ioutil" @@ -288,11 +287,11 @@ func defaultTLSCertSecretName(env *library.TestEnv) string { func createTLSCertificateSecret(ctx context.Context, t *testing.T, ns string, hostname string, ips []net.IP, secretName string, kubeClient kubernetes.Interface) *certauthority.CA { // Create a CA. - ca, err := certauthority.New(pkix.Name{CommonName: "Acme Corp"}, 1000*time.Hour) + ca, err := certauthority.New("Acme Corp", 1000*time.Hour) require.NoError(t, err) // Using the CA, create a TLS server cert. - tlsCert, err := ca.Issue(pkix.Name{CommonName: hostname}, []string{hostname}, ips, 1000*time.Hour) + tlsCert, err := ca.IssueServerCert([]string{hostname}, ips, 1000*time.Hour) require.NoError(t, err) // Write the serving cert to the SNI secret. diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 9d6b6787..fe2060ea 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -6,7 +6,6 @@ package integration import ( "context" "crypto/tls" - "crypto/x509/pkix" "encoding/base64" "encoding/json" "fmt" @@ -58,7 +57,7 @@ func TestSupervisorLogin(t *testing.T) { // Generate a CA bundle with which to serve this provider. t.Logf("generating test CA") - ca, err := certauthority.New(pkix.Name{CommonName: "Downstream Test CA"}, 1*time.Hour) + ca, err := certauthority.New("Downstream Test CA", 1*time.Hour) require.NoError(t, err) // Create an HTTP client that can reach the downstream discovery endpoint using the CA certs. @@ -85,12 +84,7 @@ func TestSupervisorLogin(t *testing.T) { // Use the CA to issue a TLS server cert. t.Logf("issuing test certificate") - tlsCert, err := ca.Issue( - pkix.Name{CommonName: issuerURL.Hostname()}, - []string{issuerURL.Hostname()}, - nil, - 1*time.Hour, - ) + tlsCert, err := ca.IssueServerCert([]string{issuerURL.Hostname()}, nil, 1*time.Hour) require.NoError(t, err) certPEM, keyPEM, err := certauthority.ToPEM(tlsCert) require.NoError(t, err)