Skip to content

Conversation

@loicsikidi
Copy link
Contributor

a naive implementation of Command/Response marshalling in order to address #414

I'm open to feedback if there a better way to provide this feature.

@loicsikidi loicsikidi force-pushed the feature/marshall-command-response branch from 22ad7f5 to 2bd232d Compare October 9, 2025 08:14
@loicsikidi loicsikidi force-pushed the feature/marshall-command-response branch from 21becf4 to 59f5c5a Compare October 19, 2025 13:43
@loicsikidi
Copy link
Contributor Author

@chrisfenner once this PR will be approved it will be required to squash because latest commit messages won't be useful.

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.

This is looking really awesome! I have a few more comments to try to drive down the complexity of this solution and maximize the usability, but I think we're really close. Thanks for incorporating my feedback!

@loicsikidi loicsikidi force-pushed the feature/marshall-command-response branch from 8f3ee81 to 9b4e0b3 Compare October 23, 2025 08:38
@loicsikidi
Copy link
Contributor Author

loicsikidi commented Oct 23, 2025

Hi @chrisfenner sorry for the delay,

I've added 3 atomic commits to address your latest feedbacks.

  • 6842806 :
    • refactor CommandPreimage's marshal/unmarshal
    • add comments to say that we currently don't support encrypted params
    • remove ToCPHashPreimage()
  • bb5085e : deduplicate unmarshalparameter logic. WDYT?
  • 9b4e0b3 : refactor populateHandlesFromNames

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.

Thanks for your patience on my review, @loicsikidi -- your reply came in while I was at a conference, and then I have been out sick the past week. I think we are very close here, just a couple more things to make sure that this satisfies your original use case of making it as easy as possible for users to compute cpHash/rpHash for audit- and audit-adjacent TPM activities.

@loicsikidi
Copy link
Contributor Author

loicsikidi commented Nov 6, 2025

As agreed I've refactored Marshal/Unmarshal.

Command

cmdData, err := MarshalCommand(myCmd)
// handle err
cpHash := sha256.Sum256(cmdData)
// do something interesting with cpHash

Response

rspData, err := MarshalResponse(myCmd, myRsp)
// handle err
rpHash := sha256.Sum256(rspData)
// do something interesting with rpHash

I'm waiting for your feedbacks.

PS: I see the light at the end of the tunnel 😄

@loicsikidi loicsikidi force-pushed the feature/marshall-command-response branch from 26b1b05 to 07b2132 Compare November 6, 2025 21:30
@loicsikidi loicsikidi force-pushed the feature/marshall-command-response branch from 07b2132 to aa80b13 Compare November 6, 2025 21:31
// rpHash := sha256.Sum256(rspData)
//
// Note: Encrypted response parameters (via sessions) are not currently supported.
func MarshalResponse[C Command[R, *R], R any](cmd C, rsp *R) ([]byte, error) {
Copy link
Contributor Author

@loicsikidi loicsikidi Nov 6, 2025

Choose a reason for hiding this comment

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

@chrisfenner

I need to get the Command Code as an input.

I think that it's more natural to pass the command but maybe that TPMCC is better.

WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

I think what you've written is perfect. Very natural. Thanks!

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.

Thanks so much for this change and for incorporating the feedback!

// rpHash := sha256.Sum256(rspData)
//
// Note: Encrypted response parameters (via sessions) are not currently supported.
func MarshalResponse[C Command[R, *R], R any](cmd C, rsp *R) ([]byte, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I think what you've written is perfect. Very natural. Thanks!

@chrisfenner chrisfenner merged commit 5ef7ca4 into google:main Nov 10, 2025
4 checks passed
@chrisfenner
Copy link
Member

Thank you @loicsikidi for this change! I'm very happy with how the feature turned out. I appreciate your willingness to entertain my many comments :)

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