From ae5aad178d2efb577f0816186dc508da915f1338 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 21 Nov 2024 15:18:43 -0800 Subject: [PATCH] TokenCredentialRequest uses actual cert expiry time instead of estimate and also audit logs both the NotBefore and NotAfter of the issued cert. Implemented by changing the return type of the cert issuer helpers to make them also return the NotBefore and NotAfter values of the new cert, along with the key PEM and cert PEM. --- internal/cert/pem.go | 13 ++++ internal/certauthority/certauthority.go | 34 +++++---- internal/certauthority/certauthority_test.go | 60 ++++++++------- .../dynamiccertauthority.go | 9 ++- .../dynamiccertauthority_test.go | 20 ++--- internal/clientcertissuer/issuer.go | 13 ++-- internal/clientcertissuer/issuer_test.go | 22 +++--- internal/concierge/apiserver/apiserver.go | 2 - .../impersonator/impersonator_test.go | 10 +-- .../apicerts/certs_observer_test.go | 12 +-- .../webhookcachefiller_test.go | 4 +- .../github_upstream_watcher_test.go | 6 +- internal/dynamiccert/provider_test.go | 16 ++-- internal/mocks/mockissuer/mockissuer.go | 10 +-- internal/registry/credentialrequest/rest.go | 22 +++--- .../registry/credentialrequest/rest_test.go | 74 +++++++++++-------- site/content/docs/reference/audit-logging.md | 5 +- test/integration/audit_test.go | 12 +-- .../concierge_impersonation_proxy_test.go | 14 ++-- 19 files changed, 199 insertions(+), 159 deletions(-) create mode 100644 internal/cert/pem.go diff --git a/internal/cert/pem.go b/internal/cert/pem.go new file mode 100644 index 000000000..1a6b4d458 --- /dev/null +++ b/internal/cert/pem.go @@ -0,0 +1,13 @@ +// Copyright 2024 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package cert + +import "time" + +type PEM struct { + CertPEM []byte + KeyPEM []byte + NotBefore time.Time + NotAfter time.Time +} diff --git a/internal/certauthority/certauthority.go b/internal/certauthority/certauthority.go index ab9e086d2..76ef5d97f 100644 --- a/internal/certauthority/certauthority.go +++ b/internal/certauthority/certauthority.go @@ -1,4 +1,4 @@ -// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2024 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. @@ -20,6 +20,7 @@ import ( "net" "time" + "go.pinniped.dev/internal/cert" "go.pinniped.dev/internal/constable" ) @@ -38,7 +39,7 @@ type env struct { // clock tells the current time (usually time.Now(), but broken out here for tests). clock func() time.Time - // parse function to parse an ASN.1 byte slice into an x509 struct (normally x509.ParseCertificate) + // parse function to parse an ASN.1 byte slice into a x509 struct (normally x509.ParseCertificate) parseCert func([]byte) (*x509.Certificate, error) } @@ -180,19 +181,19 @@ func (c *CA) IssueServerCert(dnsNames []string, ips []net.IP, ttl time.Duration) } // 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) { +// for the certificate and private key, along with the notBefore and notAfter values. +func (c *CA) IssueClientCertPEM(username string, groups []string, ttl time.Duration) (*cert.PEM, error) { return toPEM(c.IssueClientCert(username, groups, ttl)) } // 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) { +// for the certificate and private key, along with the notBefore and notAfter values. +func (c *CA) IssueServerCertPEM(dnsNames []string, ips []net.IP, ttl time.Duration) (*cert.PEM, 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. + // Choose a random 128-bit serial number. serialNumber, err := randomSerial(c.env.serialRNG) if err != nil { return nil, fmt.Errorf("could not generate serial number for certificate: %w", err) @@ -209,7 +210,7 @@ func (c *CA) issueCert(extKeyUsage x509.ExtKeyUsage, subject pkix.Name, dnsNames notBefore := now.Add(-certBackdate) notAfter := now.Add(ttl) - // Parse the DER encoded certificate to get an x509.Certificate. + // Parse the DER encoded certificate to get a x509.Certificate. caCert, err := x509.ParseCertificate(c.caCertBytes) if err != nil { return nil, fmt.Errorf("could not parse CA certificate: %w", err) @@ -246,18 +247,23 @@ func (c *CA) issueCert(extKeyUsage x509.ExtKeyUsage, subject pkix.Name, dnsNames }, nil } -func toPEM(cert *tls.Certificate, err error) ([]byte, []byte, error) { +func toPEM(certificate *tls.Certificate, err error) (*cert.PEM, error) { // If the wrapped IssueServerCert() returned an error, pass it back. if err != nil { - return nil, nil, err + return nil, err } - certPEM, keyPEM, err := ToPEM(cert) + certPEM, keyPEM, err := ToPEM(certificate) if err != nil { - return nil, nil, err + return nil, err } - return certPEM, keyPEM, nil + return &cert.PEM{ + CertPEM: certPEM, + KeyPEM: keyPEM, + NotBefore: certificate.Leaf.NotBefore, + NotAfter: certificate.Leaf.NotAfter, + }, nil } // ToPEM encodes a tls.Certificate into a private key PEM and a cert chain PEM. @@ -279,7 +285,7 @@ func ToPEM(cert *tls.Certificate) ([]byte, []byte, error) { return certPEM, keyPEM, nil } -// randomSerial generates a random 128 bit serial number. +// randomSerial generates a random 128-bit serial number. func randomSerial(rng io.Reader) (*big.Int, error) { return rand.Int(rng, new(big.Int).Lsh(big.NewInt(1), 128)) } diff --git a/internal/certauthority/certauthority_test.go b/internal/certauthority/certauthority_test.go index 0128b1ad0..b59eeb0f5 100644 --- a/internal/certauthority/certauthority_test.go +++ b/internal/certauthority/certauthority_test.go @@ -228,7 +228,7 @@ 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) + now := time.Date(2020, 7, 10, 12, 41, 12, 0, time.UTC) realCA, err := Load(testCert, testKey) require.NoError(t, err) @@ -323,6 +323,8 @@ func TestIssue(t *testing.T) { } else { require.NoError(t, err) require.NotNil(t, got) + require.Equal(t, now.Add(-5*time.Minute), got.Leaf.NotBefore) // always back-dated + require.Equal(t, now.Add(10*time.Minute), got.Leaf.NotAfter) } got, err = tt.ca.IssueClientCert("test-user", []string{"group1", "group2"}, 10*time.Minute) if tt.wantErr != "" { @@ -331,6 +333,8 @@ func TestIssue(t *testing.T) { } else { require.NoError(t, err) require.NotNil(t, got) + require.Equal(t, now.Add(-5*time.Minute), got.Leaf.NotBefore) // always back-dated + require.Equal(t, now.Add(10*time.Minute), got.Leaf.NotAfter) } }) } @@ -341,26 +345,26 @@ func TestToPEM(t *testing.T) { require.NoError(t, err) t.Run("error from input", func(t *testing.T) { - certPEM, keyPEM, err := toPEM(nil, fmt.Errorf("some error")) + pem, err := toPEM(nil, fmt.Errorf("some error")) require.EqualError(t, err, "some error") - require.Nil(t, certPEM) - require.Nil(t, keyPEM) + require.Nil(t, pem) }) t.Run("invalid private key", func(t *testing.T) { cert := realCert cert.PrivateKey = nil - certPEM, keyPEM, err := toPEM(&cert, nil) + pem, err := toPEM(&cert, nil) require.EqualError(t, err, "failed to marshal private key into PKCS8: x509: unknown key type while marshaling PKCS#8: ") - require.Nil(t, certPEM) - require.Nil(t, keyPEM) + require.Nil(t, pem) }) t.Run("success", func(t *testing.T) { - certPEM, keyPEM, err := toPEM(&realCert, nil) + pem, err := toPEM(&realCert, nil) require.NoError(t, err) - require.NotEmpty(t, certPEM) - require.NotEmpty(t, keyPEM) + require.NotEmpty(t, pem.CertPEM) + require.NotEmpty(t, pem.KeyPEM) + require.Equal(t, time.Date(2020, time.July, 25, 21, 4, 18, 0, time.UTC), pem.NotBefore) + require.Equal(t, time.Date(2030, time.July, 23, 21, 4, 18, 0, time.UTC), pem.NotAfter) }) } @@ -381,21 +385,21 @@ func TestIssueMethods(t *testing.T) { require.NoError(t, err) validateClientCert(t, ca.Bundle(), certPEM, keyPEM, user, groups, ttl) - certPEM, keyPEM, err = ca.IssueClientCertPEM(user, groups, ttl) + pem, err := ca.IssueClientCertPEM(user, groups, ttl) require.NoError(t, err) - validateClientCert(t, ca.Bundle(), certPEM, keyPEM, user, groups, ttl) + validateClientCert(t, ca.Bundle(), pem.CertPEM, pem.KeyPEM, user, groups, ttl) - certPEM, keyPEM, err = ca.IssueClientCertPEM(user, nil, ttl) + pem, err = ca.IssueClientCertPEM(user, nil, ttl) require.NoError(t, err) - validateClientCert(t, ca.Bundle(), certPEM, keyPEM, user, nil, ttl) + validateClientCert(t, ca.Bundle(), pem.CertPEM, pem.KeyPEM, user, nil, ttl) - certPEM, keyPEM, err = ca.IssueClientCertPEM(user, []string{}, ttl) + pem, err = ca.IssueClientCertPEM(user, []string{}, ttl) require.NoError(t, err) - validateClientCert(t, ca.Bundle(), certPEM, keyPEM, user, nil, ttl) + validateClientCert(t, ca.Bundle(), pem.CertPEM, pem.KeyPEM, user, nil, ttl) - certPEM, keyPEM, err = ca.IssueClientCertPEM("", []string{}, ttl) + pem, err = ca.IssueClientCertPEM("", []string{}, ttl) require.NoError(t, err) - validateClientCert(t, ca.Bundle(), certPEM, keyPEM, "", nil, ttl) + validateClientCert(t, ca.Bundle(), pem.CertPEM, pem.KeyPEM, "", nil, ttl) }) t.Run("server certs", func(t *testing.T) { @@ -408,25 +412,25 @@ func TestIssueMethods(t *testing.T) { require.NoError(t, err) validateServerCert(t, ca.Bundle(), certPEM, keyPEM, dnsNames, ips, ttl) - certPEM, keyPEM, err = ca.IssueServerCertPEM(dnsNames, ips, ttl) + pem, err := ca.IssueServerCertPEM(dnsNames, ips, ttl) require.NoError(t, err) - validateServerCert(t, ca.Bundle(), certPEM, keyPEM, dnsNames, ips, ttl) + validateServerCert(t, ca.Bundle(), pem.CertPEM, pem.KeyPEM, dnsNames, ips, ttl) - certPEM, keyPEM, err = ca.IssueServerCertPEM(nil, ips, ttl) + pem, err = ca.IssueServerCertPEM(nil, ips, ttl) require.NoError(t, err) - validateServerCert(t, ca.Bundle(), certPEM, keyPEM, nil, ips, ttl) + validateServerCert(t, ca.Bundle(), pem.CertPEM, pem.KeyPEM, nil, ips, ttl) - certPEM, keyPEM, err = ca.IssueServerCertPEM(dnsNames, nil, ttl) + pem, err = ca.IssueServerCertPEM(dnsNames, nil, ttl) require.NoError(t, err) - validateServerCert(t, ca.Bundle(), certPEM, keyPEM, dnsNames, nil, ttl) + validateServerCert(t, ca.Bundle(), pem.CertPEM, pem.KeyPEM, dnsNames, nil, ttl) - certPEM, keyPEM, err = ca.IssueServerCertPEM([]string{}, ips, ttl) + pem, err = ca.IssueServerCertPEM([]string{}, ips, ttl) require.NoError(t, err) - validateServerCert(t, ca.Bundle(), certPEM, keyPEM, nil, ips, ttl) + validateServerCert(t, ca.Bundle(), pem.CertPEM, pem.KeyPEM, nil, ips, ttl) - certPEM, keyPEM, err = ca.IssueServerCertPEM(dnsNames, []net.IP{}, ttl) + pem, err = ca.IssueServerCertPEM(dnsNames, []net.IP{}, ttl) require.NoError(t, err) - validateServerCert(t, ca.Bundle(), certPEM, keyPEM, dnsNames, nil, ttl) + validateServerCert(t, ca.Bundle(), pem.CertPEM, pem.KeyPEM, dnsNames, nil, ttl) }) } diff --git a/internal/certauthority/dynamiccertauthority/dynamiccertauthority.go b/internal/certauthority/dynamiccertauthority/dynamiccertauthority.go index e75c7a257..197112797 100644 --- a/internal/certauthority/dynamiccertauthority/dynamiccertauthority.go +++ b/internal/certauthority/dynamiccertauthority/dynamiccertauthority.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package dynamiccertauthority implements a x509 certificate authority capable of issuing @@ -10,6 +10,7 @@ import ( "k8s.io/apiserver/pkg/server/dynamiccertificates" + "go.pinniped.dev/internal/cert" "go.pinniped.dev/internal/certauthority" "go.pinniped.dev/internal/clientcertissuer" ) @@ -32,15 +33,15 @@ func (c *ca) Name() string { } // 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) IssueClientCertPEM(username string, groups []string, ttl time.Duration) ([]byte, []byte, error) { +// pair of PEM-formatted byte slices for the certificate and private key, along with the notBefore and notAfter values. +func (c *ca) IssueClientCertPEM(username string, groups []string, ttl time.Duration) (*cert.PEM, error) { caCrtPEM, caKeyPEM := c.provider.CurrentCertKeyContent() // in the future we could split dynamiccert.Private into two interfaces (Private and PrivateRead) // and have this code take PrivateRead as input. We would then add ourselves as a listener to // the PrivateRead. This would allow us to only reload the CA contents when they actually change. ca, err := certauthority.Load(string(caCrtPEM), string(caKeyPEM)) if err != nil { - return nil, nil, err + return nil, err } return ca.IssueClientCertPEM(username, groups, ttl) diff --git a/internal/certauthority/dynamiccertauthority/dynamiccertauthority_test.go b/internal/certauthority/dynamiccertauthority/dynamiccertauthority_test.go index 665f6d191..34935e845 100644 --- a/internal/certauthority/dynamiccertauthority/dynamiccertauthority_test.go +++ b/internal/certauthority/dynamiccertauthority/dynamiccertauthority_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/require" + "go.pinniped.dev/internal/cert" "go.pinniped.dev/internal/clientcertissuer" "go.pinniped.dev/internal/dynamiccert" "go.pinniped.dev/internal/testutil" @@ -93,36 +94,35 @@ func TestCAIssuePEM(t *testing.T) { // Can't run these steps in parallel, because each one depends on the previous steps being // run. - crtPEM, keyPEM, err := issuePEM(provider, ca, step.caCrtPEM, step.caKeyPEM) + pem, err := issuePEM(provider, ca, step.caCrtPEM, step.caKeyPEM) if step.wantError != "" { require.EqualError(t, err, step.wantError) - require.Empty(t, crtPEM) - require.Empty(t, keyPEM) + require.Nil(t, pem) } else { require.NoError(t, err) - require.NotEmpty(t, crtPEM) - require.NotEmpty(t, keyPEM) + require.NotEmpty(t, pem.CertPEM) + require.NotEmpty(t, pem.KeyPEM) caCrtPEM, _ := provider.CurrentCertKeyContent() - crtAssertions := testutil.ValidateClientCertificate(t, string(caCrtPEM), string(crtPEM)) + crtAssertions := testutil.ValidateClientCertificate(t, string(caCrtPEM), string(pem.CertPEM)) 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)) + crtAssertions.RequireMatchesPrivateKey(string(pem.KeyPEM)) } }) } } -func issuePEM(provider dynamiccert.Provider, ca clientcertissuer.ClientCertIssuer, caCrt, caKey []byte) ([]byte, []byte, error) { +func issuePEM(provider dynamiccert.Provider, ca clientcertissuer.ClientCertIssuer, caCrt, caKey []byte) (*cert.PEM, error) { // if setting fails, look at that error if caCrt != nil || caKey != nil { if err := provider.SetCertKeyContent(caCrt, caKey); err != nil { - return nil, nil, err + return nil, err } } - // otherwise check to see if their is an issuing error + // otherwise check to see if there is an issuing error return ca.IssueClientCertPEM("some-username", []string{"some-group1", "some-group2"}, time.Hour*24) } diff --git a/internal/clientcertissuer/issuer.go b/internal/clientcertissuer/issuer.go index f84f7beda..3d7901051 100644 --- a/internal/clientcertissuer/issuer.go +++ b/internal/clientcertissuer/issuer.go @@ -10,6 +10,7 @@ import ( utilerrors "k8s.io/apimachinery/pkg/util/errors" + "go.pinniped.dev/internal/cert" "go.pinniped.dev/internal/constable" ) @@ -17,7 +18,7 @@ const defaultCertIssuerErr = constable.Error("failed to issue cert") type ClientCertIssuer interface { Name() string - IssueClientCertPEM(username string, groups []string, ttl time.Duration) (certPEM, keyPEM []byte, err error) + IssueClientCertPEM(username string, groups []string, ttl time.Duration) (pem *cert.PEM, err error) } var _ ClientCertIssuer = ClientCertIssuers{} @@ -37,20 +38,20 @@ func (c ClientCertIssuers) Name() string { return strings.Join(names, ",") } -func (c ClientCertIssuers) IssueClientCertPEM(username string, groups []string, ttl time.Duration) ([]byte, []byte, error) { +func (c ClientCertIssuers) IssueClientCertPEM(username string, groups []string, ttl time.Duration) (*cert.PEM, error) { errs := make([]error, 0, len(c)) for _, issuer := range c { - certPEM, keyPEM, err := issuer.IssueClientCertPEM(username, groups, ttl) + pem, err := issuer.IssueClientCertPEM(username, groups, ttl) if err == nil { - return certPEM, keyPEM, nil + return pem, nil } errs = append(errs, fmt.Errorf("%s failed to issue client cert: %w", issuer.Name(), err)) } if err := utilerrors.NewAggregate(errs); err != nil { - return nil, nil, err + return nil, err } - return nil, nil, defaultCertIssuerErr + return nil, defaultCertIssuerErr } diff --git a/internal/clientcertissuer/issuer_test.go b/internal/clientcertissuer/issuer_test.go index 81a615971..a14e08607 100644 --- a/internal/clientcertissuer/issuer_test.go +++ b/internal/clientcertissuer/issuer_test.go @@ -11,6 +11,7 @@ import ( "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" + "go.pinniped.dev/internal/cert" "go.pinniped.dev/internal/mocks/mockissuer" ) @@ -85,7 +86,7 @@ func TestIssueClientCertPEM(t *testing.T) { errClientCertIssuer.EXPECT().Name().Return("error cert issuer") errClientCertIssuer.EXPECT(). IssueClientCertPEM("username", []string{"group1", "group2"}, 32*time.Second). - Return(nil, nil, errors.New("error from wrapped cert issuer")) + Return(nil, errors.New("error from wrapped cert issuer")) return ClientCertIssuers{errClientCertIssuer} }, wantErrorMessage: "error cert issuer failed to issue client cert: error from wrapped cert issuer", @@ -96,7 +97,7 @@ func TestIssueClientCertPEM(t *testing.T) { validClientCertIssuer := mockissuer.NewMockClientCertIssuer(ctrl) validClientCertIssuer.EXPECT(). IssueClientCertPEM("username", []string{"group1", "group2"}, 32*time.Second). - Return([]byte("cert"), []byte("key"), nil) + Return(&cert.PEM{CertPEM: []byte("cert"), KeyPEM: []byte("key")}, nil) return ClientCertIssuers{validClientCertIssuer} }, wantCert: []byte("cert"), @@ -109,12 +110,12 @@ func TestIssueClientCertPEM(t *testing.T) { errClientCertIssuer.EXPECT().Name().Return("error cert issuer") errClientCertIssuer.EXPECT(). IssueClientCertPEM("username", []string{"group1", "group2"}, 32*time.Second). - Return(nil, nil, errors.New("error from wrapped cert issuer")) + Return(nil, errors.New("error from wrapped cert issuer")) validClientCertIssuer := mockissuer.NewMockClientCertIssuer(ctrl) validClientCertIssuer.EXPECT(). IssueClientCertPEM("username", []string{"group1", "group2"}, 32*time.Second). - Return([]byte("cert"), []byte("key"), nil) + Return(&cert.PEM{CertPEM: []byte("cert"), KeyPEM: []byte("key")}, nil) return ClientCertIssuers{ errClientCertIssuer, validClientCertIssuer, @@ -130,13 +131,13 @@ func TestIssueClientCertPEM(t *testing.T) { err1ClientCertIssuer.EXPECT().Name().Return("error1 cert issuer") err1ClientCertIssuer.EXPECT(). IssueClientCertPEM("username", []string{"group1", "group2"}, 32*time.Second). - Return(nil, nil, errors.New("error1 from wrapped cert issuer")) + Return(nil, errors.New("error1 from wrapped cert issuer")) err2ClientCertIssuer := mockissuer.NewMockClientCertIssuer(ctrl) err2ClientCertIssuer.EXPECT().Name().Return("error2 cert issuer") err2ClientCertIssuer.EXPECT(). IssueClientCertPEM("username", []string{"group1", "group2"}, 32*time.Second). - Return(nil, nil, errors.New("error2 from wrapped cert issuer")) + Return(nil, errors.New("error2 from wrapped cert issuer")) return ClientCertIssuers{ err1ClientCertIssuer, @@ -152,17 +153,16 @@ func TestIssueClientCertPEM(t *testing.T) { t.Run(testcase.name, func(t *testing.T) { t.Parallel() - certPEM, keyPEM, err := testcase.buildIssuerMocks(). + pem, err := testcase.buildIssuerMocks(). IssueClientCertPEM("username", []string{"group1", "group2"}, 32*time.Second) if testcase.wantErrorMessage != "" { require.ErrorContains(t, err, testcase.wantErrorMessage) - require.Empty(t, certPEM) - require.Empty(t, keyPEM) + require.Nil(t, pem) } else { require.NoError(t, err) - require.Equal(t, testcase.wantCert, certPEM) - require.Equal(t, testcase.wantKey, keyPEM) + require.Equal(t, testcase.wantCert, pem.CertPEM) + require.Equal(t, testcase.wantKey, pem.KeyPEM) } }) } diff --git a/internal/concierge/apiserver/apiserver.go b/internal/concierge/apiserver/apiserver.go index b58a4b2c2..88d38f922 100644 --- a/internal/concierge/apiserver/apiserver.go +++ b/internal/concierge/apiserver/apiserver.go @@ -15,7 +15,6 @@ import ( "k8s.io/apiserver/pkg/registry/rest" genericapiserver "k8s.io/apiserver/pkg/server" utilversion "k8s.io/apiserver/pkg/util/version" - "k8s.io/utils/clock" "go.pinniped.dev/internal/clientcertissuer" "go.pinniped.dev/internal/controllerinit" @@ -89,7 +88,6 @@ func (c completedConfig) New() (*PinnipedServer, error) { c.ExtraConfig.Issuer, tokenCredReqGVR.GroupResource(), c.ExtraConfig.AuditLogger, - clock.RealClock{}, ) return tokenCredReqGVR, tokenCredStorage }, diff --git a/internal/concierge/impersonator/impersonator_test.go b/internal/concierge/impersonator/impersonator_test.go index ede5ba385..aa49eaed2 100644 --- a/internal/concierge/impersonator/impersonator_test.go +++ b/internal/concierge/impersonator/impersonator_test.go @@ -70,10 +70,10 @@ func TestImpersonator(t *testing.T) { err = caContent.SetCertKeyContent(ca.Bundle(), caKey) require.NoError(t, err) - cert, key, err := ca.IssueServerCertPEM(nil, []net.IP{net.ParseIP("127.0.0.1")}, time.Hour) + pem, err := ca.IssueServerCertPEM(nil, []net.IP{net.ParseIP("127.0.0.1")}, time.Hour) require.NoError(t, err) certKeyContent := dynamiccert.NewServingCert("cert-key") - err = certKeyContent.SetCertKeyContent(cert, key) + err = certKeyContent.SetCertKeyContent(pem.CertPEM, pem.KeyPEM) require.NoError(t, err) unrelatedCA, err := certauthority.New("ca", time.Hour) @@ -1997,11 +1997,11 @@ type clientCert struct { func newClientCert(t *testing.T, ca *certauthority.CA, username string, groups []string) *clientCert { t.Helper() - certPEM, keyPEM, err := ca.IssueClientCertPEM(username, groups, time.Hour) + pem, err := ca.IssueClientCertPEM(username, groups, time.Hour) require.NoError(t, err) return &clientCert{ - certPEM: certPEM, - keyPEM: keyPEM, + certPEM: pem.CertPEM, + keyPEM: pem.KeyPEM, } } diff --git a/internal/controller/apicerts/certs_observer_test.go b/internal/controller/apicerts/certs_observer_test.go index 245fba1f0..ccb11929e 100644 --- a/internal/controller/apicerts/certs_observer_test.go +++ b/internal/controller/apicerts/certs_observer_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package apicerts @@ -173,10 +173,10 @@ func TestObserverControllerSync(t *testing.T) { ca, err := certauthority.Load(string(caCrt), string(caKey)) require.NoError(t, err) - crt, key, err := ca.IssueServerCertPEM(nil, nil, time.Hour) + pem, err := ca.IssueServerCertPEM(nil, nil, time.Hour) require.NoError(t, err) - err = dynamicCertProvider.SetCertKeyContent(crt, key) + err = dynamicCertProvider.SetCertKeyContent(pem.CertPEM, pem.KeyPEM) r.NoError(err) }) @@ -202,7 +202,7 @@ func TestObserverControllerSync(t *testing.T) { ca, err := certauthority.Load(string(caCrt), string(caKey)) require.NoError(t, err) - crt, key, err := ca.IssueServerCertPEM(nil, nil, time.Hour) + pem, err := ca.IssueServerCertPEM(nil, nil, time.Hour) require.NoError(t, err) apiServingCertSecret := &corev1.Secret{ @@ -212,8 +212,8 @@ func TestObserverControllerSync(t *testing.T) { }, Data: map[string][]byte{ "caCertificate": []byte("fake cert"), - "tlsPrivateKey": key, - "tlsCertificateChain": crt, + "tlsPrivateKey": pem.KeyPEM, + "tlsCertificateChain": pem.CertPEM, }, } err = kubeInformerClient.Tracker().Add(apiServingCertSecret) diff --git a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go index b1654be82..89ce4cdaa 100644 --- a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go +++ b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go @@ -79,7 +79,7 @@ func TestController(t *testing.T) { require.NoError(t, err) someUnknownHostNames := []string{"some-dns-name", "some-other-dns-name"} someLocalIPAddress := []net.IP{net.ParseIP("10.2.3.4")} - pemServerCertForUnknownServer, _, err := caForUnknownServer.IssueServerCertPEM( + pemServerCertForUnknownServer, err := caForUnknownServer.IssueServerCertPEM( someUnknownHostNames, someLocalIPAddress, time.Hour, @@ -216,7 +216,7 @@ func TestController(t *testing.T) { badWebhookAuthenticatorSpecGoodEndpointButUnknownCA := authenticationv1alpha1.WebhookAuthenticatorSpec{ Endpoint: goodWebhookDefaultServingCertEndpoint, TLS: &authenticationv1alpha1.TLSSpec{ - CertificateAuthorityData: base64.StdEncoding.EncodeToString(pemServerCertForUnknownServer), + CertificateAuthorityData: base64.StdEncoding.EncodeToString(pemServerCertForUnknownServer.CertPEM), }, } diff --git a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go index eb4325c9c..c65b300fb 100644 --- a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go @@ -127,7 +127,7 @@ func TestController(t *testing.T) { caForUnknownServer, err := certauthority.New("Some Unknown CA", time.Hour) require.NoError(t, err) - unknownServerCABytes, _, err := caForUnknownServer.IssueServerCertPEM( + unknownServerPEM, err := caForUnknownServer.IssueServerCertPEM( []string{"some-dns-name", "some-other-dns-name"}, []net.IP{net.ParseIP("10.2.3.4")}, time.Hour, @@ -1849,7 +1849,7 @@ func TestController(t *testing.T) { func() runtime.Object { badIDP := validFilledOutIDP.DeepCopy() badIDP.Spec.GitHubAPI.TLS = &idpv1alpha1.TLSSpec{ - CertificateAuthorityData: base64.StdEncoding.EncodeToString(unknownServerCABytes), + CertificateAuthorityData: base64.StdEncoding.EncodeToString(unknownServerPEM.CertPEM), } return badIDP }(), @@ -1861,7 +1861,7 @@ func TestController(t *testing.T) { Spec: func() idpv1alpha1.GitHubIdentityProviderSpec { badSpec := validFilledOutIDP.Spec.DeepCopy() badSpec.GitHubAPI.TLS = &idpv1alpha1.TLSSpec{ - CertificateAuthorityData: base64.StdEncoding.EncodeToString(unknownServerCABytes), + CertificateAuthorityData: base64.StdEncoding.EncodeToString(unknownServerPEM.CertPEM), } return *badSpec }(), diff --git a/internal/dynamiccert/provider_test.go b/internal/dynamiccert/provider_test.go index 385adc364..ca55f5d8e 100644 --- a/internal/dynamiccert/provider_test.go +++ b/internal/dynamiccert/provider_test.go @@ -114,13 +114,13 @@ func TestProviderWithDynamicServingCertificateController(t *testing.T) { newCA, err := certauthority.New(names.SimpleNameGenerator.GenerateName("new-ca"), time.Hour) require.NoError(t, err) - certPEM, keyPEM, err := newCA.IssueServerCertPEM(nil, []net.IP{net.ParseIP("127.0.0.2")}, time.Hour) + pem, err := newCA.IssueServerCertPEM(nil, []net.IP{net.ParseIP("127.0.0.2")}, time.Hour) require.NoError(t, err) - err = certKey.SetCertKeyContent(certPEM, keyPEM) + err = certKey.SetCertKeyContent(pem.CertPEM, pem.KeyPEM) require.NoError(t, err) - cert, err := tls.X509KeyPair(certPEM, keyPEM) + cert, err := tls.X509KeyPair(pem.CertPEM, pem.KeyPEM) require.NoError(t, err) return []tls.Certificate{cert} @@ -144,13 +144,13 @@ func TestProviderWithDynamicServingCertificateController(t *testing.T) { newCA, err := certauthority.New(names.SimpleNameGenerator.GenerateName("new-ca"), time.Hour) require.NoError(t, err) - certPEM, keyPEM, err := newCA.IssueServerCertPEM(nil, []net.IP{net.ParseIP("127.0.0.3")}, time.Hour) + pem, err := newCA.IssueServerCertPEM(nil, []net.IP{net.ParseIP("127.0.0.3")}, time.Hour) require.NoError(t, err) - err = certKey.SetCertKeyContent(certPEM, keyPEM) + err = certKey.SetCertKeyContent(pem.CertPEM, pem.KeyPEM) require.NoError(t, err) - cert, err := tls.X509KeyPair(certPEM, keyPEM) + cert, err := tls.X509KeyPair(pem.CertPEM, pem.KeyPEM) require.NoError(t, err) return []tls.Certificate{cert} @@ -170,10 +170,10 @@ func TestProviderWithDynamicServingCertificateController(t *testing.T) { err = caContent.SetCertKeyContent(ca.Bundle(), caKey) require.NoError(t, err) - cert, key, err := ca.IssueServerCertPEM(nil, []net.IP{net.ParseIP("127.0.0.1")}, time.Hour) + pem, err := ca.IssueServerCertPEM(nil, []net.IP{net.ParseIP("127.0.0.1")}, time.Hour) require.NoError(t, err) certKeyContent := NewServingCert("cert-key") - err = certKeyContent.SetCertKeyContent(cert, key) + err = certKeyContent.SetCertKeyContent(pem.CertPEM, pem.KeyPEM) require.NoError(t, err) tlsConfig := ptls.Default(nil) diff --git a/internal/mocks/mockissuer/mockissuer.go b/internal/mocks/mockissuer/mockissuer.go index 1950e3dd8..4a22d01de 100644 --- a/internal/mocks/mockissuer/mockissuer.go +++ b/internal/mocks/mockissuer/mockissuer.go @@ -17,6 +17,7 @@ import ( reflect "reflect" time "time" + cert "go.pinniped.dev/internal/cert" gomock "go.uber.org/mock/gomock" ) @@ -45,13 +46,12 @@ func (m *MockClientCertIssuer) EXPECT() *MockClientCertIssuerMockRecorder { } // IssueClientCertPEM mocks base method. -func (m *MockClientCertIssuer) IssueClientCertPEM(username string, groups []string, ttl time.Duration) ([]byte, []byte, error) { +func (m *MockClientCertIssuer) IssueClientCertPEM(username string, groups []string, ttl time.Duration) (*cert.PEM, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "IssueClientCertPEM", username, groups, ttl) - ret0, _ := ret[0].([]byte) - ret1, _ := ret[1].([]byte) - ret2, _ := ret[2].(error) - return ret0, ret1, ret2 + ret0, _ := ret[0].(*cert.PEM) + ret1, _ := ret[1].(error) + return ret0, ret1 } // IssueClientCertPEM indicates an expected call of IssueClientCertPEM. diff --git a/internal/registry/credentialrequest/rest.go b/internal/registry/credentialrequest/rest.go index a8ef1c318..6edd83340 100644 --- a/internal/registry/credentialrequest/rest.go +++ b/internal/registry/credentialrequest/rest.go @@ -20,7 +20,6 @@ import ( "k8s.io/apiserver/pkg/authentication/user" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/rest" - "k8s.io/utils/clock" loginapi "go.pinniped.dev/generated/latest/apis/concierge/login" "go.pinniped.dev/internal/auditevent" @@ -40,14 +39,12 @@ func NewREST( issuer clientcertissuer.ClientCertIssuer, resource schema.GroupResource, auditLogger plog.AuditLogger, - clock clock.Clock, ) *REST { return &REST{ authenticator: authenticator, issuer: issuer, tableConvertor: rest.NewDefaultTableConvertor(resource), auditLogger: auditLogger, - clock: clock, } } @@ -56,7 +53,6 @@ type REST struct { issuer clientcertissuer.ClientCertIssuer tableConvertor rest.TableConvertor auditLogger plog.AuditLogger - clock clock.Clock } // Assert that our *REST implements all the optional interfaces that we expect it to implement. @@ -162,9 +158,7 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation return authenticationFailedResponse(), nil } - // this timestamp should be returned from IssueClientCertPEM but this is a safe approximation - expires := metav1.NewTime(r.clock.Now().UTC().Add(clientCertificateTTL)) - certPEM, keyPEM, err := r.issuer.IssueClientCertPEM(userInfo.GetName(), userInfo.GetGroups(), clientCertificateTTL) + pem, err := r.issuer.IssueClientCertPEM(userInfo.GetName(), userInfo.GetGroups(), clientCertificateTTL) if err != nil { r.auditLogger.Audit(auditevent.TokenCredentialRequestUnexpectedError, &plog.AuditParams{ ReqCtx: ctx, @@ -177,6 +171,9 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation return authenticationFailedResponse(), nil } + notBefore := metav1.NewTime(pem.NotBefore) + notAfter := metav1.NewTime(pem.NotAfter) + r.auditLogger.Audit(auditevent.TokenCredentialRequestAuthenticatedUser, &plog.AuditParams{ ReqCtx: ctx, PIIKeysAndValues: []any{ @@ -184,7 +181,10 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation "groups", userInfo.GetGroups(), }, KeysAndValues: []any{ - "issuedClientCertExpires", expires.Format(time.RFC3339), + "issuedClientCert", map[string]string{ + "notBefore": notBefore.Format(time.RFC3339), + "notAfter": notAfter.Format(time.RFC3339), + }, "authenticator", credentialRequest.Spec.Authenticator, }, }) @@ -192,9 +192,9 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation return &loginapi.TokenCredentialRequest{ Status: loginapi.TokenCredentialRequestStatus{ Credential: &loginapi.ClusterCredential{ - ExpirationTimestamp: expires, - ClientCertificateData: string(certPEM), - ClientKeyData: string(keyPEM), + ExpirationTimestamp: notAfter, + ClientCertificateData: string(pem.CertPEM), + ClientKeyData: string(pem.KeyPEM), }, }, }, nil diff --git a/internal/registry/credentialrequest/rest_test.go b/internal/registry/credentialrequest/rest_test.go index 854746738..2bbf38a9b 100644 --- a/internal/registry/credentialrequest/rest_test.go +++ b/internal/registry/credentialrequest/rest_test.go @@ -24,11 +24,10 @@ import ( "k8s.io/apiserver/pkg/authentication/user" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/rest" - "k8s.io/utils/clock" - clocktesting "k8s.io/utils/clock/testing" "k8s.io/utils/ptr" loginapi "go.pinniped.dev/generated/latest/apis/concierge/login" + "go.pinniped.dev/internal/cert" "go.pinniped.dev/internal/clientcertissuer" "go.pinniped.dev/internal/mocks/mockcredentialrequest" "go.pinniped.dev/internal/mocks/mockissuer" @@ -37,7 +36,7 @@ import ( ) func TestNew(t *testing.T) { - r := NewREST(nil, nil, schema.GroupResource{Group: "bears", Resource: "panda"}, nil, clock.RealClock{}) + r := NewREST(nil, nil, schema.GroupResource{Group: "bears", Resource: "panda"}, nil) require.NotNil(t, r) require.False(t, r.NamespaceScoped()) require.Equal(t, []string{"pinniped"}, r.Categories()) @@ -78,16 +77,14 @@ func TestCreate(t *testing.T) { var ctrl *gomock.Controller var auditLogger plog.AuditLogger var actualAuditLog *bytes.Buffer - var frozenNow time.Time - var frozenClock *clocktesting.FakeClock + var fakeNow time.Time var wantAuditLog []testutil.WantedAuditLog it.Before(func() { r = require.New(t) ctrl = gomock.NewController(t) auditLogger, actualAuditLog = plog.TestAuditLogger(t) - frozenNow = time.Date(2024, time.September, 12, 4, 25, 56, 778899, time.UTC) - frozenClock = clocktesting.NewFakeClock(frozenNow) + fakeNow = time.Date(2024, time.September, 12, 4, 25, 56, 778899, time.UTC) }) it.After(func() { @@ -110,9 +107,14 @@ func TestCreate(t *testing.T) { "test-user", []string{"test-group-1", "test-group-2"}, 5*time.Minute, - ).Return([]byte("test-cert"), []byte("test-key"), nil) + ).Return(&cert.PEM{ + CertPEM: []byte("test-cert"), + KeyPEM: []byte("test-key"), + NotBefore: fakeNow.Add(-5 * time.Minute), + NotAfter: fakeNow.Add(5 * time.Minute), + }, nil) - storage := NewREST(requestAuthenticator, clientCertIssuer, schema.GroupResource{}, auditLogger, frozenClock) + storage := NewREST(requestAuthenticator, clientCertIssuer, schema.GroupResource{}, auditLogger) response, err := callCreate(storage, req) @@ -122,7 +124,7 @@ func TestCreate(t *testing.T) { r.Equal(response, &loginapi.TokenCredentialRequest{ Status: loginapi.TokenCredentialRequestStatus{ Credential: &loginapi.ClusterCredential{ - ExpirationTimestamp: metav1.NewTime(frozenNow.Add(5 * time.Minute).UTC()), + ExpirationTimestamp: metav1.NewTime(fakeNow.Add(5 * time.Minute).UTC()), ClientCertificateData: "test-cert", ClientKeyData: "test-key", }, @@ -141,7 +143,10 @@ func TestCreate(t *testing.T) { "kind": "FakeAuthenticatorKind", "name": "fake-authenticator-name", }, - "issuedClientCertExpires": "2024-09-12T04:30:56Z", // this is frozenNow + 5 minutes in UTC + "issuedClientCert": map[string]any{ + "notBefore": "2024-09-12T04:20:56Z", // this is fakeNow - 5 minutes in UTC + "notAfter": "2024-09-12T04:30:56Z", // this is fakeNow + 5 minutes in UTC + }, "personalInfo": map[string]any{ "username": "test-user", "groups": []any{"test-group-1", "test-group-2"}, @@ -163,9 +168,9 @@ func TestCreate(t *testing.T) { clientCertIssuer := mockissuer.NewMockClientCertIssuer(ctrl) clientCertIssuer.EXPECT(). IssueClientCertPEM(gomock.Any(), gomock.Any(), gomock.Any()). - Return(nil, nil, fmt.Errorf("some certificate authority error")) + Return(nil, fmt.Errorf("some certificate authority error")) - storage := NewREST(requestAuthenticator, clientCertIssuer, schema.GroupResource{}, auditLogger, frozenClock) + storage := NewREST(requestAuthenticator, clientCertIssuer, schema.GroupResource{}, auditLogger) response, err := callCreate(storage, req) requireSuccessfulResponseWithAuthenticationFailureMessage(t, err, response) @@ -194,7 +199,7 @@ func TestCreate(t *testing.T) { requestAuthenticator := mockcredentialrequest.NewMockTokenCredentialRequestAuthenticator(ctrl) requestAuthenticator.EXPECT().AuthenticateTokenCredentialRequest(gomock.Any(), req).Return(nil, nil) - storage := NewREST(requestAuthenticator, nil, schema.GroupResource{}, auditLogger, frozenClock) + storage := NewREST(requestAuthenticator, nil, schema.GroupResource{}, auditLogger) response, err := callCreate(storage, req) @@ -224,7 +229,7 @@ func TestCreate(t *testing.T) { requestAuthenticator.EXPECT().AuthenticateTokenCredentialRequest(gomock.Any(), req). Return(nil, errors.New("some webhook error")) - storage := NewREST(requestAuthenticator, nil, schema.GroupResource{}, auditLogger, frozenClock) + storage := NewREST(requestAuthenticator, nil, schema.GroupResource{}, auditLogger) response, err := callCreate(storage, req) @@ -255,7 +260,7 @@ func TestCreate(t *testing.T) { requestAuthenticator.EXPECT().AuthenticateTokenCredentialRequest(gomock.Any(), req). Return(&user.DefaultInfo{Name: "", UID: "test-uid"}, nil) - storage := NewREST(requestAuthenticator, nil, schema.GroupResource{}, auditLogger, frozenClock) + storage := NewREST(requestAuthenticator, nil, schema.GroupResource{}, auditLogger) response, err := callCreate(storage, req) @@ -295,7 +300,7 @@ func TestCreate(t *testing.T) { Groups: []string{"test-group-1", "test-group-2"}, }, nil) - storage := NewREST(requestAuthenticator, nil, schema.GroupResource{}, auditLogger, frozenClock) + storage := NewREST(requestAuthenticator, nil, schema.GroupResource{}, auditLogger) response, err := callCreate(storage, req) @@ -335,7 +340,7 @@ func TestCreate(t *testing.T) { Extra: map[string][]string{"test-key": {"test-val-1", "test-val-2"}}, }, nil) - storage := NewREST(requestAuthenticator, nil, schema.GroupResource{}, auditLogger, frozenClock) + storage := NewREST(requestAuthenticator, nil, schema.GroupResource{}, auditLogger) response, err := callCreate(storage, req) @@ -366,7 +371,7 @@ func TestCreate(t *testing.T) { it("CreateFailsWhenGivenTheWrongInputType", func() { notACredentialRequest := runtime.Unknown{} - response, err := NewREST(nil, nil, schema.GroupResource{}, auditLogger, frozenClock).Create( + response, err := NewREST(nil, nil, schema.GroupResource{}, auditLogger).Create( genericapirequest.NewContext(), ¬ACredentialRequest, rest.ValidateAllObjectFunc, @@ -376,7 +381,7 @@ func TestCreate(t *testing.T) { }) it("CreateFailsWhenTokenValueIsEmptyInRequest", func() { - storage := NewREST(nil, nil, schema.GroupResource{}, auditLogger, frozenClock) + storage := NewREST(nil, nil, schema.GroupResource{}, auditLogger) response, err := callCreate(storage, credentialRequest(loginapi.TokenCredentialRequestSpec{ Token: "", })) @@ -386,7 +391,7 @@ func TestCreate(t *testing.T) { }) it("CreateFailsWhenValidationFails", func() { - storage := NewREST(nil, nil, schema.GroupResource{}, auditLogger, frozenClock) + storage := NewREST(nil, nil, schema.GroupResource{}, auditLogger) response, err := storage.Create( context.Background(), validCredentialRequest(), @@ -408,7 +413,7 @@ func TestCreate(t *testing.T) { fakeReqContext := audit.WithAuditContext(context.Background()) audit.WithAuditID(fakeReqContext, "fake-audit-id") - storage := NewREST(requestAuthenticator, successfulIssuer(ctrl), schema.GroupResource{}, auditLogger, frozenClock) + storage := NewREST(requestAuthenticator, successfulIssuer(ctrl, fakeNow), schema.GroupResource{}, auditLogger) response, err := storage.Create( fakeReqContext, req, @@ -433,7 +438,10 @@ func TestCreate(t *testing.T) { "kind": "FakeAuthenticatorKind", "name": "fake-authenticator-name", }, - "issuedClientCertExpires": "2024-09-12T04:30:56Z", // this is frozenNow + 5 minutes in UTC + "issuedClientCert": map[string]any{ + "notBefore": "2024-09-12T04:20:56Z", // this is fakeNow - 5 minutes in UTC + "notAfter": "2024-09-12T04:30:56Z", // this is fakeNow + 5 minutes in UTC + }, "personalInfo": map[string]any{ "username": "test-user", "groups": []any{}, @@ -449,7 +457,7 @@ func TestCreate(t *testing.T) { requestAuthenticator.EXPECT().AuthenticateTokenCredentialRequest(gomock.Any(), req.DeepCopy()). Return(&user.DefaultInfo{Name: "test-user"}, nil) - storage := NewREST(requestAuthenticator, successfulIssuer(ctrl), schema.GroupResource{}, auditLogger, frozenClock) + storage := NewREST(requestAuthenticator, successfulIssuer(ctrl, fakeNow), schema.GroupResource{}, auditLogger) fakeReqContext := audit.WithAuditContext(context.Background()) audit.WithAuditID(fakeReqContext, "fake-audit-id") @@ -484,7 +492,10 @@ func TestCreate(t *testing.T) { "kind": "FakeAuthenticatorKind", "name": "fake-authenticator-name", }, - "issuedClientCertExpires": "2024-09-12T04:30:56Z", // this is frozenNow + 5 minutes in UTC + "issuedClientCert": map[string]any{ + "notBefore": "2024-09-12T04:20:56Z", // this is fakeNow - 5 minutes in UTC + "notAfter": "2024-09-12T04:30:56Z", // this is fakeNow + 5 minutes in UTC + }, "personalInfo": map[string]any{ "username": "test-user", "groups": []any{}, @@ -494,7 +505,7 @@ func TestCreate(t *testing.T) { }) it("CreateFailsWhenRequestOptionsDryRunIsNotEmpty", func() { - response, err := NewREST(nil, nil, schema.GroupResource{}, auditLogger, frozenClock).Create( + response, err := NewREST(nil, nil, schema.GroupResource{}, auditLogger).Create( genericapirequest.NewContext(), validCredentialRequest(), rest.ValidateAllObjectFunc, @@ -507,7 +518,7 @@ func TestCreate(t *testing.T) { }) it("CreateFailsWhenNamespaceIsNotEmpty", func() { - response, err := NewREST(nil, nil, schema.GroupResource{}, auditLogger, frozenClock).Create( + response, err := NewREST(nil, nil, schema.GroupResource{}, auditLogger).Create( genericapirequest.WithNamespace(genericapirequest.NewContext(), "some-ns"), validCredentialRequest(), rest.ValidateAllObjectFunc, @@ -576,10 +587,15 @@ func requireSuccessfulResponseWithAuthenticationFailureMessage(t *testing.T, err }) } -func successfulIssuer(ctrl *gomock.Controller) clientcertissuer.ClientCertIssuer { +func successfulIssuer(ctrl *gomock.Controller, fakeNow time.Time) clientcertissuer.ClientCertIssuer { clientCertIssuer := mockissuer.NewMockClientCertIssuer(ctrl) clientCertIssuer.EXPECT(). IssueClientCertPEM(gomock.Any(), gomock.Any(), gomock.Any()). - Return([]byte("test-cert"), []byte("test-key"), nil) + Return(&cert.PEM{ + CertPEM: []byte("test-cert"), + KeyPEM: []byte("test-key"), + NotBefore: fakeNow.Add(-5 * time.Minute), + NotAfter: fakeNow.Add(5 * time.Minute), + }, nil) return clientCertIssuer } diff --git a/site/content/docs/reference/audit-logging.md b/site/content/docs/reference/audit-logging.md index 93763d5d4..c3ab8c2ed 100644 --- a/site/content/docs/reference/audit-logging.md +++ b/site/content/docs/reference/audit-logging.md @@ -499,7 +499,10 @@ the Kubernetes audit logs for the same request, allowing them to be correlated. "username": "pinny@example.com", "groups": ["developers", "auditors"] }, - "issuedClientCertExpires": "2024-11-21T17:54:11Z", + "issuedClientCert": { + "notAfter": "2024-11-21T17:54:11Z", + "notBefore": "2024-11-21T17:44:11Z" + }, "authenticator": { "apiGroup": "authentication.concierge.pinniped.dev", "kind": "JWTAuthenticator", diff --git a/test/integration/audit_test.go b/test/integration/audit_test.go index 976142371..335d09fd2 100644 --- a/test/integration/audit_test.go +++ b/test/integration/audit_test.go @@ -190,10 +190,10 @@ func TestAuditLogsDuringLogin_Disruptive(t *testing.T) { timeBeforeLogin, ) removeSomeKeysFromEachAuditLogEvent(allConciergeTCRLogs) - // Also remove issuedClientCertExpires, which is a timestamp that we can't easily predict for the assertions below. + // Also remove issuedClientCert, which contains timestamps that we can't easily predict for the assertions below. for _, log := range allConciergeTCRLogs { - require.NotEmpty(t, log["issuedClientCertExpires"]) - delete(log, "issuedClientCertExpires") + require.NotEmpty(t, log["issuedClientCert"]) + delete(log, "issuedClientCert") } // All values in the personalInfo map should be redacted by default. @@ -338,10 +338,10 @@ func TestAuditLogsDuringLogin_Disruptive(t *testing.T) { timeBeforeLogin, ) removeSomeKeysFromEachAuditLogEvent(allConciergeTCRLogs) - // Also remove issuedClientCertExpires, which is a timestamp that we can't easily predict for the assertions below. + // Also remove issuedClientCert, which contains timestamps that we can't easily predict for the assertions below. for _, log := range allConciergeTCRLogs { - require.NotEmpty(t, log["issuedClientCertExpires"]) - delete(log, "issuedClientCertExpires") + require.NotEmpty(t, log["issuedClientCert"]) + delete(log, "issuedClientCert") } // All values in the personalInfo map should not be redacted anymore. diff --git a/test/integration/concierge_impersonation_proxy_test.go b/test/integration/concierge_impersonation_proxy_test.go index 96df0343c..4ea8525fc 100644 --- a/test/integration/concierge_impersonation_proxy_test.go +++ b/test/integration/concierge_impersonation_proxy_test.go @@ -1783,8 +1783,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl externallyProvidedCA, err = certauthority.New("Impersonation Proxy Integration Test CA", 1*time.Hour) require.NoError(t, err) - var externallyProvidedTLSServingCertPEM, externallyProvidedTLSServingKeyPEM []byte - externallyProvidedTLSServingCertPEM, externallyProvidedTLSServingKeyPEM, err = externallyProvidedCA.IssueServerCertPEM([]string{proxyServiceEndpoint}, nil, 1*time.Hour) + externallyProvidedTLSServingCertPEM, err := externallyProvidedCA.IssueServerCertPEM([]string{proxyServiceEndpoint}, nil, 1*time.Hour) require.NoError(t, err) // Specifically use corev1.Secret.StringData @@ -1796,8 +1795,8 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl corev1.SecretTypeTLS, map[string]string{ "ca.crt": string(externallyProvidedCA.Bundle()), - corev1.TLSCertKey: string(externallyProvidedTLSServingCertPEM), - corev1.TLSPrivateKeyKey: string(externallyProvidedTLSServingKeyPEM), + corev1.TLSCertKey: string(externallyProvidedTLSServingCertPEM.CertPEM), + corev1.TLSPrivateKeyKey: string(externallyProvidedTLSServingCertPEM.KeyPEM), }) _, originalInternallyGeneratedCAPEM := performImpersonatorDiscoveryURL(ctx, t, env, adminConciergeClient) @@ -1855,8 +1854,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl externallyProvidedCA, err = certauthority.New("Impersonation Proxy Integration Test CA", 1*time.Hour) require.NoError(t, err) - var externallyProvidedTLSServingCertPEM, externallyProvidedTLSServingKeyPEM []byte - externallyProvidedTLSServingCertPEM, externallyProvidedTLSServingKeyPEM, err = externallyProvidedCA.IssueServerCertPEM([]string{proxyServiceEndpoint}, nil, 1*time.Hour) + externallyProvidedTLSServingCertPEM, err := externallyProvidedCA.IssueServerCertPEM([]string{proxyServiceEndpoint}, nil, 1*time.Hour) require.NoError(t, err) // Specifically use corev1.Secret.Data @@ -1868,8 +1866,8 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl corev1.SecretTypeTLS, map[string][]byte{ "ca.crt": externallyProvidedCA.Bundle(), - corev1.TLSCertKey: externallyProvidedTLSServingCertPEM, - corev1.TLSPrivateKeyKey: externallyProvidedTLSServingKeyPEM, + corev1.TLSCertKey: externallyProvidedTLSServingCertPEM.CertPEM, + corev1.TLSPrivateKeyKey: externallyProvidedTLSServingCertPEM.KeyPEM, }) _, originalInternallyGeneratedCAPEM := performImpersonatorDiscoveryURL(ctx, t, env, adminConciergeClient)