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

Commit78554eb

Browse files
committed
feat: add best effort attempt to revoke oauth access token in provider
1 parent38d3d06 commit78554eb

File tree

19 files changed

+482
-69
lines changed

19 files changed

+482
-69
lines changed

‎coderd/apidoc/docs.go‎

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

‎coderd/apidoc/swagger.json‎

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

‎coderd/coderdtest/oidctest/idp.go‎

Lines changed: 59 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,8 @@ import (
4646
"github.com/coder/coder/v2/testutil"
4747
)
4848

49+
typeHookRevokeTokenFnfunc() (httpStatusint,errerror)
50+
4951
typetokenstruct {
5052
issued time.Time
5153
emailstring
@@ -196,9 +198,11 @@ type FakeIDP struct {
196198
// hookValidRedirectURL can be used to reject a redirect url from the
197199
// IDP -> Application. Almost all IDPs have the concept of
198200
// "Authorized Redirect URLs". This can be used to emulate that.
199-
hookValidRedirectURLfunc(redirectURLstring)error
200-
hookUserInfofunc(emailstring) (jwt.MapClaims,error)
201-
hookAccessTokenJWTfunc(emailstring,exp time.Time) jwt.MapClaims
201+
hookValidRedirectURLfunc(redirectURLstring)error
202+
hookUserInfofunc(emailstring) (jwt.MapClaims,error)
203+
hookRevokeTokenHookRevokeTokenFn
204+
revokeTokenGitHubFormatbool// GitHub doesn't follow token revocation RFC spec
205+
hookAccessTokenJWTfunc(emailstring,exp time.Time) jwt.MapClaims
202206
// defaultIDClaims is if a new client connects and we didn't preset
203207
// some claims.
204208
defaultIDClaims jwt.MapClaims
@@ -327,6 +331,19 @@ func WithStaticUserInfo(info jwt.MapClaims) func(*FakeIDP) {
327331
}
328332
}
329333

334+
funcWithRevokeTokenRFC(revokeFuncHookRevokeTokenFn)func(*FakeIDP) {
335+
returnfunc(f*FakeIDP) {
336+
f.hookRevokeToken=revokeFunc
337+
}
338+
}
339+
340+
funcWithRevokeTokenGitHub(revokeFuncHookRevokeTokenFn)func(*FakeIDP) {
341+
returnfunc(f*FakeIDP) {
342+
f.hookRevokeToken=revokeFunc
343+
f.revokeTokenGitHubFormat=true
344+
}
345+
}
346+
330347
funcWithDefaultIDClaims(claims jwt.MapClaims)func(*FakeIDP) {
331348
returnfunc(f*FakeIDP) {
332349
f.defaultIDClaims=claims
@@ -358,6 +375,7 @@ type With429Arguments struct {
358375
AuthorizePathbool
359376
KeysPathbool
360377
UserInfoPathbool
378+
RevokePathbool
361379
DeviceAuthbool
362380
DeviceVerifybool
363381
}
@@ -387,6 +405,10 @@ func With429(params With429Arguments) func(*FakeIDP) {
387405
http.Error(rw,"429, being manually blocked (userinfo)",http.StatusTooManyRequests)
388406
return
389407
}
408+
ifparams.RevokePath&&strings.Contains(r.URL.Path,revokeTokenPath) {
409+
http.Error(rw,"429, being manually blocked (revoke)",http.StatusTooManyRequests)
410+
return
411+
}
390412
ifparams.DeviceAuth&&strings.Contains(r.URL.Path,deviceAuth) {
391413
http.Error(rw,"429, being manually blocked (device-auth)",http.StatusTooManyRequests)
392414
return
@@ -408,8 +430,10 @@ const (
408430
authorizePath="/oauth2/authorize"
409431
keysPath="/oauth2/keys"
410432
userInfoPath="/oauth2/userinfo"
411-
deviceAuth="/login/device/code"
412-
deviceVerify="/login/device"
433+
// nolint:gosec // It also thinks this is a secret lol
434+
revokeTokenPath="/oauth2/revoke"
435+
deviceAuth="/login/device/code"
436+
deviceVerify="/login/device"
413437
)
414438

415439
funcNewFakeIDP(t testing.TB,opts...FakeIDPOpt)*FakeIDP {
@@ -486,6 +510,7 @@ func (f *FakeIDP) updateIssuerURL(t testing.TB, issuer string) {
486510
TokenURL:u.ResolveReference(&url.URL{Path:tokenPath}).String(),
487511
JWKSURL:u.ResolveReference(&url.URL{Path:keysPath}).String(),
488512
UserInfoURL:u.ResolveReference(&url.URL{Path:userInfoPath}).String(),
513+
RevokeURL:u.ResolveReference(&url.URL{Path:revokeTokenPath}).String(),
489514
DeviceCodeURL:u.ResolveReference(&url.URL{Path:deviceAuth}).String(),
490515
Algorithms: []string{
491516
"RS256",
@@ -756,6 +781,7 @@ type ProviderJSON struct {
756781
TokenURLstring`json:"token_endpoint"`
757782
JWKSURLstring`json:"jwks_uri"`
758783
UserInfoURLstring`json:"userinfo_endpoint"`
784+
RevokeURLstring`json:"revocation_endpoint"`
759785
DeviceCodeURLstring`json:"device_authorization_endpoint"`
760786
Algorithms []string`json:"id_token_signing_alg_values_supported"`
761787
// This is custom
@@ -1146,6 +1172,29 @@ func (f *FakeIDP) httpHandler(t testing.TB) http.Handler {
11461172
_=json.NewEncoder(rw).Encode(claims)
11471173
}))
11481174

1175+
mux.Handle(revokeTokenPath,http.HandlerFunc(func(rw http.ResponseWriter,r*http.Request) {
1176+
iff.revokeTokenGitHubFormat {
1177+
u,p,ok:=r.BasicAuth()
1178+
if!ok||!(u==f.clientID&&p==f.clientSecret) {
1179+
httpError(rw,http.StatusForbidden,xerrors.Errorf("basic auth failed"))
1180+
return
1181+
}
1182+
}else {
1183+
_,ok:=validateMW(rw,r)
1184+
if!ok {
1185+
httpError(rw,http.StatusForbidden,xerrors.Errorf("token validation failed"))
1186+
return
1187+
}
1188+
}
1189+
1190+
code,err:=f.hookRevokeToken()
1191+
iferr!=nil {
1192+
httpError(rw,code,xerrors.Errorf("hook err: %w",err))
1193+
return
1194+
}
1195+
httpapi.Write(r.Context(),rw,code,"")
1196+
}))
1197+
11491198
// There is almost no difference between this and /userinfo.
11501199
// The main tweak is that this route is "mounted" vs "handle" because "/userinfo"
11511200
// should be strict, and this one needs to handle sub routes.
@@ -1474,12 +1523,16 @@ func (f *FakeIDP) ExternalAuthConfig(t testing.TB, id string, custom *ExternalAu
14741523
DisplayName:id,
14751524
InstrumentedOAuth2Config:oauthCfg,
14761525
ID:id,
1526+
ClientID:f.clientID,
1527+
ClientSecret:f.clientSecret,
14771528
// No defaults for these fields by omitting the type
14781529
Type:"",
14791530
DisplayIcon:f.WellknownConfig().UserInfoURL,
14801531
// Omit the /user for the validate so we can easily append to it when modifying
14811532
// the cfg for advanced tests.
1482-
ValidateURL:f.locked.IssuerURL().ResolveReference(&url.URL{Path:"/external-auth-validate/"}).String(),
1533+
ValidateURL:f.locked.IssuerURL().ResolveReference(&url.URL{Path:"/external-auth-validate/"}).String(),
1534+
RevokeURL:f.locked.IssuerURL().ResolveReference(&url.URL{Path:revokeTokenPath}).String(),
1535+
RevokeTimeout:1*time.Second,
14831536
DeviceAuth:&externalauth.DeviceAuth{
14841537
Config:oauthCfg,
14851538
ClientID:f.clientID,

‎coderd/externalauth.go‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ func (api *API) externalAuthByID(w http.ResponseWriter, r *http.Request) {
8787
// @Tags Git
8888
// @Produce json
8989
// @Param externalauth path string true "Git Provider ID" format(string)
90-
// @Success 200
90+
// @Success 200 {object} codersdk.DeleteExternalAuthByIDResponse
9191
// @Router /external-auth/{externalauth} [delete]
9292
func (api*API)deleteExternalAuthByID(w http.ResponseWriter,r*http.Request) {
9393
config:=httpmw.ExternalAuthParam(r)
@@ -98,7 +98,6 @@ func (api *API) deleteExternalAuthByID(w http.ResponseWriter, r *http.Request) {
9898
ProviderID:config.ID,
9999
UserID:apiKey.UserID,
100100
})
101-
102101
iferr!=nil {
103102
iferrors.Is(err,sql.ErrNoRows) {
104103
httpapi.ResourceNotFound(w)
@@ -128,12 +127,12 @@ func (api *API) deleteExternalAuthByID(w http.ResponseWriter, r *http.Request) {
128127
}
129128

130129
ok,err:=config.RevokeToken(ctx,link)
131-
iferr!=nil||!ok {
132-
httpapi.Write(ctx,w,http.StatusOK, codersdk.DeleteExternalAuthByIDResponse{TokenRevocationSuccessful:false})
133-
return
134-
}
130+
resp:= codersdk.DeleteExternalAuthByIDResponse{TokenRevoked:ok}
135131

136-
httpapi.Write(ctx,w,http.StatusOK, codersdk.DeleteExternalAuthByIDResponse{TokenRevocationSuccessful:true})
132+
iferr!=nil {
133+
resp.TokenRevocationError=err.Error()
134+
}
135+
httpapi.Write(ctx,w,http.StatusOK,resp)
137136
}
138137

139138
// @Summary Post external auth device by ID

‎coderd/externalauth/externalauth.go‎

Lines changed: 46 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,9 @@ const (
3434
// database for a failed refresh token. In rare cases, the error could be a large
3535
// HTML payload.
3636
failureReasonLimit=400
37+
38+
// tokenRevocationTimeout timeout for requests to external oauth provider.
39+
tokenRevocationTimeout=10*time.Second
3740
)
3841

3942
// Config is used for authentication for Git operations.
@@ -44,7 +47,7 @@ type Config struct {
4447
// Type is the type of provider.
4548
Typestring
4649

47-
ClientIdstring
50+
ClientIDstring
4851
ClientSecretstring
4952
// DeviceAuth is set if the provider uses the device flow.
5053
DeviceAuth*DeviceAuth
@@ -72,7 +75,8 @@ type Config struct {
7275
// not be validated before being returned.
7376
ValidateURLstring
7477

75-
RevokeURLstring
78+
RevokeURLstring
79+
RevokeTimeout time.Duration
7680

7781
// Regex is a Regexp matched against URLs for
7882
// a Git clone. e.g. "Username for 'https://github.com':"
@@ -395,13 +399,9 @@ func (c *Config) RevokeToken(ctx context.Context, link database.ExternalAuthLink
395399
returnfalse,nil
396400
}
397401

398-
varerrerror
399-
varreq*http.Request
400-
ifc.Type==codersdk.EnhancedExternalAuthProviderGitHub.String() {
401-
req,err=c.tokenRevocationRequestGitHub(ctx,link)
402-
}else {
403-
req,err=c.tokenRevocationRequestRFC7009(ctx,link)
404-
}
402+
reqCtx,cancel:=context.WithTimeout(ctx,c.RevokeTimeout)
403+
defercancel()
404+
req,err:=c.TokenRevocationRequest(reqCtx,link)
405405
iferr!=nil {
406406
returnfalse,err
407407
}
@@ -411,34 +411,60 @@ func (c *Config) RevokeToken(ctx context.Context, link database.ExternalAuthLink
411411
returnfalse,err
412412
}
413413
deferres.Body.Close()
414+
body,err:=io.ReadAll(res.Body)
415+
iferr!=nil {
416+
returnfalse,err
417+
}
414418

415-
returnres.StatusCode==http.StatusOK,nil
419+
ifc.TokenRevocationResponseOk(res) {
420+
returntrue,nil
421+
}
422+
returnfalse,xerrors.Errorf("failed to revoke token: %d %s",res.StatusCode,string(body))
416423
}
417424

418-
func (c*Config)tokenRevocationRequestRFC7009(ctx context.Context,link database.ExternalAuthLink) (*http.Request,error) {
425+
func (c*Config)TokenRevocationRequest(ctx context.Context,link database.ExternalAuthLink) (*http.Request,error) {
426+
ifc.Type==codersdk.EnhancedExternalAuthProviderGitHub.String() {
427+
returnc.TokenRevocationRequestGitHub(ctx,link)
428+
}
429+
returnc.TokenRevocationRequestRFC7009(ctx,link)
430+
}
431+
432+
func (c*Config)TokenRevocationRequestRFC7009(ctx context.Context,link database.ExternalAuthLink) (*http.Request,error) {
419433
p:= url.Values{}
420-
p.Add("client_id",c.ClientId)
434+
p.Add("client_id",c.ClientID)
421435
p.Add("client_secret",c.ClientSecret)
422436
p.Add("token_type_hint","refresh_token")
423437
p.Add("token",link.OAuthRefreshToken)
424-
body:=p.Encode()
425-
returnhttp.NewRequestWithContext(ctx,http.MethodPost,c.RevokeURL,strings.NewReader(body))
438+
req,err:=http.NewRequestWithContext(ctx,http.MethodPost,c.RevokeURL,strings.NewReader(p.Encode()))
439+
iferr!=nil {
440+
returnnil,err
441+
}
442+
req.Header.Set("Authorization",fmt.Sprintf("Bearer %s",link.OAuthAccessToken))
443+
returnreq,nil
426444
}
427445

428-
func (c*Config)tokenRevocationRequestGitHub(ctx context.Context,link database.ExternalAuthLink) (*http.Request,error) {
429-
// GitHub doesn't follow RFC spec, GitHub specific request is needed
446+
func (c*Config)TokenRevocationRequestGitHub(ctx context.Context,link database.ExternalAuthLink) (*http.Request,error) {
447+
// GitHub doesn't follow RFC spec
430448
// https://docs.github.com/en/rest/apps/oauth-applications?apiVersion=2022-11-28#delete-an-app-authorization
431-
body:=fmt.Sprintf("{\"access_token\":\"%s\"}",link.OAuthAccessToken)
449+
body:=fmt.Sprintf("{\"access_token\":%q}",link.OAuthAccessToken)
432450
req,err:=http.NewRequestWithContext(ctx,http.MethodDelete,c.RevokeURL,strings.NewReader(body))
433451
iferr!=nil {
434452
returnnil,err
435453
}
436454
req.Header.Add("Accept","application/vnd.github+json")
437455
req.Header.Add("X-GitHub-Api-Version","2022-11-28")
438-
req.SetBasicAuth(c.ClientId,c.ClientSecret)
456+
req.SetBasicAuth(c.ClientID,c.ClientSecret)
439457
returnreq,nil
440458
}
441459

460+
func (c*Config)TokenRevocationResponseOk(res*http.Response)bool {
461+
// RFC spec on successful revocation returns 200, GitHub 204
462+
ifc.Type==codersdk.EnhancedExternalAuthProviderGitHub.String() {
463+
returnres.StatusCode==http.StatusNoContent
464+
}
465+
returnres.StatusCode==http.StatusOK
466+
}
467+
442468
typeDeviceAuthstruct {
443469
// Config is provided for the http client method.
444470
Config promoauth.InstrumentedOAuth2Config
@@ -665,13 +691,14 @@ func ConvertConfig(instrument *promoauth.Factory, entries []codersdk.ExternalAut
665691
cfg:=&Config{
666692
InstrumentedOAuth2Config:instrumented,
667693
ID:entry.ID,
668-
ClientId:entry.ClientID,
694+
ClientID:entry.ClientID,
669695
ClientSecret:entry.ClientSecret,
670696
Regex:regex,
671697
Type:entry.Type,
672698
NoRefresh:entry.NoRefresh,
673699
ValidateURL:entry.ValidateURL,
674700
RevokeURL:entry.RevokeURL,
701+
RevokeTimeout:tokenRevocationTimeout,
675702
AppInstallationsURL:entry.AppInstallationsURL,
676703
AppInstallURL:entry.AppInstallURL,
677704
DisplayName:entry.DisplayName,

0 commit comments

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