Skip to content

Conversation

@friedrichsenm
Copy link

@friedrichsenm friedrichsenm commented May 20, 2025

This PR updates the PolicyCommand interface to add ExecutePolicyInSession. This new method extends the given policy session with the policy command and returns the response.

Closes #401

@friedrichsenm friedrichsenm marked this pull request as ready for review May 20, 2025 14:31
@friedrichsenm friedrichsenm requested review from a team, alexmwu and jkl73 as code owners May 20, 2025 14:31
@friedrichsenm
Copy link
Author

It looked like most of the policy test were testing the functionality of the policy commands themselves so I'm not sure if the one test I modified is considered meaningful enough. I can add a separate test to just test this interface (perhaps with a trial session or something) if you think that is appropriate.

tpm2/tpm2.go Outdated

// PolicyCommand is a TPM command that can be part of a TPM policy.
type PolicyCommand interface {
type PolicyCommand[R any, PR *R] interface {
Copy link
Member

Choose a reason for hiding this comment

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

Couple options come to mind here:

  1. We can toss the (non-error) response from the policy command, assuming that users who want to consume the output of the Policy command are not doing so using this interface.
  2. We could make this interface function just return any, error and leave it to the caller to do the type assertion if they wish to consume the data.

What do you think? Do either of these options break your use-case?

Copy link
Author

Choose a reason for hiding this comment

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

I think returning an any would be better. I'm not currently using any of the fields in the returns, but I might want to and it wouldn't be too hard to handle returns from this in a switch statement to handle the non-trivial cases.

@friedrichsenm
Copy link
Author

@chrisfenner, I found an Update method that was missing the error return parameter and added that as a second commit. I can put that in a separate PR if you'd prefer.

@friedrichsenm friedrichsenm requested a review from chrisfenner May 27, 2025 19:15
@chrisfenner
Copy link
Member

Hi @friedrichsenm, sorry for losing track of this. If you're still interested in the change would you mind rebasing to resolve the merge conflicts?

This PR updates the PolicyCommand interface to add ExecutePolicyInSession.
This new method extends the given policy session with the policy command
and returns the response.
@friedrichsenm friedrichsenm force-pushed the mattF_extend_PolicySessions_take2 branch from f55f163 to beb426b Compare September 3, 2025 14:04
@friedrichsenm
Copy link
Author

@chrisfenner, sorry for the delayed response as well! You caught me when I was on vacation. I rebased my PR and it should be ready to go if there aren't any further changes/corrections.

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.

Consider an update to PolicyCommand or a new interface for working with policy sessions

2 participants