Skip to content

Conversation

nckrss
Copy link
Contributor

@nckrss nckrss commented Feb 16, 2024

  • See definition in Part 3, Commands, section 17.2

- See definition in Part 3, Commands, section 17.2
@nckrss nckrss requested review from alexmwu, jkl73 and a team as code owners February 16, 2024 18:49
@nckrss nckrss mentioned this pull request Feb 16, 2024
Copy link
Member

@chrisfenner chrisfenner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have only some very minor requests while the Cirrus CI re-runs. Thank you for sending this change!

// Context Handle Values come from table 211
const (
// an ordinary transient object
TPMIDHSSavedTransient TPMIDHSaved = 0x80000000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: These symbols don't have a name in the spec. I like your suggestions except for the extra S: could you name them TPMIDHSavedTransient, TPMIDHSavedSequence, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in f3f6077

switch (byte)(h >> 24) {
case 0x00, 0x02, 0x03, 0x40:
switch (TPMHT)(h >> 24) {
case TPMHTPCR, TPMHTHMACSession, TPMHTPolicySession, TPMHTPermanent, TPMHTTransient:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure it's right to add TPMHTTransient here? A transient object's name is not just the big-endian byte array of its handle, unlike with PCR, HMAC and Policy sessions, and Permanent entities.

(Thank you for the cleanup though, putting the actual symbols here!)

Copy link
Contributor Author

@nckrss nckrss Feb 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for calling this out, I mis-interpreted Part 2 Structures section 14.6.2 savedHandle.

Based on the table from Part 1 Architecture section 16 Table 3, I changed the handling in 0d43555 to address the case of a sequence object and to allow for Name computation of Transient and Persistent Objects in future implementations.

I first thought of changing the AuthHandle{} KnownName() method because passing in an empty AuthHandle.Name1 would default to AuthHandle.Handle.KnownName() being called, which will always be nil because the Name associated with a sequenceHandle is an empty buffer1. The nil Name value would then error in cmdNames.

Rather than modify the AuthHandle struct method I opted to extend the Name computation since it is defined for both Transient and Persistent Objects in Part Architecture section 16 Table 3.

Concerns or suggestions?

1 Part 1: Architecture, section 32.4.5

@nckrss
Copy link
Contributor Author

nckrss commented Feb 21, 2024

I updated the Dockerfile to use golang:1.21 because using golang:latest introduces incompatible changes between the linter v1.52.2 and go 1.22. Updating the linter to v1.56.2 (latest) raises new lint errors not in scope for this PR. If you're open to updating dependencies, I can open a separate PR to update the dependencies and fix lint errors.

@nckrss nckrss requested a review from chrisfenner February 21, 2024 17:51
@chrisfenner chrisfenner merged commit 08987ce into google:main Feb 22, 2024
@chrisfenner
Copy link
Member

Thanks @nckrss for the change!

@chrisfenner
Copy link
Member

I updated the Dockerfile to use golang:1.21 because using golang:latest introduces incompatible changes between the linter v1.52.2 and go 1.22. Updating the linter to v1.56.2 (latest) raises new lint errors not in scope for this PR. If you're open to updating dependencies, I can open a separate PR to update the dependencies and fix lint errors.

If you would like to do this, I would love to review the PR! Thanks in advance!

@nckrss nckrss deleted the tpm2_hmac_start branch February 23, 2024 01:03
@nckrss
Copy link
Contributor Author

nckrss commented Feb 27, 2024

If you would like to do this, I would love to review the PR! Thanks in advance!

Opened #353

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants