From 2ded017120cd8b158eded2618832719535f8e4ca Mon Sep 17 00:00:00 2001 From: Benjamin Knofe-Vider Date: Fri, 4 Sep 2026 14:26:31 +0200 Subject: [PATCH] fix(admin): verify AWS ALB OIDC JWT wire format --- controlplane/admin/README.md | 10 ++- controlplane/admin/alb_oidc.go | 48 ++++++++----- controlplane/admin/alb_oidc_test.go | 102 ++++++++++++++++++++-------- controlplane/admin/authz_test.go | 4 +- docs/design/admin-ui.md | 5 +- 5 files changed, 117 insertions(+), 52 deletions(-) diff --git a/controlplane/admin/README.md b/controlplane/admin/README.md index 6b21dfb0..92451c66 100644 --- a/controlplane/admin/README.md +++ b/controlplane/admin/README.md @@ -41,9 +41,13 @@ with a `Role`: `RoleGate` enforces the split: mutating verbs (POST/PUT/PATCH/DELETE) and the audit-log GET require admin; other GETs allow viewer. `AuditMiddleware` records -every mutation. The ALB OIDC JWT signature is currently trusted-by-network (the -internal LB is the only ingress and strips client copies); verifying it by `kid` -is a hardening follow-up (see the design doc). +every mutation. The ALB OIDC JWT's ES256 signature is verified against AWS's +regional public-key endpoint. Its expiry, signer region, configured issuer, and +configured client are validated from the protected header before payload user +claims are trusted. `DUCKGRES_ADMIN_SSO_ISSUER` enables SSO; when unset, SSO is +disabled and only bearer tokens authenticate. `DUCKGRES_ADMIN_SSO_CLIENT_ID` +pins the client when set. `DUCKGRES_ADMIN_SSO_REGION` selects the key endpoint +and signer region, defaulting to `DUCKGRES_AWS_REGION`. `?token=` URL auth is deliberately rejected (#721). diff --git a/controlplane/admin/alb_oidc.go b/controlplane/admin/alb_oidc.go index aa323c6c..9bed3c6f 100644 --- a/controlplane/admin/alb_oidc.go +++ b/controlplane/admin/alb_oidc.go @@ -13,6 +13,7 @@ import ( "encoding/pem" "fmt" "io" + "math/big" "net/http" "regexp" "strings" @@ -25,8 +26,9 @@ import ( // // The verifier checks the ES256 signature against the ALB's regional public // key. AWS publishes these keys at a fixed HTTPS endpoint per region. The -// verifier also checks the exp, signer, iss, and client claims. A request -// that never crossed the ALB cannot produce a JWT that passes these checks. +// verifier also checks the exp, signer, iss, and client protected-header +// fields. A request that never crossed the ALB cannot produce a JWT that +// passes these checks. // // This verification is the trust boundary for operator identity. Earlier // versions trusted the header without a signature check. That trusted the @@ -90,8 +92,12 @@ func (v *ALBOIDCVerifier) Verify(token string) (map[string]any, error) { } } var header struct { - Alg string `json:"alg"` - Kid string `json:"kid"` + Alg string `json:"alg"` + Kid string `json:"kid"` + Signer string `json:"signer"` + Issuer string `json:"iss"` + ClientID string `json:"client"` + Expires *int64 `json:"exp"` } if err := json.Unmarshal(headerBytes, &header); err != nil { return nil, errMalformedJWT @@ -118,42 +124,48 @@ func (v *ALBOIDCVerifier) Verify(token string) (map[string]any, error) { } } digest := sha256.Sum256([]byte(parts[0] + "." + parts[1])) - if !ecdsa.VerifyASN1(key, digest[:], sig) { + // JWS encodes an ES256 signature as the fixed-width concatenation R || S, + // with 32 bytes per integer. It is not an ASN.1 DER signature. + if len(sig) != 64 { return nil, &jwtError{"JWT signature verification failed"} } + r := new(big.Int).SetBytes(sig[:32]) + s := new(big.Int).SetBytes(sig[32:]) + if !ecdsa.Verify(key, digest[:], r, s) { + return nil, &jwtError{"JWT signature verification failed"} + } + if err := v.checkHeader(header.Expires, header.Signer, header.Issuer, header.ClientID); err != nil { + return nil, err + } claims, err := decodeJWTClaims(token) if err != nil { return nil, err } - if err := v.checkClaims(claims); err != nil { - return nil, err - } return claims, nil } -// checkClaims validates the registered claims of a verified JWT. -func (v *ALBOIDCVerifier) checkClaims(claims map[string]any) error { +// checkHeader validates the ALB metadata in the verified JWT's protected +// header. The payload contains only the user claims returned by the IdP. +func (v *ALBOIDCVerifier) checkHeader(exp *int64, signer, issuer, clientID string) error { // The ALB always sets exp. Reject tokens without it. - exp, ok := claims["exp"].(float64) - if !ok { + if exp == nil { return &jwtError{"missing exp claim"} } // Allow 60 seconds of clock skew between the ALB and this process. - if time.Unix(int64(exp), 0).Add(60 * time.Second).Before(v.now()) { + if time.Unix(*exp, 0).Add(60 * time.Second).Before(v.now()) { return &jwtError{"JWT expired"} } - // The signer claim names the ALB that signed the JWT. It must be an ALB - // in the configured region. This blocks JWTs that a different ALB signed. - signer := stringClaim(claims, "signer") + // The signer header field names the ALB that signed the JWT. Require an ALB + // ARN in the configured region. expectedPrefix := "arn:aws:elasticloadbalancing:" + v.region + ":" if !strings.HasPrefix(signer, expectedPrefix) { return &jwtError{"unexpected JWT signer"} } - if v.issuer != "" && stringClaim(claims, "iss") != v.issuer { + if v.issuer != "" && issuer != v.issuer { return &jwtError{"unexpected JWT issuer"} } - if v.clientID != "" && stringClaim(claims, "client") != v.clientID { + if v.clientID != "" && clientID != v.clientID { return &jwtError{"unexpected JWT client"} } return nil diff --git a/controlplane/admin/alb_oidc_test.go b/controlplane/admin/alb_oidc_test.go index 455c3e2b..628f677e 100644 --- a/controlplane/admin/alb_oidc_test.go +++ b/controlplane/admin/alb_oidc_test.go @@ -48,58 +48,94 @@ func testALBVerifier(t *testing.T, key *ecdsa.PrivateKey, issuer, clientID strin } v.keyURL = func(kid string) string { return srv.URL + "/" + url.PathEscape(kid) } v.httpClient = srv.Client() + v.now = func() time.Time { return testALBNow } return v } -// signedOIDC builds an ES256-signed JWT with the given claims, in the -// X-Amzn-Oidc-Data wire format (header.payload.signature). +// signedOIDC builds an AWS ALB X-Amzn-Oidc-Data JWT. ALB security metadata +// lives in the protected header, while the payload contains only user claims. func signedOIDC(t *testing.T, key *ecdsa.PrivateKey, claims map[string]any) string { t.Helper() - headerSeg := base64.RawURLEncoding.EncodeToString([]byte(`{"alg":"ES256","kid":"test-kid","typ":"JWT"}`)) - payload, err := json.Marshal(claims) + return signedOIDCWithHeader(t, key, validALBHeader(nil), claims) +} + +func signedOIDCWithHeader(t *testing.T, key *ecdsa.PrivateKey, header, claims map[string]any) string { + t.Helper() + signingInput := oidcSigningInput(t, header, claims) + digest := sha256.Sum256([]byte(signingInput)) + r, s, err := ecdsa.Sign(rand.Reader, key, digest[:]) if err != nil { - t.Fatalf("marshal claims: %v", err) + t.Fatalf("sign test JWT: %v", err) } - payloadSeg := base64.RawURLEncoding.EncodeToString(payload) - digest := sha256.Sum256([]byte(headerSeg + "." + payloadSeg)) + sig := make([]byte, 64) + r.FillBytes(sig[:32]) + s.FillBytes(sig[32:]) + return signingInput + "." + base64.URLEncoding.EncodeToString(sig) +} + +func signedOIDCASN1(t *testing.T, key *ecdsa.PrivateKey, header, claims map[string]any) string { + t.Helper() + signingInput := oidcSigningInput(t, header, claims) + digest := sha256.Sum256([]byte(signingInput)) sig, err := ecdsa.SignASN1(rand.Reader, key, digest[:]) if err != nil { t.Fatalf("sign test JWT: %v", err) } - return headerSeg + "." + payloadSeg + "." + base64.RawURLEncoding.EncodeToString(sig) + return signingInput + "." + base64.URLEncoding.EncodeToString(sig) +} + +func oidcSigningInput(t *testing.T, header, claims map[string]any) string { + t.Helper() + headerJSON, err := json.Marshal(header) + if err != nil { + t.Fatalf("marshal header: %v", err) + } + payloadJSON, err := json.Marshal(claims) + if err != nil { + t.Fatalf("marshal claims: %v", err) + } + // AWS ALB uses padded base64url segments. + return base64.URLEncoding.EncodeToString(headerJSON) + "." + base64.URLEncoding.EncodeToString(payloadJSON) } -// validALBClaims returns claims that pass the verifier's claim checks for a -// verifier built with issuer testALBIssuer and clientID testALBClientID. -// Callers override individual keys to build rejection cases. const ( testALBIssuer = "https://cognito-idp.us-east-1.amazonaws.com/us-east-1_TEST" testALBClientID = "test-client-id" + testALBSigner = "arn:aws:elasticloadbalancing:us-east-1:123456789012:loadbalancer/app/test/abc" ) -func validALBClaims(overrides map[string]any) map[string]any { - claims := map[string]any{ - "exp": float64(time.Now().Add(time.Hour).Unix()), - "signer": "arn:aws:elasticloadbalancing:us-east-1:123456789012:loadbalancer/internal/test/abc", +var testALBNow = time.Date(2026, time.September, 4, 12, 0, 0, 0, time.UTC) + +// validALBHeader returns the protected header emitted by AWS ALB. Callers +// override individual fields to build rejection cases. +func validALBHeader(overrides map[string]any) map[string]any { + header := map[string]any{ + "alg": "ES256", + "kid": "test-kid", + "signer": testALBSigner, "iss": testALBIssuer, "client": testALBClientID, + "exp": testALBNow.Add(time.Hour).Unix(), } for k, v := range overrides { - claims[k] = v + header[k] = v } - return claims + return header } func TestALBOIDCVerifierAcceptsValidToken(t *testing.T) { key := testALBSigningKey(t) v := testALBVerifier(t, key, testALBIssuer, testALBClientID) - claims, err := v.Verify(signedOIDC(t, key, validALBClaims(map[string]any{"email": "a@posthog.com"}))) + claims, err := v.Verify(signedOIDC(t, key, map[string]any{"email": "a@posthog.com"})) if err != nil { t.Fatalf("Verify: %v", err) } if claims["email"] != "a@posthog.com" { t.Fatalf("email = %v", claims["email"]) } + if _, ok := claims["exp"]; ok { + t.Fatal("Verify returned protected-header metadata as a payload claim") + } } func TestALBOIDCVerifierRejections(t *testing.T) { @@ -107,13 +143,21 @@ func TestALBOIDCVerifierRejections(t *testing.T) { otherKey := testALBSigningKey(t) v := testALBVerifier(t, key, testALBIssuer, testALBClientID) - wrongSig := signedOIDC(t, otherKey, validALBClaims(nil)) + wrongSig := signedOIDC(t, otherKey, map[string]any{"email": "a@posthog.com"}) - tampered := signedOIDC(t, key, validALBClaims(nil)) + tampered := signedOIDC(t, key, map[string]any{"email": "a@posthog.com"}) // Replace the payload with attacker claims, keep the original signature. - forgedPayload, _ := json.Marshal(validALBClaims(map[string]any{"email": "admin@posthog.com", "role": "admin"})) + forgedPayload, _ := json.Marshal(map[string]any{"email": "admin@posthog.com", "role": "admin"}) parts := splitForTest(t, tampered) - tampered = parts[0] + "." + base64.RawURLEncoding.EncodeToString(forgedPayload) + "." + parts[2] + tampered = parts[0] + "." + base64.URLEncoding.EncodeToString(forgedPayload) + "." + parts[2] + + missingExpHeader := validALBHeader(nil) + delete(missingExpHeader, "exp") + metadataInPayload := validALBHeader(nil) + metadataInPayload["email"] = "a@posthog.com" + noUserClaims := map[string]any{} + validParts := splitForTest(t, signedOIDC(t, key, map[string]any{"email": "a@posthog.com"})) + shortSignature := validParts[0] + "." + validParts[1] + "." + base64.URLEncoding.EncodeToString(make([]byte, 63)) cases := []struct { name string @@ -122,11 +166,15 @@ func TestALBOIDCVerifierRejections(t *testing.T) { {"signature from wrong key", wrongSig}, {"tampered payload", tampered}, {"unsigned legacy token", mkUnsignedOIDC(map[string]any{"email": "a@posthog.com"})}, - {"expired", signedOIDC(t, key, validALBClaims(map[string]any{"exp": float64(time.Now().Add(-time.Hour).Unix())}))}, - {"missing exp", signedOIDC(t, key, map[string]any{"signer": "arn:aws:elasticloadbalancing:us-east-1:123456789012:loadbalancer/internal/test/abc", "iss": testALBIssuer, "client": testALBClientID})}, - {"signer in another region", signedOIDC(t, key, validALBClaims(map[string]any{"signer": "arn:aws:elasticloadbalancing:eu-central-1:123456789012:loadbalancer/internal/test/abc"}))}, - {"wrong issuer", signedOIDC(t, key, validALBClaims(map[string]any{"iss": "https://cognito-idp.us-east-1.amazonaws.com/us-east-1_OTHER"}))}, - {"wrong client", signedOIDC(t, key, validALBClaims(map[string]any{"client": "other-client"}))}, + {"ASN.1 signature", signedOIDCASN1(t, key, validALBHeader(nil), map[string]any{"email": "a@posthog.com"})}, + {"short JWS signature", shortSignature}, + {"expired", signedOIDCWithHeader(t, key, validALBHeader(map[string]any{"exp": testALBNow.Add(-time.Hour).Unix()}), noUserClaims)}, + {"missing exp", signedOIDCWithHeader(t, key, missingExpHeader, noUserClaims)}, + {"non-integer exp", signedOIDCWithHeader(t, key, validALBHeader(map[string]any{"exp": "not-a-number"}), noUserClaims)}, + {"metadata only in payload", signedOIDCWithHeader(t, key, map[string]any{"alg": "ES256", "kid": "test-kid"}, metadataInPayload)}, + {"signer in another region", signedOIDCWithHeader(t, key, validALBHeader(map[string]any{"signer": "arn:aws:elasticloadbalancing:eu-central-1:123456789012:loadbalancer/app/test/abc"}), noUserClaims)}, + {"wrong issuer", signedOIDCWithHeader(t, key, validALBHeader(map[string]any{"iss": "https://cognito-idp.us-east-1.amazonaws.com/us-east-1_OTHER"}), noUserClaims)}, + {"wrong client", signedOIDCWithHeader(t, key, validALBHeader(map[string]any{"client": "other-client"}), noUserClaims)}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { diff --git a/controlplane/admin/authz_test.go b/controlplane/admin/authz_test.go index a59427d3..5569f383 100644 --- a/controlplane/admin/authz_test.go +++ b/controlplane/admin/authz_test.go @@ -38,7 +38,7 @@ func newTestSSO(t *testing.T) *testSSO { // claim overrides (typically "email"). func (s *testSSO) oidc(t *testing.T, overrides map[string]any) string { t.Helper() - return signedOIDC(t, s.key, validALBClaims(overrides)) + return signedOIDC(t, s.key, overrides) } // roleByEmail builds a fake RoleResolver from an email→role map. Unknown @@ -159,7 +159,7 @@ func TestAuthMiddlewareRejectsForgedSSO(t *testing.T) { value string }{ {"unsigned data JWT", albOIDCDataHeader, mkUnsignedOIDC(map[string]any{"email": "a@posthog.com"})}, - {"wrong-key signature", albOIDCDataHeader, signedOIDC(t, otherKey, validALBClaims(map[string]any{"email": "a@posthog.com"}))}, + {"wrong-key signature", albOIDCDataHeader, signedOIDC(t, otherKey, map[string]any{"email": "a@posthog.com"})}, {"identity-only header", "X-Amzn-Oidc-Identity", "a@posthog.com"}, } for _, tc := range cases { diff --git a/docs/design/admin-ui.md b/docs/design/admin-ui.md index 8fc64f2f..bbdc43df 100644 --- a/docs/design/admin-ui.md +++ b/docs/design/admin-ui.md @@ -65,8 +65,9 @@ the proxy is not an open PromQL relay. Org-labelled metrics we expose: ### 3. RBAC + audit — `authz.go`, `audit.go` - `AuthMiddleware`: verify `x-amzn-oidc-data` (ALB-signed JWT) via the ALB regional - public-key endpoint (`alb_oidc.go`, keys cached per `kid`) — signature, `exp`, - `signer` (must be an ALB ARN in the configured region), `iss` + public-key endpoint (`alb_oidc.go`, keys cached per `kid`) — the JOSE ES256 + signature plus protected-header fields `exp`, `signer` (must be an ALB ARN in + the configured region), `iss` (`DUCKGRES_ADMIN_SSO_ISSUER`), `client` (`DUCKGRES_ADMIN_SSO_CLIENT_ID`). The unsigned `x-amzn-oidc-identity` header is never trusted. Without `DUCKGRES_ADMIN_SSO_ISSUER` the SSO path is off and only bearer tokens