many: add target install system checks for passphrase support #14943

ZeyadYasser · 2025-01-17T12:47:43Z

This PR adds a simple snapd version check that gates passphrase authentication support in the install API based on the target install system.

For passphrase support:

snapd version must be at least 2.68 (assuming that fde-branch is merged as part of 2.68)
- This is checked through the snapd info file under usr/lib/snapd/info
snap-bootstrap version in kernel snap's initrd is from snapd 2.68+
- This is checked through the snapd info file under snapd-info

Please check SD201 for more details.

bboozzoo · 2025-01-23T10:58:40Z

overlord/devicestate/devicemgr.go

@@ -2201,6 +2201,29 @@ func (m *DeviceManager) systems() ([]*System, error) {
 	return systems, nil
 }

+func snapdVersionByTypeFromSeed20(s seed.Seed, types []snap.Type) (snapdVersionByType map[snap.Type]string, err error) {


The bare map that's passed around without additional context looks slightly weird. I think I'd return a structure here, e.g.

// in overlord/install/install.go type SystemSnapdVersions struct { // SnapdVersion is the version of snapd in a given system SnapdVersion string // SnapdInitramfsVersion is the version of snapd related component, which participates // in the boot process and performs unlocking. Typically snap-bootstrap in the kernel snap. SnapdInitramfsVersion string }

I thought of doing this, but loadSystemAndEssentialSnaps expects a list of essential snap types so changing its internals to implicitly add snapd and kernel types could have unintended consequences, also snapd is only expected on UC20+.

I am open to suggestions, just wanted to share my thought process.

my bad, this was intended for #14943 (comment), regarding the struct, totally agree, thanks for the suggestion.

bboozzoo · 2025-01-23T11:06:27Z

overlord/devicestate/handlers_install.go

+	// Find snapd versions for snapd and kernel snaps in the seed for
+	// the passphrase/PINs auth checks.
+	// FDE is only supported in UC20+ (i.e. Model grade is set).
+	if volumesAuthRequired && systemAndSeeds.Model.Grade() != asserts.ModelGradeUnset {


can we always attempt to get the info?

Suggested change

if volumesAuthRequired && systemAndSeeds.Model.Grade() != asserts.ModelGradeUnset {

if systemAndSeeds.Model.Grade() != asserts.ModelGradeUnset {

bboozzoo · 2025-01-23T11:08:59Z

overlord/devicestate/handlers_install.go

@@ -1278,7 +1295,19 @@ func (m *DeviceManager) doInstallSetupStorageEncryption(t *state.Task, _ *tomb.T
 		return fmt.Errorf("reading gadget information: %v", err)
 	}

-	encryptInfo, err := m.encryptionSupportInfo(systemAndSeeds.Model, secboot.TPMProvisionFull, systemAndSeeds.InfosByType[snap.TypeKernel], gadgetInfo)


actually would it make sense to add the snapd version structure to systemAndEssentialSnaps type and populate it in loadAndMountSystemLabelSnaps() ?

As @pedronis explained we don't expect the relevant code paths to only run for UC20+, I embedded the snapd version structure to systemAndEssentialSnaps. Thanks for the suggestion!

#14943 (comment)

codecov · 2025-01-24T13:18:34Z

Codecov Report

Attention: Patch coverage is 78.02198% with 20 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@35ad4ae). Learn more about missing BASE report.
Report is 191 commits behind head on master.

Files with missing lines	Patch %	Lines
overlord/install/install.go	74.28%	6 Missing and 3 partials ⚠️
overlord/devicestate/handlers_install.go	78.94%	4 Missing ⚠️
snap/info.go	75.00%	3 Missing and 1 partial ⚠️
overlord/devicestate/devicemgr.go	85.71%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             master   #14943   +/-   ##
=========================================
  Coverage          ?   78.15%           
=========================================
  Files             ?     1168           
  Lines             ?   157051           
  Branches          ?        0           
=========================================
  Hits              ?   122740           
  Misses            ?    26703           
  Partials          ?     7608

Flag	Coverage Δ
unittests	`78.15% <78.02%> (?)`

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

thanks, some comments

pedronis · 2025-01-28T19:10:37Z

overlord/devicestate/handlers_install.go

+	// Find snapd versions for snapd and kernel snaps in the seed for
+	// the passphrase/PINs auth checks.
+	// FDE is only supported in UC20+ (i.e. Model grade is set).
+	if systemAndSeeds.Model.Grade() != asserts.ModelGradeUnset {


I'm actually not sure we need the check at this point because we wouldn't have made this far if the system is not UC20+

though it would be interesting to double check what kind of errors /v2/systems/foo gives on UC18/classic system

Thank you! I didn't know that UC18 is not even installable through the systems API, and indeed snap prepare-image for a UC18 model doesn't even create the system-seed directory.

Removed restriction, Now the snapd versions structure is populated loadAndMountSystemLabelSnaps and embedded inside systemAndEssentialSnaps.

pedronis · 2025-01-28T19:10:51Z

overlord/devicestate/devicemgr.go

+	// Find snapd versions for snapd and kernel snaps in the seed for
+	// the passphrase/PINs auth checks.
+	// FDE is only supported in UC20+ (i.e. Model grade is set).
+	if systemAndSnaps.Model.Grade() != asserts.ModelGradeUnset {


pedronis · 2025-01-28T19:11:25Z

overlord/devicestate/devicemgr.go

@@ -2201,6 +2201,31 @@ func (m *DeviceManager) systems() ([]*System, error) {
 	return systems, nil
 }

+func snapdVersionByTypeFromSeed20(s seed.Seed, types []snap.Type) (systemSnapdVersions *install.SystemSnapdVersions, err error) {
+	perf := &timings.Timings{}
+	if err := s.LoadEssentialMeta(types, perf); err != nil {


can't we avoid this, by adding TypeSnapd to the system load calls in the couple of callers to this?

ZeyadYasser · 2025-01-29T11:18:31Z

Rebased on top of latest fde branch

pedronis

thank you

ZeyadYasser · 2025-01-29T14:16:58Z

Marking blocked until fde branch is merged into master

bboozzoo

LGTM

github-actions · 2025-01-29T14:48:25Z

Mon Feb 10 20:08:10 UTC 2025
The following results are from: https://github.com/canonical/snapd/actions/runs/13238489160

Failures:

Preparing:

openstack:debian-12-64:tests/smoke/
openstack:opensuse-tumbleweed-64
openstack:opensuse-tumbleweed-64
openstack:opensuse-tumbleweed-64
openstack:opensuse-tumbleweed-64
openstack:opensuse-tumbleweed-64
openstack:opensuse-tumbleweed-64
openstack:opensuse-15.6-64
openstack:opensuse-15.6-64
openstack:opensuse-15.6-64
openstack:opensuse-15.6-64
openstack:opensuse-15.6-64
openstack:opensuse-15.6-64

Executing:

google-nested:ubuntu-24.04-64:tests/nested/manual/remodel-min-size
google:ubuntu-25.04-64:tests/main/security-device-cgroups-required-or-optional
google:ubuntu-25.04-64:tests/main/security-device-cgroups-strict-enforced
google:ubuntu-25.04-64:tests/main/security-device-cgroups:uinput
google:ubuntu-25.04-64:tests/main/security-device-cgroups-serial-port
google:ubuntu-25.04-64:tests/main/security-device-cgroups:kmsg
google:ubuntu-25.04-64:tests/main/security-device-cgroups-self-manage
google:ubuntu-25.04-64:tests/main/security-device-cgroups-helper
google:ubuntu-25.04-64:tests/main/cgroup-devices-v2

Restoring:

openstack:debian-12-64:tests/smoke/
openstack:opensuse-tumbleweed-64
openstack:opensuse-tumbleweed-64
openstack:opensuse-tumbleweed-64
openstack:opensuse-tumbleweed-64
openstack:opensuse-tumbleweed-64
openstack:opensuse-tumbleweed-64
google:ubuntu-25.04-64:tests/main/security-device-cgroups-strict-enforced

alfonsosanchezbeato

LGTM, thanks

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

… versions Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

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 17, 2025

ZeyadYasser requested review from pedronis and alfonsosanchezbeato January 17, 2025 12:47

github-actions bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Jan 17, 2025

ZeyadYasser mentioned this pull request Jan 20, 2025

many: add passphrase authentication support v2 #14954

Merged

bboozzoo reviewed Jan 23, 2025

View reviewed changes

ZeyadYasser requested a review from bboozzoo January 24, 2025 13:01

ZeyadYasser added this to the 2.68 milestone Jan 28, 2025

pedronis reviewed Jan 28, 2025

View reviewed changes

ZeyadYasser requested a review from pedronis January 29, 2025 10:18

pedronis force-pushed the fde-manager-features branch from 7005c64 to 25f9119 Compare January 29, 2025 10:49

ZeyadYasser force-pushed the fde-check-install-target-system-passphrase-support branch from 1dac19e to a08ed37 Compare January 29, 2025 11:17

pedronis approved these changes Jan 29, 2025

View reviewed changes

ZeyadYasser added the ⛔ Blocked label Jan 29, 2025

bboozzoo approved these changes Jan 29, 2025

View reviewed changes

alfonsosanchezbeato approved these changes Feb 4, 2025

View reviewed changes

ZeyadYasser force-pushed the fde-check-install-target-system-passphrase-support branch from a08ed37 to 577ec57 Compare February 7, 2025 11:43

ZeyadYasser changed the base branch from fde-manager-features to master February 7, 2025 11:55

ZeyadYasser closed this Feb 7, 2025

ZeyadYasser reopened this Feb 7, 2025

ZeyadYasser added Skip spread Indicate that spread job should not run and removed ⛔ Blocked labels Feb 10, 2025

ZeyadYasser closed this Feb 10, 2025

ZeyadYasser reopened this Feb 10, 2025

ZeyadYasser removed the Skip spread Indicate that spread job should not run label Feb 10, 2025

ZeyadYasser force-pushed the fde-check-install-target-system-passphrase-support branch from 577ec57 to 310cede Compare February 10, 2025 07:33

ZeyadYasser added 3 commits February 10, 2025 12:09

many: add target install system checks for passphrase support

bf1b6d8

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

fixup! o/{install,devicestate}: use a struct instead of map for snapd…

0cc60ec

… versions Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

fixup! many: compute snapd versions in loadSystemAndEssentialSnaps

52d961d

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

ZeyadYasser force-pushed the fde-check-install-target-system-passphrase-support branch from 310cede to 52d961d Compare February 10, 2025 10:15

ernestl merged commit 84e9ddc into canonical:master Feb 11, 2025
63 of 67 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

many: add target install system checks for passphrase support #14943

many: add target install system checks for passphrase support #14943

ZeyadYasser commented Jan 17, 2025

bboozzoo Jan 23, 2025

ZeyadYasser Jan 24, 2025

ZeyadYasser Jan 24, 2025

bboozzoo Jan 23, 2025

bboozzoo Jan 23, 2025

ZeyadYasser Jan 29, 2025

ZeyadYasser Jan 29, 2025

codecov bot commented Jan 24, 2025 •

edited

Loading

pedronis left a comment

pedronis Jan 28, 2025

pedronis Jan 28, 2025

ZeyadYasser Jan 29, 2025 •

edited

Loading

ZeyadYasser Jan 29, 2025

pedronis Jan 28, 2025

pedronis Jan 28, 2025

ZeyadYasser Jan 29, 2025

ZeyadYasser commented Jan 29, 2025

pedronis left a comment

ZeyadYasser commented Jan 29, 2025

bboozzoo left a comment

github-actions bot commented Jan 29, 2025 •

edited

Loading

alfonsosanchezbeato left a comment

	if volumesAuthRequired && systemAndSeeds.Model.Grade() != asserts.ModelGradeUnset {
	if systemAndSeeds.Model.Grade() != asserts.ModelGradeUnset {

many: add target install system checks for passphrase support #14943

many: add target install system checks for passphrase support #14943

Conversation

ZeyadYasser commented Jan 17, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 24, 2025 • edited Loading

Codecov Report

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

ZeyadYasser Jan 29, 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

ZeyadYasser commented Jan 29, 2025

pedronis left a comment

Choose a reason for hiding this comment

ZeyadYasser commented Jan 29, 2025

bboozzoo left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 29, 2025 • edited Loading

Failures:

Preparing:

Executing:

Restoring:

alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 24, 2025 •

edited

Loading

ZeyadYasser Jan 29, 2025 •

edited

Loading

github-actions bot commented Jan 29, 2025 •

edited

Loading