Updated osquery-perf for certs#48499
Open
getvictor wants to merge 20 commits into
Open
Conversation
Surface the existing "Certificates" card on the host details page for Windows hosts, with parity to macOS. Requires osquery 5.23.1 or higher. Backend (server/service/osquery_utils/queries.go): - certificates_windows query now selects sid, store_location, subject2, issuer2, gated by a Discovery query on the subject2 column so older osquery agents (< 5.23.1, which lack the column) collect nothing instead of erroring. - Scope is derived from the registry hive SID (S-1-5-21-* => User, else System with no owner) rather than the username == "SYSTEM" heuristic, which mislabeled machine-wide LocalMachine certs. Dedup by (SHA1, scope, username) collapses redundant hive views. DN parsing (server/fleet/host_certificates.go): - Replace the parseWindowsDN stub with a real X.500 (CERT_X500_NAME_STR) parser for subject2/issuer2: comma-separated key=value RDNs, quoted values, multi-valued (+) RDNs. Shared CN/O/OU/C mapping with the macOS parser. Populates the Issuer column and details modal for Windows. Reconciliation (server/datastore/mysql/host_certificates.go): - UpdateHostCertificates takes an observedScopes argument. nil (macOS/MDM) reconciles every scope, unchanged. The Windows path passes the scopes it observed so a logged-off user's certificates (hive not loaded) are preserved rather than soft-deleted, while System is always observed. Frontend: - Un-gate the Certificates card for Windows hosts, rename the "Keychain" column to "Scope", and make the table help text platform-aware. Claude-Session: https://claude.ai/code/session_01H3HZ8eEZcVZoBU9rjy6ci8
…host-certificates
Member
Author
|
@coderabbitai full review |
Member
Author
|
/agentic_review |
Contributor
✅ Action performedFull review finished. |
Contributor
WalkthroughThe change refactors certificate simulation in 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@cmd/osquery-perf/certificates.go`:
- Around line 353-360: The simulator in certificates.go is emitting Windows-only
columns that the production host-detail query does not currently request, so
align the simulated result shape with the query in queries.go. Update the
Windows certificate payload built in the certificate generation path so the
fields exercised by osquery-perf match the production Windows certificate
columns selected by the host-detail query, using the existing helpers like
windowsDN and windowsLegacyDN where appropriate.
🪄 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: CHILL
Plan: Pro
Run ID: b5a0b576-399f-465a-93cd-d0add9e6cef1
📒 Files selected for processing (2)
cmd/osquery-perf/agent.gocmd/osquery-perf/certificates.go
Code Review by Qodo
1. Windows cert scope misread
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Updates osquery-perf’s simulated certificates detail-query output to better reflect cross-platform certificate ingestion needs (notably Windows), replacing the prior inlined generators in agent.go with a dedicated certificate generator module.
Changes:
- Added a new certificate simulation/generation module with shared + per-host cert specs and platform-specific renderers (macOS + Windows).
- Replaced the old per-platform certificate generators and cache in
agent.gowith a per-host spec cache (hostCertSpecs) guarded by a mutex.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/osquery-perf/certificates.go | New certificate spec generator and macOS/Windows row renderers for the certificates_* detail queries. |
| cmd/osquery-perf/agent.go | Removes old certificate cache/generators and adds per-host cert spec storage on the agent. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #48499 +/- ##
==========================================
- Coverage 68.02% 68.02% -0.01%
==========================================
Files 3679 3680 +1
Lines 233804 233868 +64
Branches 12453 12302 -151
==========================================
+ Hits 159052 159079 +27
- Misses 60446 60479 +33
- Partials 14306 14310 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
CI Feedback 🧐(Feedback updated until commit 5a17bb8)A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
…host-certificates # Conflicts: # server/datastore/mysql/schema.sql
…94-osquery-perf-windows-certs
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/osquery-perf/certificates.go (1)
160-225: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCross-scope duplicate can silently desync during churn.
newPerHostCertSpecs()intentionally duplicatesspecs[0]into a second scope (same serial/SHA1, machine + user) to model the cross-scope dedup case the server needs to handle. ButchurnPerHostCertSpecs()picks a random index each rotation and mutates its serial independently, with no awareness that index 0 and the last-appended duplicate are supposed to stay in sync. Once churn hits only one side of the pair, the two entries diverge (different SHA1s), and the "one host_certificates row, two host_certificate_sources rows" scenario this duplication was built to exercise quietly stops being modeled for that host going forward.Since
certChurnPercentis rolled on every detail refresh, this drift can happen fairly quickly in a long-running load test.💡 Possible mitigation
Track which index holds the cross-scope duplicate (e.g., a field on
agentor a sentinel insimulatedCert) and keep it excluded from independent random selection inchurnPerHostCertSpecs, always refreshing it in lockstep with its paired base cert.func (a *agent) churnPerHostCertSpecs() { if len(a.hostCertSpecs) == 0 { return } n := rand.Intn(min(10, len(a.hostCertSpecs))) + 1 for range n { idx := rand.Intn(len(a.hostCertSpecs)) a.hostCertSpecs[idx].serial = uuid.NewString() a.hostCertSpecs[idx].commonName = uuid.NewString() a.hostCertSpecs[idx].notValidAfterUnix = fmt.Sprint(time.Now().Add(-1 * certDay).Add(time.Duration(rand.Intn(100)) * certDay).Unix()) + // if idx is part of a cross-scope pair, mirror the update onto the paired index + // so both entries retain the same serial/sha1 across rotations. } }🤖 Prompt for 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. In `@cmd/osquery-perf/certificates.go` around lines 160 - 225, The cross-scope duplicated cert created in newPerHostCertSpecs can drift when churnPerHostCertSpecs mutates only one copy, breaking the intended shared-SHA1 machine/user model. Update the agent/cert bookkeeping so the paired duplicate (the extra cert appended in newPerHostCertSpecs) is identifiable, then make churnPerHostCertSpecs either exclude that duplicate from random independent churn or always update it in lockstep with its source cert. Use the existing newPerHostCertSpecs, newPerHostCertSpec, and churnPerHostCertSpecs symbols to keep the duplicate and its paired scope synchronized.
🤖 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.
Nitpick comments:
In `@cmd/osquery-perf/certificates.go`:
- Around line 160-225: The cross-scope duplicated cert created in
newPerHostCertSpecs can drift when churnPerHostCertSpecs mutates only one copy,
breaking the intended shared-SHA1 machine/user model. Update the agent/cert
bookkeeping so the paired duplicate (the extra cert appended in
newPerHostCertSpecs) is identifiable, then make churnPerHostCertSpecs either
exclude that duplicate from random independent churn or always update it in
lockstep with its source cert. Use the existing newPerHostCertSpecs,
newPerHostCertSpec, and churnPerHostCertSpecs symbols to keep the duplicate and
its paired scope synchronized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f5aff7d9-48a2-4f68-afaf-9fa30f4b8871
📒 Files selected for processing (2)
cmd/osquery-perf/agent.gocmd/osquery-perf/certificates.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/osquery-perf/agent.go
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
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.
Related issue: Resolves #31294
osquery-perf changes only
Checklist for submitter
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes