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.

Sign up for GitHub

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

Jump to bottom

Tpm attestation fields clarification #791

Merged
merged 4 commits into from
Feb 9, 2018
Merged

Tpm attestation fields clarification #791

merged 4 commits into from
Feb 9, 2018

Conversation

akshayku
Copy link
Contributor

@akshayku akshayku commented Feb 8, 2018 •
edited by pr-preview bot
Loading

@akshayku akshayku added this to the CR milestone Feb 8, 2018
@akshayku
Copy link
Contributor Author

akshayku commented Feb 8, 2018

This fixes #372

@akshayku akshayku mentioned this pull request Feb 8, 2018
Closed
@rlin1
added reference to the Standard Attestation Structure table as it is …
cdf234d 
…hard to understand the field names without...
@rlin1
Copy link
Contributor

rlin1 commented Feb 8, 2018

Should we add a note that the value of extraData (i.e. the attToBeSigned) is set by the caller of the TPM and it is NOT verified by the TPM itself?

@nadalin nadalin requested a review from rlin1 February 8, 2018 16:21
rlin1
rlin1 approved these changes Feb 8, 2018
View reviewed changes
index.bs Outdated
@@ -3203,6 +3203,8 @@ engine.
- Verify that `attested` contains a `TPMS_CERTIFY_INFO` structure, whose `name` field contains a valid Name for |pubArea|,
as computed using the algorithm in the `nameAlg` field of |pubArea| using the procedure specified in [[TPMv2-Part1]]
section 16.
Copy link
Contributor

Choose a reason for hiding this comment

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

TPMS_CERTIFY_INFO is defined in TPMVv2-Part2, section 10.12.3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

@akshayku
Copy link
Contributor Author

akshayku commented Feb 8, 2018

@rlin1 I don't know whether it is necessary or provide any value to the RP.

@nadalin
Copy link
Contributor

nadalin commented Feb 8, 2018

@selfissued Please review

selfissued
selfissued approved these changes Feb 8, 2018
View reviewed changes
Copy link
Contributor

@selfissued selfissued 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 a good improvement.

If you want, add the missing comma that is syntactically required after "i.e." before merging, but if you don't want to take the time, this can be fixed in the overall review pass.

@nadalin
Copy link
Contributor

nadalin commented Feb 8, 2018

@akshayku please merge

@akshayku akshayku merged commit f41cf83 into master Feb 9, 2018
@akshayku akshayku deleted the akshay-tpm-attestation-clarification branch February 9, 2018 00:09
WebAuthnBot pushed a commit that referenced this pull request Feb 9, 2018
Built by Travis-CI: f41cf83 Tpm attestation fields clarification (#791)
925403f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rlin1 rlin1 rlin1 approved these changes

@selfissued selfissued selfissued approved these changes

Assignees

@akshayku akshayku

Labels
type:editorial
Projects
None yet
Milestone
Last Working Draft
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@akshayku @rlin1 @nadalin @selfissued