Refactor GetEventLog

[alexmwu]

Currently, we have a confusing interface for GetEventLog. In order to take advantage of this model, we need to wrap the EventLog method in a new structure whenever we pass the simulator as an embedded field to a test. See #128, specifically ignoreClose and ignoreCloseWithEventLogGetter as an example. We do this because the event log and the TPM are closely tied in a simulated environment; the TPM's PCRs need to reflect the event log events to pass our tests.

To prevent this, we should figure out how best to

Here are a few options to consider:

1. Refactor our testing to have a test.GetTPMAndEventLog replace test.GetTPM. This can return some TPMAndEventLog structure that tests can optionally take one or both of. We then need to revisit client.Attest and probably introduce an eventLog []byte parameter again that calls GetEventLog when nil.

• this option makes Attest less usable. Users of the library should not have to separately fetch the event log. There are well known ways to fetch on Linux (/sys/kernel/security/tpm0/binary_bios_measurements) and Windows (https://docs.microsoft.com/en-us/windows/win32/api/tbs/nf-tbs-tbsi_get_tcg_log). So, we should do it for them.

2. Create some wrapper interface around both TPM and EventLog (for lack of a better name, call it TPMContext). It could look something like type TPMContext interface { io.ReadWriterCloser EventLog() []byte

• option 2 allows us to be more flexible with how we implement and tie together RWC and EventLog. This would require a more significant refactor and is a breaking change.

3. Introduce some attestHelper and only test that, leaving the event log for the test to provide.

• This is brittle and doesn't test the actual interface we provide.

Thoughts?

[josephlr]

Discussed offline with Alex, for command tests we should probably switch to doing something like:

// in cmd/open.go
var openTPM func() (io.ReadWriteCloser, error) = openImpl

// in cmd/seal.go
	tpm, err := openTPM()

// in cmd/seal_test.go
func TestSealPlain(t *testing.T) {
	// Before:
	// rwc := test.GetTPM(t)
	// defer client.CheckedClose(t, rwc)
	// ExternalTPM = rwc
	// After:
	openTPM = func() { test.GetTPM(t) }
	...
}

and just not have ExternalTpm at all.

[josephlr]

We should also make the GetEventLog interface definition private

[josephlr]

Short-Term: go-tpm should define a TPM interface that's basically:

interface TPM {
	io.ReadWriteCloser
	EventLog() ([]byte, error)
}

And tpm2.OpenTPM should return a TPM (instead of an io.ReadWriteCloser. This is technically a breaking change due to issues discussed in google/go-tpm#256 (comment), but will not break too many users.

Then client.GetEventLog mostly just handles interface casing for the user:

package client

import (
	"io"
	"github.com/google/go-tpm/tpm2"
)

func GetEventLog(rw io.ReadWriter) ([]byte, error) {
	if tpm, ok := rw.(tpm2.TPM); ok {
		return tpm.EventLog()
	}
	// Fallback only used on Linux
	return getRealEventLog()
}

[alexmwu]

Discussed offline with Alex, for command tests we should probably switch to doing something like:

// in cmd/open.go
var openTPM func() (io.ReadWriteCloser, error) = openImpl

// in cmd/seal.go
	tpm, err := openTPM()

// in cmd/seal_test.go
func TestSealPlain(t *testing.T) {
	// Before:
	// rwc := test.GetTPM(t)
	// defer client.CheckedClose(t, rwc)
	// ExternalTPM = rwc
	// After:
	openTPM = func() { test.GetTPM(t) }
	...
}

and just not have ExternalTpm at all.

The problem with this is our cmd tests need a reference to the same TPM instance as the cmd execution context run in that test. See https://github.com/google/go-tpm-tools/blob/master/cmd/seal_test.go#L94-L97 as an example. That test uses the output of test.GetTPM(t) to extend to PCRs. Basically, any test that requires the a handle to the TPM won't be possible as (like the AttestTest that also needs the TPM to get a ref to the AK).

I can't think of a way around this without having a global variable like ExternalTPM without somehow getting the cmd to return a ref to the TPM (which doesn't make sense). I will go ahead and create a TPM interface in go-tpm, but it looks like the current hacky fix for ignoreClose will stay for now.

[alexmwu]

#302