Skip to content

Commit 72dff7f

Browse files
johnstcnjohnstcn
authored
fix(enterprise/dbcrypt): do not skip deleted users when encrypting or deleting (#9694)
- Broadens scope of data generation in TestServerDBCrypt over all user login types, statuses, and deletion status. - Adds support for specifying user status / user deletion status in dbgen - Adds more comprehensive logging in TestServerDBCrypt upon test failure (to be generalized and expanded upon in a follow-up) - Adds AllUserIDs query, updates dbcrypt to use this instead of GetUsers.
1 parent bc97eaa commit 72dff7f

File tree

11 files changed

+214
-50
lines changed

11 files changed

+214
-50
lines changed

‎coderd/database/dbauthz/dbauthz.go

+9
Original file line numberDiff line numberDiff line change
@@ -664,6 +664,15 @@ func (q *querier) ActivityBumpWorkspace(ctx context.Context, arg uuid.UUID) erro
664664
return update(q.log, q.auth, fetch, q.db.ActivityBumpWorkspace)(ctx, arg)
665665
}
666666

667+
func (q *querier) AllUserIDs(ctx context.Context) ([]uuid.UUID, error) {
668+
// Although this technically only reads users, only system-related functions should be
669+
// allowed to call this.
670+
if err := q.authorizeContext(ctx, rbac.ActionRead, rbac.ResourceSystem); err != nil {
671+
return nil, err
672+
}
673+
return q.db.AllUserIDs(ctx)
674+
}
675+
667676
func (q *querier) CleanTailnetCoordinators(ctx context.Context) error {
668677
if err := q.authorizeContext(ctx, rbac.ActionDelete, rbac.ResourceTailnetCoordinator); err != nil {
669678
return err

‎coderd/database/dbfake/dbfake.go

+10
Original file line numberDiff line numberDiff line change
@@ -812,6 +812,16 @@ func (q *FakeQuerier) ActivityBumpWorkspace(ctx context.Context, workspaceID uui
812812
return sql.ErrNoRows
813813
}
814814

815+
func (q *FakeQuerier) AllUserIDs(_ context.Context) ([]uuid.UUID, error) {
816+
q.mutex.RLock()
817+
defer q.mutex.RUnlock()
818+
userIDs := make([]uuid.UUID, 0, len(q.users))
819+
for idx := range q.users {
820+
userIDs[idx] = q.users[idx].ID
821+
}
822+
return userIDs, nil
823+
}
824+
815825
func (*FakeQuerier) CleanTailnetCoordinators(_ context.Context) error {
816826
return ErrUnimplemented
817827
}

‎coderd/database/dbgen/dbgen.go

+9-1
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,7 @@ func User(t testing.TB, db database.Store, orig database.User) database.User {
227227

228228
user, err = db.UpdateUserStatus(genCtx, database.UpdateUserStatusParams{
229229
ID: user.ID,
230-
Status: database.UserStatusActive,
230+
Status: takeFirst(orig.Status, database.UserStatusActive),
231231
UpdatedAt: dbtime.Now(),
232232
})
233233
require.NoError(t, err, "insert user")
@@ -240,6 +240,14 @@ func User(t testing.TB, db database.Store, orig database.User) database.User {
240240
})
241241
require.NoError(t, err, "user last seen")
242242
}
243+
244+
if orig.Deleted {
245+
err = db.UpdateUserDeletedByID(genCtx, database.UpdateUserDeletedByIDParams{
246+
ID: user.ID,
247+
Deleted: orig.Deleted,
248+
})
249+
require.NoError(t, err, "set user as deleted")
250+
}
243251
return user
244252
}
245253

‎coderd/database/dbmetrics/dbmetrics.go

+7
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/dbmock/dbmock.go

+15
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/querier.go

+2
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries.sql.go

+28
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/dbcrypt.sql

+1
Original file line numberDiff line numberDiff line change
@@ -16,3 +16,4 @@ AND
1616
INSERT INTO dbcrypt_keys
1717
(number, active_key_digest, created_at, test)
1818
VALUES (@number::int, @active_key_digest::text, CURRENT_TIMESTAMP, @test::text);
19+

‎coderd/database/queries/users.sql

+5
Original file line numberDiff line numberDiff line change
@@ -262,3 +262,8 @@ WHERE
262262
last_seen_at < @last_seen_after :: timestamp
263263
AND status = 'active'::user_status
264264
RETURNING id, email, last_seen_at;
265+
266+
-- AllUserIDs returns all UserIDs regardless of user status or deletion.
267+
-- name: AllUserIDs :many
268+
SELECT DISTINCT id FROM USERS;
269+

‎enterprise/cli/server_dbcrypt_test.go

+106-27
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,9 @@ import (
2020
"github.com/coder/coder/v2/pty/ptytest"
2121
)
2222

