Skip to content

Commit b9251fd

Browse files
kylecarbskylecarbs
committed
Fix dbcrypt
1 parent 6651fe1 commit b9251fd

File tree

10 files changed

+144
-13
lines changed

10 files changed

+144
-13
lines changed

‎coderd/database/dbauthz/querier.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1113,6 +1113,10 @@ func (q *querier) InsertUserLink(ctx context.Context, arg database.InsertUserLin
11131113
return q.db.InsertUserLink(ctx, arg)
11141114
}
11151115

1116+
func (q *querier) DeleteUserLinkByLinkedID(ctx context.Context, linkedID string) error {
1117+
return deleteQ(q.log, q.auth, q.db.GetUserLinkByLinkedID, q.db.DeleteUserLinkByLinkedID)(ctx, linkedID)
1118+
}
1119+
11161120
func (q *querier) SoftDeleteUserByID(ctx context.Context, id uuid.UUID) error {
11171121
deleteF := func(ctx context.Context, id uuid.UUID) error {
11181122
return q.db.UpdateUserDeletedByID(ctx, database.UpdateUserDeletedByIDParams{
@@ -1201,6 +1205,15 @@ func (q *querier) GetGitAuthLink(ctx context.Context, arg database.GetGitAuthLin
12011205
return fetch(q.log, q.auth, q.db.GetGitAuthLink)(ctx, arg)
12021206
}
12031207

1208+
func (q *querier) DeleteGitAuthLink(ctx context.Context, arg database.DeleteGitAuthLinkParams) error {
1209+
return deleteQ(q.log, q.auth, func(ctx context.Context, arg database.DeleteGitAuthLinkParams) (database.GitAuthLink, error) {
1210+
return q.db.GetGitAuthLink(ctx, database.GetGitAuthLinkParams{
1211+
ProviderID: arg.ProviderID,
1212+
UserID: arg.UserID,
1213+
})
1214+
}, q.db.DeleteGitAuthLink)(ctx, arg)
1215+
}
1216+
12041217
func (q *querier) InsertGitAuthLink(ctx context.Context, arg database.InsertGitAuthLinkParams) (database.GitAuthLink, error) {
12051218
return insert(q.log, q.auth, rbac.ResourceUserData.WithOwner(arg.UserID.String()).WithID(arg.UserID), q.db.InsertGitAuthLink)(ctx, arg)
12061219
}

‎coderd/database/dbcrypt/dbcrypt.go

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,6 @@ import (
1717
// If it is encrypted but a key is not provided, an error is returned.
1818
const MagicPrefix = "dbcrypt-"
1919

20-
// ErrInvalidCipher is returned when an invalid cipher is provided
21-
// for the encrypted data.
22-
var ErrInvalidCipher = xerrors.New("an invalid encryption cipher was provided for the encrypted data")
23-
2420
type Options struct {
2521
// ExternalTokenCipher is an optional cipher that is used
2622
// to encrypt/decrypt user link and git auth link tokens. If this is nil,
@@ -56,15 +52,19 @@ func (db *dbCrypt) GetUserLinkByLinkedID(ctx context.Context, linkedID string) (
5652
if err != nil {
5753
return database.UserLink{}, err
5854
}
59-
return link, db.decryptFields(&link.OAuthAccessToken, &link.OAuthRefreshToken)
55+
return link, db.decryptFields(func() error {
56+
return db.Store.DeleteUserLinkByLinkedID(ctx, linkedID)
57+
}, &link.OAuthAccessToken, &link.OAuthRefreshToken)
6058
}
6159

6260
func (db *dbCrypt) GetUserLinkByUserIDLoginType(ctx context.Context, params database.GetUserLinkByUserIDLoginTypeParams) (database.UserLink, error) {
6361
link, err := db.Store.GetUserLinkByUserIDLoginType(ctx, params)
6462
if err != nil {
6563
return database.UserLink{}, err
6664
}
67-
return link, db.decryptFields(&link.OAuthAccessToken, &link.OAuthRefreshToken)
65+
return link, db.decryptFields(func() error {
66+
return db.Store.DeleteUserLinkByLinkedID(ctx, link.LinkedID)
67+
}, &link.OAuthAccessToken, &link.OAuthRefreshToken)
6868
}
6969

7070
func (db *dbCrypt) InsertUserLink(ctx context.Context, params database.InsertUserLinkParams) (database.UserLink, error) {
@@ -96,7 +96,12 @@ func (db *dbCrypt) GetGitAuthLink(ctx context.Context, params database.GetGitAut
9696
if err != nil {
9797
return database.GitAuthLink{}, err
9898
}
99-
return link, db.decryptFields(&link.OAuthAccessToken, &link.OAuthRefreshToken)
99+
return link, db.decryptFields(func() error {
100+
return db.Store.DeleteGitAuthLink(ctx, database.DeleteGitAuthLinkParams{
101+
ProviderID: params.ProviderID,
102+
UserID: params.UserID,
103+
})
104+
}, &link.OAuthAccessToken, &link.OAuthRefreshToken)
100105
}
101106

102107
func (db *dbCrypt) UpdateGitAuthLink(ctx context.Context, params database.UpdateGitAuthLinkParams) (database.GitAuthLink, error) {
@@ -130,7 +135,15 @@ func (db *dbCrypt) encryptFields(fields ...*string) error {
130135

131136
// decryptFields decrypts the given fields in place.
132137
// If the value fails to decrypt, sql.ErrNoRows will be returned.
133-
func (db *dbCrypt) decryptFields(fields ...*string) error {
138+
func (db *dbCrypt) decryptFields(deleteFn func() error, fields ...*string) error {
139+
delete := func() error {
140+
err := deleteFn()
141+
if err != nil {
142+
return xerrors.Errorf("delete encrypted row: %w", err)
143+
}
144+
return sql.ErrNoRows
145+
}
146+
134147
cipherPtr := db.ExternalTokenCipher.Load()
135148
// If no cipher is loaded, then we don't need to encrypt or decrypt anything!
136149
if cipherPtr == nil {
@@ -139,7 +152,9 @@ func (db *dbCrypt) decryptFields(fields ...*string) error {
139152
continue
140153
}
141154
if strings.HasPrefix(*field, MagicPrefix) {
142-
return ErrInvalidCipher
155+
// If we have a magic prefix but encryption is disabled,
156+
// we should delete the row.
157+
return delete()
143158
}
144159
}
145160
return nil
@@ -156,7 +171,8 @@ func (db *dbCrypt) decryptFields(fields ...*string) error {
156171

157172
decrypted, err := cipher.Decrypt([]byte((*field)[len(MagicPrefix):]))
158173
if err != nil {
159-
return xerrors.Errorf("%w: %s", ErrInvalidCipher, err)
174+
// If the encryption key changed, we should delete the row.
175+
return delete()
160176
}
161177
*field = string(decrypted)
162178
}

‎coderd/database/dbcrypt/dbcrypt_test.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package dbcrypt_test
33
import (
44
"context"
55
"crypto/rand"
6+
"database/sql"
67
"io"
78
"sync/atomic"
89
"testing"
@@ -69,7 +70,7 @@ func TestUserLinks(t *testing.T) {
6970
initCipher(t, cipher)
7071

7172
link, err = crypt.GetUserLinkByLinkedID(ctx, link.LinkedID)
72-
require.ErrorIs(t, err, dbcrypt.ErrInvalidCipher)
73+
require.ErrorIs(t, err, sql.ErrNoRows)
7374
})
7475

7576
t.Run("GetUserLinkByUserIDLoginType", func(t *testing.T) {
@@ -95,7 +96,7 @@ func TestUserLinks(t *testing.T) {
9596
UserID: link.UserID,
9697
LoginType: link.LoginType,
9798
})
98-
require.ErrorIs(t, err, dbcrypt.ErrInvalidCipher)
99+
require.ErrorIs(t, err, sql.ErrNoRows)
99100
})
100101
}
101102

@@ -164,7 +165,7 @@ func TestGitAuthLinks(t *testing.T) {
164165
UserID: link.UserID,
165166
ProviderID: link.ProviderID,
166167
})
167-
require.ErrorIs(t, err, dbcrypt.ErrInvalidCipher)
168+
require.ErrorIs(t, err, sql.ErrNoRows)
168169
})
169170
}
170171

‎coderd/database/dbfake/databasefake.go

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4380,6 +4380,24 @@ func (q *fakeQuerier) UpdateGroupByID(_ context.Context, arg database.UpdateGrou
43804380
return database.Group{}, sql.ErrNoRows
43814381
}
43824382

4383+
func (q *fakeQuerier) DeleteGitAuthLink(_ context.Context, arg database.DeleteGitAuthLinkParams) error {
4384+
q.mutex.Lock()
4385+
defer q.mutex.Unlock()
4386+
4387+
for index, link := range q.gitAuthLinks {
4388+
if link.ProviderID != arg.ProviderID {
4389+
continue
4390+
}
4391+
if link.UserID != arg.UserID {
4392+
continue
4393+
}
4394+
q.gitAuthLinks[index] = q.gitAuthLinks[len(q.gitAuthLinks)-1]
4395+
q.gitAuthLinks = q.gitAuthLinks[:len(q.gitAuthLinks)-1]
4396+
return nil
4397+
}
4398+
return sql.ErrNoRows
4399+
}
4400+
43834401
func (q *fakeQuerier) DeleteGitSSHKey(_ context.Context, userID uuid.UUID) error {
43844402
q.mutex.Lock()
43854403
defer q.mutex.Unlock()
@@ -4911,6 +4929,21 @@ func (q *fakeQuerier) DeleteReplicasUpdatedBefore(_ context.Context, before time
49114929
return nil
49124930
}
49134931

4932+
func (q *fakeQuerier) DeleteUserLinkByLinkedID(ctx context.Context, linkedID string) error {
4933+
q.mutex.Lock()
4934+
defer q.mutex.Unlock()
4935+
4936+
for index, link := range q.userLinks {
4937+
if link.LinkedID != linkedID {
4938+
continue
4939+
}
4940+
q.userLinks[index] = q.userLinks[len(q.userLinks)-1]
4941+
q.userLinks = q.userLinks[:len(q.userLinks)-1]
4942+
return nil
4943+
}
4944+
return sql.ErrNoRows
4945+
}
4946+
49144947
func (q *fakeQuerier) InsertReplica(_ context.Context, arg database.InsertReplicaParams) (database.Replica, error) {
49154948
if err := validateDatabaseType(arg); err != nil {
49164949
return database.Replica{}, err

‎coderd/database/dbmock/store.go

Lines changed: 28 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/querier.go

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries.sql.go

Lines changed: 26 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/gitauth.sql

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,3 +27,6 @@ UPDATE git_auth_links SET
2727
oauth_refresh_token = $5,
2828
oauth_expiry = $6
2929
WHERE provider_id = $1 AND user_id = $2 RETURNING *;
30+
31+
-- name: DeleteGitAuthLink :exec
32+
DELETE FROM git_auth_links WHERE provider_id = $1 AND user_id = $2;

‎coderd/database/queries/user_links.sql

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,3 +44,9 @@ SET
4444
oauth_expiry = $3
4545
WHERE
4646
user_id = $4 AND login_type = $5 RETURNING *;
47+
48+
-- name: DeleteUserLinkByLinkedID :exec
49+
DELETE FROM
50+
user_links
51+
WHERE
52+
linked_id = $1;

‎site/src/api/typesGenerated.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -362,6 +362,7 @@ export interface DeploymentValues {
362362
readonly agent_fallback_troubleshooting_url?: string
363363
readonly browser_only?: boolean
364364
readonly scim_api_key?: string
365+
readonly external_token_encryption_key: string
365366
readonly provisioner?: ProvisionerConfig
366367
readonly rate_limit?: RateLimitConfig
367368
// This is likely an enum in an external package ("github.com/coder/coder/cli/clibase.StringArray")
@@ -1395,6 +1396,7 @@ export type FeatureName =
13951396
| "audit_log"
13961397
| "browser_only"
13971398
| "external_provisioner_daemons"
1399+
| "external_token_encryption"
13981400
| "high_availability"
13991401
| "multiple_git_auth"
14001402
| "scim"
@@ -1407,6 +1409,7 @@ export const FeatureNames: FeatureName[] = [
14071409
"audit_log",
14081410
"browser_only",
14091411
"external_provisioner_daemons",
1412+
"external_token_encryption",
14101413
"high_availability",
14111414
"multiple_git_auth",
14121415
"scim",

0 commit comments

Comments
 (0)