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

Authnr selection aaguidlist #479

Closed
wants to merge 16 commits into from
Closed

Authnr selection aaguidlist #479

wants to merge 16 commits into from

Conversation

rlin1
Copy link
Contributor

@rlin1 rlin1 commented May 29, 2017

Close #478


Preview | Diff

@rlin1 rlin1 self-assigned this May 29, 2017
@rlin1
Copy link
Contributor Author

rlin1 commented May 29, 2017

The big PR 442 was split into smaller PRs. This is the one addressing aaguidList

Copy link
Member

@jyasskin jyasskin left a comment

Choose a reason for hiding this comment

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

This was defined as an extension. What has changed to start mandating it for all implementations?

Also, why is the AAGUID the right thing to filter on rather than something like a certificate in the attestation chain?

index.bs Outdated
@@ -1067,8 +1068,21 @@ associated.
:: This member describes the [=[RPS]=]' requirements regarding availability of the [=Client-side-resident Credential
Private Key=]. If the parameter is set to true, the authenticator MUST create a
[=Client-side-resident Credential Private Key=] when creating a [=public key credential=].

: <dfn>aaguidList</dfn>
:: If this member is [=present|present=], eligible authenticators are filtered to the ones identified by matching
Copy link
Member

Choose a reason for hiding this comment

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

AAGUID filtering needs to happen as a step in https://w3c.github.io/webauthn/#op-make-cred or https://w3c.github.io/webauthn/#createCredential depending on whether the Client or Authenticator has the relevant information, not in the definition of the dictionary field.

@rlin1
Copy link
Contributor Author

rlin1 commented May 30, 2017

What has changed to start mandating it for all implementations?
In the beginning of the WebAuthn spec process, some people were convinced that RPs wouldn't have the need to influence the authenticator selection.

At this stage, we already have seen that there is a need to the RP to influence the authenticator selection in order to improve the user experience. As a consequence we have been adding the AuthenticatorSelectionCriteria object to the core specification.

The discussion regarding the fields in AuthenticatorSelectionCriteria shows that it is difficult to determine now what the right criteria for authenticator selection will be in the future.

So in my opinion there is substantial value for one generic method allowing the RP to formulate any rule for influencing the authenticator selection.
More specifically, I am convinced that the ability for providing a list of authenticator model names (i.e. AAGUIDs) provides exactly that. It allows the RP to use whatever set of criteria they want on the server side to generate such (prioritized) list and then make it easy for the platform to follow that list.

Also, why is the AAGUID the right thing to filter on rather than something like a certificate in the attestation chain?

The AAGUID was introduced as an abstraction for the authenticator model name. I think it is less bulky to send over a (potentially large) list of AAGUIDs than sending over a (potentially) even larger list of acceptable attestation certificates.
Note: One authenticator model might even have several attestation certificates in use (for the various batches of authenticator instances of such model).

index.bs Outdated
@@ -598,6 +598,8 @@ When this method is invoked, the user agent MUST execute the following algorithm
to |authenticator|'s attachment modality, [=iteration/continue=].
1. If {{AuthenticatorSelectionCriteria/requireResidentKey}} is set to |true| and the |authenticator|
is not capable of storing a [=Client-Side-Resident Credential Private Key=], [=iteration/continue=].
1. If {{AuthenticatorSelectionCriteria/aaguidList}} is [=present|present=] and the
to |authenticator|'s AAGUID protection method is not included in that list, [=iteration/continue=].
Copy link
Contributor

Choose a reason for hiding this comment

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

what's a "AAGUID protection method"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy&paste error. Corrected that

@balfanz
Copy link
Contributor

balfanz commented May 31, 2017

Can someone explain how AAGUIDs are used today? I personally am not aware of any authenticators that use AAGUIDs, so I would be inclined to not include this functionality in the spec.

But I do think such authenticators exist. Can @rlin1 or someone else explain how this functionality would be used? What could you do that you can't do without this PR, and why is that important?

@wseltzer wseltzer modified the milestone: WD-06 May 31, 2017
@rlin1
Copy link
Contributor Author

rlin1 commented May 31, 2017

