Skip to content
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

many: add passphrase authentication support v2 #14954

Open
wants to merge 7 commits into
base: fde-manager-features
Choose a base branch
from

Conversation

ZeyadYasser
Copy link
Contributor

@ZeyadYasser ZeyadYasser commented Jan 20, 2025

This PR is a re-implementation of #14918 with suggestion from @valentindavid #14918 (review) of not having volumes authentication options attached to the bootstrapped containers but rather passed along with the rest of other FDE params. A followup PR could be created to tidy up the passed FDE parameters (e.g. data/save bootstrap containers, primary key, volume auth options).

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

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

The TPM connection workaround can be removed once canonical/secboot#360 lands.

@ZeyadYasser 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 20, 2025
@ZeyadYasser ZeyadYasser changed the title many: add passphrase authentication support many: add passphrase authentication support v2 Jan 20, 2025
Copy link

codecov bot commented Jan 20, 2025

Codecov Report

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

Please upload report for BASE (fde-manager-features@640541f). Learn more about missing BASE report.

Files with missing lines Patch % Lines
secboot/secboot_tpm.go 83.33% 10 Missing and 5 partials ⚠️
boot/seal.go 66.66% 2 Missing ⚠️
overlord/devicestate/handlers_install.go 50.00% 0 Missing and 2 partials ⚠️
cmd/snap-bootstrap/cmd_initramfs_mounts.go 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@                   Coverage Diff                   @@
##             fde-manager-features   #14954   +/-   ##
=======================================================
  Coverage                        ?   78.15%           
=======================================================
  Files                           ?     1167           
  Lines                           ?   155089           
  Branches                        ?        0           
=======================================================
  Hits                            ?   121208           
  Misses                          ?    26368           
  Partials                        ?     7513           
Flag Coverage Δ
unittests 78.15% <83.60%> (?)

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.

boot/seal.go Outdated Show resolved Hide resolved
boot/seal.go Outdated Show resolved Hide resolved
boot/seal.go Outdated Show resolved Hide resolved
@ZeyadYasser ZeyadYasser requested a review from bboozzoo January 22, 2025 19:07
Copy link
Contributor

@bboozzoo bboozzoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you

@pedronis
Copy link
Collaborator

we might want to get the branch in shape for merging before considering landing this

@ZeyadYasser
Copy link
Contributor Author

@bboozzoo I had to revert the suggestion from #14918 (comment) because internally secboot actually closes the passed TPM connection, and since keys for both data and save partitions are created we need a new TPM connection for each. My bad for not remembering this when addressing the comment.

@ZeyadYasser ZeyadYasser added this to the 2.68 milestone Jan 28, 2025
@pedronis pedronis force-pushed the fde-manager-features branch from 7005c64 to 25f9119 Compare January 29, 2025 10:49
Signed-off-by: Zeyad Gouda <[email protected]>
… context

Internally the secboot library closes the passed TPM connection, until
this is fixed, a separate connection per newTPMProtectedKey call fixes
this issue.

Signed-off-by: Zeyad Gouda <[email protected]>
@ZeyadYasser ZeyadYasser force-pushed the fde-passphrase-support-v2 branch from a9c0ef2 to 23a6d7c Compare January 29, 2025 11:46
@ZeyadYasser
Copy link
Contributor Author

Rebased on top of latest fde branch, and updated secboot to include the double TPM connection fix

@ZeyadYasser
Copy link
Contributor Author

Removed blocked label since now fde branch is in a more stable state

@ZeyadYasser
Copy link
Contributor Author

added blocked label again, as agreement was to merge into master directly

Copy link

Wed Jan 29 15:48:26 UTC 2025
The following results are from: https://github.com/canonical/snapd/actions/runs/13030627188

Failures:

Preparing:

  • google-nested:ubuntu-20.04-64:tests/nested/manual/remodel-uc20-to-uc22:encrypted
  • google-nested:ubuntu-22.04-64:tests/nested/classic/snapshots-with-core-refresh-revert
  • google-nested:ubuntu-22.04-64:tests/nested/manual/recovery-system-reboot:factory_reset
  • google-nested:ubuntu-22.04-64:tests/nested/manual/recovery-system-reboot:recover
  • google-nested:ubuntu-24.04-64
  • google-nested:ubuntu-24.04-64
  • google-nested:ubuntu-24.04-64:tests/nested/manual/build-with-kernel-modules-components:encrypted
  • google-nested:ubuntu-24.04-64:tests/nested/manual/uc-update-assets-secure:boot
  • google-nested:ubuntu-24.04-64:tests/nested/manual/recovery-system-reboot:factory_reset
  • google-nested:ubuntu-24.04-64:tests/nested/manual/core20-set-efi-boot-vars:BOOTDIR_NOSEC
  • google-nested:ubuntu-24.04-64:tests/nested/manual/kernel-modules-components:encrypted
  • google-nested:ubuntu-24.04-64:tests/nested/manual/core20-fault-inject-on-install-component:kernel_panic_prepare_kernel_components
  • google-nested:ubuntu-24.04-64:tests/nested/manual/recovery-system-reboot:recover
  • google-nested:ubuntu-24.04-64:tests/nested/manual/core20-fault-inject-on-remodel:kernel_reboot_remodel_boot_assets
  • google-nested:ubuntu-24.04-64:tests/nested/manual/core20-auto-remove-user
  • google-nested:ubuntu-24.04-64:tests/nested/manual/uc20-install-in-initrd:both
  • google-nested:ubuntu-24.04-64:tests/nested/manual/component-recovery-system-offline
  • google-nested:ubuntu-24.04-64:tests/nested/manual/hybrid-remodel
  • google-nested:ubuntu-24.04-64:tests/nested/manual/uc20-fde-hooks:tokens

