many: add passphrase authentication support #14918

ZeyadYasser · 2025-01-10T17:53:01Z

This PR implements passphrase authentication support during installation. Currently all volumes created during installation are passphrase encrypted, i.e. ubuntu-data and ubuntu-save.

The implications of having ubuntu-save optionally passphrase-protected is that a factory reset would require a passphrase to be entered to work. This is still an open question on what is optimal from a UX perspective.

Note: This is still not exposed through the API until target system and entropy checks are added. This is done by not including passphrase-auth in the list of supported encryption features (Check SD201).

I tested the implementation against the spread test from #14867.

This is still not exposed through the API until target system and entropy checks are added. Signed-off-by: Zeyad Gouda <[email protected]>

pedronis

question and small comment

pedronis · 2025-01-15T12:44:05Z

overlord/devicestate/handlers_install.go

@@ -1356,7 +1357,7 @@ func createSaveBootstrappedContainer(saveNode string) (secboot.BootstrappedConta
 		return nil, err
 	}

-	return secbootCreateBootstrappedContainer(secboot.DiskUnlockKey(saveEncryptionKey), saveNode), nil
+	return secbootCreateBootstrappedContainer(secboot.DiskUnlockKey(saveEncryptionKey), saveNode, nil), nil


shouldn't we take the value from the cache passed it in? is the test currently only dealing with mocked stuff?

createSaveBootstrappedContainer is only called during factory reset

snapd/overlord/devicestate/handlers_install.go

Line 629 in 39dc92b

saveBoostrapContainer, err := createSaveBootstrappedContainer(saveNode)

The value is passed from cache in EncryptPartitions() in the install code path.

can you point to the relevant code for EncryptPartitions?

Ahh yes, my bad

snapd/overlord/devicestate/handlers_install.go

Line 1297 in 39dc92b

encryptionSetupData, err := installEncryptPartitions(onVolumes, volumesAuth, encType, systemAndSeeds.Model, mntPtForType[snap.TypeGadget], mntPtForType[snap.TypeKernel], perfTimings)

I was confused because I missed this change:
https://github.com/canonical/snapd/pull/14918/files#diff-f796a3322af4dad794f8018bb1c4de9e30562adc6e9a5a43e30595e157a138a2R722

pedronis · 2025-01-15T12:44:38Z

secboot/secboot_tpm.go

+	return nil
+}
+
+func kdfOptions(volumesAuth device.VolumesAuthOptions) (sb.KDFOptions, error) {


this probably should take a pointer as well to device.VolumesAuthOptions)

updated, thanks!

Signed-off-by: Zeyad Gouda <[email protected]>

codecov · 2025-01-15T14:05:42Z

Codecov Report

Attention: Patch coverage is 93.97590% with 5 lines in your changes missing coverage. Please review.

Please upload report for BASE (fde-manager-features@5ec2e43). Learn more about missing BASE report.
Report is 8 commits behind head on fde-manager-features.

Files with missing lines	Patch %	Lines
secboot/secboot_tpm.go	95.65%	2 Missing and 1 partial ⚠️
secboot/bootstrap_container.go	66.66%	1 Missing ⚠️
secboot/bootstrap_container_sb.go	80.00%	1 Missing ⚠️

Additional details and impacted files

@@                   Coverage Diff                   @@
##             fde-manager-features   #14918   +/-   ##
=======================================================
  Coverage                        ?   78.16%           
=======================================================
  Files                           ?     1166           
  Lines                           ?   154990           
  Branches                        ?        0           
=======================================================
  Hits                            ?   121149           
  Misses                          ?    26330           
  Partials                        ?     7511

Flag	Coverage Δ
unittests	`78.16% <93.97%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pedronis

thank you

valentindavid

This looks fine apart from putting the volume auth options into the bootstrapped container. It should be one set options for the whole system. In the extreme, it could make sense to have different options for every slot. Or a middle ground for each key role. But not per volume, it does not make sense. The run save key and the run data key should have the same configuration. The recover boot key and the recover save key should have the same configuration.

Not related totally related to this PR, but I think the idea that installation is what sets a passphrase is wrong for user experience. It should be the first boot that does it.

Install should be about hardware (including keyboard mapping), software and default locale. Not user configuration. The first boot is where anything from the user can be configured. That means a passphrase should be set during first boot. There is nothing to protect anyway before the user has stored any personal configuration.
If you do not split the install from the first boot. You cannot possibly have factory reset.
Then you also need also the code to work both for installation context and changes. Which is twice the amount of bugs.
You cannot have a (real) OEM installation.
It is a weird thing that some Linux distributions still mix up those 2 phases. Other operating systems do it correctly.

bboozzoo · 2025-01-20T10:28:19Z

secboot/secboot_tpm.go

@@ -490,15 +572,6 @@ func SealKeys(keys []SealKeyRequest, params *SealKeysParams) ([]byte, error) {
 		return nil, fmt.Errorf("at least one set of model-specific parameters is required")
 	}

-	tpm, err := sbConnectToDefaultTPM()


would it be a problem if the withTPMConnection() wrapper was called here?

bboozzoo · 2025-01-20T10:29:04Z

secboot/secboot_tpm.go

@@ -481,6 +483,86 @@ func ProvisionForCVM(initramfsUbuntuSeedDir string) error {
 	return nil
 }

+func withTPMConnection(fn func(tpm *sb_tpm2.Connection)) error {


Suggested change

func withTPMConnection(fn func(tpm *sb_tpm2.Connection)) error {

func withSingleTPMConnection(fn func(tpm *sb_tpm2.Connection)) error {

bboozzoo · 2025-01-20T10:30:43Z

secboot/secboot_tpm.go

@@ -481,6 +483,86 @@ func ProvisionForCVM(initramfsUbuntuSeedDir string) error {
 	return nil
 }

+func withTPMConnection(fn func(tpm *sb_tpm2.Connection)) error {


the function likely needs a comment explaining how the global state is changed

ZeyadYasser · 2025-01-20T20:32:33Z

@pedronis @valentindavid @bboozzoo I opened #14954 in favor of this PR to implement suggestions from #14918 (review), changes were big enough to warrant a separate PR to avoid confusing diffs.

ZeyadYasser · 2025-01-20T20:36:21Z

Closed in favor of #14954

ZeyadYasser added Needs Samuele review Needs a review from Samuele before it can land Run nested The PR also runs tests inluded in nested suite FDE Manager Pull requests that target FDE manager branch labels Jan 10, 2025

ZeyadYasser requested review from bboozzoo, valentindavid and pedronis January 10, 2025 17:53

pedronis force-pushed the fde-manager-features branch from 5802840 to 5ec2e43 Compare January 13, 2025 09:50

many: add passphrase authentication support

7da02d3

This is still not exposed through the API until target system and entropy checks are added. Signed-off-by: Zeyad Gouda <[email protected]>

ZeyadYasser force-pushed the fde-passphrase-support branch from 8b6b37c to 7da02d3 Compare January 13, 2025 12:04

pedronis reviewed Jan 15, 2025

View reviewed changes

fixup! secboot: pass pointer to kdfOptions

c77e269

Signed-off-by: Zeyad Gouda <[email protected]>

ZeyadYasser requested a review from pedronis January 15, 2025 13:44

pedronis approved these changes Jan 16, 2025

View reviewed changes

valentindavid reviewed Jan 17, 2025

View reviewed changes

bboozzoo reviewed Jan 20, 2025

View reviewed changes

ZeyadYasser mentioned this pull request Jan 20, 2025

many: add passphrase authentication support v2 #14954

Open

ZeyadYasser closed this Jan 20, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

many: add passphrase authentication support #14918

many: add passphrase authentication support #14918

ZeyadYasser commented Jan 10, 2025

pedronis left a comment

pedronis Jan 15, 2025

ZeyadYasser Jan 15, 2025

pedronis Jan 15, 2025 •

edited

Loading

ZeyadYasser Jan 15, 2025

pedronis Jan 16, 2025

pedronis Jan 15, 2025

ZeyadYasser Jan 15, 2025

codecov bot commented Jan 15, 2025 •

edited

Loading

pedronis left a comment

valentindavid left a comment

bboozzoo Jan 20, 2025

bboozzoo Jan 20, 2025

bboozzoo Jan 20, 2025

ZeyadYasser commented Jan 20, 2025

ZeyadYasser commented Jan 20, 2025

	func withTPMConnection(fn func(tpm *sb_tpm2.Connection)) error {
	func withSingleTPMConnection(fn func(tpm *sb_tpm2.Connection)) error {

many: add passphrase authentication support #14918

many: add passphrase authentication support #14918

Conversation

ZeyadYasser commented Jan 10, 2025

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 15, 2025 • edited Loading

Codecov Report

pedronis left a comment

Choose a reason for hiding this comment

valentindavid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZeyadYasser commented Jan 20, 2025

ZeyadYasser commented Jan 20, 2025

pedronis Jan 15, 2025 •

edited

Loading

codecov bot commented Jan 15, 2025 •

edited

Loading