fix(common): normalize SASL mechanism/bools before validating (lib-streaming parity) - #1809
Merged
Conversation
…reaming parity) The SASL validation compared the raw STREAMING_SASL_MECHANISM against an uppercase allowlist and gated activation on the raw value, but lib-streaming normalizes: the mechanism is case-insensitive and TrimSpace'd (an all-whitespace value means no SASL), and the booleans follow strconv.ParseBool (true/1/t, case-insensitive). So a valid lowercase 'scram-sha-512', a whitespace-padded mechanism, or STREAMING_TLS_ENABLED=True were falsely rejected. Validate on the normalized values: upper(trim(toString mechanism)) for activation + allowlist, trim(username), and case-insensitive true/1/t for TLS and plaintext. The raw value is still emitted as-is (the app normalizes at runtime).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesStreaming SASL validation
Possibly related PRs
✨ Finishing Touches✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/lerian-common/templates/_streaming.tpl`:
- Around line 72-87: Add value-driven Helm render tests covering the
normalization logic around $saslMechNorm, $tlsOn, and $plaintextOn: supported
lowercase and whitespace-padded mechanisms, whitespace-only mechanisms and
usernames, true/1/t boolean forms, unauthorized SASL, unsupported mechanisms,
and preservation of the unchanged raw mechanism output. Assert both successful
rendered manifests and expected template failures without changing the existing
validation behavior.
- Around line 76-82: Normalize explicit nil overrides for
configmap.STREAMING_SASL_MECHANISM and configmap.STREAMING_SASL_USERNAME before
applying toString in the streaming validation block. Ensure nil mechanism and
username values become empty values, so the mechanism follows the missing-value
path and the username fails the required-value check instead of being converted
to “<nil>”.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1f4e7818-e426-4b80-a4be-0372a465d5c6
📒 Files selected for processing (1)
charts/lerian-common/templates/_streaming.tpl
…ooleans The prior lower()/trim() over-normalized: STREAMING_TLS_ENABLED=TrUe passed chart validation but the runtime's GetenvBoolOrDefault uses strconv.ParseBool, which rejects TrUe (and " true ") and falls back to false — so the chart shipped a SASL-without-TLS config that crashes at bootstrap. Accept exactly ParseBool's true set (1/t/T/TRUE/true/True), no lower/trim, so the chart's TLS-or-plaintext decision matches the runtime. The mechanism stays upper(trim(...)) — lib-streaming IS case-insensitive and TrimSpace's the mechanism (unlike the booleans).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/lerian-common/templates/_streaming.tpl`:
- Around line 85-88: The comment in the streaming template incorrectly describes
only the true-value set; update it to document that strconv.ParseBool also
accepts 0, f, F, FALSE, False, and false as valid false values, while other
spellings return an error and GetenvBoolOrDefault falls back to false.
- Around line 89-91: Update the shared streaming configuration flow around
$tlsOn and $plaintextOn so every SASL/TLS value, including raw overrides emitted
by the streaming manifests, passes through the same validation. Route those
override paths through the existing streaming helper or validation logic rather
than rendering values directly, while preserving valid-value behavior and
rejecting invalid settings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bc2b3d4a-efc7-402a-ab0c-338f0aeaa0b3
📒 Files selected for processing (1)
charts/lerian-common/templates/_streaming.tpl
CodeRabbit: an explicit null override (configmap.STREAMING_SASL_MECHANISM/USERNAME: ~) made sprig render nil as '<nil>', so a null mechanism looked 'unsupported' and a null username passed the non-empty check. Coerce the override to '' at resolution (so the emitted value is a valid empty string, not a null ConfigMap value) and in the normalization. Also correct the ParseBool comment: the false set (0/f/F/FALSE/False/false) parses to false; only other spellings error and fall back to the default.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/lerian-common/templates/_streaming.tpl`:
- Around line 79-87: Normalize the SASL mechanism value before it reaches the
secret helper: trim the raw STREAMING_SASL_MECHANISM value and pass the
normalized value through the helper invocation associated with this validation
flow. Ensure whitespace-only mechanisms remain disabled and do not trigger
required username or password validation, while preserving existing
case-insensitive mechanism validation for non-empty values.
- Around line 60-61: Update the SASL field assignments in the streaming template
to coerce only nil or absent values to empty strings, preserving false and
numeric zero until validation. Apply the same nil-only handling to the SASL
values used by the secret helper near its mechanism and username checks, rather
than relying on raw truthiness; use the existing validation flow for preserved
non-string values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 727c97c1-6cad-4356-91a4-aa346e1e16c7
📒 Files selected for processing (1)
charts/lerian-common/templates/_streaming.tpl
…parity)
streaming.env already treats the mechanism via trim(toString(mechanism|default ''))
so a whitespace-only value means 'no SASL'. streaming.secret still gated the
STREAMING_SASL_USERNAME/PASSWORD requirements on the RAW .saslMechanism, so a
whitespace-only (' ') mechanism was disabled by the ConfigMap yet raw-truthy in
the Secret — wrongly forcing both credentials. Normalize the mechanism the same way
here and gate the credential checks on the normalized value; a YAML null/false
collapses to '' (off) on both sides. Fail messages still show the operator's original
input. Addresses CodeRabbit review (streaming.secret raw-truthiness / whitespace).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/lerian-common/templates/_streaming.tpl`:
- Line 179: Update the fail message in the streaming SASL username validation to
format .saslMechanism with the generic value formatter, preserving the existing
message and validation behavior while ensuring boolean and numeric YAML values
display correctly.
- Around line 178-179: Update the SASL credential validation around the
`saslUsername` check in the streaming template to trim whitespace before
evaluating whether a username is present. Ensure whitespace-only values are
rejected when `saslMechanism` is set, preventing password processing without a
usable username.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f604e791-0239-4b7a-a1ca-ed4a80669daf
📒 Files selected for processing (1)
charts/lerian-common/templates/_streaming.tpl
…cret Follow-up to the mechanism normalization: apply streaming.env's full parity in streaming.secret. - Normalize saslUsername the same way (trim(toString(username|default ""))) and gate the STREAMING_SASL_USERNAME requirement on it, so a whitespace-only username is rejected here just as streaming.env rejects it (was raw-truthy → accepted). - Format .saslMechanism in the fail message with %v instead of %s so a non-string YAML value (bool/number) prints as-is instead of %!s(...). Addresses CodeRabbit review on streaming.secret (:179).
… auth downgrade) Gandalf/CodeRabbit blocker: sprig `| default ""` on STREAMING_SASL_MECHANISM collapsed a YAML false/0 to "", so `--set configmap.STREAMING_SASL_MECHANISM=false` rendered STREAMING_SASL_MECHANISM: "" and SILENTLY DISABLED SASL instead of rejecting an invalid mechanism — an authentication downgrade. Coerce the resolved mechanism to "" ONLY when it is nil (kindIs "invalid"), so a null override still avoids the literal "<nil>" (SASL off) but a non-null false/0 is preserved: toString gives "false"/"0", which fails the PLAIN/SCRAM allowlist and aborts the render. String normalization (lowercase, outer/whitespace-only) is unchanged. Also format the unsupported-mechanism value via toString so a non-string prints cleanly (was %!q(bool=false)). Negative assertions for false/0 (and the normalization matrix) land in the consumer render-assertion gate in #1741 (the harness does not exist on this library branch). Addresses the Gandalf review on HEAD 6940159.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the edge case raised on midaz #1741 (Gandalf): the SASL validation rejects valid mechanisms in lowercase or with surrounding whitespace.
Problem
lib-streamingtreats the SASL mechanism as case-insensitive andTrimSpaces it (an all-whitespace value = no SASL), and parses the booleans withstrconv.ParseBool(true/1/t, case-insensitive) — seeinternal/config/config.go(SASLMechanism is one of PLAIN, SCRAM-SHA-256, SCRAM-SHA-512 (case-insensitive)). The chart compared the raw value against an uppercase allowlist and gated activation on the raw value, so:scram-sha-512(lowercase) + TLS + creds → falsely fails;SCRAM-SHA-512(padded) → falsely fails;STREAMING_TLS_ENABLED=True→ falsely fails "SASL requires TLS".Fix
Validate on the normalized values:
upper(trim(toString mechanism))for both the activation decision and the allowlist,trim(username)for the required check, and case-insensitivetrue/1/tfor TLS and the plaintext opt-in. The raw value is still emitted as-is (the app normalizes at runtime).Validation
scram-sha-512+ user + TLSSCRAM-SHA-512+ user + TLSSTREAMING_TLS_ENABLED=TrueGSSAPIhelm lintpasses. Lowercase + whitespace-only assertions land with the re-pin in #1741.