security: sign auth provider state#21061
Draft
stehessel wants to merge 6 commits into
Draft
Conversation
|
Skipping CI for Draft Pull Request. |
Contributor
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
github-actions
Bot
added
area/auth
area/central
area/ui
ai-review
coderabbit-review
labels
Contributor
🚀 Build Images ReadyImages are ready for commit 88bf5f1. To use with deploy scripts: export MAIN_IMAGE_TAG=4.12.x-139-g88bf5f1097 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/auth/authproviders/saml/backend_factory_impl.go`:
- Around line 64-68: The RedeemStateNonce call in backend_factory_impl.go
returns providerID and clientState but lacks the empty providerID validation
present in other factories; after calling idputil.RedeemStateNonce (the
providerID, clientState, err assignment), add a check for providerID == "" and
return an HTTP 400 malformed state error (use the same pattern as the
OIDC/OpenShift factories, e.g., return "", clientState,
httputil.NewError(http.StatusBadRequest, "malformed state")) to keep behavior
consistent.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 8c8706c1-30ef-4196-b113-62d36733dd3b
⛔ Files ignored due to path filters (4)
generated/api/v1/authprovider_service.pb.gois excluded by!**/*.pb.go,!**/generated/**generated/api/v1/authprovider_service.swagger.jsonis excluded by!**/generated/**generated/api/v1/authprovider_service_vtproto.pb.gois excluded by!**/*.pb.go,!**/generated/**generated/internalapi/central/v1/token_service.pb.gois excluded by!**/*.pb.go,!**/generated/**
📒 Files selected for processing (12)
central/authprovider/service/service_impl.gopkg/auth/authproviders/idputil/state_store.gopkg/auth/authproviders/idputil/state_store_test.gopkg/auth/authproviders/oidc/backend_factory_impl.gopkg/auth/authproviders/oidc/backend_impl.gopkg/auth/authproviders/openshift/backend_factory_impl.gopkg/auth/authproviders/openshift/backend_impl.gopkg/auth/authproviders/saml/backend_factory_impl.gopkg/auth/authproviders/saml/backend_impl.goproto/api/v1/authprovider_service.protoui/apps/platform/src/sagas/authSagas.jsui/apps/platform/src/services/AuthService/AuthService.ts
💤 Files with no reviewable changes (1)
- ui/apps/platform/src/sagas/authSagas.js
The auth provider state parameter (containing provider ID and client state with callback URLs) travels through external IdPs unsigned during SSO flows. An attacker who modifies the state can redirect auth tokens to a controlled destination. Sign the state with HMAC-SHA256 before sending to the IdP and verify on callback. The signing key is generated per-Central-instance in memory, matching the existing nonce pool pattern. Non-SSO paths (basic auth, token exchange API) are unaffected as they use the existing unsigned MakeState/SplitState functions. Partially generated by AI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace HMAC-signed inline state with server-side storage keyed by opaque nonces, per Auth0 and RFC 9700 recommendations. The state data (provider ID, callback URL) never leaves Central, providing both integrity and confidentiality with simpler code and no key management. The StateStore issues random nonces via cryptoutils.NonceGenerator, stores associated state with a 5-minute TTL, and supports consuming (Redeem, for SSO callbacks) and non-consuming (Lookup, for the ExchangeToken API which calls ResolveProviderAndClientState twice) retrieval modes. Partially generated by AI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
With opaque state nonces, the UI can no longer determine the auth mode (roxctl authorize vs normal login) by parsing the raw state string. Add an authorize_roxctl field to ExchangeTokenResponse so the server communicates the mode explicitly, matching the existing test field pattern. This fixes OIDC fragment response mode compatibility with roxctl central login. Partially generated by AI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The SplitState fallback in ResolveProviderAndClientState allows an attacker to bypass the nonce store by providing a crafted state in the old providerID:clientState format. If the crafted state encodes AuthorizeRoxctlMode with an attacker-controlled callback URL, the UI would redirect the victim's token to that URL. Only set authorize_roxctl=true when LookupStateNonce confirms the state was issued by Central, not when it was resolved via the SplitState fallback path. Partially generated by AI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add empty providerID check after RedeemStateNonce in the SAML backend factory, consistent with the OIDC and OpenShift factories. Without this check a redeemed nonce carrying an empty provider ID would be silently accepted instead of returning a 400 error. Partially generated by AI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The isTestMode function and testLoginClientState constant became unused after earlier commits on this branch removed their call sites. Removes both to fix the no-unused-vars lint error. Partially generated by AI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
stehessel
force-pushed
the
security/sign-auth-provider-state
branch
from
3b24397 to
88bf5f1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
change me!
User-facing documentation
Testing and quality
Automated testing
How I validated my change
change me!