opensource/coder
3
0
Fork 0
mirror of https://github.com/coder/coder.git synced 2026-09-24 15:04:27 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix(coderd): ensure that clearing invalid oauth refresh tokens works with dbcrypt (#15721)

Browse Source 
https://github.com/coder/coder/pull/15608 introduced a buggy behaviour
with dbcrypt enabled.
When clearing an oauth refresh token, we had been setting the value to
the empty string.
The database encryption package considers decrypting an empty string to
be an error, as an empty encrypted string value will still have a nonce
associated with it and thus not actually be empty when stored at rest.

Instead of 'deleting' the refresh token, 'update' it to be the empty
string.
This plays nicely with dbcrypt.

It also adds a 'utility test' in the dbcrypt package to help encrypt a
value. This was useful when manually fixing users affected by this bug
on our dogfood instance.
This commit is contained in:
Cian Johnston
2024-12-03 13:26:31 -06:00
committed by GitHub
parent ebfc133641
commit e744cde86f
13 changed files with 184 additions and 94 deletions
Download Patch File Download Diff File Expand all files Collapse all files
coderd/database/dbauthz/dbauthz.go
+7 -7
View File
@@ -3367,13 +3367,6 @@ func (q *querier) RegisterWorkspaceProxy(ctx context.Context, arg database.Regis
return updateWithReturn(q.log, q.auth, fetch, q.db.RegisterWorkspaceProxy)(ctx, arg)
}
func (q *querier) RemoveRefreshToken(ctx context.Context, arg database.RemoveRefreshTokenParams) error {
fetch := func(ctx context.Context, arg database.RemoveRefreshTokenParams) (database.ExternalAuthLink, error) {
return q.db.GetExternalAuthLink(ctx, database.GetExternalAuthLinkParams{UserID: arg.UserID, ProviderID: arg.ProviderID})
}
return fetchAndExec(q.log, q.auth, policy.ActionUpdatePersonal, fetch, q.db.RemoveRefreshToken)(ctx, arg)
}
func (q *querier) RemoveUserFromAllGroups(ctx context.Context, userID uuid.UUID) error {
// This is a system function to clear user groups in group sync.
if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceSystem); err != nil {
@@ -3472,6 +3465,13 @@ func (q *querier) UpdateExternalAuthLink(ctx context.Context, arg database.Updat
return fetchAndQuery(q.log, q.auth, policy.ActionUpdatePersonal, fetch, q.db.UpdateExternalAuthLink)(ctx, arg)
}
func (q *querier) UpdateExternalAuthLinkRefreshToken(ctx context.Context, arg database.UpdateExternalAuthLinkRefreshTokenParams) error {
fetch := func(ctx context.Context, arg database.UpdateExternalAuthLinkRefreshTokenParams) (database.ExternalAuthLink, error) {
return q.db.GetExternalAuthLink(ctx, database.GetExternalAuthLinkParams{UserID: arg.UserID, ProviderID: arg.ProviderID})
}
return fetchAndExec(q.log, q.auth, policy.ActionUpdatePersonal, fetch, q.db.UpdateExternalAuthLinkRefreshToken)(ctx, arg)
}
func (q *querier) UpdateGitSSHKey(ctx context.Context, arg database.UpdateGitSSHKeyParams) (database.GitSSHKey, error) {
fetch := func(ctx context.Context, arg database.UpdateGitSSHKeyParams) (database.GitSSHKey, error) {
return q.db.GetGitSSHKey(ctx, arg.UserID)
coderd/database/dbauthz/dbauthz_test.go
+7 -5
View File
@@ -1282,12 +1282,14 @@ func (s *MethodTestSuite) TestUser() {
UserID: u.ID,
}).Asserts(u, policy.ActionUpdatePersonal)
}))
s.Run("RemoveRefreshToken", s.Subtest(func(db database.Store, check *expects) {
s.Run("UpdateExternalAuthLinkRefreshToken", s.Subtest(func(db database.Store, check *expects) {
link := dbgen.ExternalAuthLink(s.T(), db, database.ExternalAuthLink{})
check.Args(database.RemoveRefreshTokenParams{
ProviderID: link.ProviderID,
UserID: link.UserID,
UpdatedAt: link.UpdatedAt,
check.Args(database.UpdateExternalAuthLinkRefreshTokenParams{
OAuthRefreshToken: "",
OAuthRefreshTokenKeyID: "",
ProviderID: link.ProviderID,
UserID: link.UserID,
UpdatedAt: link.UpdatedAt,
}).Asserts(rbac.ResourceUserObject(link.UserID), policy.ActionUpdatePersonal)
}))
s.Run("UpdateExternalAuthLink", s.Subtest(func(db database.Store, check *expects) {
coderd/database/dbmem/dbmem.go
+23 -23
View File
@@ -8607,29 +8607,6 @@ func (q *FakeQuerier) RegisterWorkspaceProxy(_ context.Context, arg database.Reg
return database.WorkspaceProxy{}, sql.ErrNoRows
}
func (q *FakeQuerier) RemoveRefreshToken(_ context.Context, arg database.RemoveRefreshTokenParams) error {
if err := validateDatabaseType(arg); err != nil {
return err
}
q.mutex.Lock()
defer q.mutex.Unlock()
for index, gitAuthLink := range q.externalAuthLinks {
if gitAuthLink.ProviderID != arg.ProviderID {
continue
}
if gitAuthLink.UserID != arg.UserID {
continue
}
gitAuthLink.UpdatedAt = arg.UpdatedAt
gitAuthLink.OAuthRefreshToken = ""
q.externalAuthLinks[index] = gitAuthLink
return nil
}
return sql.ErrNoRows
}
func (q *FakeQuerier) RemoveUserFromAllGroups(_ context.Context, userID uuid.UUID) error {
q.mutex.Lock()
defer q.mutex.Unlock()
@@ -8849,6 +8826,29 @@ func (q *FakeQuerier) UpdateExternalAuthLink(_ context.Context, arg database.Upd
return database.ExternalAuthLink{}, sql.ErrNoRows
}
func (q *FakeQuerier) UpdateExternalAuthLinkRefreshToken(_ context.Context, arg database.UpdateExternalAuthLinkRefreshTokenParams) error {
if err := validateDatabaseType(arg); err != nil {
return err
}
q.mutex.Lock()
defer q.mutex.Unlock()
for index, gitAuthLink := range q.externalAuthLinks {
if gitAuthLink.ProviderID != arg.ProviderID {
continue
}
if gitAuthLink.UserID != arg.UserID {
continue
}
gitAuthLink.UpdatedAt = arg.UpdatedAt
gitAuthLink.OAuthRefreshToken = arg.OAuthRefreshToken
q.externalAuthLinks[index] = gitAuthLink
return nil
}
return sql.ErrNoRows
}
func (q *FakeQuerier) UpdateGitSSHKey(_ context.Context, arg database.UpdateGitSSHKeyParams) (database.GitSSHKey, error) {
if err := validateDatabaseType(arg); err != nil {
return database.GitSSHKey{}, err
coderd/database/dbmetrics/querymetrics.go
+7 -7
View File
@@ -2107,13 +2107,6 @@ func (m queryMetricsStore) RegisterWorkspaceProxy(ctx context.Context, arg datab
return proxy, err
}
func (m queryMetricsStore) RemoveRefreshToken(ctx context.Context, arg database.RemoveRefreshTokenParams) error {
start := time.Now()
r0 := m.s.RemoveRefreshToken(ctx, arg)
m.queryLatencies.WithLabelValues("RemoveRefreshToken").Observe(time.Since(start).Seconds())
return r0
}
func (m queryMetricsStore) RemoveUserFromAllGroups(ctx context.Context, userID uuid.UUID) error {
start := time.Now()
r0 := m.s.RemoveUserFromAllGroups(ctx, userID)
@@ -2184,6 +2177,13 @@ func (m queryMetricsStore) UpdateExternalAuthLink(ctx context.Context, arg datab
return link, err
}
func (m queryMetricsStore) UpdateExternalAuthLinkRefreshToken(ctx context.Context, arg database.UpdateExternalAuthLinkRefreshTokenParams) error {
start := time.Now()
r0 := m.s.UpdateExternalAuthLinkRefreshToken(ctx, arg)
m.queryLatencies.WithLabelValues("UpdateExternalAuthLinkRefreshToken").Observe(time.Since(start).Seconds())
return r0
}
func (m queryMetricsStore) UpdateGitSSHKey(ctx context.Context, arg database.UpdateGitSSHKeyParams) (database.GitSSHKey, error) {
start := time.Now()
key, err := m.s.UpdateGitSSHKey(ctx, arg)
coderd/database/dbmock/dbmock.go
+14 -14
View File
@@ -4492,20 +4492,6 @@ func (mr *MockStoreMockRecorder) RegisterWorkspaceProxy(arg0, arg1 any) *gomock.
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RegisterWorkspaceProxy", reflect.TypeOf((*MockStore)(nil).RegisterWorkspaceProxy), arg0, arg1)
}
// RemoveRefreshToken mocks base method.
func (m *MockStore) RemoveRefreshToken(arg0 context.Context, arg1 database.RemoveRefreshTokenParams) error {
m.ctrl.T.Helper()
ret := m.ctrl.Call(m, "RemoveRefreshToken", arg0, arg1)
ret0, _ := ret[0].(error)
return ret0
}
// RemoveRefreshToken indicates an expected call of RemoveRefreshToken.
func (mr *MockStoreMockRecorder) RemoveRefreshToken(arg0, arg1 any) *gomock.Call {
mr.mock.ctrl.T.Helper()
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RemoveRefreshToken", reflect.TypeOf((*MockStore)(nil).RemoveRefreshToken), arg0, arg1)
}
// RemoveUserFromAllGroups mocks base method.
func (m *MockStore) RemoveUserFromAllGroups(arg0 context.Context, arg1 uuid.UUID) error {
m.ctrl.T.Helper()
@@ -4651,6 +4637,20 @@ func (mr *MockStoreMockRecorder) UpdateExternalAuthLink(arg0, arg1 any) *gomock.
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateExternalAuthLink", reflect.TypeOf((*MockStore)(nil).UpdateExternalAuthLink), arg0, arg1)
}
// UpdateExternalAuthLinkRefreshToken mocks base method.
func (m *MockStore) UpdateExternalAuthLinkRefreshToken(arg0 context.Context, arg1 database.UpdateExternalAuthLinkRefreshTokenParams) error {
m.ctrl.T.Helper()
ret := m.ctrl.Call(m, "UpdateExternalAuthLinkRefreshToken", arg0, arg1)
ret0, _ := ret[0].(error)
return ret0
}
// UpdateExternalAuthLinkRefreshToken indicates an expected call of UpdateExternalAuthLinkRefreshToken.
func (mr *MockStoreMockRecorder) UpdateExternalAuthLinkRefreshToken(arg0, arg1 any) *gomock.Call {
mr.mock.ctrl.T.Helper()
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateExternalAuthLinkRefreshToken", reflect.TypeOf((*MockStore)(nil).UpdateExternalAuthLinkRefreshToken), arg0, arg1)
}
// UpdateGitSSHKey mocks base method.
func (m *MockStore) UpdateGitSSHKey(arg0 context.Context, arg1 database.UpdateGitSSHKeyParams) (database.GitSSHKey, error) {
m.ctrl.T.Helper()
coderd/database/querier.go
+1 -4
View File
@@ -426,10 +426,6 @@ type sqlcQuerier interface {
OrganizationMembers(ctx context.Context, arg OrganizationMembersParams) ([]OrganizationMembersRow, error)
ReduceWorkspaceAgentShareLevelToAuthenticatedByTemplate(ctx context.Context, templateID uuid.UUID) error
RegisterWorkspaceProxy(ctx context.Context, arg RegisterWorkspaceProxyParams) (WorkspaceProxy, error)
// Removing the refresh token disables the refresh behavior for a given
// auth token. If a refresh token is marked invalid, it is better to remove it
// then continually attempt to refresh the token.
RemoveRefreshToken(ctx context.Context, arg RemoveRefreshTokenParams) error
RemoveUserFromAllGroups(ctx context.Context, userID uuid.UUID) error
RemoveUserFromGroups(ctx context.Context, arg RemoveUserFromGroupsParams) ([]uuid.UUID, error)
RevokeDBCryptKey(ctx context.Context, activeKeyDigest string) error
@@ -445,6 +441,7 @@ type sqlcQuerier interface {
UpdateCryptoKeyDeletesAt(ctx context.Context, arg UpdateCryptoKeyDeletesAtParams) (CryptoKey, error)
UpdateCustomRole(ctx context.Context, arg UpdateCustomRoleParams) (CustomRole, error)
UpdateExternalAuthLink(ctx context.Context, arg UpdateExternalAuthLinkParams) (ExternalAuthLink, error)
UpdateExternalAuthLinkRefreshToken(ctx context.Context, arg UpdateExternalAuthLinkRefreshTokenParams) error
UpdateGitSSHKey(ctx context.Context, arg UpdateGitSSHKeyParams) (GitSSHKey, error)
UpdateGroupByID(ctx context.Context, arg UpdateGroupByIDParams) (Group, error)
UpdateInactiveUsersToDormant(ctx context.Context, arg UpdateInactiveUsersToDormantParams) ([]UpdateInactiveUsersToDormantRow, error)
coderd/database/queries.sql.go
+34 -23
View File
@@ -1194,29 +1194,6 @@ func (q *sqlQuerier) InsertExternalAuthLink(ctx context.Context, arg InsertExter
return i, err
}
const removeRefreshToken = `-- name: RemoveRefreshToken :exec
UPDATE
external_auth_links
SET
oauth_refresh_token = '',
updated_at = $1
WHERE provider_id = $2 AND user_id = $3
`
type RemoveRefreshTokenParams struct {
UpdatedAt time.Time `db:"updated_at" json:"updated_at"`
ProviderID string `db:"provider_id" json:"provider_id"`
UserID uuid.UUID `db:"user_id" json:"user_id"`
}
// Removing the refresh token disables the refresh behavior for a given
// auth token. If a refresh token is marked invalid, it is better to remove it
// then continually attempt to refresh the token.
func (q *sqlQuerier) RemoveRefreshToken(ctx context.Context, arg RemoveRefreshTokenParams) error {
_, err := q.db.ExecContext(ctx, removeRefreshToken, arg.UpdatedAt, arg.ProviderID, arg.UserID)
return err
}
const updateExternalAuthLink = `-- name: UpdateExternalAuthLink :one
UPDATE external_auth_links SET
updated_at = $3,
@@ -1269,6 +1246,40 @@ func (q *sqlQuerier) UpdateExternalAuthLink(ctx context.Context, arg UpdateExter
return i, err
}
const updateExternalAuthLinkRefreshToken = `-- name: UpdateExternalAuthLinkRefreshToken :exec
UPDATE
external_auth_links
SET
oauth_refresh_token = $1,
updated_at = $2
WHERE
provider_id = $3
AND
user_id = $4
AND
-- Required for sqlc to generate a parameter for the oauth_refresh_token_key_id
$5 :: text = $5 :: text
`
type UpdateExternalAuthLinkRefreshTokenParams struct {
OAuthRefreshToken string `db:"oauth_refresh_token" json:"oauth_refresh_token"`
UpdatedAt time.Time `db:"updated_at" json:"updated_at"`
ProviderID string `db:"provider_id" json:"provider_id"`
UserID uuid.UUID `db:"user_id" json:"user_id"`
OAuthRefreshTokenKeyID string `db:"oauth_refresh_token_key_id" json:"oauth_refresh_token_key_id"`
}
func (q *sqlQuerier) UpdateExternalAuthLinkRefreshToken(ctx context.Context, arg UpdateExternalAuthLinkRefreshTokenParams) error {
_, err := q.db.ExecContext(ctx, updateExternalAuthLinkRefreshToken,
arg.OAuthRefreshToken,
arg.UpdatedAt,
arg.ProviderID,
arg.UserID,
arg.OAuthRefreshTokenKeyID,
)
return err
}
const getFileByHashAndCreator = `-- name: GetFileByHashAndCreator :one
SELECT
hash, created_at, created_by, mimetype, data, id
coderd/database/queries/externalauth.sql
+9 -6
View File
@@ -43,13 +43,16 @@ UPDATE external_auth_links SET
oauth_extra = $9
WHERE provider_id = $1 AND user_id = $2 RETURNING *;
-- name: RemoveRefreshToken :exec
-- Removing the refresh token disables the refresh behavior for a given
-- auth token. If a refresh token is marked invalid, it is better to remove it
-- then continually attempt to refresh the token.
-- name: UpdateExternalAuthLinkRefreshToken :exec
UPDATE
external_auth_links
SET
oauth_refresh_token = '',
oauth_refresh_token = @oauth_refresh_token,
updated_at = @updated_at
WHERE provider_id = @provider_id AND user_id = @user_id;
WHERE
provider_id = @provider_id
AND
user_id = @user_id
AND
-- Required for sqlc to generate a parameter for the oauth_refresh_token_key_id
@oauth_refresh_token_key_id :: text = @oauth_refresh_token_key_id :: text;