feat(saml): SAML 2.0 IdP core — metadata, AuthnRequest, signed assertions (#255)#272
feat(saml): SAML 2.0 IdP core — metadata, AuthnRequest, signed assertions (#255)#272iarunsaragadam wants to merge 1 commit into
Conversation
…d assertions Add a pkg/samlidp SAML 2.0 Identity Provider following the pluggable-provider pattern: an Issuer interface (MetadataProvider + AssertionIssuer) with a no-op default and an RSA-signing implementation. The RSA issuer generates the IdP EntityDescriptor metadata (signing cert + SSO/SLO endpoints), parses inbound SAML AuthnRequests, and mints enveloped-signature SAML assertions (RSA-SHA256 over SHA-256 digests) wrapped in a SAML Response addressed to the SP ACS URL. ACS falls back to the registered value on mismatch to prevent assertion open-redirect; the request issuer is validated against the SP. Gate behind GATEWAY_SAML_IDP_ENABLED with entityID/SSO/key/cert config and a Validate() invariant that fails closed when enabled-but-incomplete. Wire a buildSAMLIssuer selector in internal/app and mount the /saml/metadata HTTP surface only when the issuer is enabled, so a disabled deployment returns 404 with unchanged behavior. Tests cover the no-op, RSA construction/validation (incl. key/cert mismatch), metadata content, AuthnRequest parse success/failure, and full cryptographic verification of the produced assertion signature, plus config validation and the gate-on/gate-off HTTP surface.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // immediately after the <saml:Issuer> element (the position xmldsig | ||
| // schema-validation requires for SAML assertions). | ||
| func (i *RSAIssuer) signAssertion(assertionID, assertion string) (string, error) { | ||
| digest := sha256.Sum256([]byte(assertion)) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a6264f679
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if err != nil { | ||
| return nil, fmt.Errorf("saml issuer: %w", err) | ||
| } | ||
| (&samlHandler{issuer: samlIssuer, logger: logger}).register(mux) |
There was a problem hiding this comment.
In the built server this route is registered on the same mux that is later wrapped by SessionAuthMiddleware/AuthMiddleware, but /saml/metadata is not included in isAuthExempt. When SAML is enabled, an SP importing the IdP metadata without a gateway JWT will receive 401 before samlHandler.metadata runs, so the SP cannot fetch the signing certificate needed to trust the IdP.
Useful? React with 👍 / 👎.
| // immediately after the <saml:Issuer> element (the position xmldsig | ||
| // schema-validation requires for SAML assertions). | ||
| func (i *RSAIssuer) signAssertion(assertionID, assertion string) (string, error) { | ||
| digest := sha256.Sum256([]byte(assertion)) |
There was a problem hiding this comment.
For any response verified by a standard XML-DSig implementation, this hashes the literal serialized assertion even though the declared transform is exclusive canonicalization. Verifiers remove the enveloped signature and canonicalize the assertion before recomputing DigestValue (for example, empty elements like SubjectConfirmationData/> are serialized differently), so the digest and signature produced here will not match what SPs verify; the current test only replays the same raw string path and misses this interop failure.
Useful? React with 👍 / 👎.
| fmt.Fprintf(&b, `<md:SingleLogoutService Binding=%q Location=%q/>`, bindingPOST, i.sloURL) | ||
| } | ||
| fmt.Fprintf(&b, `<md:NameIDFormat>%s</md:NameIDFormat>`, nameIDFormatEmail) | ||
| fmt.Fprintf(&b, `<md:SingleSignOnService Binding=%q Location=%q/>`, bindingRedirect, i.ssoURL) |
There was a problem hiding this comment.
When the configured SSO URL contains characters that are legal in URLs but special in XML attributes, such as & in a query string, %q emits the raw ampersand rather than XML-escaping it. That makes the generated metadata not well-formed for common endpoint shapes like https://idp.example/sso?tenant=a&env=b, preventing SPs from importing it; the same manual attribute-writing pattern should use an XML encoder or XML attribute escaping.
Useful? React with 👍 / 👎.
iarunsaragadam
left a comment
iarunsaragadam
left a comment
There was a problem hiding this comment.
PR review gate: ❌ BLOCKED
| Reviewer | Effective verdict | Surviving blockers |
|---|---|---|
| Product | REQUEST_CHANGES | 1 |
| Security | REQUEST_CHANGES | 1 |
| Performance | APPROVE | 0 |
| Maintainability | APPROVE | 0 |
| Correctness | REQUEST_CHANGES | 1 |
Conditional reviewers: Contract & Migration: skipped (no proto/API/schema/migration changes). Accessibility & UX: skipped (no UI changes).
Confirmed blockers
Security & Correctness — XML attribute values emitted via %q are not XML-escaped (malformed / injectable signed assertion)
- Location:
pkg/samlidp/issue.go:116-144,186-189;pkg/samlidp/rsa.go:23-33(Metadata);pkg/samlidp/util.go(missing attribute escaper). - Both Security and Correctness independently confirmed this. Every XML attribute value (
InResponseTo,Format,Recipient,Destination, attributeName,entityID,Location, …) is interpolated withfmt's%q, which applies Go source-string quoting, not XML attribute escaping:&/</>pass through bare and"becomes a backslash-quote that prematurely closes the attribute.escape()(util.go) is correct but only applied to element text. - The load-bearing path is attacker/legitimately-controlled: the inbound AuthnRequest
@IDis echoed verbatim intoInResponseToon both the signed assertion (issue.go:123) and the response (issue.go:186).ParseAuthnRequest(rsa.go:150-172) validates only non-emptiness, not the XML NCName/ID grammar. Reproduced:req.ID = _x"><evil>&yields a Response that failsxml.Unmarshal("invalid character entity & (no semicolon)"). SAML IDs decoded from inbound XML may legitimately contain&(as&), so this breaks real SSO flows, and at lenient SP parsers it injects raw</>/&structure into the signed security token. - Fix: add an attribute-value escaper (escape
&,<,>,",', plus tab/newline/CR per XML attr-value normalization) and use it for ALL interpolated attribute values instead of%q— or build the document withencoding/xmlstructs /xml.EscapeTextso escaping cannot be forgotten. Additionally validatereq.IDagainst the XML NCName/ID grammar inParseAuthnRequestand reject non-conforming IDs. Add regression tests that round-trip areq.IDand attribute names containing",<,>,&throughIssue()and assert the produced Response re-parses as well-formed XML andverifyAssertionSignaturestill passes.
Product — Enabled gate advertises SSO/SLO endpoints that return 404 (broken operator experience)
- Location:
internal/app/saml_http.go:29-34;pkg/samlidp/issue.go:20-35. - When
GATEWAY_SAML_IDP_ENABLEDis on,samlHandler.registermounts only/saml/metadata. The published EntityDescriptor advertisesSingleSignOnService(atcfg.SAMLSSOURL, HTTP-POST + HTTP-Redirect) and, when configured,SingleLogoutService(cfg.SAMLSLOURL) — neither of which is served anywhere (no SSO/SLO handler;ParseAuthnRequest/Issuehave no production caller). An operator who sets the env vars and hands/saml/metadatato Okta/Azure/Google gets a clean metadata import followed by a 404 on first login, with no signal this is expected. The flag presents as a complete SAML IdP but only metadata serving works. - Fix: either (a) do not advertise
SingleSignOnService/SingleLogoutServicein the EntityDescriptor (or gate metadata behind a sub-flag) until the SSO/SLO bindings are actually mounted, so published metadata never points an SP at a 404; or (b) hold this slice until the interactive SSO POST/Redirect handler lands so "enabled" means a usable IdP. If metadata-only is intentional, surface the limitation operator-facing (clear "metadata-only, SSO binding not yet available" boot warning + docs), not just in the PR body.
Non-blocking findings
- Security (minor): Digest is computed over hand-built bytes with no real exclusive-C14N; the in-repo "signature verifies" test re-digests the same raw bytes, so it can't detect canonicalization drift and SP interop is unproven (issue.go:152-159). Run the serialized assertion through a real exc-C14N impl before digesting, or verify against a third-party SAML library.
- Security (nit): AuthnRequest signature is not verified (
WantAuthnRequestsSigned=false, issue.go:24; rsa.go:150-172). Acceptable for the constrained bearer-assertion model, but track a follow-up to support signed AuthnRequests before exposing the interactive SSO binding. - Performance (nit):
Metadata()rebuilds identical immutable XML on every request (issue.go:18); optionally precompute once inNewRSAIssuer. Micro-optimization on a low-frequency endpoint — fine as-is. - Maintainability (major, non-blocking): Inconsistent escaping discipline + a
util.gocomment that wrongly claims%q"escapes for double-quoted attributes" (util.go:30-40; issue.go:23,116-140,188). Consolidate to one attribute escaper and fix/delete the misleading comment. (Overlaps the confirmed blocker above.) - Maintainability (minor): No
Issue/Metadatatest feeds special characters (<,&, quote) through the attribute path; onlyTestEscapeexercisesescape()in isolation. Add one. (Covered by the blocker's required regression test.) - Maintainability (nit): Const-block doc comment references nonexistent
ProviderName(constants areProviderNoop/ProviderRSA) — samlidp.go:26-33. - Maintainability (nit): Nested single-branch
ifinIssue(issue.go:53-60) can be flattened;acs := sp.ACSURLdefault already covers empty/mismatch cases. - Correctness (minor):
Issue()doesn't require non-emptyreq.ID; an empty ID emitsInResponseTo=""rather than omitting the attribute (issue.go:41-50,94). Only reachable from direct callers (ParseAuthnRequestrejects empty IDs). Require non-empty for SP-initiated or omit the attribute when empty.
Recommendation
Block on the merge gate (advisory): the slice's signing/protocol core, fail-closed construction, ACS open-redirect fallback, and gate-off behavior are clean and well-tested, but two real defects must land first. The %q-instead-of-XML-escaping bug (confirmed by both Security and Correctness) corrupts the core signed auth primitive on legitimate input and is the priority fix — add a single attribute escaper, validate req.ID, and add round-trip regression tests. Second, stop advertising unmounted SSO/SLO endpoints (or scope the flag/messaging) so enabling the feature doesn't hand operators a metadata import that 404s on first login. The non-blocking C14N/interop gap and the missing special-char tests should be addressed alongside the escaping fix since they share the same code path.
Generated by the PR review gate (AGENTS.md §11) — 5 always-on reviewers + conditional Contract/Accessibility, with adversarial verification.
2 participants
Closes #255
Summary
Lands the SAML 2.0 Identity Provider core as a complete, tested, additive slice following the pluggable-provider pattern (interface + impl + no-op default, config-gated, wired in
internal/app).What's included (complete + tested)
pkg/samlidp—Issuerinterface (MetadataProvider+AssertionIssuer):NoopIssuerdefault (returnsErrDisabled; never enabled).RSAIssuer: IdPEntityDescriptormetadata (signing cert + HTTP-POST/Redirect SSO + optional SLO), inboundAuthnRequestparsing, and enveloped-signature SAML assertions (RSA-SHA256 / SHA-256 digest) wrapped in a SAMLResponseto the SP ACS URL.Issuervalidated against the SP; key/cert pair verified at construction (fail-closed).GATEWAY_SAML_IDP_ENABLED+GATEWAY_SAML_ENTITY_ID/_SSO_URL/_SLO_URL/_SIGNING_KEY/_SIGNING_CERT, with aValidate()invariant that fails closed when enabled-but-incomplete.buildSAMLIssuerselector +/saml/metadataHTTP surface mounted only when enabled; disabled deployments return 404, behavior unchanged.Acceptance criteria status
Deferred (follow-ups, noted per the "largest complete slice" rule)
The interactive browser SSO POST/Redirect binding requires bridging to the SSO-completion session path (mint first-party
method=ssotokens), and the persisted SP-connection store + conformance across memory/postgres/sqlite plus additive lifecycle RPCs are sizeable. The signing/protocol core they depend on is landed and verified here; those layers can build onsamlidp.Issuer+ServiceProviderwithout rework. No migration was added in this slice.🤖 Generated with Claude Code