Merge pull request 'feat(cli): allow updates to runners' secrets' (#4619) from tseeker/forgejo:20240722-update-secret into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/4619
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>
Reviewed-by: Gusted <gusted@noreply.codeberg.org>

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)

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)

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{})
 }

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())