Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make a marshallable interface/type constraint #292

Open
2 tasks
chrisfenner opened this issue Jul 12, 2022 · 2 comments
Open
2 tasks

Make a marshallable interface/type constraint #292

chrisfenner opened this issue Jul 12, 2022 · 2 comments
Labels
tpmdirect Issues specific to the tpmdirect development work

Comments

@chrisfenner
Copy link
Member

  • Marshal takes interface{} so will panic if you accidentally pass something that is not marshallable.

    • Possible improvements:

      • Define an interface for types that can be Marshalled in a compact way that minimizes code bloat.
  • Type annotations are a bit magic.

    • Someone working on this code might get the gotpm: type annotations incorrect or slightly incorrect. It would be interesting to look into ways to make it more obvious what to do when introducing new TPM structures from the spec.
@chrisfenner chrisfenner added the tpmdirect Issues specific to the tpmdirect development work label Jul 12, 2022
@chrisfenner
Copy link
Member Author

Let's add a Go1.18 type constraint for this:

There's tons of types, so we can divide them by category:

type TPMA interface {
  TPMAAlgorithm | TPMAObject | TPMASession | TPMALocality | ...
}
type Marshallable  interface {
  TPMA | TPMT | TPMS | ...
}

While we're taking a dependency on Go1.18, let's also consider using type constraints for the unions:

// TPMUCapabilities represents a TPMU_CAPABILITIES.
// See definition in Part 2: Structures, section 10.10.1.
type TPMUCapabilities struct {
	Algorithms    *TPMLAlgProperty       `gotpm:"selector=0x00000000"` // TPM_CAP_ALGS
	Handles       *TPMLHandle            `gotpm:"selector=0x00000001"` // TPM_CAP_HANDLES
	Command       *TPMLCCA               `gotpm:"selector=0x00000002"` // TPM_CAP_COMMANDS
	PPCommands    *TPMLCC                `gotpm:"selector=0x00000003"` // TPM_CAP_PP_COMMANDS
	AuditCommands *TPMLCC                `gotpm:"selector=0x00000004"` // TPM_CAP_AUDIT_COMMANDS
	AssignedPCR   *TPMLPCRSelection      `gotpm:"selector=0x00000005"` // TPM_CAP_PCRS
	TPMProperties *TPMLTaggedTPMProperty `gotpm:"selector=0x00000006"` // TPM_CAP_TPM_PROPERTIES
	PCRProperties *TPMLTaggedPCRProperty `gotpm:"selector=0x00000007"` // TPM_CAP_PCR_PROPERTIES
	ECCCurves     *TPMLECCCurve          `gotpm:"selector=0x00000008"` // TPM_CAP_ECC_CURVES
	AuthPolicies  *TPMLTaggedPolicy      `gotpm:"selector=0x00000009"` // TPM_CAP_AUTH_POLICIES
	ACTData       *TPMLACTData           `gotpm:"selector=0x0000000A"` // TPM_CAP_ACT
}
// TPMSCapabilityData represents a TPMS_CAPABILITY_DATA.
// See definition in Part 2: Structures, section 10.10.2.
type TPMSCapabilityData struct {
	// the capability
	Capability TPMCap
	// the capability data
	Data TPMUCapabilities `gotpm:"tag=Capability"`
}

We can make marshalling trivial with a type constraint:

// TPMUCapabilities represents a TPMU_CAPABILITIES.
// See definition in Part 2: Structures, section 10.10.1.
type TPMUCapabilities interface {
	TPMLAlgProperty | TPMLHandle | TPMLCCA | TPMLCC | TPMLPCRSelection |TPMLTaggedTPMProperty | TPMLTaggedPCRProperty |TPMLECCCurve | TPMLTaggedPolicy | TPMLACTData
}
// TPMSCapabilityData represents a TPMS_CAPABILITY_DATA.
// See definition in Part 2: Structures, section 10.10.2.
type TPMSCapabilityData struct {
	// the capability
	Capability TPMCap
	// the capability data
	Data TPMUCapabilities
}

This has the added benefit of deleting a level of nesting:

parms := tpmu.PublicParms{
	ECCDetail: &tpms.ECCParms{
		Scheme: tpmt.ECCScheme{
			Scheme: tpm.AlgECDSA,
			Details: tpmu.AsymScheme{
				ECDSA: &tpms.SigSchemeECDSA{
					HashAlg: tpm.AlgSHA256,
				},
			},
		},
		CurveID: tpm.ECCNistP256,
	},
}

to

parms := tpmu.PublicParms{
	ECCDetail: &tpms.ECCParms{
		Scheme: tpmt.ECCScheme{
			Scheme: tpm.AlgECDSA,
			Details: tpms.SigSchemeECDSA{
				HashAlg: tpm.AlgSHA256,
			},
		},
		CurveID: tpm.ECCNistP256,
	},
}

However we have to do an extra step on the library side to make unmarshalling work. For each of the TPMU we have to define a "custom" unmarshalling function that checks the value of the selector and unmarshals a value of the correct corresponding type. This is not a big deal, given that there are about 18 or so TPMU structures - not too many to do by hand.

The Unmarshal function can check if a type implements customUnmarshallable and invoke the custom unmarshaller, and we can hide the details of this from the callers. Also, removing the TPMU handler type annotations from the library will simplify it at least enough to offset the complexity of adding an interface.

@josephlr @alexmwu WDYT?

@chrisfenner chrisfenner changed the title Rough edges of tpmdirect to smooth out Make a marshallable interface/type constraint Sep 3, 2022
@chrisfenner
Copy link
Member Author

Similar to what we saw in #307, TPMU types can't be based on type constraints since they have to be types.

chrisfenner added a commit that referenced this issue Feb 9, 2023
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 #309 )

Generics to the rescue. Here's what this commit 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tpmdirect Issues specific to the tpmdirect development work
Projects
None yet
Development

No branches or pull requests

1 participant