Merge pull request 'Use hashed webhook secrets for constant-time comparison (closes #13)' (#15) from clawbot/upaas:fix/issue-13 into main

Reviewed-on: #15

internal/database/database.go

@@ -3,7 +3,9 @@ package database
 
 import (
 	"context"
+	"crypto/sha256"
 	"database/sql"
+	"encoding/hex"
 	"fmt"
 	"log/slog"
 	"os"
@@ -158,6 +160,60 @@ func (d *Database) connect(ctx context.Context) error {
 		return fmt.Errorf("failed to run migrations: %w", err)
 	}
 
+	// Backfill webhook_secret_hash for any rows that have a secret but no hash
+	err = d.backfillWebhookSecretHashes(ctx)
+	if err != nil {
+		return fmt.Errorf("failed to backfill webhook secret hashes: %w", err)
+	}
+
+	return nil
+}
+
+// HashWebhookSecret returns the hex-encoded SHA-256 hash of a webhook secret.
+func HashWebhookSecret(secret string) string {
+	sum := sha256.Sum256([]byte(secret))
+	return hex.EncodeToString(sum[:])
+}
+
+func (d *Database) backfillWebhookSecretHashes(ctx context.Context) error {
+	rows, err := d.database.QueryContext(ctx,
+		"SELECT id, webhook_secret FROM apps WHERE webhook_secret_hash = '' AND webhook_secret != ''")
+	if err != nil {
+		return fmt.Errorf("querying apps for backfill: %w", err)
+	}
+	defer func() { _ = rows.Close() }()
+
+	type row struct {
+		id, secret string
+	}
+
+	var toUpdate []row
+
+	for rows.Next() {
+		var r row
+		if scanErr := rows.Scan(&r.id, &r.secret); scanErr != nil {
+			return fmt.Errorf("scanning app for backfill: %w", scanErr)
+		}
+
+		toUpdate = append(toUpdate, r)
+	}
+
+	if rowsErr := rows.Err(); rowsErr != nil {
+		return fmt.Errorf("iterating apps for backfill: %w", rowsErr)
+	}
+
+	for _, r := range toUpdate {
+		hash := HashWebhookSecret(r.secret)
+
+		_, updateErr := d.database.ExecContext(ctx,
+			"UPDATE apps SET webhook_secret_hash = ? WHERE id = ?", hash, r.id)
+		if updateErr != nil {
+			return fmt.Errorf("updating webhook_secret_hash for app %s: %w", r.id, updateErr)
+		}
+
+		d.log.Info("backfilled webhook_secret_hash", "app_id", r.id)
+	}
+
 	return nil
 }
 

internal/database/hash_test.go

@@ -0,0 +1,28 @@
+package database_test
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"git.eeqj.de/sneak/upaas/internal/database"
+)
+
+func TestHashWebhookSecret(t *testing.T) {
+	t.Parallel()
+
+	// Known SHA-256 of "test-secret"
+	hash := database.HashWebhookSecret("test-secret")
+	assert.Equal(t,
+		"9caf06bb4436cdbfa20af9121a626bc1093c4f54b31c0fa937957856135345b6",
+		hash,
+	)
+
+	// Different secrets produce different hashes
+	hash2 := database.HashWebhookSecret("other-secret")
+	assert.NotEqual(t, hash, hash2)
+
+	// Same secret always produces same hash (deterministic)
+	hash3 := database.HashWebhookSecret("test-secret")
+	assert.Equal(t, hash, hash3)
+}

internal/database/migrations/005_add_webhook_secret_hash.sql

@@ -0,0 +1,2 @@
+-- Add webhook_secret_hash column for constant-time secret lookup
+ALTER TABLE apps ADD COLUMN webhook_secret_hash TEXT NOT NULL DEFAULT '';

internal/models/app.go

@@ -10,6 +10,12 @@ import (
 	"git.eeqj.de/sneak/upaas/internal/database"
 )
 
