carverauto/serviceradar

Fork 0

Edge onboarding/502 error #2548

Merged

mfreeman451 merged 8 commits from refs/pull/2548/head into main

2025-12-12 04:07:28 +00:00

mfreeman451 commented

2025-12-12 02:53:48 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #2109
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2109
Original created: 2025-12-12T02:53:48Z
Original updated: 2025-12-12T04:07:31Z
Original head: carverauto/serviceradar:edge_onboarding/502_error
Original base: main
Original merged: 2025-12-12T04:07:28Z 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

Fix sysmon bare-metal edge onboarding with mTLS defaults and improved error handling
Add sysmon metrics provider fallback logic to support multiple backend implementations
Enhance sysmon checker packages with optional endpoint metadata for certificate SANs
Improve edge package delivery error classification and logging for actionable diagnostics
Update sysmon default configs from SPIFFE to mTLS for bare-metal installations

Diagram Walkthrough

flowchart LR
  A["Edge Onboarding<br/>Package Creation"] -->|"Default to mTLS<br/>for sysmon"| B["Security Mode<br/>Resolution"]
  B -->|"Include checker<br/>endpoint"| C["mTLS Bundle<br/>Generation"]
  C -->|"Decrypt & deliver"| D["Package Delivery"]
  D -->|"Classify errors"| E["Error Response<br/>with diagnostics"]
  F["Sysmon Metrics<br/>Provider"] -->|"Fallback logic"| G["Resolve from<br/>metricsManager or DB"]
  H["Sysmon Config<br/>Templates"] -->|"Change to mTLS"| I["Bare-metal<br/>Defaults"]

File Walkthrough

Relevant files

Error handling

1 files

edge_onboarding.go
Add decryption error handling and logging +4/-0

Tests

3 files

edge_onboarding_test.go `Test decryption error response classification`	+17/-0
edge_onboarding_test.go `Test sysmon mTLS defaults and decryption error classification`	+72/-6
metrics_test.go `Test sysmon prefixed service name handling`	+56/-0

Enhancement

6 files

sysmon.go `Add metrics provider resolution with fallback logic`	+23/-3
edge_onboarding.go `Default sysmon checker to mTLS and add endpoint metadata`	+38/-9
metrics.go `Support sysmon service name prefixes in gRPC processing`	+4/-6
edge_onboarding.go `Add decrypt failed error type`	+1/-0
server.rs `Implement GetResults and StreamResults gRPC methods`	+90/-1
bundle.rs `Add certificate ownership management for bare-metal`	+45/-0

Configuration changes

4 files

build.rs `Update proto path to use shared proto directory`	+6/-2
BUILD.bazel `Update proto dependency to shared location`	+1/-1
default_template.json `Change default security mode from SPIFFE to mTLS`	+4/-6
sysmon.json.example `Change example config from SPIFFE to mTLS defaults`	+4/-6

Formatting

6 files

main.rs `Improve mTLS token environment variable handling`	+3/-1
config.rs `Format checker config generation code`	+3/-3
download.rs `Format package download response parsing`	+3/-1
lib.rs `Format mTLS bootstrap bundle extraction`	+1/-4
token.rs `Format host override URL parsing logic`	+6/-9
token_tests.rs `Format token parsing test assertions`	+21/-7

Documentation

4 files

proposal.md `Document sysmon bare-metal mTLS and onboarding changes`	+27/-0
spec.md `Add edge onboarding requirements for sysmon mTLS`	+31/-0
spec.md `Add sysmon bare-metal mTLS default requirements`	+24/-0
tasks.md `Track implementation tasks for sysmon mTLS defaults`	+18/-0

Dependencies

1 files

Cargo.toml
Add libc dependency for certificate ownership +1/-0

Imported from GitHub pull request. Original GitHub pull request: #2109 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2109 Original created: 2025-12-12T02:53:48Z Original updated: 2025-12-12T04:07:31Z Original head: carverauto/serviceradar:edge_onboarding/502_error Original base: main Original merged: 2025-12-12T04:07:28Z 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** - Fix sysmon bare-metal edge onboarding with mTLS defaults and improved error handling - Add sysmon metrics provider fallback logic to support multiple backend implementations - Enhance sysmon checker packages with optional endpoint metadata for certificate SANs - Improve edge package delivery error classification and logging for actionable diagnostics - Update sysmon default configs from SPIFFE to mTLS for bare-metal installations ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Edge Onboarding<br/>Package Creation"] -->|"Default to mTLS<br/>for sysmon"| B["Security Mode<br/>Resolution"] B -->|"Include checker<br/>endpoint"| C["mTLS Bundle<br/>Generation"] C -->|"Decrypt & deliver"| D["Package Delivery"] D -->|"Classify errors"| E["Error Response<br/>with diagnostics"] F["Sysmon Metrics<br/>Provider"] -->|"Fallback logic"| G["Resolve from<br/>metricsManager or DB"] H["Sysmon Config<br/>Templates"] -->|"Change to mTLS"| I["Bare-metal<br/>Defaults"] ``` <details><summary><h3>File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Error handling</strong></td><td><details><summary>1 files</summary><table> <tr> <td><strong>edge_onboarding.go</strong><dd><code>Add decryption error handling and logging</code>                                </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2109/files#diff-c494568149a0b526ca5349c1d78d5e5ee7ec4144ecfd324c44f25fef584292bc">+4/-0</a>      </td> </tr> </table></details></td></tr><tr><td><strong>Tests</strong></td><td><details><summary>3 files</summary><table> <tr> <td><strong>edge_onboarding_test.go</strong><dd><code>Test decryption error response classification</code>                        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2109/files#diff-ff98b520d3df871ff219eb56ab1a2ac2d55afb266b87393bd63360635ba02880">+17/-0</a>    </td> </tr> <tr> <td><strong>edge_onboarding_test.go</strong><dd><code>Test sysmon mTLS defaults and decryption error classification</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2109/files#diff-83ba269452783436d693c0f17ab42c959efdef44c7cf6ec27301dd1b0e7d5744">+72/-6</a>    </td> </tr> <tr> <td><strong>metrics_test.go</strong><dd><code>Test sysmon prefixed service name handling</code>                              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2109/files#diff-2b0ce06b068be7b4418c3fe4d23e3b8bf536d0cc2b9201a6949976e298a9e95e">+56/-0</a>    </td> </tr> </table></details></td></tr><tr><td><strong>Enhancement</strong></td><td><details><summary>6 files</summary><table> <tr> <td><strong>sysmon.go</strong><dd><code>Add metrics provider resolution with fallback logic</code>            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2109/files#diff-4879e8b367ee55413c6cb006f23bb912261df5310c640d3af352033cf97ffc84">+23/-3</a>    </td> </tr> <tr> <td><strong>edge_onboarding.go</strong><dd><code>Default sysmon checker to mTLS and add endpoint metadata</code>  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2109/files#diff-85874e3c4bdcc9110db09909f10648d44cdee554b26c987f910502321eb20b5c">+38/-9</a>    </td> </tr> <tr> <td><strong>metrics.go</strong><dd><code>Support sysmon service name prefixes in gRPC processing</code>    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2109/files#diff-6d98e853ce17576c088e77956ae4ecfa8078019e0bff107a79d8d1d6ed2443ad">+4/-6</a>      </td> </tr> <tr> <td><strong>edge_onboarding.go</strong><dd><code>Add decrypt failed error type</code>                                                        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2109/files#diff-0c66da94363b25ecb9e4766c8f12cde387c1874aeb9099447c609c4e867b4fe0">+1/-0</a>      </td> </tr> <tr> <td><strong>server.rs</strong><dd><code>Implement GetResults and StreamResults gRPC methods</code>            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2109/files#diff-2c4395fee16396339c3eea518ad9bec739174c67c9cedf62e6848c17136dd33e">+90/-1</a>    </td> </tr> <tr> <td><strong>bundle.rs</strong><dd><code>Add certificate ownership management for bare-metal</code>            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2109/files#diff-fc09c2867b39b36b1463f053656aaad70a259979ef64a00fcb4130a437d4ef3d">+45/-0</a>    </td> </tr> </table></details></td></tr><tr><td><strong>Configuration changes</strong></td><td><details><summary>4 files</summary><table> <tr> <td><strong>build.rs</strong><dd><code>Update proto path to use shared proto directory</code>                    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2109/files#diff-2e519950c72a7e0bd53e22628bcd1a5dc31f6a6a0070b4c47221b1948a3b2099">+6/-2</a>      </td> </tr> <tr> <td><strong>BUILD.bazel</strong><dd><code>Update proto dependency to shared location</code>                              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2109/files#diff-6ab6e69dfb3cd621d100077fa496690634adb5fcd88806f891575024f1835480">+1/-1</a>      </td> </tr> <tr> <td><strong>default_template.json</strong><dd><code>Change default security mode from SPIFFE to mTLS</code>                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2109/files#diff-13a2206be25eb545a343eebfa161a2d5e78148a9c1308ac729b6b3452e1cbe8e">+4/-6</a>      </td> </tr> <tr> <td><strong>sysmon.json.example</strong><dd><code>Change example config from SPIFFE to mTLS defaults</code>              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2109/files#diff-67eb08bf964d99c4805359fe64cab2daa1d801afdf54b4952df5ec368b9d1322">+4/-6</a>      </td> </tr> </table></details></td></tr><tr><td><strong>Formatting</strong></td><td><details><summary>6 files</summary><table> <tr> <td><strong>main.rs</strong><dd><code>Improve mTLS token environment variable handling</code>                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2109/files#diff-cef134403622089f759cfa006eb04dc8f1cfda08497a9d38586cc4de03d14820">+3/-1</a>      </td> </tr> <tr> <td><strong>config.rs</strong><dd><code>Format checker config generation code</code>                                        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2109/files#diff-27bc48513a0a6f736d1b6efe62b5eb7dcfccc82e70f4cb9678d54ee1efe9455d">+3/-3</a>      </td> </tr> <tr> <td><strong>download.rs</strong><dd><code>Format package download response parsing</code>                                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2109/files#diff-9e5dc37d31894894a78d7eb118c72f7303a8be99adb992ac51de9e664fdee2f2">+3/-1</a>      </td> </tr> <tr> <td><strong>lib.rs</strong><dd><code>Format mTLS bootstrap bundle extraction</code>                                    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2109/files#diff-5d04060c8999c31f6f944993ba5c30a1d7cc1882441105b20e6aae0aa0c2eb52">+1/-4</a>      </td> </tr> <tr> <td><strong>token.rs</strong><dd><code>Format host override URL parsing logic</code>                                      </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2109/files#diff-522ddb82ad9919bd416394e34bbfd4ce3e8793723b260a08843d7500ae0b98cd">+6/-9</a>      </td> </tr> <tr> <td><strong>token_tests.rs</strong><dd><code>Format token parsing test assertions</code>                                          </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2109/files#diff-5c3192711c7e59713634180745b00fa6ac48caff6d9fbe5392b357362ace1761">+21/-7</a>    </td> </tr> </table></details></td></tr><tr><td><strong>Documentation</strong></td><td><details><summary>4 files</summary><table> <tr> <td><strong>proposal.md</strong><dd><code>Document sysmon bare-metal mTLS and onboarding changes</code>      </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2109/files#diff-60a55fc01a540b121930f2f101856a199ab8f4da0a8b769de7e7ee7fc919e73d">+27/-0</a>    </td> </tr> <tr> <td><strong>spec.md</strong><dd><code>Add edge onboarding requirements for sysmon mTLS</code>                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2109/files#diff-f8816226222dc2a96910e7041c3bbdc1eded92241099cdee2f4e5e6f018b4b60">+31/-0</a>    </td> </tr> <tr> <td><strong>spec.md</strong><dd><code>Add sysmon bare-metal mTLS default requirements</code>                    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2109/files#diff-c0abe980d21089c9f160818c9604d0d5fc7389f2d2d64c65ce19b1c0dc6154f7">+24/-0</a>    </td> </tr> <tr> <td><strong>tasks.md</strong><dd><code>Track implementation tasks for sysmon mTLS defaults</code>            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2109/files#diff-2c26d63c5f429a88b310d196eee58d985559d48fa568c7ac23fa60df456048d5">+18/-0</a>    </td> </tr> </table></details></td></tr><tr><td><strong>Dependencies</strong></td><td><details><summary>1 files</summary><table> <tr> <td><strong>Cargo.toml</strong><dd><code>Add libc dependency for certificate ownership</code>                        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2109/files#diff-a642d44a71cf5973a88eab24508c6e2de618ffaf8afd0d6124013ed7e7f70ca7">+1/-0</a>      </td> </tr> </table></details></td></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-12-12 02:54:22 +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/2109#issuecomment-3644685675
Original created: 2025-12-12T02:54:22Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
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: Audit logging: New error branches add some logging but critical actions like package delivery success/failure and decryption errors may not consistently include user/context identifiers for full auditability across all added paths. Referred Code `case errors.Is(err, models.ErrEdgeOnboardingDecryptFailed): s.logger.Error().Err(err).Str("package_id", id).Msg("edge onboarding: package decryption failed") writeError(w, "package decryption failed; please reissue token", http.StatusInternalServerError) default: s.logger.Error().Err(err).Str("package_id", id).Msg("edge onboarding: deliver package failed") writeError(w, "failed to deliver edge package", http.StatusBadGateway) }` Learn more about managing compliance generic rules or creating your own custom rules
Update

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/2109#issuecomment-3644685675 Original created: 2025-12-12T02:54:22Z --- ## 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>🟢</td><td><details><summary><strong>No security concerns identified</strong></summary> No security vulnerabilities detected by AI analysis. Human verification advised for critical code. </details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary>🎫 <strong>No ticket provided </strong></summary> - [ ] Create ticket/issue  </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/2109/files#diff-c494568149a0b526ca5349c1d78d5e5ee7ec4144ecfd324c44f25fef584292bcR520-R526'><strong>Audit logging</strong></a>: New error branches add some logging but critical actions like package delivery <br>success/failure and decryption errors may not consistently include user/context <br>identifiers for full auditability across all added paths.<br> <details open><summary>Referred Code</summary> ```go case errors.Is(err, models.ErrEdgeOnboardingDecryptFailed): s.logger.Error().Err(err).Str("package_id", id).Msg("edge onboarding: package decryption failed") writeError(w, "package decryption failed; please reissue token", http.StatusInternalServerError) default: s.logger.Error().Err(err).Str("package_id", id).Msg("edge onboarding: deliver package failed") writeError(w, "failed to deliver edge package", http.StatusBadGateway) } ``` </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"> - [ ] Update  </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-12-12 02:55:41 +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/2109#issuecomment-3644688295
Original created: 2025-12-12T02:55:41Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Security	Replace unsafe FFI with safe alternative Replace the unsafe FFI call to `getpwnam` with a thread-safe alternative from the `users` crate to avoid potential race conditions. rust/edge-onboarding/src/bundle.rs [29-64] #[cfg(unix)] fn maybe_chown_cert_files(paths: &[PathBuf]) -> Result<()> { - use std::ffi::CString; - use std::os::unix::ffi::OsStrExt; + use std::os::unix::fs::chown; // Only relevant when bootstrap is run as root on bare metal. if unsafe { libc::geteuid() } != 0 { return Ok(()); } - let user = CString::new("serviceradar").expect("static string"); - let pw = unsafe { libc::getpwnam(user.as_ptr()) }; - if pw.is_null() { - tracing::warn!("service user serviceradar not found; leaving cert ownership unchanged"); - return Ok(()); - } + let user = match users::get_user_by_name("serviceradar") { + Some(u) => u, + None => { + tracing::warn!("service user serviceradar not found; leaving cert ownership unchanged"); + return Ok(()); + } + }; - let uid = unsafe { (pw).pw_uid }; - let gid = unsafe { (pw).pw_gid }; + let uid = user.uid(); + let gid = user.primary_group_id(); for path in paths { - let c_path = CString::new(path.as_os_str().as_bytes()).map_err(\|e\| Error::Io { + chown(path, Some(uid), Some(gid)).map_err(\|e\| Error::Io { path: path.display().to_string(), - source: std::io::Error::new(std::io::ErrorKind::InvalidInput, e), + source: e, })?; - let rc = unsafe { libc::chown(c_path.as_ptr(), uid, gid) }; - if rc != 0 { - return Err(Error::Io { - path: path.display().to_string(), - source: std::io::Error::last_os_error(), - }); - } } Ok(()) } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential thread-safety issue with `getpwnam` and proposes a safer, idiomatic Rust alternative using the `users` crate, which improves code robustness.	Medium
High-level	Consolidate sysmon service name handling The logic for identifying sysmon services is inconsistent between `pkg/core/metrics.go` and `pkg/core/edge_onboarding.go`. This logic should be centralized into a shared helper function to ensure consistent behavior and improve maintainability. Examples: pkg/core/metrics.go [1421-1422] `case svc.ServiceName == sysmonServiceType \|\| svc.ServiceName == "sysmon-osx" \|\| strings.HasPrefix(svc.ServiceName, sysmonServiceType+"-"): return s.processSysmonMetrics(ctx, pollerID, partition, agentID, serviceData, now)` pkg/core/edge_onboarding.go [2809-2817] `if normalizedKind == "sysmon" \|\| normalizedKind == "sysmon-osx" { filtered := agent.Checks[:0] for _, check := range agent.Checks { if strings.EqualFold(check.Type, pkg.CheckerKind) { continue } filtered = append(filtered, check) } agent.Checks = filtered` Solution Walkthrough: Before: // In pkg/core/metrics.go func (s Server) processGRPCService(...) { switch { // ... case svc.ServiceName == "sysmon" \|\| svc.ServiceName == "sysmon-osx" \|\| strings.HasPrefix(svc.ServiceName, "sysmon-"): return s.processSysmonMetrics(...) // ... } return nil } // In pkg/core/edge_onboarding.go func (s edgeOnboardingService) addCheckerToPollerConfig(...) { normalizedKind := strings.ToLower(strings.TrimSpace(pkg.CheckerKind)) // Clean up legacy sysmon entries if normalizedKind == "sysmon" \|\| normalizedKind == "sysmon-osx" { // ... filter logic ... } // ... } After: // In a shared package, e.g., pkg/models func IsSysmonService(serviceName string) bool { return serviceName == "sysmon" \|\| strings.HasPrefix(serviceName, "sysmon-") } // In pkg/core/metrics.go func (s Server) processGRPCService(...) { switch { // ... case models.IsSysmonService(svc.ServiceName): return s.processSysmonMetrics(...) // ... } return nil } // In pkg/core/edge_onboarding.go func (s edgeOnboardingService) addCheckerToPollerConfig(...) { normalizedKind := strings.ToLower(strings.TrimSpace(pkg.CheckerKind)) if models.IsSysmonService(normalizedKind) { // ... filter logic ... } // ... } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies an inconsistency in how sysmon-like services are identified between `pkg/core/metrics.go` and `pkg/core/edge_onboarding.go`, which could lead to future bugs and maintenance issues.	Low
Update

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2109#issuecomment-3644688295 Original created: 2025-12-12T02:55:41Z --- ## 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>Replace unsafe FFI with safe alternative</summary> ___ **Replace the unsafe FFI call to <code>getpwnam</code> with a thread-safe alternative from the <br><code>users</code> crate to avoid potential race conditions.** [rust/edge-onboarding/src/bundle.rs [29-64]](https://github.com/carverauto/serviceradar/pull/2109/files#diff-fc09c2867b39b36b1463f053656aaad70a259979ef64a00fcb4130a437d4ef3dR29-R64) ```diff #[cfg(unix)] fn maybe_chown_cert_files(paths: &[PathBuf]) -> Result<()> { - use std::ffi::CString; - use std::os::unix::ffi::OsStrExt; + use std::os::unix::fs::chown; // Only relevant when bootstrap is run as root on bare metal. if unsafe { libc::geteuid() } != 0 { return Ok(()); } - let user = CString::new("serviceradar").expect("static string"); - let pw = unsafe { libc::getpwnam(user.as_ptr()) }; - if pw.is_null() { - tracing::warn!("service user serviceradar not found; leaving cert ownership unchanged"); - return Ok(()); - } + let user = match users::get_user_by_name("serviceradar") { + Some(u) => u, + None => { + tracing::warn!("service user serviceradar not found; leaving cert ownership unchanged"); + return Ok(()); + } + }; - let uid = unsafe { (*pw).pw_uid }; - let gid = unsafe { (*pw).pw_gid }; + let uid = user.uid(); + let gid = user.primary_group_id(); for path in paths { - let c_path = CString::new(path.as_os_str().as_bytes()).map_err(|e| Error::Io { + chown(path, Some(uid), Some(gid)).map_err(|e| Error::Io { path: path.display().to_string(), - source: std::io::Error::new(std::io::ErrorKind::InvalidInput, e), + source: e, })?; - let rc = unsafe { libc::chown(c_path.as_ptr(), uid, gid) }; - if rc != 0 { - return Err(Error::Io { - path: path.display().to_string(), - source: std::io::Error::last_os_error(), - }); - } } Ok(()) } ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies a potential thread-safety issue with `getpwnam` and proposes a safer, idiomatic Rust alternative using the `users` crate, which improves code robustness. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=1>High-level</td> <td> <details><summary>Consolidate sysmon service name handling</summary> ___ **The logic for identifying sysmon services is inconsistent between <br><code>pkg/core/metrics.go</code> and <code>pkg/core/edge_onboarding.go</code>. This logic should be <br>centralized into a shared helper function to ensure consistent behavior and <br>improve maintainability.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2109/files#diff-6d98e853ce17576c088e77956ae4ecfa8078019e0bff107a79d8d1d6ed2443adR1421-R1422">pkg/core/metrics.go [1421-1422]</a> </summary> ```go case svc.ServiceName == sysmonServiceType || svc.ServiceName == "sysmon-osx" || strings.HasPrefix(svc.ServiceName, sysmonServiceType+"-"): return s.processSysmonMetrics(ctx, pollerID, partition, agentID, serviceData, now) ``` </details> <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2109/files#diff-85874e3c4bdcc9110db09909f10648d44cdee554b26c987f910502321eb20b5cR2809-R2817">pkg/core/edge_onboarding.go [2809-2817]</a> </summary> ```go if normalizedKind == "sysmon" || normalizedKind == "sysmon-osx" { filtered := agent.Checks[:0] for _, check := range agent.Checks { if strings.EqualFold(check.Type, pkg.CheckerKind) { continue } filtered = append(filtered, check) } agent.Checks = filtered ``` </details> ### Solution Walkthrough: #### Before: ```go // In pkg/core/metrics.go func (s *Server) processGRPCService(...) { switch { // ... case svc.ServiceName == "sysmon" || svc.ServiceName == "sysmon-osx" || strings.HasPrefix(svc.ServiceName, "sysmon-"): return s.processSysmonMetrics(...) // ... } return nil } // In pkg/core/edge_onboarding.go func (s *edgeOnboardingService) addCheckerToPollerConfig(...) { normalizedKind := strings.ToLower(strings.TrimSpace(pkg.CheckerKind)) // Clean up legacy sysmon entries if normalizedKind == "sysmon" || normalizedKind == "sysmon-osx" { // ... filter logic ... } // ... } ``` #### After: ```go // In a shared package, e.g., pkg/models func IsSysmonService(serviceName string) bool { return serviceName == "sysmon" || strings.HasPrefix(serviceName, "sysmon-") } // In pkg/core/metrics.go func (s *Server) processGRPCService(...) { switch { // ... case models.IsSysmonService(svc.ServiceName): return s.processSysmonMetrics(...) // ... } return nil } // In pkg/core/edge_onboarding.go func (s *edgeOnboardingService) addCheckerToPollerConfig(...) { normalizedKind := strings.ToLower(strings.TrimSpace(pkg.CheckerKind)) if models.IsSysmonService(normalizedKind) { // ... filter logic ... } // ... } ``` <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion correctly identifies an inconsistency in how sysmon-like services are identified between `pkg/core/metrics.go` and `pkg/core/edge_onboarding.go`, which could lead to future bugs and maintenance issues. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td><td></td></tr></tbody></table>