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 “enterprise” attestation type. #1366
Conversation
In controlled deployments, organisations may wish to tie specific registrations back to individual authenticators. Obviously this has privacy concerns and needs to be gated on local configuration, or special configuration on the authenticator. However, as cloud services are increasingly used, RP IDs are no longer neatly divided into enterprise and consumer contexts, and the RP might _not_ wish to receive the enterprise attestation when used in a consumer context. This change adds a new level of attestation, “enterprise”, which allows RPs to indicate when they would like to, possibly, receive an attestation that may include uniquely identifying information. This leaves “direct” with its current, less privacy-impacting meaning. Fixes w3c#1147
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall looks good, tho at detail level need to signal to authnr at the correct time in the [Create] alg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thx @agl !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added #1376 about adding to enums
update 2020-03-11 by @equalsJeffH: this reference to issue #1376 means that we are going to, as a part of finalizing this PR, alter the use of the AttestationConveyancePreference
enum such that it is replaced by a sequence of DOMStrings
(and retain the enum as a "value registry"). See PR #1275 "Type transport strings as DOMStrings." for an example of how we did this with the AuthenticatorTransport
enum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, thanks. One necessary fixup since we're technically not origin-based, but RP ID-based. Also, in a PR on github.com/alg/webauthn:ep
, I fixed-up a link error and clarified how enterpriseAttestationPossible
is factored into the authenticatorMakeCredential
operation. HTH
Co-Authored-By: =JeffH <jdhodges@google.com>
Co-Authored-By: =JeffH <jdhodges@google.com>
That PR was pulling in some other changes so I've just rebased it into this change. I believe everything should be included here now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thx @agl :)
on 2020-04-01 call: reviewers please review :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
(From call: I need to update this.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
Co-Authored-By: J.C. Jones <james.jc.jones@gmail.com>
All comments are resolved, are we ok to merge this now? |
The current indentation renders this step as a sub-step of "For each credential descriptor C in pkOptions.excludeCredentials", meaning the operation is to be invoked once per `excludeCredentials` entry and not at all if `excludeCredentials` is empty. The indentation appears to have been broken in commit b44009c (originally 7acc1d5) in PR #1366, caused by a mix of tab and space characters being replaced with just spaces.
The current indentation renders this step as a sub-step of "For each credential descriptor C in pkOptions.excludeCredentials", meaning the operation is to be invoked once per `excludeCredentials` entry and not at all if `excludeCredentials` is empty. The indentation appears to have been broken in commit b44009c (originally 7acc1d5) in PR #1366, caused by a mix of tab and space characters being replaced with just spaces.
In controlled deployments, organisations may wish to tie specific
registrations back to individual authenticators. Obviously this has
privacy concerns and needs to be gated on local configuration, or
special configuration on the authenticator. However, as cloud services
are increasingly used, RP IDs are no longer neatly divided into
enterprise and consumer contexts, and the RP might not wish to receive
the enterprise attestation when used in a consumer context.
This change adds a new level of attestation, “enterprise”, which allows
RPs to indicate when they would like to, possibly, receive an
attestation that may include uniquely identifying information. This
leaves “direct” with its current, less privacy-impacting meaning.
Fixes #1147
Improves #1376
Preview | Diff