From 320ab7ed7fd77d49d709cdb4a5cb6be7751ceeef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emmanuel=20BENO=C3=8ET?= Date: Mon, 22 Jul 2024 11:55:43 +0200 Subject: [PATCH] feat(cli): allow updates to runners' secrets This commit allows the `forgejo-cli actions register` command to change an existing runner's secret, as discussed in #4610. It refactors `RegisterRunner` to extract the code that hashes the token, moving this code to a method called `UpdateSecret` on `ActionRunner`. A test for the method has been added. The `RegisterRunner` function is updated so that: - it relies on `ActionRunner.UpdateSecret` when creating new runners, - it checks whether an existing runner's secret still matches the one passed on the command line, - it updates the runner's secret if it wasn't created and it no longer matches. A test has been added for the new behaviour. --- models/actions/forgejo.go | 35 ++++++++++++++++++++++------------ models/actions/forgejo_test.go | 31 ++++++++++++++++++++++++++++++ models/actions/runner.go | 18 +++++++++++++++++ models/actions/runner_test.go | 16 ++++++++++++++++ 4 files changed, 88 insertions(+), 12 deletions(-) diff --git a/models/actions/forgejo.go b/models/actions/forgejo.go index 29e8588e2..5ea77f447 100644 --- a/models/actions/forgejo.go +++ b/models/actions/forgejo.go @@ -4,7 +4,7 @@ package actions import ( "context" - "encoding/hex" + "crypto/subtle" "fmt" auth_model "code.gitea.io/gitea/models/auth" @@ -26,25 +26,30 @@ func RegisterRunner(ctx context.Context, ownerID, repoID int64, token string, la has, err := db.GetEngine(ctx).Where("uuid=?", uuidString).Get(&runner) if err != nil { return nil, fmt.Errorf("GetRunner %v", err) - } else if !has { + } + + var mustUpdateSecret bool + if has { + // + // The runner exists, check if the rest of the token has changed. + // + mustUpdateSecret = subtle.ConstantTimeCompare( + []byte(runner.TokenHash), + []byte(auth_model.HashToken(token, runner.TokenSalt)), + ) != 1 + } else { // // The runner does not exist yet, create it // - saltBytes, err := util.CryptoRandomBytes(16) - if err != nil { - return nil, fmt.Errorf("CryptoRandomBytes %v", err) - } - salt := hex.EncodeToString(saltBytes) - - hash := auth_model.HashToken(token, salt) - runner = ActionRunner{ UUID: uuidString, - TokenHash: hash, - TokenSalt: salt, AgentLabels: []string{}, } + if err := runner.UpdateSecret(token); err != nil { + return &runner, fmt.Errorf("can't set new runner's secret: %w", err) + } + if err := CreateRunner(ctx, &runner); err != nil { return &runner, fmt.Errorf("can't create new runner %w", err) } @@ -64,6 +69,12 @@ func RegisterRunner(ctx context.Context, ownerID, repoID int64, token string, la runner.AgentLabels = *labels cols = append(cols, "agent_labels") } + if mustUpdateSecret { + if err := runner.UpdateSecret(token); err != nil { + return &runner, fmt.Errorf("can't change runner's secret: %w", err) + } + cols = append(cols, "token_hash", "token_salt") + } if err := UpdateRunner(ctx, &runner, cols...); err != nil { return &runner, fmt.Errorf("can't update the runner %+v %w", runner, err) diff --git a/models/actions/forgejo_test.go b/models/actions/forgejo_test.go index 1bc81e713..8d4145b53 100644 --- a/models/actions/forgejo_test.go +++ b/models/actions/forgejo_test.go @@ -11,6 +11,7 @@ import ( "code.gitea.io/gitea/models/unittest" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestActions_RegisterRunner_Token(t *testing.T) { @@ -28,6 +29,36 @@ func TestActions_RegisterRunner_Token(t *testing.T) { assert.EqualValues(t, 1, subtle.ConstantTimeCompare([]byte(runner.TokenHash), []byte(auth_model.HashToken(token, runner.TokenSalt))), "the token cannot be verified with the same method as routers/api/actions/runner/interceptor.go as of 8228751c55d6a4263f0fec2932ca16181c09c97d") } +// TestActions_RegisterRunner_TokenUpdate tests that a token's secret is updated +// when a runner already exists and RegisterRunner is called with a token +// parameter whose first 16 bytes match that record but where the last 24 bytes +// do not match. +func TestActions_RegisterRunner_TokenUpdate(t *testing.T) { + const recordID = 12345678 + oldToken := "7e577e577e577e57feedfacefeedfacefeedface" + newToken := "7e577e577e577e57deadbeefdeadbeefdeadbeef" + assert.NoError(t, unittest.PrepareTestDatabase()) + before := unittest.AssertExistsAndLoadBean(t, &ActionRunner{ID: recordID}) + require.Equal(t, + before.TokenHash, auth_model.HashToken(oldToken, before.TokenSalt), + "the initial token should match the runner's secret", + ) + + RegisterRunner(db.DefaultContext, before.OwnerID, before.RepoID, newToken, nil, before.Name, before.Version) + + after := unittest.AssertExistsAndLoadBean(t, &ActionRunner{ID: recordID}) + + assert.Equal(t, before.UUID, after.UUID) + assert.NotEqual(t, + after.TokenHash, auth_model.HashToken(oldToken, after.TokenSalt), + "the old token can still be verified", + ) + assert.Equal(t, + after.TokenHash, auth_model.HashToken(newToken, after.TokenSalt), + "the new token cannot be verified", + ) +} + func TestActions_RegisterRunner_CreateWithLabels(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) ownerID := int64(0) diff --git a/models/actions/runner.go b/models/actions/runner.go index cfe936c49..3302a930a 100644 --- a/models/actions/runner.go +++ b/models/actions/runner.go @@ -6,10 +6,12 @@ package actions import ( "context" "encoding/binary" + "encoding/hex" "fmt" "strings" "time" + auth_model "code.gitea.io/gitea/models/auth" "code.gitea.io/gitea/models/db" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/shared/types" @@ -151,6 +153,22 @@ func (r *ActionRunner) GenerateToken() (err error) { return err } +// UpdateSecret updates the hash based on the specified token. It does not +// ensure that the runner's UUID matches the first 16 bytes of the token. +func (r *ActionRunner) UpdateSecret(token string) error { + saltBytes, err := util.CryptoRandomBytes(16) + if err != nil { + return fmt.Errorf("CryptoRandomBytes %v", err) + } + + salt := hex.EncodeToString(saltBytes) + + r.Token = token + r.TokenSalt = salt + r.TokenHash = auth_model.HashToken(token, salt) + return nil +} + func init() { db.RegisterModel(&ActionRunner{}) } diff --git a/models/actions/runner_test.go b/models/actions/runner_test.go index a71f5f004..311a6eb0b 100644 --- a/models/actions/runner_test.go +++ b/models/actions/runner_test.go @@ -7,12 +7,28 @@ import ( "fmt" "testing" + auth_model "code.gitea.io/gitea/models/auth" "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/unittest" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +// TestUpdateSecret checks that ActionRunner.UpdateSecret() sets the Token, +// TokenSalt and TokenHash fields based on the specified token. +func TestUpdateSecret(t *testing.T) { + runner := ActionRunner{} + token := "0123456789012345678901234567890123456789" + + err := runner.UpdateSecret(token) + + require.NoError(t, err) + assert.Equal(t, token, runner.Token) + assert.Regexp(t, "^[0-9a-f]{32}$", runner.TokenSalt) + assert.Equal(t, runner.TokenHash, auth_model.HashToken(token, runner.TokenSalt)) +} + func TestDeleteRunner(t *testing.T) { const recordID = 12345678 assert.NoError(t, unittest.PrepareTestDatabase())