secboot: add argon2 out-of-process kdf support #15073

ZeyadYasser · 2025-02-13T14:48:13Z

MaybeRunArgon2OutOfProcessRequestHandler is supposed to be called from the main() of binaries involved with sealing/unsealing of keys (i.e. snapd and snap-bootstrap).

This switches the binary to a special mode where it acts as an argon2 out-of-process helper command, and exits when its work is done.

For more context, check docs for sb.WaitForAndRunArgon2OutOfProcessRequest and sb.NewOutOfProcessArgon2KDF for details on how the flow works in secboot.

This PR also reverts workaround from #15058 as now the argon2 kdf variants are working.

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

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

…)" This reverts commit ec60555.

github-actions · 2025-02-13T15:02:10Z

Fri Feb 14 15:18:15 UTC 2025
The following results are from: https://github.com/canonical/snapd/actions/runs/13329597963

Failures:

Executing:

google-core:ubuntu-core-20-64:tests/core/gadget-update-pc
google:ubuntu-24.10-64:tests/main/cloud-init
google:ubuntu-24.04-64:tests/main/progress

Restoring:

google-core:ubuntu-core-20-64:tests/core/gadget-update-pc
google-core:ubuntu-core-20-64:tests/core/
google-core:ubuntu-core-20-64

pedronis

couple initial comments

pedronis · 2025-02-13T17:26:21Z

cmd/snap-bootstrap/main.go

