From 165197e472d4c45f575dbfcf4e702e19bbcda4e7 Mon Sep 17 00:00:00 2001 From: Stavros Date: Tue, 7 Apr 2026 19:04:20 +0300 Subject: [PATCH] feat: add pkce support to oidc server (#766) * feat: add pkce support to oidc server * tests: add test cases for pkce * fix: review comments * chore: remove debug line * chore: remove simple logger from testing * tests: add test for invalid challenge method * chore: fix typo --- frontend/src/lib/hooks/oidc.ts | 11 +- .../migrations/000007_oidc_pkce.down.sql | 1 + .../assets/migrations/000007_oidc_pkce.up.sql | 1 + .../controller/context_controller_test.go | 2 + internal/controller/health_controller_test.go | 2 + internal/controller/oidc_controller.go | 11 + internal/controller/oidc_controller_test.go | 225 ++++++++++++++++++ internal/controller/proxy_controller_test.go | 3 +- .../controller/resources_controller_test.go | 2 + internal/controller/user_controller_test.go | 3 +- .../controller/well_known_controller_test.go | 2 + internal/repository/models.go | 15 +- internal/repository/oidc_queries.sql.go | 39 +-- internal/service/oidc_service.go | 51 +++- internal/utils/tlog/log_wrapper.go | 11 + sql/oidc_queries.sql | 5 +- sql/oidc_schemas.sql | 3 +- sqlc.yml | 2 + 18 files changed, 350 insertions(+), 39 deletions(-) create mode 100644 internal/assets/migrations/000007_oidc_pkce.down.sql create mode 100644 internal/assets/migrations/000007_oidc_pkce.up.sql diff --git a/frontend/src/lib/hooks/oidc.ts b/frontend/src/lib/hooks/oidc.ts index a52b37b..6d9ca85 100644 --- a/frontend/src/lib/hooks/oidc.ts +++ b/frontend/src/lib/hooks/oidc.ts @@ -5,6 +5,8 @@ export type OIDCValues = { redirect_uri: string; state: string; nonce: string; + code_challenge: string; + code_challenge_method: string; }; interface IuseOIDCParams { @@ -14,7 +16,12 @@ interface IuseOIDCParams { missingParams: string[]; } -const optionalParams: string[] = ["state", "nonce"]; +const optionalParams: string[] = [ + "state", + "nonce", + "code_challenge", + "code_challenge_method", +]; export function useOIDCParams(params: URLSearchParams): IuseOIDCParams { let compiled: string = ""; @@ -28,6 +35,8 @@ export function useOIDCParams(params: URLSearchParams): IuseOIDCParams { redirect_uri: params.get("redirect_uri") ?? "", state: params.get("state") ?? "", nonce: params.get("nonce") ?? "", + code_challenge: params.get("code_challenge") ?? "", + code_challenge_method: params.get("code_challenge_method") ?? "", }; for (const key of Object.keys(values)) { diff --git a/internal/assets/migrations/000007_oidc_pkce.down.sql b/internal/assets/migrations/000007_oidc_pkce.down.sql new file mode 100644 index 0000000..a1d8cda --- /dev/null +++ b/internal/assets/migrations/000007_oidc_pkce.down.sql @@ -0,0 +1 @@ +ALTER TABLE "oidc_codes" DROP COLUMN "code_challenge"; diff --git a/internal/assets/migrations/000007_oidc_pkce.up.sql b/internal/assets/migrations/000007_oidc_pkce.up.sql new file mode 100644 index 0000000..485ee75 --- /dev/null +++ b/internal/assets/migrations/000007_oidc_pkce.up.sql @@ -0,0 +1 @@ +ALTER TABLE "oidc_codes" ADD COLUMN "code_challenge" TEXT DEFAULT ""; diff --git a/internal/controller/context_controller_test.go b/internal/controller/context_controller_test.go index 9a1ba45..3cff31c 100644 --- a/internal/controller/context_controller_test.go +++ b/internal/controller/context_controller_test.go @@ -10,10 +10,12 @@ import ( "github.com/steveiliop56/tinyauth/internal/config" "github.com/steveiliop56/tinyauth/internal/controller" "github.com/steveiliop56/tinyauth/internal/utils" + "github.com/steveiliop56/tinyauth/internal/utils/tlog" "github.com/stretchr/testify/assert" ) func TestContextController(t *testing.T) { + tlog.NewTestLogger().Init() controllerConfig := controller.ContextControllerConfig{ Providers: []controller.Provider{ { diff --git a/internal/controller/health_controller_test.go b/internal/controller/health_controller_test.go index c8cb36f..a12fd43 100644 --- a/internal/controller/health_controller_test.go +++ b/internal/controller/health_controller_test.go @@ -8,10 +8,12 @@ import ( "github.com/gin-gonic/gin" "github.com/steveiliop56/tinyauth/internal/controller" + "github.com/steveiliop56/tinyauth/internal/utils/tlog" "github.com/stretchr/testify/assert" ) func TestHealthController(t *testing.T) { + tlog.NewTestLogger().Init() tests := []struct { description string path string diff --git a/internal/controller/oidc_controller.go b/internal/controller/oidc_controller.go index 76b096d..81708a1 100644 --- a/internal/controller/oidc_controller.go +++ b/internal/controller/oidc_controller.go @@ -34,6 +34,7 @@ type TokenRequest struct { RefreshToken string `form:"refresh_token" url:"refresh_token"` ClientSecret string `form:"client_secret" url:"client_secret"` ClientID string `form:"client_id" url:"client_id"` + CodeVerifier string `form:"code_verifier" url:"code_verifier"` } type CallbackError struct { @@ -308,6 +309,16 @@ func (controller *OIDCController) Token(c *gin.Context) { return } + ok := controller.oidc.ValidatePKCE(entry.CodeChallenge, req.CodeVerifier) + + if !ok { + tlog.App.Warn().Msg("PKCE validation failed") + c.JSON(400, gin.H{ + "error": "invalid_grant", + }) + return + } + tokenRes, err := controller.oidc.GenerateAccessToken(c, client, entry) if err != nil { diff --git a/internal/controller/oidc_controller_test.go b/internal/controller/oidc_controller_test.go index c3943f7..a6c362d 100644 --- a/internal/controller/oidc_controller_test.go +++ b/internal/controller/oidc_controller_test.go @@ -1,6 +1,8 @@ package controller_test import ( + "crypto/sha256" + "encoding/base64" "encoding/json" "net/http/httptest" "net/url" @@ -15,11 +17,13 @@ import ( "github.com/steveiliop56/tinyauth/internal/controller" "github.com/steveiliop56/tinyauth/internal/repository" "github.com/steveiliop56/tinyauth/internal/service" + "github.com/steveiliop56/tinyauth/internal/utils/tlog" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) func TestOIDCController(t *testing.T) { + tlog.NewTestLogger().Init() tempDir := t.TempDir() oidcServiceCfg := service.OIDCServiceConfig{ @@ -431,6 +435,227 @@ func TestOIDCController(t *testing.T) { assert.False(t, ok, "Did not expect email claim in userinfo response") }, }, + { + description: "Ensure plain PKCE succeeds", + middlewares: []gin.HandlerFunc{ + simpleCtx, + }, + run: func(t *testing.T, router *gin.Engine, recorder *httptest.ResponseRecorder) { + reqBody := service.AuthorizeRequest{ + Scope: "openid", + ResponseType: "code", + ClientID: "some-client-id", + RedirectURI: "https://test.example.com/callback", + State: "some-state", + Nonce: "some-nonce", + CodeChallenge: "some-challenge", + // Not setting a code challenge method should default to "plain" + CodeChallengeMethod: "", + } + reqBodyBytes, err := json.Marshal(reqBody) + assert.NoError(t, err) + + req := httptest.NewRequest("POST", "/api/oidc/authorize", strings.NewReader(string(reqBodyBytes))) + req.Header.Set("Content-Type", "application/json") + router.ServeHTTP(recorder, req) + assert.Equal(t, 200, recorder.Code) + + var res map[string]any + err = json.Unmarshal(recorder.Body.Bytes(), &res) + assert.NoError(t, err) + + redirectURI := res["redirect_uri"].(string) + url, err := url.Parse(redirectURI) + assert.NoError(t, err) + + queryParams := url.Query() + assert.Equal(t, queryParams.Get("state"), "some-state") + + code := queryParams.Get("code") + assert.NotEmpty(t, code) + + // Now exchange the code for a token + recorder = httptest.NewRecorder() + tokenReqBody := controller.TokenRequest{ + GrantType: "authorization_code", + Code: code, + RedirectURI: "https://test.example.com/callback", + CodeVerifier: "some-challenge", + } + reqBodyEncoded, err := query.Values(tokenReqBody) + assert.NoError(t, err) + + req = httptest.NewRequest("POST", "/api/oidc/token", strings.NewReader(reqBodyEncoded.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.SetBasicAuth("some-client-id", "some-client-secret") + router.ServeHTTP(recorder, req) + + assert.Equal(t, 200, recorder.Code) + }, + }, + { + description: "Ensure S256 PKCE succeeds", + middlewares: []gin.HandlerFunc{ + simpleCtx, + }, + run: func(t *testing.T, router *gin.Engine, recorder *httptest.ResponseRecorder) { + hasher := sha256.New() + hasher.Write([]byte("some-challenge")) + codeChallenge := hasher.Sum(nil) + codeChallengeEncoded := base64.RawURLEncoding.EncodeToString(codeChallenge) + reqBody := service.AuthorizeRequest{ + Scope: "openid", + ResponseType: "code", + ClientID: "some-client-id", + RedirectURI: "https://test.example.com/callback", + State: "some-state", + Nonce: "some-nonce", + CodeChallenge: codeChallengeEncoded, + CodeChallengeMethod: "S256", + } + reqBodyBytes, err := json.Marshal(reqBody) + assert.NoError(t, err) + + req := httptest.NewRequest("POST", "/api/oidc/authorize", strings.NewReader(string(reqBodyBytes))) + req.Header.Set("Content-Type", "application/json") + router.ServeHTTP(recorder, req) + assert.Equal(t, 200, recorder.Code) + + var res map[string]any + err = json.Unmarshal(recorder.Body.Bytes(), &res) + assert.NoError(t, err) + + redirectURI := res["redirect_uri"].(string) + url, err := url.Parse(redirectURI) + assert.NoError(t, err) + + queryParams := url.Query() + assert.Equal(t, queryParams.Get("state"), "some-state") + + code := queryParams.Get("code") + assert.NotEmpty(t, code) + + // Now exchange the code for a token + recorder = httptest.NewRecorder() + tokenReqBody := controller.TokenRequest{ + GrantType: "authorization_code", + Code: code, + RedirectURI: "https://test.example.com/callback", + CodeVerifier: "some-challenge", + } + reqBodyEncoded, err := query.Values(tokenReqBody) + assert.NoError(t, err) + + req = httptest.NewRequest("POST", "/api/oidc/token", strings.NewReader(reqBodyEncoded.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.SetBasicAuth("some-client-id", "some-client-secret") + router.ServeHTTP(recorder, req) + + assert.Equal(t, 200, recorder.Code) + }, + }, + { + description: "Ensure request with invalid PKCE fails", + middlewares: []gin.HandlerFunc{ + simpleCtx, + }, + run: func(t *testing.T, router *gin.Engine, recorder *httptest.ResponseRecorder) { + hasher := sha256.New() + hasher.Write([]byte("some-challenge")) + codeChallenge := hasher.Sum(nil) + codeChallengeEncoded := base64.RawURLEncoding.EncodeToString(codeChallenge) + reqBody := service.AuthorizeRequest{ + Scope: "openid", + ResponseType: "code", + ClientID: "some-client-id", + RedirectURI: "https://test.example.com/callback", + State: "some-state", + Nonce: "some-nonce", + CodeChallenge: codeChallengeEncoded, + CodeChallengeMethod: "S256", + } + reqBodyBytes, err := json.Marshal(reqBody) + assert.NoError(t, err) + + req := httptest.NewRequest("POST", "/api/oidc/authorize", strings.NewReader(string(reqBodyBytes))) + req.Header.Set("Content-Type", "application/json") + router.ServeHTTP(recorder, req) + assert.Equal(t, 200, recorder.Code) + + var res map[string]any + err = json.Unmarshal(recorder.Body.Bytes(), &res) + assert.NoError(t, err) + + redirectURI := res["redirect_uri"].(string) + url, err := url.Parse(redirectURI) + assert.NoError(t, err) + + queryParams := url.Query() + assert.Equal(t, queryParams.Get("state"), "some-state") + + code := queryParams.Get("code") + assert.NotEmpty(t, code) + + // Now exchange the code for a token + recorder = httptest.NewRecorder() + tokenReqBody := controller.TokenRequest{ + GrantType: "authorization_code", + Code: code, + RedirectURI: "https://test.example.com/callback", + CodeVerifier: "some-challenge-1", + } + reqBodyEncoded, err := query.Values(tokenReqBody) + assert.NoError(t, err) + + req = httptest.NewRequest("POST", "/api/oidc/token", strings.NewReader(reqBodyEncoded.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.SetBasicAuth("some-client-id", "some-client-secret") + router.ServeHTTP(recorder, req) + + assert.Equal(t, 400, recorder.Code) + }, + }, + { + description: "Ensure request with invalid challenge method fails", + middlewares: []gin.HandlerFunc{ + simpleCtx, + }, + run: func(t *testing.T, router *gin.Engine, recorder *httptest.ResponseRecorder) { + hasher := sha256.New() + hasher.Write([]byte("some-challenge")) + codeChallenge := hasher.Sum(nil) + codeChallengeEncoded := base64.RawURLEncoding.EncodeToString(codeChallenge) + reqBody := service.AuthorizeRequest{ + Scope: "openid", + ResponseType: "code", + ClientID: "some-client-id", + RedirectURI: "https://test.example.com/callback", + State: "some-state", + Nonce: "some-nonce", + CodeChallenge: codeChallengeEncoded, + CodeChallengeMethod: "foo", + } + reqBodyBytes, err := json.Marshal(reqBody) + assert.NoError(t, err) + + req := httptest.NewRequest("POST", "/api/oidc/authorize", strings.NewReader(string(reqBodyBytes))) + req.Header.Set("Content-Type", "application/json") + router.ServeHTTP(recorder, req) + assert.Equal(t, 200, recorder.Code) + + var res map[string]any + err = json.Unmarshal(recorder.Body.Bytes(), &res) + assert.NoError(t, err) + + redirectURI := res["redirect_uri"].(string) + url, err := url.Parse(redirectURI) + assert.NoError(t, err) + + queryParams := url.Query() + error := queryParams.Get("error") + assert.NotEmpty(t, error) + }, + }, } app := bootstrap.NewBootstrapApp(config.Config{}) diff --git a/internal/controller/proxy_controller_test.go b/internal/controller/proxy_controller_test.go index c68d1d2..89e94de 100644 --- a/internal/controller/proxy_controller_test.go +++ b/internal/controller/proxy_controller_test.go @@ -17,6 +17,7 @@ import ( ) func TestProxyController(t *testing.T) { + tlog.NewTestLogger().Init() tempDir := t.TempDir() authServiceCfg := service.AuthServiceConfig{ @@ -390,8 +391,6 @@ func TestProxyController(t *testing.T) { }, } - tlog.NewSimpleLogger().Init() - oauthBrokerCfgs := make(map[string]config.OAuthServiceConfig) app := bootstrap.NewBootstrapApp(config.Config{}) diff --git a/internal/controller/resources_controller_test.go b/internal/controller/resources_controller_test.go index 2376aa9..2b89aa1 100644 --- a/internal/controller/resources_controller_test.go +++ b/internal/controller/resources_controller_test.go @@ -8,11 +8,13 @@ import ( "github.com/gin-gonic/gin" "github.com/steveiliop56/tinyauth/internal/controller" + "github.com/steveiliop56/tinyauth/internal/utils/tlog" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) func TestResourcesController(t *testing.T) { + tlog.NewTestLogger().Init() tempDir := t.TempDir() resourcesControllerCfg := controller.ResourcesControllerConfig{ diff --git a/internal/controller/user_controller_test.go b/internal/controller/user_controller_test.go index b4cff86..d69f930 100644 --- a/internal/controller/user_controller_test.go +++ b/internal/controller/user_controller_test.go @@ -22,6 +22,7 @@ import ( ) func TestUserController(t *testing.T) { + tlog.NewTestLogger().Init() tempDir := t.TempDir() authServiceCfg := service.AuthServiceConfig{ @@ -274,8 +275,6 @@ func TestUserController(t *testing.T) { }, } - tlog.NewSimpleLogger().Init() - oauthBrokerCfgs := make(map[string]config.OAuthServiceConfig) app := bootstrap.NewBootstrapApp(config.Config{}) diff --git a/internal/controller/well_known_controller_test.go b/internal/controller/well_known_controller_test.go index 20dc2a1..f956fda 100644 --- a/internal/controller/well_known_controller_test.go +++ b/internal/controller/well_known_controller_test.go @@ -13,11 +13,13 @@ import ( "github.com/steveiliop56/tinyauth/internal/controller" "github.com/steveiliop56/tinyauth/internal/repository" "github.com/steveiliop56/tinyauth/internal/service" + "github.com/steveiliop56/tinyauth/internal/utils/tlog" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) func TestWellKnownController(t *testing.T) { + tlog.NewTestLogger().Init() tempDir := t.TempDir() oidcServiceCfg := service.OIDCServiceConfig{ diff --git a/internal/repository/models.go b/internal/repository/models.go index 42da065..de6986d 100644 --- a/internal/repository/models.go +++ b/internal/repository/models.go @@ -5,13 +5,14 @@ package repository type OidcCode struct { - Sub string - CodeHash string - Scope string - RedirectURI string - ClientID string - ExpiresAt int64 - Nonce string + Sub string + CodeHash string + Scope string + RedirectURI string + ClientID string + ExpiresAt int64 + Nonce string + CodeChallenge string } type OidcToken struct { diff --git a/internal/repository/oidc_queries.sql.go b/internal/repository/oidc_queries.sql.go index 944eceb..7404d2b 100644 --- a/internal/repository/oidc_queries.sql.go +++ b/internal/repository/oidc_queries.sql.go @@ -17,21 +17,23 @@ INSERT INTO "oidc_codes" ( "redirect_uri", "client_id", "expires_at", - "nonce" + "nonce", + "code_challenge" ) VALUES ( - ?, ?, ?, ?, ?, ?, ? + ?, ?, ?, ?, ?, ?, ?, ? ) -RETURNING sub, code_hash, scope, redirect_uri, client_id, expires_at, nonce +RETURNING sub, code_hash, scope, redirect_uri, client_id, expires_at, nonce, code_challenge ` type CreateOidcCodeParams struct { - Sub string - CodeHash string - Scope string - RedirectURI string - ClientID string - ExpiresAt int64 - Nonce string + Sub string + CodeHash string + Scope string + RedirectURI string + ClientID string + ExpiresAt int64 + Nonce string + CodeChallenge string } func (q *Queries) CreateOidcCode(ctx context.Context, arg CreateOidcCodeParams) (OidcCode, error) { @@ -43,6 +45,7 @@ func (q *Queries) CreateOidcCode(ctx context.Context, arg CreateOidcCodeParams) arg.ClientID, arg.ExpiresAt, arg.Nonce, + arg.CodeChallenge, ) var i OidcCode err := row.Scan( @@ -53,6 +56,7 @@ func (q *Queries) CreateOidcCode(ctx context.Context, arg CreateOidcCodeParams) &i.ClientID, &i.ExpiresAt, &i.Nonce, + &i.CodeChallenge, ) return i, err } @@ -156,7 +160,7 @@ func (q *Queries) CreateOidcUserInfo(ctx context.Context, arg CreateOidcUserInfo const deleteExpiredOidcCodes = `-- name: DeleteExpiredOidcCodes :many DELETE FROM "oidc_codes" WHERE "expires_at" < ? -RETURNING sub, code_hash, scope, redirect_uri, client_id, expires_at, nonce +RETURNING sub, code_hash, scope, redirect_uri, client_id, expires_at, nonce, code_challenge ` func (q *Queries) DeleteExpiredOidcCodes(ctx context.Context, expiresAt int64) ([]OidcCode, error) { @@ -176,6 +180,7 @@ func (q *Queries) DeleteExpiredOidcCodes(ctx context.Context, expiresAt int64) ( &i.ClientID, &i.ExpiresAt, &i.Nonce, + &i.CodeChallenge, ); err != nil { return nil, err } @@ -286,7 +291,7 @@ func (q *Queries) DeleteOidcUserInfo(ctx context.Context, sub string) error { const getOidcCode = `-- name: GetOidcCode :one DELETE FROM "oidc_codes" WHERE "code_hash" = ? -RETURNING sub, code_hash, scope, redirect_uri, client_id, expires_at, nonce +RETURNING sub, code_hash, scope, redirect_uri, client_id, expires_at, nonce, code_challenge ` func (q *Queries) GetOidcCode(ctx context.Context, codeHash string) (OidcCode, error) { @@ -300,6 +305,7 @@ func (q *Queries) GetOidcCode(ctx context.Context, codeHash string) (OidcCode, e &i.ClientID, &i.ExpiresAt, &i.Nonce, + &i.CodeChallenge, ) return i, err } @@ -307,7 +313,7 @@ func (q *Queries) GetOidcCode(ctx context.Context, codeHash string) (OidcCode, e const getOidcCodeBySub = `-- name: GetOidcCodeBySub :one DELETE FROM "oidc_codes" WHERE "sub" = ? -RETURNING sub, code_hash, scope, redirect_uri, client_id, expires_at, nonce +RETURNING sub, code_hash, scope, redirect_uri, client_id, expires_at, nonce, code_challenge ` func (q *Queries) GetOidcCodeBySub(ctx context.Context, sub string) (OidcCode, error) { @@ -321,12 +327,13 @@ func (q *Queries) GetOidcCodeBySub(ctx context.Context, sub string) (OidcCode, e &i.ClientID, &i.ExpiresAt, &i.Nonce, + &i.CodeChallenge, ) return i, err } const getOidcCodeBySubUnsafe = `-- name: GetOidcCodeBySubUnsafe :one -SELECT sub, code_hash, scope, redirect_uri, client_id, expires_at, nonce FROM "oidc_codes" +SELECT sub, code_hash, scope, redirect_uri, client_id, expires_at, nonce, code_challenge FROM "oidc_codes" WHERE "sub" = ? ` @@ -341,12 +348,13 @@ func (q *Queries) GetOidcCodeBySubUnsafe(ctx context.Context, sub string) (OidcC &i.ClientID, &i.ExpiresAt, &i.Nonce, + &i.CodeChallenge, ) return i, err } const getOidcCodeUnsafe = `-- name: GetOidcCodeUnsafe :one -SELECT sub, code_hash, scope, redirect_uri, client_id, expires_at, nonce FROM "oidc_codes" +SELECT sub, code_hash, scope, redirect_uri, client_id, expires_at, nonce, code_challenge FROM "oidc_codes" WHERE "code_hash" = ? ` @@ -361,6 +369,7 @@ func (q *Queries) GetOidcCodeUnsafe(ctx context.Context, codeHash string) (OidcC &i.ClientID, &i.ExpiresAt, &i.Nonce, + &i.CodeChallenge, ) return i, err } diff --git a/internal/service/oidc_service.go b/internal/service/oidc_service.go index f732d4d..7990ef8 100644 --- a/internal/service/oidc_service.go +++ b/internal/service/oidc_service.go @@ -75,12 +75,14 @@ type TokenResponse struct { } type AuthorizeRequest struct { - Scope string `json:"scope" binding:"required"` - ResponseType string `json:"response_type" binding:"required"` - ClientID string `json:"client_id" binding:"required"` - RedirectURI string `json:"redirect_uri" binding:"required"` - State string `json:"state"` - Nonce string `json:"nonce"` + Scope string `json:"scope" binding:"required"` + ResponseType string `json:"response_type" binding:"required"` + ClientID string `json:"client_id" binding:"required"` + RedirectURI string `json:"redirect_uri" binding:"required"` + State string `json:"state"` + Nonce string `json:"nonce"` + CodeChallenge string `json:"code_challenge"` + CodeChallengeMethod string `json:"code_challenge_method"` } type OIDCServiceConfig struct { @@ -293,6 +295,13 @@ func (service *OIDCService) ValidateAuthorizeParams(req AuthorizeRequest) error return errors.New("invalid_request_uri") } + // PKCE code challenge method if set + if req.CodeChallenge != "" && req.CodeChallengeMethod != "" { + if req.CodeChallengeMethod != "S256" && req.CodeChallengeMethod != "plain" { + return errors.New("invalid_request") + } + } + return nil } @@ -306,8 +315,7 @@ func (service *OIDCService) StoreCode(c *gin.Context, sub string, code string, r // Fixed 10 minutes expiresAt := time.Now().Add(time.Minute * time.Duration(10)).Unix() - // Insert the code into the database - _, err := service.queries.CreateOidcCode(c, repository.CreateOidcCodeParams{ + entry := repository.CreateOidcCodeParams{ Sub: sub, CodeHash: service.Hash(code), // Here it's safe to split and trust the output since, we validated the scopes before @@ -316,7 +324,19 @@ func (service *OIDCService) StoreCode(c *gin.Context, sub string, code string, r ClientID: req.ClientID, ExpiresAt: expiresAt, Nonce: req.Nonce, - }) + } + + if req.CodeChallenge != "" { + if req.CodeChallengeMethod == "S256" { + entry.CodeChallenge = req.CodeChallenge + } else { + entry.CodeChallenge = service.hashAndEncodePKCE(req.CodeChallenge) + tlog.App.Warn().Msg("Received plain PKCE code challenge, it's recommended to use S256 for better security") + } + } + + // Insert the code into the database + _, err := service.queries.CreateOidcCode(c, entry) return err } @@ -728,3 +748,16 @@ func (service *OIDCService) GetJWK() ([]byte, error) { return jwk.Public().MarshalJSON() } + +func (service *OIDCService) ValidatePKCE(codeChallenge string, codeVerifier string) bool { + if codeChallenge == "" { + return true + } + return codeChallenge == service.hashAndEncodePKCE(codeVerifier) +} + +func (service *OIDCService) hashAndEncodePKCE(codeVerifier string) string { + hasher := sha256.New() + hasher.Write([]byte(codeVerifier)) + return base64.RawURLEncoding.EncodeToString(hasher.Sum(nil)) +} diff --git a/internal/utils/tlog/log_wrapper.go b/internal/utils/tlog/log_wrapper.go index ef24bf7..1b9c643 100644 --- a/internal/utils/tlog/log_wrapper.go +++ b/internal/utils/tlog/log_wrapper.go @@ -55,6 +55,17 @@ func NewSimpleLogger() *Logger { }) } +func NewTestLogger() *Logger { + return NewLogger(config.LogConfig{ + Level: "trace", + Streams: config.LogStreams{ + HTTP: config.LogStreamConfig{Enabled: true}, + App: config.LogStreamConfig{Enabled: true}, + Audit: config.LogStreamConfig{Enabled: true}, + }, + }) +} + func (l *Logger) Init() { Audit = l.Audit HTTP = l.HTTP diff --git a/sql/oidc_queries.sql b/sql/oidc_queries.sql index 096d0a7..0fb0261 100644 --- a/sql/oidc_queries.sql +++ b/sql/oidc_queries.sql @@ -6,9 +6,10 @@ INSERT INTO "oidc_codes" ( "redirect_uri", "client_id", "expires_at", - "nonce" + "nonce", + "code_challenge" ) VALUES ( - ?, ?, ?, ?, ?, ?, ? + ?, ?, ?, ?, ?, ?, ?, ? ) RETURNING *; diff --git a/sql/oidc_schemas.sql b/sql/oidc_schemas.sql index 822293e..4b61b39 100644 --- a/sql/oidc_schemas.sql +++ b/sql/oidc_schemas.sql @@ -5,7 +5,8 @@ CREATE TABLE IF NOT EXISTS "oidc_codes" ( "redirect_uri" TEXT NOT NULL, "client_id" TEXT NOT NULL, "expires_at" INTEGER NOT NULL, - "nonce" TEXT DEFAULT "" + "nonce" TEXT DEFAULT "", + "code_challenge" TEXT DEFAULT "" ); CREATE TABLE IF NOT EXISTS "oidc_tokens" ( diff --git a/sqlc.yml b/sqlc.yml index ac3572c..de08738 100644 --- a/sqlc.yml +++ b/sqlc.yml @@ -26,3 +26,5 @@ sql: go_type: "string" - column: "oidc_tokens.nonce" go_type: "string" + - column: "oidc_codes.code_challenge" + go_type: "string"