Re: [pbs-devel] [PATCH proxmox-backup 3/3] config/tfa: webauthn: disallow registering a token twice

[Dominik Csapak <d.csapak@proxmox.com>]

On 2/22/21 15:08, Thomas Lamprecht wrote:
> On 22.02.21 10:43, Dominik Csapak wrote:
>> by adding the existing credential id to the 'excludeCredentials' list
> 
> But the webauthn does not cares about this, meaning its intended to
> work.

yes it should work, but this option is exactly made for this purpose.

excludeCredentials, of type sequence<PublicKeyCredentialDescriptor>, 
defaulting to []

This member is intended for use by Relying Parties that wish to limit 
the creation of multiple credentials for the same account on a single 
authenticator. The client is requested to return an error if the new 
credential would be created on an authenticator that also contains one 
of the credentials enumerated in this parameter.

the spec recommends it even for having multiple authenticators.

Relying Parties SHOULD allow and encourage users to register multiple 
credentials to the same account.
Relying Parties SHOULD make use of the excludeCredentials and user.id 
options to ensure that these different credentials are bound to 
different authenticators.

> 
>> this prevents the browser from registering a token twice, which
>> lets authentication fail on some browser/token combinations
>> (e.g. onlykey+chromium)
> 
> isn't that a FW bug there and should be fixed there?

it seems that it is actually more a chromium/chrome bug, since
it works with firefox

there seems to be something weird with how chrome sends the
challenges [0][1] though i am not sure i completely understand
the merge request

a discussion on the onlykey forums suggests that the same problem
occurs on solokey and on a yubikey 5 too [0]

the tests we ran before were all with yubikey 4 (afaik)
which do not support fido2 only u2f, which is handled differently
in chromium

> 
> Would like to avoid such special handling for buggy FW/HW/.. especially
> if the workaround is as simple as "just don't register it twice"
> (outside of testing I never came to the idea of registering a token
> more than once in those accounts I use a fido/u2f token)

well the use case of having the same authenticator registered twice
is not really clear to me, and the browser gives a nice
error message that the key is already registered

this avoid a situation where e.g. a user wants to register multiple
tokens (for redundancy/backup) and accidentally uses the
same token twice without noticing

in case he loses the first, there would be no way to login again without
interfering from an admin or using a recovery key, etc)

0: 
https://onlykey.discourse.group/t/multiple-webauthn-registrations-fail/238/10
1: https://github.com/duo-labs/webauthn.io/issues/15
2: https://chromium-review.googlesource.com/c/chromium/src/+/1629587

> 
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   src/config/tfa.rs         | 15 +++++++++++++--
>>   www/window/AddWebauthn.js |  7 +++++++
>>   2 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/config/tfa.rs b/src/config/tfa.rs
>> index 29e0fb48..7c656d20 100644
>> --- a/src/config/tfa.rs
>> +++ b/src/config/tfa.rs
>> @@ -803,9 +803,20 @@ impl TfaUserData {
>>           userid: &Userid,
>>           description: String,
>>       ) -> Result<String, Error> {
>> +        let cred_ids: Vec<_> = self
>> +            .enabled_webauthn_entries()
>> +            .map(|cred| cred.cred_id.clone())
>> +            .collect();
>> +
>>           let userid_str = userid.to_string();
>> -        let (challenge, state) = webauthn
>> -            .generate_challenge_register(&userid_str, Some(UserVerificationPolicy::Discouraged))?;
>> +        let (challenge, state) = webauthn.generate_challenge_register_options(
>> +            userid_str.as_bytes().to_vec(),
>> +            userid_str.clone(),
>> +            userid_str.clone(),
>> +            Some(cred_ids),
>> +            Some(UserVerificationPolicy::Discouraged),
>> +        )?;
>> +
>>           let challenge_string = challenge.public_key.challenge.to_string();
>>           let challenge = serde_json::to_string(&challenge)?;
>>   
>> diff --git a/www/window/AddWebauthn.js b/www/window/AddWebauthn.js
>> index 16731a63..a3888206 100644
>> --- a/www/window/AddWebauthn.js
>> +++ b/www/window/AddWebauthn.js
>> @@ -82,6 +82,13 @@ Ext.define('PBS.window.AddWebauthn', {
>>   		challenge_obj.publicKey.user.id =
>>   		    PBS.Utils.base64url_to_bytes(challenge_obj.publicKey.user.id);
>>   
>> +		// convert existing authenticators structure
>> +		challenge_obj.publicKey.excludeCredentials =
>> +		    (challenge_obj.publicKey.excludeCredentials || []).map((cred) => ({
>> +			id: PBS.Utils.base64url_to_bytes(cred.id),
>> +			type: cred.type,
>> +		    }));
>> +
>>   		let msg = Ext.Msg.show({
>>   		    title: `Webauthn: ${gettext('Setup')}`,
>>   		    message: gettext('Please press the button on your Webauthn Device'),
>>
>