recovery: integrate TPM provisioning by cmatsuoka · Pull Request #7085 · canonical/snapd

cmatsuoka · 2019-07-09T21:04:40Z

Execute TPM provisioning on install using Chris Coulson's FDE utils code
from https://github.com/chrisccoulson/ubuntu-core-fde-utils. This also
requires google/go-tpm#109 which has been added
to vendored packages.

Try to provision the TPM as late as possible to prevent a situation
where the installation fails after the TPM is provisioned. Provisioning
happens just before sealing the LUKS device keyfile and the lockout
authorization value is stored inside the encrypted partition.

Signed-off-by: Claudio Matsuoka claudio.matsuoka@canonical.com

Execute TPM provisioning on install using Chris Coulson's FDE utils code from https://github.com/chrisccoulson/ubuntu-core-fde-utils. This also requires google/go-tpm#109 which has been added to vendored packages. Try to provision the TPM as late as possible to prevent a situation where the installation fails after the TPM is provisioned. Provisioning happens just before sealing the LUKS device keyfile and the lockout authorization value is stored inside the encrypted partition. Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>

recovery/device.go

zyga

Quick pass of my uninformed eyes. I didn't review any of the TPM mechanics but I left a lot of little wording nitpicks for error messages and some misc comments.

recovery/device.go

zyga · 2019-07-10T07:08:46Z

recovery/fdeutils/provisioning.go

