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

Encrypt OAuth2 Client Secret in db with SecretKey #30482

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ASiebens
Copy link

The OAuth2 Client Secret is currently stored in plaintext in the db.

This PR encrypts them with the setting.SecretKey.

(similar in changes to #15547)

⚠️ WARNING ⚠️

Changing the SECRET_KEY in the app.ini will now cause these passwords to become unreadable.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 14, 2024
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 14, 2024
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Apr 14, 2024
@lunny lunny added this to the 1.23.0 milestone Apr 15, 2024
@lunny lunny added the backport/v1.22 This PR should be backported to Gitea 1.22 label Apr 15, 2024
@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 Apr 15, 2024
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 16, 2024

I would suggest to have a complete secret rotation solution before merge this one. There are too many legacy problem for the secret system, continue using it would cause more problems.

A complete secret rotation solution could be like this: each secret key has a version, EncryptSecret should return the version with encrypted content.

@wxiaoguang wxiaoguang removed this from the 1.23.0 milestone Apr 16, 2024
@wxiaoguang wxiaoguang removed the backport/v1.22 This PR should be backported to Gitea 1.22 label Apr 16, 2024
@lunny
Copy link
Member

lunny commented Apr 16, 2024

But this could be quick fix which will not affect the whole design.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 16, 2024

But is the approach stable enough? For example, if secret.DecryptSecret returns error (by missing secret), then users will always see 500 error, because of the returned err? How to recover?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. modifies/go Pull requests that update Go code size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants