go.mod: update secboot to 42c7ea9715b3 #14253

valentindavid · 2024-07-25T11:29:02Z

Also update github.com/mvo5/goconfigparser to latest.

This commit of secboot is right before the introduction of new key data format.

valentindavid · 2024-07-25T13:09:26Z

This is taken from #13278 (which is from #13210)

secboot/secboot_sb_test.go

valentindavid · 2024-07-26T11:01:44Z

This depends on #14252

pedronis

did a pass, some questions and comments, maybe Chris should also take a look at this?

pedronis · 2024-07-26T12:39:41Z

secboot/secboot_sb.go

-	if err := sbActivateVolumeWithRecoveryKey(name, device, nil, &options); err != nil {
+	authRequestor, err := newAuthRequestor()
+	if err != nil {
+		return fmt.Errorf("cannot build an auth requestor: %v", err)


sounds like this should be an i"nternal error:" ?

pedronis · 2024-07-26T12:40:50Z

secboot/secboot_tpm.go

@@ -280,8 +286,14 @@ func unlockEncryptedPartitionWithSealedKey(mapperName, sourceDevice, keyfile str
 		return NotUnlocked, fmt.Errorf("cannot read key data: %v", err)
 	}
 	options := activateVolOpts(allowRecovery)
+	options.Model = sb.SkipSnapModelCheck
 	// ignoring model checker as it doesn't work with tpm "legacy" platform key data


shouldn't this comment go before the previous line? or should this comment be changed instead?

pedronis · 2024-07-26T12:43:18Z

secboot/secboot_hooks.go

-	var handle []byte
-	if keySetup.Handle == nil {
-		// this will reach fde-reveal-key as null but should be ok
-		handle = []byte("null")


have we tested if this work or is not needed anymore? I suppose this is about trying things with the OptEE hooks we have

The original commit: 9807656

secboot: secboot.sb.NewKeyData() now marhals the handle internally

The old version of secboot.sb.NewKeyData() was expecting a handle
in json encoded form. However the new code is now doing the
marshalling itself.

In the future PRs, the code will be different. We will return the handle from a method implementing KeyProtector.ProtectKey.

Looking at the spike branch, something similar is also done there. It has to do with dereference. So for sure the code is tested with nil, since I had to deal with it.

secboot/secboot_hooks.go

pedronis

thanks, couple of comments

pedronis · 2024-08-09T15:07:22Z

secboot/secboot_hooks.go

-	}
-	if !ok {
-		return res, fmt.Errorf("cannot unlock volume: model %s/%s not authorized", model.BrandID(), model.Model())
-	}


there'a defer here that can also go away now as there's no code that can set error in the rest of the function anymore?

secboot/secboot_sb_test.go

pedronis

thanks

pedronis · 2024-08-12T14:07:14Z

secboot/secboot_sb_test.go

+
+			return kd, nil
+		})
+		defer restore()

 		restore = secboot.MockSbActivateVolumeWithKeyData(func(volumeName, sourceDevicePath string, authRequestor sb.AuthRequestor, kdf sb.KDF, options *sb.ActivateVolumeOptions, keys ...*sb.KeyData) error {

 			c.Assert(volumeName, Equals, "name-"+randomUUID)
 			c.Assert(sourceDevicePath, Equals, devicePath)
 			c.Assert(keys, HasLen, 1)
 			c.Assert(keys[0], NotNil)


we might leave this one out now?

I can remove it.

alfonsosanchezbeato

LGTM, small nitpick below

alfonsosanchezbeato · 2024-08-13T19:37:21Z

secboot/secboot_hooks.go

 	options := activateVolOpts(opts.AllowRecoveryKey)
-	modChecker, err := sbActivateVolumeWithKeyData(mapperName, sourceDevice, keyData, options)
+	options.Model = model


nitpick: maybe it would make sense if activateVolOpts() accepts the model as input so it creates ActivateVolumeOptions with it already instead of assigning a line later. As this is done in the 2 cases where this function is called.

Well, since we will remove this one and call SetModel instead, I prefer not to change it.

Maybe this is not completely true. For the old keys will still need this to be set. But we will possibly we will only add it when we have old key files and not when we have key slot token data. Then we can set it to nil to make it force to fail if it finds old data. I will add a FIXME here.

Also both unlock for FDE hook and tpm will be joined in the same function at some point.

bboozzoo

LGTM

chrisccoulson

This looks ok to me - I've left a few minor comments

chrisccoulson · 2024-08-21T16:28:22Z

secboot/secboot_hooks.go

+		return res, fmt.Errorf("internal error: cannot build an auth requestor: %v", err)
+	}
+
+	err = sbActivateVolumeWithKeyData(mapperName, sourceDevice, authRequestor, sb.Argon2iKDF(), options, keyData)


Does it work if you pass nil instead of sb.Argon2iKDF? The intention here is that this API would always be executed remotely in a short-lived helper process because otherwise there's a need to force a GC between invocations. As there's technically no passphrase support right now, nothing should call this anyway.

This parameter is removed in the next queued up PR. But I can change that and see if the tests are still working

Unlocking failed with:

[ 4.522158] snap-bootstrap[195]: 2024/08/22 12:44:12.816188 main.go:63: execution error: cannot unlock encrypted partition: nil kdf

chrisccoulson · 2024-08-21T16:42:45Z

secboot/encrypt_sb.go

@@ -38,8 +38,7 @@ import (
 )

 var (
-	sbInitializeLUKS2Container       = sb.InitializeLUKS2Container
-	sbAddRecoveryKeyToLUKS2Container = sb.AddRecoveryKeyToLUKS2Container


Did nothing call this before?

It was removed in 3988a93 which was in PR #11715

This mini key manager has a copy of the internal code of secboot.

I could make a separate commit of removing this and the mock.

chrisccoulson · 2024-08-21T16:45:22Z

secboot/secboot_hooks.go

+	// FIXME: consider setting it in activateVolOpts if we keep
+	// this function separate from the tpm one for key data in
+	// files. Otherwise we should only set it if we provide key
+	// data generation 1.


I'd like to actually remove the Model field in secboot given that future versions of snap-bootstrap are going to call bootscope.SetModel. We should probably obtain the model from there for existing keys rather than have a separate option.

That would be great.

Its use was removed in 3988a93 which was in PR canonical#11715

Also update github.com/mvo5/goconfigparser to latest.

valentindavid force-pushed the valentindavid/secboot-update-part-1 branch 3 times, most recently from 416ca44 to 742eb61 Compare July 25, 2024 12:47

valentindavid commented Jul 25, 2024

View reviewed changes

secboot/secboot_sb_test.go Outdated Show resolved Hide resolved

valentindavid commented Jul 25, 2024

View reviewed changes

secboot/secboot_sb_test.go Outdated Show resolved Hide resolved

valentindavid commented Jul 25, 2024

View reviewed changes

secboot/secboot_sb_test.go Outdated Show resolved Hide resolved

valentindavid force-pushed the valentindavid/secboot-update-part-1 branch 2 times, most recently from 6179daa to ff9418e Compare July 25, 2024 15:00

valentindavid added the Run nested The PR also runs tests inluded in nested suite label Jul 25, 2024

valentindavid closed this Jul 25, 2024

valentindavid reopened this Jul 25, 2024

valentindavid added the FDE Manager Pull requests that target FDE manager branch label Jul 26, 2024

valentindavid marked this pull request as ready for review July 26, 2024 11:02

valentindavid requested a review from pedronis July 26, 2024 11:13

pedronis force-pushed the fde-manager-features branch from b51bae4 to 225786c Compare July 26, 2024 12:29

valentindavid force-pushed the valentindavid/secboot-update-part-1 branch from ff9418e to 445c1d8 Compare July 26, 2024 12:31

pedronis reviewed Jul 26, 2024

View reviewed changes

valentindavid requested a review from chrisccoulson July 26, 2024 13:46

bboozzoo self-requested a review July 29, 2024 13:46

This was referenced Jul 31, 2024

many: split tests related to fdestate backend from boot package #14282

Closed

many: update secboot and use new key format #14304

Merged

valentindavid requested a review from pedronis August 5, 2024 11:38

valentindavid force-pushed the valentindavid/secboot-update-part-1 branch from 3445da9 to 4f0b595 Compare August 7, 2024 16:32

pedronis requested review from alfonsosanchezbeato and removed request for chrisccoulson August 9, 2024 11:39

pedronis reviewed Aug 9, 2024

View reviewed changes

valentindavid requested a review from pedronis August 12, 2024 09:05

pedronis approved these changes Aug 12, 2024

View reviewed changes

alfonsosanchezbeato approved these changes Aug 13, 2024

View reviewed changes

bboozzoo approved these changes Aug 14, 2024

View reviewed changes

valentindavid force-pushed the valentindavid/secboot-update-part-1 branch from 762adfe to 6cc667c Compare August 15, 2024 08:24

Meulengracht requested a review from chrisccoulson August 20, 2024 12:44

chrisccoulson approved these changes Aug 21, 2024

View reviewed changes

valentindavid force-pushed the valentindavid/secboot-update-part-1 branch from 6cc667c to 1b78a4a Compare August 22, 2024 09:10

secboot: remove dead mock for secboot.AddRecoveryKeyToLUKS2Container

4b5bc57

Its use was removed in 3988a93 which was in PR canonical#11715

valentindavid force-pushed the valentindavid/secboot-update-part-1 branch 3 times, most recently from 3f7f246 to d20a337 Compare August 23, 2024 09:49

github-actions bot added the Run Nested -auto- Label automatically added in case nested tests need to be executed label Aug 23, 2024

valentindavid force-pushed the valentindavid/secboot-update-part-1 branch from d20a337 to bd90f5b Compare August 23, 2024 11:09

go.mod: update secboot to 42c7ea9715b3

89a01e9

Also update github.com/mvo5/goconfigparser to latest.

valentindavid force-pushed the valentindavid/secboot-update-part-1 branch from bd90f5b to 89a01e9 Compare August 23, 2024 13:12

pedronis merged commit 1e0707b into canonical:fde-manager-features Aug 26, 2024
46 of 51 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

go.mod: update secboot to 42c7ea9715b3 #14253

go.mod: update secboot to 42c7ea9715b3 #14253

valentindavid commented Jul 25, 2024 •

edited

Loading

valentindavid commented Jul 25, 2024

valentindavid commented Jul 26, 2024

pedronis left a comment

pedronis Jul 26, 2024

pedronis Jul 26, 2024

pedronis Jul 26, 2024

valentindavid Jul 26, 2024

valentindavid Jul 26, 2024

valentindavid Jul 26, 2024

pedronis left a comment

pedronis Aug 9, 2024

pedronis left a comment

pedronis Aug 12, 2024

valentindavid Aug 13, 2024

alfonsosanchezbeato left a comment

alfonsosanchezbeato Aug 13, 2024

valentindavid Aug 14, 2024

valentindavid Aug 14, 2024

valentindavid Aug 14, 2024

bboozzoo left a comment

chrisccoulson left a comment

chrisccoulson Aug 21, 2024

valentindavid Aug 22, 2024

valentindavid Aug 22, 2024

chrisccoulson Aug 21, 2024

valentindavid Aug 22, 2024

chrisccoulson Aug 21, 2024

valentindavid Aug 22, 2024

go.mod: update secboot to 42c7ea9715b3 #14253

go.mod: update secboot to 42c7ea9715b3 #14253

Conversation

valentindavid commented Jul 25, 2024 • edited Loading

valentindavid commented Jul 25, 2024

valentindavid commented Jul 26, 2024

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

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

alfonsosanchezbeato 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

bboozzoo left a comment

Choose a reason for hiding this comment

chrisccoulson 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

Choose a reason for hiding this comment

valentindavid commented Jul 25, 2024 •

edited

Loading