diff --git a/deploy/concierge/deployment.yaml b/deploy/concierge/deployment.yaml index 6aac1fd1e..f6f0b37e5 100644 --- a/deploy/concierge/deployment.yaml +++ b/deploy/concierge/deployment.yaml @@ -103,6 +103,8 @@ data: tls: onedottwo: allowedCiphers: (@= str(data.values.allowed_ciphers_for_tls_onedottwo) @) + audit: + logUsernamesAndGroups: (@= data.values.audit.log_usernames_and_groups @) --- #@ if data.values.image_pull_dockerconfigjson and data.values.image_pull_dockerconfigjson != "": apiVersion: v1 diff --git a/deploy/concierge/values.yaml b/deploy/concierge/values.yaml index 24ccea66a..e18b1cc8c 100644 --- a/deploy/concierge/values.yaml +++ b/deploy/concierge/values.yaml @@ -231,3 +231,15 @@ no_proxy: "$(KUBERNETES_SERVICE_HOST),169.254.169.254,127.0.0.1,localhost,.svc,. #! An empty array is perfectly valid, as is any array of strings. allowed_ciphers_for_tls_onedottwo: - "" + +#@schema/title "Audit logging configuration" +#@schema/desc "Customize the content of audit log events." +audit: + + #@schema/title "Log usernames and groups" + #@ log_usernames_and_groups_desc = "Enables or disables printing usernames and group names in audit logs. Options are 'enabled' or 'disabled'. \ + #@ If enabled, usernames are group names may be printed in audit log events. \ + #@ If disabled, usernames and group names will be redacted from audit logs because they might contain personally identifiable information." + #@schema/desc log_usernames_and_groups_desc + #@schema/validation one_of=["enabled", "disabled"] + log_usernames_and_groups: disabled diff --git a/deploy/supervisor/helpers.lib.yaml b/deploy/supervisor/helpers.lib.yaml index 693dedaa1..cf3d3d55b 100644 --- a/deploy/supervisor/helpers.lib.yaml +++ b/deploy/supervisor/helpers.lib.yaml @@ -57,6 +57,10 @@ _: #@ template.replace(data.values.custom_labels) #@ "onedottwo": { #@ "allowedCiphers": data.values.allowed_ciphers_for_tls_onedottwo #@ } +#@ }, +#@ "audit": { +#@ "logUsernamesAndGroups": data.values.audit.log_usernames_and_groups, +#@ "logInternalPaths": data.values.audit.log_internal_paths #@ } #@ } #@ if data.values.log_level: diff --git a/deploy/supervisor/values.yaml b/deploy/supervisor/values.yaml index f4aab5e62..7badb9e4f 100644 --- a/deploy/supervisor/values.yaml +++ b/deploy/supervisor/values.yaml @@ -220,3 +220,23 @@ endpoints: { } #! An empty array is perfectly valid, as is any array of strings. allowed_ciphers_for_tls_onedottwo: - "" + +#@schema/title "Audit logging configuration" +#@schema/desc "Customize the content of audit log events." +audit: + + #@schema/title "Log usernames and groups" + #@ log_usernames_and_groups_desc = "Enables or disables printing usernames and group names in audit logs. Options are 'enabled' or 'disabled'. \ + #@ If enabled, usernames are group names may be printed in audit log events. \ + #@ If disabled, usernames and group names will be redacted from audit logs because they might contain personally identifiable information." + #@schema/desc log_usernames_and_groups_desc + #@schema/validation one_of=["enabled", "disabled"] + log_usernames_and_groups: disabled + + #@schema/title "Log HTTPS requests for internal paths" + #@ log_internal_paths = "Enables or disables request logging for internal paths in audit logs. Options are 'enabled' or 'disabled'. \ + #@ If enabled, requests to certain paths that are typically only used internal to the cluster (e.g. /healthz) will be enabled, which can be very verbose. \ + #@ If disabled, requests to those paths will not be audit logged." + #@schema/desc log_internal_paths + #@schema/validation one_of=["enabled", "disabled"] + log_internal_paths: disabled diff --git a/internal/concierge/apiserver/apiserver.go b/internal/concierge/apiserver/apiserver.go index 184b5e00b..a147efb8e 100644 --- a/internal/concierge/apiserver/apiserver.go +++ b/internal/concierge/apiserver/apiserver.go @@ -39,6 +39,7 @@ type ExtraConfig struct { LoginConciergeGroupVersion schema.GroupVersion IdentityConciergeGroupVersion schema.GroupVersion TokenClient *tokenclient.TokenClient + AuditLogger plog.AuditLogger } type PinnipedServer struct { @@ -82,7 +83,7 @@ func (c completedConfig) New() (*PinnipedServer, error) { for _, f := range []func() (schema.GroupVersionResource, rest.Storage){ func() (schema.GroupVersionResource, rest.Storage) { tokenCredReqGVR := c.ExtraConfig.LoginConciergeGroupVersion.WithResource("tokencredentialrequests") - tokenCredStorage := credentialrequest.NewREST(c.ExtraConfig.Authenticator, c.ExtraConfig.Issuer, tokenCredReqGVR.GroupResource(), plog.New()) + tokenCredStorage := credentialrequest.NewREST(c.ExtraConfig.Authenticator, c.ExtraConfig.Issuer, tokenCredReqGVR.GroupResource(), c.ExtraConfig.AuditLogger) return tokenCredReqGVR, tokenCredStorage }, func() (schema.GroupVersionResource, rest.Storage) { diff --git a/internal/concierge/server/server.go b/internal/concierge/server/server.go index f6648336c..67629c6ab 100644 --- a/internal/concierge/server/server.go +++ b/internal/concierge/server/server.go @@ -180,21 +180,9 @@ func (a *App) runServer(ctx context.Context) error { dynamiccertauthority.New(impersonationProxySigningCertProvider), // fallback to our internal CA if we need to } - // Get the aggregated API server config. - aggregatedAPIServerConfig, err := getAggregatedAPIServerConfig( - dynamicServingCertProvider, - authenticators, - certIssuer, - buildControllers, - *cfg.APIGroupSuffix, - *cfg.AggregatedAPIServerPort, - scheme, - loginGV, - identityGV, - ) - if err != nil { - return fmt.Errorf("could not configure aggregated API server: %w", err) - } + auditLogger := plog.NewAuditLogger(plog.AuditLogConfig{ + LogUsernamesAndGroupNames: cfg.Audit.LogUsernamesAndGroups.Enabled(), + }) // Configure a token client that retrieves relatively short-lived tokens from the API server. // It uses a k8s client without leader election because all pods need tokens. @@ -206,13 +194,31 @@ func (a *App) runServer(ctx context.Context) error { if err != nil { return fmt.Errorf("could not create default kubernetes client: %w", err) } - aggregatedAPIServerConfig.ExtraConfig.TokenClient = tokenclient.New( + tokenClient := tokenclient.New( cfg.NamesConfig.ImpersonationProxyServiceAccount, k8sClient.Kubernetes.CoreV1().ServiceAccounts(podInfo.Namespace), impersonationProxyTokenCache.Set, plog.New(), tokenclient.WithExpirationSeconds(oneDayInSeconds)) + // Get the aggregated API server config. + aggregatedAPIServerConfig, err := getAggregatedAPIServerConfig( + dynamicServingCertProvider, + authenticators, + certIssuer, + buildControllers, + *cfg.APIGroupSuffix, + *cfg.AggregatedAPIServerPort, + scheme, + loginGV, + identityGV, + auditLogger, + tokenClient, + ) + if err != nil { + return fmt.Errorf("could not configure aggregated API server: %w", err) + } + // Complete the aggregated API server config and make a server instance. server, err := aggregatedAPIServerConfig.Complete().New() if err != nil { @@ -235,6 +241,8 @@ func getAggregatedAPIServerConfig( aggregatedAPIServerPort int64, scheme *runtime.Scheme, loginConciergeGroupVersion, identityConciergeGroupVersion schema.GroupVersion, + auditLogger plog.AuditLogger, + tokenClient *tokenclient.TokenClient, ) (*apiserver.Config, error) { codecs := serializer.NewCodecFactory(scheme) @@ -301,6 +309,8 @@ func getAggregatedAPIServerConfig( NegotiatedSerializer: codecs, LoginConciergeGroupVersion: loginConciergeGroupVersion, IdentityConciergeGroupVersion: identityConciergeGroupVersion, + TokenClient: tokenClient, + AuditLogger: auditLogger, }, } return apiServerConfig, nil diff --git a/internal/config/concierge/config.go b/internal/config/concierge/config.go index d2faeeda4..9ee82c2f7 100644 --- a/internal/config/concierge/config.go +++ b/internal/config/concierge/config.go @@ -88,6 +88,10 @@ func FromPath(ctx context.Context, path string, setAllowedCiphers ptls.SetAllowe return nil, fmt.Errorf("validate tls: %w", err) } + if err := validateAudit(&config.Audit); err != nil { + return nil, fmt.Errorf("validate audit: %w", err) + } + if config.Labels == nil { config.Labels = make(map[string]string) } @@ -200,3 +204,11 @@ func validateServerPort(port *int64) error { } return nil } + +func validateAudit(auditConfig *AuditSpec) error { + v := auditConfig.LogUsernamesAndGroups + if v != "" && v != Enabled && v != Disabled { + return constable.Error("invalid logUsernamesAndGroups format, valid choices are 'enabled', 'disabled', or empty string (equivalent to 'disabled')") + } + return nil +} diff --git a/internal/config/concierge/config_test.go b/internal/config/concierge/config_test.go index 37eacd150..82a8c9f4e 100644 --- a/internal/config/concierge/config_test.go +++ b/internal/config/concierge/config_test.go @@ -67,6 +67,8 @@ func TestFromPath(t *testing.T) { - foo - bar - TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305 + audit: + logUsernamesAndGroups: enabled `), wantConfig: &Config{ DiscoveryInfo: DiscoveryInfoSpec{ @@ -115,6 +117,9 @@ func TestFromPath(t *testing.T) { }, }, }, + Audit: AuditSpec{ + LogUsernamesAndGroups: "enabled", + }, }, }, { @@ -155,6 +160,8 @@ func TestFromPath(t *testing.T) { log: level: all format: json + audit: + logUsernamesAndGroups: disabled `), wantConfig: &Config{ DiscoveryInfo: DiscoveryInfoSpec{ @@ -195,6 +202,9 @@ func TestFromPath(t *testing.T) { Level: plog.LevelAll, Format: plog.FormatJSON, }, + Audit: AuditSpec{ + LogUsernamesAndGroups: "disabled", + }, }, }, { @@ -287,6 +297,7 @@ func TestFromPath(t *testing.T) { NamePrefix: ptr.To("pinniped-kube-cert-agent-"), Image: ptr.To("debian:latest"), }, + Audit: AuditSpec{LogUsernamesAndGroups: ""}, }, }, { @@ -629,6 +640,28 @@ func TestFromPath(t *testing.T) { allowedCiphersError: fmt.Errorf("some error from setAllowedCiphers"), wantError: "validate tls: some error from setAllowedCiphers", }, + { + name: "invalid audit.logUsernamesAndGroups format", + yaml: here.Doc(` + --- + names: + servingCertificateSecret: pinniped-concierge-api-tls-serving-certificate + credentialIssuer: pinniped-config + apiService: pinniped-api + impersonationLoadBalancerService: impersonationLoadBalancerService-value + impersonationClusterIPService: impersonationClusterIPService-value + impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value + impersonationCACertificateSecret: impersonationCACertificateSecret-value + impersonationSignerSecret: impersonationSignerSecret-value + impersonationSignerSecret: impersonationSignerSecret-value + agentServiceAccount: agentServiceAccount-value + impersonationProxyServiceAccount: impersonationProxyServiceAccount-value + impersonationProxyLegacySecret: impersonationProxyLegacySecret-value + audit: + logUsernamesAndGroups: this-value-is-not-allowed + `), + wantError: "validate audit: invalid logUsernamesAndGroups format, valid choices are 'enabled', 'disabled', or empty string (equivalent to 'disabled')", + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { diff --git a/internal/config/concierge/types.go b/internal/config/concierge/types.go index b4b814845..550abf991 100644 --- a/internal/config/concierge/types.go +++ b/internal/config/concierge/types.go @@ -5,6 +5,11 @@ package concierge import "go.pinniped.dev/internal/plog" +const ( + Enabled = "enabled" + Disabled = "disabled" +) + // Config contains knobs to set up an instance of the Pinniped Concierge. type Config struct { DiscoveryInfo DiscoveryInfoSpec `json:"discovery"` @@ -17,6 +22,17 @@ type Config struct { Labels map[string]string `json:"labels"` Log plog.LogSpec `json:"log"` TLS TLSSpec `json:"tls"` + Audit AuditSpec `json:"audit"` +} + +type AuditUsernamesAndGroups string + +func (l AuditUsernamesAndGroups) Enabled() bool { + return l == Enabled +} + +type AuditSpec struct { + LogUsernamesAndGroups AuditUsernamesAndGroups `json:"logUsernamesAndGroups"` } type TLSSpec struct { diff --git a/internal/config/supervisor/config.go b/internal/config/supervisor/config.go index 7c5119cbb..d8fc4db78 100644 --- a/internal/config/supervisor/config.go +++ b/internal/config/supervisor/config.go @@ -100,6 +100,10 @@ func FromPath(ctx context.Context, path string, setAllowedCiphers ptls.SetAllowe return nil, fmt.Errorf("validate tls: %w", err) } + if err := validateAudit(&config.Audit); err != nil { + return nil, fmt.Errorf("validate audit: %w", err) + } + return &config, nil } @@ -214,3 +218,23 @@ func validateServerPort(port *int64) error { } return nil } + +func validateAudit(auditConfig *AuditSpec) error { + const errFmt = "invalid %s format, valid choices are 'enabled', 'disabled', or empty string (equivalent to 'disabled')" + + switch auditConfig.LogUsernamesAndGroups { + case Enabled, Disabled, "": + // no-op + default: + return fmt.Errorf(errFmt, "logUsernamesAndGroups") + } + + switch auditConfig.LogInternalPaths { + case Enabled, Disabled, "": + // no-op + default: + return fmt.Errorf(errFmt, "logInternalPaths") + } + + return nil +} diff --git a/internal/config/supervisor/config_test.go b/internal/config/supervisor/config_test.go index d88344f3b..3492e98c0 100644 --- a/internal/config/supervisor/config_test.go +++ b/internal/config/supervisor/config_test.go @@ -52,6 +52,9 @@ func TestFromPath(t *testing.T) { - foo - bar - TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305 + audit: + logUsernamesAndGroups: enabled + logInternalPaths: enabled `), wantConfig: &Config{ APIGroupSuffix: ptr.To("some.suffix.com"), @@ -86,6 +89,10 @@ func TestFromPath(t *testing.T) { }, }, }, + Audit: AuditSpec{ + LogUsernamesAndGroups: "enabled", + LogInternalPaths: "enabled", + }, }, }, { @@ -123,6 +130,42 @@ func TestFromPath(t *testing.T) { }, }, AggregatedAPIServerPort: ptr.To[int64](10250), + Audit: AuditSpec{ + LogInternalPaths: "", + LogUsernamesAndGroups: "", + }, + }, + }, + { + name: "audit settings can be disabled explicitly", + yaml: here.Doc(` + --- + names: + defaultTLSCertificateSecret: my-secret-name + audit: + logInternalPaths: disabled + logUsernamesAndGroups: disabled + `), + wantConfig: &Config{ + APIGroupSuffix: ptr.To("pinniped.dev"), + Labels: map[string]string{}, + NamesConfig: NamesConfigSpec{ + DefaultTLSCertificateSecret: "my-secret-name", + }, + Endpoints: &Endpoints{ + HTTPS: &Endpoint{ + Network: "tcp", + Address: ":8443", + }, + HTTP: &Endpoint{ + Network: "disabled", + }, + }, + AggregatedAPIServerPort: ptr.To[int64](10250), + Audit: AuditSpec{ + LogInternalPaths: "disabled", + LogUsernamesAndGroups: "disabled", + }, }, }, { @@ -267,6 +310,28 @@ func TestFromPath(t *testing.T) { `), wantError: "validate aggregatedAPIServerPort: must be within range 1024 to 65535", }, + { + name: "invalid audit.logUsernamesAndGroups format", + yaml: here.Doc(` + --- + names: + defaultTLSCertificateSecret: my-secret-name + audit: + logUsernamesAndGroups: this-is-not-a-valid-value + `), + wantError: "validate audit: invalid logUsernamesAndGroups format, valid choices are 'enabled', 'disabled', or empty string (equivalent to 'disabled')", + }, + { + name: "invalid audit.logInternalPaths format", + yaml: here.Doc(` + --- + names: + defaultTLSCertificateSecret: my-secret-name + audit: + logInternalPaths: this-is-not-a-valid-value + `), + wantError: "validate audit: invalid logInternalPaths format, valid choices are 'enabled', 'disabled', or empty string (equivalent to 'disabled')", + }, { name: "returns setAllowedCiphers errors", yaml: here.Doc(` diff --git a/internal/config/supervisor/types.go b/internal/config/supervisor/types.go index 4f94b81d6..f4771078c 100644 --- a/internal/config/supervisor/types.go +++ b/internal/config/supervisor/types.go @@ -7,6 +7,11 @@ import ( "go.pinniped.dev/internal/plog" ) +const ( + Enabled = "enabled" + Disabled = "disabled" +) + // Config contains knobs to set up an instance of the Pinniped Supervisor. type Config struct { APIGroupSuffix *string `json:"apiGroupSuffix,omitempty"` @@ -20,11 +25,18 @@ type Config struct { } type AuditInternalPaths string +type AuditUsernamesAndGroups string -const AuditInternalPathsEnabled = "Enabled" +func (l AuditInternalPaths) Enabled() bool { + return l == Enabled +} +func (l AuditUsernamesAndGroups) Enabled() bool { + return l == Enabled +} type AuditSpec struct { - InternalPaths AuditInternalPaths `json:"internalPaths"` + LogInternalPaths AuditInternalPaths `json:"logInternalPaths"` + LogUsernamesAndGroups AuditUsernamesAndGroups `json:"logUsernamesAndGroups"` } type TLSSpec struct { diff --git a/internal/controller/supervisorstorage/garbage_collector_test.go b/internal/controller/supervisorstorage/garbage_collector_test.go index b879233c2..741b71dcf 100644 --- a/internal/controller/supervisorstorage/garbage_collector_test.go +++ b/internal/controller/supervisorstorage/garbage_collector_test.go @@ -57,7 +57,7 @@ func TestGarbageCollectorControllerInformerFilters(t *testing.T) { nil, secretsInformer, observableWithInformerOption.WithInformer, // make it possible to observe the behavior of the Filters - plog.New(), + nil, ) secretsInformerFilter = observableWithInformerOption.GetFilterForInformer(secretsInformer) }) @@ -139,7 +139,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) { syncContext *controllerlib.Context fakeClock *clocktesting.FakeClock frozenNow time.Time - auditLog *bytes.Buffer + actualAuditLog *bytes.Buffer wantAuditLogs []testutil.WantedAuditLog ) @@ -148,7 +148,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) { var startInformersAndController = func(idpCache dynamicupstreamprovider.DynamicUpstreamIDPProvider) { // Set this at the last second to allow for injection of server override. var auditLogger plog.AuditLogger - auditLogger, auditLog = plog.TestLogger(t) + auditLogger, actualAuditLog = plog.TestAuditLogger(t) subject = GarbageCollectorController( idpCache, fakeClock, @@ -198,7 +198,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) { it.After(func() { cancelContextCancelFunc() - testutil.CompareAuditLogs(t, wantAuditLogs, auditLog.String()) + testutil.CompareAuditLogs(t, wantAuditLogs, actualAuditLog.String()) }) when("there are secrets without the garbage-collect-after annotation", func() { diff --git a/internal/federationdomain/endpoints/auth/auth_handler_test.go b/internal/federationdomain/endpoints/auth/auth_handler_test.go index 313c63123..160c1291f 100644 --- a/internal/federationdomain/endpoints/auth/auth_handler_test.go +++ b/internal/federationdomain/endpoints/auth/auth_handler_test.go @@ -4027,7 +4027,7 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo supervisorClient *supervisorfake.Clientset, kubeClient *fake.Clientset, secretsClient v1.SecretInterface, - auditLog *bytes.Buffer, + actualAuditLog *bytes.Buffer, ) { if test.kubeResources != nil { test.kubeResources(t, supervisorClient, kubeClient) @@ -4118,7 +4118,7 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo if test.wantAuditLogs != nil { wantAuditLogs := test.wantAuditLogs(stateparam.Encoded(actualQueryStateParam), sessionID) testutil.WantAuditIDOnEveryAuditLog(wantAuditLogs, "fake-audit-id") - testutil.CompareAuditLogs(t, wantAuditLogs, auditLog.String()) + testutil.CompareAuditLogs(t, wantAuditLogs, actualAuditLog.String()) } switch { @@ -4177,7 +4177,7 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo if len(test.wantDownstreamAdditionalClaims) > 0 { require.True(t, oidcIDPsCount > 0, "wantDownstreamAdditionalClaims requires at least one OIDC IDP") } - auditLogger, auditLog := plog.TestLogger(t) + auditLogger, actualAuditLog := plog.TestAuditLogger(t) subject := NewHandler( downstreamIssuer, idps, @@ -4186,7 +4186,7 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo test.stateEncoder, test.cookieEncoder, auditLogger, ) - runOneTestCase(t, test, subject, kubeOauthStore, supervisorClient, kubeClient, secretsClient, auditLog) + runOneTestCase(t, test, subject, kubeOauthStore, supervisorClient, kubeClient, secretsClient, actualAuditLog) }) } @@ -4202,7 +4202,7 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo oauthHelperWithRealStorage, kubeOauthStore := createOauthHelperWithRealStorage(secretsClient, oidcClientsClient) oauthHelperWithNullStorage, _ := createOauthHelperWithNullStorage(secretsClient, oidcClientsClient) idpLister := test.idps.BuildFederationDomainIdentityProvidersListerFinder() - auditLogger, auditLog := plog.TestLogger(t) + auditLogger, actualAuditLog := plog.TestAuditLogger(t) subject := NewHandler( downstreamIssuer, idpLister, @@ -4212,8 +4212,8 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo auditLogger, ) - runOneTestCase(t, test, subject, kubeOauthStore, supervisorClient, kubeClient, secretsClient, auditLog) - auditLog.Reset() // clear the log for the next authorize call + runOneTestCase(t, test, subject, kubeOauthStore, supervisorClient, kubeClient, secretsClient, actualAuditLog) + actualAuditLog.Reset() // clear the log for the next authorize call // Call the idpLister's setter to change the upstream IDP settings. newProviderSettings := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder(). @@ -4288,7 +4288,7 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo // modified expectations. This should ensure that the implementation is using the in-memory cache // of upstream IDP settings appropriately in terms of always getting the values from the cache // on every request. - runOneTestCase(t, test, subject, kubeOauthStore, supervisorClient, kubeClient, secretsClient, auditLog) + runOneTestCase(t, test, subject, kubeOauthStore, supervisorClient, kubeClient, secretsClient, actualAuditLog) }) } diff --git a/internal/federationdomain/endpoints/callback/callback_handler_test.go b/internal/federationdomain/endpoints/callback/callback_handler_test.go index 583fdc02f..e2aafc691 100644 --- a/internal/federationdomain/endpoints/callback/callback_handler_test.go +++ b/internal/federationdomain/endpoints/callback/callback_handler_test.go @@ -1853,7 +1853,7 @@ func TestCallbackEndpoint(t *testing.T) { jwksProviderIsUnused := jwks.NewDynamicJWKSProvider() oauthHelper := oidc.FositeOauth2Helper(oauthStore, downstreamIssuer, hmacSecretFunc, jwksProviderIsUnused, timeoutsConfiguration, nil) - logger, log := plog.TestLogger(t) + auditLogger, actualAuditLog := plog.TestAuditLogger(t) subject := NewHandler( test.idps.BuildFederationDomainIdentityProvidersListerFinder(), @@ -1861,7 +1861,7 @@ func TestCallbackEndpoint(t *testing.T) { happyStateCodec, happyCookieCodec, happyUpstreamRedirectURI, - logger, + auditLogger, ) reqContext := context.WithValue(context.Background(), struct{ name string }{name: "test"}, "request-context") @@ -1959,7 +1959,7 @@ func TestCallbackEndpoint(t *testing.T) { if test.wantAuditLogs != nil { wantAuditLogs := test.wantAuditLogs(testutil.GetStateParam(t, test.path), sessionID) testutil.WantAuditIDOnEveryAuditLog(wantAuditLogs, "fake-audit-id") - testutil.CompareAuditLogs(t, wantAuditLogs, log.String()) + testutil.CompareAuditLogs(t, wantAuditLogs, actualAuditLog.String()) } }) } diff --git a/internal/federationdomain/endpoints/login/login_handler_test.go b/internal/federationdomain/endpoints/login/login_handler_test.go index fa753b05f..3182e096f 100644 --- a/internal/federationdomain/endpoints/login/login_handler_test.go +++ b/internal/federationdomain/endpoints/login/login_handler_test.go @@ -449,9 +449,9 @@ func TestLoginEndpoint(t *testing.T) { return test.postHandlerErr } - logger, log := plog.TestLogger(t) + auditLogger, actualAuditLog := plog.TestAuditLogger(t) - subject := NewHandler(happyStateCodec, happyCookieCodec, testGetHandler, testPostHandler, logger) + subject := NewHandler(happyStateCodec, happyCookieCodec, testGetHandler, testPostHandler, auditLogger) subject.ServeHTTP(rsp, req) @@ -468,7 +468,7 @@ func TestLoginEndpoint(t *testing.T) { if test.wantAuditLogs != nil { wantAuditLogs := test.wantAuditLogs(testutil.GetStateParam(t, test.path)) testutil.WantAuditIDOnEveryAuditLog(wantAuditLogs, "fake-audit-id") - testutil.CompareAuditLogs(t, wantAuditLogs, log.String()) + testutil.CompareAuditLogs(t, wantAuditLogs, actualAuditLog.String()) } }) } diff --git a/internal/federationdomain/endpoints/login/post_login_handler_test.go b/internal/federationdomain/endpoints/login/post_login_handler_test.go index 3a0545533..62ab8ebf9 100644 --- a/internal/federationdomain/endpoints/login/post_login_handler_test.go +++ b/internal/federationdomain/endpoints/login/post_login_handler_test.go @@ -1147,7 +1147,9 @@ func TestPostLoginEndpoint(t *testing.T) { rsp := httptest.NewRecorder() - subject := NewPostHandler(downstreamIssuer, tt.idps.BuildFederationDomainIdentityProvidersListerFinder(), oauthHelper, plog.New()) + auditLogger, _ := plog.TestAuditLogger(t) + + subject := NewPostHandler(downstreamIssuer, tt.idps.BuildFederationDomainIdentityProvidersListerFinder(), oauthHelper, auditLogger) err := subject(rsp, req, happyEncodedUpstreamState, tt.decodedState) if tt.wantErr != "" { diff --git a/internal/federationdomain/endpoints/token/token_handler_test.go b/internal/federationdomain/endpoints/token/token_handler_test.go index 4059b68bf..db196ddb2 100644 --- a/internal/federationdomain/endpoints/token/token_handler_test.go +++ b/internal/federationdomain/endpoints/token/token_handler_test.go @@ -5107,18 +5107,18 @@ func exchangeAuthcodeForTokens( test.makeJwksSigningKeyAndProvider = generateJWTSigningKeyAndJWKSProvider } - logger, actualAuditLog := plog.TestLogger(t) + auditLogger, actualAuditLog := plog.TestAuditLogger(t) var oauthHelper fosite.OAuth2Provider // Note that makeHappyOauthHelper() calls simulateAuthEndpointHavingAlreadyRun() to preload the session storage. - oauthHelper, authCode, jwtSigningKey = makeHappyOauthHelper(t, authRequest, oauthStore, test.makeJwksSigningKeyAndProvider, test.customSessionData, test.modifySession, logger) + oauthHelper, authCode, jwtSigningKey = makeHappyOauthHelper(t, authRequest, oauthStore, test.makeJwksSigningKeyAndProvider, test.customSessionData, test.modifySession, auditLogger) subject = NewHandler( idps, oauthHelper, timeoutsConfiguration.OverrideDefaultAccessTokenLifespan, timeoutsConfiguration.OverrideDefaultIDTokenLifespan, - logger, + auditLogger, ) authorizeEndpointGrantedOpenIDScope := strings.Contains(authRequest.Form.Get("scope"), "openid") diff --git a/internal/federationdomain/endpointsmanager/manager.go b/internal/federationdomain/endpointsmanager/manager.go index 5d0d943cd..03141895a 100644 --- a/internal/federationdomain/endpointsmanager/manager.go +++ b/internal/federationdomain/endpointsmanager/manager.go @@ -62,7 +62,7 @@ func NewManager( secretsClient corev1client.SecretInterface, oidcClientsClient v1alpha1.OIDCClientInterface, auditLogger plog.AuditLogger, - auditCfg supervisor.AuditSpec, + auditInternalPathsCfg supervisor.AuditInternalPaths, ) *Manager { m := &Manager{ providerHandlers: make(map[string]http.Handler), @@ -74,7 +74,7 @@ func NewManager( auditLogger: auditLogger, } // nextHandler is the next handler in the chain, called when this manager didn't know how to handle a request - m.buildHandlerChain(nextHandler, auditCfg) + m.buildHandlerChain(nextHandler, auditInternalPathsCfg) return m } @@ -193,11 +193,11 @@ func (m *Manager) SetFederationDomains(federationDomains ...*federationdomainpro } } -func (m *Manager) buildHandlerChain(nextHandler http.Handler, auditCfg supervisor.AuditSpec) { +func (m *Manager) buildHandlerChain(nextHandler http.Handler, auditInternalPathsCfg supervisor.AuditInternalPaths) { // Build the basic handler for FederationDomain endpoints. handler := m.buildManagerHandler(nextHandler) // Log all requests, including audit ID. - handler = requestlogger.WithHTTPRequestAuditLogging(handler, m.auditLogger, auditCfg) + handler = requestlogger.WithHTTPRequestAuditLogging(handler, m.auditLogger, auditInternalPathsCfg) // Add random audit ID to request context and response headers. handler = requestlogger.WithAuditID(handler) m.handlerChain = handler diff --git a/internal/federationdomain/endpointsmanager/manager_test.go b/internal/federationdomain/endpointsmanager/manager_test.go index b92af11da..cfb1a1d48 100644 --- a/internal/federationdomain/endpointsmanager/manager_test.go +++ b/internal/federationdomain/endpointsmanager/manager_test.go @@ -360,6 +360,8 @@ func TestManager(t *testing.T) { cache.SetStateEncoderHashKey(issuer2, []byte("some-state-encoder-hash-key-2")) cache.SetStateEncoderBlockKey(issuer2, []byte("16-bytes-STATE02")) + auditLogger, _ := plog.TestAuditLogger(t) + subject = NewManager( nextHandler, dynamicJWKSProvider, @@ -367,8 +369,8 @@ func TestManager(t *testing.T) { &cache, secretsClient, oidcClientsClient, - plog.New(), - supervisor.AuditSpec{}, + auditLogger, + supervisor.Enabled, ) }) diff --git a/internal/federationdomain/requestlogger/request_logger.go b/internal/federationdomain/requestlogger/request_logger.go index 899ebceda..44c8c089c 100644 --- a/internal/federationdomain/requestlogger/request_logger.go +++ b/internal/federationdomain/requestlogger/request_logger.go @@ -49,9 +49,9 @@ func WithAuditID(handler http.Handler) http.Handler { }) } -func WithHTTPRequestAuditLogging(handler http.Handler, auditLogger plog.AuditLogger, auditCfg supervisor.AuditSpec) http.Handler { +func WithHTTPRequestAuditLogging(handler http.Handler, auditLogger plog.AuditLogger, auditInternalPathsCfg supervisor.AuditInternalPaths) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - rl := newRequestLogger(req, w, auditLogger, time.Now(), auditCfg) + rl := newRequestLogger(req, w, auditLogger, time.Now(), auditInternalPathsCfg) rl.logRequestReceived() defer rl.logRequestComplete() @@ -73,19 +73,25 @@ type requestLogger struct { userAgent string w http.ResponseWriter - auditLogger plog.AuditLogger - auditCfg supervisor.AuditSpec + auditLogger plog.AuditLogger + auditInternalPaths bool } -func newRequestLogger(req *http.Request, w http.ResponseWriter, auditLogger plog.AuditLogger, startTime time.Time, auditCfg supervisor.AuditSpec) *requestLogger { +func newRequestLogger( + req *http.Request, + w http.ResponseWriter, + auditLogger plog.AuditLogger, + startTime time.Time, + auditInternalPathsCfg supervisor.AuditInternalPaths, +) *requestLogger { return &requestLogger{ - req: req, - w: w, - startTime: startTime, - clock: clock.RealClock{}, - userAgent: req.UserAgent(), // cache this from the req to avoid any possibility of concurrent read/write problems with headers map - auditLogger: auditLogger, - auditCfg: auditCfg, + req: req, + w: w, + startTime: startTime, + clock: clock.RealClock{}, + userAgent: req.UserAgent(), // cache this from the req to avoid any possibility of concurrent read/write problems with headers map + auditLogger: auditLogger, + auditInternalPaths: auditInternalPathsCfg.Enabled(), } } @@ -98,7 +104,7 @@ func internalPaths() []string { func (rl *requestLogger) logRequestReceived() { r := rl.req - if rl.auditCfg.InternalPaths != supervisor.AuditInternalPathsEnabled && slices.Contains(internalPaths(), r.URL.Path) { + if !rl.auditInternalPaths && slices.Contains(internalPaths(), r.URL.Path) { return } @@ -148,7 +154,7 @@ func getLocationForAuditLogs(location string) string { func (rl *requestLogger) logRequestComplete() { r := rl.req - if rl.auditCfg.InternalPaths != supervisor.AuditInternalPathsEnabled && slices.Contains(internalPaths(), r.URL.Path) { + if !rl.auditInternalPaths && slices.Contains(internalPaths(), r.URL.Path) { return } diff --git a/internal/federationdomain/requestlogger/request_logger_test.go b/internal/federationdomain/requestlogger/request_logger_test.go index 1ad3e2653..ccf986aed 100644 --- a/internal/federationdomain/requestlogger/request_logger_test.go +++ b/internal/federationdomain/requestlogger/request_logger_test.go @@ -13,7 +13,6 @@ import ( "go.uber.org/mock/gomock" clocktesting "k8s.io/utils/clock/testing" - "go.pinniped.dev/internal/config/supervisor" "go.pinniped.dev/internal/mocks/mockresponsewriter" "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/testutil" @@ -39,52 +38,44 @@ func TestLogRequestReceived(t *testing.T) { } tests := []struct { - name string - path string - auditCfg supervisor.AuditSpec - wantAuditLogs []testutil.WantedAuditLog + name string + path string + auditInternalPaths bool + wantAuditLogs []testutil.WantedAuditLog }{ { - name: "when internal paths are not Enabled, ignores internal paths", - path: "/healthz", - auditCfg: supervisor.AuditSpec{ - InternalPaths: "Disabled", - }, - wantAuditLogs: noAuditEventsWanted, + name: "when internal paths are not enabled, ignores internal paths", + path: "/healthz", + auditInternalPaths: false, + wantAuditLogs: noAuditEventsWanted, }, { - name: "when internal paths are not Enabled, audits external path", - path: "/pretend-to-login", - auditCfg: supervisor.AuditSpec{ - InternalPaths: "Disabled", - }, - wantAuditLogs: happyAuditEventWanted("/pretend-to-login"), + name: "when internal paths are not enabled, audits external path", + path: "/pretend-to-login", + auditInternalPaths: false, + wantAuditLogs: happyAuditEventWanted("/pretend-to-login"), }, { - name: "when internal paths are Enabled, audits internal paths", - path: "/healthz", - auditCfg: supervisor.AuditSpec{ - InternalPaths: "Enabled", - }, - wantAuditLogs: happyAuditEventWanted("/healthz"), + name: "when internal paths are enabled, audits internal paths", + path: "/healthz", + auditInternalPaths: true, + wantAuditLogs: happyAuditEventWanted("/healthz"), }, { - name: "when internal paths are Enabled, audits external paths", - path: "/pretend-to-login", - auditCfg: supervisor.AuditSpec{ - InternalPaths: "Enabled", - }, - wantAuditLogs: happyAuditEventWanted("/pretend-to-login"), + name: "when internal paths are enabled, audits external paths", + path: "/pretend-to-login", + auditInternalPaths: true, + wantAuditLogs: happyAuditEventWanted("/pretend-to-login"), }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { t.Parallel() - logger, log := plog.TestLogger(t) + auditLogger, actualAuditLog := plog.TestAuditLogger(t) subject := requestLogger{ - auditLogger: logger, + auditLogger: auditLogger, req: &http.Request{ Method: "some-method", Proto: "some-proto", @@ -97,13 +88,13 @@ func TestLogRequestReceived(t *testing.T) { ServerName: "some-sni-server-name", }, }, - userAgent: "some-user-agent", - auditCfg: test.auditCfg, + userAgent: "some-user-agent", + auditInternalPaths: test.auditInternalPaths, } subject.logRequestReceived() - testutil.CompareAuditLogs(t, test.wantAuditLogs, log.String()) + testutil.CompareAuditLogs(t, test.wantAuditLogs, actualAuditLog.String()) }) } } @@ -127,45 +118,37 @@ func TestLogRequestComplete(t *testing.T) { } tests := []struct { - name string - path string - location string - auditCfg supervisor.AuditSpec - wantAuditLogs []testutil.WantedAuditLog + name string + path string + location string + auditInternalPaths bool + wantAuditLogs []testutil.WantedAuditLog }{ { - name: "when internal paths are not Enabled, ignores internal paths", - path: "/healthz", - auditCfg: supervisor.AuditSpec{ - InternalPaths: "Disabled", - }, - wantAuditLogs: noAuditEventsWanted, + name: "when internal paths are not enabled, ignores internal paths", + path: "/healthz", + auditInternalPaths: false, + wantAuditLogs: noAuditEventsWanted, }, { - name: "when internal paths are not Enabled, audits external path with location (redacting unknown query params)", - path: "/pretend-to-login", - location: "http://127.0.0.1?foo=bar&foo=quz&lorem=ipsum", - auditCfg: supervisor.AuditSpec{ - InternalPaths: "Disabled", - }, - wantAuditLogs: happyAuditEventWanted("/pretend-to-login", "http://127.0.0.1?foo=redacted&foo=redacted&lorem=redacted"), + name: "when internal paths are not enabled, audits external path with location (redacting unknown query params)", + path: "/pretend-to-login", + location: "http://127.0.0.1?foo=bar&foo=quz&lorem=ipsum", + auditInternalPaths: false, + wantAuditLogs: happyAuditEventWanted("/pretend-to-login", "http://127.0.0.1?foo=redacted&foo=redacted&lorem=redacted"), }, { - name: "when internal paths are Enabled, audits internal paths", - path: "/healthz", - auditCfg: supervisor.AuditSpec{ - InternalPaths: "Enabled", - }, - wantAuditLogs: happyAuditEventWanted("/healthz", "no location header"), + name: "when internal paths are enabled, audits internal paths", + path: "/healthz", + auditInternalPaths: true, + wantAuditLogs: happyAuditEventWanted("/healthz", "no location header"), }, { - name: "when internal paths are Enabled, audits external paths", - path: "/pretend-to-login", - location: "some-location", - auditCfg: supervisor.AuditSpec{ - InternalPaths: "Enabled", - }, - wantAuditLogs: happyAuditEventWanted("/pretend-to-login", "some-location"), + name: "when internal paths are enabled, audits external paths", + path: "/pretend-to-login", + location: "some-location", + auditInternalPaths: true, + wantAuditLogs: happyAuditEventWanted("/pretend-to-login", "some-location"), }, { name: "audits path without location", @@ -203,10 +186,10 @@ func TestLogRequestComplete(t *testing.T) { }) } - logger, log := plog.TestLogger(t) + auditLogger, actualAuditLog := plog.TestAuditLogger(t) subject := requestLogger{ - auditLogger: logger, + auditLogger: auditLogger, startTime: startTime, clock: frozenClock, req: &http.Request{ @@ -214,14 +197,14 @@ func TestLogRequestComplete(t *testing.T) { Path: test.path, }, }, - status: 777, - w: mockResponseWriter, - auditCfg: test.auditCfg, + status: 777, + w: mockResponseWriter, + auditInternalPaths: test.auditInternalPaths, } subject.logRequestComplete() - testutil.CompareAuditLogs(t, test.wantAuditLogs, log.String()) + testutil.CompareAuditLogs(t, test.wantAuditLogs, actualAuditLog.String()) }) } } diff --git a/internal/plog/plog.go b/internal/plog/plog.go index d78022d10..1915b0302 100644 --- a/internal/plog/plog.go +++ b/internal/plog/plog.go @@ -71,8 +71,6 @@ type AuditLogger interface { // If test assertions are desired, Logger should be passed in as an input. New should be used as the // production implementation and TestLogger should be used to write test assertions. type Logger interface { - AuditLogger - Error(msg string, err error, keysAndValues ...any) Warning(msg string, keysAndValues ...any) WarningErr(msg string, err error, keysAndValues ...any) @@ -92,6 +90,7 @@ type Logger interface { // for internal and test use only withDepth(d int) Logger withLogrMod(mod func(logr.Logger) logr.Logger) Logger + audit(msg string, keysAndValues ...any) } // MinLogger is the overlap between Logger and logr.Logger. @@ -107,28 +106,34 @@ type pLogger struct { depth int } +type AuditLogConfig struct { + LogUsernamesAndGroupNames bool +} + +type auditLogger struct { + cfg AuditLogConfig + logger Logger +} + func New() Logger { return pLogger{} } -// Error logs show in the pod log output as `"level":"error","message":"some error msg"` -// where the message text comes from the err parameter. -// They also contain the standard `timestamp` and `caller` keys, along with any other keysAndValues. -// Only when the global log level is configured to "trace" or "all", then they will also include a `stacktrace` key. -// Error logs cannot be suppressed by the global log level configuration. -func (p pLogger) Error(msg string, err error, keysAndValues ...any) { - p.logr().WithCallDepth(p.depth+1).Error(err, msg, keysAndValues...) +func NewAuditLogger(cfg AuditLogConfig) AuditLogger { + return &auditLogger{ + cfg: cfg, + logger: New(), + } } // Audit logs show in the pod log output as `"level":"info","message":"some msg","auditEvent":true` // where the message text comes from the msg parameter. // They also contain the standard `timestamp` and `caller` keys, along with any other keysAndValues. // Only when the global log level is configured to "trace" or "all", then they will also include a `stacktrace` key. -// Audit logs cannot be suppressed by the global log level configuration, but rather can be disabled -// by their own separate configuration. This is because Audit logs should always be printed when they are desired -// by the admin, regardless of global log level, yet the admin should also have a way to entirely disable them -// when they want to avoid potential PII (e.g. usernames) in their pod logs. -func (p pLogger) Audit(msg auditevent.Message, reqCtx context.Context, session SessionIDGetter, keysAndValues ...any) { +// Audit logs cannot be suppressed by the global log level configuration. This is because Audit logs should always +// be printed, regardless of global log level. Audit logs offer their own configuration options, such as a way to +// avoid potential PII (e.g. usernames and group names) in their pod logs. +func (a *auditLogger) Audit(msg auditevent.Message, reqCtx context.Context, session SessionIDGetter, keysAndValues ...any) { // Always add a key/value auditEvent=true. keysAndValues = slices.Concat([]any{"auditEvent", true}, keysAndValues) @@ -148,7 +153,23 @@ func (p pLogger) Audit(msg auditevent.Message, reqCtx context.Context, session S keysAndValues = slices.Concat([]any{"sessionID", sessionID}, keysAndValues) } - p.logr().V(klogLevelWarning).WithCallDepth(p.depth+1).Info(string(msg), keysAndValues...) + a.logger.audit(string(msg), keysAndValues...) +} + +// audit is used internally by AuditLogger to print an audit log event to the pLogger's output. +func (p pLogger) audit(msg string, keysAndValues ...any) { + // Always print log message (klogLevelWarning cannot be suppressed by configuration), + // and always use the Info function because audit logs are not warnings or errors. + p.logr().V(klogLevelWarning).WithCallDepth(p.depth+1).Info(msg, keysAndValues...) +} + +// Error logs show in the pod log output as `"level":"error","message":"some error msg"` +// where the message text comes from the err parameter. +// They also contain the standard `timestamp` and `caller` keys, along with any other keysAndValues. +// Only when the global log level is configured to "trace" or "all", then they will also include a `stacktrace` key. +// Error logs cannot be suppressed by the global log level configuration. +func (p pLogger) Error(msg string, err error, keysAndValues ...any) { + p.logr().WithCallDepth(p.depth+1).Error(err, msg, keysAndValues...) } func (p pLogger) warningDepth(msg string, depth int, keysAndValues ...any) { diff --git a/internal/plog/testing.go b/internal/plog/testing.go index c7b639410..121dbc0f7 100644 --- a/internal/plog/testing.go +++ b/internal/plog/testing.go @@ -71,6 +71,13 @@ func TestLogger(t *testing.T) (Logger, *bytes.Buffer) { &log } +func TestAuditLogger(t *testing.T) (AuditLogger, *bytes.Buffer) { + t.Helper() + + underlyingLogger, logBuf := TestLogger(t) + return &auditLogger{logger: underlyingLogger, cfg: AuditLogConfig{LogUsernamesAndGroupNames: true}}, logBuf +} + func TestConsoleLogger(t *testing.T, w io.Writer) Logger { t.Helper() diff --git a/internal/registry/credentialrequest/rest_test.go b/internal/registry/credentialrequest/rest_test.go index 20f8b6c41..c1c8e5e10 100644 --- a/internal/registry/credentialrequest/rest_test.go +++ b/internal/registry/credentialrequest/rest_test.go @@ -33,7 +33,7 @@ import ( ) func TestNew(t *testing.T) { - r := NewREST(nil, nil, schema.GroupResource{Group: "bears", Resource: "panda"}, plog.New()) + 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()) @@ -70,6 +70,7 @@ func TestCreate(t *testing.T) { var ctrl *gomock.Controller var logger *testutil.TranscriptLogger var originalKLogLevel klog.Level + var auditLogger plog.AuditLogger it.Before(func() { r = require.New(t) @@ -79,6 +80,7 @@ func TestCreate(t *testing.T) { originalKLogLevel = testutil.GetGlobalKlogLevel() // trace.Log() utility will only log at level 2 or above, so set that for this test. testutil.SetGlobalKlogLevel(t, 2) //nolint:staticcheck // old test of code using trace.Log() + auditLogger, _ = plog.TestAuditLogger(t) }) it.After(func() { @@ -104,7 +106,7 @@ func TestCreate(t *testing.T) { 5*time.Minute, ).Return([]byte("test-cert"), []byte("test-key"), nil) - storage := NewREST(requestAuthenticator, clientCertIssuer, schema.GroupResource{}, plog.New()) + storage := NewREST(requestAuthenticator, clientCertIssuer, schema.GroupResource{}, auditLogger) response, err := callCreate(context.Background(), storage, req) @@ -143,7 +145,7 @@ func TestCreate(t *testing.T) { IssueClientCertPEM(gomock.Any(), gomock.Any(), gomock.Any()). Return(nil, nil, fmt.Errorf("some certificate authority error")) - storage := NewREST(requestAuthenticator, clientCertIssuer, schema.GroupResource{}, plog.New()) + storage := NewREST(requestAuthenticator, clientCertIssuer, schema.GroupResource{}, auditLogger) response, err := callCreate(context.Background(), storage, req) requireSuccessfulResponseWithAuthenticationFailureMessage(t, err, response) @@ -156,7 +158,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{}, plog.New()) + storage := NewREST(requestAuthenticator, nil, schema.GroupResource{}, auditLogger) response, err := callCreate(context.Background(), storage, req) @@ -171,7 +173,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{}, plog.New()) + storage := NewREST(requestAuthenticator, nil, schema.GroupResource{}, auditLogger) response, err := callCreate(context.Background(), storage, req) @@ -186,7 +188,7 @@ func TestCreate(t *testing.T) { requestAuthenticator.EXPECT().AuthenticateTokenCredentialRequest(gomock.Any(), req). Return(&user.DefaultInfo{Name: ""}, nil) - storage := NewREST(requestAuthenticator, nil, schema.GroupResource{}, plog.New()) + storage := NewREST(requestAuthenticator, nil, schema.GroupResource{}, auditLogger) response, err := callCreate(context.Background(), storage, req) @@ -205,7 +207,7 @@ func TestCreate(t *testing.T) { Groups: []string{"test-group-1", "test-group-2"}, }, nil) - storage := NewREST(requestAuthenticator, nil, schema.GroupResource{}, plog.New()) + storage := NewREST(requestAuthenticator, nil, schema.GroupResource{}, auditLogger) response, err := callCreate(context.Background(), storage, req) @@ -224,7 +226,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{}, plog.New()) + storage := NewREST(requestAuthenticator, nil, schema.GroupResource{}, auditLogger) response, err := callCreate(context.Background(), storage, req) @@ -234,7 +236,7 @@ func TestCreate(t *testing.T) { it("CreateFailsWhenGivenTheWrongInputType", func() { notACredentialRequest := runtime.Unknown{} - response, err := NewREST(nil, nil, schema.GroupResource{}, plog.New()).Create( + response, err := NewREST(nil, nil, schema.GroupResource{}, auditLogger).Create( genericapirequest.NewContext(), ¬ACredentialRequest, rest.ValidateAllObjectFunc, @@ -245,7 +247,7 @@ func TestCreate(t *testing.T) { }) it("CreateFailsWhenTokenValueIsEmptyInRequest", func() { - storage := NewREST(nil, nil, schema.GroupResource{}, plog.New()) + storage := NewREST(nil, nil, schema.GroupResource{}, auditLogger) response, err := callCreate(context.Background(), storage, credentialRequest(loginapi.TokenCredentialRequestSpec{ Token: "", })) @@ -256,7 +258,7 @@ func TestCreate(t *testing.T) { }) it("CreateFailsWhenValidationFails", func() { - storage := NewREST(nil, nil, schema.GroupResource{}, plog.New()) + storage := NewREST(nil, nil, schema.GroupResource{}, auditLogger) response, err := storage.Create( context.Background(), validCredentialRequest(), @@ -276,7 +278,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{}, plog.New()) + storage := NewREST(requestAuthenticator, successfulIssuer(ctrl), schema.GroupResource{}, auditLogger) response, err := storage.Create( context.Background(), req, @@ -297,7 +299,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{}, plog.New()) + storage := NewREST(requestAuthenticator, successfulIssuer(ctrl), schema.GroupResource{}, auditLogger) validationFunctionWasCalled := false var validationFunctionSawTokenValue string response, err := storage.Create( @@ -317,7 +319,7 @@ func TestCreate(t *testing.T) { }) it("CreateFailsWhenRequestOptionsDryRunIsNotEmpty", func() { - response, err := NewREST(nil, nil, schema.GroupResource{}, plog.New()).Create( + response, err := NewREST(nil, nil, schema.GroupResource{}, auditLogger).Create( genericapirequest.NewContext(), validCredentialRequest(), rest.ValidateAllObjectFunc, @@ -331,7 +333,7 @@ func TestCreate(t *testing.T) { }) it("CreateFailsWhenNamespaceIsNotEmpty", func() { - response, err := NewREST(nil, nil, schema.GroupResource{}, plog.New()).Create( + response, err := NewREST(nil, nil, schema.GroupResource{}, auditLogger).Create( genericapirequest.WithNamespace(genericapirequest.NewContext(), "some-ns"), validCredentialRequest(), rest.ValidateAllObjectFunc, diff --git a/internal/supervisor/server/server.go b/internal/supervisor/server/server.go index 871b9abf3..8307df4db 100644 --- a/internal/supervisor/server/server.go +++ b/internal/supervisor/server/server.go @@ -149,6 +149,7 @@ func prepareControllers( pinnipedInformers supervisorinformers.SharedInformerFactory, leaderElector controllerinit.RunnerWrapper, podInfo *downward.PodInfo, + auditLogger plog.AuditLogger, ) controllerinit.RunnerBuilder { const certificateName string = "pinniped-supervisor-api-tls-serving-certificate" clientSecretSupervisorGroupData := groupsuffix.SupervisorAggregatedGroups(*cfg.APIGroupSuffix) @@ -167,7 +168,7 @@ func prepareControllers( kubeClient, secretInformer, controllerlib.WithInformer, - plog.New(), + auditLogger, ), singletonWorker, ). @@ -451,6 +452,10 @@ func runSupervisor(ctx context.Context, podInfo *downward.PodInfo, cfg *supervis return fmt.Errorf("cannot create k8s client without leader election: %w", err) } + auditLogger := plog.NewAuditLogger(plog.AuditLogConfig{ + LogUsernamesAndGroupNames: cfg.Audit.LogUsernamesAndGroups.Enabled(), + }) + kubeInformers := k8sinformers.NewSharedInformerFactoryWithOptions( client.Kubernetes, defaultResyncInterval, @@ -484,8 +489,8 @@ func runSupervisor(ctx context.Context, podInfo *downward.PodInfo, cfg *supervis &secretCache, clientWithoutLeaderElection.Kubernetes.CoreV1().Secrets(serverInstallationNamespace), // writes to kube storage are allowed for non-leaders client.PinnipedSupervisor.ConfigV1alpha1().OIDCClients(serverInstallationNamespace), - plog.New(), - cfg.Audit, + auditLogger, + cfg.Audit.LogInternalPaths, ) // Get the "real" name of the client secret supervisor API group (i.e., the API group name with the @@ -508,6 +513,7 @@ func runSupervisor(ctx context.Context, podInfo *downward.PodInfo, cfg *supervis pinnipedInformers, leaderElector, podInfo, + auditLogger, ) shutdown := &sync.WaitGroup{}