Improve SAMLResponse validation in SSO callbacks#47463
Conversation
lucasmrod
force-pushed
the
16371-improve-sso-callback-validation
branch
from
38e0517 to
c7dea28
Compare
There was a problem hiding this comment.
Pull request overview
This PR hardens Fleet’s unauthenticated SSO callback endpoints against pre-auth resource-exhaustion attacks by adding size limits, rate limiting, and SAMLResponse XML shape validation before signature verification/canonicalization.
Changes:
- Added XML depth/element-count validation for decoded
SAMLResponsebefore passing it tocrewjam/saml/goxmldsig. - Added a shared max-size constant for SSO callback payloads and enforced it at both HTTP body and form value levels (including query-string bypass).
- Applied login-bucket rate limiting + request body size limits to the regular and MDM SSO callback routes.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| server/sso/authorization_response.go | Adds SAMLResponse XML shape validation (depth + element count) prior to signature/cert processing. |
| server/sso/authorization_response_test.go | Adds unit tests covering valid/invalid XML and depth/width limit enforcement. |
| server/service/sessions.go | Enforces a max size on SAMLResponse form value (covers POST body and query string). |
| server/service/sessions_test.go | Adds tests ensuring oversized query-string SAMLResponse is rejected and normal payloads pass. |
| server/service/handler.go | Adds rate limiting + request body size limits to SSO callback endpoints; refactors limiter setup for reuse. |
| server/fleet/request.go | Introduces MaxSSOCallbackSize constant (256 KiB) for SSO callback endpoints. |
| changes/improve-sso-samlresponse-validation | Documents the user-visible security hardening changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughThis PR hardens SSO SAMLResponse validation across three validation layers. It introduces a 256 KiB size limit for SSO callback payloads and validates XML structure (nesting depth and element count) before signature verification to reject pathological SAML documents early. Request-level validation checks SAMLResponse parameter size in the callback decoder. HTTP enforcement adds rate limiting with separate buckets for login and MDM SSO endpoints, applies body size caps to both SSO callback routes, and refactors the rate-limiting middleware setup. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #47463 +/- ##
==========================================
- Coverage 67.20% 67.20% -0.01%
==========================================
Files 3350 3352 +2
Lines 228159 228264 +105
Branches 11751 11751
==========================================
+ Hits 153342 153411 +69
- Misses 60991 61027 +36
Partials 13826 13826
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/fleet/serve.go`:
- Around line 1024-1026: The SSORateLimitPerMinute check currently skips
creating the SSO limiter when config.Auth.SSORateLimitPerMinute == 0; update the
logic so a limiter is applied by default: either change the condition in
serve.go to treat 0 as “use login rate limit” or >= 0 and add fallback logic to
compute the effective rate (e.g., use the login rate-limit bucket) before
calling service.WithSsoRateLimit(throttled.PerMin(...)), or set the default in
the config (config.Auth.SSORateLimitPerMinute) to 10 in config.go; locate the
check around config.Auth.SSORateLimitPerMinute and the call to
service.WithSsoRateLimit/throttled.PerMin to implement the chosen fix and ensure
0 triggers the fallback to the login rate limit.
In `@server/config/config.go`:
- Around line 1390-1391: Set the registered default for
auth.sso_rate_limit_per_minute to 10 and update its help text to accurately
reflect that the default is 10 (rather than saying it falls back to the login
rate limit); specifically change the call to man.addConfigInt for
"auth.sso_rate_limit_per_minute" to use default 10 and revise the help string,
and also update any docs that state the default
(docs/Configuration/fleet-server-configuration.md) to match; keep the existing
logic in cmd/fleet/serve.go that only applies a limiter when
config.Auth.SSORateLimitPerMinute > 0 so behavior is consistent with the new
default.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6cd15dd8-0cea-4545-8f86-6e0728494e5d
⛔ Files ignored due to path filters (1)
docs/Configuration/fleet-server-configuration.mdis excluded by!**/*.md
📒 Files selected for processing (3)
cmd/fleet/serve.goserver/config/config.goserver/service/handler.go
| // has no traversal limit of its own. | ||
| func validateSAMLResponseShape(samlResponse []byte) error { | ||
| doc := etree.NewDocument() | ||
| if err := doc.ReadFromBytes(samlResponse); err != nil { |
There was a problem hiding this comment.
one minor note here is that this loads the whole payload into memory, but given the current guards at the middleware layer, an attack would consume at most 256 KB * 10 = ~2.5 MB per minute which should be ok.
There was a problem hiding this comment.
Yeah, both the rate limit + size cap should keep things safe.
| // bucket) so a flood on the unauthenticated callback can't exhaust the | ||
| // rate-limit budget that legitimate password logins depend on. The rate | ||
| // defaults to the login rate unless explicitly overridden. | ||
| ssoRateLimit := loginRateLimit |
There was a problem hiding this comment.
Do we want to do the same thing for the mdmSsoLimiter? Right now both the logic and mdm sso limiter share the same bucket
There was a problem hiding this comment.
I wanted to keep this new rate limiting separate to reduce breaking other endpoints (this new rate limit will be separate from login/MDM-SSO endpoints)
juan-fdz-hawa
left a comment
juan-fdz-hawa
left a comment
There was a problem hiding this comment.
Looks good! I left two comments, but they’re just minor suggestions rather than roadblocks.:
- If we think memory consumption might be an
validateSAMLResponseShape, perhaps consider implementing the validation using a stream reader:
dec := xml.NewDecoder(bytes.NewReader(samlResponse))
depth, count := 0, 0
for {
tok, err := dec.Token()
if err == io.EOF {
return nil
}
if err != nil {
return fmt.Errorf("parsing SAMLResponse XML: %w", err)
}
switch tok.(type) {
case xml.StartElement:
depth++
count++
if depth > maxSAMLResponseDepth {
return fmt.Errorf("SAMLResponse exceeds maximum nesting depth of %d", maxSAMLResponseDepth)
}
if count > maxSAMLResponseElements {
return fmt.Errorf("SAMLResponse exceeds maximum element count of %d", maxSAMLResponseElements)
}
case xml.EndElement:
depth--
}
}
- Consider maybe defining a separate bucket for the MDM SSO limiter to keep things consistent.
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit