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

Indicate resident key credential "preferred" during registration and find out what the authenticator offered #991

Closed
sbweeden opened this issue Jul 11, 2018 · 36 comments · Fixed by #1191

Comments

@sbweeden
Copy link
Contributor

sbweeden commented Jul 11, 2018

I'd like to revisit the scenario I was trying to achieve in the (now closed) issue #987

Why can't the "requireResidentKey" authenticator selection criteria be a value like "preferred", "required" or "never" instead of a boolean true/false? In addition the registration response could indicate residentKey true/false as a flag similar to the userPresent and userVerified flags, or in an extension.

That way, without knowing ahead of time if an authenticator is capable of resident key (even a portable security key), the relying party could request it with fallback to derived credential (without a hard error), then notify the user of what actually happened and therefore what scenarios their key might be used for (replacement for regular username/password authentication, or only 2nd-factor scenarios)?

@emlun
Copy link
Member

emlun commented Jul 11, 2018

I think this sounds like a good idea for L2.

@nadalin
Copy link
Contributor

nadalin commented Jul 11, 2018

@sbweeden out of scope for L1 moved to L2

@ve7jtb
Copy link
Contributor

ve7jtb commented Aug 17, 2018

It may be a smoother registration flow to try resident and fall back,
The one concern I have is that that might make everyone ask for resident keys and cause some authenticators to fill up faster. Something to discuss for L2.

@herrjemand
Copy link
Contributor

This is a great idea

@leshi
Copy link
Contributor

leshi commented Mar 7, 2019

Let's make a PR for this.

@sbweeden
Copy link
Contributor Author

To kick off the solutioning discussion - how do we expose this as part of the WebAuthnAPI?

I don't know enough about CTAP2 to know if this is viable, but from a pure WebAuthn/RP perspective, here's a suggestion to start with that attempts to preserve backwards compatibility with the existing API. There are other options such as extensions, however I prefer not to go down the extensions route as I think it is important that all user-agents support this capability and therefore it be part of the core API.

Currently in L1, options.authenticatorSelection.requireResidentKey is of type boolean. This can't easily change without breaking backwards compatibility unless we could have requireResidentyKey be one of a couple of different data types (boolean OR the proposed ResidentKeyRequirement), so the proposal is to leave this parameter as-is. In L1, it is already OPTIONAL, and defaults to false. My suggestion is to ADD another optional member to the AuthenticatorSelectionCriteria dictionary, as follows:

Part 1 - Allowing the RP to express that resident keys are "preferred", if supported, but that it's ok to fallback to non-resident

dictionary AuthenticatorSelectionCriteria {
    AuthenticatorAttachment      authenticatorAttachment;
    boolean                      requireResidentKey = false;
    ResidentKeyRequirement       residentKey = "discouraged";
    UserVerificationRequirement  userVerification = "preferred";
};

enum ResidentKeyRequirement {
    "discouraged",
    "preferred",
    "required"
};

Only one of requireResidentKey or residentKey should be present in AuthenticatorSelectionCriteria, and use of residentKey is encouraged. If both are present and the user-agent supports it, residentKey takes precedence over requireResidentKey.

Logically, residentKey replaces requireResidentKey, with:

  • requireResidentKey=false being equivalent to residentKey="discouraged" (this is also why "discouraged" is the default)
  • requireResidentKey=true being equivalent to residentKey="required"

The new functionality is exposed via the requiredResidentKey="preferred" value.

Part 2 - How authenticators report on what type of key was provisioned (resident / non-resident)

