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

Add support for hash sequence #219

Merged
merged 10 commits into from
Oct 14, 2020
Merged

Add support for hash sequence #219

merged 10 commits into from
Oct 14, 2020

Conversation

langbeck
Copy link
Contributor

@langbeck langbeck commented Sep 22, 2020

Adding support for HashSequenceStart, SequenceUpdate and SequenceComplete.
Closes issue #179

@google-cla
Copy link

google-cla bot commented Sep 22, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@langbeck
Copy link
Contributor Author

@googlebot I signed it!

@langbeck
Copy link
Contributor Author

Hi @josephlr
I'll send another commit with some tests once you confirm that the proposed API seems fine.

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 the change! This looks good to me, I have a structural comment (interested in @josephlr or @twitchy-jsonp opinions on it as well) and one optional request.

tpm2/tpm2.go Outdated Show resolved Hide resolved
tpm2/tpm2.go Show resolved Hide resolved
tpm2/constants.go Outdated Show resolved Hide resolved
@langbeck
Copy link
Contributor Author

Do you guys want me to change anything else?
This repo does auto-merge once I get the approval of all 3 reviewers?

@twitchy-jsonp
Copy link
Contributor

Sorry for the delay, I usually wait for another reviewer before merging.

In any case, can you rebase onto master? Github might be able to do it automatically.

@langbeck
Copy link
Contributor Author

Rebased!


@twitchy-jsonp And how about the enum discussion above?
Do still you want me to change to numerical ordering?

Note that everytime someone ask me to change something I'll ask for that reviewer approval again.

@twitchy-jsonp
Copy link
Contributor

I think I prefer consistency over correctness for something simple like an enum, and hence would prefer a consistent order. That said, if you are gunning to get this merged, I can leave it be, either way.

@langbeck
Copy link
Contributor Author

langbeck commented Sep 29, 2020

Then I'll send a commit putting the new values in numerical order.
I still need to submit a commit with tests for this new API.
Right now I just want a signal that new change requests to this PR API are unlikely, so I can start working on doing the tests.

Note: I'm using this library in a project and I already have more changes/fixes to submit, but I can only do so after this one gets merged

@twitchy-jsonp
Copy link
Contributor

twitchy-jsonp commented Sep 29, 2020

Right now I just want a signal that new change requests to this PR API are unlikely, so I can start working on doing the tests.

@chrisfenner who is the expert here. LGTM.

@langbeck
Copy link
Contributor Author

Enum reordered and tests completed. =)

@langbeck
Copy link
Contributor Author

langbeck commented Oct 4, 2020

@chrisfenner @twitchy-jsonp @josephlr it does appear that you guys don't have much time do maintain this repo, so I would like to offer my help as maintainer.

I work at HPE and I've wrote a Go TSS package covering almost the same commands and types that you guys have here.
I've implemented all the calls following the exact inputs and outputs as described on TPM2 spec part 3 (thus would be no need to have those *Ex methods that we have here).
That version is HPE's property and I can't use it on opensource projects.

All this just is just an attempt convince you guys that I do have experience with TPM2 and that I'm able to help this project.
Cheers,
Dórian

@langbeck
Copy link
Contributor Author

Could someone finish merging this PR, please?

Copy link
Member

@josephlr josephlr 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 really great, I have a bunch of style nits, but no substantial issues.

Thanks for submitting this and sorry for the delay in review.

tpm2/tpm2.go Outdated Show resolved Hide resolved
tpm2/tpm2.go Outdated Show resolved Hide resolved
tpm2/tpm2.go Outdated Show resolved Hide resolved
tpm2/tpm2.go Outdated Show resolved Hide resolved
tpm2/tpm2.go Outdated Show resolved Hide resolved
tpm2/test/tpm2_test.go Outdated Show resolved Hide resolved
tpm2/test/tpm2_test.go Outdated Show resolved Hide resolved
tpm2/test/tpm2_test.go Outdated Show resolved Hide resolved
tpm2/test/tpm2_test.go Outdated Show resolved Hide resolved
tpm2/test/tpm2_test.go Outdated Show resolved Hide resolved
@josephlr
Copy link
Member

@chrisfenner @twitchy-jsonp @josephlr it does appear that you guys don't have much time do maintain this repo, so I would like to offer my help as maintainer.

Thanks for the offer. If you want to help out with some existing pending PRs, we could totally look into adding you as a maintainer. My apologies that this review languished for so long. It's a good change that should have been quickly reviewed an approved.

We've been meaning to rework this library to autogenerate all the necessary code for all commands/structures from the Spec, but that effort has languished (mostly due to other stuff coming up at work). I would really like to get an autogenerated API, so that we don't keep having to add commands piecemeal.

I work at HPE and I've wrote a Go TSS package covering almost the same commands and types that you guys have here.
I've implemented all the calls following the exact inputs and outputs as described on TPM2 spec part 3 (thus would be no need to have those *Ex methods that we have here).

This is really cool. Needless to say, there aren't a ton of people who have the necessary expertise, so anyone willing to help is welcome.

@langbeck
Copy link
Contributor Author

@josephlr I've just fixed the import and hashCount issues.
Before I do the "nit" changes please, allow me to quickly make a case in favor of the current coding style.
After doing some experiments with "non-Gophers" inside the company I noticed that, among other things, don't using inline features of the language and having space after a "logical block" of Go code considerably reduces the need for prior knowledge of the language in order to understand the problem solved by the code. This make it easier for non-Gophers, that does have knowledge of a technology, to review Go code.

If you still prefer the more condensed form, I already have the "nit" changes made and ready to Go.
Just let me know and I'll send the next commit with these changes.

Regarding the code generation, how far did you guys went?

@josephlr
Copy link
Member

If you still prefer the more condensed form, I already have the "nit" changes made and ready to Go.
Just let me know and I'll send the next commit with these changes.

I totally get the point you're making here, but we generally shoot for consistency with respect to the style, and it seems like inline error handling (at least for shot statements) is somewhat the norm. I realize it's not perfect, but it's the approach I try to use across our projects.

If you could add the nit fixes, I can get this merged.

Regarding the code generation, how far did you guys went?

We should open up a tracking issue for this stuff. Long story short, we asked Microsoft to open-source the codegen tool they use for other languages, and they did. See: https://github.com/microsoft/TSS.MSR/tree/master/TssCodeGen

Their official process is quite complex, but we would ideally add Go to the languages currently supported, so that we can just generate the entire Go API surface at once.

@twitchy-jsonp twitchy-jsonp merged commit 289acaa into google:master Oct 14, 2020
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.

4 participants