diff --git a/internal/auditevent/audit_event.go b/internal/auditevent/audit_event.go index 3077f5263..aaba2aee6 100644 --- a/internal/auditevent/audit_event.go +++ b/internal/auditevent/audit_event.go @@ -20,6 +20,7 @@ const ( UpstreamOIDCTokenRevoked Message = "Upstream OIDC Token Revoked" //nolint:gosec // this is not a credential SessionGarbageCollected Message = "Session Garbage Collected" UpstreamAuthorizeRedirect Message = "Upstream Authorize Redirect" + OIDCClientSecretRequestUpdatedSecrets Message = "OIDCClientSecretRequest Updated Secrets" TokenCredentialRequestAuthenticatedUser Message = "TokenCredentialRequest Authenticated User" //nolint:gosec // this is not a credential TokenCredentialRequestAuthenticationFailed Message = "TokenCredentialRequest Authentication Failed" //nolint:gosec // this is not a credential TokenCredentialRequestUnexpectedError Message = "TokenCredentialRequest Unexpected Error" //nolint:gosec // this is not a credential diff --git a/internal/registry/clientsecretrequest/rest.go b/internal/registry/clientsecretrequest/rest.go index 4ac86fb59..333a7b4d3 100644 --- a/internal/registry/clientsecretrequest/rest.go +++ b/internal/registry/clientsecretrequest/rest.go @@ -28,8 +28,11 @@ import ( "k8s.io/utils/trace" clientsecretapi "go.pinniped.dev/generated/latest/apis/supervisor/clientsecret" + supervisorconfigv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/config/v1alpha1" configv1alpha1clientset "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned/typed/config/v1alpha1" + "go.pinniped.dev/internal/auditevent" "go.pinniped.dev/internal/oidcclientsecretstorage" + "go.pinniped.dev/internal/plog" ) // Cost is a good bcrypt cost for 2022, should take about 250 ms to validate. @@ -48,6 +51,7 @@ func NewREST( randByteGenerator io.Reader, byteHasher byteHasher, timeNowFunc timeNowFunc, + auditLogger plog.AuditLogger, ) *REST { return &REST{ secretStorage: oidcclientsecretstorage.New(secretsClient), @@ -58,6 +62,7 @@ func NewREST( byteHasher: byteHasher, tableConvertor: rest.NewDefaultTableConvertor(resource), timeNowFunc: timeNowFunc, + auditLogger: auditLogger, } } @@ -70,6 +75,7 @@ type REST struct { byteHasher byteHasher tableConvertor rest.TableConvertor timeNowFunc timeNowFunc + auditLogger plog.AuditLogger } // Assert that our *REST implements all the optional interfaces that we expect it to implement. @@ -121,7 +127,12 @@ func (*REST) GetSingularName() string { return "oidcclientsecretrequest" } -func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) { +func (r *REST) Create( + ctx context.Context, + obj runtime.Object, + createValidation rest.ValidateObjectFunc, + options *metav1.CreateOptions, +) (runtime.Object, error) { t := trace.FromContext(ctx).Nest("create", trace.Field{Key: "kind", Value: "OIDCClientSecretRequest"}, trace.Field{Key: "metadata.name", Value: name(obj)}, @@ -137,14 +148,9 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation t.Step("validateRequest") // Find the specified OIDCClient. - oidcClient, err := r.oidcClientsClient.Get(ctx, req.Name, metav1.GetOptions{}) + oidcClient, err := r.findClient(ctx, req.Name, t) if err != nil { - traceFailureWithError(t, "oidcClientsClient.Get", err) - if apierrors.IsNotFound(err) { - errs := field.ErrorList{field.NotFound(field.NewPath("metadata", "name"), req.Name)} - return nil, apierrors.NewInvalid(kindFromContext(ctx), req.Name, errs) - } - return nil, apierrors.NewInternalError(fmt.Errorf("getting client %q failed", req.Name)) + return nil, err } t.Step("oidcClientsClient.Get") @@ -155,19 +161,20 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation traceFailureWithError(t, "secretStorage.Get", err) return nil, apierrors.NewInternalError(fmt.Errorf("getting secret for client %q failed", req.Name)) } + numPreviouslyStoredHashes := len(hashes) t.Step("secretStorage.Get") // If requested, generate a new client secret and add it to the list. - var secret string + var generatedSecret string if req.Spec.GenerateNewSecret { - secret, err = generateSecret(r.randByteGenerator) + generatedSecret, err = generateSecret(r.randByteGenerator) if err != nil { traceFailureWithError(t, "generateSecret", err) return nil, apierrors.NewInternalError(fmt.Errorf("client secret generation failed")) } t.Step("generateSecret") - hash, err := r.byteHasher([]byte(secret), r.cost) + hash, err := r.byteHasher([]byte(generatedSecret), r.cost) if err != nil { traceFailureWithError(t, "bcrypt.GenerateFromPassword", err) return nil, apierrors.NewInternalError(fmt.Errorf("hash generation failed")) @@ -179,8 +186,16 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation // If requested, remove all client secrets except for the most recent one. needsRevoke := req.Spec.RevokeOldSecrets && len(hashes) > 0 + numRevokedHashes := 0 if needsRevoke { hashes = []string{hashes[0]} + if generatedSecret == "" { + // There is no newly generated secret, so one old hash is retained and all others are revoked. + numRevokedHashes = numPreviouslyStoredHashes - 1 + } else { + // The newly generated secret was added to the list, and all old hashes are revoked. + numRevokedHashes = numPreviouslyStoredHashes + } } // If anything was requested to change... @@ -204,6 +219,16 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation return nil, apierrors.NewInternalError(fmt.Errorf("setting client secret failed")) } t.Step("secretStorage.Set") + + r.auditLogger.Audit(auditevent.OIDCClientSecretRequestUpdatedSecrets, &plog.AuditParams{ + ReqCtx: ctx, + KeysAndValues: []any{ + "clientID", req.Name, + "generatedSecret", len(generatedSecret) > 0, + "revokedSecrets", numRevokedHashes, + "totalSecrets", len(hashes), + }, + }) } // Return the new secret in plaintext, if one was generated, along with the total number of secrets. @@ -218,12 +243,25 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation RevokeOldSecrets: req.Spec.RevokeOldSecrets, }, Status: clientsecretapi.OIDCClientSecretRequestStatus{ - GeneratedSecret: secret, + GeneratedSecret: generatedSecret, TotalClientSecrets: len(hashes), }, }, nil } +func (r *REST) findClient(ctx context.Context, clientName string, tracer *trace.Trace) (*supervisorconfigv1alpha1.OIDCClient, error) { + oidcClient, err := r.oidcClientsClient.Get(ctx, clientName, metav1.GetOptions{}) + if err != nil { + traceFailureWithError(tracer, "oidcClientsClient.Get", err) + if apierrors.IsNotFound(err) { + errs := field.ErrorList{field.NotFound(field.NewPath("metadata", "name"), clientName)} + return nil, apierrors.NewInvalid(kindFromContext(ctx), clientName, errs) + } + return nil, apierrors.NewInternalError(fmt.Errorf("getting client %q failed", clientName)) + } + return oidcClient, nil +} + func (r *REST) validateRequest( ctx context.Context, obj runtime.Object, diff --git a/internal/registry/clientsecretrequest/rest_test.go b/internal/registry/clientsecretrequest/rest_test.go index 365f97d75..bf9b05607 100644 --- a/internal/registry/clientsecretrequest/rest_test.go +++ b/internal/registry/clientsecretrequest/rest_test.go @@ -21,6 +21,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" + "k8s.io/apiserver/pkg/audit" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/rest" kubefake "k8s.io/client-go/kubernetes/fake" @@ -31,6 +32,7 @@ import ( supervisorconfigv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/config/v1alpha1" supervisorfake "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned/fake" "go.pinniped.dev/internal/oidcclientsecretstorage" + "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/testutil" ) @@ -44,6 +46,7 @@ func TestNew(t *testing.T) { nil, nil, nil, + nil, ) require.NotNil(t, r) @@ -120,6 +123,7 @@ func TestCreate(t *testing.T) { wantErrStatus *metav1.Status wantHashes *wantHashes wantLogStepSubstrings []string + wantAuditLog []testutil.WantedAuditLog }{ { name: "wrong type of request object provided", @@ -714,6 +718,15 @@ func TestCreate(t *testing.T) { `secretStorage.Set`, `END`, }, + wantAuditLog: []testutil.WantedAuditLog{ + testutil.WantAuditLog("OIDCClientSecretRequest Updated Secrets", map[string]any{ + "auditID": "fake-audit-id", + "clientID": "client.oauth.pinniped.dev-happy-new-secret", + "generatedSecret": true, + "revokedSecrets": float64(0), + "totalSecrets": float64(1), + }), + }, }, { name: "happy path: secret exists, prepend new secret hash to secret to the list of hashes for found oidcclient", @@ -783,6 +796,15 @@ func TestCreate(t *testing.T) { `secretStorage.Set`, `END`, }, + wantAuditLog: []testutil.WantedAuditLog{ + testutil.WantAuditLog("OIDCClientSecretRequest Updated Secrets", map[string]any{ + "auditID": "fake-audit-id", + "clientID": "client.oauth.pinniped.dev-append-new-secret-hash", + "generatedSecret": true, + "revokedSecrets": float64(0), + "totalSecrets": float64(3), + }), + }, }, { name: "happy path: secret exists, append new secret hash to secret and revoke old for found oidcclient", @@ -849,9 +871,18 @@ func TestCreate(t *testing.T) { `secretStorage.Set`, `END`, }, + wantAuditLog: []testutil.WantedAuditLog{ + testutil.WantAuditLog("OIDCClientSecretRequest Updated Secrets", map[string]any{ + "auditID": "fake-audit-id", + "clientID": "client.oauth.pinniped.dev-append-new-secret-hash", + "generatedSecret": true, + "revokedSecrets": float64(2), + "totalSecrets": float64(1), + }), + }, }, { - name: "happy path: secret exists, revoke old secrets but retain latest for found oidcclient", + name: "happy path: secret exists, revoke oldest secrets but retain latest old secret for found oidcclient", args: args{ ctx: namespacedContext, obj: &clientsecretapi.OIDCClientSecretRequest{ @@ -874,6 +905,7 @@ func TestCreate(t *testing.T) { []string{ "hashed-password-1", "hashed-password-2", + "hashed-password-3", }, )) }, @@ -913,6 +945,15 @@ func TestCreate(t *testing.T) { `secretStorage.Set`, `END`, }, + wantAuditLog: []testutil.WantedAuditLog{ + testutil.WantAuditLog("OIDCClientSecretRequest Updated Secrets", map[string]any{ + "auditID": "fake-audit-id", + "clientID": "client.oauth.pinniped.dev-some-client", + "generatedSecret": false, + "revokedSecrets": float64(2), + "totalSecrets": float64(1), + }), + }, }, { name: "secret exists but oidcclient secret has too many hashes, fails to create when RevokeOldSecrets:false (max 5), secret is not updated", @@ -1413,6 +1454,15 @@ func TestCreate(t *testing.T) { `secretStorage.Set`, `END`, }, + wantAuditLog: []testutil.WantedAuditLog{ + testutil.WantAuditLog("OIDCClientSecretRequest Updated Secrets", map[string]any{ + "auditID": "fake-audit-id", + "clientID": "client.oauth.pinniped.dev-some-client", + "generatedSecret": true, + "revokedSecrets": float64(1), + "totalSecrets": float64(1), + }), + }, }, { name: "happy path: generate new secret when existing secrets is max (5)", @@ -1482,6 +1532,15 @@ func TestCreate(t *testing.T) { `secretStorage.Set`, `END`, }, + wantAuditLog: []testutil.WantedAuditLog{ + testutil.WantAuditLog("OIDCClientSecretRequest Updated Secrets", map[string]any{ + "auditID": "fake-audit-id", + "clientID": "client.oauth.pinniped.dev-some-client", + "generatedSecret": true, + "revokedSecrets": float64(5), + "totalSecrets": float64(1), + }), + }, }, { name: "happy path: generate new secret when existing secrets exceeds maximum (5)", @@ -1552,6 +1611,15 @@ func TestCreate(t *testing.T) { `secretStorage.Set`, `END`, }, + wantAuditLog: []testutil.WantedAuditLog{ + testutil.WantAuditLog("OIDCClientSecretRequest Updated Secrets", map[string]any{ + "auditID": "fake-audit-id", + "clientID": "client.oauth.pinniped.dev-some-client", + "generatedSecret": true, + "revokedSecrets": float64(6), + "totalSecrets": float64(1), + }), + }, }, } for _, tt := range tests { @@ -1606,6 +1674,10 @@ func TestCreate(t *testing.T) { fakeByteGenerator = strings.NewReader(fakeRandomBytes + "these extra bytes should be ignored since we only read 32 bytes") } + auditLogger, actualAuditLog := plog.TestAuditLogger(t) + ctx := audit.WithAuditContext(tt.args.ctx) + audit.WithAuditID(ctx, "fake-audit-id") + r := NewREST( schema.GroupResource{Group: "bears", Resource: "panda"}, secretsClient, @@ -1615,9 +1687,10 @@ func TestCreate(t *testing.T) { fakeByteGenerator, fakeHasher, fakeTimeNowFunc, + auditLogger, ) - got, err := r.Create(tt.args.ctx, tt.args.obj, tt.args.createValidation, tt.args.options) + got, err := r.Create(ctx, tt.args.obj, tt.args.createValidation, tt.args.options) require.Equal(t, tt.want, got) if tt.wantErrStatus != nil { @@ -1646,6 +1719,8 @@ func TestCreate(t *testing.T) { } requireExactlyOneLogLineWithMultipleSteps(t, logger, tt.wantLogStepSubstrings) + + testutil.CompareAuditLogs(t, tt.wantAuditLog, actualAuditLog.String()) }) } } diff --git a/internal/supervisor/apiserver/apiserver.go b/internal/supervisor/apiserver/apiserver.go index 2d078897a..b698d4cae 100644 --- a/internal/supervisor/apiserver/apiserver.go +++ b/internal/supervisor/apiserver/apiserver.go @@ -39,6 +39,7 @@ type ExtraConfig struct { Secrets corev1client.SecretInterface OIDCClients configv1alpha1clientset.OIDCClientInterface Namespace string + AuditLogger plog.AuditLogger } type PinnipedServer struct { @@ -92,6 +93,7 @@ func (c completedConfig) New() (*PinnipedServer, error) { rand.Reader, bcrypt.GenerateFromPassword, metav1.Now, + c.ExtraConfig.AuditLogger, ) return clientSecretReqGVR, clientSecretReqStorage }, diff --git a/internal/supervisor/server/server.go b/internal/supervisor/server/server.go index 8307df4db..a87160486 100644 --- a/internal/supervisor/server/server.go +++ b/internal/supervisor/server/server.go @@ -529,6 +529,7 @@ func runSupervisor(ctx context.Context, podInfo *downward.PodInfo, cfg *supervis clientWithoutLeaderElection.Kubernetes.CoreV1().Secrets(serverInstallationNamespace), client.PinnipedSupervisor.ConfigV1alpha1().OIDCClients(serverInstallationNamespace), serverInstallationNamespace, + auditLogger, ) if err != nil { return fmt.Errorf("could not configure aggregated API server: %w", err) @@ -639,6 +640,7 @@ func getAggregatedAPIServerConfig( secrets corev1client.SecretInterface, oidcClients v1alpha1.OIDCClientInterface, serverInstallationNamespace string, + auditLogger plog.AuditLogger, ) (*apiserver.Config, error) { codecs := serializer.NewCodecFactory(scheme) @@ -705,6 +707,7 @@ func getAggregatedAPIServerConfig( Secrets: secrets, OIDCClients: oidcClients, Namespace: serverInstallationNamespace, + AuditLogger: auditLogger, }, } return apiServerConfig, nil