Again there are several options including:

  • Using one of the reserved bits in flags that's part of authData
  • A new member of the attestation object (this is not signed, so don't like it)
  • Extension data (I'd rather not use extensions for the same reason mentioned earlier)

My suggestion is to consider using one of the flags bits (e.g. bit 5). The flag could be used during both the registration ceremony (to indicate whether resident key was provisioned or not), and authentication ceremony (to indicate if a resident key was used). The latter is not really necessary because the server could remember if the CPK was resident following the registration ceremony.

Other considerations

The algorithm for creating a new credential (https://w3c.github.io/webauthn/#createCredential) will need to be updated to consider new processing rules for residentKey="preferred".

Similarly, section 6.3.2 (https://w3c.github.io/webauthn/#op-make-cred) requires update in a couple of places, which probably also means CTAP2 requires update.

If the above suggestion were to be implemented, the IDL Index (https://w3c.github.io/webauthn/#idl-index) would also need updating.

@emlun
Copy link
Member

emlun commented Mar 13, 2019

Thanks @sbweeden, I think this looks pretty good as a starting point, but (re @leshi) I think we need more discussion on the deeper implications of this before we're ready to move forward with a PR.

Part 1: WebIDL API

I mostly agree with your draft, but I would rename "discouraged" to "indifferent":

dictionary AuthenticatorSelectionCriteria {
    AuthenticatorAttachment      authenticatorAttachment;
    boolean                      requireResidentKey = false;
    ResidentKeyRequirement       residentKey = "indifferent";
    UserVerificationRequirement   userVerification = "preferred";
};

enum ResidentKeyRequirement {
    "indifferent",
    "preferred",
    "required"
};

And the client/authenticator behaviour would be:

  • If "indifferent", the client MAY use any authenticator. The authenticator MAY create a non-RK credential or an RK credential, at its discretion.
  • If "preferred", the client should try not to use an RK-incapable authenticator. The authenticator SHOULD create an RK credential and MAY return an error if it cannot.
    • "MAY return an error if it cannot" is debatable - this could instead be "MUST create a non-RK credential if it cannot create an RK credential".
  • If "required", the client should try not to use an RK-incapable authenticator. The authenticator MUST create an RK credential and MUST return an error if it cannot.

We could also add a "forbidden" value, but as noted in #1149 I don't think there's actually any use case for it.

Part 2: Authenticator report

This is where it gets tricky. Any change we make to what the authenticator returns will require changes to authenticators, and whatever solution we think up for L2, it will still be near impossible to know for sure whether a L1 authenticator created an RK or not. This means we probably need some way for the authenticator to signal that it's a L2 authenticator. For example, if we set a new flag bit to 1 to mean RK, then 0 could mean either "not RK" or "L1 authenticator (maybe RK)". Like the RK indicator, there are a few ways we could add an L2 signal:

  • Use another reserved bit. That would mean we consume 2 of the remaining 4 flags and are left with only 2 for the future. Say we need another flag for L3, and a way to signal L3 authenticator, and we've already consumed all 4 reserved bits. Probably not ideal.
  • New member of the attestation object. I agree this is undesirable since it's not signed, although the possible impact of tampering with this flag should be minimal.
  • Extension data. We could then debate whether we should also lift the recommendation that RPs reject responses with extensions they didn't request, or if the RP should have to opt into receiving this extension output.

I agree that putting this in extension data is not ideal, but it might be the best option here since a single reserved bit isn't quite enough. What do others think?

@ve7jtb
Copy link
Contributor

ve7jtb commented Mar 13, 2019

I can see an argument for the RP to create a credential that can only be used with an allow list. I however don't think it will get used much, but if we think we may want prohibited now would be the time to add it.

It would make it clear to some platform authenticators that they need to require an allow list even if the credential is storred locally.

I suspect the best way to report back is extension data. The custom bits will get used too quickly.
If the RP wants to know if the credential is resident vs server they could send the extension with it being essentially empty. Or we could allow unsolicited extensions. It could be a slightly more generic credential meta-data extension in case we want to add some other new info about the created credential.

@emlun
Copy link
Member

emlun commented Mar 13, 2019

I can see an argument for the RP to create a credential that can only be used with an allow list.

Ah, I hadn't considered that aspect of it, that does make sense.

One issue with unsolicited extensions is that RPs that implement the L1 RP operations as written would reject L2 authenticators, so if we want to go down that route it's probably best to try to move this quickly - maybe even issue errata for the L1 RP ops.

@sbweeden
Copy link
Contributor Author

sbweeden commented Mar 13, 2019

The point of this proposal is not really centred on an argument that the RP wants to create a credential with an allowList. It's centred on the fact that many authenticators (including existing U2F authenticators) are incapable of creating a resident key credential and the RP doesn't want to prevent them from registering, and more to the point wants to allow them to register in one go, without an error experience.

@nadalin
Copy link
Contributor

nadalin commented Mar 13, 2019

@sbweeden and @equalsJeffH to work on creating PR

@ve7jtb
Copy link
Contributor

ve7jtb commented Mar 13, 2019

I think Jeff had ResidentKeyRequirement prohibited in a previous proposal. [issue #1149]

The only reason I can think of for someone wanting that is so the credential would require an allow list.
I think that use case is more or less covered with the privacy extension [credProtect extension in CTAP (WIP)], requiring a allow list or UV/PIN.

Some authenticators always make resident credentials even if non-resident is requested. Those credentials do currently show up if no allow list is sent.

If we don't have a real use case for forcing non-resident, then we should probably not include it.

If people are doing a second-factor flow they probably should not be asking for preferred.
A good number of authenticators have limited slots for resident credentials. Using them up for credentials that are always going to be used with an allow list seems like a waste. The authenticator will fill up and only be able to create non-resident after the first number of sites.

I think we need required, preferred and something like discouraged, indifferent, or ambivalent so that the user agent can prefer non-resident on roaming authenticators. More or less the existing behaviour with the addition of preferred.

@selfissued
Copy link
Contributor

John's reasoning sounds right to me.

@sbweeden
Copy link
Contributor Author

In follow-up from the working group call on March 13 and other discussion here, the semantics of the Web IDL changes proposed by @emlun seem agreeable. There was also a preference amongst the majority of those who spoke up to use extensions to communicate what the authenticator actually used rather than a flags bit. This comes with the caveat that there will never be a guarantee that the RP will know if a resident key credential has been provisioned because all extensions are OPTIONAL. The hope is that the proposal is really useful and will therefore be adopted by clients and authenticators.

The indication of resident/non-resident key type is really only needed as a registration extension, but might be useful also as an authentication extension as it could allow RP's to discover the type of credential for existing registrations that were created under L1.

As such, I'll propose the following extension definition which covers both the create() and get() flows. If there is objection to using it on get(), then that part can be easily dropped. The extension is designed in a way such that if there are any other credential properties that might be of use, these can be included in the outputs dictionary. I've defined the extension as an authenticator extension as it is undefined as to whether or not the client has any way (other than for platform authenticators) to know what the authenticator provisioned or used. Clients may augment the extension output if they do have this knowledge. If anyone has further input on this, please chime in - I'm hoping to use the IDL updates proposed by @emlun plus this extension to form the basis of a PR for this issue.

10.10 Credential Properties Extension

This extension allows WebAuthn Relying Parties to query additional properties of the authenticator's credential created during a registration ceremony, or used during an authentication ceremony.

Extension identifier
credProps

Operation applicability
Registration and Authentication

Client extension input
The Boolean value true to indicate that this extension is requested by the Relying Party.

    partial dictionary AuthenticationExtensionsClientInputs {
        boolean credProps;
    };

Client extension processing
None, except creating the authenticator extension input from the client extension input.

Client extension output
Returns the dictionary of discovered credential properties as reported by either the authenticator itself or by the client if known, such as when a platform authenticator is used. This dictionary is initially defined with one credential property rk to indicate if the authenticator has created or used a resident key credential. Other credential properties MAY be returned in this extension.

    partial dictionary CredentialPropertiesOutput {
    	boolean rk;
    };

    partial dictionary AuthenticationExtensionsClientOutputs {
        CredentialPropertiesOutput credProps;
    };

Authenticator extension input
The Boolean value true, encoded in CBOR (major type 7, value 21).

Authenticator extension processing
None, other than to report on credential properties in the output.

Authenticator extension output
The authenticator sets the authenticator extension output credProps of type CredentialPropertiesOutput encoded as a CBOR map.

@emlun
Copy link
Member

emlun commented Mar 14, 2019

Other credential properties MAY be returned in this extension.

I'm positive to this idea, but I think we'd better try to make sure that nonstandard property additions don't collide with any standard properties added in the future. I see a few obvious ways to do this:

  1. Require that any new property names are added to the WebAuthn registry.

  2. Require that any nonstandard property names begin with x-, or similar, as is done in many other standards like HTTP headers.

  3. Add a second optional property containing an unspecified dictionary, something like:

    partial dictionary CredentialPropertiesOutput {
        boolean rk;
        dictionary ext;
    };
    

    where anything may be added in the ext dictionary. I'm not completely sure WebIDL allows this, though.

Alternatively we can just forbid nonstandard properties instead. What do others think?

@christiaanbrand
Copy link

Way too much to read here... so I'll start afresh.

I'll just add my 2'cs here: Just as we have a tri-state for UV, we need a tri-state for RK.
It needs to be {Required, Allowed, Forbidden/Disallowed}.

And there's a very particular reason why an RP might want to do Forbidden/Disallows: If they really really really don't want to deal with PINs set up on tokens. I believe we now have consensus, that when a non-RK credential is created, EVEN IF A PIN IS SET UP ON A TOKEN, if UV=False, it will not require the user to enter a PIN during creation. I need a way to force that a credential is non-resident in order to get that behavior, hence this property.

Actually, I think even if someone is making a resident credential, if UV=false it shouldn't require a PIN (or other form of UV), but this once seems to require more convincing.

@ve7jtb
Copy link
Contributor

ve7jtb commented Mar 14, 2019

Cristian

I see your argument for disallowed if CTAP changes the pin rules for creating non resident and keeps the pin as a requirement for resident.

I think we still have a problem if in your tri state allowed is interprited as the platform making it resident if it can.

I suspect that we really have 4 states.
Reqired - Must be resident or error (Same as current true)
Preferred - Create a resident credential if possible, but can return non resident if that is the only option
Allowed - Create a non resident credential, but for authenticators that only support resident allow those to be created. (Same as the current false) note Windows and potentialy Android platform authenticators currently allways make resident credentials
Forbidden - make a credential that requires an allow list and won't require a device pin or UV (platform authenticators can flag credentials to require the allow list to avoid returning an error.

We don't really want authenticators returning errors for Forbidden. That would cause RP to avoid using it.

I take it that what you want with forbidden is really just that the user is not prompted for PIN or UV just UP top make a credential. The allow list requirement is really just a way to get that based on the proposed CTAP changes.

If CTAP didn't require a PIN/UV for make credential if UV is not required in the request then you might not need forbidden?

Just trying to inderstand the interaction between this and the proposed CTAP changes.

Not all authenticators have infinate space for resident credentials.
Our current options of resident required and

@emlun
Copy link
Member

emlun commented Mar 14, 2019

With the introduction of "Forbidden: won't require a device pin or UV" I feel like the scope has expanded. Should we really try to cram all of these sematics into a single option, or should we instead do all of this in the new extension and let it both specify and query for credential properties?

@sbweeden
Copy link
Contributor Author

sbweeden commented Mar 14, 2019

I think the model is ok, don't see why we don't just add forbidden to the ResidentKeyRequirement enum. My POV is that this should be part of core spec rather than an extension.

@nadalin
Copy link
Contributor

nadalin commented Mar 20, 2019

@sbweeden to create PR

@sbweeden
Copy link
Contributor Author

There is an interesting usability discussion for password-less (and username-less) authentication in the FIDO-DEV group here:

https://groups.google.com/a/fidoalliance.org/forum/#!topic/fido-dev/ALQj3JXuyhs

sbweeden added a commit to sbweeden/webauthn that referenced this issue Mar 25, 2019
@sbweeden
Copy link
Contributor Author

@equalsJeffH @ve7jtb @emlun - can you guys take a look at PR #1191 and see if you think this is headed in the right direction?

@emlun
Copy link
Member

emlun commented Mar 26, 2019

I think the forbidden value requires a breaking change to the authenticator API.

Currently the client simply forwards the options.authenticatorSelection.requireResidentKey (or false if not set) to the authenticatorMakeCredential operation. true means the authenticator MUST create a client-side-resident credential, and false means the authenticator MAY create a client-side- or a server-side-resident credential at its discretion.

PR #1191 as currently written adds some logic where the client will contextually ignore authenticators based on their characteristics, but still ultimately sends the same requireResidentKey parameter to authenticatorMakeCredential. This is probably the best we can do without breaking the authenticator API. However, when the authenticator is capable of both client-side- and server-side-resident credentials, then the existing Boolean requireResidentKey parameter to authenticatorMakeCredential doesn't capture the requirement that the authenticator MUST NOT create a client-side-resident credential.

So I think we need to either

  1. break the authenticator API, by introducing a new parameter that replaces requireResidentKey, or
  2. drop the forbidden value.

(1) obviously has the downside of requiring change to CTAP as well, but it also comes with additional benefits for preferred and indifferent, as authenticators capable of both storage modalities can then decide based on context. For example, an authenticator with limited storage space could create a resident credential when preferred if there are 10 or more storage slots available.

Thoughts on that?

@ve7jtb
Copy link
Contributor

ve7jtb commented Mar 27, 2019 via email

@emlun
Copy link
Member

emlun commented Mar 28, 2019

For what it's worth, the addition of the credProps extension is already "breaking" in the sense that it requires authenticators to change to support it. But it is of course less disruptive if we don't have to change the API.

@akshayku
Copy link
Contributor

akshayku commented Mar 28, 2019

Joining the party late. :) Too much to read here..


Requirements:

  • By Christiaan: Don't want PIN during registration in general. Don't care about whether resident or not yet. Have a consensus for non-resident keys for external authenticators. Want same for resident keys also, but consensus is not there yet.

  • By Me: Don't force authenticators to not require user verification if they don't want to. Some authenticators (external or platform) probably want to improve security and will always ask for user verification. It may be required for upper levels as per SPWG. Don't force them to create a credential without user verification if they don't want to. They may want to have a differentiation or may have other security requirements. Applies to both resident keys as well as non-resident ones.

  • By Me: Don't force authenticators to create non-resident keys if they don't want to. Some authenticators (external or platform) who don't want to/can't export private keys even in encrypted format. Don't force them. This is the current semantics of requireResidentKey = false. Don't loose that.

  • By Shane: Tell RP what kind of credential was created. Resident or not resident. Probably will prefer for resident keys but OK with non-resident keys also. Don't error out when resident keys are not supported. Which is the case for requireResidentKey = false

Let me know if I am not capturing the requirements correctly or missing something.


My Recommendation:


Input:

enum ResidentKeyRequirement {
    "discouraged",
    "preferred",
    "required"
};

Where

  • required means create resident keys and error out if you can't
  • preferred means try to create a resident keys but if that is not supported, try to create a non-resident one
  • discouraged means try to create a non-resident keys but if that is not supported, try to create a resident one.

Output:

    partial dictionary CredentialPropertiesOutput {
    	boolean rk;
    };

    partial dictionary AuthenticationExtensionsClientOutputs {
        CredentialPropertiesOutput credProps;
    };


@emlun
Copy link
Member

emlun commented Mar 28, 2019

@akshayku Do you mean that the ResidentKeyRequirement value would be sent to all the way to the authenticator, so that it can decide on its own what to do in case of preferred or discouraged? That would require the same breaking API change as forbidden does.

@akshayku
Copy link
Contributor

It doesn't require CTAP changes.

Platform will take care of this w.r.t CTAP communication.

  • required: Platform will send rk=true to the authenticator and will fail if that call fails.
  • preferred: Platform will send rk=true to the authenticator and if that fails, will send another command to the authenticator with rk=false (or omit this parameter)
  • discouraged: Platform will send rk=false (or omit this parameter) to the authenticator and if that fails, will send another command to the authenticator with rk=true

@emlun
Copy link
Member

emlun commented Mar 28, 2019

Ok, in that case it seems to me more like "allowed" or "indifferent" than "discouraged" - because rk=false doesn't communicate that the RP has an active preference for non-RK, right? (as opposed to a passive indifference, which is what rk=false is specified to mean in L1)

@akshayku
Copy link
Contributor

RP may have active preference for non-RK.

Christiaan wants it so that PIN doesn't happen as that is where the general consensus is in FIDO community. But that should not be a absolute requirement, IMO, for the authenticators . An authenticator may want to do always user verification because of security levels or differentiation or whatever.

Another RP may actually want it because, it is creating a bunch of credentials and storage is limited. So he may actually prefer non-RK. But from authenticators side, in some cases, there is no issue with storage or it can't/don't want to do non-resident keys. So we have to allow that also.

RP wants one variable, which works across all kind of authenticators. Hence the fallback mechanism.

@akshayku
Copy link
Contributor

akshayku commented Mar 28, 2019

Regarding rk=false, in practicality, that parameter is for external authenticator and all vendors are actually doing non-resident keys today when rk=false (or ommitted). I would prefer to clarify CTAP spec that rk=false means non-resident keys.

@emlun
Copy link
Member

emlun commented Mar 28, 2019

Ok, but in that case we would have to issue errata for WebAuthn L1 also, since authenticatorMakeCredential step 7.4 reads (bold emphasis added)

  1. If requireResidentKey is true or the authenticator chooses to create a client-side-resident public key credential source:

Can we do that? I suspect there are platform authenticators that create an RK even if rk=false.

@akshayku
Copy link
Contributor

akshayku commented Apr 3, 2019

Whatever we are discussing if for next level. In that world, when ResidentKeyRequirement is present, requireResidentKey will be ignored. That will result in changes to the algorithms which is fine as these are enhancements.

@emlun
Copy link
Member

emlun commented Apr 3, 2019

Yes, but I took your previous remark to mean "we can redefine what rk=false means because all existing authenticators already do what we would redefine it to". What's to prevent new authenticators appearing between now and L2 release which do not behave in that same way?

@akshayku
Copy link
Contributor

akshayku commented Apr 3, 2019

What I meant was, IMO, defining rk behavior is a FIDO issue.

WebAuthn deals with residentKey or ResidentKeyRequirement which are web concepts. Platform translates these concepts to authenticators by looking at CTAP spec. I don't think we need errata for WebAuthn L1.

I will see how to define the correct behavior in CTAP spec.

@emlun
Copy link
Member

emlun commented Apr 3, 2019

Notes from 2019-04-03 WG call:

  • @akshayku echoes @sbweeden's original proposal: required, preferred, discouraged.
  • @agl remarks that "discouraged" is concerning: RP may want a credential to be usable only with allowList. Currently in CTAP2 and WebAuthn L1, requireResidentKey: false doesn't guarantee that.
  • "forbidden" would ease @agl's concerns; unclear if we can guarantee that much. In practice few existing authenticators create RK when given requireResidentKey: false, so maybe we can retroactively modify requireResidentKey: false to mean "RK forbidden"?
  • @emlun thinks there should also be an "indifferent" value in addition to "discouraged", because requireResidentKey: false maps closer to "indifferent" than to "discouraged".
  • Broad agreement that we should reformulate the descriptions of resident keys to be more focused on the aspect "can be used with empty allowList" rather than "stored in the authenticator".

sbweeden added a commit to sbweeden/webauthn that referenced this issue Apr 17, 2019
…have changed the set of permitted ResidentKeyRequirement values back to one of discouraged, preferred, required.
akshayku pushed a commit that referenced this issue May 8, 2019
…ments (#1191)

* Initial proposal for modifications to resident key credential requirements to address
#991

* No-op change to force re-check of ipr.

* Change back to IBM

* Resolve first round of feedback from @emlun

* Fix editorial issue reported by @emlun

* Add definition of non-resident credential and reference it in new ResidentKeyRequirement enum definitions.

* Rewrite resident key option handling as switch statements

* Incoporated meta-PR from @emlun and addressed current round of comments.

* Based on comments from WG call on 2019-04-03 (captured in #991) I have changed the set of permitted ResidentKeyRequirement values back to one of discouraged, preferred, required.

* Updates based on review feedback and discussion of WG call on 2019-04-17.

* Address all review comments from week of April 17-24, 2019.

* Address feedback from @agl

* Minor update to credProps extension description per @emlun

* Addressed comments from @equalsJeffH

* Address a bunch of comments from @equalsJeffH that I missed in the conversation.

* Additional review fix from @equalsJeffH

* Fix minor typo spotted by @equalsJeffH

* build on credProps dfn to formalize authnr/client processing

* Minor edits to fix compile issue and grammar in the CredProps extension

* Per WebAuthn call on May 1, 2019, remove authenticator extension component of the new credProps exception.

* Address editorial comments from @emlun and @equalsJeffH

* Minor editorials from @emlun
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment