Encrypt migration credentials at rest #15895
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Not sure this is the best approach, we could encrypt the entire `PayloadContent` instead. Also instead of clearing individual fields in payload content, we could just delete the task once it has (successfully) finished..?
noerw
added
the
topic/security
zeripath
reviewed
May 16, 2021
GiteaBot
added
lgtm/need 2
Codecov Report
@@ Coverage Diff @@
## main #15895 +/- ##
=======================================
Coverage 44.08% 44.08%
=======================================
Files 682 682
Lines 82384 82420 +36
=======================================
+ Hits 36316 36334 +18
- Misses 40157 40170 +13
- Partials 5911 5916 +5
Continue to review full report at Codecov.
|
|
You could add a doctor task and back port the doctor sub command. |
zeripath
approved these changes
May 18, 2021
GiteaBot
added
lgtm/need 1
lunny
reviewed
May 20, 2021
noerw
added
the
topic/repo-migration
lunny
reviewed
May 24, 2021
lunny
approved these changes
May 24, 2021
GiteaBot
added
lgtm/done
zeripath
added a commit
to zeripath/gitea
that referenced
this pull request
Jun 17, 2021
Backport go-gitea#15895 Storing these credentials is a liability. * Encrypt credentials with SECRET_KEY before persisting to task queue table (they need to be persisted due to the nature of the task queue) - security in depth: helps when attacker has access to DB only, but not app.ini * Delete all credentials (even encrypted) from the task table, once the migration is done, for safety - security in depth: minimizes leaked data if attacker gains access to snapshot of both DB and app.ini A Doctor task needs to be created to delete finished tasks and encrypt current tasks.
6543
pushed a commit
that referenced
this pull request
Jun 17, 2021
Backport #15895 Storing these credentials is a liability. * Encrypt credentials with SECRET_KEY before persisting to task queue table (they need to be persisted due to the nature of the task queue) - security in depth: helps when attacker has access to DB only, but not app.ini * Delete all credentials (even encrypted) from the task table, once the migration is done, for safety - security in depth: minimizes leaked data if attacker gains access to snapshot of both DB and app.ini
AbdulrhmnGhanem
pushed a commit
to kitspace/gitea
that referenced
this pull request
Aug 10, 2021
* encrypt migration credentials in task persistence Not sure this is the best approach, we could encrypt the entire `PayloadContent` instead. Also instead of clearing individual fields in payload content, we could just delete the task once it has (successfully) finished..? * remove credentials of past migrations * only run DB migration for completed tasks * fix binding * add omitempty * never serialize unencrypted credentials * fix import order Co-authored-by: techknowlogick <techknowlogick@gitea.io> Co-authored-by: zeripath <art27@cantab.net> Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Storing these credentials is a liability (or plain stupid)
app.initasktable, once the migration is done, for safetyWill backport this to 1.14, implementing the migration as a doctor task.