audit log OIDCClientSecretRequests

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

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,

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

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

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