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

Switch |tokenBindingId| to a structure. #802

Merged
merged 2 commits into from Feb 16, 2018
Merged

Switch |tokenBindingId| to a structure. #802

merged 2 commits into from Feb 16, 2018

Conversation

agl
Copy link
Contributor

@agl agl commented Feb 14, 2018

The existing string was not able to express the ternary nature of token
binding for a given connection. See referenced bug for discussion.

Fixes #798


Preview | Diff

The existing string was not able to express the ternary nature of token
binding for a given connection. See referenced bug for discussion.

Fixes w3c#798
Copy link
Contributor

@equalsJeffH equalsJeffH left a comment

Choose a reason for hiding this comment

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

Good catch, LGTM

Copy link
Member

@emlun emlun 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 save for some small details.

index.bs Outdated
: {{CollectedClientData/tokenBindingId}}
:: The [=Token Binding ID=] associated with |callerOrigin|, if one is available.
: {{CollectedClientData/tokenBinding}}
:: The status of [=Token Binding=] between the client and the |callerOrigin| as well as the [=Token Binding ID=] associated with |callerOrigin|, if one is available.
Copy link
Member

Choose a reason for hiding this comment

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

Tiny nit: I suggest moving the comma to just before "as well": "The status of [...], as well as the [...] if one is available". So that "if one is available" refers to just the token binding ID instead of the whole structure. You're welcome to ignore this comment if you wish.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Done.

index.bs Outdated
: {{CollectedClientData/tokenBindingId}}
:: The [=Token Binding ID=] associated with |callerOrigin|, if one is available.
: {{CollectedClientData/tokenBinding}}
:: The status of [=Token Binding=] between the client and the |callerOrigin| as well as the [=Token Binding ID=] associated with |callerOrigin|, if one is available.
Copy link
Member

Choose a reason for hiding this comment

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

Tiny nit: I suggest moving the comma to just before "as well": "The status of [...], as well as the [...] if one is available". So that "if one is available" refers to just the token binding ID instead of the whole structure. You're welcome to ignore this comment if you wish.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Likewise.)

@@ -2930,8 +2937,7 @@ When verifying a given {{PublicKeyCredential}} structure (|credential|) as part

1. Verify that the value of <code>|C|.{{CollectedClientData/origin}}</code> matches the [=[RP]=]'s [=origin=].

1. Verify that the value of <code>|C|.{{CollectedClientData/tokenBindingId}}</code> (if present) matches the [=Token Binding ID=]
for the TLS connection over which the signature was obtained.
1. Verify that the value of <code>|C|.{{CollectedClientData/tokenBinding}}.{{TokenBinding/status}}</code> matches the state of [=Token Binding=] for the TLS connection over which the attestation was obtained. If [=Token Binding=] was used on that TLS connection, also verify that <code>|C|.{{CollectedClientData/tokenBinding}}.{{TokenBinding/id}}</code> matches the [=base64url encoding=] of the [=Token Binding ID=] for the connection.
Copy link
Member

Choose a reason for hiding this comment

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

"signature" has been erroneously changed to "attestation" here, in "for the TLS connection over which the signature was obtained". Perhaps we should change "signature" to "[=assertion=]" while we're at it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that. Done.

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

@akshayku akshayku 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.

@equalsJeffH
Copy link
Contributor

thx @emlun for the fine-detail review & suggestions :)

@nadalin nadalin modified the milestones: CR, Last Working Draft Feb 14, 2018
@equalsJeffH
Copy link
Contributor

merge this?

@akshayku
Copy link
Contributor

Yup.

@agl
Copy link
Contributor Author

agl commented Feb 15, 2018

(I believe the repo is locked and so merges aren't possible, at least for me. However, if anyone has the button to click, I believe this is ready to merge.)

@nadalin nadalin merged commit 05335d4 into w3c:master Feb 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants