feat(service): enrich auth decision logs with configurable fields from filter metadata #671
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -556,6 +556,58 @@ func (pipeline *AuthPipeline) metricLabels() map[string]string { | |
| return labels | ||
| } | ||
|
|
||
| const loggingFieldPrefix = "logging." | ||
|
|
||
| func truncateValue(s string, maxLen int) string { | ||
| if maxLen > 0 && len(s) > maxLen { | ||
| return s[:maxLen] + "...(truncated)" | ||
|
Comment on lines
+562
to
+563
There was a problem hiding this comment. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n -C6 'logging-fields-max-value-bytes|LOGGING_FIELDS_MAX_VALUE_BYTES|LoggingFieldsMaxValueBytes' .Repository: Kuadrant/authorino Length of output: 5999 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- repository guidance files ---'
find /tmp/coderabbit-repo-knowledge/kuadrant-authorino-98f7ae98 \
-maxdepth 2 -type f -name '*.md' -print
printf '%s\n' '--- auth_pipeline.go target and related helpers ---'
sed -n '540,620p' pkg/service/auth_pipeline.go
rg -n -C4 'func .*truncate|truncateValue|loggingFields\(' pkg/service
printf '%s\n' '--- configuration validation and tests ---'
rg -n -C5 'loggingFieldsMaxValueBytes|LoggingFieldsMaxValueBytes|logging-fields-max-value-bytes|LOGGING_FIELDS_MAX_VALUE_BYTES' \
--glob '!vendor/**' --glob '!node_modules/**' .Repository: Kuadrant/authorino Length of output: 15364 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- flag parsing and validation ---'
rg -n -C8 'func EnvVar|EnvVar\(|Execute\(|loggingFieldsMaxValueBytes' \
--glob '*.go' --glob '!vendor/**' .
printf '%s\n' '--- relevant repository guidance ---'
cat /tmp/coderabbit-repo-knowledge/kuadrant-authorino-98f7ae98/learnings/repo-wide.mdRepository: Kuadrant/authorino Length of output: 20293 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- EnvVar integer conversion ---'
sed -n '27,58p' pkg/utils/envvar.go
printf '%s\n' '--- command construction and execution ---'
rg -n -C10 'func .*Cmd|ExecuteC|Execute\(' main.go cmd --glob '*.go' 2>/dev/null || true
printf '%s\n' '--- logging-field tests near the affected behaviour ---'
sed -n '700,775p' pkg/service/auth_pipeline_test.goRepository: Kuadrant/authorino Length of output: 6861 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- logging-field sink ---'
rg -n -C12 'func \(a \*AuthService\) logAuthResult|loggingFields' pkg/service/auth.goRepository: Kuadrant/authorino Length of output: 4569 Enforce and validate the logging-field value limit.
🤖 Prompt for AI Agents
Author
There was a problem hiding this comment. for bullet 1: This is valid, but negligible. The purpose of this is to prevent multi KB values from hitting stdout, not to enforce the exact byte cap. There was a problem hiding this comment.
The negative-value behavior remains a low-probability misconfiguration case. I will leave the finding open for your disposition. 🐇 ✏️ Learnings added
You are interacting with an AI system. |
||
| } | ||
| return s | ||
| } | ||
|
|
||
| func (pipeline *AuthPipeline) loggingFields(maxValueBytes int) map[string]string { | ||
| fields := make(map[string]string) | ||
|
|
||
| filteredMetadata := pipeline.GetRequest().GetAttributes().GetMetadataContext().GetFilterMetadata() | ||
| if customFields, ok := filteredMetadata["io.kuadrant.logging.fields"]; ok { | ||
|
Collaborator
There was a problem hiding this comment. We should have a way to block specific custom field names that overlap with fields of the current structured log record which already contains I believe the current zap JSON encoder does not de-duplicate keys. Rather, it emits duplicates verbatim. If the downstream JSON parsers keep the last occurrence for each duplicate, then a (malicious) user-defined field Maybe an easy solution could be namespacing all custom field names? |
||
| for k, v := range customFields.Fields { | ||
| key := loggingFieldPrefix + k | ||
| switch kind := v.Kind.(type) { | ||
| case *structpb.Value_StringValue: | ||
| fields[key] = truncateValue(kind.StringValue, maxValueBytes) | ||
|
|
||
| case *structpb.Value_NumberValue: | ||
| fields[key] = fmt.Sprintf("%v", kind.NumberValue) | ||
|
|
||
| case *structpb.Value_BoolValue: | ||
| fields[key] = fmt.Sprintf("%v", kind.BoolValue) | ||
|
|
||
| case *structpb.Value_StructValue: | ||
| if celExprField, ok := kind.StructValue.Fields["cel_expr"]; ok { | ||
|
Collaborator
There was a problem hiding this comment. We're currently looking into enforcing a complexity budget for CEL expressions in an AuthConfig. Because these customisations come from request metadata they could end up dodging the limit enforcement. Perhaps it's out of scope now, but we'll have to find a way to conciliate per-AuthConfig budgets with a complexity that can change per request. My worry here is the added CEL cost on the hot path. |
||
| if exprStr := celExprField.GetStringValue(); exprStr != "" { | ||
| expr, err := cel.NewExpression(exprStr) | ||
| if err != nil { | ||
| pipeline.Logger.Error(err, "failed to parse CEL expression", "expression", exprStr) | ||
| continue | ||
| } | ||
| value, err := expr.ResolveFor(pipeline.GetAuthorizationJSON()) | ||
| if err != nil { | ||
| pipeline.Logger.Error(err, "failed to evaluate CEL expression", "expression", exprStr) | ||
| continue | ||
| } | ||
| fields[key] = truncateValue(fmt.Sprintf("%v", value), maxValueBytes) | ||
| } | ||
| } | ||
|
|
||
| default: | ||
| pipeline.Logger.V(1).Info("unexpected value kind", "kind", kind) | ||
| } | ||
| } | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. I wonder if we should cap the resolved custom values to a maximum length. Especially dynamically resolved values from custom CEL expressions, such as |
||
|
|
||
|
Collaborator
There was a problem hiding this comment. Another sanitisation concern – In |
||
| return fields | ||
| } | ||
|
|
||
| func (pipeline *AuthPipeline) GetRequest() *envoy_auth.CheckRequest { | ||
| return pipeline.Request | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can we ensure this is compatible with redaction of sensitive data as controlled by the
--log-redact-add-field,log-redact-remove-field,--log-redact-add-headerand--log-redact-remove-headerflags?These flags were introduced to configure the mechanism that mitigates risks of exfiltrating sensitive data to debug logs. With this change, production-level logs are subject to the same risks.
How can a sysadmin control which user-defined customisations are allowed and which ones are not?