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 not 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