Insert host_mdm_managed_certificates rows from non-proxied cert ingestion (PR 5/8)#45037
Conversation
…tion Extend UpdateHostCertificates with two changes that activate auto-renewal for non-proxied SCEP/ACME flows where Fleet wasn't in the issuance path: 1. Loosen the matcher's SupportsRenewalID() guard so empty/NULL Type rows are not silently skipped — without this, ingestion-created rows would never have their not_valid_after advanced after a renewal completes. 2. New INSERT pass: for each profile installed on the host without an existing host_mdm_managed_certificates row, scan the toInsertBySHA1 pool for a cert whose Subject CN/OU contains "fleet-<profile_uuid>" and create the row. Type is left NULL; ca_name is derived from the issuer's CN. A dedicated insertHostMDMManagedCertDB helper handles empty-Type → NULL conversion (the column's enum doesn't accept empty strings) and uses INSERT IGNORE so a concurrent proxy issuance doesn't get clobbered.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis PR extends the Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
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 `@server/datastore/mysql/host_certificates.go`:
- Around line 236-297: The code only attempts to create missing
host_mdm_managed_certificates when toInsertBySHA1 is non-empty and searches that
map for matches, which misses matches present only in incomingBySHA1; change the
condition and loop to use the full incoming set: replace the outer if
len(toInsertBySHA1) > 0 check with len(incomingBySHA1) > 0 (or remove the check
and rely on candidateProfileUUIDs), and when searching for bestMatch inside the
profileUUID loop iterate over incomingBySHA1 (or the variable name that holds
the full incoming certs) instead of toInsertBySHA1; keep existingProfileUUIDs,
candidateProfileUUIDs, hostMDMManagedCertsToInsert, and the matching/validity
logic the same so duplicate inserts are still prevented.
🪄 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: 4d170025-27b7-4c3f-8a4a-1b79d3a2b606
📒 Files selected for processing (2)
server/datastore/mysql/host_certificates.goserver/datastore/mysql/host_certificates_test.go
The previous gate and inner loop only saw certs that were new in the current call. A cert already in host_certificates from a prior call (so present in incomingBySHA1 but not in toInsertBySHA1) would be missed when its matching profile was installed afterward — no INSERT would fire even though a valid marker-bearing cert existed for the profile. Switch the gate and inner-loop pool to incomingBySHA1; the existingProfileUUIDs / candidateProfileUUIDs check still prevents duplicate inserts.
![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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 40639-cert-renew #45037 +/- ##
===================================================
Coverage ? 66.81%
===================================================
Files ? 2698
Lines ? 217601
Branches ? 10142
===================================================
Hits ? 145392
Misses ? 59035
Partials ? 13174
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
sgress454
left a comment
sgress454
left a comment
There was a problem hiding this comment.
couple questions but nothing blocking if you've already thought them through
| if c.Type != "" { | ||
| typeArg = string(c.Type) | ||
| } |
There was a problem hiding this comment.
Isn't the expectation here that this will always be null? so we could just set NULL directly in the INSERT statement, rather than checking cert type at all?
There was a problem hiding this comment.
True, there's no other callers to insertHostMDMManagedCertDB so it simplified things a bit
| // Surface the issuer's CN as ca_name for support visibility; | ||
| // fall back to a sentinel since the column is NOT NULL. | ||
| caName := bestMatch.IssuerCommonName | ||
| if caName == "" { | ||
| caName = "non_proxied" | ||
| } |
There was a problem hiding this comment.
Not sure about this one -- CN can change, right? It looks like we have WHERE clauses that match against ca_name, which could fail if it changes between renewals. 🤖 says that the proxied certs use a fixed string here that won't change; seems like we should consider doing the same here (e.g. just always use non_proxied).
There was a problem hiding this comment.
good catch, switched to a sentinel here
Stop deriving ca_name from the cert's Issuer CN. Use the fixed sentinel "non_proxied" instead, matching the proxied-flows convention of Fleet-controlled ca_name values and removing drift risk if the upstream CA ever renames. The cert's actual issuer remains available in host_certificates for support visibility.
|
👍 , +1 still stands |
2 participants
Related issue: Resolves #44345
What this PR does
Adds an INSERT pass to
UpdateHostCertificatesthat createshost_mdm_managed_certificatesrows when an ingested cert's Subject contains thefleet-<profile_uuid>marker for a profile installed on the host. Rows are created withtype = NULL. Also relaxes the matcher'sSupportsRenewalID()skip so NULL-type rows can be updated on subsequent ingestion.Why this is needed
Phase 2 (#40639) auto-renews certs from non-proxied CAs. The renewal cron acts on
host_mdm_managed_certificatesrows that today only get created at proxy issuance. This PR creates them from cert ingestion instead, so non-proxied flows (e.g. customer-cisneros-a's Hydrant ACME, Okta SCEP) can be renewed by the existing mechanism.Checklist for submitter
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements)Testing