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 MakeCredential and ActivateCredential to Context #158

Merged

Conversation

puiterwijk
Copy link
Collaborator

No description provided.

Copy link
Member

@ionut-arm ionut-arm left a comment

Choose a reason for hiding this comment

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

Looks good! Only a couple of requests

@@ -653,6 +653,77 @@ impl Context {
}
}

pub fn activate_credential(
Copy link
Member

Choose a reason for hiding this comment

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

Could you add some docs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hah! I had actually typed those, but then realized just now that I never committed that!

Copy link
Member

Choose a reason for hiding this comment

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

Actually, are we missing some clippy lint here, I would've thought the CI should fail if something wasn't documented. Or was it because we have way too many types and most are not documented?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

oops 😅

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have so much stuff undocumented so enable that would mean a serious amount of errors.

src/context.rs Show resolved Hide resolved
These are basically buffers, but they used a different field name.
So in order to accomodate that, I abstracted the buffer_type to named_field_buffer_type.

Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>
This implements the MakeCredential and ActivateCredential TPM2 commands.

Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>
Copy link
Collaborator

@Superhepper Superhepper left a comment

Choose a reason for hiding this comment

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

Looks good to me!

I only have one thing to comment about but it is more a matter of preference and feel free to ignore it. I would preferred the name to be IdObject instead of IDObject. But that is just my personal preference and feel free to ignore it.

@puiterwijk puiterwijk merged commit c5d4d0e into parallaxsecond:master Dec 9, 2020
@puiterwijk puiterwijk deleted the make_activate_credential branch December 9, 2020 09:39
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.

3 participants