Merge pull request #310 from chrisccoulson/efi-more-refactoring-to-in…

…troduce-preinstall-subpackage

efi: refactoring to prepare for the introduction of efi/preinstall.

`efi/preinstall` will depend on `efi` so it is necessary to break some cyclic dependencies, which I've done via the `efi/internal` package.

The most problematic dependency is that `efi/preinstall` needs to use the `efi.PCRProfileOption` interface which contains an unexported method that takes a pointer to the unexported `pcrProfileGenerator` type - a type that should remain unexported. This has been solved by adding a simple `internal.PCRProfileOptionVisitor` interface that is implemented by `pcrProfileGenerator` and can be consumed in both `efi` and `efi/preinstall` packages. This has simplified direct testing of types that implement the `efi.PCRProfileOption` interface as well.

On this note, this did highlight a couple of testing gaps - we had no direct tests for the `WithSignatureDBUpdates` and `WithShimSbatPolicyLatest` options. I've added tests for these, which found a bug in the way the branching for the quirk handling worked for `WithSignatureDBUpdates`.

As part of this, I did rename a couple of types to give them names I felt make more sense. Eg, `rootVarsCollector` is now `variableSetCollector`, to bring it more in line with the new `internal.VariableSet` interface. The `rootVarReader` type is now `initialVarReader`, but other than that, no other changes to this code has been made - there's just been a lot of moving of types to the new `efi/internal` package.

efi/efi.go

@@ -22,11 +22,7 @@ package efi
 import "github.com/canonical/go-tpm2"
 
 const (
-	platformFirmwarePCR tpm2.Handle = 0 // SRTM, POST BIOS, and Embedded Drivers
-	driversAndAppsPCR   tpm2.Handle = 2 // UEFI Drivers and UEFI Applications
-	bootManagerCodePCR  tpm2.Handle = 4 // Boot Manager Code and Boot Attempts PCR
-	secureBootPolicyPCR tpm2.Handle = 7 // Secure Boot Policy Measurements PCR
-	kernelConfigPCR     tpm2.Handle = 12
+	kernelConfigPCR tpm2.Handle = 12
 )
 
 // pcrFlags corresponds to a set of PCRs. This can only represent actual PCRs, it

efi/efi_test.go

@@ -33,6 +33,7 @@ import (
 	. "gopkg.in/check.v1"
 
 	. "github.com/snapcore/secboot/efi"
+	"github.com/snapcore/secboot/efi/internal"
 	"github.com/snapcore/secboot/internal/efitest"
 	"github.com/snapcore/secboot/internal/testutil"
 )
