-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Allow U2F 2FA without TOTP #11573
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
IMHO We should enforce totp since u2f is currently only supported in a few browsers. |
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. |
@jonasfranz It's not possible to set up U2F in a non-supporting browser in the first place. |
@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 |
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 |
Hmm, I didn't think about the API. Back to the drawing board. |
@kdomanski I think its fine we just have to inform the user ... |
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. 😃 |
Updated tests which were setting U2F on root user without enabling TOTP. |
@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. |
I would ad no hit to api 403 responce and yes we should add add this to swagger docs #11667 (a other pull) |
but when adding a U2F key without an existing TOTP a hint on the UI could warn about API and account recovery etc ... |
@kdomanski Many users are using multiple devices. I think we should require TOTP by default and make a configuration option to disable this requirement. |
+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. |
Then we would gain nothing, if you want a config, do the reverse: enable a config that requires TOTP before U2F. We want U2F without TOTP. By default. |
Added a warning box that only appears when there's no TOTP enrollment. |
@rugk imho github also allow u2f only if you have topt enrolled |
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.) |
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). |
templates/base/footer.tmpl
Outdated
@@ -26,9 +26,7 @@ | |||
{{if .RequireMinicolors}} | |||
<script src="{{StaticUrlPrefix}}/vendor/plugins/jquery.minicolors/jquery.minicolors.min.js"></script> | |||
{{end}} | |||
{{if .RequireU2F}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this removal here?
ref #11585
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Until #11585 is merged, this line completely prevents U2F from operating if the user doesn't have TOTP enrollments.
templates/base/head.tmpl
Outdated
@@ -69,7 +69,6 @@ | |||
Minicolors: {{if .RequireMinicolors}}true{{else}}false{{end}}, | |||
SimpleMDE: {{if .RequireSimpleMDE}}true{{else}}false{{end}}, | |||
Tribute: {{if .RequireTribute}}true{{else}}false{{end}}, | |||
U2F: {{if .RequireU2F}}true{{else}}false{{end}}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary, handled by #11585 and will cause conflict for zero benefit
since now a admin can reset 2FA it's less concerning that user can loose access to it's account |
Why was this closed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the late review. I've updated this to head and I think this is now ready
Signed-off-by: Andrew Thornton <[email protected]>
🚀 |
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