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

Use credential record abstraction in devicePubKey extension #1812

Merged
merged 6 commits into from
Jan 13, 2023

Conversation

emlun
Copy link
Member

@emlun emlun commented Oct 5, 2022

This applies the new abstraction from #1773 and #1807 to the devicePubKey extension, which can substantially streamline the RP processing steps for the new extension.


Preview | Diff

@emlun emlun added type:editorial process:meta-pr Pull requests into other pull requests rather than main labels Oct 5, 2022
@emlun emlun requested a review from agl October 5, 2022 20:57
@emlun emlun self-assigned this Oct 5, 2022
@emlun
Copy link
Member Author

emlun commented Oct 5, 2022

(@agl incidentally, you may use commit 107825f as a merge-from-master into PR #1663 if you like.)

Base automatically changed from jeffh-fix-1658-device-bound-key-extension to main October 7, 2022 20:05
@emlun emlun removed the process:meta-pr Pull requests into other pull requests rather than main label Oct 10, 2022
@nadalin nadalin added this to the L3-WD-01 milestone Dec 14, 2022
@timcappalli timcappalli self-requested a review December 14, 2022 20:08
Copy link
Member

@timcappalli timcappalli left a comment

Choose a reason for hiding this comment

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

A few inline clarification questions and one potential terminology change

index.bs Outdated Show resolved Hide resolved
index.bs Outdated
##### Registration (`create()`) ##### {#sctn-device-publickey-extension-verification-create}

If the `devicePubKey` extension was included on a {{CredentialsContainer/create()|navigator.credentials.create()}} call, then the below verification steps are performed in the context of <a href=#reg-ceremony-verify-extension-outputs>this step</a> of [[#sctn-registering-a-new-credential]] using these variables established therein: |credential|, |clientExtensionResults|, |authData|, and |hash|. [=[RP]=] policy may specify whether a response without a `devicePubKey` is acceptable.
If the `devicePubKey` extension was included on a {{CredentialsContainer/create()|navigator.credentials.create()}} call, then the below verification steps are performed in the context of <a href=#reg-ceremony-verify-extension-outputs>step 18</a> of [[#sctn-registering-a-new-credential]] using these variables established therein: |credential|, |clientExtensionResults|, |authData|, and |hash|. [=[RP]=] policy may specify whether a response without a `devicePubKey` is acceptable.
Copy link
Member

Choose a reason for hiding this comment

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

If the devicePubKey extension was included on a navigator.credentials.create() call

The RP may request a DPK via the extension, but the authenticator may not provide one. Is this referring to the request or response?

Do we need a statement in the processing logic for when the authenticator doesn't provide it back, or would that just be generic for all extensions?

Copy link
Member

Choose a reason for hiding this comment

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

(and now in hindsight, I realize these comments are likely more appropriate for the DPK PR itself, not yours. let me know if I should move them)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that's covered by the last part:

[=[RP]=] policy may specify whether a response without a devicePubKey is acceptable.

But evidently it could still use some clarification, then. :) How about this?

Suggested change
If the `devicePubKey` extension was included on a {{CredentialsContainer/create()|navigator.credentials.create()}} call, then the below verification steps are performed in the context of <a href=#reg-ceremony-verify-extension-outputs>step 18</a> of [[#sctn-registering-a-new-credential]] using these variables established therein: |credential|, |clientExtensionResults|, |authData|, and |hash|. [=[RP]=] policy may specify whether a response without a `devicePubKey` is acceptable.
If the `devicePubKey` extension was requested in a {{CredentialsContainer/create()|navigator.credentials.create()}} call, then the below verification steps are performed in the context of <a href=#reg-ceremony-verify-extension-outputs>step 18</a> of [[#sctn-registering-a-new-credential]] using these variables established therein: |credential|, |clientExtensionResults|, |authData|, and |hash|. [=[RP]=] policy may specify whether a response without a `devicePubKey` is acceptable.

Or perhaps even:

Suggested change
If the `devicePubKey` extension was included on a {{CredentialsContainer/create()|navigator.credentials.create()}} call, then the below verification steps are performed in the context of <a href=#reg-ceremony-verify-extension-outputs>step 18</a> of [[#sctn-registering-a-new-credential]] using these variables established therein: |credential|, |clientExtensionResults|, |authData|, and |hash|. [=[RP]=] policy may specify whether a response without a `devicePubKey` is acceptable.
If the [=[RP]=] requested the `devicePubKey` extension in a {{CredentialsContainer/create()|navigator.credentials.create()}} call, then the below verification steps are performed in the context of <a href=#reg-ceremony-verify-extension-outputs>step 18</a> of [[#sctn-registering-a-new-credential]] using these variables established therein: |credential|, |clientExtensionResults|, |authData|, and |hash|. [=[RP]=] policy may specify whether a response without a `devicePubKey` is acceptable.

Copy link
Member Author

Choose a reason for hiding this comment

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

@timcappalli Any thoughts on the above suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

@emlun the latter (If the RP requested) I think is the most clear.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @timcappalli!

I also edited the last phrase to

[...] without a devicePubKey extension output is acceptable.

I'll assume that's an acceptable edit too.

index.bs Outdated Show resolved Hide resolved
@emlun emlun merged commit d3270c5 into main Jan 13, 2023
@emlun emlun deleted the dpk-credential-record branch January 13, 2023 16:46
github-actions bot added a commit that referenced this pull request Jan 13, 2023
SHA: d3270c5
Reason: push, by emlun

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
emlun added a commit that referenced this pull request Jan 10, 2024
…ect}} added in meta-PR #1812

Originally added in commit 25291de.
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.

4 participants