@@ -40,6 +41,11 @@ such as initramfs.
 )

 func main() {
+	if err := secboot.MaybeRunArgon2OutOfProcessRequestHandler(); err != nil {
+		fmt.Fprintf(os.Stderr, "cannot run argon2 out-of-process helper command: %v", err)


it's a bit unclear to me why we don't want to deal with this inside the helper directly

Thanks, updated!

pedronis · 2025-02-13T17:26:28Z

cmd/snapd/main.go

@@ -57,6 +58,11 @@ func main() {
 		snapdtool.ExecInSnapdOrCoreSnap()
 	}

+	if err := secboot.MaybeRunArgon2OutOfProcessRequestHandler(); err != nil {
+		fmt.Fprintf(os.Stderr, "cannot run argon2 out-of-process helper command: %v", err)


pedronis · 2025-02-13T17:27:01Z

secboot/argon2_out_of_process_sb.go

+}
+
+const outOfProcessArgon2KDFTimeout = 100 * time.Millisecond
+const argon2Cmd = "run-argon2"


my preference is something like an option "--argon2-proc"

I don't have strong opinions on this, I went with a sub-command to be ready in case we want to pass args in the future to customize how the out-of-process helper is configured, but since this will be internal and be self invoking we can easily switch later if need arise and not worry about the binaries being in sync.

switched to --argon2-proc

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

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

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

pedronis

question related to what happens if a binary uses argon2 but is not set up to process --argon2-proc ?

secboot/argon2_out_of_process_sb.go

codecov · 2025-02-14T09:58:52Z

Codecov Report

Attention: Patch coverage is 75.00000% with 11 lines in your changes missing coverage. Please review.

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

Files with missing lines	Patch %	Lines
secboot/argon2_out_of_process_sb.go	85.18%	3 Missing and 1 partial ⚠️
secboot/argon2_out_of_process_dummy.go	0.00%	3 Missing ⚠️
cmd/snap-bootstrap/main.go	0.00%	2 Missing ⚠️
cmd/snapd/main.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             master   #15073   +/-   ##
=========================================
  Coverage          ?   78.06%           
=========================================
  Files             ?     1183           
  Lines             ?   157779           
  Branches          ?        0           
=========================================
  Hits              ?   123174           
  Misses            ?    26956           
  Partials          ?     7649

Flag	Coverage Δ
unittests	`78.06% <75.00%> (?)`

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.

bboozzoo · 2025-02-14T10:25:53Z

secboot/argon2_out_of_process_sb.go

+)
+
+func init() {
+	setArgon2KDF()


this will now affect every program in which one of the packages imports secboot

switched to setting the kdf implementation from MaybeRunArgon2OutOfProcessRequestHandler itself as described here #15073 (comment)

bboozzoo · 2025-02-14T10:27:00Z

secboot/argon2_out_of_process_dummy.go

+
+package secboot
+
+func MaybeRunArgon2OutOfProcessRequestHandler() {}


I think this should raise an error/panic if it observes a trigger for out of process secboot runner

bboozzoo · 2025-02-14T10:30:34Z

secboot/argon2_out_of_process_sb.go

+// and sb.NewOutOfProcessArgon2KDF for details on how the flow works
+// in secboot.
+func MaybeRunArgon2OutOfProcessRequestHandler() {
+	if len(os.Args) < 2 || os.Args[1] != outOfProcessArgon2Arg {


I'm bit uncomfortable that this hijacks command line arguments and adds an argument which not exposed anywhere but the secboot package.

bboozzoo · 2025-02-14T10:46:37Z

secboot/argon2_out_of_process_sb.go

+// For more context, check docs for sb.WaitForAndRunArgon2OutOfProcessRequest
+// and sb.NewOutOfProcessArgon2KDF for details on how the flow works
+// in secboot.
+func MaybeRunArgon2OutOfProcessRequestHandler() {


What if we change the API a little such that it can be used like so, in snapd:

// snapd/main.go func executeSecbootArgon2Runner { logger.Noticef("running argon2 out-of-process helper") // Ignore the lock release callback and use implicit release on process termination. _, err := secboot.ExecuteAsArgon2Runner(secboot.Argon2RunnerOptions{ Watchdog: false, }) if err != nil { logger.Errorf("cannot execute runner: %v", err) os.Exit(1) } os.Exit(0) } func main() { ... InstallSecbootHandlerOnArg([]string{"--secboot-argon2-runner"}) if len(os.Args) == 2 && os.Args[1] == "--secboot-argon2-runner" { executeArgon2Runner() // not-reached } ... }

and snap-bootstrap:

// snap-boostrap/main.go func main() { ... InstallSecbootHandlerOnArg([]string{"secboot-argon2-runner"}) } // snap-boostrap/cmd_secboot_argon2_runner.go func init() { addCommandBuilder(func(parser *flags.Parser) { cmd, err := parser.AddCommand("secboot2_argon2-runner", short, long, &cmdArgon2Runner{}) if err != nil { panic(err) } // make it hidden cmd.Hidden = true }) } type cmdArgon2Runner struct {} func (c *cmdArgon2Runner) Execute([]string) error { logger.Noticef("running argon2 out-of-process helper") // Ignore the lock release callback and use implicit release on process termination. _, err := secboot.ExecuteAsArgon2Runner(secboot.Argon2RunnerOptions{ Watchdog: false, }) return err }

that's a lot of extra code that I'm not keen on. Maybe the compromise is to pass the marker flag to the MaybeRunArgon2OutOfProcessRequestHandler

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

bboozzoo · 2025-02-14T12:11:45Z

secboot/argon2_out_of_process_dummy.go

+
+package secboot
+
+func MaybeRunArgon2OutOfProcessRequestHandler() {}


Suggested change

func MaybeRunArgon2OutOfProcessRequestHandler() {}

func MaybeRunArgon2OutOfProcessRequestHandler() {

if !isOutOfProcessArgon2KDFMode() {

panic("internal error: unexpected call to execute as argon2 runner in non-secboot binary")

}

}

I think it should panic when isOutOfProcessArgon2KDFMode is true

yes, my bad, I copied directly without changing it

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

pedronis

thanks, couple small comments

pedronis · 2025-02-14T15:13:40Z

cmd/snap-bootstrap/main.go

@@ -40,6 +41,8 @@ such as initramfs.
 )

 func main() {
+	secboot.HijackAndRunArgon2OutOfProcessHandlerOnArg([]string{"--argon2-proc"})


given that is passed in now, I'm not against dropping "--" here

pedronis · 2025-02-14T15:14:06Z

secboot/argon2_out_of_process_sb_test.go

+
+var _ = Suite(&argon2Suite{})
+
+func (*argon2Suite) TestMaybeRunArgon2OutOfProcessRequestHandlerArgon2Mode(c *C) {


tests might need to be renamed now

pedronis · 2025-02-14T15:18:33Z

secboot/secboot.go

@@ -268,3 +269,17 @@ func MarkSuccessful() error {

 	return nil
 }
+
+func isOutOfProcessArgon2KDFMode(args []string) bool {


maybe we should just have this in a small separate argon2_out_of_process.go ?

ZeyadYasser added the Needs Samuele review Needs a review from Samuele before it can land label Feb 13, 2025

ZeyadYasser added this to the 2.68.1 milestone Feb 13, 2025

ZeyadYasser requested review from bboozzoo and pedronis February 13, 2025 14:48

ZeyadYasser added 3 commits February 13, 2025 16:58

secboot: add argon2 out-of-process command helpers

11223a9

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

cmd/{snapd,snap-bootstrap}: add argon2 out-of-process special mode su…

ed24159

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

Revert "secboot: default to pbkdf2 instead of argon2id (canonical#15058…

4d9e5c0

…)" This reverts commit ec60555.

ZeyadYasser force-pushed the fde-argon2-out-of-process-kdf branch from 9b854ca to 4d9e5c0 Compare February 13, 2025 14:59

pedronis reviewed Feb 13, 2025

View reviewed changes

ZeyadYasser added 3 commits February 14, 2025 10:05

fixup! secboot: use --argon2-proc arg instead of run-argon2 sub-command

6df58d8

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

fixup! secboot: handle error directly in MaybeRunArgon2OutOfProcessRe…

02ca933

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

fixup! secboot: disable argon2 out-of-process tests when built with n…

3e1485d

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

ZeyadYasser requested a review from pedronis February 14, 2025 09:34

pedronis reviewed Feb 14, 2025

View reviewed changes

secboot/argon2_out_of_process_sb.go Show resolved Hide resolved

bboozzoo reviewed Feb 14, 2025

View reviewed changes

fixup! secboot: set argon2 kdf from MaybeRunArgon2OutOfProcessRequest…

ce2f485

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

bboozzoo reviewed Feb 14, 2025

View reviewed changes

fixup! secboot: allow passing matching args for argon2 special mode

3cebcc4

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

ZeyadYasser requested review from bboozzoo and pedronis February 14, 2025 12:53

pedronis approved these changes Feb 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

secboot: add argon2 out-of-process kdf support #15073

secboot: add argon2 out-of-process kdf support #15073

ZeyadYasser commented Feb 13, 2025

github-actions bot commented Feb 13, 2025 •

edited

Loading

pedronis left a comment

pedronis Feb 13, 2025

ZeyadYasser Feb 14, 2025

pedronis Feb 13, 2025

pedronis Feb 13, 2025

ZeyadYasser Feb 14, 2025

ZeyadYasser Feb 14, 2025

pedronis left a comment

codecov bot commented Feb 14, 2025 •

edited

Loading

bboozzoo Feb 14, 2025

ZeyadYasser Feb 14, 2025

bboozzoo Feb 14, 2025

bboozzoo Feb 14, 2025

bboozzoo Feb 14, 2025

pedronis Feb 14, 2025 •

edited

Loading

bboozzoo Feb 14, 2025

ZeyadYasser Feb 14, 2025

bboozzoo Feb 14, 2025

pedronis left a comment

pedronis Feb 14, 2025

pedronis Feb 14, 2025

pedronis Feb 14, 2025


		package secboot

		func MaybeRunArgon2OutOfProcessRequestHandler() {}


		var _ = Suite(&argon2Suite{})

		func (argon2Suite) TestMaybeRunArgon2OutOfProcessRequestHandlerArgon2Mode(c C) {

secboot: add argon2 out-of-process kdf support #15073

Are you sure you want to change the base?

secboot: add argon2 out-of-process kdf support #15073

Conversation

ZeyadYasser commented Feb 13, 2025

github-actions bot commented Feb 13, 2025 • edited Loading

Failures:

Executing:

Restoring:

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

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

pedronis left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 14, 2025 • edited Loading

Codecov Report

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

pedronis Feb 14, 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

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

Choose a reason for hiding this comment

github-actions bot commented Feb 13, 2025 •

edited

Loading

codecov bot commented Feb 14, 2025 •

edited

Loading

pedronis Feb 14, 2025 •

edited

Loading