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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 14 commits into from
Jul 15, 2020
Merged

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

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
dfeddea
gadget/install,vendor: use snapcore/secboot luks2 api
cmatsuoka Jun 8, 2020
cec9e58
gadget/install: update encryption tests
cmatsuoka Jun 22, 2020
3bf6e07
secboot: add tests for encryption functions using snapcore/secboot
cmatsuoka Jul 2, 2020
2779b55
secboot: add build constraints to files using snapcore/secboot
cmatsuoka Jul 2, 2020
adc23dc
Merge branch 'uc20-secboot-update-tpm-connection' into uc20-secboot-u…
cmatsuoka Jul 9, 2020
b29b950
Merge branch 'master' into uc20-secboot-use-luks-apis
cmatsuoka Jul 9, 2020
f3dee5f
gadget/install: only build encryption helpers if we have secboot
cmatsuoka Jul 9, 2020
593dfb9
Merge branch 'master' into uc20-secboot-use-luks-apis
cmatsuoka Jul 13, 2020
16ef4c0
gadget/install: rename newEncryptedDevice test errors
cmatsuoka Jul 13, 2020
abed949
gadget/install: also test node name in encrypted format mock
cmatsuoka Jul 13, 2020
aa7d453
gadget/install: move encryption and recovery keys to test suite
cmatsuoka Jul 13, 2020
7922057
gadget/install: rename AddRecoveryKey test errors
cmatsuoka Jul 13, 2020
c52045e
Merge branch 'master' into uc20-secboot-use-luks-apis
cmatsuoka Jul 14, 2020
242d288
vendor: update snapcore/secboot
cmatsuoka Jul 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 8 additions & 103 deletions gadget/install/encrypt.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// -*- Mode: Go; indent-tabs-mode: t -*-
// +build !nosecboot

/*
* Copyright (C) 2020 Canonical Ltd
Expand All @@ -23,12 +24,8 @@ import (
"bytes"
"fmt"
"io/ioutil"
"os"
"os/exec"
"path/filepath"
"syscall"

"github.com/snapcore/snapd/dirs"
"github.com/snapcore/snapd/gadget"
"github.com/snapcore/snapd/osutil"
"github.com/snapcore/snapd/secboot"
Expand All @@ -38,6 +35,11 @@ var (
tempFile = ioutil.TempFile
)

var (
secbootFormatEncryptedDevice = secboot.FormatEncryptedDevice
secbootAddRecoveryKey = secboot.AddRecoveryKey
)

// encryptedDevice represents a LUKS-backed encrypted block device.
type encryptedDevice struct {
parent *gadget.OnDiskStructure
Expand All @@ -57,7 +59,7 @@ func newEncryptedDevice(part *gadget.OnDiskStructure, key secboot.EncryptionKey,
Node: fmt.Sprintf("/dev/mapper/%s", name),
}

if err := cryptsetupFormat(key, name+"-enc", part.Node); err != nil {
if err := secbootFormatEncryptedDevice(key, name+"-enc", part.Node); err != nil {
return nil, fmt.Errorf("cannot format encrypted device: %v", err)
}

Expand All @@ -69,47 +71,13 @@ func newEncryptedDevice(part *gadget.OnDiskStructure, key secboot.EncryptionKey,
}

func (dev *encryptedDevice) AddRecoveryKey(key secboot.EncryptionKey, rkey secboot.RecoveryKey) error {
return cryptsetupAddKey(key, rkey, dev.parent.Node)
return secbootAddRecoveryKey(key, rkey, dev.parent.Node)
}

func (dev *encryptedDevice) Close() error {
return cryptsetupClose(dev.name)
}

func cryptsetupFormat(key secboot.EncryptionKey, label, node string) error {
// We use a keyfile with the same entropy as the derived key so we can
// keep the KDF iteration count to a minimum. Longer processing will not
// increase security in this case.
args := []string{
// batch processing, no password verification
"-q",
// formatting a new device
"luksFormat",
// use LUKS2
"--type", "luks2",
// read key from stdin
"--key-file", "-",
// use AES-256 with XTS block cipher mode (XTS requires 2 keys)
"--cipher", "aes-xts-plain64", "--key-size", "512",
// use --iter-time 1 with the default KDF argon2i so
// to do virtually no derivation, here key is a random
// key with good entropy, not a passphrase, so
// spending time deriving from it is not necessary or
// makes sense
"--pbkdf", "argon2i", "--iter-time", "1",
// set LUKS2 label
"--label", label,
// device to format
node,
}
cmd := exec.Command("cryptsetup", args...)
cmd.Stdin = bytes.NewReader(key[:])
if output, err := cmd.CombinedOutput(); err != nil {
return osutil.OutputErr(output, err)
}
return nil
}

func cryptsetupOpen(key secboot.EncryptionKey, node, name string) error {
cmd := exec.Command("cryptsetup", "open", "--key-file", "-", node, name)
cmd.Stdin = bytes.NewReader(key[:])
Expand All @@ -125,66 +93,3 @@ func cryptsetupClose(name string) error {
}
return nil
}

func cryptsetupAddKey(key secboot.EncryptionKey, rkey secboot.RecoveryKey, node string) error {
// create a named pipe to pass the recovery key
fpath := filepath.Join(dirs.SnapRunDir, "tmp-rkey")
if err := os.MkdirAll(dirs.SnapRunDir, 0755); err != nil {
return err
}
if err := syscall.Mkfifo(fpath, 0600); err != nil {
return fmt.Errorf("cannot create named pipe: %v", err)
}
defer os.RemoveAll(fpath)

// add a new key to slot 1 reading the passphrase from the named pipe
// (explicitly choose keyslot 1 to ensure we have a predictable slot
// number in case we decide to kill all other slots later)
args := []string{
// add a new key
"luksAddKey",
// the encrypted device
node,
// batch processing, no password verification
"-q",
// read existing key from stdin
"--key-file", "-",
// store it in keyslot 1
"--key-slot", "1",
// the named pipe
fpath,
}

cmd := exec.Command("cryptsetup", args...)
cmd.Stdin = bytes.NewReader(key[:])
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
if err := cmd.Start(); err != nil {
return err
}

// open the named pipe and write the recovery key
file, err := os.OpenFile(fpath, os.O_WRONLY, 0600)
if err != nil {
return fmt.Errorf("cannot open recovery key pipe: %v", err)
}
n, err := file.Write(rkey[:])
if n != len(rkey) {
file.Close()
return fmt.Errorf("cannot write recovery key: short write (%d bytes written)", n)
}
if err != nil {
cmd.Process.Kill()
file.Close()
return fmt.Errorf("cannot write recovery key: %v", err)
}
if err := file.Close(); err != nil {
cmd.Process.Kill()
return fmt.Errorf("cannot close recovery key pipe: %v", err)
}
if err := cmd.Wait(); err != nil {
return fmt.Errorf("cannot add recovery key: %v", err)
}

return nil
}
181 changes: 113 additions & 68 deletions gadget/install/encrypt_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@
package install_test

import (
"errors"
"fmt"
"os"
"path/filepath"

. "gopkg.in/check.v1"

Expand Down Expand Up @@ -58,78 +58,123 @@ func (s *encryptSuite) SetUpTest(c *C) {
c.Assert(os.MkdirAll(dirs.SnapRunDir, 0755), IsNil)
}

func (s *encryptSuite) TestEncryptHappy(c *C) {
s.mockCryptsetup = testutil.MockCommand(c, "cryptsetup", "")
s.AddCleanup(s.mockCryptsetup.Restore)

// create empty key to prevent blocking on lack of system entropy
key := secboot.EncryptionKey{}
dev, err := install.NewEncryptedDevice(&mockDeviceStructure, key, "some-label")
c.Assert(err, IsNil)
c.Assert(dev.Node, Equals, "/dev/mapper/some-label")

c.Assert(s.mockCryptsetup.Calls(), DeepEquals, [][]string{
func (s *encryptSuite) TestNewEncryptedDevice(c *C) {
for _, tc := range []struct {
formatErr error
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

openErr string
err string
}{
{
"cryptsetup", "-q", "luksFormat", "--type", "luks2", "--key-file", "-",
"--cipher", "aes-xts-plain64", "--key-size", "512", "--pbkdf", "argon2i",
"--iter-time", "1", "--label", "some-label-enc", "/dev/node1",
formatErr: nil,
openErr: "",
err: "",
},
{
"cryptsetup", "open", "--key-file", "-", "/dev/node1", "some-label",
formatErr: errors.New("format error"),
openErr: "",
err: "cannot format encrypted device: format error",
},
})

err = dev.Close()
c.Assert(err, IsNil)
}

func (s *encryptSuite) TestEncryptFormatError(c *C) {
s.mockCryptsetup = testutil.MockCommand(c, "cryptsetup", `[ "$2" == "luksFormat" ] && exit 127 || exit 0`)
s.AddCleanup(s.mockCryptsetup.Restore)

key := secboot.EncryptionKey{}
_, err := install.NewEncryptedDevice(&mockDeviceStructure, key, "some-label")
c.Assert(err, ErrorMatches, "cannot format encrypted device:.*")
}

func (s *encryptSuite) TestEncryptOpenError(c *C) {
s.mockCryptsetup = testutil.MockCommand(c, "cryptsetup", `[ "$1" == "open" ] && exit 127 || exit 0`)
s.AddCleanup(s.mockCryptsetup.Restore)

key := secboot.EncryptionKey{}
_, err := install.NewEncryptedDevice(&mockDeviceStructure, key, "some-label")
c.Assert(err, ErrorMatches, "cannot open encrypted device on /dev/node1:.*")
}

func (s *encryptSuite) TestEncryptAddKey(c *C) {
capturedFifo := filepath.Join(c.MkDir(), "captured-stdin")
s.mockCryptsetup = testutil.MockCommand(c, "cryptsetup", fmt.Sprintf(`[ "$1" == "luksAddKey" ] && cat %s/tmp-rkey > %s || exit 0`, dirs.SnapRunDir, capturedFifo))
s.AddCleanup(s.mockCryptsetup.Restore)

key := secboot.EncryptionKey{}
dev, err := install.NewEncryptedDevice(&mockDeviceStructure, key, "some-label")
c.Assert(err, IsNil)

rkey := secboot.RecoveryKey{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15}
err = dev.AddRecoveryKey(key, rkey)
c.Assert(err, IsNil)

c.Assert(s.mockCryptsetup.Calls(), DeepEquals, [][]string{
{
"cryptsetup", "-q", "luksFormat", "--type", "luks2", "--key-file", "-",
"--cipher", "aes-xts-plain64", "--key-size", "512", "--pbkdf", "argon2i",
"--iter-time", "1", "--label", "some-label-enc", "/dev/node1",
formatErr: nil,
openErr: "open error",
err: "cannot open encrypted device on /dev/node1: open error",
},
{
"cryptsetup", "open", "--key-file", "-", "/dev/node1", "some-label",
},
{
"cryptsetup", "luksAddKey", "/dev/node1", "-q", "--key-file", "-",
"--key-slot", "1", filepath.Join(dirs.SnapRunDir, "tmp-rkey"),
},
})
c.Assert(capturedFifo, testutil.FileEquals, rkey[:])
} {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

script := ""
if tc.openErr != "" {
script = fmt.Sprintf("echo '%s'>&2; exit 1", tc.openErr)

}
s.mockCryptsetup = testutil.MockCommand(c, "cryptsetup", script)
s.AddCleanup(s.mockCryptsetup.Restore)

// 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 := install.MockSecbootFormatEncryptedDevice(func(key secboot.EncryptionKey, label, node string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

We are not checking node here, why is that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, this was really missing, thanks Michael.

calls++
c.Assert(key, DeepEquals, myKey)
c.Assert(label, Equals, "some-label-enc")
return tc.formatErr
})
defer restore()

dev, err := install.NewEncryptedDevice(&mockDeviceStructure, myKey, "some-label")
c.Assert(calls, Equals, 1)
if tc.err == "" {
c.Assert(err, IsNil)
} else {
c.Assert(err, ErrorMatches, tc.err)
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

}
c.Assert(dev.Node, Equals, "/dev/mapper/some-label")

err = dev.Close()
c.Assert(err, IsNil)

c.Assert(s.mockCryptsetup.Calls(), DeepEquals, [][]string{
{"cryptsetup", "open", "--key-file", "-", "/dev/node1", "some-label"},
{"cryptsetup", "close", "some-label"},
})
}
}

err = dev.Close()
c.Assert(err, IsNil)
func (s *encryptSuite) TestAddRecoveryKey(c *C) {
for _, tc := range []struct {
addErr error
Copy link
Contributor

Choose a reason for hiding this comment

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

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

err string
}{
{addErr: nil, err: ""},
{addErr: errors.New("add key error"), err: "add key error"},
} {

Copy link
Contributor

Choose a reason for hiding this comment

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

Extra whitespace

s.mockCryptsetup = testutil.MockCommand(c, "cryptsetup", "")
s.AddCleanup(s.mockCryptsetup.Restore)

// create empty key to prevent blocking on lack of system entropy
myKey := secboot.EncryptionKey{}
for i := range myKey {
myKey[i] = byte(i)
}
myRKey := secboot.RecoveryKey{15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0}

restore := install.MockSecbootFormatEncryptedDevice(func(key secboot.EncryptionKey, label, node string) error {
return nil
})
defer restore()

calls := 0
restore = install.MockSecbootAddRecoveryKey(func(key secboot.EncryptionKey, rkey secboot.RecoveryKey, node string) error {
calls++
c.Assert(key, DeepEquals, myKey)
c.Assert(rkey, DeepEquals, myRKey)
c.Assert(node, Equals, "/dev/node1")
return tc.addErr
})
defer restore()

dev, err := install.NewEncryptedDevice(&mockDeviceStructure, myKey, "some-label")
c.Assert(err, IsNil)

err = dev.AddRecoveryKey(myKey, myRKey)
c.Assert(calls, Equals, 1)
if tc.err == "" {
c.Assert(err, IsNil)
} else {
c.Assert(err, ErrorMatches, tc.err)
continue
}

err = dev.Close()
c.Assert(err, IsNil)

c.Assert(s.mockCryptsetup.Calls(), DeepEquals, [][]string{
{"cryptsetup", "open", "--key-file", "-", "/dev/node1", "some-label"},
{"cryptsetup", "close", "some-label"},
})
}
}
17 changes: 17 additions & 0 deletions gadget/install/export_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"time"

"github.com/snapcore/snapd/gadget"
"github.com/snapcore/snapd/secboot"
)

var (
Expand Down Expand Up @@ -70,3 +71,19 @@ func MockEnsureNodesExist(f func(dss []gadget.OnDiskStructure, timeout time.Dura
ensureNodesExist = old
}
}

func MockSecbootFormatEncryptedDevice(f func(key secboot.EncryptionKey, label, node string) error) (restore func()) {
old := secbootFormatEncryptedDevice
secbootFormatEncryptedDevice = f
return func() {
secbootFormatEncryptedDevice = old
}
}

func MockSecbootAddRecoveryKey(f func(key secboot.EncryptionKey, rkey secboot.RecoveryKey, node string) error) (restore func()) {
old := secbootAddRecoveryKey
secbootAddRecoveryKey = f
return func() {
secbootAddRecoveryKey = old
}
}
Loading