Added integration test for TPM. - #31315
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 📝 WalkthroughWalkthroughThe TPM 2.0 Secure Hardware (SecureHW) implementation for Linux was refactored. The old implementation was removed and replaced with a new file that provides TPM key management and signing. Tests for TPM key file loading were added, and an integration test using a TPM simulator was introduced. Some dependencies were updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Runner
participant TPM as TPM Simulator
participant SecureHW as tpm2SecureHW
participant Key as tpm2Key
participant SCEP as SCEP Client
participant Server as Enrollment Server
Test->>TPM: Open simulator
Test->>SecureHW: Create SecureHW with TPM
SecureHW->>TPM: Create parent key
SecureHW->>TPM: Create ECC key
SecureHW->>Key: Return tpm2Key
Test->>Key: Get public key
Test->>SCEP: Create client with key and enrollment secret
SCEP->>Server: Fetch certificate
Server-->>SCEP: Return certificate
SCEP-->>Test: Certificate and public key
Test->>Key: Create HTTP signer
Test->>Server: Send signed enrollment request
Server-->>Test: Return node key
Test->>Key: Close
Test->>SecureHW: Load key from file
SecureHW->>TPM: Load parent key
SecureHW->>TPM: Load ECC key
SecureHW->>Key: Return loaded tpm2Key
Test->>Key: Create HTTP signer
Test->>Server: Send signed config request
Server-->>Test: Return config
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
ee/orbit/pkg/securehw/securehw_linux.go (1)
16-42: Fix the no-op logger issue.The code creates a no-op logger on line 39, which disables all logging from the TPM implementation. This appears to be a bug.
return &tpm2SecureHW{ device: device, - logger: zerolog.Nop(), + logger: logger.With().Str("component", "securehw-tpm").Logger(), keyFilePath: filepath.Join(metadataDir, "host_identity_tpm.pem"), }, nil
🧹 Nitpick comments (2)
ee/orbit/pkg/securehw/securehw_test.go (1)
1-2: Consider documenting why Windows is excluded.The build constraint excludes Windows, but it would be helpful to add a comment explaining why these tests don't run on Windows.
//go:build !windows +// Tests use TPM simulator which may have platform-specific behavioree/orbit/pkg/securehw/securehw_tpm.go (1)
279-279: Consider adding a comment about deterministic parent keys.While the comment on line 278 mentions this, it might be helpful to reference the
createParentKeymethod documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
ee/orbit/pkg/securehw/securehw_linux.go(2 hunks)ee/orbit/pkg/securehw/securehw_stub.go(0 hunks)ee/orbit/pkg/securehw/securehw_test.go(1 hunks)ee/orbit/pkg/securehw/securehw_tpm.go(1 hunks)ee/server/integrationtest/hostidentity/hostidentity_test.go(5 hunks)go.mod(2 hunks)
💤 Files with no reviewable changes (1)
- ee/orbit/pkg/securehw/securehw_stub.go
🧰 Additional context used
🧠 Learnings (3)
ee/orbit/pkg/securehw/securehw_tpm.go (2)
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: #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/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: #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: #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: #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: #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.
⏰ 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). (20)
- GitHub Check: test-go (vuln, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (mysql, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (vuln, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (main, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (mysql, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (integration-mdm, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (main, 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:8.0.36, false)
- GitHub Check: test-go (fleetctl, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (fleetctl, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (integration-core, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (integration-core, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (integration-enterprise, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: lint (windows-latest)
- GitHub Check: lint (ubuntu-latest)
- GitHub Check: lint (macos-latest)
- GitHub Check: build-binaries
- GitHub Check: Analyze (go)
- GitHub Check: publish
🔇 Additional comments (12)
go.mod (2)
251-251: Good addition for TPM support.The
github.com/google/go-tpm-toolsdependency provides TPM key file handling functionality that's used in the new TPM implementation.
335-335: Minor version update looks good.The update of
go.uber.org/multierrfrom v1.10.0 to v1.11.0 is a minor version bump that should be backward compatible.ee/orbit/pkg/securehw/securehw_test.go (1)
15-117: Excellent test coverage for TPM key file loading.The test cases comprehensively cover the different failure scenarios for loading TPM key files. Good use of table-driven tests would make this even cleaner, but the current structure is clear and effective.
ee/server/integrationtest/hostidentity/hostidentity_test.go (3)
15-32: Good organization of new imports.The new imports for TPM simulator, orbit SCEP client, and secure hardware support are properly organized and necessary for the integration tests.
211-217: Smart refactoring to use the wrapper.Using
fleethttpsig.Signerwrapper instead of directhttpsig.NewSignerprovides better abstraction and consistency across the codebase.
989-1165: Comprehensive integration test for TPM functionality.This test thoroughly exercises the TPM-based secure hardware implementation:
- Creates and uses TPM keys
- Tests SCEP certificate enrollment
- Verifies HTTP signature generation
- Tests key persistence and reloading
The test provides excellent coverage of the end-to-end flow.
ee/orbit/pkg/securehw/securehw_tpm.go (6)
29-51: Good design for test helper function.The
NewTestSecureHWfunction provides a clean way to inject a TPM simulator for testing. The validation and logging are appropriate.
113-119: Proper error handling with resource cleanup.Good use of deferred cleanup function to ensure TPM resources are freed on error paths.
138-194: Smart implementation of deterministic parent key.Creating the parent key deterministically using
CreatePrimaryensures consistent key hierarchies across load operations. This is a clever approach that avoids the need to persist the parent key.
196-239: Practical curve detection method.Testing P-384 support by attempting to create a key is more reliable than querying capabilities. Good fallback to P-256 when P-384 isn't supported.
248-248: Secure file permissions.Using 0600 permissions ensures only the owner can read the key file, which is appropriate for sensitive key material.
499-523: Correct implementation of RFC 9421 signature format.The fixed-width r||s format for HTTP signatures is properly implemented with appropriate padding. Good error checking for oversized components.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #31315 +/- ##
==========================================
+ Coverage 64.01% 64.16% +0.15%
==========================================
Files 1913 1914 +1
Lines 188293 188305 +12
Branches 5629 5629
==========================================
+ Hits 120534 120825 +291
+ Misses 58311 57998 -313
- Partials 9448 9482 +34
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:
|
This reverts commit 7b340b4.
2 participants
For #31048
This change includes some refactoring of orbit code. No functional changes. Moved non-Linux-specific code from
securehw_linux.gotosecurehw_tpm.goso that tests on any platform can use it.There are no server changes impacting the upcoming 4.72 release. Just tests.
Checklist for submitter
Testing
fleetd/orbit/Fleet Desktop
runtime.GOOSis used as needed to isolate changesSummary by CodeRabbit
New Features
Bug Fixes
Tests
Chores