@@ -693,61 +694,79 @@ type efiSuite struct{}
 var _ = Suite(&efiSuite{})
 
 func (s *efiSuite) TestMakePcrFlags1(c *C) {
-	flags := MakePcrFlags(SecureBootPolicyPCR)
-	c.Check(flags, Equals, PcrFlags(1<<SecureBootPolicyPCR))
+	flags := MakePcrFlags(internal.SecureBootPolicyPCR)
+	c.Check(flags, Equals, PcrFlags(1<<internal.SecureBootPolicyPCR))
 }
 
 func (s *efiSuite) TestMakePcrFlags2(c *C) {
-	flags := MakePcrFlags(BootManagerCodePCR)
-	c.Check(flags, Equals, PcrFlags(1<<BootManagerCodePCR))
+	flags := MakePcrFlags(internal.BootManagerCodePCR)
+	c.Check(flags, Equals, PcrFlags(1<<internal.BootManagerCodePCR))
 }
 
 func (s *efiSuite) TestMakePcrFlags3(c *C) {
-	flags := MakePcrFlags(BootManagerCodePCR, SecureBootPolicyPCR)
-	c.Check(flags, Equals, PcrFlags(1<<BootManagerCodePCR|1<<SecureBootPolicyPCR))
+	flags := MakePcrFlags(internal.BootManagerCodePCR, internal.SecureBootPolicyPCR)
+	c.Check(flags, Equals, PcrFlags(1<<internal.BootManagerCodePCR|1<<internal.SecureBootPolicyPCR))
 }
 
 func (e *efiSuite) TestPcrFlagsPCRs1(c *C) {
-	flags := PcrFlags(1 << SecureBootPolicyPCR)
-	c.Check(flags.PCRs(), DeepEquals, tpm2.HandleList{SecureBootPolicyPCR})
+	flags := PcrFlags(1 << internal.SecureBootPolicyPCR)
+	c.Check(flags.PCRs(), DeepEquals, tpm2.HandleList{internal.SecureBootPolicyPCR})
 }
 
 func (e *efiSuite) TestPcrFlagsPCRs2(c *C) {
-	flags := PcrFlags(1 << BootManagerCodePCR)
-	c.Check(flags.PCRs(), DeepEquals, tpm2.HandleList{BootManagerCodePCR})
+	flags := PcrFlags(1 << internal.BootManagerCodePCR)
+	c.Check(flags.PCRs(), DeepEquals, tpm2.HandleList{internal.BootManagerCodePCR})
 }
 
 func (e *efiSuite) TestPcrFlagsPCRs3(c *C) {
-	flags := PcrFlags((1 << BootManagerCodePCR) | (1 << SecureBootPolicyPCR))
-	c.Check(flags.PCRs(), DeepEquals, tpm2.HandleList{BootManagerCodePCR, SecureBootPolicyPCR})
+	flags := PcrFlags((1 << internal.BootManagerCodePCR) | (1 << internal.SecureBootPolicyPCR))
+	c.Check(flags.PCRs(), DeepEquals, tpm2.HandleList{internal.BootManagerCodePCR, internal.SecureBootPolicyPCR})
 }
 
 func (e *efiSuite) TestPcrFlagsContains1(c *C) {
-	flags := PcrFlags(1 << SecureBootPolicyPCR)
-	c.Check(flags.Contains(SecureBootPolicyPCR), testutil.IsTrue)
+	flags := PcrFlags(1 << internal.SecureBootPolicyPCR)
+	c.Check(flags.Contains(internal.SecureBootPolicyPCR), testutil.IsTrue)
 }
 
 func (e *efiSuite) TestPcrFlagsContains2(c *C) {
-	flags := PcrFlags(1 << BootManagerCodePCR)
-	c.Check(flags.Contains(BootManagerCodePCR), testutil.IsTrue)
+	flags := PcrFlags(1 << internal.BootManagerCodePCR)
+	c.Check(flags.Contains(internal.BootManagerCodePCR), testutil.IsTrue)
 }
 
 func (e *efiSuite) TestPcrFlagsContains3(c *C) {
-	flags := PcrFlags((1 << BootManagerCodePCR) | (1 << SecureBootPolicyPCR))
-	c.Check(flags.Contains(BootManagerCodePCR, SecureBootPolicyPCR), testutil.IsTrue)
+	flags := PcrFlags((1 << internal.BootManagerCodePCR) | (1 << internal.SecureBootPolicyPCR))
+	c.Check(flags.Contains(internal.BootManagerCodePCR, internal.SecureBootPolicyPCR), testutil.IsTrue)
 }
 
 func (e *efiSuite) TestPcrFlagsContains4(c *C) {
-	flags := PcrFlags((1 << BootManagerCodePCR) | (1 << SecureBootPolicyPCR))
-	c.Check(flags.Contains(SecureBootPolicyPCR), testutil.IsTrue)
+	flags := PcrFlags((1 << internal.BootManagerCodePCR) | (1 << internal.SecureBootPolicyPCR))
+	c.Check(flags.Contains(internal.SecureBootPolicyPCR), testutil.IsTrue)
 }
 
 func (e *efiSuite) TestPcrFlagsDoesNotContain1(c *C) {
-	flags := PcrFlags(1 << SecureBootPolicyPCR)
-	c.Check(flags.Contains(PlatformFirmwarePCR), testutil.IsFalse)
+	flags := PcrFlags(1 << internal.SecureBootPolicyPCR)
+	c.Check(flags.Contains(internal.PlatformFirmwarePCR), testutil.IsFalse)
 }
 
 func (e *efiSuite) TestPcrFlagsDoesNotContain2(c *C) {
-	flags := PcrFlags(1 << SecureBootPolicyPCR)
-	c.Check(flags.Contains(PlatformFirmwarePCR, SecureBootPolicyPCR), testutil.IsFalse)
+	flags := PcrFlags(1 << internal.SecureBootPolicyPCR)
+	c.Check(flags.Contains(internal.PlatformFirmwarePCR, internal.SecureBootPolicyPCR), testutil.IsFalse)
+}
+
+type mockPcrProfileOptionVisitor struct {
+	pcrs         tpm2.HandleList
+	env          HostEnvironment
+	varModifiers []internal.InitialVariablesModifier
+}
+
+func (v *mockPcrProfileOptionVisitor) AddPCRs(pcrs ...tpm2.Handle) {
+	v.pcrs = append(v.pcrs, pcrs...)
+}
+
+func (v *mockPcrProfileOptionVisitor) SetEnvironment(env internal.HostEnvironment) {
+	v.env = env
+}
+
+func (v *mockPcrProfileOptionVisitor) AddInitialVariablesModifier(fn internal.InitialVariablesModifier) {
+	v.varModifiers = append(v.varModifiers, fn)
 }

efi/env.go

@@ -29,6 +29,7 @@ import (
 
 	efi "github.com/canonical/go-efilib"
 	"github.com/canonical/tcglog-parser"
+	"github.com/snapcore/secboot/efi/internal"
 	"golang.org/x/xerrors"
 )
 
@@ -54,8 +55,9 @@ func WithHostEnvironment(env HostEnvironment) PCRProfileOption {
 	return &hostEnvironmentOption{HostEnvironment: env}
 }
 
-func (o *hostEnvironmentOption) applyOptionTo(gen *pcrProfileGenerator) {
-	gen.env = o.HostEnvironment
+func (o *hostEnvironmentOption) ApplyOptionTo(visitor internal.PCRProfileOptionVisitor) error {
+	visitor.SetEnvironment(o.HostEnvironment)
+	return nil
 }
 
 // varReader is a subset of HostEnvironment that is just for EFI variables
@@ -64,6 +66,8 @@ type varReader interface {
 	ReadVar(name string, guid efi.GUID) ([]byte, efi.VariableAttributes, error)
 }
 
+// varReadWriter is an interface for reading and mock updating EFI variables during
+// profile generation.
 type varReadWriter interface {
 	varReader
 
@@ -74,29 +78,29 @@ type varReadWriter interface {
 // varUpdate is the sequence of updates generated by a profile branch, associated with
 // a varBranch.
 type varUpdate struct {
-	previous *varUpdate
-	name     efi.VariableDescriptor
-	attrs    efi.VariableAttributes
-	data     []byte
+	previous *varUpdate             // link to the previous update
+	name     efi.VariableDescriptor // the variable name associated with the update
+	attrs    efi.VariableAttributes // the corresponding attributes
+	data     []byte                 // the updated variable data
 }
 
 type varBranchRegisterUpdatesFn func(*varUpdate) error
 
-// varBranch corresponds to a EFI variable state associated with a profile branch,
+// varBranch corresponds to a EFI variable set associated with a profile branch,
 // consisting of an initial starting environment and a sequence of updates that have been
 // added during profile generation. Each profile generation loop starts with one of these
 // without any updates. Branches in a profile inherit a copy of this from the parent branch
 // and may make modifications to EFI variables (eg, applying a SBAT update) which may affect
 // other branches - in this case, the profile generation may be re-executed multiple times
-// with different starting states as a result of these updates. These starting states are
-// computed by the assocated rootVarsCollector.
+// with different initial sets as a result of these updates. These starting sets are
+// computed and tracked by the assocated variableSetCollector.
 type varBranch struct {
 	initial         varReader  // the initial starting environment
 	updates         *varUpdate // the updates applied by the associated branch
 	registerUpdates varBranchRegisterUpdatesFn
 }
 
-// ReadVar implements varReader.ReadVar.
+// ReadVar implements varReader.ReadVar and internal.VariableSet.ReadVar.
 func (s *varBranch) ReadVar(name string, guid efi.GUID) ([]byte, efi.VariableAttributes, error) {
 	update := s.updates
 	for ; update != nil; update = update.previous {
@@ -108,7 +112,9 @@ func (s *varBranch) ReadVar(name string, guid efi.GUID) ([]byte, efi.VariableAtt
 	return s.initial.ReadVar(name, guid)
 }
 
-// WriteVar implements varReadWriter.WriteVar.
+// WriteVar implements varReadWriter.WriteVar and internal.VariableSet.WriteVar.
+// Calling this creates and registers an update which may create a new initial
+// starting variable set for another profile generation loop.
 func (s *varBranch) WriteVar(name string, guid efi.GUID, attrs efi.VariableAttributes, data []byte) error {
 	orig, _, err := s.ReadVar(name, guid)
 	switch {
@@ -130,26 +136,34 @@ func (s *varBranch) WriteVar(name string, guid efi.GUID, attrs efi.VariableAttri
 	return s.registerUpdates(s.updates)
 }
 
-// rootVarReaderKey is a SHA1 used to uniquely identify the contents of a rootVarReader.
+// Clone implements internal.VariableSet.Clone. It returns an exact copy of this
+// varBranch to enable callers to WriteVar to create multiple branches with different
+// write sequences.
+func (s *varBranch) Clone() internal.VariableSet {
+	clone := *s
+	return &clone
+}
+
+// initialVarReaderKey is a SHA1 used to uniquely identify the contents of a initialVarReader.
 // This is to ensure that we don't generate multiple profile branches with the same
 // starting state.
-type rootVarReaderKey [sha1.Size]byte
+type initialVarReaderKey [sha1.Size]byte
 
 type varContents struct {
 	attrs efi.VariableAttributes
 	data  []byte
 }
 
-// rootVarReader provides the initial state for each profile generation branch. It
-// consists of the host environment and updates that have been generated by previous
+// initialVarReader provides the initial starting set for each profile generation branch.
+// It consists of the host environment and updates that have been generated by previous
 // iterations of the profile generation.
-type rootVarReader struct {
+type initialVarReader struct {
 	host      varReader
 	overrides map[efi.VariableDescriptor]varContents
 }
 
 // ReadVar implements varReader.ReadVar
-func (r *rootVarReader) ReadVar(name string, guid efi.GUID) ([]byte, efi.VariableAttributes, error) {
+func (r *initialVarReader) ReadVar(name string, guid efi.GUID) ([]byte, efi.VariableAttributes, error) {
 	override, exists := r.overrides[efi.VariableDescriptor{Name: name, GUID: guid}]
 	if exists {
 		return override.data, override.attrs, nil
@@ -160,7 +174,7 @@ func (r *rootVarReader) ReadVar(name string, guid efi.GUID) ([]byte, efi.Variabl
 // Key returns the unique key for this reader, and is based on the set of updates on the
 // original host environment. This is used to track which initial states have been handled
 // already for a profile.
-func (r *rootVarReader) Key() rootVarReaderKey {
+func (r *initialVarReader) Key() initialVarReaderKey {
 	// Ensure that this is stable by building an ordered list
 	var descs []efi.VariableDescriptor
 	for desc := range r.overrides {
@@ -172,7 +186,7 @@ func (r *rootVarReader) Key() rootVarReaderKey {
 
 	// Start with an empty key which corresponds to the original
 	// host environment.
-	var key rootVarReaderKey
+	var key initialVarReaderKey
 
 	for _, desc := range descs {
 		override := r.overrides[desc]
@@ -191,9 +205,9 @@ func (r *rootVarReader) Key() rootVarReaderKey {
 	return key
 }
 
-// Copy returns a copy of this rootVarReader.
-func (r *rootVarReader) Copy() *rootVarReader {
-	out := &rootVarReader{
+// Copy returns a copy of this initialVarReader.
+func (r *initialVarReader) Copy() *initialVarReader {
+	out := &initialVarReader{
 		host:      r.host,
 		overrides: make(map[efi.VariableDescriptor]varContents)}
 	for k, v := range r.overrides {
@@ -202,8 +216,8 @@ func (r *rootVarReader) Copy() *rootVarReader {
 	return out
 }
 
-// ApplyUpdates applies the specified sequence of updates to this rootVarReader.
-func (r *rootVarReader) ApplyUpdates(updates *varUpdate) error {
+// ApplyUpdates applies the specified sequence of updates to this initialVarReader.
+func (r *initialVarReader) ApplyUpdates(updates *varUpdate) error {
 	// arrange in order of application from oldest to newest
 	var updateSlice []*varUpdate
 	for ; updates != nil; updates = updates.previous {
@@ -227,28 +241,27 @@ func (r *rootVarReader) ApplyUpdates(updates *varUpdate) error {
 	return nil
 }
 
-// rootVarsCollector keeps track of all of the starting EFI variable states that
+// variableSetCollector keeps track of all of the starting EFI variable sets that
 // a profile needs to be generated against. The profile generation runs an outer
-// loop that adds a branch for each of the starting states. Profile generation may
+// loop that adds a branch for each of the starting sets. Profile generation may
 // add more starting states as some branches have paths that update EFI variables
 // (such as applying SBAT updates) which may have an effect on other branches.
-type rootVarsCollector struct {
+type variableSetCollector struct {
 	host varReader // the externally supplied host environment
 
-	seen map[rootVarReaderKey]struct{} // known starting states
+	seen map[initialVarReaderKey]struct{} // known starting states
 
-	// todo contains a list of starting states that a profile branch still
-	// needs to be generated for. When this is empty, pendingUpdates is
-	// drained in order to try to create more.
-	todo []*rootVarReader
+	// todo contains a list of starting sets that a profile branch still
+	// needs to be generated for.
+	todo []*initialVarReader
 }
 
-func newRootVarsCollector(vars varReader) *rootVarsCollector {
-	return &rootVarsCollector{
+func newVariableSetCollector(vars varReader) *variableSetCollector {
+	return &variableSetCollector{
 		host: vars,
-		seen: map[rootVarReaderKey]struct{}{rootVarReaderKey{}: struct{}{}}, // add the current environment
-		todo: []*rootVarReader{
-			&rootVarReader{
+		seen: map[initialVarReaderKey]struct{}{initialVarReaderKey{}: struct{}{}}, // add the current environment
+		todo: []*initialVarReader{
+			&initialVarReader{
 				host:      vars,
 				overrides: make(map[efi.VariableDescriptor]varContents)}}} // add the current environment
 }
@@ -258,54 +271,53 @@ func newRootVarsCollector(vars varReader) *rootVarsCollector {
 // processed. This should be called on every update - this ensures that if a branch
 // makes more than one update, the generated profile will be valid for intermediate
 // states.
-func (c *rootVarsCollector) registerUpdatesFor(initial *rootVarReader, updates *varUpdate) error {
-	newRoot := initial.Copy()
-	if err := newRoot.ApplyUpdates(updates); err != nil {
+func (c *variableSetCollector) registerUpdatesFor(initial *initialVarReader, updates *varUpdate) error {
+	newInitial := initial.Copy()
+	if err := newInitial.ApplyUpdates(updates); err != nil {
 		return xerrors.Errorf("cannot compute updated starting state: %w", err)
 	}
 
-	key := newRoot.Key()
+	key := newInitial.Key()
 	if _, exists := c.seen[key]; exists {
 		// we've already generated this starting state
 		return nil
 	}
 
 	c.seen[key] = struct{}{}
-	c.todo = append(c.todo, newRoot)
+	c.todo = append(c.todo, newInitial)
 
 	return nil
 }
 
-func (c *rootVarsCollector) newVarBranch(root *rootVarReader) *varBranch {
+func (c *variableSetCollector) newVarBranch(root *initialVarReader) *varBranch {
 	return &varBranch{
 		initial: root,
 		registerUpdates: func(updates *varUpdate) error {
 			return c.registerUpdatesFor(root, updates)
 		}}
 }
 
-// More indicates that there are more starting states to generate profile
+// More indicates that there are more starting sets to generate profile
 // branches for.
-func (c *rootVarsCollector) More() bool {
+func (c *variableSetCollector) More() bool {
 	return len(c.todo) > 0
 }
 
-// Next returns the next starting state to generate a profile branch for. This will
-// not return the same starting state more than once.
-func (c *rootVarsCollector) Next() *varBranch {
+// Next returns the next starting set to generate a profile branch for. This will
+// not return the same starting set more than once.
+func (c *variableSetCollector) Next() *varBranch {
 	next := c.todo[0]
 	c.todo = c.todo[1:]
 	return c.newVarBranch(next)
 }
 
-// PeekAll returns all of the pending starting states to generate profile
-// branches for.
-func (c *rootVarsCollector) PeekAll() []*varBranch {
+// PeekAll returns all of the pending starting sets to generate profile
+// branches for, without consuming them. It allows them to be added to
+// with options that run before the profile generation.
+func (c *variableSetCollector) PeekAll() []*varBranch {
 	var out []*varBranch
 	for _, r := range c.todo {
 		out = append(out, c.newVarBranch(r))
 	}
 	return out
 }
-
-type rootVarsModifier func(*rootVarsCollector) error