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

Commitf895f94

Browse files
fix: stop extending API key access if OIDC refresh is available (cherry-pick#17878) (#17962)
Cherry-picked fix: stop extending API key access if OIDC refresh isavailable (#17878)fixes#17070Cleans up our handling of APIKey expiration and OIDC to keep themseparate concepts. For an OIDC-login APIKey, both the APIKey and OIDClink must be valid to login. If the OIDC link is expired and we have arefresh token, we will attempt to refresh.OIDC refreshes do not have any effect on APIKey expiry.#17070 (comment)explains why this is the correct behavior.Co-authored-by: Spike Curtis <spike@coder.com>
1 parent186d9b0 commitf895f94

File tree

4 files changed

+210
-48
lines changed

4 files changed

+210
-48
lines changed

‎coderd/coderdtest/oidctest/idp.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -215,7 +215,7 @@ func WithCustomClientAuth(hook func(t testing.TB, req *http.Request) (url.Values
215215
// WithLogging is optional, but will log some HTTP calls made to the IDP.
216216
funcWithLogging(t testing.TB,options*slogtest.Options)func(*FakeIDP) {
217217
returnfunc(f*FakeIDP) {
218-
f.logger=slogtest.Make(t,options)
218+
f.logger=slogtest.Make(t,options).Named("fakeidp")
219219
}
220220
}
221221

@@ -700,6 +700,7 @@ func (f *FakeIDP) newToken(t testing.TB, email string, expires time.Time) string
700700
func (f*FakeIDP)newRefreshTokens(emailstring)string {
701701
refreshToken:=uuid.NewString()
702702
f.refreshTokens.Store(refreshToken,email)
703+
f.logger.Info(context.Background(),"new refresh token",slog.F("email",email),slog.F("token",refreshToken))
703704
returnrefreshToken
704705
}
705706

@@ -909,6 +910,7 @@ func (f *FakeIDP) httpHandler(t testing.TB) http.Handler {
909910
return
910911
}
911912

913+
f.logger.Info(r.Context(),"http idp call refresh_token",slog.F("token",refreshToken))
912914
_,ok:=f.refreshTokens.Load(refreshToken)
913915
if!assert.True(t,ok,"invalid refresh_token") {
914916
http.Error(rw,"invalid refresh_token",http.StatusBadRequest)
@@ -932,6 +934,7 @@ func (f *FakeIDP) httpHandler(t testing.TB) http.Handler {
932934
f.refreshTokensUsed.Store(refreshToken,true)
933935
// Always invalidate the refresh token after it is used.
934936
f.refreshTokens.Delete(refreshToken)
937+
f.logger.Info(r.Context(),"refresh token invalidated",slog.F("token",refreshToken))
935938
case"urn:ietf:params:oauth:grant-type:device_code":
936939
// Device flow
937940
varresp externalauth.ExchangeDeviceCodeResponse

‎coderd/httpmw/apikey.go

Lines changed: 48 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -232,16 +232,21 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon
232232
returnoptionalWrite(http.StatusUnauthorized,resp)
233233
}
234234

235-
var (
236-
link database.UserLink
237-
now=dbtime.Now()
238-
// Tracks if the API key has properties updated
239-
changed=false
240-
)
235+
now:=dbtime.Now()
236+
ifkey.ExpiresAt.Before(now) {
237+
returnoptionalWrite(http.StatusUnauthorized, codersdk.Response{
238+
Message:SignedOutErrorMessage,
239+
Detail:fmt.Sprintf("API key expired at %q.",key.ExpiresAt.String()),
240+
})
241+
}
242+
243+
// We only check OIDC stuff if we have a valid APIKey. An expired key means we don't trust the requestor
244+
// really is the user whose key they have, and so we shouldn't be doing anything on their behalf including possibly
245+
// refreshing the OIDC token.
241246
ifkey.LoginType==database.LoginTypeGithub||key.LoginType==database.LoginTypeOIDC {
242247
varerrerror
243248
//nolint:gocritic // System needs to fetch UserLink to check if it's valid.
244-
link,err=cfg.DB.GetUserLinkByUserIDLoginType(dbauthz.AsSystemRestricted(ctx), database.GetUserLinkByUserIDLoginTypeParams{
249+
link,err:=cfg.DB.GetUserLinkByUserIDLoginType(dbauthz.AsSystemRestricted(ctx), database.GetUserLinkByUserIDLoginTypeParams{
245250
UserID:key.UserID,
246251
LoginType:key.LoginType,
247252
})
@@ -258,7 +263,7 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon
258263
})
259264
}
260265
// Check if the OAuth token is expired
261-
iflink.OAuthExpiry.Before(now)&&!link.OAuthExpiry.IsZero()&&link.OAuthRefreshToken!="" {
266+
if!link.OAuthExpiry.IsZero()&&link.OAuthExpiry.Before(now) {
262267
ifcfg.OAuth2Configs.IsZero() {
263268
returnwrite(http.StatusInternalServerError, codersdk.Response{
264269
Message:internalErrorMessage,
@@ -267,12 +272,15 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon
267272
})
268273
}
269274

275+
varfriendlyNamestring
270276
varoauthConfig promoauth.OAuth2Config
271277
switchkey.LoginType {
272278
casedatabase.LoginTypeGithub:
273279
oauthConfig=cfg.OAuth2Configs.Github
280+
friendlyName="GitHub"
274281
casedatabase.LoginTypeOIDC:
275282
oauthConfig=cfg.OAuth2Configs.OIDC
283+
friendlyName="OpenID Connect"
276284
default:
277285
returnwrite(http.StatusInternalServerError, codersdk.Response{
278286
Message:internalErrorMessage,
@@ -292,36 +300,53 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon
292300
})
293301
}
294302

295-
// If it is, let's refresh it from the provided config
303+
iflink.OAuthRefreshToken=="" {
304+
returnoptionalWrite(http.StatusUnauthorized, codersdk.Response{
305+
Message:SignedOutErrorMessage,
306+
Detail:fmt.Sprintf("%s session expired at %q. Try signing in again.",friendlyName,link.OAuthExpiry.String()),
307+
})
308+
}
309+
// We have a refresh token, so let's try it
296310
token,err:=oauthConfig.TokenSource(r.Context(),&oauth2.Token{
297311
AccessToken:link.OAuthAccessToken,
298312
RefreshToken:link.OAuthRefreshToken,
299313
Expiry:link.OAuthExpiry,
300314
}).Token()
301315
iferr!=nil {
302316
returnwrite(http.StatusUnauthorized, codersdk.Response{
303-
Message:"Could not refresh expired Oauth token. Try re-authenticating to resolve this issue.",
304-
Detail:err.Error(),
317+
Message:fmt.Sprintf(
318+
"Could not refresh expired %s token. Try re-authenticating to resolve this issue.",
319+
friendlyName),
320+
Detail:err.Error(),
305321
})
306322
}
307323
link.OAuthAccessToken=token.AccessToken
308324
link.OAuthRefreshToken=token.RefreshToken
309325
link.OAuthExpiry=token.Expiry
310-
key.ExpiresAt=token.Expiry
311-
changed=true
326+
//nolint:gocritic // system needs to update user link
327+
link,err=cfg.DB.UpdateUserLink(dbauthz.AsSystemRestricted(ctx), database.UpdateUserLinkParams{
328+
UserID:link.UserID,
329+
LoginType:link.LoginType,
330+
OAuthAccessToken:link.OAuthAccessToken,
331+
OAuthAccessTokenKeyID: sql.NullString{},// dbcrypt will update as required
332+
OAuthRefreshToken:link.OAuthRefreshToken,
333+
OAuthRefreshTokenKeyID: sql.NullString{},// dbcrypt will update as required
334+
OAuthExpiry:link.OAuthExpiry,
335+
// Refresh should keep the same debug context because we use
336+
// the original claims for the group/role sync.
337+
Claims:link.Claims,
338+
})
339+
iferr!=nil {
340+
returnwrite(http.StatusInternalServerError, codersdk.Response{
341+
Message:internalErrorMessage,
342+
Detail:fmt.Sprintf("update user_link: %s.",err.Error()),
343+
})
344+
}
312345
}
313346
}
314347

315-
// Checking if the key is expired.
316-
// NOTE: The `RequireAuth` React component depends on this `Detail` to detect when
317-
// the users token has expired. If you change the text here, make sure to update it
318-
// in site/src/components/RequireAuth/RequireAuth.tsx as well.
319-
ifkey.ExpiresAt.Before(now) {
320-
returnoptionalWrite(http.StatusUnauthorized, codersdk.Response{
321-
Message:SignedOutErrorMessage,
322-
Detail:fmt.Sprintf("API key expired at %q.",key.ExpiresAt.String()),
323-
})
324-
}
348+
// Tracks if the API key has properties updated
349+
changed:=false
325350

326351
// Only update LastUsed once an hour to prevent database spam.
327352
ifnow.Sub(key.LastUsed)>time.Hour {
@@ -363,29 +388,6 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon
363388
Detail:fmt.Sprintf("API key couldn't update: %s.",err.Error()),
364389
})
365390
}
366-
// If the API Key is associated with a user_link (e.g. Github/OIDC)
367-
// then we want to update the relevant oauth fields.
368-
iflink.UserID!=uuid.Nil {
369-
//nolint:gocritic // system needs to update user link
370-
link,err=cfg.DB.UpdateUserLink(dbauthz.AsSystemRestricted(ctx), database.UpdateUserLinkParams{
371-
UserID:link.UserID,
372-
LoginType:link.LoginType,
373-
OAuthAccessToken:link.OAuthAccessToken,
374-
OAuthAccessTokenKeyID: sql.NullString{},// dbcrypt will update as required
375-
OAuthRefreshToken:link.OAuthRefreshToken,
376-
OAuthRefreshTokenKeyID: sql.NullString{},// dbcrypt will update as required
377-
OAuthExpiry:link.OAuthExpiry,
378-
// Refresh should keep the same debug context because we use
379-
// the original claims for the group/role sync.
380-
Claims:link.Claims,
381-
})
382-
iferr!=nil {
383-
returnwrite(http.StatusInternalServerError, codersdk.Response{
384-
Message:internalErrorMessage,
385-
Detail:fmt.Sprintf("update user_link: %s.",err.Error()),
386-
})
387-
}
388-
}
389391

390392
// We only want to update this occasionally to reduce DB write
391393
// load. We update alongside the UserLink and APIKey since it's

‎coderd/httpmw/apikey_test.go

Lines changed: 157 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -508,6 +508,102 @@ func TestAPIKey(t *testing.T) {
508508
require.Equal(t,sentAPIKey.ExpiresAt,gotAPIKey.ExpiresAt)
509509
})
510510

511+
t.Run("APIKeyExpiredOAuthExpired",func(t*testing.T) {
512+
t.Parallel()
513+
var (
514+
db=dbmem.New()
515+
user=dbgen.User(t,db, database.User{})
516+
sentAPIKey,token=dbgen.APIKey(t,db, database.APIKey{
517+
UserID:user.ID,
518+
LastUsed:dbtime.Now().AddDate(0,0,-1),
519+
ExpiresAt:dbtime.Now().AddDate(0,0,-1),
520+
LoginType:database.LoginTypeOIDC,
521+
})
522+
_=dbgen.UserLink(t,db, database.UserLink{
523+
UserID:user.ID,
524+
LoginType:database.LoginTypeOIDC,
525+
OAuthExpiry:dbtime.Now().AddDate(0,0,-1),
526+
})
527+
528+
r=httptest.NewRequest("GET","/",nil)
529+
rw=httptest.NewRecorder()
530+
)
531+
r.Header.Set(codersdk.SessionTokenHeader,token)
532+
533+
// Include a valid oauth token for refreshing. If this token is invalid,
534+
// it is difficult to tell an auth failure from an expired api key, or
535+
// an expired oauth key.
536+
oauthToken:=&oauth2.Token{
537+
AccessToken:"wow",
538+
RefreshToken:"moo",
539+
Expiry:dbtime.Now().AddDate(0,0,1),
540+
}
541+
httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{
542+
DB:db,
543+
OAuth2Configs:&httpmw.OAuth2Configs{
544+
OIDC:&testutil.OAuth2Config{
545+
Token:oauthToken,
546+
},
547+
},
548+
RedirectToLogin:false,
549+
})(successHandler).ServeHTTP(rw,r)
550+
res:=rw.Result()
551+
deferres.Body.Close()
552+
require.Equal(t,http.StatusUnauthorized,res.StatusCode)
553+
554+
gotAPIKey,err:=db.GetAPIKeyByID(r.Context(),sentAPIKey.ID)
555+
require.NoError(t,err)
556+
557+
require.Equal(t,sentAPIKey.LastUsed,gotAPIKey.LastUsed)
558+
require.Equal(t,sentAPIKey.ExpiresAt,gotAPIKey.ExpiresAt)
559+
})
560+
561+
t.Run("APIKeyExpiredOAuthNotExpired",func(t*testing.T) {
562+
t.Parallel()
563+
var (
564+
db=dbmem.New()
565+
user=dbgen.User(t,db, database.User{})
566+
sentAPIKey,token=dbgen.APIKey(t,db, database.APIKey{
567+
UserID:user.ID,
568+
LastUsed:dbtime.Now().AddDate(0,0,-1),
569+
ExpiresAt:dbtime.Now().AddDate(0,0,-1),
570+
LoginType:database.LoginTypeOIDC,
571+
})
572+
_=dbgen.UserLink(t,db, database.UserLink{
573+
UserID:user.ID,
574+
LoginType:database.LoginTypeOIDC,
575+
})
576+
577+
r=httptest.NewRequest("GET","/",nil)
578+
rw=httptest.NewRecorder()
579+
)
580+
r.Header.Set(codersdk.SessionTokenHeader,token)
581+
582+
oauthToken:=&oauth2.Token{
583+
AccessToken:"wow",
584+
RefreshToken:"moo",
585+
Expiry:dbtime.Now().AddDate(0,0,1),
586+
}
587+
httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{
588+
DB:db,
589+
OAuth2Configs:&httpmw.OAuth2Configs{
590+
OIDC:&testutil.OAuth2Config{
591+
Token:oauthToken,
592+
},
593+
},
594+
RedirectToLogin:false,
595+
})(successHandler).ServeHTTP(rw,r)
596+
res:=rw.Result()
597+
deferres.Body.Close()
598+
require.Equal(t,http.StatusUnauthorized,res.StatusCode)
599+
600+
gotAPIKey,err:=db.GetAPIKeyByID(r.Context(),sentAPIKey.ID)
601+
require.NoError(t,err)
602+
603+
require.Equal(t,sentAPIKey.LastUsed,gotAPIKey.LastUsed)
604+
require.Equal(t,sentAPIKey.ExpiresAt,gotAPIKey.ExpiresAt)
605+
})
606+
511607
t.Run("OAuthRefresh",func(t*testing.T) {
512608
t.Parallel()
513609
var (
@@ -553,7 +649,67 @@ func TestAPIKey(t *testing.T) {
553649
require.NoError(t,err)
554650

555651
require.Equal(t,sentAPIKey.LastUsed,gotAPIKey.LastUsed)
556-
require.Equal(t,oauthToken.Expiry,gotAPIKey.ExpiresAt)
652+
// Note that OAuth expiry is independent of APIKey expiry, so an OIDC refresh DOES NOT affect the expiry of the
653+
// APIKey
654+
require.Equal(t,sentAPIKey.ExpiresAt,gotAPIKey.ExpiresAt)
655+
656+
gotLink,err:=db.GetUserLinkByUserIDLoginType(r.Context(), database.GetUserLinkByUserIDLoginTypeParams{
657+
UserID:user.ID,
658+
LoginType:database.LoginTypeGithub,
659+
})
660+
require.NoError(t,err)
661+
require.Equal(t,gotLink.OAuthRefreshToken,"moo")
662+
})
663+
664+
t.Run("OAuthExpiredNoRefresh",func(t*testing.T) {
665+
t.Parallel()
666+
var (
667+
ctx=testutil.Context(t,testutil.WaitShort)
668+
db=dbmem.New()
669+
user=dbgen.User(t,db, database.User{})
670+
sentAPIKey,token=dbgen.APIKey(t,db, database.APIKey{
671+
UserID:user.ID,
672+
LastUsed:dbtime.Now(),
673+
ExpiresAt:dbtime.Now().AddDate(0,0,1),
674+
LoginType:database.LoginTypeGithub,
675+
})
676+
677+
r=httptest.NewRequest("GET","/",nil)
678+
rw=httptest.NewRecorder()
679+
)
680+
_,err:=db.InsertUserLink(ctx, database.InsertUserLinkParams{
681+
UserID:user.ID,
682+
LoginType:database.LoginTypeGithub,
683+
OAuthExpiry:dbtime.Now().AddDate(0,0,-1),
684+
OAuthAccessToken:"letmein",
685+
})
686+
require.NoError(t,err)
687+
688+
r.Header.Set(codersdk.SessionTokenHeader,token)
689+
690+
oauthToken:=&oauth2.Token{
691+
AccessToken:"wow",
692+
RefreshToken:"moo",
693+
Expiry:dbtime.Now().AddDate(0,0,1),
694+
}
695+
httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{
696+
DB:db,
697+
OAuth2Configs:&httpmw.OAuth2Configs{
698+
Github:&testutil.OAuth2Config{
699+
Token:oauthToken,
700+
},
701+
},
702+
RedirectToLogin:false,
703+
})(successHandler).ServeHTTP(rw,r)
704+
res:=rw.Result()
705+
deferres.Body.Close()
706+
require.Equal(t,http.StatusUnauthorized,res.StatusCode)
707+
708+
gotAPIKey,err:=db.GetAPIKeyByID(r.Context(),sentAPIKey.ID)
709+
require.NoError(t,err)
710+
711+
require.Equal(t,sentAPIKey.LastUsed,gotAPIKey.LastUsed)
712+
require.Equal(t,sentAPIKey.ExpiresAt,gotAPIKey.ExpiresAt)
557713
})
558714

559715
t.Run("RemoteIPUpdates",func(t*testing.T) {

‎coderd/oauthpki/okidcpki_test.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,7 @@ func TestAzureAKPKIWithCoderd(t *testing.T) {
144144
returnvalues,nil
145145
}),
146146
oidctest.WithServing(),
147+
oidctest.WithLogging(t,nil),
147148
)
148149
cfg:=fake.OIDCConfig(t,scopes,func(cfg*coderd.OIDCConfig) {
149150
cfg.AllowSignups=true

0 commit comments

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