-
Notifications
You must be signed in to change notification settings - Fork 15
Udevadm settle and kcrypt configuration on cmdline #988
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
jimmykarily
wants to merge
17
commits into
main
Choose a base branch
from
udevadm-settle
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+2,518
−257
Open
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
c316269
Try to run udevadm settle before encrypting the disks
jimmykarily 129ab9a
Add more debug logs in the sdk
jimmykarily b37cb54
Print errors and bump the sdk to unmount the partition to be encrypted
jimmykarily 5530f0a
Extract kcrypt config to the cmdline
jimmykarily 470f15f
Set grubenv value correctly and umount partitions before decryption
jimmykarily c4c3cc6
Copy the cloud config file after "luksification"
jimmykarily b2437c0
Skip grup options hook in uki mode
jimmykarily 993ebd5
[TMP] Add refactoring plan docs
jimmykarily efe710c
Create common Encrypt method with structure for both uki and non-uki
jimmykarily a18ea68
Simplify the top level Encrypt method
jimmykarily cd6c033
Move Encrypt method and helpers to a separate file
jimmykarily f56c916
Don't call redundant `sync` and use logger
jimmykarily 21a1535
Fix order of mountint/umounting/locking/unlocking/etc
jimmykarily 68637f2
Fix compilation issues and suffix issues
jimmykarily 44fc2a8
Don't prefix cmdline options with `kairos.`
jimmykarily c087323
Use pushed branches instead of local replacements
jimmykarily d90e135
Move copying of cloud config where it was
jimmykarily File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,148 @@ | ||
| # Partition Encryption Refactoring - Summary | ||
|
|
||
| ## ✅ REFACTORING STATUS (Updated: 2025-10-24) | ||
|
|
||
| ### Implementation Complete - Interface-Based Approach ✅ | ||
|
|
||
| We implemented a cleaner architecture using the **Strategy Pattern** with a `PartitionEncryptor` interface. | ||
|
|
||
| #### Completed Work: | ||
|
|
||
| **1. kairos-sdk/kcrypt/encryptor.go (NEW FILE)** | ||
| - `PartitionEncryptor` interface with `Encrypt()`, `Unlock()`, `Name()`, `Validate()` methods | ||
| - Three implementations: `RemoteKMSEncryptor`, `TPMWithPCREncryptor`, `LocalTPMNVEncryptor` | ||
| - `GetEncryptor(logger)` factory with automatic config scanning and validation | ||
|
|
||
| **2. kairos-sdk/kcrypt/tpm_passphrase.go (NEW FILE)** | ||
| - Local TPM NV passphrase management (moved from kcrypt-challenger) | ||
|
|
||
| **3. kairos-sdk/kcrypt/lock.go + unlock.go (MODIFIED)** | ||
| - New encryption functions for local TPM NV passphrase | ||
| - **SECURITY FIX** ✅: Removed passphrase logging (only log length) | ||
|
|
||
| **4. kairos-agent/internal/agent/hooks/encrypt.go (NEW - ~350 LINES)** | ||
| - Unified `Encrypt()` method for both UKI and non-UKI modes | ||
| - Helper methods: `determinePartitionsToEncrypt()`, `preparePartitionsForEncryption()`, | ||
| `backupOEMIfNeeded()`, `restoreOEMIfNeeded()`, `copyCloudConfigToOEM()`, `udevAdmSettle()` | ||
| - **BUG FIX** ✅: `GetEncryptor` called BEFORE unmounting OEM (so it can read kcrypt config) | ||
| - **BUG FIX** ✅: `restoreOEMIfNeeded` uses dmsetup to find mapper device + udev settle | ||
| - Legacy methods: `EncryptNonUKI()`, `EncryptUKI()` (backward compatibility) | ||
|
|
||
| **5. kairos-agent/internal/agent/hooks/finish.go (SIMPLIFIED - 51 LINES)** | ||
| - Clean orchestration, minimal imports | ||
|
|
||
| **6. kairos-agent/internal/agent/hooks/hook.go (MODIFIED)** | ||
| - **BUG FIX** ✅: `lockPartitions()` uses `dmsetup ls` to properly close mapper devices | ||
|
|
||
| #### Critical Bug Fixes (2025-10-24): | ||
|
|
||
| **Issue 1: Challenger Config Ignored** ✅ | ||
| - Root cause: `GetEncryptor` called after OEM unmounted | ||
| - Fix: Call `GetEncryptor` at step 1.5 (before unmounting) | ||
| - Result: Challenger server now properly detected and used | ||
|
|
||
| **Issue 2: OEM Restore Failed** ✅ | ||
| - Root cause: `blkid -L` returned LUKS container, not mapper device; device node not ready | ||
| - Fix: Use `dmsetup ls` to verify mapper exists + `udevAdmSettle()` to wait for device node | ||
| - Result: OEM successfully restored after encryption | ||
|
|
||
| **Issue 3: Passphrase Logging** ✅ | ||
| - Root cause: Passphrases logged in plaintext | ||
| - Fix: Removed from all log messages, only log length | ||
| - Result: No sensitive data in logs | ||
|
|
||
| **Issue 4: Partition Locking Failed** ✅ | ||
| - Root cause: Tried to close by label path instead of mapper name | ||
| - Fix: Use `dmsetup ls --target crypt` to find active mappers | ||
| - Result: All encrypted devices properly closed | ||
|
|
||
| #### Decision Logic: | ||
| 1. If `challenger_server` or `mdns` → **Remote KMS** (both UKI & non-UKI) | ||
| 2. Else if UKI mode → **TPM + PCR policy** | ||
| 3. Else → **Local TPM NV passphrase** | ||
|
|
||
| #### Production Status: | ||
| - ✅ All code compiles successfully | ||
| - ✅ Challenger-based encryption tested and working | ||
| - ✅ OEM backup/restore working | ||
| - ✅ Encrypted devices properly locked | ||
| - ✅ No security issues | ||
| - ⚠️ Remove `replace` directive from go.mod before production merge | ||
|
|
||
| ### Remaining Work: | ||
| - ⏳ **kcrypt-challenger cleanup**: Remove local TPM NV logic (now in kairos-sdk) | ||
| - ⏳ **Testing**: Full end-to-end testing of all three encryption methods | ||
| - ⏳ **Cloud-init path bug**: Fix mkdir on file paths in `pkg/utils/runstage.go:83` | ||
| - ⏳ **immucore integration** (optional) | ||
|
|
||
| --- | ||
|
|
||
| ## Proposed Architecture (ORIGINAL PLAN) | ||
|
|
||
| ### Component Responsibilities | ||
|
|
||
| - **kairos-agent**: Orchestrates encryption with unified code path for UKI and non-UKI modes. Decides passphrase source based on config (remote KMS configured → use plugin, otherwise local). Handles common operations: unmount partitions, backup/restore OEM, unlock/wait for encrypted devices. | ||
|
|
||
| - **kairos-sdk**: Provides single `Encrypt()` function that accepts passphrase source parameter (remote/local-TPM/ephemeral). Handles all LUKS creation, PCR policy enrollment, and partition formatting. Contains local TPM NV passphrase storage/retrieval logic. | ||
|
|
||
| - **kcrypt-challenger**: Focused solely on remote KMS operations - implements TPM attestation protocol to retrieve passphrases from remote server. No local encryption logic. | ||
|
|
||
| ## Unified Encryption Flow (kairos-agent) | ||
|
|
||
| ```go | ||
| func encryptPartitions(config Config, isUKI bool) error { | ||
| // 1. Common preparation (extracted methods) | ||
| partitions := determinePartitionsToEncrypt(config, isUKI) | ||
| preparePartitionsForEncryption(partitions) // unmount all | ||
| oemBackup := backupOEMIfNeeded(partitions) // backup OEM data | ||
| defer restoreOEMIfNeeded(oemBackup) | ||
|
|
||
| // 2. For each partition | ||
| for _, partition := range partitions { | ||
| // 2a. Determine passphrase source | ||
| source := determinePassphraseSource(config, isUKI) | ||
| // Options: "remote" (KMS), "local_tpm" (NV memory), "ephemeral" (random) | ||
|
|
||
| // 2b. Get passphrase when needed | ||
| passphrase := getPassphrase(partition, config, source) | ||
|
|
||
| // 2c. Encrypt with unified logic (kairos-sdk) | ||
| kcrypt.Encrypt( | ||
| partition, | ||
| passphrase, | ||
| pcrBinding: isUKI ? config.BindPCRs : nil, | ||
| keepPasswordSlot: source != "ephemeral", | ||
| ) | ||
| // This decides: | ||
| // - Creates LUKS with passphrase | ||
| // - If pcrBinding: adds TPM PCR policy keyslot | ||
| // - If !keepPasswordSlot: wipes password keyslot (TPM-only unlock) | ||
| } | ||
|
|
||
| // 3. Common cleanup (extracted methods) | ||
| unlockEncryptedPartitions(partitions) | ||
| waitForUnlockedPartitions(partitions) | ||
| lockPartitions(partitions) | ||
|
|
||
| return nil | ||
| } | ||
| ``` | ||
|
|
||
| **Key Points:** | ||
| - Passphrase retrieval is a separate step, only called when needed | ||
| - Encryption logic (kairos-sdk) decides whether to keep password keyslot based on source | ||
| - Common operations extracted into reusable methods | ||
| - Same flow for UKI and non-UKI, only parameters differ | ||
|
|
||
| ## Key Changes by Repository | ||
|
|
||
| - **kairos-agent**: Merge `Encrypt()` and `EncryptUKI()` into unified flow, extract common operations (unmount, backup OEM, unlock, restore), simple decision logic for passphrase source | ||
| - **kairos-sdk**: Add unified `Encrypt()` function with passphrase source parameter, move local TPM NV logic from kcrypt-challenger, consolidate all LUKS/PCR operations | ||
| - **kcrypt-challenger**: Remove local TPM NV logic, remain as remote KMS client only (no changes to remote attestation protocol) | ||
|
|
||
| ## Benefits | ||
|
|
||
| ✅ UKI gains remote KMS support | ||
| ✅ Single code path eliminates duplication | ||
| ✅ Clear separation: remote=plugin, local=sdk | ||
| ✅ Backwards compatible |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are temporary, used to put the AI into context every time. I will remove them before merge.