From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 5A90C69132 for ; Tue, 23 Feb 2021 08:49:33 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 5237919DDC for ; Tue, 23 Feb 2021 08:49:33 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id E15D619DCF for ; Tue, 23 Feb 2021 08:49:29 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id ACC1741B53 for ; Tue, 23 Feb 2021 08:49:29 +0100 (CET) Message-ID: <85852eef-25d6-a4bb-b70b-15f01fa45eec@proxmox.com> Date: Tue, 23 Feb 2021 08:49:28 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:86.0) Gecko/20100101 Thunderbird/86.0 Content-Language: en-US To: Proxmox Backup Server development discussion , Dominik Csapak References: <20210222094301.13858-1-d.csapak@proxmox.com> <20210222094301.13858-4-d.csapak@proxmox.com> From: Thomas Lamprecht In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.556 Adjusted score from AWL reputation of From: address CTE_8BIT_MISMATCH 0.999 Header says 7bits but body disagrees KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [googlesource.com, discourse.group, tfa.rs, user.id, proxmox.com] Subject: Re: [pbs-devel] [PATCH proxmox-backup 3/3] config/tfa: webauthn: disallow registering a token twice X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 23 Feb 2021 07:49:33 -0000 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, 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 >>> --- >>>   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 { >>> +        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@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel > >