Skip to content

fix(enterprise/dbcrypt): do not skip deleted users when encrypting or deleting #9694

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Sep 15, 2023
Merged

fix(enterprise/dbcrypt): do not skip deleted users when encrypting or deleting #9694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
355b2c1
add some more logging to test
johnstcn Sep 15, 2023
39aa29e
validate working with multiple user statuses
johnstcn Sep 15, 2023
3950c09
fmt query
johnstcn Sep 15, 2023
49b9d90
create deleted users
johnstcn Sep 15, 2023
5c1b687
robustificate data generation
johnstcn Sep 15, 2023
9bb1a5d
only dump on test failure
johnstcn Sep 15, 2023
7da5972
more logging
johnstcn Sep 15, 2023
2f63e43
do not skip deleted users when encrypting or decrypting
johnstcn Sep 15, 2023
11f85bf
make AllUserIDs a fully-fledged query citizen
johnstcn Sep 15, 2023
0c11d6c
fixup! make AllUserIDs a fully-fledged query citizen
johnstcn Sep 15, 2023
0f164a3
fixup! fixup! make AllUserIDs a fully-fledged query citizen
johnstcn Sep 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions coderd/database/dbauthz/dbauthz.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -664,6 +664,15 @@ func (q *querier) ActivityBumpWorkspace(ctx context.Context, arg uuid.UUID) erro
return update(q.log, q.auth, fetch, q.db.ActivityBumpWorkspace)(ctx, arg)
}

func (q *querier) AllUserIDs(ctx context.Context) ([]uuid.UUID, error) {
// Although this technically only reads users, only system-related functions should be
// allowed to call this.
if err := q.authorizeContext(ctx, rbac.ActionRead, rbac.ResourceSystem); err != nil {
return nil, err
}
return q.db.AllUserIDs(ctx)
}

func (q *querier) CleanTailnetCoordinators(ctx context.Context) error {
if err := q.authorizeContext(ctx, rbac.ActionDelete, rbac.ResourceTailnetCoordinator); err != nil {
return err
Expand Down
10 changes: 10 additions & 0 deletions coderd/database/dbfake/dbfake.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -812,6 +812,16 @@ func (q *FakeQuerier) ActivityBumpWorkspace(ctx context.Context, workspaceID uui
return sql.ErrNoRows
}

func (q *FakeQuerier) AllUserIDs(_ context.Context) ([]uuid.UUID, error) {
q.mutex.RLock()
defer q.mutex.RUnlock()
userIDs := make([]uuid.UUID, 0, len(q.users))
for idx := range q.users {
userIDs[idx] = q.users[idx].ID
}
return userIDs, nil
}

func (*FakeQuerier) CleanTailnetCoordinators(_ context.Context) error {
return ErrUnimplemented
}
Expand Down
10 changes: 9 additions & 1 deletion coderd/database/dbgen/dbgen.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ func User(t testing.TB, db database.Store, orig database.User) database.User {

user, err = db.UpdateUserStatus(genCtx, database.UpdateUserStatusParams{
ID: user.ID,
Status: database.UserStatusActive,
Status: takeFirst(orig.Status, database.UserStatusActive),
UpdatedAt: dbtime.Now(),
})
require.NoError(t, err, "insert user")
Expand All @@ -240,6 +240,14 @@ func User(t testing.TB, db database.Store, orig database.User) database.User {
})
require.NoError(t, err, "user last seen")
}

if orig.Deleted {
err = db.UpdateUserDeletedByID(genCtx, database.UpdateUserDeletedByIDParams{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI There is also dormant status, hopefully it does not change anything.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All combinations of login_type, status, and deleted should be covered by this test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fairly sure user dormancy and deletion are completely orthogonal i.e. both can be set independently.

ID: user.ID,
Deleted: orig.Deleted,
})
require.NoError(t, err, "set user as deleted")
}
return user
}

Expand Down
7 changes: 7 additions & 0 deletions coderd/database/dbmetrics/dbmetrics.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 15 additions & 0 deletions coderd/database/dbmock/dbmock.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions coderd/database/querier.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

28 changes: 28 additions & 0 deletions coderd/database/queries.sql.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions coderd/database/queries/dbcrypt.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,4 @@ AND
INSERT INTO dbcrypt_keys
(number, active_key_digest, created_at, test)
VALUES (@number::int, @active_key_digest::text, CURRENT_TIMESTAMP, @test::text);

5 changes: 5 additions & 0 deletions coderd/database/queries/users.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,3 +262,8 @@ WHERE
last_seen_at < @last_seen_after :: timestamp
AND status = 'active'::user_status
RETURNING id, email, last_seen_at;

-- AllUserIDs returns all UserIDs regardless of user status or deletion.
-- name: AllUserIDs :many
SELECT DISTINCT id FROM USERS;

133 changes: 106 additions & 27 deletions enterprise/cli/server_dbcrypt_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ import (
"github.com/coder/coder/v2/pty/ptytest"
)

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

t.Cleanup(func() {
if t.Failed() {
t.Logf("Dumping data due to failed test. I hope you find what you're looking for!")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, this should be a general pattern in coder/coder. Great idea!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. Actually I'm wondering if it would make sense to have/write a full psql dump as well (or instead). It'd be great if the test author didn't need to consider all failure scenarios and the information would always be there in full (and repeatable by importing dump).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add that as a follow-up PR! Maybe something like dbtestutil.WithDumpOnFailure(t)?

dumpUsers(t, sqlDB)
}
})

// Populate the database with some unencrypted data.
users := genData(t, db, 10)
t.Logf("Generating unencrypted data")
users := genData(t, db)

// Setup an initial cipher
// Setup an initial cipher A
keyA := mustString(t, 32)
cipherA, err := dbcrypt.NewCiphers([]byte(keyA))
require.NoError(t, err)

// Create an encrypted database
cryptdb, err := dbcrypt.New(ctx, db, cipherA...)
require.NoError(t, err)

// Populate the database with some encrypted data using cipher A.
t.Logf("Generating data encrypted with cipher A")
newUsers := genData(t, cryptdb)

// Validate that newly created users were encrypted with cipher A
for _, usr := range newUsers {
requireEncryptedWithCipher(ctx, t, db, cipherA[0], usr.ID)
}
users = append(users, newUsers...)