23+
// TestServerDBCrypt tests end-to-end encryption, decryption, and deletion
24+
// of encrypted user data.
25+
//
2326
// nolint: paralleltest // use of t.Setenv
2427
func TestServerDBCrypt(t *testing.T) {
2528
if !dbtestutil.WillUsePostgres() {
@@ -41,15 +44,38 @@ func TestServerDBCrypt(t *testing.T) {
4144
})
4245
db := database.New(sqlDB)
4346

47+
t.Cleanup(func() {
48+
if t.Failed() {
49+
t.Logf("Dumping data due to failed test. I hope you find what you're looking for!")
50+
dumpUsers(t, sqlDB)
51+
}
52+
})
53+
4454
// Populate the database with some unencrypted data.
45-
users := genData(t, db, 10)
55+
t.Logf("Generating unencrypted data")
56+
users := genData(t, db)
4657

47-
// Setup an initial cipher
58+
// Setup an initial cipher A
4859
keyA := mustString(t, 32)
4960
cipherA, err := dbcrypt.NewCiphers([]byte(keyA))
5061
require.NoError(t, err)
5162

63+
// Create an encrypted database
64+
cryptdb, err := dbcrypt.New(ctx, db, cipherA...)
65+
require.NoError(t, err)
66+
67+
// Populate the database with some encrypted data using cipher A.
68+
t.Logf("Generating data encrypted with cipher A")
69+
newUsers := genData(t, cryptdb)
70+
71+
// Validate that newly created users were encrypted with cipher A
72+
for _, usr := range newUsers {
73+
requireEncryptedWithCipher(ctx, t, db, cipherA[0], usr.ID)
74+
}
75+
users = append(users, newUsers...)
76+
5277
// Encrypt all the data with the initial cipher.
78+
t.Logf("Encrypting all data with cipher A")
5379
inv, _ := newCLI(t, "server", "dbcrypt", "rotate",
5480
"--postgres-url", connectionURL,
5581
"--new-key", base64.StdEncoding.EncodeToString([]byte(keyA)),
@@ -65,18 +91,12 @@ func TestServerDBCrypt(t *testing.T) {
6591
requireEncryptedWithCipher(ctx, t, db, cipherA[0], usr.ID)
6692
}
6793

68-
// Create an encrypted database
69-
cryptdb, err := dbcrypt.New(ctx, db, cipherA...)
70-
require.NoError(t, err)
71-
72-
// Populate the database with some encrypted data using cipher A.
73-
users = append(users, genData(t, cryptdb, 10)...)
74-
7594
// Re-encrypt all existing data with a new cipher.
7695
keyB := mustString(t, 32)
7796
cipherBA, err := dbcrypt.NewCiphers([]byte(keyB), []byte(keyA))
7897
require.NoError(t, err)
7998

99+
t.Logf("Enrypting all data with cipher B")
80100
inv, _ = newCLI(t, "server", "dbcrypt", "rotate",
81101
"--postgres-url", connectionURL,
82102
"--new-key", base64.StdEncoding.EncodeToString([]byte(keyB)),
@@ -94,6 +114,7 @@ func TestServerDBCrypt(t *testing.T) {
94114
}
95115

96116
// Assert that we can revoke the old key.
117+
t.Logf("Revoking cipher A")
97118
err = db.RevokeDBCryptKey(ctx, cipherA[0].HexDigest())
98119
require.NoError(t, err, "failed to revoke old key")
99120

@@ -109,13 +130,15 @@ func TestServerDBCrypt(t *testing.T) {
109130
require.Empty(t, oldKey.ActiveKeyDigest.String, "expected the old key to not be active")
110131

111132
// Revoking the new key should fail.
133+
t.Logf("Attempting to revoke cipher B should fail as it is still in use")
112134
err = db.RevokeDBCryptKey(ctx, cipherBA[0].HexDigest())
113135
require.Error(t, err, "expected to fail to revoke the new key")
114136
var pgErr *pq.Error
115137
require.True(t, xerrors.As(err, &pgErr), "expected a pg error")
116138
require.EqualValues(t, "23503", pgErr.Code, "expected a foreign key constraint violation error")
117139

118140
// Decrypt the data using only cipher B. This should result in the key being revoked.
141+
t.Logf("Decrypting with cipher B")
119142
inv, _ = newCLI(t, "server", "dbcrypt", "decrypt",
120143
"--postgres-url", connectionURL,
121144
"--keys", base64.StdEncoding.EncodeToString([]byte(keyB)),
@@ -144,6 +167,7 @@ func TestServerDBCrypt(t *testing.T) {
144167
cipherC, err := dbcrypt.NewCiphers([]byte(keyC))
145168
require.NoError(t, err)
146169

170+
t.Logf("Re-encrypting with cipher C")
147171
inv, _ = newCLI(t, "server", "dbcrypt", "rotate",
148172
"--postgres-url", connectionURL,
149173
"--new-key", base64.StdEncoding.EncodeToString([]byte(keyC)),
@@ -161,6 +185,7 @@ func TestServerDBCrypt(t *testing.T) {
161185
}
162186

163187
// Now delete all the encrypted data.
188+
t.Logf("Deleting all encrypted data")
164189
inv, _ = newCLI(t, "server", "dbcrypt", "delete",
165190
"--postgres-url", connectionURL,
166191
"--external-token-encryption-keys", base64.StdEncoding.EncodeToString([]byte(keyC)),
@@ -191,30 +216,84 @@ func TestServerDBCrypt(t *testing.T) {
191216
}
192217
}
193218

194-
func genData(t *testing.T, db database.Store, n int) []database.User {
219+
func genData(t *testing.T, db database.Store) []database.User {
195220
t.Helper()
196221
var users []database.User
197-
for i := 0; i < n; i++ {
198-
usr := dbgen.User(t, db, database.User{
199-
LoginType: database.LoginTypeOIDC,
200-
})
201-
_ = dbgen.UserLink(t, db, database.UserLink{
202-
UserID: usr.ID,
203-
LoginType: usr.LoginType,
204-
OAuthAccessToken: "access-" + usr.ID.String(),
205-
OAuthRefreshToken: "refresh-" + usr.ID.String(),
206-
})
207-
_ = dbgen.GitAuthLink(t, db, database.GitAuthLink{
208-
UserID: usr.ID,
209-
ProviderID: "fake",
210-
OAuthAccessToken: "access-" + usr.ID.String(),
211-
OAuthRefreshToken: "refresh-" + usr.ID.String(),
212-
})
213-
users = append(users, usr)
222+
// Make some users
223+
for _, status := range database.AllUserStatusValues() {
224+
for _, loginType := range database.AllLoginTypeValues() {
225+
for _, deleted := range []bool{false, true} {
226+
usr := dbgen.User(t, db, database.User{
227+
LoginType: loginType,
228+
Status: status,
229+
Deleted: deleted,
230+
})
231+
_ = dbgen.GitAuthLink(t, db, database.GitAuthLink{
232+
UserID: usr.ID,
233+
ProviderID: "fake",
234+
OAuthAccessToken: "access-" + usr.ID.String(),
235+
OAuthRefreshToken: "refresh-" + usr.ID.String(),
236+
})
237+
// Fun fact: our schema allows _all_ login types to have
238+
// a user_link. Even though I'm not sure how it could occur
239+
// in practice, making sure to test all combinations here.
240+
_ = dbgen.UserLink(t, db, database.UserLink{
241+
UserID: usr.ID,
242+
LoginType: usr.LoginType,
243+
OAuthAccessToken: "access-" + usr.ID.String(),
244+
OAuthRefreshToken: "refresh-" + usr.ID.String(),
245+
})
246+
users = append(users, usr)
247+
}
248+
}
214249
}
215250
return users
216251
}
217252

253+
func dumpUsers(t *testing.T, db *sql.DB) {
254+
t.Helper()
255+
rows, err := db.QueryContext(context.Background(), `SELECT
256+
u.id,
257+
u.login_type,
258+
u.status,
259+
u.deleted,
260+
ul.oauth_access_token_key_id AS uloatkid,
261+
ul.oauth_refresh_token_key_id AS ulortkid,
262+
gal.oauth_access_token_key_id AS galoatkid,
263+
gal.oauth_refresh_token_key_id AS galortkid
264+
FROM users u
265+
LEFT OUTER JOIN user_links ul ON u.id = ul.user_id
266+
LEFT OUTER JOIN git_auth_links gal ON u.id = gal.user_id
267+
ORDER BY u.created_at ASC;`)
268+
require.NoError(t, err)
269+
defer rows.Close()
270+
for rows.Next() {
271+
var (
272+
id string
273+
loginType string
274+
status string
275+
deleted bool
276+
UlOatKid sql.NullString
277+
UlOrtKid sql.NullString
278+
GalOatKid sql.NullString
279+
GalOrtKid sql.NullString
280+
)
281+
require.NoError(t, rows.Scan(
282+
&id,
283+
&loginType,
284+
&status,
285+
&deleted,
286+
&UlOatKid,
287+
&UlOrtKid,
288+
&GalOatKid,
289+
&GalOrtKid,
290+
))
291+
t.Logf("user: id:%s login_type:%-8s status:%-9s deleted:%-5t ul_kids{at:%-7s rt:%-7s} gal_kids{at:%-7s rt:%-7s}",
292+
id, loginType, status, deleted, UlOatKid.String, UlOrtKid.String, GalOatKid.String, GalOrtKid.String,
293+
)
294+
}
295+
}
296+
218297
func mustString(t *testing.T, n int) string {
219298
t.Helper()
220299
s, err := cryptorand.String(n)

0 commit comments

Comments
 (0)