TODO: add reference to FIDO Metadata Service as an example on where/how to get information on authenticator models.

@rlin1
Copy link
Contributor Author

rlin1 commented Jun 14, 2017

Response to #479 (comment):

I am not aware of any webauthn authenticators today. I am aware of UAF and U2F authenticators.
UAF authenticators use a slightly different notion of a "model name" called AAID - and they implement that.
In U2F there is no equivalent of a model name, the closest thing to it is the list of attestationCertificateKeyIdentifiers.

In UAF the RP is able to influence Authenticator selection for "makeCredential" based on the AAID (and other criteria). The authenticator selection-based AAID can be used to support arbitrary criteria to be implemented by the RP - even without requiring dedicated support for that in the API.
For example: the RP could select an authenticator for registration that meets some specific security certification, etc.

I don't know how the RP could do that in U2F or in WebAuthentication - without this aaguidList approach.

@nadalin
Copy link
Contributor

nadalin commented Jun 22, 2017

Please update this by 6/27 so we can discuss on 6/28 call

@rlin1
Copy link
Contributor Author

rlin1 commented Aug 28, 2017

changes requested by @leshi have been addressed.

@equalsJeffH equalsJeffH changed the title Authnr sel aaguidlist Authnr selection aaguidlist Nov 20, 2017
@equalsJeffH
Copy link
Contributor

please merge-from-master (into this branch) and fix build errors, to facilitate further reviews, thx.

@equalsJeffH
Copy link
Contributor

I merged-from-master and fixed conflicts.

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.

Overall this seems fine to me, we already state in #authenticator-model that "Each authenticator has an AAGUID...". As specified here, using the AAGUID for authnr selection is optional. And it could be helpful for enterprise use cases.

A meta-level question is whether having an AAGUID is mandatory for webauthn authnrs. The above-quoted statement does not use RFC2119 language. We could punt to FIDO Authnr Certification requirements, and not stipulate whether AAGUID is mandatory-to-impl here in the webauthn spec.

Given our discussions concerning what became #attestation-convey, and its defaulting to "none", consumer-space RPs will not be able to rely on knowing users' authnrs' AAGUIDs.

So whether or not AAGUIDs are actually MTI for authnrs, many RPs will not receive them, i.e., they are optionally available to RPs in any case. That ought to be made clear whether or not we decide to accept this PR, because AAGUID-based selection is presently a defined extension.

@akshayku
Copy link
Contributor

Overall, this PR looks fine to me but this is breaking WD07 which we all agreed was feature complete and probably not needed. Correct me if I am wrong here.

There are two kinds of RPs, one who care about particular authenticators (for whatever reason, legal or otherwise and I don't want to rehash that discussion) and ones who don't care. Both of these can be addressed by current spec via direct attestation or null attestation(default case). This PR will short circuit faster (platform deciding earlier instead of RP) but essentially both ways achieve the same thing. If I remember correctly, there are other selection criterias (in addition to AAGUID) also which were pushed to L2 sometime back.

Should we take care of this in L2 or do we need it now?

@akshayku
Copy link
Contributor

I also see few more selection criteria (Matcher Protection #447, Key Protection #446, Biometric accuracy #445) that we have moved to L2. I would suggest all new selection criteria PRs/Issues (Like this one, Biometric Criteria #510) to be addressed in L2.

@rlin1
Copy link
Contributor Author

rlin1 commented Jan 10, 2018

Decided on call to move to L2.

@nadalin nadalin modified the milestones: CR, L2-WD-00 Jan 16, 2018
@jcjones jcjones removed their request for review November 14, 2018 18:02
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 would be a breaking change - and one that adds no new functionality, as there is already a normative extension to obtain this behavior. As we should avoid breaking changes in L2, this PR should be closed with no action.

@equalsJeffH equalsJeffH modified the milestones: L2-WD-00, L2-WD-01 Feb 13, 2019
@nadalin
Copy link
Contributor

nadalin commented Mar 7, 2019

Per 03/07/19 F2F Close

@nadalin nadalin closed this Mar 7, 2019
@emlun emlun deleted the authnr-sel-aaguidlist branch June 22, 2022 21:58
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.

9 participants