Add permanent errors to Fleet Desktop for TPM-backed httpsig work - #31220
Conversation
📝 WalkthroughWalkthroughThis update adds logic to handle missing or mismatched host identity certificates and secure hardware keys. It introduces a permanent error display in Fleet Desktop when certificate setup fails, updates environment variable names in documentation and scripts, and ensures stale certificate files are removed if a secure hardware key is not found. Changes
Sequence Diagram(s)sequenceDiagram
participant OrbitAgent
participant HostIdentity
participant FleetDesktop
participant User
OrbitAgent->>HostIdentity: Setup TPM-backed certificate
alt Key or certificate error
HostIdentity-->>OrbitAgent: Error (e.g., TPM unavailable, key missing)
OrbitAgent->>FleetDesktop: Launch with FLEET_DESKTOP_PERMANENT_ERROR
FleetDesktop->>User: Display permanent error message in tray
else Success
HostIdentity-->>OrbitAgent: Certificate ready
OrbitAgent->>User: Normal operation
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related issues
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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 (
|
| // First let's clear any existing certificate in | ||
| // case a user or process deleted the keyfile but not | ||
| // the issued-via-SCEP certificate. | ||
| certPath := filepath.Join(metadataDir, constant.FleetHTTPSignatureCertificateFileName) | ||
| if err := os.RemoveAll(certPath); err != nil { | ||
| return nil, fmt.Errorf("failed to clear the host identity certificate: %w", err) | ||
| } |
There was a problem hiding this comment.
This is not related to the Fleet Desktop change, I added this for the following scenario described in the main story:
- Delete one or more certificate files host_identity* in the host, restart fleetd, and make sure fleetd is able to get a new cert and talk to Fleet.
| @@ -1 +1 @@ | |||
| * Added support to generate a TPM 2.0 private key and issue a SCEP certificate for signing of HTTP requests (via new environment variable `ORBIT_FLEET_MANAGED_CLIENT_CERTIFICATE`). | |||
| - Added support to generate a TPM 2.0 private key and issue a SCEP certificate for signing of HTTP requests (via new environment variable `ORBIT_FLEET_MANAGED_HOST_IDENTITY_CERTIFICATE`). | |||
There was a problem hiding this comment.
I missed this rename on the previous rename PR.
There was a problem hiding this comment.
I missed these renames on the previous rename PR.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #31220 +/- ##
==========================================
- Coverage 64.03% 64.00% -0.03%
==========================================
Files 1908 1908
Lines 187750 187799 +49
Branches 5336 5336
==========================================
- Hits 120224 120208 -16
- Misses 58079 58139 +60
- Partials 9447 9452 +5
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 review |
✅ Actions performedReview 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
🧹 Nitpick comments (2)
ee/orbit/pkg/hostidentity/host_identity.go (1)
61-67: Consider usingos.Removeinstead ofos.RemoveAllfor better safety.Since you're removing a single certificate file,
os.Removewould be more appropriate and safer thanos.RemoveAll. TheRemoveAllfunction is designed for removing directories and their contents, which could potentially remove more than intended if the path unexpectedly becomes a directory.- if err := os.RemoveAll(certPath); err != nil { + if err := os.Remove(certPath); err != nil && !os.IsNotExist(err) {Note: I also added a check for
os.IsNotExist(err)since it's fine if the certificate file doesn't exist - we just want to ensure it's not there.orbit/cmd/orbit/orbit.go (1)
981-992: Good error handling approach with clear user messagingThis error handling logic properly provides user-friendly error messages through Fleet Desktop when TPM certificate setup fails. The approach of checking the fleet-desktop flag before showing UI errors and providing different messages based on error type is well thought out.
There's a small typo on line 2321: "permantent" should be "permanent".
- log.Debug().Msg("opening Fleet Desktop with permantent error") + log.Debug().Msg("opening Fleet Desktop with permanent error")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ee/orbit/pkg/hostidentity/host_identity.go(1 hunks)orbit/changes/fleetd-tpm-key(1 hunks)orbit/cmd/desktop/desktop.go(3 hunks)orbit/cmd/orbit/orbit.go(2 hunks)tools/tuf/test/gen_pkgs.sh(5 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 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/hostidentity/host_identity.go (3)
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.
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.
tools/tuf/test/gen_pkgs.sh (1)
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.
orbit/cmd/orbit/orbit.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.
🧬 Code Graph Analysis (1)
ee/orbit/pkg/hostidentity/host_identity.go (1)
orbit/pkg/constant/constant.go (1)
FleetHTTPSignatureCertificateFileName(79-79)
🔇 Additional comments (6)
orbit/changes/fleetd-tpm-key (1)
1-1: Good correction to align the documentation with the actual variable name.This change properly updates the environment variable reference to match the consistent naming used throughout the codebase for TPM-backed host identity certificates.
tools/tuf/test/gen_pkgs.sh (2)
30-30: Good documentation update to reflect the corrected variable name.The comment now accurately describes the
FLEET_MANAGED_HOST_IDENTITY_CERTIFICATEenvironment variable and correctly notes it's Linux-only.
85-85: Consistent variable name updates across all Linux package types.All references to the environment variable have been properly updated from
FLEET_MANAGED_CLIENT_CERTIFICATEtoFLEET_MANAGED_HOST_IDENTITY_CERTIFICATEacross DEB and RPM package generation commands. The MSI sections correctly omit this variable since it's Linux-only.Also applies to: 110-110, 135-135, 160-160
orbit/cmd/desktop/desktop.go (2)
90-92: Excellent early error handling to prevent normal startup flow.This check ensures that when a permanent error is detected, the application immediately switches to error display mode rather than attempting normal initialization, which is exactly the right approach for critical TPM/certificate failures.
798-824: Permanent error display implementation approvedWe’ve confirmed that the project’s go.mod specifies Go 1.24.5 and that
strings.SplitSeqis available (added in Go 1.23). No compatibility issues—no changes needed.orbit/cmd/orbit/orbit.go (1)
2300-2343: Well-designed function for displaying permanent errorsThis function handles the Fleet Desktop error display flow nicely:
- Properly checks for GUI user availability before attempting to launch
- Kills existing instances to prevent conflicts
- Uses environment variables to pass error messages cleanly
- Runs as the correct user with proper permissions
- Provides adequate time (5 minutes) for users to see the error
The implementation follows established patterns from the existing
desktopRunner.Execute()method and handles errors appropriately with good context.
sgress454
left a comment
sgress454
left a comment
There was a problem hiding this comment.
LGTM. Tested that the "launch desktop w/ permanent error" feature works. Only thing I noticed was that when launched w/ permanent error, quitting Orbit doesn't cause the desktop to immediately quit. I don't think this is an issue since when Orbit starts, it explicitly quits any desktop instances it finds. Just pointing it out.
Did you test this on macOS? On Linux as a fleetd package it seems to work (killing orbit kills child processes). IIRC macOS is different due to the use of |
2 participants
For #30478.
Figma: https://www.figma.com/design/qBsJ8Qpz0ZSCASbLBYL59v/-28818-Verify-identity-of-Linux-hosts-when-talking-to-Fleet--ala-Apple-MDM-?node-id=5301-90&t=t9Kuq7QUXOJkhaff-1
When the host doesn't have a TPM 2.0 device:
When the host was installed with an invalid enroll secret (which means it cannot generate a certificate):
Summary by CodeRabbit
New Features
Bug Fixes
Documentation