From f10872880bb0534285dc39fb29b5c55c632c4ce4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarl=20Andr=C3=A9=20H=C3=BCbenthal?= Date: Fri, 20 Jan 2023 07:02:30 +0100 Subject: [PATCH 1/4] allow code grant without secret if code verifier is defined --- manage/manager.go | 8 ++++++-- server/server_test.go | 24 +++++++++++++----------- 2 files changed, 19 insertions(+), 13 deletions(-) diff --git a/manage/manager.go b/manage/manager.go index c11f391..214751d 100755 --- a/manage/manager.go +++ b/manage/manager.go @@ -218,7 +218,7 @@ func (m *Manager) GenerateAuthToken(ctx context.Context, rt oauth2.ResponseType, } return ti, nil } - + // get authorization code data func (m *Manager) getAuthorizationCode(ctx context.Context, code string) (oauth2.TokenInfo, error) { ti, err := m.tokenStore.GetByCode(ctx, code) @@ -288,7 +288,11 @@ func (m *Manager) GenerateAccessToken(ctx context.Context, gt oauth2.GrantType, return nil, errors.ErrInvalidClient } } else if len(cli.GetSecret()) > 0 && tgr.ClientSecret != cli.GetSecret() { - return nil, errors.ErrInvalidClient + // auth code flow doesnt require client_secret if used with PKCE and state parameter + // this is especially useful for mobile apps, that cant hold the secret + if !(gt == oauth2.AuthorizationCode && tgr.ClientSecret == "" && tgr.CodeVerifier != "") { + return nil, errors.ErrInvalidClient + } } if tgr.RedirectURI != "" { if err := m.validateURI(cli.GetDomain(), tgr.RedirectURI); err != nil { diff --git a/server/server_test.go b/server/server_test.go index 0fd00d5..c752776 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -5,7 +5,6 @@ import ( "fmt" "net/http" "net/http/httptest" - "net/url" "testing" "github.com/gavv/httpexpect" @@ -26,9 +25,10 @@ var ( clientSecret = "11111111" plainChallenge = "ThisIsAFourtyThreeCharactersLongStringThing" - s256Challenge = "s256test" - // echo s256test | sha256 | base64 | tr '/+' '_-' - s256ChallengeHash = "W6YWc_4yHwYN-cGDgGmOMHF3l7KDy7VcRjf7q2FVF-o=" + s256Challenge = "s256tests256tests256tests256tests256tests256test" + // sha2562 := sha256.Sum256([]byte(s256Challenge)) + // fmt.Printf(base64.URLEncoding.EncodeToString(sha2562[:])) + s256ChallengeHash = "To2Xqv01cm16bC9Sf7KRRS8CO2SFss_HSMQOr3sdCDE=" ) func init() { @@ -107,7 +107,7 @@ func TestAuthorizeCode(t *testing.T) { WithQuery("client_id", clientID). WithQuery("scope", "all"). WithQuery("state", "123"). - WithQuery("redirect_uri", url.QueryEscape(csrv.URL+"/oauth2")). + WithQuery("redirect_uri", csrv.URL+"/oauth2"). Expect().Status(http.StatusOK) } @@ -134,7 +134,7 @@ func TestAuthorizeCodeWithChallengePlain(t *testing.T) { WithFormField("grant_type", "authorization_code"). WithFormField("client_id", clientID). WithFormField("code", code). - WithBasicAuth("code_verifier", "testchallenge"). + WithFormField("code_verifier", plainChallenge). Expect(). Status(http.StatusOK). JSON().Object() @@ -152,13 +152,14 @@ func TestAuthorizeCodeWithChallengePlain(t *testing.T) { userID = "000000" return }) + srv.SetClientInfoHandler(server.ClientFormHandler) e.GET("/authorize"). WithQuery("response_type", "code"). WithQuery("client_id", clientID). WithQuery("scope", "all"). WithQuery("state", "123"). - WithQuery("redirect_uri", url.QueryEscape(csrv.URL+"/oauth2")). + WithQuery("redirect_uri", csrv.URL+"/oauth2"). WithQuery("code_challenge", plainChallenge). Expect().Status(http.StatusOK) } @@ -186,7 +187,7 @@ func TestAuthorizeCodeWithChallengeS256(t *testing.T) { WithFormField("grant_type", "authorization_code"). WithFormField("client_id", clientID). WithFormField("code", code). - WithBasicAuth("code_verifier", s256Challenge). + WithFormField("code_verifier", s256Challenge). Expect(). Status(http.StatusOK). JSON().Object() @@ -204,13 +205,14 @@ func TestAuthorizeCodeWithChallengeS256(t *testing.T) { userID = "000000" return }) + srv.SetClientInfoHandler(server.ClientFormHandler) e.GET("/authorize"). WithQuery("response_type", "code"). WithQuery("client_id", clientID). WithQuery("scope", "all"). WithQuery("state", "123"). - WithQuery("redirect_uri", url.QueryEscape(csrv.URL+"/oauth2")). + WithQuery("redirect_uri", csrv.URL+"/oauth2"). WithQuery("code_challenge", s256ChallengeHash). WithQuery("code_challenge_method", "S256"). Expect().Status(http.StatusOK) @@ -238,7 +240,7 @@ func TestImplicit(t *testing.T) { WithQuery("client_id", clientID). WithQuery("scope", "all"). WithQuery("state", "123"). - WithQuery("redirect_uri", url.QueryEscape(csrv.URL+"/oauth2")). + WithQuery("redirect_uri", csrv.URL+"/oauth2"). Expect().Status(http.StatusOK) } @@ -384,7 +386,7 @@ func TestRefreshing(t *testing.T) { WithQuery("client_id", clientID). WithQuery("scope", "all"). WithQuery("state", "123"). - WithQuery("redirect_uri", url.QueryEscape(csrv.URL+"/oauth2")). + WithQuery("redirect_uri", csrv.URL+"/oauth2"). Expect().Status(http.StatusOK) } From e0f39b97beea4473afdffa23d91ff135a2c7c6c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarl=20Andr=C3=A9=20H=C3=BCbenthal?= Date: Fri, 20 Jan 2023 09:08:54 +0100 Subject: [PATCH 2/4] add public field to Client and ClientInfo --- manage/manager.go | 8 ++------ model.go | 1 + models/client.go | 6 ++++++ server/server_test.go | 17 +++++++++-------- 4 files changed, 18 insertions(+), 14 deletions(-) diff --git a/manage/manager.go b/manage/manager.go index 214751d..21f08cc 100755 --- a/manage/manager.go +++ b/manage/manager.go @@ -287,12 +287,8 @@ func (m *Manager) GenerateAccessToken(ctx context.Context, gt oauth2.GrantType, if !cliPass.VerifyPassword(tgr.ClientSecret) { return nil, errors.ErrInvalidClient } - } else if len(cli.GetSecret()) > 0 && tgr.ClientSecret != cli.GetSecret() { - // auth code flow doesnt require client_secret if used with PKCE and state parameter - // this is especially useful for mobile apps, that cant hold the secret - if !(gt == oauth2.AuthorizationCode && tgr.ClientSecret == "" && tgr.CodeVerifier != "") { - return nil, errors.ErrInvalidClient - } + } else if cli.IsPublic() == false && len(cli.GetSecret()) > 0 && tgr.ClientSecret != cli.GetSecret() { + return nil, errors.ErrInvalidClient } if tgr.RedirectURI != "" { if err := m.validateURI(cli.GetDomain(), tgr.RedirectURI); err != nil { diff --git a/model.go b/model.go index 121a42d..ec52244 100644 --- a/model.go +++ b/model.go @@ -10,6 +10,7 @@ type ( GetID() string GetSecret() string GetDomain() string + IsPublic() bool GetUserID() string } diff --git a/models/client.go b/models/client.go index e7ad7f5..e9d051f 100644 --- a/models/client.go +++ b/models/client.go @@ -5,6 +5,7 @@ type Client struct { ID string Secret string Domain string + Public bool UserID string } @@ -23,6 +24,11 @@ func (c *Client) GetDomain() string { return c.Domain } +// GetUserID user id +func (c *Client) IsPublic() bool { + return c.Public +} + // GetUserID user id func (c *Client) GetUserID() string { return c.UserID diff --git a/server/server_test.go b/server/server_test.go index c752776..ff6e347 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -36,12 +36,13 @@ func init() { manager.MustTokenStorage(store.NewMemoryTokenStore()) } -func clientStore(domain string) oauth2.ClientStore { +func clientStore(domain string, public bool) oauth2.ClientStore { clientStore := store.NewClientStore() clientStore.Set(clientID, &models.Client{ ID: clientID, Secret: clientSecret, Domain: domain, + Public: public, }) return clientStore } @@ -95,7 +96,7 @@ func TestAuthorizeCode(t *testing.T) { })) defer csrv.Close() - manager.MapClientStorage(clientStore(csrv.URL)) + manager.MapClientStorage(clientStore(csrv.URL, true)) srv = server.NewDefaultServer(manager) srv.SetUserAuthorizationHandler(func(w http.ResponseWriter, r *http.Request) (userID string, err error) { userID = "000000" @@ -146,7 +147,7 @@ func TestAuthorizeCodeWithChallengePlain(t *testing.T) { })) defer csrv.Close() - manager.MapClientStorage(clientStore(csrv.URL)) + manager.MapClientStorage(clientStore(csrv.URL, true)) srv = server.NewDefaultServer(manager) srv.SetUserAuthorizationHandler(func(w http.ResponseWriter, r *http.Request) (userID string, err error) { userID = "000000" @@ -199,7 +200,7 @@ func TestAuthorizeCodeWithChallengeS256(t *testing.T) { })) defer csrv.Close() - manager.MapClientStorage(clientStore(csrv.URL)) + manager.MapClientStorage(clientStore(csrv.URL, true)) srv = server.NewDefaultServer(manager) srv.SetUserAuthorizationHandler(func(w http.ResponseWriter, r *http.Request) (userID string, err error) { userID = "000000" @@ -228,7 +229,7 @@ func TestImplicit(t *testing.T) { csrv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) defer csrv.Close() - manager.MapClientStorage(clientStore(csrv.URL)) + manager.MapClientStorage(clientStore(csrv.URL, false)) srv = server.NewDefaultServer(manager) srv.SetUserAuthorizationHandler(func(w http.ResponseWriter, r *http.Request) (userID string, err error) { userID = "000000" @@ -251,7 +252,7 @@ func TestPasswordCredentials(t *testing.T) { defer tsrv.Close() e := httpexpect.New(t, tsrv.URL) - manager.MapClientStorage(clientStore("")) + manager.MapClientStorage(clientStore("", false)) srv = server.NewDefaultServer(manager) srv.SetPasswordAuthorizationHandler(func(ctx context.Context, clientID, username, password string) (userID string, err error) { if username == "admin" && password == "123456" { @@ -284,7 +285,7 @@ func TestClientCredentials(t *testing.T) { defer tsrv.Close() e := httpexpect.New(t, tsrv.URL) - manager.MapClientStorage(clientStore("")) + manager.MapClientStorage(clientStore("", false)) srv = server.NewDefaultServer(manager) srv.SetClientInfoHandler(server.ClientFormHandler) @@ -374,7 +375,7 @@ func TestRefreshing(t *testing.T) { })) defer csrv.Close() - manager.MapClientStorage(clientStore(csrv.URL)) + manager.MapClientStorage(clientStore(csrv.URL, true)) srv = server.NewDefaultServer(manager) srv.SetUserAuthorizationHandler(func(w http.ResponseWriter, r *http.Request) (userID string, err error) { userID = "000000" From be5bcc1014b60b05a66862422d411c354500370c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarl=20Andr=C3=A9=20H=C3=BCbenthal?= Date: Fri, 20 Jan 2023 09:40:21 +0100 Subject: [PATCH 3/4] use public field to block confidential grants --- manage/manager.go | 6 +++++- server/server_test.go | 8 +++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/manage/manager.go b/manage/manager.go index 21f08cc..ad09790 100755 --- a/manage/manager.go +++ b/manage/manager.go @@ -287,7 +287,7 @@ func (m *Manager) GenerateAccessToken(ctx context.Context, gt oauth2.GrantType, if !cliPass.VerifyPassword(tgr.ClientSecret) { return nil, errors.ErrInvalidClient } - } else if cli.IsPublic() == false && len(cli.GetSecret()) > 0 && tgr.ClientSecret != cli.GetSecret() { + } else if len(cli.GetSecret()) > 0 && tgr.ClientSecret != cli.GetSecret() { return nil, errors.ErrInvalidClient } if tgr.RedirectURI != "" { @@ -296,6 +296,10 @@ func (m *Manager) GenerateAccessToken(ctx context.Context, gt oauth2.GrantType, } } + if gt == oauth2.ClientCredentials && cli.IsPublic() == true { + return nil, errors.ErrInvalidClient + } + if gt == oauth2.AuthorizationCode { ti, err := m.getAndDelAuthorizationCode(ctx, tgr) if err != nil { diff --git a/server/server_test.go b/server/server_test.go index ff6e347..1eb0bcd 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -38,9 +38,15 @@ func init() { func clientStore(domain string, public bool) oauth2.ClientStore { clientStore := store.NewClientStore() + var secret string + if public { + secret = "" + } else { + secret = clientSecret + } clientStore.Set(clientID, &models.Client{ ID: clientID, - Secret: clientSecret, + Secret: secret, Domain: domain, Public: public, }) From 4d9fa1ec6261d32d93ce047b49d0ea412e8d36bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarl=20Andr=C3=A9=20H=C3=BCbenthal?= Date: Fri, 20 Jan 2023 09:54:00 +0100 Subject: [PATCH 4/4] fix doc --- models/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/client.go b/models/client.go index e9d051f..c31ad7f 100644 --- a/models/client.go +++ b/models/client.go @@ -24,7 +24,7 @@ func (c *Client) GetDomain() string { return c.Domain } -// GetUserID user id +// IsPublic public func (c *Client) IsPublic() bool { return c.Public }