chore: shorten provisioner key (#14017)

f0ssel · web-flow · commit 6c2336b8e927 · 2024-07-25T16:08:12.000-05:00
diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go
@@ -1680,6 +1680,10 @@ func (q *querier) GetProvisionerJobsCreatedAfter(ctx context.Context, createdAt
 	return q.db.GetProvisionerJobsCreatedAfter(ctx, createdAt)
 }
 
+func (q *querier) GetProvisionerKeyByHashedSecret(ctx context.Context, hashedSecret []byte) (database.ProvisionerKey, error) {
+	return fetch(q.log, q.auth, q.db.GetProvisionerKeyByHashedSecret)(ctx, hashedSecret)
+}
+
 func (q *querier) GetProvisionerKeyByID(ctx context.Context, id uuid.UUID) (database.ProvisionerKey, error) {
 	return fetch(q.log, q.auth, q.db.GetProvisionerKeyByID)(ctx, id)
 }
diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go
@@ -1825,6 +1825,11 @@ func (s *MethodTestSuite) TestProvisionerKeys() {
 		pk := dbgen.ProvisionerKey(s.T(), db, database.ProvisionerKey{OrganizationID: org.ID})
 		check.Args(pk.ID).Asserts(pk, policy.ActionRead).Returns(pk)
 	}))
