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

[Thomas Lamprecht]

On 22.02.21 15:47, Dominik Csapak wrote:
> 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.

hmm, OK

> 
>>
>>> 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

for the record, some yubikey 5 got ordered, so we can check that too.

> 
>>
>> 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

all browser we support? I.e., Firefox and Safari too?

If that's the case the change seems sensible, lets see what Wolfgang
thinks..

> 
> 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 at 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'),
>>>
>>
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
>