From 9a6676b0542fd52a2a3cf9ac3fc361fe2bdac257 Mon Sep 17 00:00:00 2001 From: Stavros Date: Mon, 6 Apr 2026 23:09:14 +0300 Subject: [PATCH 1/7] feat: add pkce support to oidc server --- frontend/src/lib/hooks/oidc.ts | 11 +++- .../migrations/000007_oidc_pkce.down.sql | 2 + .../assets/migrations/000007_oidc_pkce.up.sql | 2 + internal/controller/oidc_controller.go | 11 ++++ internal/repository/models.go | 16 +++--- internal/repository/oidc_queries.sql.go | 48 +++++++++++----- internal/service/oidc_service.go | 57 ++++++++++++++++--- sql/oidc_queries.sql | 6 +- sql/oidc_schemas.sql | 4 +- sqlc.yml | 4 ++ 10 files changed, 126 insertions(+), 35 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 a52b37b5..6d9ca854 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 00000000..2e9bab8d --- /dev/null +++ b/internal/assets/migrations/000007_oidc_pkce.down.sql @@ -0,0 +1,2 @@ +ALTER TABLE "oidc_codes" DROP COLUMN "code_challenge"; +ALTER TABLE "oidc_codes" DROP COLUMN "code_challenge_method"; 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 00000000..2b711f4b --- /dev/null +++ b/internal/assets/migrations/000007_oidc_pkce.up.sql @@ -0,0 +1,2 @@ +ALTER TABLE "oidc_codes" ADD COLUMN "code_challenge" TEXT DEFAULT ""; +ALTER TABLE "oidc_codes" ADD COLUMN "code_challenge_method" TEXT DEFAULT ""; diff --git a/internal/controller/oidc_controller.go b/internal/controller/oidc_controller.go index 76b096d2..2e53b1b8 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, entry.CodeChallengeMethod, 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/repository/models.go b/internal/repository/models.go index 42da065a..79cb8eca 100644 --- a/internal/repository/models.go +++ b/internal/repository/models.go @@ -5,13 +5,15 @@ 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 + CodeChallengeMethod string } type OidcToken struct { diff --git a/internal/repository/oidc_queries.sql.go b/internal/repository/oidc_queries.sql.go index 944ecebe..0d3b7ab8 100644 --- a/internal/repository/oidc_queries.sql.go +++ b/internal/repository/oidc_queries.sql.go @@ -17,21 +17,25 @@ INSERT INTO "oidc_codes" ( "redirect_uri", "client_id", "expires_at", - "nonce" + "nonce", + "code_challenge", + "code_challenge_method" ) 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, code_challenge_method ` 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 + CodeChallengeMethod string } func (q *Queries) CreateOidcCode(ctx context.Context, arg CreateOidcCodeParams) (OidcCode, error) { @@ -43,6 +47,8 @@ func (q *Queries) CreateOidcCode(ctx context.Context, arg CreateOidcCodeParams) arg.ClientID, arg.ExpiresAt, arg.Nonce, + arg.CodeChallenge, + arg.CodeChallengeMethod, ) var i OidcCode err := row.Scan( @@ -53,6 +59,8 @@ func (q *Queries) CreateOidcCode(ctx context.Context, arg CreateOidcCodeParams) &i.ClientID, &i.ExpiresAt, &i.Nonce, + &i.CodeChallenge, + &i.CodeChallengeMethod, ) return i, err } @@ -156,7 +164,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, code_challenge_method ` func (q *Queries) DeleteExpiredOidcCodes(ctx context.Context, expiresAt int64) ([]OidcCode, error) { @@ -176,6 +184,8 @@ func (q *Queries) DeleteExpiredOidcCodes(ctx context.Context, expiresAt int64) ( &i.ClientID, &i.ExpiresAt, &i.Nonce, + &i.CodeChallenge, + &i.CodeChallengeMethod, ); err != nil { return nil, err } @@ -286,7 +296,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, code_challenge_method ` func (q *Queries) GetOidcCode(ctx context.Context, codeHash string) (OidcCode, error) { @@ -300,6 +310,8 @@ func (q *Queries) GetOidcCode(ctx context.Context, codeHash string) (OidcCode, e &i.ClientID, &i.ExpiresAt, &i.Nonce, + &i.CodeChallenge, + &i.CodeChallengeMethod, ) return i, err } @@ -307,7 +319,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, code_challenge_method ` func (q *Queries) GetOidcCodeBySub(ctx context.Context, sub string) (OidcCode, error) { @@ -321,12 +333,14 @@ func (q *Queries) GetOidcCodeBySub(ctx context.Context, sub string) (OidcCode, e &i.ClientID, &i.ExpiresAt, &i.Nonce, + &i.CodeChallenge, + &i.CodeChallengeMethod, ) 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, code_challenge_method FROM "oidc_codes" WHERE "sub" = ? ` @@ -341,12 +355,14 @@ func (q *Queries) GetOidcCodeBySubUnsafe(ctx context.Context, sub string) (OidcC &i.ClientID, &i.ExpiresAt, &i.Nonce, + &i.CodeChallenge, + &i.CodeChallengeMethod, ) 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, code_challenge_method FROM "oidc_codes" WHERE "code_hash" = ? ` @@ -361,6 +377,8 @@ func (q *Queries) GetOidcCodeUnsafe(ctx context.Context, codeHash string) (OidcC &i.ClientID, &i.ExpiresAt, &i.Nonce, + &i.CodeChallenge, + &i.CodeChallengeMethod, ) return i, err } diff --git a/internal/service/oidc_service.go b/internal/service/oidc_service.go index f732d4d8..24a76cdd 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.CodeChallenge == "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,21 @@ 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 + entry.CodeChallengeMethod = "S256" + } else { + entry.CodeChallenge = service.hashAndEncodePKCE(req.CodeChallenge) + entry.CodeChallengeMethod = "plain" + 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 +750,20 @@ func (service *OIDCService) GetJWK() ([]byte, error) { return jwk.Public().MarshalJSON() } + +func (service *OIDCService) ValidatePKCE(codeChallenge string, codeChallengeMethod string, codeVerifier string) bool { + if codeChallenge == "" { + return true + } + if codeChallengeMethod == "plain" { + // Code challenge is hashed and encoded in the database for security reasons + return codeChallenge == service.hashAndEncodePKCE(codeVerifier) + } + return codeChallenge == codeVerifier +} + +func (service *OIDCService) hashAndEncodePKCE(codeVerifier string) string { + hasher := sha256.New() + hasher.Write([]byte(codeVerifier)) + return base64.URLEncoding.EncodeToString(hasher.Sum(nil)) +} diff --git a/sql/oidc_queries.sql b/sql/oidc_queries.sql index 096d0a77..42387799 100644 --- a/sql/oidc_queries.sql +++ b/sql/oidc_queries.sql @@ -6,9 +6,11 @@ INSERT INTO "oidc_codes" ( "redirect_uri", "client_id", "expires_at", - "nonce" + "nonce", + "code_challenge", + "code_challenge_method" ) VALUES ( - ?, ?, ?, ?, ?, ?, ? + ?, ?, ?, ?, ?, ?, ?, ?, ? ) RETURNING *; diff --git a/sql/oidc_schemas.sql b/sql/oidc_schemas.sql index 822293e9..762df59e 100644 --- a/sql/oidc_schemas.sql +++ b/sql/oidc_schemas.sql @@ -5,7 +5,9 @@ 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 "", + "code_challenge_method" TEXT DEFAULT "" ); CREATE TABLE IF NOT EXISTS "oidc_tokens" ( diff --git a/sqlc.yml b/sqlc.yml index ac3572c8..278f9cef 100644 --- a/sqlc.yml +++ b/sqlc.yml @@ -26,3 +26,7 @@ sql: go_type: "string" - column: "oidc_tokens.nonce" go_type: "string" + - column: "oidc_codes.code_challenge" + go_type: "string" + - column: "oidc_codes.code_challenge_method" + go_type: "string" From 5bada13919e732fee7145716646072bd07641608 Mon Sep 17 00:00:00 2001 From: Stavros Date: Tue, 7 Apr 2026 17:49:42 +0300 Subject: [PATCH 2/7] tests: add test cases for pkce --- internal/controller/oidc_controller_test.go | 179 ++++++++++++++++++++ 1 file changed, 179 insertions(+) diff --git a/internal/controller/oidc_controller_test.go b/internal/controller/oidc_controller_test.go index c3943f7b..4f7b7079 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" @@ -431,6 +433,183 @@ 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 + 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.URLEncoding.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 + 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.URLEncoding.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 + 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, 200, recorder.Code) + }, + }, } app := bootstrap.NewBootstrapApp(config.Config{}) From e451b3d62f446da257788dbeaa0b9ef99fe0f552 Mon Sep 17 00:00:00 2001 From: Stavros Date: Tue, 7 Apr 2026 18:27:45 +0300 Subject: [PATCH 3/7] fix: review comments --- .../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 | 3 +- internal/controller/oidc_controller_test.go | 11 +++-- internal/controller/proxy_controller_test.go | 1 + .../controller/resources_controller_test.go | 2 + internal/controller/user_controller_test.go | 1 + .../controller/well_known_controller_test.go | 2 + internal/repository/models.go | 17 ++++---- internal/repository/oidc_queries.sql.go | 41 ++++++++----------- internal/service/oidc_service.go | 14 ++----- internal/utils/tlog/log_wrapper.go | 11 +++++ sql/oidc_queries.sql | 5 +-- sql/oidc_schemas.sql | 3 +- sqlc.yml | 2 - 17 files changed, 62 insertions(+), 57 deletions(-) diff --git a/internal/assets/migrations/000007_oidc_pkce.down.sql b/internal/assets/migrations/000007_oidc_pkce.down.sql index 2e9bab8d..a1d8cda2 100644 --- a/internal/assets/migrations/000007_oidc_pkce.down.sql +++ b/internal/assets/migrations/000007_oidc_pkce.down.sql @@ -1,2 +1 @@ ALTER TABLE "oidc_codes" DROP COLUMN "code_challenge"; -ALTER TABLE "oidc_codes" DROP COLUMN "code_challenge_method"; diff --git a/internal/assets/migrations/000007_oidc_pkce.up.sql b/internal/assets/migrations/000007_oidc_pkce.up.sql index 2b711f4b..485ee75e 100644 --- a/internal/assets/migrations/000007_oidc_pkce.up.sql +++ b/internal/assets/migrations/000007_oidc_pkce.up.sql @@ -1,2 +1 @@ ALTER TABLE "oidc_codes" ADD COLUMN "code_challenge" TEXT DEFAULT ""; -ALTER TABLE "oidc_codes" ADD COLUMN "code_challenge_method" TEXT DEFAULT ""; diff --git a/internal/controller/context_controller_test.go b/internal/controller/context_controller_test.go index 9a1ba453..3cff31c6 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 c8cb36f3..a12fd437 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 2e53b1b8..9ad0fa9e 100644 --- a/internal/controller/oidc_controller.go +++ b/internal/controller/oidc_controller.go @@ -309,7 +309,8 @@ func (controller *OIDCController) Token(c *gin.Context) { return } - ok := controller.oidc.ValidatePKCE(entry.CodeChallenge, entry.CodeChallengeMethod, req.CodeVerifier) + tlog.App.Debug().Str("challenge", entry.CodeChallenge).Str("verifier", req.CodeVerifier).Msg("Validating PKCE") + ok := controller.oidc.ValidatePKCE(entry.CodeChallenge, req.CodeVerifier) if !ok { tlog.App.Warn().Msg("PKCE validation failed") diff --git a/internal/controller/oidc_controller_test.go b/internal/controller/oidc_controller_test.go index 4f7b7079..06bd2890 100644 --- a/internal/controller/oidc_controller_test.go +++ b/internal/controller/oidc_controller_test.go @@ -17,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{ @@ -473,6 +475,7 @@ func TestOIDCController(t *testing.T) { assert.NotEmpty(t, code) // Now exchange the code for a token + recorder = httptest.NewRecorder() tokenReqBody := controller.TokenRequest{ GrantType: "authorization_code", Code: code, @@ -499,7 +502,7 @@ func TestOIDCController(t *testing.T) { hasher := sha256.New() hasher.Write([]byte("some-challenge")) codeChallenge := hasher.Sum(nil) - codeChallengeEncoded := base64.URLEncoding.EncodeToString(codeChallenge) + codeChallengeEncoded := base64.RawURLEncoding.EncodeToString(codeChallenge) reqBody := service.AuthorizeRequest{ Scope: "openid", ResponseType: "code", @@ -533,6 +536,7 @@ func TestOIDCController(t *testing.T) { assert.NotEmpty(t, code) // Now exchange the code for a token + recorder = httptest.NewRecorder() tokenReqBody := controller.TokenRequest{ GrantType: "authorization_code", Code: code, @@ -559,7 +563,7 @@ func TestOIDCController(t *testing.T) { hasher := sha256.New() hasher.Write([]byte("some-challenge")) codeChallenge := hasher.Sum(nil) - codeChallengeEncoded := base64.URLEncoding.EncodeToString(codeChallenge) + codeChallengeEncoded := base64.RawURLEncoding.EncodeToString(codeChallenge) reqBody := service.AuthorizeRequest{ Scope: "openid", ResponseType: "code", @@ -593,6 +597,7 @@ func TestOIDCController(t *testing.T) { assert.NotEmpty(t, code) // Now exchange the code for a token + recorder = httptest.NewRecorder() tokenReqBody := controller.TokenRequest{ GrantType: "authorization_code", Code: code, @@ -607,7 +612,7 @@ func TestOIDCController(t *testing.T) { req.SetBasicAuth("some-client-id", "some-client-secret") router.ServeHTTP(recorder, req) - assert.Equal(t, 200, recorder.Code) + assert.Equal(t, 400, recorder.Code) }, }, } diff --git a/internal/controller/proxy_controller_test.go b/internal/controller/proxy_controller_test.go index c68d1d26..f3f203e2 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{ diff --git a/internal/controller/resources_controller_test.go b/internal/controller/resources_controller_test.go index 2376aa9f..2b89aa10 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 b4cff861..2db51059 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{ diff --git a/internal/controller/well_known_controller_test.go b/internal/controller/well_known_controller_test.go index 20dc2a12..f956fda5 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 79cb8eca..de6986de 100644 --- a/internal/repository/models.go +++ b/internal/repository/models.go @@ -5,15 +5,14 @@ package repository type OidcCode struct { - Sub string - CodeHash string - Scope string - RedirectURI string - ClientID string - ExpiresAt int64 - Nonce string - CodeChallenge string - CodeChallengeMethod 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 0d3b7ab8..7404d2bd 100644 --- a/internal/repository/oidc_queries.sql.go +++ b/internal/repository/oidc_queries.sql.go @@ -18,24 +18,22 @@ INSERT INTO "oidc_codes" ( "client_id", "expires_at", "nonce", - "code_challenge", - "code_challenge_method" + "code_challenge" ) VALUES ( - ?, ?, ?, ?, ?, ?, ?, ?, ? + ?, ?, ?, ?, ?, ?, ?, ? ) -RETURNING sub, code_hash, scope, redirect_uri, client_id, expires_at, nonce, code_challenge, code_challenge_method +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 - CodeChallenge string - CodeChallengeMethod 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) { @@ -48,7 +46,6 @@ func (q *Queries) CreateOidcCode(ctx context.Context, arg CreateOidcCodeParams) arg.ExpiresAt, arg.Nonce, arg.CodeChallenge, - arg.CodeChallengeMethod, ) var i OidcCode err := row.Scan( @@ -60,7 +57,6 @@ func (q *Queries) CreateOidcCode(ctx context.Context, arg CreateOidcCodeParams) &i.ExpiresAt, &i.Nonce, &i.CodeChallenge, - &i.CodeChallengeMethod, ) return i, err } @@ -164,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, code_challenge, code_challenge_method +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) { @@ -185,7 +181,6 @@ func (q *Queries) DeleteExpiredOidcCodes(ctx context.Context, expiresAt int64) ( &i.ExpiresAt, &i.Nonce, &i.CodeChallenge, - &i.CodeChallengeMethod, ); err != nil { return nil, err } @@ -296,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, code_challenge, code_challenge_method +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) { @@ -311,7 +306,6 @@ func (q *Queries) GetOidcCode(ctx context.Context, codeHash string) (OidcCode, e &i.ExpiresAt, &i.Nonce, &i.CodeChallenge, - &i.CodeChallengeMethod, ) return i, err } @@ -319,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, code_challenge, code_challenge_method +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) { @@ -334,13 +328,12 @@ func (q *Queries) GetOidcCodeBySub(ctx context.Context, sub string) (OidcCode, e &i.ExpiresAt, &i.Nonce, &i.CodeChallenge, - &i.CodeChallengeMethod, ) return i, err } const getOidcCodeBySubUnsafe = `-- name: GetOidcCodeBySubUnsafe :one -SELECT sub, code_hash, scope, redirect_uri, client_id, expires_at, nonce, code_challenge, code_challenge_method FROM "oidc_codes" +SELECT sub, code_hash, scope, redirect_uri, client_id, expires_at, nonce, code_challenge FROM "oidc_codes" WHERE "sub" = ? ` @@ -356,13 +349,12 @@ func (q *Queries) GetOidcCodeBySubUnsafe(ctx context.Context, sub string) (OidcC &i.ExpiresAt, &i.Nonce, &i.CodeChallenge, - &i.CodeChallengeMethod, ) return i, err } const getOidcCodeUnsafe = `-- name: GetOidcCodeUnsafe :one -SELECT sub, code_hash, scope, redirect_uri, client_id, expires_at, nonce, code_challenge, code_challenge_method FROM "oidc_codes" +SELECT sub, code_hash, scope, redirect_uri, client_id, expires_at, nonce, code_challenge FROM "oidc_codes" WHERE "code_hash" = ? ` @@ -378,7 +370,6 @@ func (q *Queries) GetOidcCodeUnsafe(ctx context.Context, codeHash string) (OidcC &i.ExpiresAt, &i.Nonce, &i.CodeChallenge, - &i.CodeChallengeMethod, ) return i, err } diff --git a/internal/service/oidc_service.go b/internal/service/oidc_service.go index 24a76cdd..7990ef81 100644 --- a/internal/service/oidc_service.go +++ b/internal/service/oidc_service.go @@ -297,7 +297,7 @@ func (service *OIDCService) ValidateAuthorizeParams(req AuthorizeRequest) error // PKCE code challenge method if set if req.CodeChallenge != "" && req.CodeChallengeMethod != "" { - if req.CodeChallengeMethod != "S256" || req.CodeChallenge == "plain" { + if req.CodeChallengeMethod != "S256" && req.CodeChallengeMethod != "plain" { return errors.New("invalid_request") } } @@ -329,10 +329,8 @@ func (service *OIDCService) StoreCode(c *gin.Context, sub string, code string, r if req.CodeChallenge != "" { if req.CodeChallengeMethod == "S256" { entry.CodeChallenge = req.CodeChallenge - entry.CodeChallengeMethod = "S256" } else { entry.CodeChallenge = service.hashAndEncodePKCE(req.CodeChallenge) - entry.CodeChallengeMethod = "plain" tlog.App.Warn().Msg("Received plain PKCE code challenge, it's recommended to use S256 for better security") } } @@ -751,19 +749,15 @@ func (service *OIDCService) GetJWK() ([]byte, error) { return jwk.Public().MarshalJSON() } -func (service *OIDCService) ValidatePKCE(codeChallenge string, codeChallengeMethod string, codeVerifier string) bool { +func (service *OIDCService) ValidatePKCE(codeChallenge string, codeVerifier string) bool { if codeChallenge == "" { return true } - if codeChallengeMethod == "plain" { - // Code challenge is hashed and encoded in the database for security reasons - return codeChallenge == service.hashAndEncodePKCE(codeVerifier) - } - return codeChallenge == codeVerifier + return codeChallenge == service.hashAndEncodePKCE(codeVerifier) } func (service *OIDCService) hashAndEncodePKCE(codeVerifier string) string { hasher := sha256.New() hasher.Write([]byte(codeVerifier)) - return base64.URLEncoding.EncodeToString(hasher.Sum(nil)) + return base64.RawURLEncoding.EncodeToString(hasher.Sum(nil)) } diff --git a/internal/utils/tlog/log_wrapper.go b/internal/utils/tlog/log_wrapper.go index ef24bf77..1b9c643a 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 42387799..0fb02613 100644 --- a/sql/oidc_queries.sql +++ b/sql/oidc_queries.sql @@ -7,10 +7,9 @@ INSERT INTO "oidc_codes" ( "client_id", "expires_at", "nonce", - "code_challenge", - "code_challenge_method" + "code_challenge" ) VALUES ( - ?, ?, ?, ?, ?, ?, ?, ?, ? + ?, ?, ?, ?, ?, ?, ?, ? ) RETURNING *; diff --git a/sql/oidc_schemas.sql b/sql/oidc_schemas.sql index 762df59e..4b61b39f 100644 --- a/sql/oidc_schemas.sql +++ b/sql/oidc_schemas.sql @@ -6,8 +6,7 @@ CREATE TABLE IF NOT EXISTS "oidc_codes" ( "client_id" TEXT NOT NULL, "expires_at" INTEGER NOT NULL, "nonce" TEXT DEFAULT "", - "code_challenge" TEXT DEFAULT "", - "code_challenge_method" TEXT DEFAULT "" + "code_challenge" TEXT DEFAULT "" ); CREATE TABLE IF NOT EXISTS "oidc_tokens" ( diff --git a/sqlc.yml b/sqlc.yml index 278f9cef..de08738a 100644 --- a/sqlc.yml +++ b/sqlc.yml @@ -28,5 +28,3 @@ sql: go_type: "string" - column: "oidc_codes.code_challenge" go_type: "string" - - column: "oidc_codes.code_challenge_method" - go_type: "string" From 482b3c6b573e033ab74d20bb7c2096cdd91e7010 Mon Sep 17 00:00:00 2001 From: Stavros Date: Tue, 7 Apr 2026 18:28:28 +0300 Subject: [PATCH 4/7] chore: remove debug line --- internal/controller/oidc_controller.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/controller/oidc_controller.go b/internal/controller/oidc_controller.go index 9ad0fa9e..81708a17 100644 --- a/internal/controller/oidc_controller.go +++ b/internal/controller/oidc_controller.go @@ -309,7 +309,6 @@ func (controller *OIDCController) Token(c *gin.Context) { return } - tlog.App.Debug().Str("challenge", entry.CodeChallenge).Str("verifier", req.CodeVerifier).Msg("Validating PKCE") ok := controller.oidc.ValidatePKCE(entry.CodeChallenge, req.CodeVerifier) if !ok { From 668348655f1d76172666077aef53599e8ab7ec22 Mon Sep 17 00:00:00 2001 From: Stavros Date: Tue, 7 Apr 2026 18:30:05 +0300 Subject: [PATCH 5/7] chore: remove simple logger from testing --- internal/controller/proxy_controller_test.go | 2 -- internal/controller/user_controller_test.go | 2 -- 2 files changed, 4 deletions(-) diff --git a/internal/controller/proxy_controller_test.go b/internal/controller/proxy_controller_test.go index f3f203e2..89e94de3 100644 --- a/internal/controller/proxy_controller_test.go +++ b/internal/controller/proxy_controller_test.go @@ -391,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/user_controller_test.go b/internal/controller/user_controller_test.go index 2db51059..d69f9304 100644 --- a/internal/controller/user_controller_test.go +++ b/internal/controller/user_controller_test.go @@ -275,8 +275,6 @@ func TestUserController(t *testing.T) { }, } - tlog.NewSimpleLogger().Init() - oauthBrokerCfgs := make(map[string]config.OAuthServiceConfig) app := bootstrap.NewBootstrapApp(config.Config{}) From 6ffb52a5cd540c558c49bc09496c131cc757ec58 Mon Sep 17 00:00:00 2001 From: Stavros Date: Tue, 7 Apr 2026 18:48:38 +0300 Subject: [PATCH 6/7] tests: add test for invalid challenge method --- internal/controller/oidc_controller_test.go | 41 +++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/internal/controller/oidc_controller_test.go b/internal/controller/oidc_controller_test.go index 06bd2890..cfb031ee 100644 --- a/internal/controller/oidc_controller_test.go +++ b/internal/controller/oidc_controller_test.go @@ -615,6 +615,47 @@ func TestOIDCController(t *testing.T) { 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() + code := queryParams.Get("error") + assert.NotEmpty(t, code) + }, + }, } app := bootstrap.NewBootstrapApp(config.Config{}) From def9e5aaaa2d1781b3d1a438ae591035b4dbbdc2 Mon Sep 17 00:00:00 2001 From: Stavros Date: Tue, 7 Apr 2026 19:00:09 +0300 Subject: [PATCH 7/7] chore: fix typo --- internal/controller/oidc_controller_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/controller/oidc_controller_test.go b/internal/controller/oidc_controller_test.go index cfb031ee..a6c362d7 100644 --- a/internal/controller/oidc_controller_test.go +++ b/internal/controller/oidc_controller_test.go @@ -652,8 +652,8 @@ func TestOIDCController(t *testing.T) { assert.NoError(t, err) queryParams := url.Query() - code := queryParams.Get("error") - assert.NotEmpty(t, code) + error := queryParams.Get("error") + assert.NotEmpty(t, error) }, }, }