From 3beefb29b95ea60728a8cdcca8744b4cb4378b53 Mon Sep 17 00:00:00 2001 From: Andreas Shimokawa Date: Thu, 5 Dec 2024 21:32:09 +0000 Subject: [PATCH] Do not rewrite ssh keys files when deleting a user without one (#6097) ### Problem Big instances can have huge authorized_keys files when using OpenSSH instead of the internal ssh server. Forgejo always re-generates the contents of that file when a user is deleted, even if he does not even have a public key uploaded. In case of codeberg.org, a 15MB file gets rewritten. If we batch delete 100 Spam users without ssh keys, we rewrite 1.5GB, this takes time and wears the SSD. In addition, there is a high chance of hitting a race contidion bug, when deleting users in parallel. ### Solution / Mitigation This patch prevents rewriting authorized_keys files, when not necessary. It greatly speeds up deleting malicious users, saves IO bandwidth and SSD wear. It also greatly reduces the chance of hitting a race condition bug. Fixing the race condition is not the scope of this patch though. ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests - I added test coverage for Go changes... - [x] in their respective `*_test.go` for unit tests. - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I added test coverage for JavaScript changes... - [ ] in `web_src/js/*.test.js` if it can be unit tested. - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)). ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [x] I do not want this change to show in the release notes. - [ ] I want the title to show in the release notes with a link to this pull request. - [ ] I want the content of the `release-notes/.md` to be be used for the release notes instead of the title. Co-authored-by: Gusted Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/6097 Reviewed-by: Gusted Reviewed-by: Earl Warren Co-authored-by: Andreas Shimokawa Co-committed-by: Andreas Shimokawa (cherry picked from commit 3c9b3ddf5c727d5d79bfb790454791551ffdd63a) --- services/user/TestPurgeUser/public_key.yml | 11 +++++ services/user/user.go | 23 +++++++-- services/user/user_test.go | 57 +++++++++++++++++++--- 3 files changed, 81 insertions(+), 10 deletions(-) create mode 100644 services/user/TestPurgeUser/public_key.yml diff --git a/services/user/TestPurgeUser/public_key.yml b/services/user/TestPurgeUser/public_key.yml new file mode 100644 index 0000000000..75e409aa44 --- /dev/null +++ b/services/user/TestPurgeUser/public_key.yml @@ -0,0 +1,11 @@ +- + id: 1001 + owner_id: 2 + name: user2@localhost + fingerprint: "SHA256:7s+isLFauDv7QSbhAd0Z4OGIYJlQQ4YMtOH9LdjCZL8" + content: "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIHAv3EOUcaK918Fk9d7mWuVS7oQamif/PNwqnAf/Z34G user2@localhost" + mode: 2 + type: 3 + created_unix: 1733363453 + updated_unix: 1733363453 + login_source_id: 0 diff --git a/services/user/user.go b/services/user/user.go index 653f5a9cd2..abaeb88f40 100644 --- a/services/user/user.go +++ b/services/user/user.go @@ -131,6 +131,16 @@ func DeleteUser(ctx context.Context, u *user_model.User, purge bool) error { return models.ErrDeleteLastAdminUser{UID: u.ID} } + hasSSHKey, err := db.GetEngine(ctx).Where("owner_id = ? AND type != ?", u.ID, asymkey_model.KeyTypePrincipal).Table("public_key").Exist() + if err != nil { + return err + } + + hasPrincipialSSHKey, err := db.GetEngine(ctx).Where("owner_id = ? AND type = ?", u.ID, asymkey_model.KeyTypePrincipal).Table("public_key").Exist() + if err != nil { + return err + } + if purge { // Disable the user first // NOTE: This is deliberately not within a transaction as it must disable the user immediately to prevent any further action by the user to be purged. @@ -260,11 +270,16 @@ func DeleteUser(ctx context.Context, u *user_model.User, purge bool) error { } committer.Close() - if err = asymkey_model.RewriteAllPublicKeys(ctx); err != nil { - return err + if hasSSHKey { + if err = asymkey_model.RewriteAllPublicKeys(ctx); err != nil { + return err + } } - if err = asymkey_model.RewriteAllPrincipalKeys(ctx); err != nil { - return err + + if hasPrincipialSSHKey { + if err = asymkey_model.RewriteAllPrincipalKeys(ctx); err != nil { + return err + } } // Note: There are something just cannot be roll back, diff --git a/services/user/user_test.go b/services/user/user_test.go index 45bf1e6993..ad5387c0bc 100644 --- a/services/user/user_test.go +++ b/services/user/user_test.go @@ -5,11 +5,14 @@ package user import ( "fmt" + "os" + "path/filepath" "strings" "testing" "time" "code.gitea.io/gitea/models" + asymkey_model "code.gitea.io/gitea/models/asymkey" "code.gitea.io/gitea/models/auth" "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/organization" @@ -17,6 +20,7 @@ import ( "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/test" "code.gitea.io/gitea/modules/timeutil" "github.com/stretchr/testify/assert" @@ -63,20 +67,61 @@ func TestDeleteUser(t *testing.T) { } func TestPurgeUser(t *testing.T) { - test := func(userID int64) { + defer unittest.OverrideFixtures( + unittest.FixturesOptions{ + Dir: filepath.Join(setting.AppWorkPath, "models/fixtures/"), + Base: setting.AppWorkPath, + Dirs: []string{"services/user/TestPurgeUser/"}, + }, + )() + require.NoError(t, unittest.PrepareTestDatabase()) + defer test.MockVariableValue(&setting.SSH.RootPath, t.TempDir())() + defer test.MockVariableValue(&setting.SSH.CreateAuthorizedKeysFile, true)() + defer test.MockVariableValue(&setting.SSH.CreateAuthorizedPrincipalsFile, true)() + defer test.MockVariableValue(&setting.SSH.StartBuiltinServer, false)() + require.NoError(t, asymkey_model.RewriteAllPublicKeys(db.DefaultContext)) + require.NoError(t, asymkey_model.RewriteAllPrincipalKeys(db.DefaultContext)) + + test := func(userID int64, modifySSHKey bool) { require.NoError(t, unittest.PrepareTestDatabase()) user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: userID}) - err := DeleteUser(db.DefaultContext, user, true) + fAuthorizedKeys, err := os.Open(filepath.Join(setting.SSH.RootPath, "authorized_keys")) require.NoError(t, err) + authorizedKeysStatBefore, err := fAuthorizedKeys.Stat() + require.NoError(t, err) + fAuthorizedPrincipals, err := os.Open(filepath.Join(setting.SSH.RootPath, "authorized_principals")) + require.NoError(t, err) + authorizedPrincipalsBefore, err := fAuthorizedPrincipals.Stat() + require.NoError(t, err) + + require.NoError(t, DeleteUser(db.DefaultContext, user, true)) unittest.AssertNotExistsBean(t, &user_model.User{ID: userID}) unittest.CheckConsistencyFor(t, &user_model.User{}, &repo_model.Repository{}) + + fAuthorizedKeys, err = os.Open(filepath.Join(setting.SSH.RootPath, "authorized_keys")) + require.NoError(t, err) + fAuthorizedPrincipals, err = os.Open(filepath.Join(setting.SSH.RootPath, "authorized_principals")) + require.NoError(t, err) + + authorizedKeysStatAfter, err := fAuthorizedKeys.Stat() + require.NoError(t, err) + authorizedPrincipalsAfter, err := fAuthorizedPrincipals.Stat() + require.NoError(t, err) + + if modifySSHKey { + assert.Greater(t, authorizedKeysStatAfter.ModTime(), authorizedKeysStatBefore.ModTime()) + assert.Greater(t, authorizedPrincipalsAfter.ModTime(), authorizedPrincipalsBefore.ModTime()) + } else { + assert.Equal(t, authorizedKeysStatAfter.ModTime(), authorizedKeysStatBefore.ModTime()) + assert.Equal(t, authorizedPrincipalsAfter.ModTime(), authorizedPrincipalsBefore.ModTime()) + } } - test(2) - test(4) - test(8) - test(11) + test(2, true) + test(4, false) + test(8, false) + test(11, false) org := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3}) require.Error(t, DeleteUser(db.DefaultContext, org, false))