Skip to content

feat(proxy): add saml_init for proactive SAML SSO login#33

Merged
rjeffman merged 6 commits into
LeGambiArt:mainfrom
decko:feat/saml-init
May 20, 2026
Merged

feat(proxy): add saml_init for proactive SAML SSO login#33
rjeffman merged 6 commits into
LeGambiArt:mainfrom
decko:feat/saml-init

Conversation

@decko
Copy link
Copy Markdown
Collaborator

@decko decko commented May 19, 2026

Summary

Problem

The reactive SAML SSO from #32 triggers on 401/403 + text/html responses. Some sites (e.g. Igloo-based platforms) never return 401/403 — they silently degrade to anonymous sessions with empty results. The only way to authenticate is to explicitly initiate the SAML redirect flow.

Solution

New saml_init field in the auth config:

services:
  auth:
    type: kerberos
    spnego_proactive: false
    saml_init: "/saml.redirect?relayState="

When set, the core proactively follows the SAML flow during plugin startup:

  1. GET the saml_init URL
  2. Parse SAMLRequest form, POST to IdP (SPNEGORoundTripper handles Kerberos)
  3. Parse SAMLResponse, POST back to origin server
  4. Cookie jar is authenticated before any tool calls

Handles two patterns:

  • Form-first: init URL returns a SAMLRequest form directly (Igloo, etc.)
  • Redirect-first: delegates to the existing handleSAMLSSO for meta-refresh redirects (Jenkins, etc.)

SAML init failure is fatal — the plugin won't load, preventing silent degradation to empty results.

Changes

  • internal/plugin/manifest.go — add SAMLInit field to AuthServiceConfig
  • internal/proxy/saml.go — add InitSAMLSession + followSAMLFormChain
  • internal/proxy/saml_test.go — 5 new tests (form-first, redirect-first, no content, blocked domain, absolute URL)
  • internal/plugin/manager.go — call InitSAMLSession during preparePlugin if configured

Test plan

  • All existing SAML tests pass (6/6)
  • New InitSAMLSession tests pass (5/5)
  • Full test suite passes (go test ./...)
  • Live test against an Igloo-based site with saml_init configured

🤖 Generated with Claude Code

rjeffman and others added 4 commits May 19, 2026 16:49
Update golang.org/x packages to fix HTTP/2 vulnerability (GO-2026-4918).

- golang.org/x/crypto: v0.48.0 -> v0.50.0
- golang.org/x/net: v0.51.0 -> v0.53.0 (fixes infinite loop in HTTP/2)
- golang.org/x/sync: v0.19.0 -> v0.20.0
- golang.org/x/sys: v0.41.0 -> v0.43.0
- golang.org/x/term: v0.40.0 -> v0.42.0
- golang.org/x/text: v0.34.0 -> v0.36.0

Assisted-by: Claude Sonnet 4.5 <noreply@anthropic.com>
Signed-off-by: Rafael Guterres Jeffman <rjeffman@redhat.com>
Enable Kerberos/SPNEGO authentication for plugins with automatic SAML
SSO redirect handling. Supports dynamic SPN derivation from request
hostnames and enhanced variable resolution for flexible configuration.

- Add KerberosProvider with dynamic SPN support (derives from hostname)
- Implement SAML POST binding detection and auto-replay for SSO flows
- Add |hostname modifier and nested defaults to config var resolver
- Enable per-request domain allowlisting for SSO redirects
- Initialize specialized Kerberos HTTP client in plugin manager

The SAML handler automatically follows meta-refresh and form-based
redirects, extracts hidden fields, and POSTs back to originating
server to complete SSO authentication flows.

Assisted-by: Claude Sonnet 4.5 <noreply@anthropic.com>
Signed-off-by: Rafael Guterres Jeffman <rjeffman@redhat.com>
… validation

Fixed critical vulnerability in proxy per-request domain registration that
allowed credential theft to attacker-controlled servers. Implemented defense-
in-depth approach with multiple security layers.

Security fixes:
- CRITICAL: Enforce subdomain validation - per-request domains must be
  subdomains of base domains declared in plugin.yaml or init_ok
- HIGH: Add cumulative domain cap (50 total per plugin) to prevent allowlist
  bloat and DoS via domain accumulation across multiple requests
- LOW: Normalize domains (trailing dots, case, Unicode/IDNA) to prevent
  bypass via homoglyphs, punycode, or formatting variations

Implementation:
- Created internal/domaincheck package with shared validation logic for
  consistent security checks across init-time and per-request paths
- Added BaseDomains field to PluginAuth to track trusted anchor domains
- Modified Execute() to validate domains against base domains before adding
- Updated AddAllowedDomains() to enforce cumulative domain cap
- Deduplicated validation code from proxy and plugin packages

Attack scenarios prevented:
- Direct exfiltration: jenkins_url=https://attacker.com/ → rejected
- Subdomain spoofing: eviljenkins.com → rejected (not subdomain)
- Allowlist flooding: 100 requests × 10 domains → capped at 50
- Unicode bypass: attacker․com (Unicode period) → normalized, validated
- Trailing dot duplicates: evil.com and evil.com. → deduplicated

Test coverage:
- 19 subdomain validation test cases
- Cumulative cap enforcement tests
- Unicode/IDNA normalization tests
- All existing proxy and plugin tests passing

Assisted-by: Claude Sonnet 4.5 <noreply@anthropic.com>
Signed-off-by: Rafael Guterres Jeffman <rjeffman@redhat.com>
… startup

Adds a `saml_init` field to the auth config that tells the core to
proactively authenticate via SAML during plugin initialization. This
supports sites that silently degrade to anonymous sessions instead of
returning 401/403 challenges.

The InitSAMLSession function handles two SAML patterns:
- Form-first: GET init URL returns a SAMLRequest form, POST to IdP,
  POST SAMLResponse back to origin (3-step)
- Redirect-first: delegates to the existing handleSAMLSSO for sites
  that use meta-refresh redirects to the IdP

SAML init failure is fatal — the plugin will not load if auth fails,
preventing silent degradation to empty/anonymous results.

Co-Authored-By: André "decko" de Brito <decko@redhat.com>
Co-Authored-By: Sergio Correia <scorreia@redhat.com>
Co-Authored-By: Rafael Guterres Jeffman <rjeffman@redhat.com>
@decko decko force-pushed the feat/saml-init branch 2 times, most recently from 9c1d256 to aebb1b6 Compare May 19, 2026 22:56
@rjeffman
Copy link
Copy Markdown
Collaborator

@decko reviewed with some agents, and this is what they find and I agree we should at least discuss:

BLOCKING Issues - Must Fix Before Merge

  1. isDomainAllowed Normalization Inconsistency (CRITICAL)

File: internal/proxy/proxy.go, lines 694-696
Consensus: All 4 agents agree this is critical

Issue: isDomainAllowed uses strings.EqualFold while isDomainAllowedForSSO and all other
domain validation uses domaincheck.Contains with IDNA normalization.

Impact: Domains with trailing dots, Unicode characters, or punycode could bypass validation
at the primary request gate while being accepted/rejected differently by other validation
paths.

Fix: Replace the strings.EqualFold loop with domaincheck.Contains:

  // Change this:
  for _, domain := range pa.AllowedDomains {
      if strings.EqualFold(reqHost, domain) {
          return true
      }
  }
  // To this:
  return domaincheck.Contains(pa.AllowedDomains, reqHost)
  1. AddBaseDomains Missing Cap (MEDIUM - Security)

File: internal/proxy/proxy.go, lines 175-193
Security-engineer: Blocks merge. Other agents concur.

Issue: AddBaseDomains doesn't enforce maxTotalDomains cap, while AddAllowedDomains does.
A malicious plugin could flood base domains and bypass subdomain restrictions.

Fix: Add the same cap check that exists in AddAllowedDomains:

  if len(pa.AllowedDomains) >= maxTotalDomains {
      log.Printf("[%s] WARNING: domain allowlist cap reached (%d domains), rejecting %q",
          pluginName, maxTotalDomains, d)
      continue
  }

NON-BLOCKING Issues - Acceptable to Defer

  1. HTML Parsing via Regex

Consensus: Safe due to domain validation, but may fail against some real IdPs

  • Security-engineer: H3 (Warning) - "mitigated by domain validation"
  • Rogue-reviewer: Concedes it's safe security-wise, but will cause functional failures
  • Code-reviewer: Upgraded to "Critical for functionality" but safe security-wise

Recommendation: Track as high-priority follow-up to replace with golang.org/x/net/html for
better IdP compatibility. Not merge-blocking because domain validation provides independent
security control.

Minor issues

  • context.Background() in InitSAMLSession - should use timeout (HIGH priority follow-up)

F1 (CRITICAL): Prevent SPNEGO token leakage on cross-domain redirects
by disabling automatic redirect following during SAML flows. All SAML
HTTP calls now use a no-redirect client copy.

F2 (HIGH): Fix hiddenInputRe requiring fixed attribute ordering. Parse
<input> tags by extracting all attributes into a map, then check for
type=hidden. Works with any attribute order.

F3 (HIGH): Use domaincheck.Contains for normalized domain comparison
in isDomainAllowedForSSO, preventing bypasses via trailing dots, IDNA,
or case variations.

F4 (HIGH): Gate request replay in trySAMLSSO on idempotency. SSO flow
still runs (to establish cookies), but non-idempotent requests are not
replayed after authentication.

F5 (MEDIUM): Check :- before :+ in resolveVarExpression to prevent
misparsing when :+ appears literally in a :- default value.

F6 (MEDIUM): Only count ${ (not bare {) as depth increment in
resolveVarsFunc, matching findTopLevelPattern behavior.

F7 (MEDIUM): Thread context through all SAML HTTP requests via
NewRequestWithContext for proper cancellation on shutdown.

F8 (MEDIUM): Resolve relative action URLs in parseSAMLForm by
prepending baseURL, preventing isDomainAllowedForSSO from rejecting
scheme-less URLs.

F9 (MEDIUM): Return false for non-SAML 200 responses in handleSAMLSSO
instead of true, preventing unnecessary request retries.

Co-Authored-By: André "decko" de Brito <decko@redhat.com>
Co-Authored-By: Sergio Correia <scorreia@redhat.com>
Co-Authored-By: Rafael Guterres Jeffman <rjeffman@redhat.com>
@decko decko force-pushed the feat/saml-init branch from aebb1b6 to b1b44d5 Compare May 20, 2026 00:15
Replace regex-based extractRedirectURL and parseSAMLForm with a proper
HTML tokenizer. This correctly handles any attribute ordering, malformed
HTML, and edge cases that regex patterns miss.

Also normalizes baseHost comparison in isDomainAllowedForSSO using
domaincheck.Contains (V7 fix).

Promotes golang.org/x/net from indirect to direct dependency.

Co-Authored-By: André "decko" de Brito <decko@redhat.com>
Co-Authored-By: Sergio Correia <scorreia@redhat.com>
Co-Authored-By: Rafael Guterres Jeffman <rjeffman@redhat.com>
@rjeffman
Copy link
Copy Markdown
Collaborator

LGTM.

@rjeffman rjeffman merged commit c8524d4 into LeGambiArt:main May 20, 2026
3 checks passed
@decko decko deleted the feat/saml-init branch May 20, 2026 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants