fix(sanitize): pseudonymize authserver LDAP values - #529
Conversation
Signed-off-by: Parker <infowolfe@gmail.com>
dosubot
Bot
added
the
size:XL
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds LDAP authserver redaction and pseudonymization: new mapper support for per-field authserver mappings, a builtin authserver rule, extended password detection, CLI/docs updates, and expanded tests. No changes to non-authserver sanitization behavior or runtime control flow. Changes
Sequence Diagram(s)(Skipped — changes are rule/map additions and tests without a new multi-component sequential runtime flow that warrants a diagram.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟢 Enforce conventional commitWonderful, this rule succeeded.Require conventional commit format per https://www.conventionalcommits.org/en/v1.0.0/. Skipped for dependabot and dosubot.
🟢 Full CI must passWonderful, this rule succeeded.All CI checks must pass. Activates for non-bot authors, or dependabot when files exist outside .github/workflows/.
🟢 Do not merge outdated PRsWonderful, this rule succeeded.Make sure PRs are within 10 commits of the base branch before merging
|
dosubot
Bot
added
bug
coderabbitai
Bot
added
type:bug
![coderabbitai[bot]](https://webproxy.poorya-velaei-d67.workers.dev/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
unclesp1d3r
left a comment
unclesp1d3r
left a comment
There was a problem hiding this comment.
Hey @infowolfe — thanks for this contribution, really nice work overall. The multi-layer test coverage is solid, the scoping logic for name/host is well thought out, and I appreciate the AI disclosure in the PR description. Two things I'd like addressed before merging:
- Fail-open guard in the Redactor (security maintenance hazard)
rules.go:268-274 — when authServerFieldFromPath returns "", the Redactor returns the original value unredacted:
if authServerField == "" {
return value // ← sensitive value passes through
}Meanwhile, authServerReplacement (mapper.go:354) already has a safe fallback for unknown fields:
default:
return fmt.Sprintf("[AUTHSERVER-%s-%03d]", field, seq)…but it's unreachable because the empty-string guard catches first.
This means if someone later adds a field to FieldPatterns but forgets the corresponding case in authServerFieldFromPath, the value leaks silently with no
log or warning.
Ask: Could you have authServerFieldFromPath return the raw terminal segment for its default case (instead of "") and drop the empty-string guard in the
Redactor? That way, the existing authServerReplacement default handles it in a fail-closed manner. A unit test for the "" / default path would be great too.
- Stale mode descriptions
This PR makes authserver pseudonymization active in all modes, but three descriptions still say minimal is "passwords, keys" / "credentials only":
- internal/sanitizer/rules.go:17 — // ModeMinimal redacts only the most sensitive data (passwords, keys).
- cmd/sanitize.go:32 — same comment
- cmd/sanitize.go:154 — CLI help: # Minimal redaction (credentials only)
Ask: Update these to reflect that minimal now also covers authserver values. Something like credentials and system/authserver values works.
Smaller suggestions (not blocking):
- Doc comment on
authServerFieldFromPath (rules.go:554)— this function has subtle scoping logic (ldap_*matches unconditionally, name/host requires authserver ancestor). A brief doc comment would save future contributors from breaking it. - Mapping file sensitivity note (docs/user-guide/commands/sanitize.md) — the mapping file now contains bind passwords as cleartext keys. A one-liner warning that the mapping file should be treated as sensitive as the original config would be helpful.
- Aggressive mode docs gap (sanitize.md:58-70) — the aggressive section doesn't mention authserver pseudonymization. Since it explicitly lists what it covers, the omission could confuse users.
Thanks again, this is a meaningful security improvement. Happy to help if any of the above is unclear. Also, I'm happy to take care of the smaller items, if you'd like. I just provided those as feedback for community/learning purposes. They're minor.
|
@infowolfe My only feedback items are minor, and I want to get your fix into the release I'm preparing, so I'll just make the minor changes and merge it. Great wor,k and thank you so much for your contribution! |
unclesp1d3r
left a comment
unclesp1d3r
left a comment
There was a problem hiding this comment.
Approved with minor edits for fail-open guard and stale mode description comments.
…iptions Address PR review feedback: remove fail-open guard in authserver Redactor so unknown fields flow through MapAuthServerValue's default handler instead of leaking unredacted. Update ModeMinimal descriptions to reflect authserver coverage. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
191b9df
coderabbitai
Bot
removed
documentation
…mapping Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
|
@Mergifyio queue |
Merge Queue Status
Required conditions to enter a queue
|
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/sanitize.go`:
- Around line 49-50: The help text for the "mode" flag on sanitizeCmd is
inconsistent: the initial description says "minimal (credentials + authserver
values)" but elsewhere the same mode is labeled "Credential-only"; update the
mode flag help/usage strings so both references match by changing the
"Credential-only" label to include authserver values (or alternatively, remove
"Credential-only" and use "minimal (credentials + authserver values)") wherever
the mode descriptions are set; check sanitizeCmd, the mode flag's help string
and any annotation set by setFlagAnnotation(sanitizeCmd.Flags(), "mode", ...)
and ensure all occurrences for the "minimal" enum are consistent.
In `@internal/sanitizer/rules.go`:
- Around line 250-270: The authserver_config rule currently includes
"ldap_bindpw" in its FieldPatterns so ShouldRedactField() matches it first
(CategorySystem) and prevents credential-scoped rules from seeing it; remove
"ldap_bindpw" from the FieldPatterns array in the authserver_config rule and add
a separate credential-scoped rule that matches "ldap_bindpw" (e.g., Name like
"ldap_bindpw_credential"), sets Category to the credentials/credential category,
uses the same Modes as other rules, and a Redactor that maps the value using
your credential-mapping helper (so credential classification and stats pick it
up) instead of Mapper.MapAuthServerValue/authServerFieldFromPath.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 56570a5f-1417-4a00-8e1e-2b456e109e64
📒 Files selected for processing (2)
cmd/sanitize.gointernal/sanitizer/rules.go
| "Sanitization mode: aggressive (public sharing), moderate (internal sharing), minimal (credentials + authserver values)") | ||
| setFlagAnnotation(sanitizeCmd.Flags(), "mode", []string{"sanitize"}) |
There was a problem hiding this comment.
Minimal mode help text is now self-contradictory.
Line 49 correctly says minimal mode includes authserver values, but Line 129 still labels it as “Credential-only” redaction. opnDossier sanitize --help will show two different definitions for the same mode.
Suggested wording
- minimal - Credential-only redaction for trusted environments
+ minimal - Minimal redaction for trusted environments
Redacts: passwords, secrets, API keys, PSKs, private keys, SSH keys,
authserver valuesAs per coding guidelines, "CLI command files: Focus on user experience, clear error messages, and proper flag handling" and "Confirm help text is clear and follows CLI conventions."
Also applies to: 126-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/sanitize.go` around lines 49 - 50, The help text for the "mode" flag on
sanitizeCmd is inconsistent: the initial description says "minimal (credentials
+ authserver values)" but elsewhere the same mode is labeled "Credential-only";
update the mode flag help/usage strings so both references match by changing the
"Credential-only" label to include authserver values (or alternatively, remove
"Credential-only" and use "minimal (credentials + authserver values)") wherever
the mode descriptions are set; check sanitizeCmd, the mode flag's help string
and any annotation set by setFlagAnnotation(sanitizeCmd.Flags(), "mode", ...)
and ensure all occurrences for the "minimal" enum are consistent.
| { | ||
| Name: "authserver_config", | ||
| Description: "Pseudonymizes sensitive system/authserver LDAP values", | ||
| Category: CategorySystem, | ||
| Modes: allModes, | ||
| FieldPatterns: []string{ | ||
| "system.authserver.name", | ||
| "system.authserver.host", | ||
| "ldap_port", | ||
| "ldap_basedn", | ||
| "ldap_authcn", | ||
| "ldap_extended_query", | ||
| "ldap_attr_user", | ||
| "ldap_binddn", | ||
| "ldap_bindpw", | ||
| "ldap_sync_memberof_groups", | ||
| "ldap_sync_default_groups", | ||
| }, | ||
| Redactor: func(m *Mapper, fieldName, value string) string { | ||
| return m.MapAuthServerValue(authServerFieldFromPath(fieldName), value) | ||
| }, |
There was a problem hiding this comment.
Split ldap_bindpw into a credential-scoped rule.
Because ShouldRedactField() stops at the first match, Line 264 means ldap_bindpw is always handled by authserver_config as CategorySystem; the later credential rules never see it. That keeps the replacement safe, but it still regresses credential classification for bind passwords and can skew any credential-specific stats or audit/report output.
Proposed split
+ {
+ Name: "authserver_bindpw",
+ Description: "Pseudonymizes LDAP bind passwords as credentials",
+ Category: CategoryCredentials,
+ Modes: allModes,
+ FieldPatterns: []string{
+ "ldap_bindpw",
+ },
+ Redactor: func(m *Mapper, _, value string) string {
+ return m.MapAuthServerValue(authServerFieldLDAPBindPW, value)
+ },
+ },
{
Name: "authserver_config",
Description: "Pseudonymizes sensitive system/authserver LDAP values",
Category: CategorySystem,
Modes: allModes,
FieldPatterns: []string{
"system.authserver.name",
"system.authserver.host",
"ldap_port",
"ldap_basedn",
"ldap_authcn",
"ldap_extended_query",
"ldap_attr_user",
"ldap_binddn",
- "ldap_bindpw",
"ldap_sync_memberof_groups",
"ldap_sync_default_groups",
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/sanitizer/rules.go` around lines 250 - 270, The authserver_config
rule currently includes "ldap_bindpw" in its FieldPatterns so
ShouldRedactField() matches it first (CategorySystem) and prevents
credential-scoped rules from seeing it; remove "ldap_bindpw" from the
FieldPatterns array in the authserver_config rule and add a separate
credential-scoped rule that matches "ldap_bindpw" (e.g., Name like
"ldap_bindpw_credential"), sets Category to the credentials/credential category,
uses the same Modes as other rules, and a Redactor that maps the value using
your credential-mapping helper (so credential classification and stats pick it
up) instead of Mapper.MapAuthServerValue/authServerFieldFromPath.
Merge Queue Status
This pull request spent 5 minutes 18 seconds in the queue, including 5 minutes 5 seconds running CI. Required conditions to merge
|
coderabbitai
Bot
added
documentation
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/sanitizer/rules.go (1)
250-265:⚠️ Potential issue | 🟠 MajorSplit
ldap_bindpwinto a credential-scoped rule to preserve classificationAt Line 264,
ldap_bindpwis still matched byauthserver_config(CategorySystem). Because first match wins inShouldRedactField, bind passwords won’t be classified as credentials, which regresses credential-specific audit/report classification.Proposed fix
return []Rule{ + { + Name: "authserver_bindpw", + Description: "Pseudonymizes LDAP bind passwords as credentials", + Category: CategoryCredentials, + Modes: allModes, + FieldPatterns: []string{ + "ldap_bindpw", + }, + Redactor: func(m *Mapper, _, value string) string { + return m.MapAuthServerValue(authServerFieldLDAPBindPW, value) + }, + }, { Name: "authserver_config", Description: "Pseudonymizes sensitive system/authserver LDAP values", Category: CategorySystem, Modes: allModes, FieldPatterns: []string{ "system.authserver.name", "system.authserver.host", "ldap_port", "ldap_basedn", "ldap_authcn", "ldap_extended_query", "ldap_attr_user", "ldap_binddn", - "ldap_bindpw", "ldap_sync_memberof_groups", "ldap_sync_default_groups", },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/sanitizer/rules.go` around lines 250 - 265, The authserver_config rule currently includes "ldap_bindpw" so it matches first and prevents credential classification in ShouldRedactField; remove "ldap_bindpw" from the FieldPatterns of the authserver_config rule and add a separate rule (e.g., a new rule named something like "ldap_bind_password" or a credentials-scoped rule) with Category set to credentials and FieldPatterns containing "ldap_bindpw" so that the credential-specific classification/audit logic applies; ensure the new rule uses the same Modes/ordering expectations as other credential rules so it is evaluated before general system rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/sanitizer/rules.go`:
- Around line 250-265: The authserver_config rule currently includes
"ldap_bindpw" so it matches first and prevents credential classification in
ShouldRedactField; remove "ldap_bindpw" from the FieldPatterns of the
authserver_config rule and add a separate rule (e.g., a new rule named something
like "ldap_bind_password" or a credentials-scoped rule) with Category set to
credentials and FieldPatterns containing "ldap_bindpw" so that the
credential-specific classification/audit logic applies; ensure the new rule uses
the same Modes/ordering expectations as other credential rules so it is
evaluated before general system rules.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d543ae77-8721-42de-beee-cb0f4efa7fae
📒 Files selected for processing (2)
internal/sanitizer/mapper_test.gointernal/sanitizer/rules.go
Thanks a lot, sorry, I was falling down a hole of trying to get ceph authentication working so wasn't checking email. It probably would make sense to apply similar redaction/obfuscation to other areas of Much appreciated |
2 participants
Pull Request
Description
Fixes authserver LDAP sanitization so the listed
system/authservervalues are pseudonymized consistently in all sanitize modes, restoresldap_bindpwcredential classification, and aligns tests/docs/CLI text with the actual behavior.AI assistance: used Codex for implementation support. I reviewed and edited the final change and can explain the resulting code and tests.
Type of Change
Related Issues
Testing
Pre-submission Checklist
gofmt)golangci-lint)go test ./...)go mod tidy)Test Commands Executed
just lint just test just check just ci-checkTest Results
All commands passed locally.
Changes Made
Files Modified
internal/sanitizer/rules.gointernal/sanitizer/patterns.gointernal/sanitizer/mapper.gointernal/sanitizer/*_test.gocmd/sanitize.gocmd/sanitize_test.godocs/user-guide/commands/sanitize.mdKey Changes
nameandhostscoped tosystem/authserverldap_bindpwpassword classification while preserving authserver mapping precedenceReview Checklist
For Reviewers
For Contributors
feat/,fix/,docs/, etc.)Documentation
Documentation Updates
API Changes
None.
Breaking Changes
None.
Migration Guide
Not applicable.
Security Considerations
This change strengthens sanitization by pseudonymizing sensitive
system/authserverLDAP values and preventing credential leakage forldap_bindpw.Performance Impact
No meaningful performance impact expected.
Acceptance Criteria
ldap_bindpwis treated as a credential and does not leakAdditional Notes
Mapping output remains human-readable under
mappings.authserver.<field>.Labels
bug-fixdocumentationsecurityBy submitting this pull request, I confirm that:
AI assistance: used Codex for implementation support. I reviewed and edited the final change and can explain the resulting code and tests.