From 0801518f5dd6d05a20b6997ba8a259066a8342e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emmanuel=20BENO=C3=8ET?= Date: Sun, 19 May 2024 10:46:15 +0000 Subject: [PATCH] fix(actions): prevent deleted records' UUID from colliding with new records (#3830) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit changes the code that deletes a runner so it updates the UUID before deleting the record. The new UUID is set to 8 0xff bytes followed by a little endian version of the record's numeric ID. Such UUIDs cannot be created from tokens when registering runners, as the first 16 bytes of the token are in the `[0-9a-f]` range. This should prevent deleted runners from colliding with new records if the tokens share the same first 16 characters. It is a possible solution to issue #3828 Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/3830 Reviewed-by: Earl Warren Co-authored-by: Emmanuel BENOÎT Co-committed-by: Emmanuel BENOÎT --- models/actions/main_test.go | 1 + models/actions/runner.go | 20 +++++++++-- models/actions/runner_test.go | 59 +++++++++++++++++++++++++++++++ models/fixtures/action_runner.yml | 20 +++++++++++ release-notes/8.0.0/3830.md | 1 + 5 files changed, 99 insertions(+), 2 deletions(-) create mode 100644 models/actions/runner_test.go create mode 100644 models/fixtures/action_runner.yml create mode 100644 release-notes/8.0.0/3830.md diff --git a/models/actions/main_test.go b/models/actions/main_test.go index 5d5089e3bb..3cfb395e62 100644 --- a/models/actions/main_test.go +++ b/models/actions/main_test.go @@ -12,6 +12,7 @@ import ( func TestMain(m *testing.M) { unittest.MainTest(m, &unittest.TestOptions{ FixtureFiles: []string{ + "action_runner.yml", "action_runner_token.yml", }, }) diff --git a/models/actions/runner.go b/models/actions/runner.go index 9192925d5a..cfe936c495 100644 --- a/models/actions/runner.go +++ b/models/actions/runner.go @@ -5,6 +5,7 @@ package actions import ( "context" + "encoding/binary" "fmt" "strings" "time" @@ -253,11 +254,26 @@ func UpdateRunner(ctx context.Context, r *ActionRunner, cols ...string) error { // DeleteRunner deletes a runner by given ID. func DeleteRunner(ctx context.Context, id int64) error { - if _, err := GetRunnerByID(ctx, id); err != nil { + runner, err := GetRunnerByID(ctx, id) + if err != nil { return err } - _, err := db.DeleteByID[ActionRunner](ctx, id) + // Replace the UUID, which was either based on the secret's first 16 bytes or an UUIDv4, + // with a sequence of 8 0xff bytes followed by the little-endian version of the record's + // identifier. This will prevent the deleted record's identifier from colliding with any + // new record. + b := make([]byte, 8) + binary.LittleEndian.PutUint64(b, uint64(id)) + runner.UUID = fmt.Sprintf("ffffffff-ffff-ffff-%.2x%.2x-%.2x%.2x%.2x%.2x%.2x%.2x", + b[0], b[1], b[2], b[3], b[4], b[5], b[6], b[7]) + + err = UpdateRunner(ctx, runner, "UUID") + if err != nil { + return err + } + + _, err = db.DeleteByID[ActionRunner](ctx, id) return err } diff --git a/models/actions/runner_test.go b/models/actions/runner_test.go new file mode 100644 index 0000000000..a71f5f0044 --- /dev/null +++ b/models/actions/runner_test.go @@ -0,0 +1,59 @@ +// SPDX-License-Identifier: MIT + +package actions + +import ( + "encoding/binary" + "fmt" + "testing" + + "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/models/unittest" + + "github.com/stretchr/testify/assert" +) + +func TestDeleteRunner(t *testing.T) { + const recordID = 12345678 + assert.NoError(t, unittest.PrepareTestDatabase()) + before := unittest.AssertExistsAndLoadBean(t, &ActionRunner{ID: recordID}) + + err := DeleteRunner(db.DefaultContext, recordID) + assert.NoError(t, err) + + var after ActionRunner + found, err := db.GetEngine(db.DefaultContext).ID(recordID).Unscoped().Get(&after) + assert.NoError(t, err) + assert.True(t, found) + + // Most fields (namely Name, Version, OwnerID, RepoID, Description, Base, RepoRange, + // TokenHash, TokenSalt, LastOnline, LastActive, AgentLabels and Created) are unaffected + assert.Equal(t, before.Name, after.Name) + assert.Equal(t, before.Version, after.Version) + assert.Equal(t, before.OwnerID, after.OwnerID) + assert.Equal(t, before.RepoID, after.RepoID) + assert.Equal(t, before.Description, after.Description) + assert.Equal(t, before.Base, after.Base) + assert.Equal(t, before.RepoRange, after.RepoRange) + assert.Equal(t, before.TokenHash, after.TokenHash) + assert.Equal(t, before.TokenSalt, after.TokenSalt) + assert.Equal(t, before.LastOnline, after.LastOnline) + assert.Equal(t, before.LastActive, after.LastActive) + assert.Equal(t, before.AgentLabels, after.AgentLabels) + assert.Equal(t, before.Created, after.Created) + + // Deleted contains a value + assert.NotNil(t, after.Deleted) + + // UUID was modified + assert.NotEqual(t, before.UUID, after.UUID) + // UUID starts with ffffffff-ffff-ffff- + assert.Equal(t, "ffffffff-ffff-ffff-", after.UUID[:19]) + // UUID ends with LE binary representation of record ID + idAsBinary := make([]byte, 8) + binary.LittleEndian.PutUint64(idAsBinary, uint64(recordID)) + idAsHexadecimal := fmt.Sprintf("%.2x%.2x-%.2x%.2x%.2x%.2x%.2x%.2x", idAsBinary[0], + idAsBinary[1], idAsBinary[2], idAsBinary[3], idAsBinary[4], idAsBinary[5], + idAsBinary[6], idAsBinary[7]) + assert.Equal(t, idAsHexadecimal, after.UUID[19:]) +} diff --git a/models/fixtures/action_runner.yml b/models/fixtures/action_runner.yml new file mode 100644 index 0000000000..d2615f08eb --- /dev/null +++ b/models/fixtures/action_runner.yml @@ -0,0 +1,20 @@ +- + # A global runner + # Secret is 7e577e577e577e57feedfacefeedfacefeedface + id: 12345678 + uuid: "37653537-3765-3537-3765-353737653537" + name: "test" + version: "" + owner_id: 0 + repo_id: 0 + description: "" + base: 0 + repo_range: "" + token_hash: "3af8a56b850dba8848044385fedcfa4d9432e17de9f9803e4d279991394ac2945066ceb9a5e7cbe60a087d90d4bad03a8f9b" + token_salt: "832f8529db6151a1c3c605dd7570b58f" + last_online: 0 + last_active: 0 + agent_labels: '[""]' + created: 1716104432 + updated: 1716104432 + deleted: ~ diff --git a/release-notes/8.0.0/3830.md b/release-notes/8.0.0/3830.md new file mode 100644 index 0000000000..5e46a45ec9 --- /dev/null +++ b/release-notes/8.0.0/3830.md @@ -0,0 +1 @@ +Neutralize delete runners' UUID to prevent collisions with new records