Use generics to simplify the TPMDirect interface

[chrisfenner]

This is unfortunately a large change, but I think it does a lot for the ergonomics of the TPMDirect API.

This change uses the new Go 1.18 generics to solve a few problems:

• We want people to be able to provide a flat []byte or actual structure when instantiating TPM2Bs
• We want to avoid people directly manipulating pointer values in the TPMUs or having their TPMUs in an invalid state
• We want a nice Marshal and Unmarshal function (and later, to be able to make a nice Compare function, see Add a Compare function #309 )

Generics to the rescue. Here's what this PR does:

• Add a new file called marshalling.go that handles a lot of the high level marshalling work. reflect.go is still the dirty reflection guts of the library
• Embed a new type called marshalByReflection into all the structs that can be marshalled by reflection, as a clear hint to the reflection library
• Add a new interface called UnmarshallableWithHint - most of the TPMU implement this, and the old marshalUnion and unmarshalUnion functions are gone now
• Bonus: I noticed using profiling that the tags function was allocating several orders of magnitude more memory than the rest of the library, so I rewrote it
• Introduced a generic TPM2B helper that is aliased by the concrete TPM2B types; there are constructors for instantiating TPM2B from data or structured contents.
• TPMU is public, with private fields. Introduced constructors for these with type constraints.

Fixes #307 and #292.

[chrisfenner]

@alexmwu @josephlr Hi all, sorry this sat for so long. I finally made some time to work on the feedback from our verbal review. I've deleted generics from where they weren't absolutely necessary (e.g., the old maybe type is gone, and Marshal now has no type constraints). Would you mind taking a look?

[josephlr]

A few questions:

• Why do we still have the Startup and Shutdown functions, can't we just use the direct API to call these functions?
• Why does the Response need a TPMCC as part of its interface?
• Having the Command/Response structs contain private types makes it hard to figure out how to construct our use these types.
  • Constructing is possible to figure out by searching the docs
  • However, use of the private structs (say to read fields of a union) doesn't seem to show up in godoc.
  • Would it be possible to have the types be public (TPM2BAttest) and have public constructors for these types (NewTPM2BAttest)

[josephlr]

This doesn't seem to return an error.

[chrisfenner]

Good catch. I'll fix this.

To your other questions:

Why do we still have the Startup and Shutdown functions, can't we just use the direct API to call these functions?

As an adapter so that we can use https://github.com/google/go-tpm-tools/tree/master/simulator (which will call back to Startup and Shutdown as free functions in go-tpm) until there's a version of go-tpm-tools that calls TPMDirect startup and shutdown. I'll add a comment and TODO about this.

Why does the Response need a TPMCC as part of its interface?

This allows the internal consistency check in the execute function to match the correct command and response together. I guess this could be done with generics instead now, WDYT?

Having the Command/Response structs contain private types makes it hard to figure out how to construct our use these types.

Yeah, this is a great point. I'll make e.g., the TPM2B types themselves public and provide public constructors.

[josephlr]

I was playing around a little and I think the following would work well:

Have a tpm2b submodule:

// Package tpm2b ...
//
// Document here how the TPM2B types work and give examples of how to use them
package tpm2b

type TPM2B[T any] struct {
	contents *T
	buffer   []byte
}

func New[T any](t T) TPM2B[T] {
	return TPM2B[T]{contents: &t}
}

func FromBytes[T any](b []byte) TPM2B[T] {
	return TPM2B[T]{buffer: b}
}

func (t *TPM2B[T]) Contents() (*T, error) {
	if value.contents != nil {
		return value.contents, nil
	}
	if value.buffer == nil {
		panic("TPMB had no contents or buffer")
	}
	var result T
	if err := unmarshal(bytes.NewBuffer(value.buffer), reflect.ValueOf(&result).Elem()); err != nil {
		return nil, err
	}
	
	// Cache result
	value.contents = &result
	return value.contents, nil
}

func (t *TPM2B[T]) Bytes() []byte {
	if value.buffer != nil {
		return value.buffer
	}
	if value.contents == nil {
		panic("TPMB had no contents or buffer")
	}
	var temp bytes.Buffer
	marshal(&temp, reflect.ValueOf(value.contents))
	
	// Cache Bytes
	value.buffer = temp.Bytes()
	return value.buffer
}

func (value *tpm2b[T]) marshal(buf *bytes.Buffer) {
	b := value.Bytes()
	binary.Write(buf, binary.BigEndian, uint16(len(b)))
	buf.Write(b)
}

func (value *tpm2b[T]) unmarshal(buf *bytes.Buffer) error {
	var size uint16
	binary.Read(buf, binary.BigEndian, &size)
	value.contents = nil
	value.buffer = make([]byte, size)
	_, err := io.ReadAtLeast(buf, value.buffer)
	return err
}

Then in the main tpm2 module will have just the typedefs:

type TPM2BECCPoint = tpm2b.TPM2B[TPMSECCPoint]

Good catch. I'll fix this.

I think not having Marshall/marshal return an error makes the library easier to use.

[josephlr]

For the commands and responses, is there a reason that the following doesn't work:

We have a generic interface for commands:

type Command[Response any] interface {
	CommandCode() TPMCC
	Execute(t transport.TPM, s ...Session) (Response, error)
}

and we have value recievers for the Commands and return the responses by value:

type CreateLoaded struct { /* ... */ }
type CreateLoadedResponse struct { /* ... */ }

func (CreateLoaded) CommandCode() TPMCC { return TPMCCCreateLoaded }
func (cmd CreateLoaded) Execute(t transport.TPM, s ...Session) (CreateLoadedResponse, error) {
	var rsp CreateLoadedResponse
	return rsp, execute(t, cmd, &rsp, s...)
}

We have internal execute and executeImpl functions similar to what is already in this PR:

// Typechecks that we implemented the Command interface correctly
func execute[C Command[R], R any](t transport.TPM, cmd C, rsp *R, s ...Session) error {
	return executeImpl(t, cmd.CommandCode(), cmd, rsp, s...)
}

func executeImpl(t transport.TPM, cc TPMCC, cmd any, rsp any, s ...Session) error {
	// Actual implementation of command execution
}

The reason I would prefer the Commands/Responses being passed by value is that them being pointer inputs/outputs was a little surprising when I was trying to write some code. For example, it would be nice if the following worked:

rsp, err := tpm2.GetCapability{Capability: tpm2.TPMCapAlgs, PropertyCount: 0xFFFF}.Execute(s)
if err != nil {
	log.Fatalf("Read Public: %v", err)
}

algs, err := rsp.CapabilityData.Data.Algorithms()
if err != nil {
	log.Fatalf("Didn't get algs: %v", err)
}
for _, alg := range algs.AlgProperties {
	fmt.Printf("0x%04X - %+v\n", alg.Alg, alg.AlgProperties)
}

[josephlr]

Another thing, for the TPML types, is there a reason we can't just do:

type TPMLPCRSelection []TPMSPCRSelection

and have the reflection code figure out that its a list? Or if that doesn't work, just tag it as a list in the parent structure:

type TPMSQuoteInfo struct {
	marshalByReflection
	// information on algID, PCR selected and digest
	PCRSelect TPMLPCRSelection `gotpm:"list"`
	// digest of the selected PCR using the hash of the signing key
	PCRDigest TPM2BDigest
}

It seems like we could just determine what to do based on the type alone though (i.e. we shouldn't need the tags). As if the type to marshalled/unmarshalled is an array, either:

• it's a []byte (or an alias thereof), and it gets a 16-bit size prefix
• it's some other []T, and it gets a 32-bit size prefix

[josephlr]

The above TPML change does cause one issue, it makes things like:

type capabilitiesContents interface {
	Marshallable
	*TPMLAlgProperty | *TPMLHandle | *TPMLCCA | *TPMLCC | *TPMLPCRSelection | *TPMLTaggedTPMProperty |
		*TPMLTaggedPCRProperty | *TPMLECCCurve | *TPMLTaggedPolicy | *TPMLACTData
}

no longer work as something like *TPMLCC (which would be a *[]uint32) cannot implement the Marshallable interface as it is a builtin type.

The solution seems to be to remove the Marshallable constraint from capabilitiesContents. This means that the reflection routine that marshals/unmarshals tpmuCapabilities can't just call the Marshal method, but this seems fine.

[josephlr]

One final ergonomics issue around the TPMU types.

Similar to the TPM2B types, the actual tpmu type that's in the relevant structures is not exported, so it's accessor methods are not visible in documentation. We might want to make the type public.

For exampe, if we have TPMU_FOO :

type TPMUFoo struct {
	selector TPMAlgID
	contents any
}

type FooContents interface {
	TPMSBar | TPMSBaz
}

func NewTPMUFoo[C FooContents](contents C) TPMUFoo {
	sel := /* ideally computed from C, if not possible, pass selector type in constructor */
	return TPMUFoo{
		selector: sel,
		contents: &contents,
	}
}

// Accessor methods (nil if unoccupied):
func (u *TPMUFoo) Bar() *TPMSBar {
	// ...
}
func (u *TPMUFoo) Baz() *TPMSBaz {
	// ...
}

[chrisfenner]

The reason I would prefer the Commands/Responses being passed by value is that them being pointer inputs/outputs was a little surprising when I was trying to write some code. For example, it would be nice if the following worked:

Great suggestion! This required making Marshallable and marshallableWithHint take receiver by value, and Unmarshallable and unmarshallableWithHint by pointer. I thought this would not work as I wasn't aware Go's pointer decay would support this (since the Unmarshallable interfaces include the Marshallable ones) but it turns out it works just fine. This simplifies the Command interface a bit, and even a little of the reflection code. I did end up having to meet you halfway, though. Commands' Execute will return the Response by pointer, i.e., (*FooResponse, error) so that we can return a null instead of garbage value if the command fails. However

rsp, err := (tpm2.GetCapability{Capability: tpm2.TPMCapAlgs, PropertyCount: 0xFFFF}).Execute(s)

now works (note you have to put parens around the GetCapability constructor, the compiler didn't like it without them).

[chrisfenner]

I'm in the middle of incorporating these excellent ideas. ❤️ = done, 👍 = "I think I can do this" and 👀 = "I have to try it but I like it." I'll ping when ready for another round of review!

[josephlr]

Great suggestion! This required making Marshallable and marshallableWithHint take receiver by value, and Unmarshallable and unmarshallableWithHint by pointe

I'm confused why taking/returning the Command and Response structures by pointer or value affects the receiver types for Marshallable and Unmarshallable. The commands and responses don't seem to implement those interfaces, so it seems like they won't necessarily have to be related.

i.e., (*FooResponse, error) so that we can return a null instead of garbage value if the command fail

It initially seemed to me that return the zero value for FooResponse would be fine. Is the issue just one of safety, were we want the user's program to crash if they forget to check the error? Or is the issue about if we partially Unmarshal the response into FooResponse, leading to an object that is partially valid and partially invalid?

[chrisfenner]

I'm confused why taking/returning the Command and Response structures by pointer or value affects the receiver types

The subtle problem: Command structs contain Marshallable structs inside them. Go reflection doesn't let you take the address of a member field of a struct that you got by value. So if you have to take the address of the member to use Marshal on it, you have to pass the whole containing struct by pointer as well.

It initially seemed to me that return the zero value for FooResponse would be fine.

I think this would harm users more than it helped them: maybe the user was expecting the zero value for the response. I think we should require the user to check the error (and crash obviously if they didn't).

[chrisfenner]

Another thing, for the TPML types, is there a reason we can't just do:

Unfortunately this generally means the TPML types can't be marshalled by users. I'd like users to be able to marshal any TPM type. I think the drawback to keeping it this way is just a bit more boilerplate in the structures code for TPML, so keeping it for now. WDYT?

[chrisfenner]

@josephlr Thanks for your patience and for the great feedback. I've incorporated your comments except for the idea about making TPMLSomething a []Something, because I'd like for all types to be usable with Marshal directly.

BTW, the solution to TPM2B differs from yours in a couple key ways:

• Can't be a submodule due to bidirectional dependency with tpm2
• Name changes since it unfortunately can't be submoduled
• The type parameters on TPM2B are a little complicated because it needs to capture the concrete type of both T (Marshallable) and *T (Unmarshallable) but thankfully this weirdness is completely hidden from users due to type inference.

[chrisfenner]

I'd like to mirror this into Google's internal repo as part of next steps for tpmdirect. Since this is a separate branch, it seems OK to me to go ahead and merge. Thanks for the great feedback so far!

Let's discuss any other things that need to be fixed as part of the path to 1.0 release.

[josephlr]

I'd like to mirror this into Google's internal repo as part of next steps for tpmdirect. Since this is a separate branch, it seems OK to me to go ahead and merge. Thanks for the great feedback so far!

Let's discuss any other things that need to be fixed as part of the path to 1.0 release.

That seems reasonable further changes (if any) will likely be small, and can have their own prs. I think the main test now is to actually use this library to implement some of the medium complexity TPM functionality, and see what works and what doesnt