+// appColumns is the standard column list for app queries.
+const appColumns = `id, name, repo_url, branch, dockerfile_path, webhook_secret,
+			ssh_private_key, ssh_public_key, image_id, status,
+			docker_network, ntfy_topic, slack_webhook, webhook_secret_hash,
+			created_at, updated_at`
+
 // AppStatus represents the status of an app.
 type AppStatus string
 
@@ -32,6 +38,7 @@ type App struct {
 	Branch         string
 	DockerfilePath string
 	WebhookSecret     string
+	WebhookSecretHash string
 	SSHPrivateKey     string
 	SSHPublicKey   string
 	ImageID        sql.NullString
@@ -70,11 +77,8 @@ func (a *App) Delete(ctx context.Context) error {
 
 // Reload refreshes the app from the database.
 func (a *App) Reload(ctx context.Context) error {
-	row := a.db.QueryRow(ctx, `
-		SELECT id, name, repo_url, branch, dockerfile_path, webhook_secret,
-			ssh_private_key, ssh_public_key, image_id, status,
-			docker_network, ntfy_topic, slack_webhook, created_at, updated_at
-		FROM apps WHERE id = ?`,
+	row := a.db.QueryRow(ctx,
+		"SELECT "+appColumns+" FROM apps WHERE id = ?",
 		a.ID,
 	)
 
@@ -136,13 +140,13 @@ func (a *App) insert(ctx context.Context) error {
 		INSERT INTO apps (
 			id, name, repo_url, branch, dockerfile_path, webhook_secret,
 			ssh_private_key, ssh_public_key, image_id, status,
-			docker_network, ntfy_topic, slack_webhook
-		) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`
+			docker_network, ntfy_topic, slack_webhook, webhook_secret_hash
+		) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`
 
 	_, err := a.db.Exec(ctx, query,
 		a.ID, a.Name, a.RepoURL, a.Branch, a.DockerfilePath, a.WebhookSecret,
 		a.SSHPrivateKey, a.SSHPublicKey, a.ImageID, a.Status,
-		a.DockerNetwork, a.NtfyTopic, a.SlackWebhook,
+		a.DockerNetwork, a.NtfyTopic, a.SlackWebhook, a.WebhookSecretHash,
 	)
 	if err != nil {
 		return err
@@ -177,6 +181,7 @@ func (a *App) scan(row *sql.Row) error {
 		&a.SSHPrivateKey, &a.SSHPublicKey,
 		&a.ImageID, &a.Status,
 		&a.DockerNetwork, &a.NtfyTopic, &a.SlackWebhook,
+		&a.WebhookSecretHash,
 		&a.CreatedAt, &a.UpdatedAt,
 	)
 }
@@ -193,6 +198,7 @@ func scanApps(appDB *database.Database, rows *sql.Rows) ([]*App, error) {
 			&app.SSHPrivateKey, &app.SSHPublicKey,
 			&app.ImageID, &app.Status,
 			&app.DockerNetwork, &app.NtfyTopic, &app.SlackWebhook,
+			&app.WebhookSecretHash,
 			&app.CreatedAt, &app.UpdatedAt,
 		)
 		if scanErr != nil {
@@ -221,11 +227,8 @@ func FindApp(
 	app := NewApp(appDB)
 	app.ID = appID
 
-	row := appDB.QueryRow(ctx, `
-		SELECT id, name, repo_url, branch, dockerfile_path, webhook_secret,
-			ssh_private_key, ssh_public_key, image_id, status,
-			docker_network, ntfy_topic, slack_webhook, created_at, updated_at
-		FROM apps WHERE id = ?`,
+	row := appDB.QueryRow(ctx,
+		"SELECT "+appColumns+" FROM apps WHERE id = ?",
 		appID,
 	)
 
@@ -241,7 +244,8 @@ func FindApp(
 	return app, nil
 }
 
-// FindAppByWebhookSecret finds an app by webhook secret.
+// FindAppByWebhookSecret finds an app by webhook secret using a SHA-256 hash
+// lookup. This avoids SQL string comparison timing side-channels.
 //
 //nolint:nilnil // returning nil,nil is idiomatic for "not found" in Active Record
 func FindAppByWebhookSecret(
@@ -250,13 +254,11 @@ func FindAppByWebhookSecret(
 	secret string,
 ) (*App, error) {
 	app := NewApp(appDB)
+	secretHash := database.HashWebhookSecret(secret)
 
-	row := appDB.QueryRow(ctx, `
-		SELECT id, name, repo_url, branch, dockerfile_path, webhook_secret,
-			ssh_private_key, ssh_public_key, image_id, status,
-			docker_network, ntfy_topic, slack_webhook, created_at, updated_at
-		FROM apps WHERE webhook_secret = ?`,
-		secret,
+	row := appDB.QueryRow(ctx,
+		"SELECT "+appColumns+" FROM apps WHERE webhook_secret_hash = ?",
+		secretHash,
 	)
 
 	err := app.scan(row)
@@ -273,11 +275,8 @@ func FindAppByWebhookSecret(
 
 // AllApps returns all apps ordered by name.
 func AllApps(ctx context.Context, appDB *database.Database) ([]*App, error) {
-	rows, err := appDB.Query(ctx, `
-		SELECT id, name, repo_url, branch, dockerfile_path, webhook_secret,
-			ssh_private_key, ssh_public_key, image_id, status,
-			docker_network, ntfy_topic, slack_webhook, created_at, updated_at
-		FROM apps ORDER BY name`,
+	rows, err := appDB.Query(ctx,
+		"SELECT "+appColumns+" FROM apps ORDER BY name",
 	)
 	if err != nil {
 		return nil, fmt.Errorf("querying all apps: %w", err)

internal/models/models_test.go

@@ -297,6 +297,7 @@ func TestAllApps(t *testing.T) {
 			app.Branch = testBranch
 			app.DockerfilePath = "Dockerfile"
 			app.WebhookSecret = "secret-" + strconv.Itoa(idx)
+			app.WebhookSecretHash = database.HashWebhookSecret(app.WebhookSecret)
 			app.SSHPrivateKey = "private"
 			app.SSHPublicKey = "public"
 
@@ -791,6 +792,7 @@ func createTestApp(t *testing.T, testDB *database.Database) *models.App {
 	app.Branch = testBranch
 	app.DockerfilePath = "Dockerfile"
 	app.WebhookSecret = "secret-" + t.Name()
+	app.WebhookSecretHash = database.HashWebhookSecret(app.WebhookSecret)
 	app.SSHPrivateKey = "private"
 	app.SSHPublicKey = "public"
 

internal/service/app/app.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/google/uuid"
 	"github.com/oklog/ulid/v2"
+
 	"go.uber.org/fx"
 
 	"git.eeqj.de/sneak/upaas/internal/database"
@@ -82,6 +83,7 @@ func (svc *Service) CreateApp(
 	}
 
 	app.WebhookSecret = uuid.New().String()
+	app.WebhookSecretHash = database.HashWebhookSecret(app.WebhookSecret)
 	app.SSHPrivateKey = keyPair.PrivateKey
 	app.SSHPublicKey = keyPair.PublicKey
 	app.Status = models.AppStatusPending

internal/service/webhook/webhook_test.go

@@ -91,6 +91,7 @@ func createTestApp(
 	app.Branch = branch
 	app.DockerfilePath = "Dockerfile"
 	app.WebhookSecret = "webhook-secret-123"
+	app.WebhookSecretHash = database.HashWebhookSecret(app.WebhookSecret)
 	app.SSHPrivateKey = "private-key"
 	app.SSHPublicKey = "public-key"
 	app.Status = models.AppStatusPending