fix(actions): prevent deleted records' UUID from colliding with new records (#3830)

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 <earl-warren@noreply.codeberg.org>
Co-authored-by: Emmanuel BENOÎT <tseeker@nocternity.net>
Co-committed-by: Emmanuel BENOÎT <tseeker@nocternity.net>
This commit is contained in:
Emmanuel BENOÎT 2024-05-19 10:46:15 +00:00 committed by Earl Warren
parent e4c3c039be
commit 0801518f5d
5 changed files with 99 additions and 2 deletions

View file

@ -12,6 +12,7 @@ import (
func TestMain(m *testing.M) { func TestMain(m *testing.M) {
unittest.MainTest(m, &unittest.TestOptions{ unittest.MainTest(m, &unittest.TestOptions{
FixtureFiles: []string{ FixtureFiles: []string{
"action_runner.yml",
"action_runner_token.yml", "action_runner_token.yml",
}, },
}) })

View file

@ -5,6 +5,7 @@ package actions
import ( import (
"context" "context"
"encoding/binary"
"fmt" "fmt"
"strings" "strings"
"time" "time"
@ -253,11 +254,26 @@ func UpdateRunner(ctx context.Context, r *ActionRunner, cols ...string) error {
// DeleteRunner deletes a runner by given ID. // DeleteRunner deletes a runner by given ID.
func DeleteRunner(ctx context.Context, id int64) error { 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 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 return err
} }

View file

@ -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:])
}

View file

@ -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: ~

View file

@ -0,0 +1 @@
Neutralize delete runners' UUID to prevent collisions with new records