Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commite744cde

Browse files
authored
fix(coderd): ensure that clearing invalid oauth refresh tokens works with dbcrypt (#15721)
#15608 introduced a buggy behaviourwith dbcrypt enabled.When clearing an oauth refresh token, we had been setting the value tothe empty string.The database encryption package considers decrypting an empty string tobe an error, as an empty encrypted string value will still have a nonceassociated with it and thus not actually be empty when stored at rest.Instead of 'deleting' the refresh token, 'update' it to be the emptystring.This plays nicely with dbcrypt.It also adds a 'utility test' in the dbcrypt package to help encrypt avalue. This was useful when manually fixing users affected by this bugon our dogfood instance.
1 parentebfc133 commite744cde

File tree

13 files changed

+184
-94
lines changed

13 files changed

+184
-94
lines changed

‎coderd/database/dbauthz/dbauthz.go

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3367,13 +3367,6 @@ func (q *querier) RegisterWorkspaceProxy(ctx context.Context, arg database.Regis
33673367
returnupdateWithReturn(q.log,q.auth,fetch,q.db.RegisterWorkspaceProxy)(ctx,arg)
33683368
}
33693369

3370-
func (q*querier)RemoveRefreshToken(ctx context.Context,arg database.RemoveRefreshTokenParams)error {
3371-
fetch:=func(ctx context.Context,arg database.RemoveRefreshTokenParams) (database.ExternalAuthLink,error) {
3372-
returnq.db.GetExternalAuthLink(ctx, database.GetExternalAuthLinkParams{UserID:arg.UserID,ProviderID:arg.ProviderID})
3373-
}
3374-
returnfetchAndExec(q.log,q.auth,policy.ActionUpdatePersonal,fetch,q.db.RemoveRefreshToken)(ctx,arg)
3375-
}
3376-
33773370
func (q*querier)RemoveUserFromAllGroups(ctx context.Context,userID uuid.UUID)error {
33783371
// This is a system function to clear user groups in group sync.
33793372
iferr:=q.authorizeContext(ctx,policy.ActionUpdate,rbac.ResourceSystem);err!=nil {
@@ -3472,6 +3465,13 @@ func (q *querier) UpdateExternalAuthLink(ctx context.Context, arg database.Updat
34723465
returnfetchAndQuery(q.log,q.auth,policy.ActionUpdatePersonal,fetch,q.db.UpdateExternalAuthLink)(ctx,arg)
34733466
}
34743467

3468+
func (q*querier)UpdateExternalAuthLinkRefreshToken(ctx context.Context,arg database.UpdateExternalAuthLinkRefreshTokenParams)error {
3469+
fetch:=func(ctx context.Context,arg database.UpdateExternalAuthLinkRefreshTokenParams) (database.ExternalAuthLink,error) {
3470+
returnq.db.GetExternalAuthLink(ctx, database.GetExternalAuthLinkParams{UserID:arg.UserID,ProviderID:arg.ProviderID})
3471+
}
3472+
returnfetchAndExec(q.log,q.auth,policy.ActionUpdatePersonal,fetch,q.db.UpdateExternalAuthLinkRefreshToken)(ctx,arg)
3473+
}
3474+
34753475
func (q*querier)UpdateGitSSHKey(ctx context.Context,arg database.UpdateGitSSHKeyParams) (database.GitSSHKey,error) {
34763476
fetch:=func(ctx context.Context,arg database.UpdateGitSSHKeyParams) (database.GitSSHKey,error) {
34773477
returnq.db.GetGitSSHKey(ctx,arg.UserID)

‎coderd/database/dbauthz/dbauthz_test.go

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1282,12 +1282,14 @@ func (s *MethodTestSuite) TestUser() {
12821282
UserID:u.ID,
12831283
}).Asserts(u,policy.ActionUpdatePersonal)
12841284
}))
1285-
s.Run("RemoveRefreshToken",s.Subtest(func(db database.Store,check*expects) {
1285+
s.Run("UpdateExternalAuthLinkRefreshToken",s.Subtest(func(db database.Store,check*expects) {
12861286
link:=dbgen.ExternalAuthLink(s.T(),db, database.ExternalAuthLink{})
1287-
check.Args(database.RemoveRefreshTokenParams{
1288-
ProviderID:link.ProviderID,
1289-
UserID:link.UserID,
1290-
UpdatedAt:link.UpdatedAt,
1287+
check.Args(database.UpdateExternalAuthLinkRefreshTokenParams{
1288+
OAuthRefreshToken:"",
1289+
OAuthRefreshTokenKeyID:"",
1290+
ProviderID:link.ProviderID,
1291+
UserID:link.UserID,
1292+
UpdatedAt:link.UpdatedAt,
12911293
}).Asserts(rbac.ResourceUserObject(link.UserID),policy.ActionUpdatePersonal)
12921294
}))
12931295
s.Run("UpdateExternalAuthLink",s.Subtest(func(db database.Store,check*expects) {

‎coderd/database/dbmem/dbmem.go

Lines changed: 23 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -8607,29 +8607,6 @@ func (q *FakeQuerier) RegisterWorkspaceProxy(_ context.Context, arg database.Reg
86078607
return database.WorkspaceProxy{},sql.ErrNoRows
86088608
}
86098609

8610-
func (q*FakeQuerier)RemoveRefreshToken(_ context.Context,arg database.RemoveRefreshTokenParams)error {
8611-
iferr:=validateDatabaseType(arg);err!=nil {
8612-
returnerr
8613-
}
8614-
8615-
q.mutex.Lock()
8616-
deferq.mutex.Unlock()
8617-
forindex,gitAuthLink:=rangeq.externalAuthLinks {
8618-
ifgitAuthLink.ProviderID!=arg.ProviderID {
8619-
continue
8620-
}
8621-
ifgitAuthLink.UserID!=arg.UserID {
8622-
continue
8623-
}
8624-
gitAuthLink.UpdatedAt=arg.UpdatedAt
8625-
gitAuthLink.OAuthRefreshToken=""
8626-
q.externalAuthLinks[index]=gitAuthLink
8627-
8628-
returnnil
8629-
}
8630-
returnsql.ErrNoRows
8631-
}
8632-
86338610
func (q*FakeQuerier)RemoveUserFromAllGroups(_ context.Context,userID uuid.UUID)error {
86348611
q.mutex.Lock()
86358612
deferq.mutex.Unlock()
@@ -8849,6 +8826,29 @@ func (q *FakeQuerier) UpdateExternalAuthLink(_ context.Context, arg database.Upd
88498826
return database.ExternalAuthLink{},sql.ErrNoRows
88508827
}
88518828

8829+
func (q*FakeQuerier)UpdateExternalAuthLinkRefreshToken(_ context.Context,arg database.UpdateExternalAuthLinkRefreshTokenParams)error {
8830+
iferr:=validateDatabaseType(arg);err!=nil {
8831+
returnerr
8832+
}
8833+
8834+
q.mutex.Lock()
8835+
deferq.mutex.Unlock()
8836+
forindex,gitAuthLink:=rangeq.externalAuthLinks {
8837+
ifgitAuthLink.ProviderID!=arg.ProviderID {
8838+
continue
8839+
}
8840+
ifgitAuthLink.UserID!=arg.UserID {
8841+
continue
8842+
}
8843+
gitAuthLink.UpdatedAt=arg.UpdatedAt
8844+
gitAuthLink.OAuthRefreshToken=arg.OAuthRefreshToken
8845+
q.externalAuthLinks[index]=gitAuthLink
8846+
8847+
returnnil
8848+
}
8849+
returnsql.ErrNoRows
8850+
}
8851+
88528852
func (q*FakeQuerier)UpdateGitSSHKey(_ context.Context,arg database.UpdateGitSSHKeyParams) (database.GitSSHKey,error) {
88538853
iferr:=validateDatabaseType(arg);err!=nil {
88548854
return database.GitSSHKey{},err

‎coderd/database/dbmetrics/querymetrics.go

Lines changed: 7 additions & 7 deletions
Some generated files are not rendered by default. Learn more aboutcustomizing how changed files appear on GitHub.

‎coderd/database/dbmock/dbmock.go

Lines changed: 14 additions & 14 deletions
Some generated files are not rendered by default. Learn more aboutcustomizing how changed files appear on GitHub.

‎coderd/database/querier.go

Lines changed: 1 addition & 4 deletions
Some generated files are not rendered by default. Learn more aboutcustomizing how changed files appear on GitHub.

‎coderd/database/queries.sql.go

Lines changed: 34 additions & 23 deletions
Some generated files are not rendered by default. Learn more aboutcustomizing how changed files appear on GitHub.

‎coderd/database/queries/externalauth.sql

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -43,13 +43,16 @@ UPDATE external_auth_links SET
4343
oauth_extra= $9
4444
WHERE provider_id= $1AND user_id= $2 RETURNING*;
4545

46-
-- name: RemoveRefreshToken :exec
47-
-- Removing the refresh token disables the refresh behavior for a given
48-
-- auth token. If a refresh token is marked invalid, it is better to remove it
49-
-- then continually attempt to refresh the token.
46+
-- name: UpdateExternalAuthLinkRefreshToken :exec
5047
UPDATE
5148
external_auth_links
5249
SET
53-
oauth_refresh_token='',
50+
oauth_refresh_token=@oauth_refresh_token,
5451
updated_at= @updated_at
55-
WHERE provider_id= @provider_idAND user_id= @user_id;
52+
WHERE
53+
provider_id= @provider_id
54+
AND
55+
user_id= @user_id
56+
AND
57+
-- Required for sqlc to generate a parameter for the oauth_refresh_token_key_id
58+
@oauth_refresh_token_key_id ::text= @oauth_refresh_token_key_id ::text;

‎coderd/externalauth/externalauth.go

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -143,10 +143,12 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
143143
// get rid of it. Keeping it around will cause additional refresh
144144
// attempts that will fail and cost us api rate limits.
145145
ifisFailedRefresh(existingToken,err) {
146-
dbExecErr:=db.RemoveRefreshToken(ctx, database.RemoveRefreshTokenParams{
147-
UpdatedAt:dbtime.Now(),
148-
ProviderID:externalAuthLink.ProviderID,
149-
UserID:externalAuthLink.UserID,
146+
dbExecErr:=db.UpdateExternalAuthLinkRefreshToken(ctx, database.UpdateExternalAuthLinkRefreshTokenParams{
147+
OAuthRefreshToken:"",// It is better to clear the refresh token than to keep retrying.
148+
OAuthRefreshTokenKeyID:externalAuthLink.OAuthRefreshTokenKeyID.String,
149+
UpdatedAt:dbtime.Now(),
150+
ProviderID:externalAuthLink.ProviderID,
151+
UserID:externalAuthLink.UserID,
150152
})
151153
ifdbExecErr!=nil {
152154
// This error should be rare.

‎coderd/externalauth/externalauth_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,7 @@ func TestRefreshToken(t *testing.T) {
190190

191191
// Try again with a bad refresh token error
192192
// Expect DB call to remove the refresh token
193-
mDB.EXPECT().RemoveRefreshToken(gomock.Any(),gomock.Any()).Return(nil).Times(1)
193+
mDB.EXPECT().UpdateExternalAuthLinkRefreshToken(gomock.Any(),gomock.Any()).Return(nil).Times(1)
194194
refreshErr=&oauth2.RetrieveError{// github error
195195
Response:&http.Response{
196196
StatusCode:http.StatusOK,

‎enterprise/dbcrypt/cipher_internal_test.go

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@ package dbcrypt
33
import (
44
"bytes"
55
"encoding/base64"
6+
"os"
7+
"strings"
68
"testing"
79

810
"github.com/stretchr/testify/require"
@@ -89,3 +91,35 @@ func TestCiphersBackwardCompatibility(t *testing.T) {
8991
require.NoError(t,err,"decryption should succeed")
9092
require.Equal(t,msg,string(decrypted),"decrypted message should match original message")
9193
}
94+
95+
// If you're looking here, you're probably in trouble.
96+
// Here's what you need to do:
97+
// 1. Get the current CODER_EXTERNAL_TOKEN_ENCRYPTION_KEYS environment variable.
98+
// 2. Run the following command:
99+
// ENCRYPT_ME="<value to encrypt>" CODER_EXTERNAL_TOKEN_ENCRYPTION_KEYS="<secret keys here>" go test -v -count=1 ./enterprise/dbcrypt -test.run='^TestHelpMeEncryptSomeValue$'
100+
// 3. Copy the value from the test output and do what you need with it.
101+
funcTestHelpMeEncryptSomeValue(t*testing.T) {
102+
t.Parallel()
103+
t.Skip("this only exists if you need to encrypt a value with dbcrypt, it does not actually test anything")
104+
105+
valueToEncrypt:=os.Getenv("ENCRYPT_ME")
106+
t.Logf("valueToEncrypt: %q",valueToEncrypt)
107+
keys:=os.Getenv("CODER_EXTERNAL_TOKEN_ENCRYPTION_KEYS")
108+
require.NotEmpty(t,keys,"Set the CODER_EXTERNAL_TOKEN_ENCRYPTION_KEYS environment variable to use this")
109+
110+
base64Keys:=strings.Split(keys,",")
111+
activeKey:=base64Keys[0]
112+
113+
decodedKey,err:=base64.StdEncoding.DecodeString(activeKey)
114+
require.NoError(t,err,"the active key should be valid base64")
115+
116+
cipher,err:=cipherAES256(decodedKey)
117+
require.NoError(t,err)
118+
119+
t.Logf("cipher digest: %+v",cipher.HexDigest())
120+
121+
encryptedEmptyString,err:=cipher.Encrypt([]byte(valueToEncrypt))
122+
require.NoError(t,err)
123+
124+
t.Logf("encrypted and base64-encoded: %q",base64.StdEncoding.EncodeToString(encryptedEmptyString))
125+
}

‎enterprise/dbcrypt/dbcrypt.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,21 @@ func (db *dbCrypt) UpdateExternalAuthLink(ctx context.Context, params database.U
261261
returnlink,nil
262262
}
263263

264+
func (db*dbCrypt)UpdateExternalAuthLinkRefreshToken(ctx context.Context,params database.UpdateExternalAuthLinkRefreshTokenParams)error {
265+
// We would normally use a sql.NullString here, but sqlc does not want to make
266+
// a params struct with a nullable string.
267+
vardigest sql.NullString
268+
ifparams.OAuthRefreshTokenKeyID!="" {
269+
digest.String=params.OAuthRefreshTokenKeyID
270+
digest.Valid=true
271+
}
272+
iferr:=db.encryptField(&params.OAuthRefreshToken,&digest);err!=nil {
273+
returnerr
274+
}
275+
276+
returndb.Store.UpdateExternalAuthLinkRefreshToken(ctx,params)
277+
}
278+
264279
func (db*dbCrypt)GetCryptoKeys(ctx context.Context) ([]database.CryptoKey,error) {
265280
keys,err:=db.Store.GetCryptoKeys(ctx)
266281
iferr!=nil {

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp