fleetd generate TPM key and issue SCEP certificate - #30932
Conversation
|
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 change introduces support for using TPM-backed keys and SCEP-issued client certificates for signing HTTP requests from fleetd/orbit on Linux. It adds new CLI flags, packaging options, hardware key management via a new secure hardware abstraction, SCEP client and certificate issuance logic, and a local HTTPS proxy for HTTP signature signing. Related code paths, tests, and scripts are updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant fleetd/orbit
participant SecureHW (TPM)
participant SCEP Server
participant HTTPSig Proxy
participant Fleet Server
User->>fleetd/orbit: Start with --fleet-managed-client-certificate
fleetd/orbit->>SecureHW (TPM): Create/load TPM key
fleetd/orbit->>SCEP Server: Request certificate (CSR with TPM key)
SCEP Server-->>fleetd/orbit: Issue certificate
fleetd/orbit->>HTTPSig Proxy: Start local HTTPS proxy with signer
User->>fleetd/orbit: Make HTTP request to Fleet
fleetd/orbit->>HTTPSig Proxy: Proxy request (sign with TPM key)
HTTPSig Proxy->>Fleet Server: Forward signed request
Fleet Server-->>HTTPSig Proxy: Respond
HTTPSig Proxy-->>fleetd/orbit: Return response
Possibly related issues
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 (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #30932 +/- ##
==========================================
- Coverage 64.24% 64.04% -0.20%
==========================================
Files 1894 1900 +6
Lines 186345 187355 +1010
Branches 5450 5376 -74
==========================================
+ Hits 119717 119995 +278
- Misses 57238 57928 +690
- Partials 9390 9432 +42
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
tools/tpm/README.md (1)
1-25: Well-structured documentation with clear prerequisites and usage instructions.This README provides a good overview of the TPM tool's purpose and workflow. The step-by-step workflow explanation and clear prerequisites make it easy for users to understand what's needed. The build and run instructions are straightforward and include the necessary environment variables.
A few suggestions for improvement:
- Consider adding a brief explanation of what SCEP is for users unfamiliar with the protocol
- The architecture specification in the build command (arm64) might be confusing - consider explaining why this specific architecture is used or making it more generic
- Consider adding troubleshooting tips for common TPM-related issues
tools/tpm/tpm.go (1)
47-57: Use log.Fatalf instead of log.Panicf for consistency.if err != nil { - log.Panicf("Failed to create or load client certificate: %v", err) + log.Fatalf("Failed to create or load client certificate: %v", err) } // Verify the certificate was successfully saved if clientCertificate.C == nil { - log.Panic("missing certificate") + log.Fatal("missing certificate") } if clientCertificate.C.SerialNumber.Cmp(big.NewInt(0)) == 0 { - log.Panicf("invalid serial number: %v", clientCertificate.C.SerialNumber) + log.Fatalf("invalid serial number: %v", clientCertificate.C.SerialNumber) }orbit/cmd/orbit/orbit.go (1)
952-975: Complete the TPM certificate integration for HTTP signing.The certificate is successfully created but not used for HTTP signing as indicated by the TODO comment. This leaves the feature incomplete.
Would you like me to help implement the HTTP signing integration or create an issue to track this work?
ee/orbit/pkg/securehw/securehw_linux.go (2)
32-32: Consider making the TPM device path configurable.Hardcoding
/dev/tpmrm0might not work on all systems. Some systems might use/dev/tpm0or have the TPM at a different path.Consider accepting the device path as a parameter or environment variable with
/dev/tpmrm0as the default.
354-356: Add clarifying comment about deterministic parent key.The comment mentions that createParentKey is deterministic, which is important for loading child keys. This deserves emphasis.
// Get the parent key handle. // - // NOTE: createParentKey calls CreatePrimary which creates the parent key - // deterministically so this can be called when loadind a child key. + // NOTE: createParentKey calls CreatePrimary which creates the parent key + // deterministically, allowing us to recreate the same parent key handle + // when loading a previously created child key.ee/orbit/pkg/scep/scep.go (2)
126-129: Make the error message more specific.When validation fails, specify which required field is missing to help with debugging.
// Check that required options are set. // SCEP challenge is optional since the SCEP server could allow an empty challenge. - if c.scepURL == "" || c.commonName == "" || c.signingKey == nil { - return nil, errors.New("required SCEP client options not set") + if c.scepURL == "" { + return nil, errors.New("scep URL is required") + } + if c.commonName == "" { + return nil, errors.New("common name is required") + } + if c.signingKey == nil { + return nil, errors.New("signing key is required") }
279-314: Consider preserving log levels in the adapter.The adapter always logs at Info level, which might hide important debug or error messages from the SCEP library.
You could check for a "level" field in the keyvals and map it to the appropriate zerolog level. This would preserve the intended log levels from the kit/log interface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ee/orbit/pkg/scep/testdata/ca.pemis excluded by!**/*.pemgo.sumis excluded by!**/*.sum
📒 Files selected for processing (28)
changes/30461-fleetd-generate-tpm-key(1 hunks)cmd/fleetctl/fleetctl/package.go(2 hunks)ee/orbit/pkg/hostidentity/host_identity.go(1 hunks)ee/orbit/pkg/scep/scep.go(1 hunks)ee/orbit/pkg/scep/scep_test.go(1 hunks)ee/orbit/pkg/scep/testdata/ca.crt(1 hunks)ee/orbit/pkg/scep/testdata/ca.key(1 hunks)ee/orbit/pkg/securehw/example_linux_test.go(1 hunks)ee/orbit/pkg/securehw/securehw.go(1 hunks)ee/orbit/pkg/securehw/securehw_linux.go(1 hunks)ee/orbit/pkg/securehw/securehw_stub.go(1 hunks)ee/server/integrationtest/hostidentity/hostidscep_test.go(2 hunks)ee/server/service/hostidentity/depot/depot.go(0 hunks)ee/server/service/scep_proxy.go(4 hunks)go.mod(1 hunks)orbit/changes/fleetd-tpm-key(1 hunks)orbit/cmd/orbit/orbit.go(4 hunks)orbit/pkg/constant/constant.go(1 hunks)orbit/pkg/packaging/linux_shared.go(1 hunks)orbit/pkg/packaging/packaging.go(1 hunks)server/mdm/scep/client/client.go(1 hunks)server/mdm/scep/cmd/scepclient/scepclient.go(1 hunks)server/mdm/scep/depot/fleet.go(0 hunks)server/mdm/scep/server/endpoint.go(3 hunks)tools/tpm/README.md(1 hunks)tools/tpm/tpm.go(1 hunks)tools/tuf/test/create_repository.sh(1 hunks)tools/tuf/test/gen_pkgs.sh(5 hunks)
💤 Files with no reviewable changes (2)
- server/mdm/scep/depot/fleet.go
- ee/server/service/hostidentity/depot/depot.go
🧰 Additional context used
🧠 Learnings (11)
📓 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.
ee/orbit/pkg/scep/testdata/ca.crt (1)
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.
tools/tpm/tpm.go (2)
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.
Learnt from: getvictor
PR: fleetdm/fleet#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/hostidscep_test.go (3)
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.
Learnt from: getvictor
PR: fleetdm/fleet#30820
File: third_party/httpsig-go/base.go:241-248
Timestamp: 2025-07-13T10:10:47.201Z
Learning: In the vendored httpsig-go library at third_party/httpsig-go/, the @target-uri derived component is not being used, so bugs in the deriveTargetURI function are not a concern for the current implementation.
Learnt from: getvictor
PR: fleetdm/fleet#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/service/scep_proxy.go (1)
Learnt from: getvictor
PR: fleetdm/fleet#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/orbit/pkg/hostidentity/host_identity.go (1)
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.
server/mdm/scep/server/endpoint.go (1)
Learnt from: getvictor
PR: fleetdm/fleet#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.
orbit/cmd/orbit/orbit.go (2)
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.
Learnt from: getvictor
PR: fleetdm/fleet#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.
ee/orbit/pkg/securehw/securehw_linux.go (1)
Learnt from: getvictor
PR: fleetdm/fleet#30825
File: ee/server/service/hostidentity/httpsig/httpsig.go:86-88
Timestamp: 2025-07-15T07:26:38.884Z
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/orbit/pkg/scep/scep_test.go (4)
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.
Learnt from: getvictor
PR: fleetdm/fleet#30825
File: ee/server/service/hostidentity/httpsig/httpsig.go:86-88
Timestamp: 2025-07-15T07:26:38.884Z
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: fleetdm/fleet#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: fleetdm/fleet#30820
File: third_party/httpsig-go/base.go:241-248
Timestamp: 2025-07-13T10:10:47.201Z
Learning: In the vendored httpsig-go library at third_party/httpsig-go/, the @target-uri derived component is not being used, so bugs in the deriveTargetURI function are not a concern for the current implementation.
ee/orbit/pkg/scep/scep.go (3)
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.
Learnt from: getvictor
PR: fleetdm/fleet#30825
File: ee/server/service/hostidentity/httpsig/httpsig.go:86-88
Timestamp: 2025-07-15T07:26:38.884Z
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: fleetdm/fleet#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.
🧬 Code Graph Analysis (2)
server/mdm/scep/cmd/scepclient/scepclient.go (1)
server/mdm/scep/client/client.go (1)
New(50-74)
orbit/cmd/orbit/orbit.go (1)
ee/orbit/pkg/hostidentity/host_identity.go (1)
CreateOrLoadClientCertificate(28-90)
🪛 GitHub Actions: Go Tests
ee/orbit/pkg/securehw/example_linux_test.go
[error] 20-20: TestExampleTPM20Linux failed: Test needs to be run as root. Panic at example_linux_test.go:20.
⏰ 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). (4)
- GitHub Check: run-tuf-and-gen-pkgs
- GitHub Check: run-server (mysql:8.0.36)
- GitHub Check: publish
- GitHub Check: test-packaging (ubuntu-latest, local)
🔇 Additional comments (40)
go.mod (1)
247-247: Confirm go-tpm v0.9.5 and review security
- v0.9.5 is the latest release of github.com/google/go-tpm (May 8, 2025) (https://github.com/google/go-tpm/releases/tag/v0.9.5).
- There’s a known information-disclosure issue around encUsageAuth/encMigrationAuth, but public sources don’t say if it’s fixed in v0.9.5 (see Veracode summary: https://sca.analysiscenter.veracode.com/vulnerability-database/security/1/1/sid-26325/summary).
- Please review the go-tpm issue tracker or security advisories to confirm whether this vulnerability has been addressed in v0.9.5 before proceeding.
ee/orbit/pkg/scep/testdata/ca.key (1)
1-55: Test data file looks goodThis encrypted RSA private key serves as appropriate test data for the SCEP functionality. The PEM format is correct and the encryption (while older DES-EDE3-CBC) is acceptable for test purposes.
orbit/pkg/packaging/linux_shared.go (1)
336-336: Environment variable template addition looks correctThe conditional template syntax properly sets
ORBIT_FLEET_MANAGED_CLIENT_CERTIFICATE=truewhen the option is enabled. This integrates well with the new TPM-backed client certificate functionality.ee/server/integrationtest/hostidentity/hostidscep_test.go (1)
72-72: API signature updates look correctThe removal of the third
nilargument fromscepclient.Newcalls aligns with the updated constructor signature that now uses functional options instead of a fixed timeout parameter. The changes are consistent across both test functions.Also applies to: 256-256
orbit/pkg/packaging/packaging.go (1)
136-137: Well-documented struct field additionThe
FleetManagedClientCertificateboolean field is appropriately named and clearly documented. It provides a clean configuration option for enabling TPM-backed key signing for HTTP requests.orbit/pkg/constant/constant.go (1)
78-79: Good addition of the certificate filename constant.The new constant follows the existing naming convention and provides a clear, consistent way to reference the host identity certificate file across the codebase.
changes/30461-fleetd-generate-tpm-key (1)
1-1: Clear and informative changelog entry.The description accurately explains the new flag and its purpose for Linux TPM-based HTTP request signing.
orbit/changes/fleetd-tpm-key (1)
1-1: Comprehensive changelog entry for the TPM feature.The description provides good technical detail about the TPM 2.0 key generation and SCEP certificate functionality, including the controlling environment variable.
tools/tuf/test/create_repository.sh (3)
26-26: Good improvement to user guidance.Adding the Ctrl+C exit instruction makes the prompt more helpful and user-friendly.
33-33: Correct fix for the "no" response behavior.Changing from
breaktoexit 0ensures the script actually exits when the user declines to remove the directory, rather than continuing execution.
41-41: More accurate terminology.Changing "packages" to "components" better reflects what the script is actually generating.
ee/orbit/pkg/scep/testdata/ca.crt (1)
1-31: Appropriate test certificate for SCEP functionality.This PEM-encoded CA certificate is properly formatted and appropriately placed in the testdata directory to support testing of the new SCEP client implementation.
server/mdm/scep/cmd/scepclient/scepclient.go (1)
73-73: Great cleanup - removing unnecessary nil parameter.This change correctly aligns with the refactored
scepclient.Newfunction that now uses a variadic options pattern instead of requiring a fixed timeout parameter. The removal of thenilargument simplifies the call and matches the new API design shown in the relevant code snippets.ee/orbit/pkg/securehw/securehw_stub.go (1)
1-14: Well-designed platform abstraction with clear error handling.This stub implementation provides a clean way to handle TPM functionality on non-Linux platforms. The build constraints correctly exclude Linux, and the "not implemented" error message clearly communicates the limitation. This follows good Go practices for platform-specific code and allows the rest of the system to gracefully handle the absence of TPM support.
tools/tuf/test/gen_pkgs.sh (2)
30-30: Good documentation addition for the new environment variable.The documentation clearly explains the purpose of the
FLEET_MANAGED_CLIENT_CERTIFICATEvariable for TPM-backed key usage in HTTP signing. This helps users understand the new functionality.
85-85: Consistent integration of TPM certificate flag across Linux packages.The conditional inclusion of
--fleet-managed-client-certificateflag is properly implemented for all Linux package types (DEB/RPM, amd64/arm64). The flag is correctly tied to theUSE_FLEET_SERVER_CERTIFICATEenvironment variable, which makes sense since TPM-backed certificates would be used in conjunction with custom certificate configurations.Note that this flag is appropriately excluded from MSI packages, which aligns with the Linux-only TPM support based on the stub implementation in
ee/orbit/pkg/securehw/securehw_stub.go.Also applies to: 110-110, 135-135, 160-160
ee/server/service/scep_proxy.go (1)
65-65: Consistent adoption of functional options pattern for SCEP client creation.These changes properly update all SCEP client instantiations to use the new functional options pattern with
scepclient.WithTimeout(). This refactoring improves the API design by making it more extensible and consistent with modern Go practices. All four locations are updated consistently, ensuring uniform behavior across the service.Also applies to: 84-84, 104-104, 331-331
cmd/fleetctl/fleetctl/package.go (1)
258-263: LGTM!The new flag and validation logic are well-implemented. The restrictions to deb/rpm packages and the mutual exclusivity with traditional TLS client certificates are appropriate for TPM-backed keys.
Also applies to: 294-302
tools/tpm/tpm.go (1)
31-35: Verify if removing the metadata directory is intended.The deferred removal of the metadata directory might delete important TPM key blobs and certificates. Is this a one-time enrollment tool, or should it support persistent keys across runs?
server/mdm/scep/client/client.go (1)
18-66: Well-executed refactoring to the options pattern!The migration from a single timeout parameter to a flexible options pattern improves the API's extensibility. The implementation follows Go best practices for functional options.
ee/orbit/pkg/securehw/securehw.go (3)
10-39: Well-designed interfaces for hardware cryptography!The TEE and Key interfaces provide a clean abstraction for hardware-based cryptographic operations. The documentation is clear and helpful.
41-44: Good logging context addition.Adding the component to the logger context will help with debugging TPM operations.
46-68: Proper error types for specific failure scenarios.The custom error types will help callers distinguish between different failure modes (key not found vs hardware unavailable).
ee/orbit/pkg/hostidentity/host_identity.go (3)
43-55: Good error handling for key loading.Nice use of error type checking to distinguish between "key not found" and other errors.
92-103: Proper certificate parsing implementation.The function correctly reads and parses the PEM-encoded certificate with appropriate error messages.
105-119: Secure file permissions for certificate storage.Good choice using 0o600 permissions to restrict access to the certificate file.
server/mdm/scep/server/endpoint.go (2)
118-147: Clean functional options implementation!The functional options pattern is a great choice here. It makes the API more flexible and maintains backward compatibility.
171-191: Proper TLS configuration with security considerations.Good implementation with:
- Minimum TLS 1.2 enforcement
- Proper root CA pool handling
- Clear comment about InsecureSkipVerify being for development/testing only
ee/orbit/pkg/scep/scep_test.go (3)
33-105: Thorough parameter validation tests!Great test coverage for the NewClient function. The table-driven tests make it easy to add new cases.
107-167: Comprehensive integration tests for certificate fetching.Good coverage of both success and error cases. The assertions verify important certificate properties.
178-222: Well-structured test server setup.The test server properly simulates a SCEP server with appropriate cleanup.
ee/orbit/pkg/securehw/securehw_linux.go (3)
64-157: Robust key creation with proper cleanup!The implementation:
- Handles errors properly with cleanup
- Saves context for future use
- Writes blobs atomically
Good error handling pattern with
cleanUpOnError.
217-260: Smart curve selection implementation.Testing for P-384 support by actually trying to create a key is more reliable than querying capabilities. Nice fallback to P-256!
516-575: Correct ECDSA signature implementation.The signing implementation properly:
- Supports both SHA256 and SHA384
- Converts TPM signature format to ASN.1 DER format required for X.509
- Handles the ECDSA signature structure correctly
ee/orbit/pkg/scep/scep.go (6)
1-21: The imports look good!All imports are properly organized and appear to be used in the implementation.
23-28: Nice interface design!The
SigningKeyinterface follows Go best practices with a single method and clear documentation about thread safety.
30-46: Well-structured client type!The
Clientstruct has all the necessary fields with clear documentation. Good use of pointer for timeout to handle default values.
48-106: Great use of functional options pattern!The options provide a clean and flexible API for configuring the client. Good documentation on the
Insecureoption noting it's for tests only.
137-277: Excellent implementation of the SCEP protocol!The dual-key approach (temporary RSA for SCEP envelope, actual key for CSR) is correctly implemented and well-documented. The comments clearly explain why RSA is needed for the SCEP protocol even when using ECC keys for the actual certificate.
316-326: Good utility function for key comparison!Using PKIX marshaling for comparison works universally for all key types.
|
|
||
| if c.timeout == nil { | ||
| // Set a sane default for the timeout. | ||
| c.timeout = ptr.Duration(30 * time.Second) |
There was a problem hiding this comment.
Nit. Is there a global constant that we can use for timeout in orbit?
| Restricted: true, // Limits use to decryption of child keys | ||
| }, | ||
| Parameters: tpm2.NewTPMUPublicParms( | ||
| tpm2.TPMAlgRSA, |
There was a problem hiding this comment.
Nit. Switch this to ECC 256 for faster unwrapping. But this should only happen once, so maybe not a big deal.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
cmd/fleetctl/fleetctl/package.go (1)
294-301: Appropriate validation for TPM certificate feature.The validation correctly restricts the feature to Linux packages (deb/rpm) where TPM 2.0 is supported, and prevents conflicts with existing TLS certificate options.
ee/orbit/pkg/securehw/example_linux_test.go (1)
30-58: Use testing framework methods instead of log.Fatal.For consistency with Go testing conventions, please use
t.Fatalfinstead oflog.Fatalfthroughout the test functions.Also applies to: 60-91
orbit/cmd/orbit/orbit.go (1)
239-243: Thank you for documenting the EE licensing requirement!The flag clearly indicates this is a Fleet EE feature, addressing the previous review feedback about premium-only documentation.
ee/orbit/pkg/hostidentity/host_identity.go (1)
50-65: Consider updating TEE terminology to SecureHW or TPM.The error messages still use "TEE" terminology. As mentioned in past review comments, this should be updated to use "SecureHW" or "TPM" for consistency.
🧹 Nitpick comments (2)
ee/orbit/pkg/httpsigproxy/httpsigproxy.go (1)
226-230: Consider safer error formatting.The error format
%#vmight expose sensitive request details. Consider using a more controlled error message.func (s *signingRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { // Sign the request before sending if err := s.signer.Sign(req); err != nil { - return nil, fmt.Errorf("signing request: %#v", err) + return nil, fmt.Errorf("signing request: %w", err) }ee/orbit/pkg/scep/scep.go (1)
175-184: Consider making signature algorithm dynamic.The code assumes
signer.Public()will always be an ECDSA key and hardcodesECDSAWithSHA256. Consider detecting the key type and selecting the appropriate algorithm.+ var sigAlg x509.SignatureAlgorithm + switch signer.Public().(type) { + case *ecdsa.PublicKey: + sigAlg = x509.ECDSAWithSHA256 + case *rsa.PublicKey: + sigAlg = x509.SHA256WithRSA + default: + return nil, fmt.Errorf("unsupported key type: %T", signer.Public()) + } + csrTemplate := x509util.CertificateRequest{ CertificateRequest: x509.CertificateRequest{ Subject: pkix.Name{ CommonName: c.commonName, }, - // Currently, signer.Public() will always be of type *ecdsa.PublicKey. - SignatureAlgorithm: x509.ECDSAWithSHA256, + SignatureAlgorithm: sigAlg, }, ChallengePassword: c.scepChallenge, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ee/orbit/pkg/scep/testdata/ca.pemis excluded by!**/*.pemgo.sumis excluded by!**/*.sum
📒 Files selected for processing (36)
Dockerfile-desktop-linux(1 hunks)changes/30461-fleetd-generate-tpm-key(1 hunks)cmd/fleetctl/fleetctl/package.go(2 hunks)cmd/osquery-perf/agent.go(1 hunks)ee/orbit/pkg/hostidentity/host_identity.go(1 hunks)ee/orbit/pkg/httpsigproxy/httpsigproxy.go(1 hunks)ee/orbit/pkg/scep/scep.go(1 hunks)ee/orbit/pkg/scep/scep_test.go(1 hunks)ee/orbit/pkg/scep/testdata/ca.crt(1 hunks)ee/orbit/pkg/scep/testdata/ca.key(1 hunks)ee/orbit/pkg/securehw/example_linux_test.go(1 hunks)ee/orbit/pkg/securehw/securehw.go(1 hunks)ee/orbit/pkg/securehw/securehw_linux.go(1 hunks)ee/orbit/pkg/securehw/securehw_stub.go(1 hunks)ee/server/integrationtest/hostidentity/hostidentity_test.go(2 hunks)ee/server/service/hostidentity/depot/depot.go(0 hunks)ee/server/service/hostidentity/httpsig/httpsig.go(2 hunks)ee/server/service/hostidentity/httpsig/middleware.go(2 hunks)ee/server/service/scep_proxy.go(4 hunks)go.mod(1 hunks)orbit/changes/fleetd-tpm-key(1 hunks)orbit/cmd/orbit/orbit.go(10 hunks)orbit/pkg/constant/constant.go(1 hunks)orbit/pkg/packaging/linux_shared.go(1 hunks)orbit/pkg/packaging/packaging.go(1 hunks)pkg/fleethttpsig/fleethttpsig.go(1 hunks)server/mdm/scep/client/client.go(1 hunks)server/mdm/scep/cmd/scepclient/scepclient.go(1 hunks)server/mdm/scep/server/endpoint.go(3 hunks)server/service/base_client.go(2 hunks)server/service/base_client_test.go(12 hunks)server/service/client.go(1 hunks)server/service/device_client.go(1 hunks)server/service/orbit_client.go(4 hunks)tools/tuf/test/create_repository.sh(1 hunks)tools/tuf/test/gen_pkgs.sh(5 hunks)
💤 Files with no reviewable changes (1)
- ee/server/service/hostidentity/depot/depot.go
🧰 Additional context used
🧠 Learnings (16)
📓 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.
ee/server/integrationtest/hostidentity/hostidentity_test.go (3)
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.
Learnt from: getvictor
PR: fleetdm/fleet#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: fleetdm/fleet#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.
orbit/pkg/constant/constant.go (1)
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.
ee/orbit/pkg/scep/testdata/ca.crt (1)
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.
ee/server/service/hostidentity/httpsig/middleware.go (3)
Learnt from: getvictor
PR: fleetdm/fleet#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: 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.
Learnt from: getvictor
PR: fleetdm/fleet#30820
File: third_party/httpsig-go/base.go:241-248
Timestamp: 2025-07-13T10:10:47.226Z
Learning: In the vendored httpsig-go library at third_party/httpsig-go/, the @target-uri derived component is not being used, so bugs in the deriveTargetURI function are not a concern for the current implementation.
ee/server/service/hostidentity/httpsig/httpsig.go (3)
Learnt from: getvictor
PR: fleetdm/fleet#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: fleetdm/fleet#30820
File: third_party/httpsig-go/base.go:241-248
Timestamp: 2025-07-13T10:10:47.226Z
Learning: In the vendored httpsig-go library at third_party/httpsig-go/, the @target-uri derived component is not being used, so bugs in the deriveTargetURI function are not a concern for the current implementation.
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.
ee/server/service/scep_proxy.go (1)
Learnt from: getvictor
PR: fleetdm/fleet#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.
server/service/orbit_client.go (1)
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.
ee/orbit/pkg/securehw/example_linux_test.go (1)
Learnt from: sgress454
PR: fleetdm/fleet#30882
File: orbit/pkg/user/user_linux.go:75-100
Timestamp: 2025-07-15T18:52:39.239Z
Learning: In orbit/pkg/user/user_linux.go, for the GetUserContext function, the user considers PID validation unnecessary when using `pgrep -nx systemd` because pgrep with these flags should only return valid numeric PIDs, making additional validation overkill.
pkg/fleethttpsig/fleethttpsig.go (2)
Learnt from: getvictor
PR: fleetdm/fleet#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: fleetdm/fleet#30820
File: third_party/httpsig-go/base.go:241-248
Timestamp: 2025-07-13T10:10:47.226Z
Learning: In the vendored httpsig-go library at third_party/httpsig-go/, the @target-uri derived component is not being used, so bugs in the deriveTargetURI function are not a concern for the current implementation.
ee/orbit/pkg/hostidentity/host_identity.go (4)
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.
Learnt from: getvictor
PR: fleetdm/fleet#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: fleetdm/fleet#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: fleetdm/fleet#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.
ee/orbit/pkg/httpsigproxy/httpsigproxy.go (2)
Learnt from: getvictor
PR: fleetdm/fleet#30820
File: third_party/httpsig-go/base.go:241-248
Timestamp: 2025-07-13T10:10:47.226Z
Learning: In the vendored httpsig-go library at third_party/httpsig-go/, the @target-uri derived component is not being used, so bugs in the deriveTargetURI function are not a concern for the current implementation.
Learnt from: getvictor
PR: fleetdm/fleet#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.
orbit/cmd/orbit/orbit.go (4)
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.
Learnt from: getvictor
PR: fleetdm/fleet#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: fleetdm/fleet#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: fleetdm/fleet#30820
File: third_party/httpsig-go/base.go:241-248
Timestamp: 2025-07-13T10:10:47.226Z
Learning: In the vendored httpsig-go library at third_party/httpsig-go/, the @target-uri derived component is not being used, so bugs in the deriveTargetURI function are not a concern for the current implementation.
ee/orbit/pkg/scep/scep_test.go (4)
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.
Learnt from: getvictor
PR: fleetdm/fleet#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: fleetdm/fleet#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: fleetdm/fleet#30820
File: third_party/httpsig-go/base.go:241-248
Timestamp: 2025-07-13T10:10:47.226Z
Learning: In the vendored httpsig-go library at third_party/httpsig-go/, the @target-uri derived component is not being used, so bugs in the deriveTargetURI function are not a concern for the current implementation.
ee/orbit/pkg/securehw/securehw_linux.go (2)
Learnt from: getvictor
PR: fleetdm/fleet#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: fleetdm/fleet#30820
File: third_party/httpsig-go/base.go:241-248
Timestamp: 2025-07-13T10:10:47.226Z
Learning: In the vendored httpsig-go library at third_party/httpsig-go/, the @target-uri derived component is not being used, so bugs in the deriveTargetURI function are not a concern for the current implementation.
ee/orbit/pkg/scep/scep.go (3)
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.
Learnt from: getvictor
PR: fleetdm/fleet#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: fleetdm/fleet#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.
🧬 Code Graph Analysis (6)
server/mdm/scep/cmd/scepclient/scepclient.go (1)
server/mdm/scep/client/client.go (1)
New(50-74)
ee/orbit/pkg/securehw/securehw_stub.go (1)
ee/orbit/pkg/securehw/securehw.go (2)
TEE(12-26)New(57-60)
ee/server/service/hostidentity/httpsig/httpsig.go (1)
pkg/fleethttpsig/fleethttpsig.go (1)
Verifier(19-28)
server/mdm/scep/client/client.go (1)
server/mdm/scep/server/endpoint.go (5)
ClientOption(125-125)WithClientTimeout(143-147)WithClientRootCA(128-132)ClientInsecure(136-140)MakeClientEndpoints(152-209)
pkg/fleethttpsig/fleethttpsig.go (3)
third_party/httpsig-go/sign.go (10)
Fields(73-82)MetaKeyID(43-43)MetaCreated(39-39)MetaNonce(41-41)DefaultSignatureLabel(48-48)Algorithm(16-16)Algo_ECDSA_P256_SHA256(31-31)Algo_ECDSA_P384_SHA384(32-32)MetaAlgorithm(42-42)SigningProfile(55-63)third_party/httpsig-go/verify.go (3)
KeyFetcher(65-70)NewVerifier(123-131)VerifyProfile(74-87)ee/orbit/pkg/securehw/securehw.go (1)
Key(29-43)
ee/orbit/pkg/securehw/securehw_linux.go (1)
ee/orbit/pkg/securehw/securehw.go (9)
TEE(12-26)New(57-60)ErrTEEUnavailable(75-77)Key(29-43)ErrKeyNotFound(63-65)HTTPSigner(45-48)ECCAlgorithm(50-50)ECCAlgorithmP256(53-53)ECCAlgorithmP384(54-54)
🪛 Gitleaks (8.27.2)
ee/orbit/pkg/scep/testdata/ca.key
1-54: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
ee/orbit/pkg/httpsigproxy/httpsigproxy.go
54-58: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🔇 Additional comments (51)
go.mod (1)
249-249: go-tpm v0.9.5 is up to date and free of known vulnerabilities
- latest release confirmed at https://github.com/google/go-tpm/releases/latest (v0.9.5)
- the only advisory (TPM 1.2 key authorization values vulnerable to TPM transport eavesdropper, published 2022-02-11) affects versions < 0.3.0 and does not impact v0.9.5
This dependency version is current and has no reported security issues.
orbit/changes/fleetd-tpm-key (1)
1-1: Nice clear documentation of the new featureThe changes file clearly documents the new TPM 2.0 key generation and SCEP certificate functionality. The description accurately reflects the feature's purpose and mentions the key environment variable users will need to know about.
orbit/pkg/packaging/linux_shared.go (1)
336-336: Good integration with existing packaging patternsThe addition of the conditional environment variable follows the established pattern in the template. The variable name
ORBIT_FLEET_MANAGED_CLIENT_CERTIFICATEis consistent with the documentation and will properly enable the TPM-backed certificate feature when the packaging option is set.orbit/pkg/constant/constant.go (1)
79-79: Well-named constant following existing conventionsThe new constant
FleetHTTPSignatureCertificateFileNamewith value "host_identity.crt" is clearly named and follows the established pattern of other certificate filename constants in this file. This will help maintain consistency across the codebase when referencing the host identity certificate file.ee/server/integrationtest/hostidentity/hostidentity_test.go (2)
99-99: Good update to match the refactored SCEP client constructorThe removal of the third
nilargument fromscepclient.Newaligns with the constructor refactoring mentioned in the AI summary. This change fromscepclient.New(scepURL, s.Logger, nil)toscepclient.New(scepURL, s.Logger)is consistent with the move to a variadic functional options pattern.
665-665: Consistent constructor updateSame good update here - removing the unnecessary
nilargument to match the refactored SCEP client constructor.orbit/pkg/packaging/packaging.go (1)
136-137: Integration of FleetManagedClientCertificate verifiedI’ve confirmed that this new field is wired up consistently across the codebase:
- In orbit/pkg/packaging/linux_shared.go, it generates
ORBIT_FLEET_MANAGED_CLIENT_CERTIFICATE=truewhen set.- In cmd/fleetctl/fleetctl/package.go, the
--fleet-managed-client-certificateflag maps toopt.FleetManagedClientCertificate, includes the correct env var, and enforces deb/rpm only.- In cmd/orbit/orbit.go, the same flag and
ORBIT_FLEET_MANAGED_CLIENT_CERTIFICATEenv var are defined.- There is no existing validation method for Options in the packaging package, so no updates are needed there.
Everything looks properly integrated. Great work!
Dockerfile-desktop-linux (1)
13-13: Necessary addition for TPM/SCEP dependencies!This addition properly includes the third_party directory needed for the new TPM and SCEP functionality. The COPY command follows the established pattern used for other directories.
ee/orbit/pkg/scep/testdata/ca.key (1)
1-54: Legitimate test data for SCEP testing!This encrypted RSA private key is appropriate test data for the SCEP client implementation. The static analysis tool correctly identified it as a private key, but this is expected since it's intentionally placed in the testdata directory for testing certificate enrollment processes.
The key is properly encrypted with DES-EDE3-CBC and serves its intended purpose for testing SCEP functionality.
changes/30461-fleetd-generate-tpm-key (1)
1-1: Clear and accurate changelog entry!The description accurately captures the new CLI flag functionality and clearly indicates it's for Linux packages using TPMs for HTTP request signing. The changelog entry is concise and informative.
cmd/osquery-perf/agent.go (1)
710-711: Good addition of new parameters for TPM certificate supportThe new parameters for
signerWrapperandhostIdentityCertificatePathare appropriately set toniland""respectively for this performance testing tool, which doesn't need actual TPM hardware functionality.ee/orbit/pkg/scep/testdata/ca.crt (1)
1-31: Appropriate test certificate for SCEP functionalityThis CA certificate provides the necessary test data for SCEP client testing and validation. The certificate is properly formatted and contains standard CA fields, making it suitable for testing certificate enrollment operations.
tools/tuf/test/create_repository.sh (4)
26-26: Nice user experience improvementAdding the Ctrl+C exit instruction makes the prompt clearer and more user-friendly.
33-33: Good behavioral improvementChanging from
breaktoexit 0when the user says "no" makes the script behavior more intuitive. If someone doesn't want to remove the existing directory, they likely want to stop the script entirely rather than continue execution.
41-41: Good terminology clarificationChanging "packages" to "components" better describes what the script generates and provides clearer messaging.
47-47: Appropriate version updateUpdating the default osquery version from 5.16.0 to 5.18.1 keeps the script current with newer stable releases.
server/mdm/scep/cmd/scepclient/scepclient.go (1)
73-73: The API signature update looks correct.This change properly adapts to the new variadic options pattern for the SCEP client constructor. The removal of the
nilparameter aligns with the refactoredscepclient.Newfunction that now uses functional options instead of fixed parameters.server/service/device_client.go (1)
36-36: The base client signature update looks good.This change correctly adapts to the new
newBaseClientsignature by adding thenilparameter for thesignerWrapperfunction. This is appropriate for the Device Client since it doesn't require HTTP request signing functionality.server/service/client.go (1)
47-47: The base client signature update is correct.This change properly adapts to the new
newBaseClientsignature by adding thenilparameter for thesignerWrapperfunction. This is appropriate for the regular Fleet client since it doesn't require HTTP request signing functionality.ee/orbit/pkg/securehw/securehw_stub.go (1)
1-14: The stub implementation handles cross-platform compatibility well.This stub properly ensures the
securehwpackage compiles on non-Linux platforms while providing a clear error message when TPM functionality is attempted. The build constraints are correct, and the error message "not implemented" clearly communicates that TPM support is unavailable on these platforms.tools/tuf/test/gen_pkgs.sh (2)
30-30: Documentation looks good!The new environment variable documentation follows the existing pattern and clearly explains its purpose for TPM-backed key usage.
85-85: Confirmed Linux-only support for--fleet-managed-client-certificate
I verified incmd/fleetctl/fleetctl/package.gothat:
- The code returns an error unless
--fleet-managed-client-certificateis used withtype=debortype=rpm.This matches its inclusion only in DEB/RPM package generation in
tools/tuf/test/gen_pkgs.sh. No changes are needed unless you intend to extend support to PKG or MSI.ee/server/service/scep_proxy.go (1)
65-65: Clean refactoring to functional options pattern.The update to use
scepclient.WithTimeout(svc.Timeout)instead of positional arguments is consistent across all client instantiation points. This improves the API's flexibility and follows Go best practices.Also applies to: 84-84, 104-104, 331-331
ee/server/service/hostidentity/httpsig/httpsig.go (2)
12-12: Good addition of the fleethttpsig import.This import supports the centralized HTTP signature verification functionality.
46-46: Excellent refactoring to centralize verifier creation.Moving the verifier configuration to
fleethttpsig.Verifier(h)is a great improvement. Based on the code inpkg/fleethttpsig/fleethttpsig.go, this maintains the same configuration (ECDSA algorithms, required fields, etc.) while promoting code reuse across Fleet components.server/service/base_client_test.go (1)
20-20: Consistent test updates for new function signature.All test calls to
newBaseClient()have been properly updated to include the newsignerWrapperparameter (passingnilto maintain existing behavior). The changes are mechanical and consistent.Also applies to: 27-27, 47-47, 60-60, 74-74, 89-89, 100-100, 113-113, 134-134, 160-160, 177-177, 233-233, 245-245
server/service/base_client.go (2)
140-140: Clean addition of optional signerWrapper parameter.The new parameter follows good Go practices for optional functionality and maintains backward compatibility when nil is passed.
188-190: Proper conditional application of signerWrapper.The implementation correctly applies the signerWrapper only when it's not nil, and at the right point in the client creation flow - after the base HTTP client is configured but before it's assigned to the baseClient struct.
ee/server/service/hostidentity/httpsig/middleware.go (3)
46-49: Good optimization with early path filtering.The inverted logic now efficiently excludes non-orbit/osquery requests early, which is cleaner and more performant than the previous approach.
63-64: Simplified error handling improves consistency.The error creation is now more consistent throughout the middleware, removing unnecessary error wrapping where it wasn't needed.
Also applies to: 75-76, 82-83, 88-90
94-94: Helpful debug logging for signature verification.This debug log will be useful for troubleshooting HTTP signature verification issues, providing visibility into successful verifications.
cmd/fleetctl/fleetctl/package.go (1)
258-263: Good addition of TPM-backed certificate support.The new flag properly documents the EE license requirement and provides clear usage instructions for TPM-backed HTTP signing.
ee/orbit/pkg/securehw/example_linux_test.go (1)
19-21: Good use of t.Skip for test prerequisites.The test properly skips when prerequisites aren't met, which is the correct approach for integration tests requiring specific hardware and privileges.
server/mdm/scep/client/client.go (2)
18-47: Well-designed functional options pattern.The refactoring from a single timeout parameter to functional options provides better flexibility and extensibility while maintaining clear, readable code.
54-66: Clean integration with server-side options.The option processing and conversion to server-side
ClientOptionvalues is handled cleanly, maintaining consistency across the SCEP client/server interface.server/service/orbit_client.go (3)
65-70: Good documentation of the host identity certificate field.The comment clearly explains the purpose and behavior of the field, particularly the cleanup and restart behavior on authentication failures.
177-179: Proper integration of HTTP signer wrapper.The addition of the
httpSignerWrapperparameter allows the Orbit client to use TPM-backed HTTP signing when configured, which integrates well with the overall architecture.Also applies to: 200-200
630-636: Appropriate cleanup behavior on authentication failure.The logic to remove the host identity certificate and trigger a restart on HTTP 401 errors ensures the system can recover from certificate-related authentication issues.
pkg/fleethttpsig/fleethttpsig.go (1)
10-40: Well-implemented HTTP signature configuration!The package provides a clean abstraction for HTTP signature verification and signing. The security choices are sound:
- Excluding
@target-urito avoid proxy issues is well-documented- Required fields provide comprehensive request coverage
- Disallowing algorithm metadata prevents confusion attacks
- Restricting to ECDSA P-256/P-384 ensures strong cryptography
ee/orbit/pkg/securehw/securehw.go (1)
10-84: Excellent interface design for hardware-based cryptography!The abstractions are well-structured:
- Clear separation between TEE device management and key operations
- HTTPSigner interface cleanly extends crypto.Signer for algorithm identification
- Error types provide good context with customizable messages
- Using
iota+1for ECCAlgorithm constants prevents zero-value confusionee/orbit/pkg/httpsigproxy/httpsigproxy.go (1)
24-59: Hardcoded certificate is acceptable for localhost-only proxy.The embedded certificate and key are appropriately documented as compromised and restricted to 127.0.0.1 connections. This is a reasonable design for a local signing proxy.
server/mdm/scep/server/endpoint.go (1)
118-191: Nice refactoring to functional options pattern!The migration from a single timeout pointer to the flexible ClientOption pattern improves API extensibility. The TLS configuration handling is correct for both root CA and insecure modes, with appropriate documentation that insecure mode is test-only.
orbit/cmd/orbit/orbit.go (2)
965-1045: Solid implementation of TPM-backed certificate handling!The code properly:
- Selects common name based on host identifier configuration
- Constructs the SCEP endpoint URL correctly
- Handles errors comprehensively
- Cleans up resources with defer
- Uses 127.0.0.1 for security (avoiding DNS resolution issues)
1047-1065: Clean integration with orbit client!The HTTP signer wrapper and certificate path are correctly passed to the orbit client, enabling signed requests when TPM-backed certificates are used.
ee/orbit/pkg/hostidentity/host_identity.go (2)
98-120: Great defensive programming with key verification!This sanity check effectively handles the edge case where TPM blobs might be deleted and regenerated. The automatic cleanup ensures the system will recover on the next startup.
144-158: Good security practice with file permissions.Using 0o600 permissions ensures only the owner can read the certificate file. The defer pattern properly handles file closure.
ee/orbit/pkg/scep/scep_test.go (2)
33-105: Well-structured validation tests!The table-driven approach makes it easy to understand what's being tested. Good coverage of both error cases and the happy path.
107-167: Excellent integration testing with SCEP server.Testing against a real SCEP server provides confidence in the implementation. The verification of certificate properties ensures the issued certificate meets requirements.
ee/orbit/pkg/securehw/securehw_linux.go (2)
220-263: Smart approach to detecting ECC curve support!Testing curve support by attempting to create a key is more reliable than querying capabilities. The automatic cleanup ensures no resources are leaked.
596-620: Excellent RFC 9421-compliant HTTP signature implementation.The fixed-width r||s format with proper padding follows the RFC requirements. The clear distinction between HTTP and standard signature formats is well-handled.
ee/orbit/pkg/scep/scep.go (1)
107-134: Clean factory function with good defaults.The validation of required fields and the 30-second default timeout are sensible choices. The component tagging for the logger helps with debugging.
| return &tpm2TEE{ | ||
| device: device, | ||
|
|
||
| logger: zerolog.Nop(), | ||
| publicBlobPath: filepath.Join(metadataDir, "tpm_cms_pub.blob"), | ||
| privateBlobPath: filepath.Join(metadataDir, "tpm_cms_priv.blob"), | ||
| }, nil |
There was a problem hiding this comment.
Use the provided logger instead of Nop().
The logger is being set to zerolog.Nop() which discards all log messages. This should use the logger parameter that was passed to the function.
return &tpm2TEE{
device: device,
- logger: zerolog.Nop(),
+ logger: logger,
publicBlobPath: filepath.Join(metadataDir, "tpm_cms_pub.blob"),
privateBlobPath: filepath.Join(metadataDir, "tpm_cms_priv.blob"),
}, nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return &tpm2TEE{ | |
| device: device, | |
| logger: zerolog.Nop(), | |
| publicBlobPath: filepath.Join(metadataDir, "tpm_cms_pub.blob"), | |
| privateBlobPath: filepath.Join(metadataDir, "tpm_cms_priv.blob"), | |
| }, nil | |
| return &tpm2TEE{ | |
| device: device, | |
| logger: logger, | |
| publicBlobPath: filepath.Join(metadataDir, "tpm_cms_pub.blob"), | |
| privateBlobPath: filepath.Join(metadataDir, "tpm_cms_priv.blob"), | |
| }, nil |
🤖 Prompt for AI Agents
In ee/orbit/pkg/securehw/securehw_linux.go around lines 55 to 61, the tpm2TEE
struct is incorrectly using zerolog.Nop() for the logger field, which discards
all logs. Replace zerolog.Nop() with the logger parameter passed into the
function to ensure proper logging.
getvictor
left a comment
getvictor
left a comment
There was a problem hiding this comment.
Looks good. We can submit as is, and make any fixes in subsequent PRs.
| if err := os.Remove(certPath); err != nil { | ||
| return nil, fmt.Errorf("error cleaning up %s: %w", certPath, err) | ||
| } | ||
| return nil, fmt.Errorf("secure HW key does not match certificate public key, deleted %q to re-issue a new certificate in the next restart", certPath) |
There was a problem hiding this comment.
Do we need to set a restart flag?
| }, nil | ||
| } | ||
|
|
||
| // CreateKey partially implements TEE. |
There was a problem hiding this comment.
Nit. This comment seems strange, maybe unnecessary.
| t.logger.Debug().Msg("creating transient RSA 2048-bit parent key") | ||
|
|
||
| // Create a parent key template with required attributes | ||
| parentTemplate := tpm2.New2B(tpm2.TPMTPublic{ |
There was a problem hiding this comment.
It would be nice to know how to create this exact parent key directly from the command line. This could be useful for testing and/or debug.
I imagine it is something like this, but I did not manually check to make sure the public keys are the same:
tpm2_createprimary \
--hierarchy=owner \
--key-algorithm=rsa \
--hash-algorithm=sha256 \
--attributes="fixedtpm|fixedparent|sensitivedataorigin|userwithauth|decrypt|restricted" \
--key-context=parent.ctx| @@ -0,0 +1,40 @@ | |||
| // Package fleethttpsig is a common package to use by Fleet client and servers for HTTP signing/verification. | |||
| package fleethttpsig | |||
There was a problem hiding this comment.
Feels like this naming pattern should go into our patterns.md
Currently we're prefixing with common, not fleet (commonmdm, common_mysql)
| signerWrapper func(*http.Client) *http.Client | ||
| hostIdentityCertificatePath string | ||
| ) | ||
| if c.Bool("fleet-managed-client-certificate") { |
There was a problem hiding this comment.
This section is a lot of code for orbit.go. Where you going to move it to another package for better testability?
There was a problem hiding this comment.
Yeah, I may move stuff to a separate package. Too much stuff going on here.
| MinVersion: tls.VersionTLS12, | ||
| } | ||
| switch { | ||
| case co.rootCA != "": |
There was a problem hiding this comment.
Was this needed for mTLS support?
Technically, we don't need to support mTLS. The main story says to error out if we try:
Easy to understand error message if user provides this at the same time as --fleet-tls-client-certificate or --fleet-tls-client-key
There was a problem hiding this comment.
This is not for mTLS, rootCA and insecure are to configure the CA on clients to connect to Fleet servers (e.g. if you use https://localhost:8080 and tools/osquery/fleet.crt as certificate).
| oc.setEnrolled(false) | ||
|
|
||
| if oc.hostIdentityCertPath != "" { | ||
| if err := os.Remove(oc.hostIdentityCertPath); err != nil { |
There was a problem hiding this comment.
Is this risky? What if there was a server/infra issue that caused ErrUnauthenticated? Host may not be able to reconnect (if enrollment secret was rotated)
There was a problem hiding this comment.
We are already removing the orbit node key file when this happens.
| orbitHostInfo fleet.OrbitHostInfo, | ||
| onGetConfigErrFns *OnGetConfigErrFuncs, | ||
| httpSignerWrapper func(*http.Client) *http.Client, | ||
| hostIdentityCertPath string, |
There was a problem hiding this comment.
Nit. Consider switching to functional options at some point.
There was a problem hiding this comment.
Agree. Or maybe structs so that you know what you are setting caller side.
|
Merging as Victor will be OOO and we will continue iterating and fixing any bugs next week. |
2 participants
#30461
This PR contains the changes for the happy path.
On a separate PR we will be adding tests and further fixes for edge cases.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
runtime.GOOS).Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores