Verifying CredentialID has not been previously registered and updating credential

[zacknewman]

According to finish_securitykey_registration, "You MUST assert that the registered credential id has not previously been registered. to any other account." Is this only true if one did not pass exclude_credentials into start_securitykey_registration? If not, what's the point of passing exclude_credentials if you always have to check when registration is finished? Additionally, "to any other account" means any account associated with the user UUID, correct? Or does it mean one has to check every CredentialID for every user UUID?

update_credential never returns Some(false) when needs_update returns true, correct? Asked more formally, does the following logical biconditional hold:

(∃ key | key.update_credential(res).unwrap()) ⇔ res.needs_update()

[Firstyear]

You need to check it's not registered to any other users accounts and we don't have visibility into that. We can only see what's excluded from this users account.

[Firstyear]

Also I can't read that formal math syntax, so I don't know what it means.

[zacknewman]

You need to check it's not registered to any other users accounts and we don't have visibility into that. We can only see what's excluded from this users account.

So if exclude_credentials contains all CredentialIDs from all user UUIDs, does one still have to verify that the CredentialID is not used after calling finish_securitykey_registration? If so, then passing exclude_credentials seems to not serve any purpose since all of the work that went into creating exclude_credentials needs to happen again after calling finish_securitykey_registration.

Also I can't read that formal math syntax, so I don't know what it means.

The English terminology was used before that: update_credential never returns Some(false) when needs_update returns true, correct?

As an explicit example in non-compilable Rust:

use webauthn_rs::prelude::{PublicKeyCredential, SecurityKey, Webauthn};
fn main() {
    let mut keys: [SecurityKey; 1]; // Pretend this is instantiated with a `SecurityKey`.
    let authn: WebAuthn; // Pretend this is instantiated.
    let (challenge, auth) = authn.start_securitykey_authentication(keys.as_slice()).unwrap();
    // Send data to client and retrieve response.
    ⋮
    let pub_cred: PublicKeyCredential; // Pretend this is instantiated from imaginary response above.
    let res = authn.finish_securitykey_authentication(&pub_cred, &auth).unwrap();
    // Is it possible this will `panic`?
    assert!(!res.needs_update() || key.update_credential(&res).unwrap());
}

[Firstyear]

You need to check it's not registered to any other users accounts and we don't have visibility into that. We can only see what's excluded from this users account.

So if exclude_credentials contains all CredentialIDs from all user UUIDs

You are misunderstanding.

Exclude_credentials is only this users credentials, to stop the situation where the user has say two yubikeys connected. If one is enrolled and the other is about to be enrolled, it disables the already enrolled key from proceeding, and then we double check that in the registration process.

, does one still have to verify that the CredentialID is not used after calling finish_securitykey_registration? If so, then passing exclude_credentials seems to not serve any purpose since all of the work that went into creating exclude_credentials needs to be happen again after calling finish_securitykey_registration.

Then afterwards, you need to check "okay, is this credential ID reused by any other users". That's why it's separate.

[zacknewman]

I guess if one has to verify that the used CredentialID is not used by anyone, then that's a more extreme condition than ensuring the CredentialID is not one used by the current user (the point of exclude_credentials).

Or perhaps let's ask this in a specific situation with some form of database where CredentialID is the PRIMARY KEY in some table. I know I will have to verify the CredentialID does not exist in this table after finish_securitykey_registration is called. Since this column already includes CredentialIDs associated with the current user (among all others), there seems there is nothing to gain by using exclude_credentials. In fact, in this situation it's "worse" to use exclude_credentials because that's an extra database query you have to do in addition to the (simpler since only one column, CredentialID, needs to be used in the query) existence check query you'll have to do.

[Firstyear]

If you already have a way in the database to enforce that credential ID is unique, then you have already satisfied this requirement, yes.

[zacknewman]

Hm, I'd be interested to know where it makes sense to use exclude_credentials no matter how contrived. The best I can come up with is when there is only one user, so exclude_credentials = "all credentials". Even then though, it's the same amount of work to use exclude_credentials and not need to worry about checking again vs. not using exclude_credentials and then checking when registration is done.

[Firstyear]

exclude credentials has size limits on some authenticators. Remember, exclude credentials is transmitted in the registration to the users browser.

Also when you have 100,000 users, they each enroll two credentials, then you just sent 200,000 credentials to a user for every registration. That's going to cause problems no matter how you cut it.

I think you need to consider exclude_credentials as a ui hint to help guide one user to not duplicate a credential. And then the actual check for uniqueness is enforced later.

[zacknewman]

In that situation it's better to not use exclude_credentials since verifying that none of the 200K credentials match includes the current user's. So you're duplicating a subset of the work by using exclude_credentials even when it only contains the current user's IDs.

[Firstyear]

You have to sent exclude credentials so that the users browser doesn't try to register the same security key twice. That's what it's for.

So you have to use it to make sure the users experience is a good one. Then you do the uniqueness check after.

[Firstyear]

You have to sent exclude credentials so that the users browser doesn't try to register the same security key twice. That's what it's for.

And when I say this, I mean there is actually specific logic in browsers to disable credentials in the excluded credentials list so they don't trigger during registration.

[zacknewman]

I understand that when the browser receives excluded_credentials it won't use any of those credentials (modulo bugs); however it's not sufficient to only disable those credentials since all credentials by all users need to not be registered. So it's kinda pointless.

Imagine I told you that you are not allowed to pick a number that you or anyone else in a stadium has chosen. As the limit of the numbers previously chosen approaches infinity, avoiding only your previously chosen numbers has 0% impact on avoiding a collision. This completely ignores the fact that so long as you have only chosen a few numbers that you can recollect quickly avoiding your previous numbers is "free". In the real world, retrieving those numbers is almost certainly not going to be free.

[Firstyear]

I understand that when the browser receives excluded_credentials it won't use any of those credentials (modulo bugs); however it's not sufficient to only disable those credentials since all credentials by all users need to not be registered. So it's kinda pointless.

No, it's not.

You are setting exclude credentials so that in the majority case, most users will have their UI guided to help them select and enroll the right credential.

And then if a malicious person was trying to register a duplicate credential, it's caught post registration with a "hey, maybe you shouldn't do that".

You are looking at this only from the simple view of "exclude is about excluding all these things to enforce uniqueness". It's not. It's there to help users experience to make sure that in the "happy path" they get a good experience, and then the application still can enforce the stricter rule later.

[zacknewman]

Are you saying that a browser has a high probability of re-registering an existing credential when registering a new credential?

I was under the impression that the CredentialID is "randomly" generated; thus the probability of duplicating one of the very few CredentialIDs already registered by the user is incredibly small.

[Firstyear]

I have two yubikeys. I register the first. Now I go to register the second.

exclude_credentials is set to exclude the first key. Now only my second key activates. It means I don't accidentally re-enroll the first key.

That's what it's for.

[zacknewman]

What are the odds that it re-enrolls the first key? Is this not random? If not, then I get it; however if so, then mathematically it is silly to pre-avoid collision of a very small quantity of values. If random, it's almost guaranteed to be the case that any collision that occurs is due to the CredentialIDs used by all the other users and not the current.

[Firstyear]

Why are you thinking about this like odds? Like it's statistics? You're approaching this in the completely wrong way.

Imagine you have someone where they have two yubikeys are connected, they are both blinking. maybe they are elderly. Maybe they're in a rush. Maybe they got an sms from someone. Humans are human, they are distracted. Who knows. They look at their keys. Which one do they press? They know they just enrolled one, but now they are both blinking? What do they press now? Why are they both lit up? They're confused, they then accidentally press the first key again. They get a weird error they don't understand and it confuses them. They don't know what they did wrong. They get annoyed, and have to repeat the process and they try again.

All this could be easily avoided, if during the registration only one key lights up. They know exactly what to press and interact with. It's unambiguous. It helps them have a better experience. It guides the person to the right action.

This is about human interaction, it's about empathy with others, it's about making sure that we give people a good, clear, and helpful experience.

[zacknewman]

Because I like to be objective and scientific when possible. If something doesn't make sense logically, then I have a hard time justifying it. So far you have not supplied a logical argument why exclude_credentials make sense until potentially your most recent example which I would like to pursue.

In your example, you're saying if I plug in two YubiKeys and register only one of them to a service that does not send exclude_credentials, both of my keys will light up and I have to guess which one I actually registered. However had this service sent exclude_credentials, only the YubiKey that was registered will light up? Is that correct? If so, then I get it; but I will likely test that theory out to see if it's true.

[Firstyear]

Yes that's exactly what I've been saying the whole time.

[Firstyear]

And if the keys DO light up incorrectly, that's a browser bug - not a webauthn-rs bug.

[zacknewman]

You were not clear.

Edit

Yes, they were clear.

[zacknewman]

And what about the code example question?

use webauthn_rs::prelude::{PublicKeyCredential, SecurityKey, Webauthn};
fn main() {
    let mut keys: [SecurityKey; 1]; // Pretend this is instantiated with a `SecurityKey`.
    let authn: WebAuthn; // Pretend this is instantiated.
    let (challenge, auth) = authn.start_securitykey_authentication(keys.as_slice()).unwrap();
    // Send data to client and retrieve response.
    ⋮
    let pub_cred: PublicKeyCredential; // Pretend this is instantiated from imaginary response above.
    let res = authn.finish_securitykey_authentication(&pub_cred, &auth).unwrap();
    // Is it possible this will `panic`?
    assert!(!res.needs_update() || key.update_credential(&res).unwrap());
}

[Firstyear]

You were not clear.

https://github.com/kanidm/webauthn-rs/issues/401#issuecomment-1861852361

[yaleman]

Mozilla's explanation of the client side might help.

If the create() call is attempting to create a duplicate public key credential on an authenticator, the user agent will guide to user to create the credential using a different authenticator, or fail if that is not possible.

Or perhaps the w3c discussion.

We include it because it's part of the spec, not for the fun of it.

(edit: spelling)

[Firstyear]

And what about the code example question?

use webauthn_rs::prelude::{PublicKeyCredential, SecurityKey, Webauthn};
fn main() {
    let mut keys: [SecurityKey; 1]; // Pretend this is instantiated with a `SecurityKey`.
    let authn: WebAuthn; // Pretend this is instantiated.
    let (challenge, auth) = authn.start_securitykey_authentication(keys.as_slice()).unwrap();
    // Send data to client and retrieve response.
    ⋮
    let pub_cred: PublicKeyCredential; // Pretend this is instantiated from imaginary response above.
    let res = authn.finish_securitykey_authentication(&pub_cred, &auth).unwrap();

If an excluded credential was re-used, the line above will have an error.

// Is it possible this will `panic`?
assert!(!res.needs_update() || key.update_credential(&res).unwrap());

}

[zacknewman]

I'm referring to the assert (i.e., pretend all unwraps don't panic).

[zacknewman]

Basically, I'm wondering if there is any benefit to calling needs_update and not simply calling update_credential? There is a benefit if it's possible update_credential returns Some(false), but needs_update returns true since code should seemingly error/panic/retry.

[Firstyear]

needs_update() is true if:

• The credentials internal activation counter has increased (applies to all hardware FIDO2 devices)
• If the backup state has changed (may change from false to true on certain classes of software devices like a password manager)

The point of checking needs_update() is to allow you to shortcut the update_credential() path if it is false. It's useful to check needs_update() before you dispatch a credential update to an async worker or begin a (potentially costly) db write txn.

[Firstyear]

Which also is why update_credential can return false, because if you dispatch the update events asynchronously, they can become out of order meaning that the counter update doesn't need to apply since the counter is already now greater than the value in the authentication request.

[zacknewman]

OK, so the example I sent is fine since there is no concurrency nor "expensive" operations done that could have been avoided via needs_update. The examples you provided are reasons why needs_update exists.

Thank you for time and patience with me, and I apologize for completely missing your first explanation for why exclude_credentials should be used. I almost feel like that shouldn't even be an Option now.

[zacknewman]

One last question since I have you here, is it worth trying to see if getTransports works? The question mark at the end of the comment makes it seem like not all avenues have been tried to extract that info, and I am willing to experiment.

