From 9a6676b0542fd52a2a3cf9ac3fc361fe2bdac257 Mon Sep 17 00:00:00 2001 From: Stavros Date: Mon, 6 Apr 2026 23:09:14 +0300 Subject: [PATCH] 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 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..2e9bab8 --- /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 0000000..2b711f4 --- /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 76b096d..2e53b1b 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 42da065..79cb8ec 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 944eceb..0d3b7ab 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 f732d4d..24a76cd 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 096d0a7..4238779 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 822293e..762df59 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 ac3572c..278f9ce 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"