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

Unify password changing and invalidate auth tokens #27625

Merged
merged 9 commits into from
Feb 4, 2024

Conversation

KN4CK3R
Copy link
Member

@KN4CK3R KN4CK3R commented Oct 14, 2023

  • Unify the password changing code
  • Invalidate existing auth tokens when changing passwords

@KN4CK3R KN4CK3R added the type/enhancement An improvement of existing functionality label Oct 14, 2023
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 14, 2023
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 14, 2023
@github-actions github-actions bot added 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 labels Oct 14, 2023
models/auth/auth_token.go Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 14, 2023
@KN4CK3R
Copy link
Member Author

KN4CK3R commented Oct 14, 2023

Have to fight an import cycle in the auth service first...

@KN4CK3R KN4CK3R marked this pull request as draft October 14, 2023 15:47
@delvh
Copy link
Member

delvh commented Oct 14, 2023

There's an import cycle?

@KN4CK3R
Copy link
Member Author

KN4CK3R commented Oct 14, 2023 via email

@KN4CK3R KN4CK3R marked this pull request as ready for review October 15, 2023 09:12
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 15, 2023
Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

Wait a sec... Deleting the auth tokens when changing passwords may be a major gotcha.

Is this normal behaviour for other services?

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. labels Oct 15, 2023
@KN4CK3R
Copy link
Member Author

KN4CK3R commented Oct 15, 2023

It's recommended by the original authors (https://paragonie.com/blog/2015/04/secure-authentication-php-with-long-term-persistence#secure-remember-me-cookies)

Important: If the user should ever change their password, you should invalidate all existing long-term authentication tokens for that user.

I don't know if others do that but I see the security benefit. A password change requests a re-login on other devices.
See the comments to this answer for example: https://security.stackexchange.com/a/112857

routers/web/admin/users.go Outdated Show resolved Hide resolved
@zeripath
Copy link
Contributor

It's recommended by the original authors (https://paragonie.com/blog/2015/04/secure-authentication-php-with-long-term-persistence#secure-remember-me-cookies)

Important: If the user should ever change their password, you should invalidate all existing long-term authentication tokens for that user.

I don't know if others do that but I see the security benefit. A password change requests a re-login on other devices. See the comments to this answer for example: https://security.stackexchange.com/a/112857

I agree that that it might make sense but I think we need to update the UI to make this clear to the user.

Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

I also think it is right to remove auth tokens when changing password, to make users safe.

@wxiaoguang wxiaoguang dismissed zeripath’s stale review February 3, 2024 02:38

absent for long time, feel free to provide more ideas if you have time.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Feb 3, 2024
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 3, 2024
@KN4CK3R KN4CK3R added the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Feb 3, 2024
@delvh
Copy link
Member

delvh commented Feb 3, 2024

@KN4CK3R why blocked?

@KN4CK3R
Copy link
Member Author

KN4CK3R commented Feb 3, 2024

Want to wait for #27625 (comment)

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Feb 4, 2024

One more question: if the current login uses an auth token, after deleting all auth tokens, should the current login gets a new auth token ?

(just a question, regenerating it or not, either looks good to me, and keeping things simple is also good)

@KN4CK3R
Copy link
Member Author

KN4CK3R commented Feb 4, 2024

Good question. Currently when you change your password in the account settings, you stay logged in because your session still exists but the "remember me" cookie is removed. So when your session times out, you have to login again. In that situation it may be valid to generate a new "remember me" session if it was present before changing the password.

I will have a look if there are some best practices out there.

@KN4CK3R
Copy link
Member Author

KN4CK3R commented Feb 4, 2024

I could not really find any info how to handle a password change combined with what to do with remember me infos for the local remember me session. It's consens to destroy all other sessions to lock out a possible attacker. This PR does that to the remember me tokens but I think we can't destroy other (middleware) sessions? There is no consens what should be done with the local (normal) session. Some say the user should re-login, others say otherwise.

OWASP tells to regenerate the local session after a password change but I have enough knowledge if we have a potential problem here. https://cheatsheetseries.owasp.org/cheatsheets/Session_Management_Cheat_Sheet.html#renew-the-session-id-after-any-privilege-level-change

So at the moment, I think it's ok to destroy the local remember me session and require a login after the normal session timed out. It's no big deal to change this behaviour later.

@KN4CK3R KN4CK3R added reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. and removed status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR labels Feb 4, 2024
@GiteaBot
Copy link
Contributor

GiteaBot commented Feb 4, 2024

@KN4CK3R please fix the merge conflicts. 🍵

@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 4, 2024
@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 4, 2024
@github-actions github-actions bot removed 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 labels Feb 4, 2024
@KN4CK3R
Copy link
Member Author

KN4CK3R commented Feb 4, 2024

First positive effect of #28733: Changed the changed lines in this PR from +44 -42 to +20 -1 🎉

@KN4CK3R KN4CK3R added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 4, 2024
@lunny lunny enabled auto-merge (squash) February 4, 2024 13:46
@lunny lunny merged commit 688d4a1 into go-gitea:main Feb 4, 2024
25 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 4, 2024
@KN4CK3R KN4CK3R deleted the enhancement-auth-token branch February 4, 2024 14:07
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 5, 2024
* giteaofficial/main:
  [skip ci] Updated licenses and gitignores
  Show whether a PR is WIP inside popups  (go-gitea#28975)
  Unify password changing and invalidate auth tokens (go-gitea#27625)
  Unify user update methods (go-gitea#28733)
  Do not render empty comments (go-gitea#29039)
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
- Unify the password changing code
- Invalidate existing auth tokens when changing passwords
6543 pushed a commit to 6543-forks/gitea that referenced this pull request Feb 26, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants