Skip to content

Enforce two-factor auth (2FA: TOTP or WebAuthn) #34187

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Enforce two-factor auth (2FA: TOTP or WebAuthn) #34187

wants to merge 7 commits into from

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Apr 12, 2025 •
edited
Loading

Fix #880

Design:

  1. A global setting security.TWO_FACTOR_AUTH.
    • To support org-level config, we need to introduce a better "owner setting" system first (in the future)
  2. A user without 2FA can login and may explore, but can NOT read or write to any repositories via API/web.
  3. Keep things as simple as possible.
    • This option only aggressively suggest users to enable their 2FA at the moment, it does NOT guarantee that users must have 2FA before all other operations, it should be good enough for real world use cases.
    • Some details and tests could be improved in the future since this change only adds a check and seems won't affect too much.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 12, 2025
@github-actions github-actions bot added modifies/translation modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin modifies/templates This PR modifies the template files modifies/migrations docs-update-needed The document needs to be updated synchronously labels Apr 12, 2025
@wxiaoguang wxiaoguang mentioned this pull request Apr 12, 2025
Open
@wxiaoguang wxiaoguang added this to the 1.24.0 milestone Apr 12, 2025
@wxiaoguang wxiaoguang added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Apr 12, 2025
@wxiaoguang
Copy link
Contributor Author

image

image

@wxiaoguang wxiaoguang mentioned this pull request Apr 12, 2025
Closed
@wxiaoguang wxiaoguang mentioned this pull request Apr 12, 2025
Closed
@wxiaoguang wxiaoguang force-pushed the enforce-2fa-2 branch 2 times, most recently from 3c81bf5 to f05976c Compare April 12, 2025 09:02
@wxiaoguang wxiaoguang mentioned this pull request Jul 18, 2023
Open
@wxiaoguang wxiaoguang requested a review from lunny April 14, 2025 10:11
lunny
lunny reviewed Apr 19, 2025
View reviewed changes
modules/setting/security.go
twoFactorAuth := sec.Key("TWO_FACTOR_AUTH").String()
switch twoFactorAuth {
case "":
case "enforced":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enforced sounds like user cannot visit anything except verify 2FA but now the user can visit basic pages. Maybe user another name to keep consistent? And in the future, it may supports more modes like read-only, only-important-operation and etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the enforced name is good enough. The major purpose of Gitea users is to access repositories.

read-only and only-important-operation are too wordy and more unclear.

This config option is "string enum" based, so there are always chances to choose better names in the future without breaking.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lunny

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lunny

Copy link
Member

@lunny lunny Apr 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this configuration could be extend in the future so that the name should match it's behaviour. The current behaviour will display dashboard, explores but all repositories related operations will require two factor verify. So that the name enforced is unclear. Maybe some name like repository-operation is better. The enforced sounds like even login will require two factor veirfy.

only-important-operation means a default status like what Github did, the two factor verify will be prompted only when change password or change some important informations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You decide, feel free to edit this PR directly

lunny
lunny reviewed Apr 25, 2025
View reviewed changes
models/migrations/v1_24/v320.go
type LoginSource struct {
TwoFactorPolicy string `xorm:"two_factor_policy NOT NULL DEFAULT ''"`
}
err := x.Sync(new(LoginSource))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use SyncWithOptions to avoid index sync.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the details. Feel free to edit directly and document it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lunny lunny lunny left review comments

Assignees
No one assigned
Labels
docs-update-needed The document needs to be updated synchronously lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/api This PR adds API routes or modifies them modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin modifies/go Pull requests that update Go code modifies/migrations modifies/templates This PR modifies the template files modifies/translation type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Milestone
1.24.0
Development

Successfully merging this pull request may close these issues.

Ability to enforce two-factor authentication
3 participants
@wxiaoguang @lunny @GiteaBot