gadget/install,secboot: use snapcore/secboot luks2 api

[cmatsuoka]

Previously when installing a uc20 system we called cryptsetup luksFormat
directly to create an encrypted device, and cryptsetup luksAddKey to add the
recovery key. These two operations are now provided by snapcore/secboot.
(Note that we don't call snapcore/secboot directly, we use our own secboot
package to wrap those calls instead so we can build without snapcore/secboot
depending on the build tags we use.)

[cmatsuoka]

Blocked, must update TPM connection error handling first.

[zyga]

This looks okay to me. I added a comment about one part in encrypt_tpm.go where I wasn't sure why key copy is done.

[zyga]

Nitpick, it is not obvious why a copy of the key needs to be made here and below. Could you add a comment to the code please.

[cmatsuoka]

The reason is not that we need a copy, it's because key is an array and the function expects a slice. However, the called function may be changed to require a [64]byte array instead because the first thing it does is to check the size of the slice and fail if it's not exactly 64. See canonical/secboot#45

[mvo5]

Thanks for this PR! Looks good, some ideas/suggestions inline. But no blockers AFAICT

[mvo5]

For completness we may want to have a test with both "openErr" and "formatErr" to have the full matrix

[cmatsuoka]

newEncryptedDevice has a separate test which should ensure it's working properly, it shouldn't be necessary to test it again when testing the recovery key (mockedOpenErr and mockedFormatErr are relevant only when creating the device and not when adding a recovery key).

[mvo5]

Some small comments would be nice here, i.e. we have three times "err" - either some small comment or soemthing like:

mockedFormatErr
mockedOpenErr
expectedErr

(or similar)

[mvo5]

Extra whitespace

[mvo5]

Same comment as above, i.e. maybe slightly more verbose what the relation of addErr and err is? Somehting like "mockedAddErr, expectedErr" maybe ?

[mvo5]

We are not checking node here, why is that?

[cmatsuoka]

Oops, this was really missing, thanks Michael.

[mvo5]

We probably want to check here that c.Assert(cryptsetup.Calls(), HasLen, 0)

[cmatsuoka]

Depending on the test case cryptsetup may have been called at this point (e.g. the open error is the result of a cryptsetup open operation).

[mvo5]

Same suggestion as above to move the recovery key creation into the encryptionSuite setup

[mvo5]

I wonder if we should also create the keys in encrypSuite here (like we did in the gadget code). Somehting like:

diff --git a/secboot/encrypt_test.go b/secboot/encrypt_test.go
index 14860c4e78..4d149187be 100644
--- a/secboot/encrypt_test.go
+++ b/secboot/encrypt_test.go
@@ -28,12 +28,24 @@ import (
 	"github.com/snapcore/snapd/secboot"
 )
 
-type encryptSuite struct{}
+type encryptSuite struct {
+	mockedEncryptionKey secboot.EncryptionKey
+	mockedRecoveryKey   secboot.RecoveryKey
+}
 
 var _ = Suite(&encryptSuite{})
 
+func (s *encryptSuite) TestSetUp(c *C) {
+	// create empty key to prevent blocking on lack of system entropy
+	s.mockedEncryptionKey = secboot.EncryptionKey{}
+	for i := range s.mockedEncryptionKey {
+		s.mockedEncryptionKey[i] = byte(i)
+	}
+	s.mockedRecoveryKey = secboot.RecoveryKey{15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0}
+}
+
 func (s *encryptSuite) TestRecoveryKeySave(c *C) {
-	rkey := secboot.RecoveryKey{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 255}
+	rkey := s.mockedRecoveryKey
 	err := rkey.Save("test-key")
 	c.Assert(err, IsNil)
 	fileInfo, err := os.Stat("test-key")
diff --git a/secboot/encrypt_tpm_test.go b/secboot/encrypt_tpm_test.go
index 12b984c34d..a0b692b622 100644
--- a/secboot/encrypt_tpm_test.go
+++ b/secboot/encrypt_tpm_test.go
@@ -36,23 +36,17 @@ func (s *encryptSuite) TestFormatEncryptedDevice(c *C) {
 		{initErr: nil, err: ""},
 		{initErr: errors.New("some error"), err: "some error"},
 	} {
-		// create empty key to prevent blocking on lack of system entropy
-		myKey := secboot.EncryptionKey{}
-		for i := range myKey {
-			myKey[i] = byte(i)
-		}
-
 		calls := 0
 		restore := secboot.MockSbInitializeLUKS2Container(func(devicePath, label string, key []byte) error {
 			calls++
 			c.Assert(devicePath, Equals, "/dev/node")
 			c.Assert(label, Equals, "my label")
-			c.Assert(key, DeepEquals, myKey[:])
+			c.Assert(key, DeepEquals, s.mockedEncryptionKey[:])
 			return tc.initErr
 		})
 		defer restore()
 
-		err := secboot.FormatEncryptedDevice(myKey, "my label", "/dev/node")
+		err := secboot.FormatEncryptedDevice(s.mockedEncryptionKey, "my label", "/dev/node")
 		c.Assert(calls, Equals, 1)
 		if tc.err == "" {
 			c.Assert(err, IsNil)
@@ -70,25 +64,17 @@ func (s *encryptSuite) TestAddRecoveryKey(c *C) {
 		{addErr: nil, err: ""},
 		{addErr: errors.New("some error"), err: "some error"},
 	} {
-		// create empty key to prevent blocking on lack of system entropy
-		myKey := secboot.EncryptionKey{}
-		for i := range myKey {
-			myKey[i] = byte(i)
-		}
-
-		myRecoveryKey := secboot.RecoveryKey{15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0}
-
 		calls := 0
 		restore := secboot.MockSbAddRecoveryKeyToLUKS2Container(func(devicePath string, key []byte, recoveryKey [16]byte) error {
 			calls++
 			c.Assert(devicePath, Equals, "/dev/node")
-			c.Assert(recoveryKey[:], DeepEquals, myRecoveryKey[:])
-			c.Assert(key, DeepEquals, myKey[:])
+			c.Assert(recoveryKey[:], DeepEquals, s.mockedRecoveryKey[:])
+			c.Assert(key, DeepEquals, s.mockedEncryptionKey[:])
 			return tc.addErr
 		})
 		defer restore()
 
-		err := secboot.AddRecoveryKey(myKey, myRecoveryKey, "/dev/node")
+		err := secboot.AddRecoveryKey(s.mockedEncryptionKey, s.mockedRecoveryKey, "/dev/node")
 		c.Assert(calls, Equals, 1)
 		if tc.err == "" {
 			c.Assert(err, IsNil)

but it seems less important here as we only use myKey twice and the recovery key once

[mvo5]

Thanks, looks good