[Firstyear]

getTransports is cooked on all browsers. The only reliable way to check transports is with attested passkeys, and we do this already.

[zacknewman]

Interesting, so even if I pass an attestation_ca_list into start_securitykey_registration, it won't work? Bummer.

[Firstyear]

If the credential is attested, the transport will be automatically added, even for security keys. I forgot security key does attestation.

[zacknewman]

Does enabling danger-user-presence-only-security-keys affect that? I ask since "transports":null is in the JSON that is stored on a practice database. Here is the SecurityKey JSON that is serialized after registering:

[{"id":1,"name":"foo","security_key":{"cred":{"cred_id":"888BhPns60NbF2xYBov4PGx2HJo5FO3M2setPFzbP6zYtz8QMlmHF0CP1_Wkrlnd8O_39cpKA2pye5Q7a5u6_Q","cred":{"type_":"ES256","key":{"EC_EC2":{"curve":"SECP256R1","x":"vGD_s1abr7JJFUklduidfHe6CEWuw63kyit1VFsq7LI","y":"6nFipgOlhFYmHYikxVVcMNddk2X4-elgoy3r8L9_yJs"}}},"counter":1,"transports":null,"user_verified":true,"backup_eligible":false,"backup_state":false,"registration_policy":"discouraged","extensions":{"cred_protect":"NotRequested","hmac_create_secret":"NotRequested","appid":"NotRequested","cred_props":"Ignored"},"attestation":{"data":{"Basic":["MIIC2DCCAcCgAwIBAgIJAPkeEJBFzRXcMA0GCSqGSIb3DQEBCwUAMC4xLDAqBgNVBAMTI1l1YmljbyBVMkYgUm9vdCBDQSBTZXJpYWwgNDU3MjAwNjMxMCAXDTE0MDgwMTAwMDAwMFoYDzIwNTAwOTA0MDAwMDAwWjBuMQswCQYDVQQGEwJTRTESMBAGA1UECgwJWXViaWNvIEFCMSIwIAYDVQQLDBlBdXRoZW50aWNhdG9yIEF0dGVzdGF0aW9uMScwJQYDVQQDDB5ZdWJpY28gVTJGIEVFIFNlcmlhbCAyNjk0OTE5NjEwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAATxBlFac6ZCOTaesMSVQq-qRj_pCX5LiTu2t9QhAnsXkanB0XQM4d_lfJW_YIlWvegwqiDVb9IFIBOHWC5patxso4GBMH8wEwYKKwYBBAGCxAoNAQQFBAMFBAMwIgYJKwYBBAGCxAoCBBUxLjMuNi4xLjQuMS40MTQ4Mi4xLjcwEwYLKwYBBAGC5RwCAQEEBAMCBSAwIQYLKwYBBAGC5RwBAQQEEgQQ7ogoeXIcSROXdT38zpcHKjAMBgNVHRMBAf8EAjAAMA0GCSqGSIb3DQEBCwUAA4IBAQCocCl0EQ8Ke0ySq5k4TyiCmDNWPz8KRz8lg4Gpliwg4KuWfK3S7klIfaqDpEkYUtlYsZPfckEL-_0u0ldjNzG63NwmEmFmKmAA1fVYRRSTfhiY6eIWeikeEajFkatlckQ7apazTTxCx2oO53B7PoGVzznPb1BcUKyCClGzrJg703nbNuwxcnGx6A6ctOomTC3InptvVPkVsC7ekYiTgAPanPJw0jCDSjPEaLVVh3Y7U79siV9fGFoazCoCwDpJ0AusKEZ-9HgZ9nx-zBUaHyPgiLetfCIh6y-yX68Jhc1QaWXOfFro81TLwc3kzKQe83B16eILycdxFRZWOROW2YpK"]},"metadata":{"Packed":{"aaguid":"ee882879-721c-4913-9775-3dfcce97072a"}}},"attestation_format":"Packed"}}}]

[Firstyear]

Nope, shouldn't affect it.

[Firstyear]

Transports was only added recently in git master btw.

[zacknewman]

I'm using 0.4.8, so that's probably it. Thanks for your time.