// Encrypt all the data with the initial cipher.
t.Logf("Encrypting all data with cipher A")
inv, _ := newCLI(t, "server", "dbcrypt", "rotate",
"--postgres-url", connectionURL,
"--new-key", base64.StdEncoding.EncodeToString([]byte(keyA)),
Expand All @@ -65,18 +91,12 @@ func TestServerDBCrypt(t *testing.T) {
requireEncryptedWithCipher(ctx, t, db, cipherA[0], usr.ID)
}

// Create an encrypted database
cryptdb, err := dbcrypt.New(ctx, db, cipherA...)
require.NoError(t, err)

// Populate the database with some encrypted data using cipher A.
users = append(users, genData(t, cryptdb, 10)...)

// Re-encrypt all existing data with a new cipher.
keyB := mustString(t, 32)
cipherBA, err := dbcrypt.NewCiphers([]byte(keyB), []byte(keyA))
require.NoError(t, err)

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

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

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

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

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

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

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

func genData(t *testing.T, db database.Store, n int) []database.User {
func genData(t *testing.T, db database.Store) []database.User {
t.Helper()
var users []database.User
for i := 0; i < n; i++ {
usr := dbgen.User(t, db, database.User{
LoginType: database.LoginTypeOIDC,
})
_ = dbgen.UserLink(t, db, database.UserLink{
UserID: usr.ID,
LoginType: usr.LoginType,
OAuthAccessToken: "access-" + usr.ID.String(),
OAuthRefreshToken: "refresh-" + usr.ID.String(),
})
_ = dbgen.GitAuthLink(t, db, database.GitAuthLink{
UserID: usr.ID,
ProviderID: "fake",
OAuthAccessToken: "access-" + usr.ID.String(),
OAuthRefreshToken: "refresh-" + usr.ID.String(),
})
users = append(users, usr)
// Make some users
for _, status := range database.AllUserStatusValues() {
for _, loginType := range database.AllLoginTypeValues() {
for _, deleted := range []bool{false, true} {
usr := dbgen.User(t, db, database.User{
LoginType: loginType,
Status: status,
Deleted: deleted,
})
_ = dbgen.GitAuthLink(t, db, database.GitAuthLink{
UserID: usr.ID,
ProviderID: "fake",
OAuthAccessToken: "access-" + usr.ID.String(),
OAuthRefreshToken: "refresh-" + usr.ID.String(),
})
// Fun fact: our schema allows _all_ login types to have
// a user_link. Even though I'm not sure how it could occur
// in practice, making sure to test all combinations here.
_ = dbgen.UserLink(t, db, database.UserLink{
UserID: usr.ID,
LoginType: usr.LoginType,
OAuthAccessToken: "access-" + usr.ID.String(),
OAuthRefreshToken: "refresh-" + usr.ID.String(),
})
users = append(users, usr)
}
}
}
return users
}

func dumpUsers(t *testing.T, db *sql.DB) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we limit the OS test env for this test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there were the possibility of either leaking sensitive information or performing destructive database changes, I would say yes. I don't believe either of those are the case here, though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any case, I'll be removing this in a follow-up PR and replacing it with a more general "pg_dump on test failure".

t.Helper()
rows, err := db.QueryContext(context.Background(), `SELECT
u.id,
u.login_type,
u.status,
u.deleted,
ul.oauth_access_token_key_id AS uloatkid,
ul.oauth_refresh_token_key_id AS ulortkid,
gal.oauth_access_token_key_id AS galoatkid,
gal.oauth_refresh_token_key_id AS galortkid
FROM users u
LEFT OUTER JOIN user_links ul ON u.id = ul.user_id
LEFT OUTER JOIN git_auth_links gal ON u.id = gal.user_id
ORDER BY u.created_at ASC;`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should add a test that this function works as expected since it's static and will only be run on failure, meaning it will most likely be outdated by the time it's needed. 😆

Perhaps this could be motivation for a testqueries package. Similar to what I did here: #9519 to generate a new (slim) package that can be wrapped around sql.DB.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd definitely be in favour of this; I'm fairly sure there are at least a couple of queries that only ever get run in unit tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should add a test that this function works as expected since it's static and will only be run on failure, meaning it will most likely be outdated by the time it's needed. 😆

Golden files?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think verifying a few fields in the test is enough. Otherwise you need to stabilize the generated IDs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think instead of making that query more complex I'll go ahead and add a follow-up PR to dump the entire test database on failure (optional, opt-in). Less work, less fuss, more benefit.

require.NoError(t, err)
defer rows.Close()
for rows.Next() {
var (
id string
loginType string
status string
deleted bool
UlOatKid sql.NullString
UlOrtKid sql.NullString
GalOatKid sql.NullString
GalOrtKid sql.NullString
)
require.NoError(t, rows.Scan(
&id,
&loginType,
&status,
&deleted,
&UlOatKid,
&UlOrtKid,
&GalOatKid,
&GalOrtKid,
))
t.Logf("user: id:%s login_type:%-8s status:%-9s deleted:%-5t ul_kids{at:%-7s rt:%-7s} gal_kids{at:%-7s rt:%-7s}",
id, loginType, status, deleted, UlOatKid.String, UlOrtKid.String, GalOatKid.String, GalOrtKid.String,
)
}
}

func mustString(t *testing.T, n int) string {
t.Helper()
s, err := cryptorand.String(n)
Expand Down
Loading