+)
+
+const (
+	srkHandle tpmutil.Handle = 0x81000000


It would be great to reference this to some documentation.

zyga · 2019-07-10T07:10:05Z

recovery/fdeutils/provisioning.go

+func ProvisionTPM(lockoutAuth []byte) error {
+	rw, err := tpm2.OpenTPM(tpmPath)
+	if err != nil {
+		return fmt.Errorf("failed to open TPM device: %v", err)


Small wording tweak to the error message.

Suggested change

return fmt.Errorf("failed to open TPM device: %v", err)

return fmt.Errorf("cannot to open TPM device %q: %v", tpmPath, err)

zyga · 2019-07-10T07:10:27Z

recovery/fdeutils/provisioning.go

+
+	c, _, err := tpm2.GetCapability(rw, tpm2.CapabilityTPMProperties, 1, permanentProps)
+	if err != nil {
+		return fmt.Errorf("failed to request permanent properties: %v", err)


Suggested change

return fmt.Errorf("failed to request permanent properties: %v", err)

return fmt.Errorf("cannot request permanent TPM properties: %v", err)

zyga · 2019-07-10T07:10:39Z

recovery/fdeutils/provisioning.go

+	}
+
+	if err := tpm2.Clear(rw, tpm2.HandleLockout, ""); err != nil {
+		return fmt.Errorf("failed to clear the TPM: %v", err)


Suggested change

return fmt.Errorf("failed to clear the TPM: %v", err)

return fmt.Errorf("cannot clear the TPM: %v", err)

zyga · 2019-07-10T07:12:57Z

recovery/fdeutils/provisioning.go

+func RequestTPMClearUsingPPI() error {
+	f, err := os.OpenFile(ppiPath, os.O_WRONLY, 0)
+	if err != nil {
+		return fmt.Errorf("failed to open request handle: %v", err)


Suggested change

return fmt.Errorf("failed to open request handle: %v", err)

return fmt.Errorf("cannot open TPM request handle %q: %v", ppiPath, err)

zyga · 2019-07-10T07:13:08Z

recovery/fdeutils/provisioning.go

+	defer f.Close()
+
+	if _, err := f.WriteString(clearPPIRequest); err != nil {
+		return fmt.Errorf("failed to submit request: %v", err)


Suggested change

return fmt.Errorf("failed to submit request: %v", err)

return fmt.Errorf("cannot submit TPM request: %v", err)

zyga · 2019-07-10T07:13:43Z

recovery/recovery.go

-func createWritable(keyfile string, keyfileSize int, cryptdev string) error {
-	logger.Noticef("Creating new writable")
+func createWritable(keyBuffer []byte, cryptdev string) error {
+	logger.Noticef("Creating new ubuntu-data")


Suggested change

logger.Noticef("Creating new ubuntu-data")

logger.Noticef("Creating new ubuntu-data partition")

It would be good to always refer to partitions as "foo" partition, because the bare name like ubuntu-data is not associated with partitions in any way.

zyga · 2019-07-10T07:15:17Z

recovery/recovery.go

+
+	// Don't remove this sync, it prevents file corruption on vfat
+	if err := exec.Command("sync").Run(); err != nil {
+		return fmt.Errorf("cannot sync: %s", err)


Please just use syscall.Sync instead. No need to fork for this.

Ah, excellent, that's much better, thank you.

vendor/vendor.json

mvo5

Looks good, I pushed a small tweak to the vendor.json and a small suggestion in the code.

mvo5 · 2019-07-10T07:36:47Z

recovery/fdeutils/provisioning.go

+const (
+	srkHandle tpmutil.Handle = 0x81000000
+
+	tpmPath string = "/dev/tpm0"


We don't need the "string" here (does not cause harm either though).

mvo5 · 2019-07-10T07:38:31Z

vendor/vendor.json

+			"origin": "github.com/chrisccoulson/go-tpm",
+			"path": "github.com/google/go-tpm",
+			"revision": "c6c7cb7465ae50e13263f2f8ff33f57d1f9859bc",
+			"revisionTime": "2019-07-08T13:36:22Z"


This will need the following extra line:

"tree": true

mvo5 · 2019-07-10T08:47:38Z

recovery/fdeutils/provisioning.go

+	return nil
+}
+
+func RequestTPMClearUsingPPI() error {


This is used only inside this package, it should probably be unexported - we can always export it if needed.

mvo5 · 2019-07-10T08:49:43Z

recovery/fdeutils/provisioning.go

@@ -0,0 +1,100 @@
+package fdeutils


Copyright header missing.

mvo5 · 2019-07-10T09:20:09Z

I talked to Chris and he preferes to have the code in his repo for now. I will rework this to import it from his repo.

Don't store the unsealed key to the encrypted partition in the filesystem, write it to cryptsetup's stdin instead. Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>

Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>

…FTBFS on arm64

zyga reviewed Jul 10, 2019

View reviewed changes

recovery/device.go Outdated Show resolved Hide resolved

zyga reviewed Jul 10, 2019

View reviewed changes

vendor.json: add "tree": true

f2ac2bf

mvo5 approved these changes Jul 10, 2019

View reviewed changes

mvo5 reviewed Jul 10, 2019

View reviewed changes

recovery/fdeutils/provisioning.go Outdated

@@ -0,0 +1,100 @@

package fdeutils

Copy link

Contributor

mvo5 Jul 10, 2019

Choose a reason for hiding this comment

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

Copyright header missing.

fdeutils: do not build on arm64 (no syscall.SYS_POLL there)

8f2825c

mvo5 and others added 6 commits July 10, 2019 11:32

recovery: import fdeutils from chris instead of copying it

316fb75

recovery: pass key to cryptsetup using stdin

08c31d5

Don't store the unsealed key to the encrypted partition in the filesystem, write it to cryptsetup's stdin instead. Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>

recovery: use sync from package syscall

6e4b60d

Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>

recovery: fix naked return error from travis

7e796c9

vendor: update github.com/chrisccoulson/ubuntu-core-fde-utils to fix …

1d70445

…FTBFS on arm64

recovery: rename fdeutils->fdeutil (upstream changed this)

bc0c3db

mvo5 merged commit 073180b into canonical:uc20 Jul 12, 2019

	return fmt.Errorf("failed to open TPM device: %v", err)
	return fmt.Errorf("cannot to open TPM device %q: %v", tpmPath, err)

	return fmt.Errorf("failed to request permanent properties: %v", err)
	return fmt.Errorf("cannot request permanent TPM properties: %v", err)

	return fmt.Errorf("failed to clear the TPM: %v", err)
	return fmt.Errorf("cannot clear the TPM: %v", err)

	return fmt.Errorf("failed to open request handle: %v", err)
	return fmt.Errorf("cannot open TPM request handle %q: %v", ppiPath, err)

	return fmt.Errorf("failed to submit request: %v", err)
	return fmt.Errorf("cannot submit TPM request: %v", err)

	logger.Noticef("Creating new ubuntu-data")
	logger.Noticef("Creating new ubuntu-data partition")

Comments

Conversation

cmatsuoka commented Jul 9, 2019

Uh oh!

Uh oh!

zyga left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mvo5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mvo5 commented Jul 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants