Host identity cert renewal - #31372
Conversation
This reverts commit 7b340b4.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis update introduces automated certificate renewal with proof-of-possession for host identity certificates. It adds new constants, helper functions, and middleware to handle renewal logic, key backup, and extension handling. The SCEP service and client are updated to support renewal extensions, and integration tests are expanded to cover renewal and host deletion scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Host as Host
participant SCEPClient as SCEP Client
participant SCEPServer as SCEP Server
participant Datastore as Datastore
Host->>SCEPClient: Detects certificate near expiry
SCEPClient->>Host: Backup old key, generate new key
Host->>SCEPClient: Create CSR with renewal extension (serial, signature)
SCEPClient->>SCEPServer: Submit CSR for renewal
SCEPServer->>SCEPServer: Detect renewal extension, verify signature
SCEPServer->>Datastore: Lookup old certificate by serial
SCEPServer->>SCEPServer: Validate proof-of-possession
SCEPServer-->>SCEPClient: Issue renewed certificate
SCEPClient->>Host: Save new certificate, remove old key backup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #31372 +/- ##
==========================================
+ Coverage 63.71% 64.17% +0.46%
==========================================
Files 1954 1914 -40
Lines 189981 188858 -1123
Branches 6065 5623 -442
==========================================
+ Hits 121040 121194 +154
+ Misses 59452 58145 -1307
- Partials 9489 9519 +30
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:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
ee/orbit/pkg/hostidentity/host_identity.go(6 hunks)ee/orbit/pkg/scep/scep.go(3 hunks)ee/orbit/pkg/securehw/securehw_linux.go(2 hunks)ee/orbit/pkg/securehw/securehw_tpm.go(7 hunks)ee/server/integrationtest/hostidentity/hostidentity_test.go(8 hunks)ee/server/service/hostidentity/scep.go(7 hunks)ee/server/service/hostidentity/types/host_identity_certificates.go(1 hunks)orbit/pkg/constant/constant.go(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: ee/server/service/hostidentity/depot/depot.go:104-119
Timestamp: 2025-07-08T16:06:54.576Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the security concern where shared challenges allow certificate revocation (lines 104-119) is a known issue that will be addressed in a later feature, not an immediate concern to fix.
orbit/pkg/constant/constant.go (1)
Learnt from: getvictor
PR: #30589
File: ee/server/service/hostidentity/depot/depot.go:104-119
Timestamp: 2025-07-08T16:06:54.576Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the security concern where shared challenges allow certificate revocation (lines 104-119) is a known issue that will be addressed in a later feature, not an immediate concern to fix.
ee/orbit/pkg/securehw/securehw_linux.go (2)
Learnt from: getvictor
PR: #30589
File: ee/server/service/hostidentity/depot/depot.go:104-119
Timestamp: 2025-07-08T16:06:54.576Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the security concern where shared challenges allow certificate revocation (lines 104-119) is a known issue that will be addressed in a later feature, not an immediate concern to fix.
Learnt from: getvictor
PR: #30825
File: ee/server/service/hostidentity/httpsig/httpsig.go:86-88
Timestamp: 2025-07-15T07:26:38.930Z
Learning: In ee/server/service/hostidentity/httpsig/httpsig.go, the Fetch method stub returning "not implemented" is safe because the verification profile requires MetaKeyID in RequiredMetadata, ensuring the httpsig library always calls FetchByKeyID instead of Fetch for valid signatures.
ee/server/service/hostidentity/types/host_identity_certificates.go (1)
Learnt from: getvictor
PR: #30589
File: ee/server/service/hostidentity/depot/depot.go:104-119
Timestamp: 2025-07-08T16:06:54.576Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the security concern where shared challenges allow certificate revocation (lines 104-119) is a known issue that will be addressed in a later feature, not an immediate concern to fix.
ee/server/service/hostidentity/scep.go (4)
Learnt from: getvictor
PR: #30589
File: ee/server/service/hostidentity/depot/depot.go:104-119
Timestamp: 2025-07-08T16:06:54.576Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the security concern where shared challenges allow certificate revocation (lines 104-119) is a known issue that will be addressed in a later feature, not an immediate concern to fix.
Learnt from: getvictor
PR: #30589
File: ee/server/service/hostidentity/depot/depot.go:108-111
Timestamp: 2025-07-08T16:12:48.797Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the SCEP depot interface methods like Put() do not accept context parameters, and the common_mysql.WithRetryTxx callback function type TxFn only receives a transaction parameter, not a context. Therefore, using context.Background() in tx.ExecContext calls within the transaction callback is the correct approach.
Learnt from: getvictor
PR: #30825
File: ee/server/service/hostidentity/httpsig/httpsig.go:86-88
Timestamp: 2025-07-15T07:26:38.930Z
Learning: In ee/server/service/hostidentity/httpsig/httpsig.go, the Fetch method stub returning "not implemented" is safe because the verification profile requires MetaKeyID in RequiredMetadata, ensuring the httpsig library always calls FetchByKeyID instead of Fetch for valid signatures.
Learnt from: getvictor
PR: #30589
File: ee/server/service/hostidentity/depot/depot.go:115-115
Timestamp: 2025-07-08T16:11:49.555Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the error from result.RowsAffected() is intentionally ignored because the information is only used for logging purposes, not for critical program logic.
ee/orbit/pkg/hostidentity/host_identity.go (2)
Learnt from: getvictor
PR: #30589
File: ee/server/service/hostidentity/depot/depot.go:104-119
Timestamp: 2025-07-08T16:06:54.576Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the security concern where shared challenges allow certificate revocation (lines 104-119) is a known issue that will be addressed in a later feature, not an immediate concern to fix.
Learnt from: getvictor
PR: #30589
File: ee/server/service/hostidentity/depot/depot.go:108-111
Timestamp: 2025-07-08T16:12:48.797Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the SCEP depot interface methods like Put() do not accept context parameters, and the common_mysql.WithRetryTxx callback function type TxFn only receives a transaction parameter, not a context. Therefore, using context.Background() in tx.ExecContext calls within the transaction callback is the correct approach.
ee/server/integrationtest/hostidentity/hostidentity_test.go (4)
Learnt from: getvictor
PR: #30589
File: ee/server/service/hostidentity/depot/depot.go:104-119
Timestamp: 2025-07-08T16:06:54.576Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the security concern where shared challenges allow certificate revocation (lines 104-119) is a known issue that will be addressed in a later feature, not an immediate concern to fix.
Learnt from: getvictor
PR: #30589
File: server/fleet/datastore.go:13-14
Timestamp: 2025-07-08T16:12:48.182Z
Learning: The Fleet team is not currently testing or enforcing OSS build compatibility, so imports of enterprise-only packages (like ee/server/service/hostidentity/types) into OSS code are acceptable for now.
Learnt from: getvictor
PR: #30589
File: ee/server/service/hostidentity/depot/depot.go:108-111
Timestamp: 2025-07-08T16:12:48.797Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the SCEP depot interface methods like Put() do not accept context parameters, and the common_mysql.WithRetryTxx callback function type TxFn only receives a transaction parameter, not a context. Therefore, using context.Background() in tx.ExecContext calls within the transaction callback is the correct approach.
Learnt from: getvictor
PR: #30825
File: ee/server/service/hostidentity/httpsig/httpsig.go:86-88
Timestamp: 2025-07-15T07:26:38.930Z
Learning: In ee/server/service/hostidentity/httpsig/httpsig.go, the Fetch method stub returning "not implemented" is safe because the verification profile requires MetaKeyID in RequiredMetadata, ensuring the httpsig library always calls FetchByKeyID instead of Fetch for valid signatures.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (27)
- GitHub Check: set-enroll-secret
- GitHub Check: run-server (mysql:8.0.36)
- GitHub Check: publish
- GitHub Check: test-go (vuln, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (service, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (vuln, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (fleetctl, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (mysql, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (integration-core, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (main, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (service, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (main, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (fleetctl, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (mysql, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (integration-enterprise, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (integration-mdm, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (integration-enterprise, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (integration-mdm, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (integration-core, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: build-macos-targets
- GitHub Check: test-packaging (ubuntu-latest)
- GitHub Check: test-packaging (macos-15)
- GitHub Check: lint (macos-latest)
- GitHub Check: lint (windows-latest)
- GitHub Check: lint (ubuntu-latest)
- GitHub Check: build
- GitHub Check: test-packaging (ubuntu-latest, local)
🔇 Additional comments (32)
orbit/pkg/constant/constant.go (1)
80-83: Well-structured constant additions.These constants centralize TPM key filename management and support the certificate renewal workflow. The backup filename clearly indicates its purpose during key rotation.
ee/orbit/pkg/securehw/securehw_linux.go (2)
10-10: Good practice using centralized constants.Adding the constant import supports better maintainability.
41-41: Proper use of constant instead of hardcoded string.Replacing the hardcoded filename with the constant improves maintainability and consistency across the codebase.
ee/orbit/pkg/scep/scep.go (3)
46-47: Good addition for extensibility.The extraExtensions field enables custom extensions in certificate signing requests, supporting the renewal workflow with proper encapsulation.
110-115: Consistent functional option pattern.The WithExtraExtensions option follows the established pattern in this codebase and provides a clean API for adding custom extensions.
192-192: Proper integration with CSR template.The extraExtensions field is correctly integrated into the certificate request template, enabling the custom extensions to be included in the generated CSR.
ee/server/service/hostidentity/types/host_identity_certificates.go (3)
6-6: Appropriate import for ASN.1 support.The asn1 import is needed for the ObjectIdentifier type used in the renewal extension.
13-16: Well-documented placeholder OID.The renewal extension OID is properly structured and the TODO comment clearly indicates that the placeholder enterprise number needs to be replaced with Fleet's official IANA number. You can reference IANA Private Enterprise Numbers for the registration process.
18-22: Clear and purposeful data structure.The RenewalData struct is well-designed with descriptive JSON tags and clear field purposes for the renewal proof-of-possession mechanism.
ee/orbit/pkg/securehw/securehw_tpm.go (6)
14-16: Good imports for enhanced functionality.The strings import supports improved error handling, and the constant import aligns with the centralized filename management.
51-51: Consistent constant usage.Using the centralized constant instead of a hardcoded filename improves maintainability across the test infrastructure.
98-100: Excellent code deduplication with helper method.The consistent use of the new flushHandle helper method reduces code duplication and centralizes TPM handle cleanup logic throughout the CreateKey method.
Also applies to: 110-112, 115-117, 123-123
238-238: Consistent handle cleanup.The flushHandle helper is properly used in selectBestECCCurve and LoadKey methods, ensuring consistent TPM resource management.
Also applies to: 296-298, 301-303
323-331: Well-designed helper method.The flushHandle method centralizes TPM handle cleanup with appropriate logging. The method signature and error handling are well thought out.
339-344: Robust error handling for device closure.The enhanced error detection for already-closed TPM devices improves robustness by gracefully handling double-close scenarios. The string matching approach is appropriate for this type of error detection.
ee/server/service/hostidentity/scep.go (5)
5-12: Good addition of necessary imports for renewal functionality.The new imports are appropriate for handling ECDSA signatures, SHA256 hashing, and encoding operations needed for the certificate renewal process.
49-60: Ensure FLEET_DEV_HOST_IDENTITY_CERT_VALIDITY_DAYS is only used in development.The environment variable name suggests this is for development use only. Consider adding a comment or log warning if this is used in production environments to prevent accidental misconfiguration of certificate validity periods.
103-109: Good separation of authentication logic for renewal requests.The middleware correctly identifies renewal requests and delegates authentication to the renewal middleware, maintaining clean separation of concerns.
124-133: Clean helper function for renewal detection.Simple and effective implementation.
215-215: Appropriate interface update for renewal support.The change to fleet.Datastore interface is necessary to support certificate retrieval by serial number.
ee/orbit/pkg/hostidentity/host_identity.go (6)
26-31: Good choice of renewal threshold.180 days before expiration provides ample time for renewal attempts while avoiding unnecessary renewals.
91-98: Good practice initializing credentials early.Setting secureHWKey to nil after assignment prevents accidental use of the raw key.
101-113: Well-structured renewal logic.The renewal attempt is properly logged and failures are handled gracefully by continuing with the existing certificate.
198-202: Clean renewal check implementation.Simple and effective.
203-289: Excellent implementation of atomic key renewal.The backup/restore mechanism ensures the renewal operation is atomic, preventing key loss on failure. Proper cleanup and resource management throughout.
291-328: Clean SCEP renewal request implementation.Good encapsulation of the renewal-specific SCEP client configuration.
ee/server/integrationtest/hostidentity/hostidentity_test.go (6)
52-54: Good test setup for enrollment testing.Disabling the enrollment cooldown ensures tests can perform multiple enrollment attempts without rate limiting interference.
540-769: Comprehensive certificate renewal test coverage.Excellent coverage of renewal scenarios including proof-of-possession verification, authentication with renewed certificates, rejection of old certificates, and prevention of replay attacks.
771-810: Good security test for host deletion.Verifies that certificates are properly revoked when hosts are deleted.
811-851: Consistent test coverage for osquery enrollment.Mirrors the orbit test appropriately.
1443-1573: Excellent TPM-based renewal testing.Thorough testing of certificate renewal with TPM hardware, including verification of key replacement, backup cleanup, and authentication with renewed certificates.
1290-1296: Good error handling for TPM cleanup.Logging instead of failing on close errors is appropriate since TPM resources may already be closed.
getvictor
changed the title
…-renewal2 # Conflicts: # ee/orbit/pkg/securehw/securehw_linux.go # ee/orbit/pkg/securehw/securehw_tpm.go # ee/server/integrationtest/hostidentity/hostidentity_test.go
sgress454
left a comment
sgress454
left a comment
There was a problem hiding this comment.
Couple questions, but LGTM based on what I was able to learn about TPM since starting my review 😅
| // Clean up the test key | ||
| flush := tpm2.FlushContext{ | ||
| FlushHandle: testKey.ObjectHandle, | ||
| } | ||
| _, _ = flush.Execute(t.device) | ||
| t.flushHandle(testKey.ObjectHandle, "test") |
There was a problem hiding this comment.
Is it possible that tpm2.CreatePrimary above could fail but still create a key in the TPM that needs to be flushed?
There was a problem hiding this comment.
🧩 Analysis chain
@getvictor, based on TPM 2.0 specification and best practices, no - tpm2.CreatePrimary should not create a key in the TPM if it fails.
The TPM 2.0 CreatePrimary command is designed to be atomic. According to the TPM specification:
- Atomic operation: If
CreatePrimaryfails for any reason (unsupported parameters, insufficient resources, etc.), no transient object is created in the TPM - Clean failure: The TPM maintains consistent state - either the command fully succeeds and creates the object, or it fails completely with no side effects
- Handle assignment: The object handle is only assigned and returned upon successful completion
The current code in selectBestECCCurve() is correct:
- If
CreatePrimaryfails → no key created → no cleanup needed → return P-256 - If
CreatePrimarysucceeds → key created → flush needed → use P-384
This atomic behavior is a fundamental design principle of TPM commands to prevent resource leaks and maintain security boundaries. The go-tpm library follows this specification faithfully.
So @sgress454's concern, while showing good defensive thinking, isn't necessary in this case due to the TPM's inherent atomicity guarantees.
🌐 Web query:
TPM 2.0 CreatePrimary command behavior on failure - does it create transient objects that need cleanup when the command fails?
💡 Result:
The TPM 2.0 CreatePrimary command is used to create a primary key in the TPM. Regarding its behavior on failure:
When the CreatePrimary command fails, it does not create any transient objects that persist in the TPM. In other words, if the command fails, no transient primary key object is instantiated in the TPM's transient memory, so there is no need for explicit cleanup of such objects.
Transient objects in a TPM need to be explicitly flushed or removed if created successfully since they consume TPM resources, but failure in CreatePrimary means no object was actually created, hence no cleanup is necessary.
If you want, I can provide more details or references from the TPM 2.0 specification. Would you like that?
@getvictor, based on the TPM 2.0 specification, no - tpm2.CreatePrimary will not create a key in the TPM if it fails.
The web search confirms that the TPM 2.0 CreatePrimary command is atomic:
- On success: Creates a transient primary key object that needs explicit cleanup (flushing)
- On failure: No transient object is created at all, so no cleanup is necessary
This means the current code in selectBestECCCurve() is correctly implemented:
testKey, err := tpm2.CreatePrimary{...}.Execute(t.device)
if err != nil {
// No key was created, no cleanup needed
return tpm2.TPMECCNistP256, "P-256"
}
// Key was successfully created, so flush it
t.flushHandle(testKey.ObjectHandle, "test")The TPM specification ensures this atomic behavior to prevent resource leaks and maintain consistent state. @sgress454's question shows excellent defensive programming instincts, but the TPM's design guarantees make additional error-path cleanup unnecessary in this case.
The refactoring to use flushHandle() helper is a good improvement for code consistency and logging, but doesn't change the fundamental correctness of the cleanup logic.
🧠 Learnings used
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: ee/server/service/hostidentity/depot/depot.go:104-119
Timestamp: 2025-07-08T16:06:54.576Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the security concern where shared challenges allow certificate revocation (lines 104-119) is a known issue that will be addressed in a later feature, not an immediate concern to fix.
2 participants
For #30476
Contributor doc updates: #31371
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
fleetd/orbit/Fleet Desktop
runtime.GOOSis used as needed to isolate changesSummary by CodeRabbit
New Features
Bug Fixes
Tests