Executing:

  • google-nested:ubuntu-20.04-64:tests/nested/core/core20-fault-inject-on-refresh:kernel_panic_auto_connect
  • google-nested:ubuntu-20.04-64:tests/nested/core/core20-fault-inject-on-refresh:kernel_reboot_auto_connect
  • google-nested:ubuntu-20.04-64:tests/nested/manual/remodel-offline:installed_snaps
  • google-nested:ubuntu-20.04-64:tests/nested/manual/core20-new-snapd-does-not-break-old-initrd
  • google-nested:ubuntu-20.04-64:tests/nested/manual/remodel-offline:local_assertions
  • google-nested:ubuntu-20.04-64:tests/nested/manual/remodel-offline:local_and_installed_snaps
  • google-nested:ubuntu-20.04-64:tests/nested/manual/remodel-uc-to-next-version-fakestore:hook
  • google-nested:ubuntu-22.04-64:tests/nested/core/core20-fault-inject-on-refresh:kernel_reboot_auto_connect
  • google-nested:ubuntu-22.04-64:tests/nested/core/core20-fault-inject-on-refresh:kernel_panic_auto_connect
  • google-nested:ubuntu-22.04-64:tests/nested/manual/muinstaller-real:seeded
  • google-nested:ubuntu-22.04-64:tests/nested/manual/muinstaller-real:encrypted
  • google-nested:ubuntu-24.04-64:tests/nested/core/core20-fault-inject-on-refresh:kernel_panic_auto_connect
  • google-nested:ubuntu-24.04-64:tests/nested/core/core20-fault-inject-on-refresh:kernel_reboot_auto_connect
  • google-nested:ubuntu-24.04-64:tests/nested/core/core22-basic:tokens
  • google-nested:ubuntu-24.04-64:tests/nested/manual/core20-early-config
  • google-nested:ubuntu-24.04-64:tests/nested/manual/remodel-min-size
  • google-nested:ubuntu-24.04-64:tests/nested/manual/muinstaller-real:seeded
  • google-nested:ubuntu-24.04-64:tests/nested/manual/muinstaller-real:encrypted
  • google-nested:ubuntu-24.04-64:tests/nested/manual/core20-cloud-init-maas-signed-seed-data:gadgetwithoutopinion
  • google-core:ubuntu-core-18-64:tests/core/kernel-base-gadget-pair-single-reboot:gadget_base
  • google:ubuntu-20.04-64:tests/main/lxd:snapd_cgroup_neither
  • google:ubuntu-24.04-64:tests/main/document-interfaces-url

Restoring:

  • google-nested:ubuntu-20.04-64:tests/nested/manual/remodel-uc-to-next-version-fakestore:hook
  • google-nested:ubuntu-20.04-64:tests/nested/manual/
  • google-nested:ubuntu-20.04-64
  • google-nested:ubuntu-22.04-64:tests/nested/manual/recovery-system-reboot:recover
  • google-nested:ubuntu-22.04-64:tests/nested/manual/
  • google-nested:ubuntu-22.04-64
  • google-nested:ubuntu-22.04-64:tests/nested/manual/muinstaller-real:encrypted
  • google-nested:ubuntu-22.04-64:tests/nested/manual/
  • google-nested:ubuntu-22.04-64
  • google-nested:ubuntu-24.04-64:tests/nested/manual/recovery-system-reboot:recover
  • google-nested:ubuntu-24.04-64:tests/nested/manual/
  • google-nested:ubuntu-24.04-64
  • google-nested:ubuntu-24.04-64:tests/nested/manual/core20-fault-inject-on-remodel:kernel_reboot_remodel_boot_assets
  • google-nested:ubuntu-24.04-64:tests/nested/manual/
  • google-nested:ubuntu-24.04-64
  • google-nested:ubuntu-24.04-64:tests/nested/manual/muinstaller-real:encrypted
  • google-nested:ubuntu-24.04-64:tests/nested/manual/
  • google-nested:ubuntu-24.04-64
  • google-nested:ubuntu-24.04-64:tests/nested/manual/core20-auto-remove-user
  • google-nested:ubuntu-24.04-64:tests/nested/manual/
  • google-nested:ubuntu-24.04-64
  • google-nested:ubuntu-24.04-64:tests/nested/manual/uc20-install-in-initrd:both
  • google-nested:ubuntu-24.04-64:tests/nested/manual/
  • google-nested:ubuntu-24.04-64
  • google-nested:ubuntu-24.04-64:tests/nested/manual/core20-cloud-init-maas-signed-seed-data:gadgetwithoutopinion
  • google-nested:ubuntu-24.04-64:tests/nested/manual/
  • google-nested:ubuntu-24.04-64
  • google-nested:ubuntu-24.04-64:tests/nested/manual/component-recovery-system-offline
  • google-nested:ubuntu-24.04-64:tests/nested/manual/
  • google-nested:ubuntu-24.04-64
  • google-nested:ubuntu-24.04-64:tests/nested/manual/hybrid-remodel
  • google-nested:ubuntu-24.04-64:tests/nested/manual/
  • google-nested:ubuntu-24.04-64
  • google-nested:ubuntu-24.04-64:tests/nested/manual/uc20-fde-hooks:tokens
  • google-nested:ubuntu-24.04-64:tests/nested/manual/
  • google-nested:ubuntu-24.04-64
  • google-core:ubuntu-core-18-64:tests/core/kernel-base-gadget-pair-single-reboot:gadget_base
  • google-core:ubuntu-core-18-64:tests/core/
  • google-core:ubuntu-core-18-64

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⛔ Blocked FDE Manager Pull requests that target FDE manager branch Needs Samuele review Needs a review from Samuele before it can land Run nested The PR also runs tests inluded in nested suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants