carverauto/serviceradar

Fork 0

fixing kv watcher in core and cert issues with tools container #2477

Merged

mfreeman451 merged 1 commit from refs/pull/2477/head into main

2025-11-24 23:58:26 +00:00

mfreeman451 commented

2025-11-24 23:55:58 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #2015
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2015
Original created: 2025-11-24T23:55:58Z
Original updated: 2025-11-24T23:58:32Z
Original head: carverauto/serviceradar:2014-bug-serviceradar-core-not-reading-from-secret-for-admin-pw-in-demo-namespace
Original base: main
Original merged: 2025-11-24T23:58:26Z by @mfreeman451

User description

IMPORTANT: Please sign the Developer Certificate of Origin

Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include
a DCO sign-off statement indicating the DCO acceptance in one commit message. Here
is an example DCO Signed-off-by line in a commit message:

Signed-off-by: J. Doe <j.doe@domain.com>

Describe your changes

Issue ticket number and link

Code checklist before requesting a review

I have signed the DCO?
The build completes without errors?
All tests are passing when running make test?

PR Type

Bug fix, Enhancement

Description

Make KV config watch optional via CONFIG_WATCH_ENABLED environment variable
Strip sensitive auth fields (local_users, jwt_secret) from KV overlays to preserve secret-derived values
Fix tools container DNS references to use short names instead of FQDN
Generate debug client certificate for tools container with proper SAN entries

Diagram Walkthrough

flowchart LR
  A["CONFIG_WATCH_ENABLED<br/>Environment Variable"] -->|controls| B["Core Service<br/>KV Watch"]
  C["KV Store<br/>Config Data"] -->|overlay| D["dropSensitiveAuthFields<br/>Function"]
  D -->|strips local_users<br/>jwt_secret| E["Merged Config<br/>with Secrets Preserved"]
  F["Tools Container<br/>DNS References"] -->|updated to<br/>short names| G["Service Discovery<br/>Works Correctly"]
  H["Debug Client<br/>Certificate"] -->|generated with<br/>proper SANs| I["Tools Container<br/>mTLS Ready"]

File Walkthrough

Relevant files

Enhancement

4 files

app.go `Add DisableWatch option to core app`	+2/-1
main.go `Parse CONFIG_WATCH_ENABLED environment variable`	+16/-0
generate-certs.sh `Generate debug client certificate for tools`	+1/-0
cert-scripts-configmap.yaml `Generate debug client certificate for demo environment`	+1/-0

Bug fix

3 files

kv_client.go `Strip sensitive auth fields from KV overlays`	+40/-0
kv_watch.go `Apply auth field stripping in KV watch`	+2/-0
serviceradar-tools.yaml `Fix DNS references to use short service names`	+13/-13

Tests

1 files

kv_client_test.go
Test auth secrets stripping in overlay config +42/-0

Configuration changes

3 files

docker-compose.yml `Add CONFIG_WATCH_ENABLED environment variable`	+1/-0
core.yaml `Add CONFIG_WATCH_ENABLED helm template variable`	+2/-0
values.yaml `Add configWatchEnabled helm value for core service`	+3/-0

Imported from GitHub pull request. Original GitHub pull request: #2015 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2015 Original created: 2025-11-24T23:55:58Z Original updated: 2025-11-24T23:58:32Z Original head: carverauto/serviceradar:2014-bug-serviceradar-core-not-reading-from-secret-for-admin-pw-in-demo-namespace Original base: main Original merged: 2025-11-24T23:58:26Z by @mfreeman451 --- ### **User description** ## IMPORTANT: Please sign the Developer Certificate of Origin Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include a [DCO sign-off statement]( https://developercertificate.org/) indicating the DCO acceptance in one commit message. Here is an example DCO Signed-off-by line in a commit message: ``` Signed-off-by: J. Doe <j.doe@domain.com> ``` ## Describe your changes ## Issue ticket number and link ## Code checklist before requesting a review - [ ] I have signed the DCO? - [ ] The build completes without errors? - [ ] All tests are passing when running make test? ___ ### **PR Type** Bug fix, Enhancement ___ ### **Description** - Make KV config watch optional via `CONFIG_WATCH_ENABLED` environment variable - Strip sensitive auth fields (`local_users`, `jwt_secret`) from KV overlays to preserve secret-derived values - Fix tools container DNS references to use short names instead of FQDN - Generate debug client certificate for tools container with proper SAN entries ___ ### Diagram Walkthrough ```mermaid flowchart LR A["CONFIG_WATCH_ENABLED<br/>Environment Variable"] -->|controls| B["Core Service<br/>KV Watch"] C["KV Store<br/>Config Data"] -->|overlay| D["dropSensitiveAuthFields<br/>Function"] D -->|strips local_users<br/>jwt_secret| E["Merged Config<br/>with Secrets Preserved"] F["Tools Container<br/>DNS References"] -->|updated to<br/>short names| G["Service Discovery<br/>Works Correctly"] H["Debug Client<br/>Certificate"] -->|generated with<br/>proper SANs| I["Tools Container<br/>mTLS Ready"] ``` <details> <summary><h3> File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Enhancement</strong></td><td><details><summary>4 files</summary><table> <tr> <td><strong>app.go</strong><dd><code>Add DisableWatch option to core app</code>                                            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2015/files#diff-4ad8a289575edf3b163088617b7a40ae1305c29ced0c7d59b3751c57d6938072">+2/-1</a>      </td> </tr> <tr> <td><strong>main.go</strong><dd><code>Parse CONFIG_WATCH_ENABLED environment variable</code>                    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2015/files#diff-4ab3fd1d4debc53dd2499d94a0f60c648fdae4235dd1e3678095a975f5bb434a">+16/-0</a>    </td> </tr> <tr> <td><strong>generate-certs.sh</strong><dd><code>Generate debug client certificate for tools</code>                            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2015/files#diff-224edb1896351749211a2a609692daa31f4f45cf175658124613b9dc08496d96">+1/-0</a>      </td> </tr> <tr> <td><strong>cert-scripts-configmap.yaml</strong><dd><code>Generate debug client certificate for demo environment</code>      </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2015/files#diff-6cfdb89093e1dc013b04ac606d8bfd64e3079419d3b6c94c3eed6b12531bafbb">+1/-0</a>      </td> </tr> </table></details></td></tr><tr><td><strong>Bug fix</strong></td><td><details><summary>3 files</summary><table> <tr> <td><strong>kv_client.go</strong><dd><code>Strip sensitive auth fields from KV overlays</code>                          </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2015/files#diff-0c911990aa32d43404f0f23dadc0363ec97ed4ca315592e6c9bd24510e82e06d">+40/-0</a>    </td> </tr> <tr> <td><strong>kv_watch.go</strong><dd><code>Apply auth field stripping in KV watch</code>                                      </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2015/files#diff-6d4dd0c34977dc5f8eda8c3a62075478ae14ab26d45bbcdd24974310ee839e84">+2/-0</a>      </td> </tr> <tr> <td><strong>serviceradar-tools.yaml</strong><dd><code>Fix DNS references to use short service names</code>                        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2015/files#diff-3912b6a6e1e46079148296c1c5cf6742fda7437ac0b73ad8091824d700676758">+13/-13</a>  </td> </tr> </table></details></td></tr><tr><td><strong>Tests</strong></td><td><details><summary>1 files</summary><table> <tr> <td><strong>kv_client_test.go</strong><dd><code>Test auth secrets stripping in overlay config</code>                        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2015/files#diff-09814dd9ab05f9d40f40974f297806d71fe2eca5f8120492c9f79311153d3aef">+42/-0</a>    </td> </tr> </table></details></td></tr><tr><td><strong>Configuration changes</strong></td><td><details><summary>3 files</summary><table> <tr> <td><strong>docker-compose.yml</strong><dd><code>Add CONFIG_WATCH_ENABLED environment variable</code>                        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2015/files#diff-e45e45baeda1c1e73482975a664062aa56f20c03dd9d64a827aba57775bed0d3">+1/-0</a>      </td> </tr> <tr> <td><strong>core.yaml</strong><dd><code>Add CONFIG_WATCH_ENABLED helm template variable</code>                    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2015/files#diff-06ab387d2c169d82a1de28b5e66c86f0417bd81b82a96246d0a2da8bfaa8d224">+2/-0</a>      </td> </tr> <tr> <td><strong>values.yaml</strong><dd><code>Add configWatchEnabled helm value for core service</code>              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2015/files#diff-d4449c7cb70362554b274f81eae5a4b81a8e81df494282e383d1b7ea3871c452">+3/-0</a>      </td> </tr> </table></details></td></tr></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-11-24 23:56:35 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2015#issuecomment-3573202101
Original created: 2025-11-24T23:56:35Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance

⚪

Secrets overlay risk

Description: The list of stripped auth keys in KV overlays is hardcoded to "local_users" and
"jwt_secret", which risks missing other sensitive fields and unintentionally allowing
secrets to be overlaid from KV if field names differ or new fields are added.
kv_client.go [498-532]

Referred Code

func dropSensitiveAuthFields(data []byte) []byte {
	if len(data) == 0 {
		return data
	}

	var doc map[string]interface{}
	if err := json.Unmarshal(data, &doc); err != nil {
		return data
	}

	auth, ok := doc["auth"].(map[string]interface{})
	if !ok {
		return data
	}

	changed := false
	for _, key := range []string{"local_users", "jwt_secret"} {
		if _, exists := auth[key]; exists {
			delete(auth, key)
			changed = true
		}


 ... (clipped 14 lines)

Ticket Compliance

🟡

🎫 #2014

🟢	Core should read admin credentials and other auth-sensitive values from secrets, not be overridden by KV overlays.
	Make KV config watching behavior controllable so that environments (e.g., demo) can disable or enable watching as needed.
	Ensure tools container connectivity and certificates work correctly in cluster using appropriate DNS names/SANs.
⚪	Validate in a demo namespace that admin password indeed remains sourced from secret after KV overlays and during watch updates.
⚪	Manually verify tools pod can connect via short DNS names and that the generated client certificate SANs satisfy mTLS handshakes against all services.

Codebase Duplication Compliance

⚪

Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance

🟢

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

⚪

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing audit logs: Newly added sensitive overlay filtering and KV watch behavior do not include explicit
audit logging of critical config changes or watch enablement/disablement, which may be
acceptable if logging is implemented elsewhere.

Referred Code

if data, found, err := kvStore.Get(ctx, key); err == nil && found && len(data) > 0 {
	data = dropSensitiveAuthFields(data)
	if err := MergeOverlayBytes(dst, data); err != nil {
		if watcherID != "" {
			MarkWatcherEvent(watcherID, err)
		}
		if log != nil {
			log.Warn().Err(err).Str("key", key).Msg("Failed initial KV overlay")
		}
	} else if onChange != nil {
		if watcherID != "" {
			MarkWatcherEvent(watcherID, nil)
		}
		onChange()
	}
}

ch, err := kvStore.Watch(ctx, key)
if err != nil {
	if log != nil {
		log.Warn().Err(err).Str("key", key).Msg("KV watch failed")


 ... (clipped 23 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2015#issuecomment-3573202101 Original created: 2025-11-24T23:56:35Z --- ## PR Compliance Guide 🔍  Below is a summary of compliance checks for this PR:<br> <table><tbody><tr><td colspan='2'><strong>Security Compliance</strong></td></tr> <tr><td rowspan=1>⚪</td> <td><details><summary><strong>Secrets overlay risk </strong></summary><br> <b>Description:</b> The list of stripped auth keys in KV overlays is hardcoded to "local_users" and <br>"jwt_secret", which risks missing other sensitive fields and unintentionally allowing <br>secrets to be overlaid from KV if field names differ or new fields are added.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/2015/files#diff-0c911990aa32d43404f0f23dadc0363ec97ed4ca315592e6c9bd24510e82e06dR498-R532'>kv_client.go [498-532]</a></strong><br> <details open><summary>Referred Code</summary> ```go func dropSensitiveAuthFields(data []byte) []byte { if len(data) == 0 { return data } var doc map[string]interface{} if err := json.Unmarshal(data, &doc); err != nil { return data } auth, ok := doc["auth"].(map[string]interface{}) if !ok { return data } changed := false for _, key := range []string{"local_users", "jwt_secret"} { if _, exists := auth[key]; exists { delete(auth, key) changed = true } ... (clipped 14 lines) ``` </details></details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>🟡</td> <td> <details> <summary>🎫 <a href=https://github.com/carverauto/serviceradar/issues/2014>#2014</a></summary> <table width='100%'><tbody> <tr><td rowspan=3>🟢</td> <td>Core should read admin credentials and other auth-sensitive values from secrets, not be <br>overridden by KV overlays.</td></tr> <tr><td>Make KV config watching behavior controllable so that environments (e.g., demo) can <br>disable or enable watching as needed.</td></tr> <tr><td>Ensure tools container connectivity and certificates work correctly in cluster using <br>appropriate DNS names/SANs.</td></tr> <tr><td rowspan=2>⚪</td> <td>Validate in a demo namespace that admin password indeed remains sourced from secret after <br>KV overlays and during watch updates.</td></tr> <tr><td>Manually verify tools pod can connect via short DNS names and that the generated client <br>certificate SANs satisfy mTLS handshakes against all services.</td></tr> </tbody></table> </details> </td></tr> <tr><td colspan='2'><strong>Codebase Duplication Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary><strong>Codebase context is not defined </strong></summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/core-abilities/rag_context_enrichment/'>guide</a> to enable codebase context checks. </details></td></tr> <tr><td colspan='2'><strong>Custom Compliance</strong></td></tr> <tr><td rowspan=5>🟢</td><td> <details><summary><strong>Generic: Meaningful Naming and Self-Documenting Code</strong></summary><br> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code <br>self-documenting<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Robust Error Handling and Edge Case Management</strong></summary><br> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful <br>degradation<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Secure Error Handling</strong></summary><br> **Objective:** To prevent the leakage of sensitive system information through error messages while <br>providing sufficient detail for internal debugging.<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Secure Logging Practices</strong></summary><br> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive <br>information like PII, PHI, or cardholder data.<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Security-First Input Validation and Data Handling</strong></summary><br> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent <br>vulnerabilities<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td rowspan=1>⚪</td> <td><details> <summary><strong>Generic: Comprehensive Audit Trails</strong></summary><br> **Objective:** To create a detailed and reliable record of critical system actions for security analysis <br>and compliance.<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/2015/files#diff-6d4dd0c34977dc5f8eda8c3a62075478ae14ab26d45bbcdd24974310ee839e84R60-R103'><strong>Missing audit logs</strong></a>: Newly added sensitive overlay filtering and KV watch behavior do not include explicit <br>audit logging of critical config changes or watch enablement/disablement, which may be <br>acceptable if logging is implemented elsewhere.<br> <details open><summary>Referred Code</summary> ```go if data, found, err := kvStore.Get(ctx, key); err == nil && found && len(data) > 0 { data = dropSensitiveAuthFields(data) if err := MergeOverlayBytes(dst, data); err != nil { if watcherID != "" { MarkWatcherEvent(watcherID, err) } if log != nil { log.Warn().Err(err).Str("key", key).Msg("Failed initial KV overlay") } } else if onChange != nil { if watcherID != "" { MarkWatcherEvent(watcherID, nil) } onChange() } } ch, err := kvStore.Watch(ctx, key) if err != nil { if log != nil { log.Warn().Err(err).Str("key", key).Msg("KV watch failed") ... (clipped 23 lines) ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td align="center" colspan="2">   </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant<br> 🟡 - Partial Compliant<br> 🔴 - Not Compliant<br> ⚪ - Requires Further Human Verification<br> 🏷️ - Compliance label<br> </details>

qodo-code-review[bot] commented

2025-11-24 23:57:27 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2015#issuecomment-3573203600
Original created: 2025-11-24T23:57:27Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Security	Prevent returning data with secrets Modify the `dropSensitiveAuthFields` function to return `nil` instead of the original data if JSON marshalling fails, preventing the accidental exposure of sensitive information. pkg/config/kv_client.go [525-531] `doc["auth"] = auth out, err := json.Marshal(doc) if err != nil { - return data + // Returning nil is safer than returning the original data with secrets. + // The caller will then skip the overlay. + return nil } return out` Apply / Chat Suggestion importance[1-10]: 8 __ Why: This suggestion correctly identifies a security flaw where the function could return sensitive data on a marshalling error, and the proposed fix of returning `nil` is a robust way to prevent this.	Medium
High-level	Generalize the secret stripping logic The current method of stripping sensitive fields like `local_users` and `jwt_secret` is hardcoded. This should be generalized using struct tags or a configuration list to improve scalability and maintainability. Examples: pkg/config/kv_client.go [496-532] `// dropSensitiveAuthFields strips auth secrets (local_users, jwt_secret) from KV overlays so // env/secret-derived values continue to take precedence over stale KV copies. func dropSensitiveAuthFields(data []byte) []byte { if len(data) == 0 { return data } var doc map[string]interface{} if err := json.Unmarshal(data, &doc); err != nil { return data ... (clipped 27 lines)` Solution Walkthrough: Before: `// pkg/config/kv_client.go func dropSensitiveAuthFields(data []byte) []byte { var doc map[string]interface{} if err := json.Unmarshal(data, &doc); err != nil { return data } auth, ok := doc["auth"].(map[string]interface{}) if !ok { return data } // Hardcoded list of keys to remove for _, key := range []string{"local_users", "jwt_secret"} { delete(auth, key) } // ... marshal and return data }` After: // Example using struct tags type AuthConfig struct { LocalUsers map[string]string `json:"local_users" sensitive:"true"` JWTSecret string `json:"jwt_secret" sensitive:"true"` // ... other fields } // Generic function to strip sensitive fields func dropSensitiveFields(data []byte, configStruct interface{}) []byte { // 1. Unmarshal data into a generic map. // 2. Use reflection on `configStruct` to find fields with the `sensitive:"true"` tag. // 3. Recursively traverse the map and remove keys that match the sensitive field names. // 4. Marshal and return the modified data. } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a hardcoded list of sensitive fields and proposes a more scalable design, which significantly improves the maintainability of the new configuration overlay logic.	Medium
More

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2015#issuecomment-3573203600 Original created: 2025-11-24T23:57:27Z --- ## PR Code Suggestions ✨  Explore these optional code suggestions: <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion                                                                                                                                    </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>Security</td> <td> <details><summary>Prevent returning data with secrets</summary> ___ **Modify the <code>dropSensitiveAuthFields</code> function to return <code>nil</code> instead of the <br>original data if JSON marshalling fails, preventing the accidental exposure of <br>sensitive information.** [pkg/config/kv_client.go [525-531]](https://github.com/carverauto/serviceradar/pull/2015/files#diff-0c911990aa32d43404f0f23dadc0363ec97ed4ca315592e6c9bd24510e82e06dR525-R531) ```diff doc["auth"] = auth out, err := json.Marshal(doc) if err != nil { - return data + // Returning nil is safer than returning the original data with secrets. + // The caller will then skip the overlay. + return nil } return out ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: This suggestion correctly identifies a security flaw where the function could return sensitive data on a marshalling error, and the proposed fix of returning `nil` is a robust way to prevent this. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=1>High-level</td> <td> <details><summary>Generalize the secret stripping logic</summary> ___ **The current method of stripping sensitive fields like <code>local_users</code> and <code>jwt_secret</code> <br>is hardcoded. This should be generalized using struct tags or a configuration <br>list to improve scalability and maintainability.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2015/files#diff-0c911990aa32d43404f0f23dadc0363ec97ed4ca315592e6c9bd24510e82e06dR496-R532">pkg/config/kv_client.go [496-532]</a> </summary> ```go // dropSensitiveAuthFields strips auth secrets (local_users, jwt_secret) from KV overlays so // env/secret-derived values continue to take precedence over stale KV copies. func dropSensitiveAuthFields(data []byte) []byte { if len(data) == 0 { return data } var doc map[string]interface{} if err := json.Unmarshal(data, &doc); err != nil { return data ... (clipped 27 lines) ``` </details> ### Solution Walkthrough: #### Before: ```go // pkg/config/kv_client.go func dropSensitiveAuthFields(data []byte) []byte { var doc map[string]interface{} if err := json.Unmarshal(data, &doc); err != nil { return data } auth, ok := doc["auth"].(map[string]interface{}) if !ok { return data } // Hardcoded list of keys to remove for _, key := range []string{"local_users", "jwt_secret"} { delete(auth, key) } // ... marshal and return data } ``` #### After: ```go // Example using struct tags type AuthConfig struct { LocalUsers map[string]string `json:"local_users" sensitive:"true"` JWTSecret string `json:"jwt_secret" sensitive:"true"` // ... other fields } // Generic function to strip sensitive fields func dropSensitiveFields(data []byte, configStruct interface{}) []byte { // 1. Unmarshal data into a generic map. // 2. Use reflection on `configStruct` to find fields with the `sensitive:"true"` tag. // 3. Recursively traverse the map and remove keys that match the sensitive field names. // 4. Marshal and return the modified data. } ``` <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies a hardcoded list of sensitive fields and proposes a more scalable design, which significantly improves the maintainability of the new configuration overlay logic. </details></details></td><td align=center>Medium </td></tr> <tr><td align="center" colspan="2"> - [ ] More  </td><td></td></tr></tbody></table>