pkg/authorization: prevent double audit logs

kcp-dev · Dec 21, 2022 · bf6ba0a · bf6ba0a
1 parent abf5e51
commit bf6ba0a
Show file tree

Hide file tree

Showing 3 changed files with 53 additions and 22 deletions.
diff --git a/pkg/authorization/decorator.go b/pkg/authorization/decorator.go
@@ -54,7 +54,8 @@ func NewDecorator(key string, target authorizer.Authorizer) *Decorator {
 
 // AddAuditLogging logs every decision of the target authorizer for the given audit prefix key
 // if the decision is not allowed.
-// All authorizer decisions are being logged in the audit log.
+// All authorizer decisions are being logged in the audit log if the context was set using EnableAuditLogging.
+// This prevents double audit log entries by multiple invocations of the authorizer chain.
 func (d *Decorator) AddAuditLogging() *Decorator {
 	target := d.target
 	d.target = authorizer.AuthorizerFunc(func(ctx context.Context, attr authorizer.Attributes) (authorizer.Decision, string, error) {
@@ -65,11 +66,14 @@ func (d *Decorator) AddAuditLogging() *Decorator {
 			auditReasonMsg = fmt.Sprintf("reason: %v, error: %v", reason, err)
 		}
 
-		kaudit.AddAuditAnnotations(
-			ctx,
-			d.key+"/"+auditDecision, decisionString(dec),
-			d.key+"/"+auditReason, auditReasonMsg,
-		)
+		prefixKey := ctx.Value(auditLoggingKey)
+		if prefixKey != nil && prefixKey.(bool) {
+			kaudit.AddAuditAnnotations(
+				ctx,
+				d.key+"/"+auditDecision, decisionString(dec),
+				d.key+"/"+auditReason, auditReasonMsg,
+			)
+		}
 
 		if dec != authorizer.DecisionAllow {
 			// Note: this deviates from upstream which doesn't log audit reasons.
@@ -142,3 +146,19 @@ func DelegateAuthorization(delegationReason string, delegate authorizer.Authoriz
 		return dec, "delegating due to " + delegationReason + ": " + delegateReason, err
 	})
 }
+
+type auditLoggingKeyType int
+
+const (
+	auditLoggingKey auditLoggingKeyType = iota
+)
+
+// EnableAuditLogging sets a context value that enables audit logging for the given authorizer chain.
+// If that context is not set, audit logging is skipped.
+// Note that this is only respected by authorizers that have been decorated using Decorator.AddAuditLogging.
+func EnableAuditLogging(delegate authorizer.Authorizer) authorizer.Authorizer {
+	return authorizer.AuthorizerFunc(func(ctx context.Context, a authorizer.Attributes) (authorizer.Decision, string, error) {
+		ctx = context.WithValue(ctx, auditLoggingKey, true)
+		return delegate.Authorize(ctx, a)
+	})
+}
diff --git a/pkg/authorization/decorator_test.go b/pkg/authorization/decorator_test.go
@@ -49,7 +49,7 @@ func TestDecorator(t *testing.T) {
 		wantReason   string
 	}{
 		"topAllows": {
-			authz: NewDecorator("top", alwaysAllow).AddAuditLogging().AddAnonymization().AddReasonAnnotation(),
+			authz: EnableAuditLogging(NewDecorator("top", alwaysAllow).AddAuditLogging().AddAnonymization().AddReasonAnnotation()),
 
 			wantDecision: authorizer.DecisionAllow,
 			wantReason:   "top: access granted",
@@ -59,8 +59,16 @@ func TestDecorator(t *testing.T) {
 				"top/reason":   "unanonymized allow",
 			},
 		},
+		"topAllowsWithoutAudit": {
+			authz: NewDecorator("top", alwaysAllow).AddAuditLogging().AddAnonymization().AddReasonAnnotation(),
+
+			wantDecision: authorizer.DecisionAllow,
+			wantReason:   "top: access granted",
+
+			wantAudit: nil,
+		},
 		"topAllowsWithoutReasonAnnotation": {
-			authz: NewDecorator("top", alwaysAllow).AddAuditLogging().AddAnonymization(),
+			authz: EnableAuditLogging(NewDecorator("top", alwaysAllow).AddAuditLogging().AddAnonymization()),
 
 			wantDecision: authorizer.DecisionAllow,
 			wantReason:   "access granted",
@@ -71,7 +79,7 @@ func TestDecorator(t *testing.T) {
 			},
 		},
 		"topAllowsWithoutReasonAnnotationWithoutAnonymization": {
-			authz: NewDecorator("top", alwaysAllow).AddAuditLogging(),
+			authz: EnableAuditLogging(NewDecorator("top", alwaysAllow).AddAuditLogging()),
 
 			wantDecision: authorizer.DecisionAllow,
 			wantReason:   "unanonymized allow",
@@ -82,18 +90,18 @@ func TestDecorator(t *testing.T) {
 			},
 		},
 		"topAllowsWithoutReasonAnnotationWithoutAnonymizationWithoutAuditLogging": {
-			authz: NewDecorator("top", alwaysAllow),
+			authz: EnableAuditLogging(NewDecorator("top", alwaysAllow)),
 
 			wantDecision: authorizer.DecisionAllow,
 			wantReason:   "unanonymized allow",
 
 			wantAudit: nil,
 		},
 		"topDelegatesToAllow": {
-			authz: NewDecorator("top",
+			authz: EnableAuditLogging(NewDecorator("top",
 				DelegateAuthorization("top-to-bottom",
 					NewDecorator("bottom", alwaysAllow).AddAuditLogging().AddAnonymization().AddReasonAnnotation()),
-			).AddAuditLogging().AddAnonymization(),
+			).AddAuditLogging().AddAnonymization()),
 
 			wantDecision: authorizer.DecisionAllow,
 			wantReason:   "access granted",
@@ -107,10 +115,10 @@ func TestDecorator(t *testing.T) {
 			},
 		},
 		"topDelegatesToDeny": {
-			authz: NewDecorator("top",
+			authz: EnableAuditLogging(NewDecorator("top",
 				DelegateAuthorization("top-to-bottom",
 					NewDecorator("bottom", alwaysDeny).AddAuditLogging().AddAnonymization().AddReasonAnnotation()),
-			).AddAuditLogging().AddAnonymization(),
+			).AddAuditLogging().AddAnonymization()),
 
 			wantDecision: authorizer.DecisionDeny,
 			wantReason:   "access denied",
@@ -124,11 +132,11 @@ func TestDecorator(t *testing.T) {
 			},
 		},
 		"topDelegatesToDelegateDelegatesToDeny": {
-			authz: NewDecorator("top",
+			authz: EnableAuditLogging(NewDecorator("top",
 				DelegateAuthorization("top-to-middle", NewDecorator("middle",
 					DelegateAuthorization("middle-to-bottom", NewDecorator("bottom", alwaysDeny).AddAuditLogging().AddAnonymization().AddReasonAnnotation()),
 				).AddAuditLogging().AddAnonymization().AddReasonAnnotation()),
-			).AddAuditLogging().AddAnonymization(),
+			).AddAuditLogging().AddAnonymization()),
 
 			wantDecision: authorizer.DecisionDeny,
 			wantReason:   "access denied",
@@ -145,11 +153,11 @@ func TestDecorator(t *testing.T) {
 			},
 		},
 		"topDelegatesToDelegateDelegatesToAllow": {
-			authz: NewDecorator("top",
+			authz: EnableAuditLogging(NewDecorator("top",
 				DelegateAuthorization("top-to-middle", NewDecorator("middle",
 					DelegateAuthorization("middle-to-bottom", NewDecorator("bottom", alwaysAllow).AddAuditLogging().AddAnonymization().AddReasonAnnotation()),
 				).AddAuditLogging().AddAnonymization().AddReasonAnnotation()),
-			).AddAuditLogging().AddAnonymization(),
+			).AddAuditLogging().AddAnonymization()),
 
 			wantDecision: authorizer.DecisionAllow,
 			wantReason:   "access granted",
@@ -166,11 +174,11 @@ func TestDecorator(t *testing.T) {
 			},
 		},
 		"topDelegatesToDelegateDelegatesToNoOpinion": {
-			authz: NewDecorator("top",
+			authz: EnableAuditLogging(NewDecorator("top",
 				DelegateAuthorization("top-to-middle", NewDecorator("middle",
 					DelegateAuthorization("middle-to-bottom", NewDecorator("bottom", alwaysNoOpinion).AddAuditLogging().AddAnonymization().AddReasonAnnotation()),
 				).AddAuditLogging().AddAnonymization().AddReasonAnnotation()),
-			).AddAuditLogging().AddAnonymization(),
+			).AddAuditLogging().AddAnonymization()),
 
 			wantDecision: authorizer.DecisionNoOpinion,
 			wantReason:   "access denied",
@@ -187,11 +195,11 @@ func TestDecorator(t *testing.T) {
 			},
 		},
 		"topDelegatesToDelegateDelegatesToError": {
-			authz: NewDecorator("top",
+			authz: EnableAuditLogging(NewDecorator("top",
 				DelegateAuthorization("top-to-middle", NewDecorator("middle",
 					DelegateAuthorization("middle-to-bottom", NewDecorator("bottom", alwaysError).AddAuditLogging().AddAnonymization().AddReasonAnnotation()),
 				).AddAuditLogging().AddAnonymization().AddReasonAnnotation()),
-			).AddAuditLogging().AddAnonymization(),
+			).AddAuditLogging().AddAnonymization()),
 
 			wantDecision: authorizer.DecisionNoOpinion,
 			wantReason:   "access denied",

diff --git a/pkg/server/config.go b/pkg/server/config.go
@@ -360,7 +360,10 @@ func NewConfig(opts *kcpserveroptions.CompletedOptions) (*Config, error) {
 			}
 		}
 
+		authorizerWithoutAudit := genericConfig.Authorization.Authorizer
+		genericConfig.Authorization.Authorizer = authorization.EnableAuditLogging(genericConfig.Authorization.Authorizer)
 		apiHandler = genericapiserver.DefaultBuildHandlerChainBeforeAuthz(apiHandler, genericConfig)
+		genericConfig.Authorization.Authorizer = authorizerWithoutAudit
 
 		// this will be replaced in DefaultBuildHandlerChain. So at worst we get twice as many warning.
 		// But this is not harmful as the kcp warnings are not many.