Merge pull request #312 from chrisccoulson/efi-mock-vars-backend-with…

…-hostenv

efi: change how HostEnvironment overrides EFI variables.

The `HostEnvironment` interface contains a `ReadVar` method for abstracting
reads of EFI variables. The default environment just calls
`efi.ReadVariable`, with other implementations providing mocked variables.

The `github.com/canonical/go-efilib` package provides a lot more APIs now
that read from (and some that write to) EFI variables and some of these
will be used from the new `efi/preinstall` package (these APIs are mostly
boot and secure boot related). I wanted a way to be able to override the
environment for the `preinstall.RunChecks` call (particularly as this is
very useful for testing), but the existing `HostEnvironment` approach doesn't
work well for this.

I initially considered a couple of options:
- Make it possible to mock each of the individual calls to go-efilib
  functions via the `HostEnvironment` interface.
- Export `varsBackend` from go-efilib and provide a function in to mock
  the backend by overriding the system one.

The first approach scales poorly, so my initial approach was going to be
the second one. The problem with this though is that `HostEnvironment`
will be part of a public, production API, and mocking the backend is global
to the whole process. I don't think this is appropriate in production code
where it might be possible that there may be one goroutine reading EFI
variables from the system when another goroutine decides to mock the backend
based on a supplied `HostEnvironment`.

I've taken a different approach instead, by modifying every go-efilib
function that interacts with EFI variables to accept a `context.Context`
argument. The context contains a `VarsBackend` keyed from it, and the package
exports a `DefaultVarContext` symbol corresponding to the default system
backend (`efivarfs` if it is detected, or a null backend if it isn't
detected). I've used a context rather than passing the `VarsBackend` directly
because the default efivarfs backend may make use of the optional
cancellation and deadline during writing, as it runs a retry loop that potentially
races with other applications.

This approach permits individual call sites to override the backend to
use for individual functions that interact with EFI variables (not just
`ReadVariable`, `WriteVariable` and `ListVariables`, but there's a whole set
of extra functions related to secure boot and boot configuration that
make use of reading and writing EFI variables).

The `ReadVar` method on the `HostEnvironment` interface has gone, and a new
method that returns a context has been added. The `internal.DefaultEnv`
implementation just returns `efi.DefaultVarContext`.

The `MockVars` type in `internal/efitest` has been updated to implement
`efi.VarsBackend`, and the `MockHostEnvironment` implementation in
`internal/efitest` returns a context that keys to this implementation.

This should hopefully allow `preinstall.RunChecks` to be able to take a
`HostEnvironment` argument and benefit from mocked variables even when
interacting with variables indirectly using higher level functions
exported from go-efilib.

efi/efi_test.go

@@ -21,6 +21,7 @@ package efi_test
 
 import (
 	"bytes"
+	"context"
 	"crypto"
 	"crypto/x509"
 	"errors"
@@ -770,3 +771,15 @@ func (v *mockPcrProfileOptionVisitor) SetEnvironment(env internal.HostEnvironmen
 func (v *mockPcrProfileOptionVisitor) AddInitialVariablesModifier(fn internal.InitialVariablesModifier) {
 	v.varModifiers = append(v.varModifiers, fn)
 }
+
+type mockVarReader struct {
+	ctx context.Context
+}
+
+func newMockVarReader(env HostEnvironment) *mockVarReader {
+	return &mockVarReader{ctx: env.VarContext(context.Background())}
+}
+
+func (r *mockVarReader) ReadVar(name string, guid efi.GUID) ([]byte, efi.VariableAttributes, error) {
+	return efi.ReadVariable(r.ctx, name, guid)
+}

efi/env.go

@@ -21,6 +21,7 @@ package efi
 
 import (
 	"bytes"
+	"context"
 	"crypto"
 	"crypto/sha1"
 	"encoding/binary"
@@ -37,8 +38,15 @@ import (
 // consumers of the API can provide a custom mechanism to read EFI variables or parse
 // the TCG event log.
 type HostEnvironment interface {
-	// ReadVar reads the specified EFI variable
-	ReadVar(name string, guid efi.GUID) ([]byte, efi.VariableAttributes, error)
+	// VarContext returns a copy of parent containing a VarsBackend, keyed by efi.VarsBackendKey,
+	// for interacting with EFI variables via go-efilib. This context can be passed to any
+	// go-efilib function that interacts with EFI variables. Right now, go-efilib doesn't
+	// support any other uses of the context such as cancelation or deadlines. The efivarfs
+	// backend will support this eventually for variable writes because it currently implements
+	// a retry loop that has a potential to race with other processes. Cancelation and / or
+	// deadlines for sections of code that performs multiple reads using efivarfs may be useful
+	// in the future because the kernel rate-limits reads per-user.
+	VarContext(parent context.Context) context.Context
 
 	// ReadEventLog reads the TCG event log
 	ReadEventLog() (*tcglog.Log, error)
@@ -60,7 +68,8 @@ func (o *hostEnvironmentOption) ApplyOptionTo(visitor internal.PCRProfileOptionV
 	return nil
 }
 
-// varReader is a subset of HostEnvironment that is just for EFI variables
+// varReader is an interface that provides access to reading EFI variables during
+// profile generation.
 type varReader interface {
 	// ReadVar reads the specified EFI variable
 	ReadVar(name string, guid efi.GUID) ([]byte, efi.VariableAttributes, error)
@@ -158,7 +167,7 @@ type varContents struct {
 // It consists of the host environment and updates that have been generated by previous
 // iterations of the profile generation.
 type initialVarReader struct {
-	host      varReader
+	varsCtx   context.Context
 	overrides map[efi.VariableDescriptor]varContents
 }
 
@@ -168,7 +177,7 @@ func (r *initialVarReader) ReadVar(name string, guid efi.GUID) ([]byte, efi.Vari
 	if exists {
 		return override.data, override.attrs, nil
 	}
-	return r.host.ReadVar(name, guid)
+	return efi.ReadVariable(r.varsCtx, name, guid)
 }
 
 // Key returns the unique key for this reader, and is based on the set of updates on the
@@ -208,7 +217,7 @@ func (r *initialVarReader) Key() initialVarReaderKey {
 // Copy returns a copy of this initialVarReader.
 func (r *initialVarReader) Copy() *initialVarReader {
 	out := &initialVarReader{
-		host:      r.host,
+		varsCtx:   r.varsCtx,
 		overrides: make(map[efi.VariableDescriptor]varContents)}
 	for k, v := range r.overrides {
 		out.overrides[k] = v
@@ -226,7 +235,7 @@ func (r *initialVarReader) ApplyUpdates(updates *varUpdate) error {
 
 	for _, update := range updateSlice {
 		r.overrides[update.name] = varContents{attrs: update.attrs, data: update.data}
-		origData, _, err := r.host.ReadVar(update.name.Name, update.name.GUID)
+		origData, _, err := efi.ReadVariable(r.varsCtx, update.name.Name, update.name.GUID)
 		switch {
 		case err == efi.ErrVarNotExist:
 			// ok
@@ -247,7 +256,7 @@ func (r *initialVarReader) ApplyUpdates(updates *varUpdate) error {
 // 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 variableSetCollector struct {
-	host varReader // the externally supplied host environment
+	varsCtx context.Context // a context containing the backend for reading EFI variables
 
 	seen map[initialVarReaderKey]struct{} // known starting states
 
@@ -256,17 +265,28 @@ type variableSetCollector struct {
 	todo []*initialVarReader
 }
 
-func newVariableSetCollector(vars varReader) *variableSetCollector {
+func newVariableSetCollector(env HostEnvironment) *variableSetCollector {
+	// We pass the TODO context here for now, as the context just essentially
+	// a container for holding the variable backend, so there's no need for
+	// the caller to override it.
+	//
+	// In the future, the efivarfs backend will make use of cancellation and / or
+	// deadlines it because it runs a retry loop that can race with other processes,
+	// although this package is doing no writing. Cancellation and / or deadlines
+	// may also still be useful in the read path especially where a section of code
+	// performs multiple reads via efivarfs - these reads are rate-limitted per-user
+	// in the kernel.
+	varsCtx := env.VarContext(context.TODO())
 	return &variableSetCollector{
-		host: vars,
-		seen: map[initialVarReaderKey]struct{}{initialVarReaderKey{}: struct{}{}}, // add the current environment
+		varsCtx: varsCtx,
+		seen:    map[initialVarReaderKey]struct{}{initialVarReaderKey{}: struct{}{}}, // add the current environment
 		todo: []*initialVarReader{
 			&initialVarReader{
-				host:      vars,
+				varsCtx:   varsCtx,
 				overrides: make(map[efi.VariableDescriptor]varContents)}}} // add the current environment
 }
 
-// registerUpdateFor is called when a branch updates an EFI variable. This will
+// registerUpdateFor is called when a branch updxates an EFI variable. This will
 // queue new starting states to process if the updated state hasn't already been
 // 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

efi/export_test.go

@@ -20,6 +20,8 @@
 package efi
 
 import (
+	"context"
+
 	efi "github.com/canonical/go-efilib"
 	"github.com/canonical/tcglog-parser"
 	"github.com/snapcore/secboot/efi/internal"
@@ -192,7 +194,7 @@ func MockSnapdenvTesting(testing bool) (restore func()) {
 
 func NewInitialVarReader(host HostEnvironment) *initialVarReader {
 	return &initialVarReader{
-		host:      host,
+		varsCtx:   host.VarContext(context.TODO()),
 		overrides: make(map[efi.VariableDescriptor]varContents)}
 }
 

efi/internal/default_env.go

@@ -20,6 +20,7 @@
 package internal
 
 import (
+	"context"
 	"os"
 
 	efi "github.com/canonical/go-efilib"
@@ -28,15 +29,16 @@ import (
 
 var (
 	eventLogPath = "/sys/kernel/security/tpm0/binary_bios_measurements" // Path of the TCG event log for the default TPM, in binary form
-	readVar      = efi.ReadVariable
 )
 
 type defaultEnvImpl struct{}
 
-func (e defaultEnvImpl) ReadVar(name string, guid efi.GUID) ([]byte, efi.VariableAttributes, error) {
-	return readVar(name, guid)
+// VarContext implements [HostEnvironment.VarContext].
+func (e defaultEnvImpl) VarContext(parent context.Context) context.Context {
+	return efi.WithDefaultVarsBackend(parent)
 }
 
+// ReadEventLog implements [HostEnvironment.ReadEventLog].
 func (e defaultEnvImpl) ReadEventLog() (*tcglog.Log, error) {
 	f, err := os.Open(eventLogPath)
 	if err != nil {

efi/internal/default_env_test.go

@@ -20,6 +20,7 @@
 package internal_test
 
 import (
+	"context"
 	_ "embed"
 	"io"
 	"os"
@@ -35,86 +36,26 @@ import (
 	. "gopkg.in/check.v1"
 )
 
-var (
-	//go:embed MicrosoftKEK.crt
-	msKEKCertPEM []byte
-
-	msKEKCert []byte
-)
-
-func init() {
-	msKEKCert = testutil.MustDecodePEMType("CERTIFICATE", msKEKCertPEM)
-}
-
 type defaultEnvSuite struct{}
 
 var _ = Suite(&defaultEnvSuite{})
 
-type testReadVarData struct {
-	name string
-	guid efi.GUID
-}
-
-func (s *defaultEnvSuite) testReadVar(c *C, data *testReadVarData) {
-	ownerGuid := efi.MakeGUID(0x77fa9abd, 0x0359, 0x4d32, 0xbd60, [...]uint8{0x28, 0xf4, 0xe7, 0x8f, 0x78, 0x4b})
-	kek := &efi.SignatureList{
-		Type: efi.CertX509Guid,
-		Signatures: []*efi.SignatureData{
-			{
-				Owner: ownerGuid,
-				Data:  msKEKCert,
-			},
-		},
-	}
-	dbx := efitest.NewSignatureListNullSHA256(ownerGuid)
-	vars := efitest.MakeMockVars()
-	vars.SetSecureBoot(true)
-	vars.SetKEK(c, efi.SignatureDatabase{kek})
-	vars.SetDbx(c, efi.SignatureDatabase{dbx})
-
-	restore := MockReadVar(func(name string, guid efi.GUID) ([]byte, efi.VariableAttributes, error) {
-		entry, exists := vars[efi.VariableDescriptor{Name: name, GUID: guid}]
-		if !exists {
-			return nil, 0, efi.ErrVarNotExist
-		}
-		return entry.Payload, entry.Attrs, nil
-	})
-	defer restore()
-
-	payload, attrs, err := DefaultEnv.ReadVar(data.name, data.guid)
-
-	entry, exists := vars[efi.VariableDescriptor{Name: data.name, GUID: data.guid}]
-	if !exists {
-		c.Check(err, Equals, efi.ErrVarNotExist)
-	} else {
-		c.Check(err, IsNil)
-		c.Check(attrs, Equals, entry.Attrs)
-		c.Check(payload, DeepEquals, entry.Payload)
-	}
-}
-
-func (s *defaultEnvSuite) TestReadVar1(c *C) {
-	s.testReadVar(c, &testReadVarData{
-		name: "SecureBoot",
-		guid: efi.GlobalVariable})
-}
+type testKey struct{}
 
-func (s *defaultEnvSuite) TestReadVar2(c *C) {
-	s.testReadVar(c, &testReadVarData{
-		name: "KEK",
-		guid: efi.GlobalVariable})
-}
+func (s *defaultEnvSuite) TestVarContext(c *C) {
+	ctx := DefaultEnv.VarContext(context.WithValue(context.Background(), testKey{}, int64(10)))
+	c.Assert(ctx, NotNil)
 
-func (s *defaultEnvSuite) TestReadVar3(c *C) {
-	s.testReadVar(c, &testReadVarData{
-		name: "dbx",
-		guid: efi.ImageSecurityDatabaseGuid})
-}
+	expected := efi.WithDefaultVarsBackend(context.Background())
+	c.Check(ctx.Value(efi.VarsBackendKey{}), Equals, expected.Value(efi.VarsBackendKey{}))
 
-func (s *defaultEnvSuite) TestReadVarNotExist(c *C) {
-	s.testReadVar(c, &testReadVarData{
-		name: "SecureBoot",
-		guid: efi.ImageSecurityDatabaseGuid})
+	// Make sure that the returned context has the right parent by testing the
+	// value we attached to it.
+	testVal := ctx.Value(testKey{})
+	c.Assert(testVal, NotNil)
+	testVali64, ok := testVal.(int64)
+	c.Assert(ok, testutil.IsTrue)
+	c.Check(testVali64, Equals, int64(10))
 }
 
 func (s *defaultEnvSuite) testReadEventLog(c *C, opts *efitest.LogOptions) {

efi/internal/env.go

@@ -0,0 +1,44 @@
+// -*- Mode: Go; indent-tabs-mode: t -*-
+
+/*
+ * Copyright (C) 2024 Canonical Ltd
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+package internal
+
+import (
+	"context"
+
+	"github.com/canonical/tcglog-parser"
+)
+
+// HostEnvironment is an interface that abstracts out an EFI environment, so that
+// consumers of the API can provide a custom mechanism to read EFI variables or parse
+// the TCG event log. This needs to be kept in sync with [efi.HostEnvironment].
+type HostEnvironment interface {
+	// VarContext returns a copy of parent containing a VarsBackend, keyed by efi.VarsBackendKey,
+	// for interacting with EFI variables via go-efilib. This context can be passed to any
+	// go-efilib function that interacts with EFI variables. Right now, go-efilib doesn't
+	// support any other uses of the context such as cancelation or deadlines. The efivarfs
+	// backend will support this eventually for variable writes because it currently implements
+	// a retry loop that has a potential to race with other processes. Cancelation and / or
+	// deadlines for sections of code that performs multiple reads using efivarfs may be useful
+	// in the future because the kernel rate-limits reads per-user.
+	VarContext(parent context.Context) context.Context
+
+	// ReadEventLog reads the TCG event log
+	ReadEventLog() (*tcglog.Log, error)
+}

efi/internal/export_test.go

@@ -19,20 +19,10 @@
 
 package internal
 
-import efi "github.com/canonical/go-efilib"
-
 func MockEventLogPath(path string) (restore func()) {
 	origPath := eventLogPath
 	eventLogPath = path
 	return func() {
 		eventLogPath = origPath
 	}
 }
-
-func MockReadVar(fn func(string, efi.GUID) ([]byte, efi.VariableAttributes, error)) (restore func()) {
-	origReadVar := readVar
-	readVar = fn
-	return func() {
-		readVar = origReadVar
-	}
-}

efi/internal/options.go

@@ -22,7 +22,6 @@ package internal
 import (
 	efi "github.com/canonical/go-efilib"
 	"github.com/canonical/go-tpm2"
-	"github.com/canonical/tcglog-parser"
 )
 
 type InitialVariablesModifier func(VariableSet) error
@@ -39,17 +38,6 @@ type PCRProfileOptionVisitor interface {
 	AddInitialVariablesModifier(fn InitialVariablesModifier)
 }
 
-// HostEnvironment is an interface that abstracts out an EFI environment, so that
-// consumers of the API can provide a custom mechanism to read EFI variables or parse
-// the TCG event log. This needs to be kept in sync with [efi.HostEnvironment].
-type HostEnvironment interface {
-	// ReadVar reads the specified EFI variable
-	ReadVar(name string, guid efi.GUID) ([]byte, efi.VariableAttributes, error)
-
-	// ReadEventLog reads the TCG event log
-	ReadEventLog() (*tcglog.Log, error)
-}
-
 // VariableSet corresponds to a set of EFI variables.
 type VariableSet interface {
 	// ReadVar reads the specified EFI variable for this set.