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

Get Credential List from the server during authentication. #28

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Get Credential List from the server during authentication. #28

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4daa8d6
UpdateAllowlistRecommendation
Jan 13, 2021
561291e
Long lived session key
Jan 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 50 additions & 31 deletions HowToFIDO.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,10 +231,13 @@ navigator.credentials.create({
rp: {...},
user: {...},
challenge: ...,
pubKeyCredParams: {
pubKeyCredParams: [{
type: "public-key",
alg: -7
},
}, {
type: "public-key",
alg: -257
}],
authenticatorSelection: {
authenticatorAttachment: "platform", // !!!
userVerification: "required" // !!!
Expand All @@ -252,10 +255,10 @@ navigator.credentials.create({
> canceling the creation).

Associate the returned public key and credential id with the user
account. Also, make sure you associate the credential id **with the
device** the user just authenticated from. For example, store the
credential id in a cookie (or associate it with a cookie), or store the
credential id in local storage.
account on the server. Also, make sure you associate this credential id as `2FACapableFidoCredential` and store all the transports applicable for this credential on the server. Associate a `2FACapableFIDOCredsAvailable` flag **with the
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you suggesting the tag/terminology 2FACapableFidoCredential in the UVPA case? If there is an intent to introduce this type of terminology then I think it needs a very clear definition in the Let's Define Some Terms section. For example::

  • 2FACapableFIDOCredsAvailable - a flag associated with an account at the browser (cookie or local storage) signifying that the user has (or had) at least one tagged 2FACapableFidoCredential credential registered at the RP that may be used from this device.
  • 2FACapableFidoCredential - a flag associated with a credential registered for the user at the RP signifying that the credential is capable of being used in an assertion flow with userVerification="required".

Not sure I've got that correct - but you get the idea - clarity in the meaning and intent of the term is important. If indeed the descriptions I've taken a stab at above are accurate, then I would consider changing 2FACapable to UVCapable instead. Perhaps go even further and use terminology UVPACredsAvailable and UVPACredential respectively. I don't see any updates to other sections of the document outside the UVPA use case (platform credentials) where this terminology comes into play.

Either way, be consistent with Fido vs FIDO in those labels. Right now they are not consistent. Based on other elements in the document, suggest all capitals FIDO.

I agree it's a good idea to store all the transports, however these are not guaranteed to be available from all user agents (yet).

user account** locally. For example, store the
`2FACapableFIDOCredsAvailable` flag in a cookie (or associate it with a cookie), or store the
`2FACapableFIDOCredsAvailable` flag in local storage. **Don't store the actual credential id locally**. Use `2FACapableFIDOCredsAvailable` flag instead of actual credential id which helps in cross-browser scenarios.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just delete the last two sentences here. Given there is no instruction to store the credential id locally at the user agent, there is also no need to explicitly say not to. I appreciate that you are changing the document from it's original guidance but when read standalone with the changes then it is not needed.


## 3 Performing FIDO-based Reauthentication

Expand All @@ -270,26 +273,26 @@ Reauthentication might happen for the following reasons:
re-confirm control over the user session.

Let’s look at the last case first: when it’s time to re-authenticate for
a sensitive action, check whether you have a credential id for this user
*for the purpose of reauthentication*, i.e., the kind of credential id
obtained from [<span class="underline">opting the user into FIDO-based
reauthentication</span>](#opting-into-fido-based-reauthentication). Make
sure it’s associated with the user *and* device - for example, check a
cookie or read from local storage.

If *no credential id is available*, serve a traditional login challenge
a sensitive action, check whether you have a `2FACapableFIDOCredsAvailable` flag for this user
*for the purpose of reauthentication*.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the conditional requirement around the words: for the purpose of reauthentication? Either the 2FACapableFIDOCredsAvailable flag is sufficient to pivot to what happens next, or it's not. I think these words should be deleted.


If *no `2FACapableFIDOCredsAvailable` flag is available*, serve a traditional login challenge
suitable for reauthentication\[8\], for example:

![password reauthentication](images/image15.png)

If, however, you *do* discover a credential id for the current session,
then you can use FIDO-based reauthentication:
If, however, you *do* have `2FACapableFIDOCredsAvailable` flag for the current session,
then you can use FIDO-based reauthentication.
Fetch all `2FACapableFidoCredential` credentials from the server for this user.
Don't fetch non `2FACapableFidoCredential` credentials like U2F credentials.
Prefer using long lived local session key while fetching `2FACapableFidoCredential` credentials from the server.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs to be stronger that Prefer. I don't think it should be permitted to fetch credentials from the server for a user without some long-lived session key as otherwise there is unauthenticated credential leakage.

Also order the credentials in the reverse order of when credentials are created (latest created credential is first in the list):
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps it would be better to order them based on "most recently used" rather than "most recently created"? Either way it would be useful to explain why this is being suggested.


![FIDO reauthentication](images/image2.png)

When the user is ready (in the above example, when they click on the
“Go\!” button), call `navigator.credentials.get()`, again requiring user
verification and specifying an “internal” transport:
verification:

```javascript
navigator.credentials.get({
Expand All @@ -298,9 +301,14 @@ navigator.credentials.get({
rpId: ...,
allowCredentials: [{
type: “public-key”,
id: ..., //!!! use the *one* credential id associated with
//!!! this user/device combination.
transports: [“internal”] //!!!
id: ..., // 2FACapableFidoCredential CredentialID_1
transports: ... // Transports associated with CredentialID_1
}, {
type: “public-key”,
id: ..., // 2FACapableFidoCredential CredentialID_2
transports: ... // Transports associated with CredentialID_2
}, {
...
}],
userVerification: “required”, //!!!
}
Expand Down Expand Up @@ -336,21 +344,24 @@ presumably with a different account. In this case, you should also give
the user the ability to completely remove their account from being
listed on the sign-in page.

If the user clicks on “Next”, then check whether you have a credential
id associated with the user and device (for example, check a cookie or
read from local storage). If no credential id is available, serve a
If the user clicks on “Next”, then check whether you have `2FACapableFIDOCredsAvailable` flag associated with the user (for example, check a cookie or
read from local storage). If no `2FACapableFIDOCredsAvailable` flag is available, serve a
traditional login challenge suitable for reauthentication, for example:

![Password-based re-sign-in](images/image3.png)

If, however, you *do* find a credential id for the current session, then
you can use FIDO-based reauthentication:
If, however, you *do* have `2FACapableFIDOCredsAvailable` flag for the current session,
then you can use FIDO-based reauthentication.
Fetch all `2FACapableFidoCredential` credentials from the server for this user.
Don't fetch non `2FACapableFidoCredential` credentials like U2F credentials.
Prefer using long lived local session key while fetching `2FACapableFidoCredential` credentials from the server.
Also order the credentials in the reverse order of when credentials are created (latest created credential is first in the list):
Comment on lines +357 to +358
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comments as above: I think this needs to be stronger that Prefer, and consider the ordering algorithm.


![FIDO-based re-sign-in](images/image4.png)

When the user is ready (in the above example, when they click on the
“Go\!” button), call navigator.credentials.get(), again requiring user
verification and specifying an “internal” transport:
verification:

```javascript
navigator.credentials.get({
Expand All @@ -359,9 +370,14 @@ navigator.credentials.get({
rpId: ...,
allowCredentials: [{
type: “public-key”,
id: ..., //!!! use the *one* credential id associated with
//!!! this user/device combination.
transports: [“internal”] //!!!
id: ..., // 2FACapableFidoCredential CredentialID_1
transports: ... // Transports associated with CredentialID_1
}, {
type: “public-key”,
id: ..., // 2FACapableFidoCredential CredentialID_2
transports: ... // Transports associated with CredentialID_2
}, {
...
}],
userVerification: “required”, //!!!
}
Expand Down Expand Up @@ -670,10 +686,13 @@ navigator.credentials.create({
rp: {...},
user: {...},
challenge: ...,
pubKeyCredParams: {
pubKeyCredParams: [{
type: "public-key",
alg: -7
},
}, {
type: "public-key",
alg: -257
Copy link
Contributor

Choose a reason for hiding this comment

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

There are other places besides here (and the initial UVPA scenario) where pubKeyCredParams is incorrectly represented as an object and you've corrected to an array (with additional alg). May I suggest that if you're going to fix it in two places, how about fixing it in all 5 places :)

}],
authenticatorSelection: {
authenticatorAttachment: "platform", //!!!
requireResidentKey: true, //!!!
Expand Down