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

Improving authentication with hardware keys / FIDO2 / webauthn + 2FA #21675

Open
3 tasks
noerw opened this issue Nov 3, 2022 · 5 comments
Open
3 tasks

Improving authentication with hardware keys / FIDO2 / webauthn + 2FA #21675

noerw opened this issue Nov 3, 2022 · 5 comments
Labels
topic/authentication topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! type/proposal The new feature has not been accepted yet but needs to be discussed first.

Comments

@noerw
Copy link
Member

noerw commented Nov 3, 2022

The Problem


Possbile Solutions

(1) The Very Simple Fix

Revert #11573, but otherwise leave the auth conditions as they are. Fixes the UI inconsistency and the no-backup code situation, implements github's authentication logic, but does not enable passwordless login and makes phishable credentials mandatory.

(2) The Somewhat Simple Fix

  • Refactor the web UI: hardware key UI is moved to into the 2FA UI panel, so it's clear that they are not for passwordless auth.
  • Require either >=2 fido keys, or generate backup keys, when registering the first fido key. The login flow might also need changes to accept backup codes when no TOTP is registered.

This mostly keeps the current auth logic, except for requiring a backup second factor to be set up:

username && pw && (totp || fidokey1 || fidokey2 || backupcodes)

This auth logic does not require phishable credentials.

(3) The Correct Solution

The most correct way imho would be to implement the auth logic that is outlined below as Nextcloud's implementation.
Effectively the needed changes would be:

  • allow registering hardware keys as primary or secondary factor
  • migrate existing hardware keys to secondary factor keys
  • change auth logic of primary factor keys to actually be replacements for password auth

This would result in the following auth logic:

username && (pw || fidokey1 || fidokey2) && (totp || fidokey3 || backupcodes)

Of course not all of those credentials need to be used, all but one from each block could be unavailable/unknown. This option has the benefit to get along without mandatory phishable credentials (password, TOTP), as well as enabling passwordless auth.


Context

Gitea's current (as of 1.18.0+dev-558-g94d6d93cc, 94d6d93) authentication behaviour is as follows:

context auth requirements
default username && pw
TOTP 2FA enabled username && pw && (totp || backupkeys)
(multiple) fido2 keys registered username && pw && (fidokey1 || fidokey2)
TOTP 2FA enabled & fido2 keys registered username && pw && (fidokey1 || fidokey2 || totp || backupcodes)

For reference some other implementations of FIDO2:

implementation context auth requirements
github, mastodon default username && pw && emailed-token
TOTP 2FA enabled username && pw && (totp || backupkeys)
(multiple) fido2 keys registered case not allowed, must enable 2FA
TOTP 2FA enabled & fido2 keys registered username && pw && (fidokey1 || fidokey2 || totp || backupcodes)
nextcloud default username && pw
TOTP 2FA enabled username && pw && (totp || backupkeys)
(multiple) fido2 keys registered username && (pw || fidokey1 || fidokey2)
TOTP 2FA enabled & fido2 keys registered username && (pw || fidokey1 || fidokey2) && (totp || backupcodes)

As you can see, there is no clear correct de-facto implementation, I have yet to check fido2/webauthn spec if there is a correct approved way of implementing things. However there is a strong preference in the wider community on how to do things correctly with security and usability in mind:


some SEO optimization: webauthn u2f security key 2fa totp fido fido2
@noerw noerw added type/proposal The new feature has not been accepted yet but needs to be discussed first. type/feature Completely new functionality. Can only be merged if feature freeze is not active. topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! topic/authentication and removed type/feature Completely new functionality. Can only be merged if feature freeze is not active. labels Nov 3, 2022
@lunny
Copy link
Member

lunny commented Nov 4, 2022

I would like the first implementation(github, mastodon), nextcloud's looks like a 3FA when TOTP 2FA enabled & fido2 keys registered

@noerw
Copy link
Member Author

noerw commented Nov 4, 2022

nextcloud's looks like a 3FA when TOTP 2FA enabled & fido2 keys registered

If you look closely, you see that it's still 2FA: username && (pw || fidokey1 || fidokey2) && (totp || backupcodes).

The decision here is how much effort the gitea project is willing to put into changing this auth logic, with the possible gain of having an unphishable authentication method (to be clear, Gitea currently has this property (since #11573), so we should think hard about dropping it).
That said, I don't think touching this is urgent, as there is no acute security issue with the current way this auth is handled, the UI is just misleading currently. So it may be beneficial to wait and do this properly once the resources or priority for it are there, and refactor the UI in the meantime.

@theAkito
Copy link

theAkito commented Dec 3, 2022

Was about to create a dedicated issue, but found this one, when searching for existing ones, so I'll add another improvement.
If you need a separate issue out of this, I'll make one.

Gitea's WebAuthn does not save the domain string.

Information about the topic follows.

@lunny
Copy link
Member

lunny commented Mar 8, 2023

Gitea's auth has a more complex situation. Some users want to login with a third auth server i.e. OAuth2 . I think we need to put all these situations together when refactoring/redesigning.

@f0sh
Copy link

f0sh commented Jul 19, 2023

I would like to catch up on the statement of the beginning of this issue:

It can be argued that webauthn is supposed to be passwordless auth, enabling 2FA should be a separate choice, and that mandatory TOTP 2FA is reducing security instead of increasing it due to phishing/MITM risk.

Webauthn was designed as a generic, phishing proof protocol and therefore can be used for passwordless authentication. It brings it's own function of a 2nd factor (where you can use a 2F to access your local public key) but is also downward compatible as a 2F with U2F. That's a bit confusing. Because the domain is (as the relying party RP) fundamental part of the authentication process in webauthn, there is a drastically reduced attack vector for phishing in webauthn compared to U2F, because you just cannot use your credentials on a different site, without tampering your local machine.

When it comes to the implementation, then you actually do not use (or strictly saying, you do not have to use) a username anymore, because you are working with a publickey authentication approach.

So I totally agree with what @lunny said, that

I think we need to put all these situations together when refactoring/redesigning.

because there would be actually a pool of equal authentication methods/backends, every with their own set of credentials:

  • classic username + password [+ 2FA]
  • publickey authentication (using Webauthn+FIDO2 / Passkeys)
  • API authentication (like OAuth or OpenID Connect)

This is indeed a lot of effort to implement. However, considering the move of bigtechs towards Passkeys and being already available on most platforms, this is worth the discussion and integration. Github just announced few days ago, that they now support passkey authentification as well. Paypal, Google, Apple are alreading doing it.

My point is, when tackling the described issues, I would suggest to refactor it in a way that the whole picture is considered as it is discussed in #24821. I think this will give more momentum. A first approach can be hanko.io.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic/authentication topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! type/proposal The new feature has not been accepted yet but needs to be discussed first.
Projects
None yet
Development

No branches or pull requests

4 participants