+	s.Run("GetProvisionerKeyByHashedSecret", s.Subtest(func(db database.Store, check *expects) {
+		org := dbgen.Organization(s.T(), db, database.Organization{})
+		pk := dbgen.ProvisionerKey(s.T(), db, database.ProvisionerKey{OrganizationID: org.ID, HashedSecret: []byte("foo")})
+		check.Args([]byte("foo")).Asserts(pk, policy.ActionRead).Returns(pk)
+	}))
 	s.Run("GetProvisionerKeyByName", s.Subtest(func(db database.Store, check *expects) {
 		org := dbgen.Organization(s.T(), db, database.Organization{})
 		pk := dbgen.ProvisionerKey(s.T(), db, database.ProvisionerKey{OrganizationID: org.ID})
diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go
@@ -3240,6 +3240,19 @@ func (q *FakeQuerier) GetProvisionerJobsCreatedAfter(_ context.Context, after ti
 	return jobs, nil
 }
 
+func (q *FakeQuerier) GetProvisionerKeyByHashedSecret(_ context.Context, hashedSecret []byte) (database.ProvisionerKey, error) {
+	q.mutex.RLock()
+	defer q.mutex.RUnlock()
+
+	for _, key := range q.provisionerKeys {
+		if bytes.Equal(key.HashedSecret, hashedSecret) {
+			return key, nil
+		}
+	}
+
+	return database.ProvisionerKey{}, sql.ErrNoRows
+}
+
 func (q *FakeQuerier) GetProvisionerKeyByID(_ context.Context, id uuid.UUID) (database.ProvisionerKey, error) {
 	q.mutex.RLock()
 	defer q.mutex.RUnlock()
diff --git a/coderd/database/dbmetrics/dbmetrics.go b/coderd/database/dbmetrics/dbmetrics.go
diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go
diff --git a/coderd/database/querier.go b/coderd/database/querier.go
diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go
diff --git a/coderd/database/queries/provisionerkeys.sql b/coderd/database/queries/provisionerkeys.sql
@@ -19,6 +19,14 @@ FROM
 WHERE
     id = $1;
 
+-- name: GetProvisionerKeyByHashedSecret :one
+SELECT
+    *
+FROM
+    provisioner_keys
+WHERE
+    hashed_secret = $1;
+
 -- name: GetProvisionerKeyByName :one
 SELECT
     *
diff --git a/coderd/httpmw/provisionerdaemon.go b/coderd/httpmw/provisionerdaemon.go
@@ -71,16 +71,17 @@ func ExtractProvisionerDaemonAuthenticated(opts ExtractProvisionerAuthConfig) fu
 				return
 			}
 
-			id, keyValue, err := provisionerkey.Parse(key)
+			err := provisionerkey.Validate(key)
 			if err != nil {
-				handleOptional(http.StatusUnauthorized, codersdk.Response{
+				handleOptional(http.StatusBadRequest, codersdk.Response{
 					Message: "provisioner daemon key invalid",
+					Detail:  err.Error(),
 				})
 				return
 			}
-
+			hashedKey := provisionerkey.HashSecret(key)
 			// nolint:gocritic // System must check if the provisioner key is valid.
-			pk, err := opts.DB.GetProvisionerKeyByID(dbauthz.AsSystemRestricted(ctx), id)
+			pk, err := opts.DB.GetProvisionerKeyByHashedSecret(dbauthz.AsSystemRestricted(ctx), hashedKey)
 			if err != nil {
 				if httpapi.Is404Error(err) {
 					handleOptional(http.StatusUnauthorized, codersdk.Response{
@@ -90,12 +91,13 @@ func ExtractProvisionerDaemonAuthenticated(opts ExtractProvisionerAuthConfig) fu
 				}
 
 				handleOptional(http.StatusInternalServerError, codersdk.Response{
-					Message: "get provisioner daemon key: " + err.Error(),
+					Message: "get provisioner daemon key",
+					Detail:  err.Error(),
 				})
 				return
 			}
 
-			if provisionerkey.Compare(pk.HashedSecret, provisionerkey.HashSecret(keyValue)) {
+			if provisionerkey.Compare(pk.HashedSecret, hashedKey) {
 				handleOptional(http.StatusUnauthorized, codersdk.Response{
 					Message: "provisioner daemon key invalid",
 				})
diff --git a/coderd/provisionerkey/provisionerkey.go b/coderd/provisionerkey/provisionerkey.go
@@ -3,8 +3,6 @@ package provisionerkey
 import (
 	"crypto/sha256"
 	"crypto/subtle"
-	"fmt"
-	"strings"
 
 	"github.com/google/uuid"
 	"golang.org/x/xerrors"
@@ -14,41 +12,36 @@ import (
 	"github.com/coder/coder/v2/cryptorand"
 )
 
+const (
+	secretLength = 43
+)
+
 func New(organizationID uuid.UUID, name string, tags map[string]string) (database.InsertProvisionerKeyParams, string, error) {
-	id := uuid.New()
-	secret, err := cryptorand.HexString(64)
+	secret, err := cryptorand.String(secretLength)
 	if err != nil {
-		return database.InsertProvisionerKeyParams{}, "", xerrors.Errorf("generate token: %w", err)
+		return database.InsertProvisionerKeyParams{}, "", xerrors.Errorf("generate secret: %w", err)
 	}
-	hashedSecret := HashSecret(secret)
-	token := fmt.Sprintf("%s:%s", id, secret)
 
 	if tags == nil {
 		tags = map[string]string{}
 	}
 
 	return database.InsertProvisionerKeyParams{
-		ID:             id,
+		ID:             uuid.New(),
 		CreatedAt:      dbtime.Now(),
 		OrganizationID: organizationID,
 		Name:           name,
-		HashedSecret:   hashedSecret,
+		HashedSecret:   HashSecret(secret),
 		Tags:           tags,
-	}, token, nil
+	}, secret, nil
 }
 
-func Parse(token string) (uuid.UUID, string, error) {
-	parts := strings.Split(token, ":")
-	if len(parts) != 2 {
-		return uuid.UUID{}, "", xerrors.Errorf("invalid token format")
-	}
-
-	id, err := uuid.Parse(parts[0])
-	if err != nil {
-		return uuid.UUID{}, "", xerrors.Errorf("parse id: %w", err)
+func Validate(token string) error {
+	if len(token) != secretLength {
+		return xerrors.Errorf("must be %d characters", secretLength)
 	}
 
-	return id, parts[1], nil
+	return nil
 }
 
 func HashSecret(secret string) []byte {
diff --git a/enterprise/cli/provisionerdaemonstart.go b/enterprise/cli/provisionerdaemonstart.go
@@ -122,9 +122,9 @@ func (r *RootCmd) provisionerDaemonStart() *serpent.Command {
 				if len(rawTags) > 0 {
 					return xerrors.New("cannot provide tags when using provisioner key")
 				}
-				_, _, err := provisionerkey.Parse(provisionerKey)
+				err = provisionerkey.Validate(provisionerKey)
 				if err != nil {
-					return xerrors.Errorf("parse provisioner key: %w", err)
+					return xerrors.Errorf("validate provisioner key: %w", err)
 				}
 			}
 
diff --git a/enterprise/cli/provisionerkeys_test.go b/enterprise/cli/provisionerkeys_test.go
@@ -4,11 +4,11 @@ import (
 	"strings"
 	"testing"
 
-	"github.com/google/uuid"
 	"github.com/stretchr/testify/require"
 
 	"github.com/coder/coder/v2/cli/clitest"
 	"github.com/coder/coder/v2/coderd/coderdtest"
+	"github.com/coder/coder/v2/coderd/provisionerkey"
 	"github.com/coder/coder/v2/coderd/rbac"
 	"github.com/coder/coder/v2/codersdk"
 	"github.com/coder/coder/v2/enterprise/coderd/coderdenttest"
@@ -58,10 +58,7 @@ func TestProvisionerKeys(t *testing.T) {
 		_ = pty.ReadLine(ctx)
 		key := pty.ReadLine(ctx)
 		require.NotEmpty(t, key)
-		parts := strings.Split(key, ":")
-		require.Len(t, parts, 2, "expected 2 parts")
-		_, err = uuid.Parse(parts[0])
-		require.NoError(t, err, "expected token to be a uuid")
+		require.NoError(t, provisionerkey.Validate(key))
 
 		inv, conf = newCLI(
 			t,
diff --git a/enterprise/coderd/provisionerdaemons_test.go b/enterprise/coderd/provisionerdaemons_test.go
@@ -6,7 +6,6 @@ import (
 	"fmt"
 	"io"
 	"net/http"
-	"strings"
 	"testing"
 
 	"github.com/google/uuid"
@@ -612,36 +611,12 @@ func TestProvisionerDaemonServe(t *testing.T) {
 				errStatusCode:             http.StatusUnauthorized,
 			},
 			{
-				name:                      "WrongKey",
+				name:                      "InvalidKey",
 				multiOrgFeatureEnabled:    true,
 				multiOrgExperimentEnabled: true,
 				insertParams:              insertParams,
 				requestProvisionerKey:     "provisionersftw",
-				errStatusCode:             http.StatusUnauthorized,
-			},
-			{
-				name:                      "IdOKKeyValueWrong",
-				multiOrgFeatureEnabled:    true,
-				multiOrgExperimentEnabled: true,
-				insertParams:              insertParams,
-				requestProvisionerKey:     insertParams.ID.String() + ":" + "wrong",
-				errStatusCode:             http.StatusUnauthorized,
-			},
-			{
-				name:                      "IdWrongKeyValueOK",
-				multiOrgFeatureEnabled:    true,
-				multiOrgExperimentEnabled: true,
-				insertParams:              insertParams,
-				requestProvisionerKey:     uuid.NewString() + ":" + token,
-				errStatusCode:             http.StatusUnauthorized,
-			},
-			{
-				name:                      "KeyValueOnly",
-				multiOrgFeatureEnabled:    true,
-				multiOrgExperimentEnabled: true,
-				insertParams:              insertParams,
-				requestProvisionerKey:     strings.Split(token, ":")[1],
-				errStatusCode:             http.StatusUnauthorized,
+				errStatusCode:             http.StatusBadRequest,
 			},
 			{
 				name:                      "KeyAndPSK",

Original file line number	Diff line number	Diff line change
`@@ -1680,6 +1680,10 @@ func (q *querier) GetProvisionerJobsCreatedAfter(ctx context.Context, createdAt`
`1680`	`1680`	`return q.db.GetProvisionerJobsCreatedAfter(ctx, createdAt)`
`1681`	`1681`	`}`
`1682`	`1682`
	`1683`	`+func (q *querier) GetProvisionerKeyByHashedSecret(ctx context.Context, hashedSecret []byte) (database.ProvisionerKey, error) {`
	`1684`	`+ return fetch(q.log, q.auth, q.db.GetProvisionerKeyByHashedSecret)(ctx, hashedSecret)`
	`1685`	`+}`
	`1686`	`+`
`1683`	`1687`	`func (q *querier) GetProvisionerKeyByID(ctx context.Context, id uuid.UUID) (database.ProvisionerKey, error) {`
`1684`	`1688`	`return fetch(q.log, q.auth, q.db.GetProvisionerKeyByID)(ctx, id)`
`1685`	`1689`	`}`