The Wayback Machine - https://web.archive.org/web/20220212203952/https://github.com/go-gitea/gitea/pull/11573
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

Allow U2F 2FA without TOTP #11573

Merged
merged 9 commits into from Nov 8, 2021
Merged

Allow U2F 2FA without TOTP #11573

merged 9 commits into from Nov 8, 2021

Conversation

@kdomanski
Copy link
Contributor

@kdomanski kdomanski commented May 23, 2020

This change enables the usage of U2F without being forced to enroll an TOTP authenticator.
The /user/auth/u2f has been changed to hide the "use TOTP instead" bar if TOTP is not enrolled.

Fixes #5410
Fixes #17495

@kdomanski

This comment was marked as off-topic.

@6543

This comment has been hidden.

@kdomanski

This comment has been hidden.

@jonasfranz
Copy link
Member

@jonasfranz jonasfranz commented May 23, 2020

IMHO We should enforce totp since u2f is currently only supported in a few browsers.

@silverwind
Copy link
Member

@silverwind silverwind commented May 23, 2020

Why unconditionally load the JS? Can we not gate it behind some variable that only loads it on pages where it's needed? As it's right now it will load on every single page which I find quite bad.

@kdomanski
Copy link
Contributor Author

@kdomanski kdomanski commented May 23, 2020

@jonasfranz It's not possible to set up U2F in a non-supporting browser in the first place.

@kdomanski
Copy link
Contributor Author

@kdomanski kdomanski commented May 23, 2020

@silverwind If I understand correctly, currently the U2F js would also load on every page, as long as the user is enrolled in TOTP. The core issue seems to be that the loading of that file happens in the footer template.

To address this, I'd move the loading to security_u2f.tmpl and u2f.tmpl in another PR.

@6543
Copy link
Member

@6543 6543 commented May 23, 2020

what to do with the API since there is no auth for U2F there, so at least we should put a hint if U2F is enabled without TOTP, to interact with the API the user has to create a token itself

@kdomanski
Copy link
Contributor Author

@kdomanski kdomanski commented May 23, 2020

Hmm, I didn't think about the API. Back to the drawing board.

@6543
Copy link
Member

@6543 6543 commented May 23, 2020

@kdomanski I think its fine we just have to inform the user ...

@rugk
Copy link
Contributor

@rugk rugk commented May 24, 2020

Thx for cc'ing, but I cannot review this PR as I don't know the technologies involved (Go etc.). That said, if it fixes #5410 as stated there, I'm happy the change get's into gitea. 😃

@kdomanski
Copy link
Contributor Author

@kdomanski kdomanski commented May 29, 2020

Updated tests which were setting U2F on root user without enabling TOTP.

@kdomanski
Copy link
Contributor Author

@kdomanski kdomanski commented May 29, 2020

@6543 Is it possible to use the API with login/password and TOTP? I couldn't find anything in the docs. I could add a hint in the body of the API unauthorized response, but it seems like it's orthogonal to this PR and belongs in another one.

@6543
Copy link
Member

@6543 6543 commented May 29, 2020

I would ad no hit to api 403 responce

and yes we should add add this to swagger docs #11667 (a other pull)

@6543
Copy link
Member

@6543 6543 commented May 29, 2020

but when adding a U2F key without an existing TOTP a hint on the UI could warn about API and account recovery etc ...

@jonasfranz
Copy link
Member

@jonasfranz jonasfranz commented Jun 1, 2020

@kdomanski Many users are using multiple devices. I think we should require TOTP by default and make a configuration option to disable this requirement.

@IreneKnapp
Copy link

@IreneKnapp IreneKnapp commented Jun 1, 2020

+1 to @jonasfranz's suggestion. From a security perspective the best practice on the user's side is to have two or more U2F devices, so that one can serve as a backup if the other is lost.

@rugk
Copy link
Contributor

@rugk rugk commented Jun 1, 2020

Then we would gain nothing, if you want a config, do the reverse: enable a config that requires TOTP before U2F.
No other website does require it like this.

We want U2F without TOTP. By default.

@kdomanski
Copy link
Contributor Author

@kdomanski kdomanski commented Jun 1, 2020

Added a warning box that only appears when there's no TOTP enrollment.

@lafriks
Copy link
Member

@lafriks lafriks commented Jun 9, 2020

@rugk imho github also allow u2f only if you have topt enrolled

@IreneKnapp
Copy link

@IreneKnapp IreneKnapp commented Jun 9, 2020

Google also allows TOTP to be disabled. In fact, Google offers a mode for its high-risk users in which an account cannot be configured with TOTP, SMS, or other alternate forms of authentication. (Turning this mode off incurs a mandatory delay of several weeks, and may incur a requirement to present legal identification.)

@lafriks lafriks added this to the 1.13.0 milestone Jun 9, 2020
@kdomanski
Copy link
Contributor Author

@kdomanski kdomanski commented Jun 25, 2020

@CirnoT @6543 Could you please review this?

Recording of the change in action:
Recording of the change in action

options/locale/locale_en-US.ini Outdated Show resolved Hide resolved
@CirnoT
Copy link
Contributor

@CirnoT CirnoT commented Jun 27, 2020

Aside from TOTP warning comment above I would also add additional warning whenever user has less than 2 U2F keys (second key as backup in case first is lost or breaks).

@lunny lunny added this to the 1.15.0 milestone Jan 27, 2021
@6543
Copy link
Member

@6543 6543 commented Jan 27, 2021

since now a admin can reset 2FA it's less concerning that user can loose access to it's account

@kdomanski kdomanski force-pushed the independent-u2f branch 2 times, most recently from 3399a9c to 94cf08c Feb 7, 2021
@kdomanski kdomanski force-pushed the independent-u2f branch from 94cf08c to ef166ae Feb 7, 2021
routers/user/auth.go Outdated Show resolved Hide resolved
templates/user/settings/security_u2f.tmpl Show resolved Hide resolved
@techknowlogick techknowlogick removed this from the 1.15.0 milestone Jun 15, 2021
@techknowlogick techknowlogick added this to the 1.16.0 milestone Jun 15, 2021
@kdomanski kdomanski closed this Aug 22, 2021
@rugk
Copy link
Contributor

@rugk rugk commented Aug 22, 2021

Why was this closed?

@go-gitea go-gitea locked and limited conversation to collaborators Oct 19, 2021
Copy link
Contributor

@zeripath zeripath left a comment

Apologies for the late review. I've updated this to head and I think this is now ready

lafriks
lafriks approved these changes Nov 8, 2021
@6543
Copy link
Member

@6543 6543 commented Nov 8, 2021

🚀

@6543 6543 merged commit 021df29 into go-gitea:main Nov 8, 2021
2 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.