feat(federation): add side-effect-free SAML preflight validation - #43
Merged
Conversation
📝 WalkthroughWalkthrough외부 IdP desired state에 대한 인증된 preflight 검증을 추가했습니다. SAML URI, 보안 설정, 인증서를 저장 전에 검증하고 민감값을 제거한 결과를 반환합니다. ADFS 템플릿과 온보딩·복구 문서, CI 커버리지 검증도 갱신했습니다. Changes외부 페더레이션 사전 검증
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant ValidateEndpoint
participant FederationService
participant DesiredState
participant Keycloak
Operator->>ValidateEndpoint: POST /federation/identity-providers:validate
ValidateEndpoint->>FederationService: validate_registration(registration)
FederationService-->>ValidateEndpoint: redacted result and ready_to_apply
Operator->>DesiredState: PUT /federation/identity-providers/{alias}
DesiredState->>FederationService: validate registration before persistence
DesiredState->>Keycloak: converge provider configuration
Keycloak-->>DesiredState: apply status
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
seonghobae
marked this pull request as ready for review
August 4, 2026 03:50
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
docs/federation-onboarding.md (1)
8-13: 🔒 Security & Privacy | 🔵 Trivial네트워크 노출 경계를 이 운영 절차에 명시하십시오.
이 절은 operator API의 인증과 토큰 보관만 설명합니다. WAF에서 어떤 endpoint를 공개할 수 있는지 설명하지 않습니다. 배포 지침에 Keyverse admin/SCIM API와 허용된 Keycloak OIDC endpoint만 WAF에 공개하고, federation service 내부망과 Keycloak Admin REST는 공개하지 않는 규칙을 추가하십시오.
Based on learnings, 내부 네트워크는 공개하지 않고 WAF에는 Keycloak OIDC 엔드포인트와 admin/SCIM API만 노출해야 합니다.
🤖 Prompt for 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. In `@docs/federation-onboarding.md` around lines 8 - 13, Update the “Trust boundary” section to document the network exposure boundary: instruct deployments to expose only the Keyverse admin/SCIM APIs and permitted Keycloak OIDC endpoints through the WAF, while keeping the federation service internal network and Keycloak Admin REST inaccessible publicly.Source: Learnings
🤖 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 @.github/workflows/measure-account-unification-coverage.yml:
- Around line 3-10: Update the workflow trigger and job condition so coverage
runs for pull_request events and pushes to the repository’s default branch, not
only feat/federation-preflight-validation. Extend the paths list to include
services/account_unification/pyproject.toml and
services/account_unification/uv.lock, and keep the job’s if condition aligned
with the supported events and branches.
In `@deploy/templates/README.md`:
- Around line 27-33: Protect the bearer token from shell tracing by wrapping
token retrieval and AUTH_CONFIG creation in both deploy/templates/README.md
lines 27-33 and docs/federation-onboarding.md lines 42-48 with xtrace-disabled
handling; after unset TOKEN, restore the trace state that was active before the
protected section.
- Around line 37-52: Update the preflight-and-PUT command sequences in
deploy/templates/README.md lines 37-52 and docs/federation-onboarding.md lines
52-67 so the PUT runs only when the preflight returns HTTP 200 and its JSON
response has ready_to_apply set to true; do not rely solely on curl’s exit
status or --fail-with-body, which permits 3xx responses.
In `@docs/federation-onboarding.md`:
- Around line 79-95: Make the recovery command block independently executable by
recreating BASE and AUTH_CONFIG within that block, retrieving the token from KV,
and generating a temporary curl config with 0600 permissions. Ensure both the
provider-list and provider-apply calls use these locally initialized values,
then remove the temporary config after both operations complete.
- Around line 110-114: Update the manual rollover procedure in the
metadata-disabled path to set both the previous and new certificates,
comma-separated, in the PUT payload’s signingCertificate field during the
rollover window. Explicitly retain both as active trusted certificates, then
remove the previous certificate after the upstream rollover period ends; do not
describe storing the old certificate separately as sufficient.
In `@docs/superpowers/specs/2026-08-04-keyverse-federation-preflight-design.md`:
- Around line 124-127: Update the signingCertificate validation described in the
preflight specification to require a non-empty value that Base64-decodes and
parses as a valid X.509 certificate, excluding PEM headers and footers. Ensure
validation failures prevent ready_to_apply=true, and add coverage for both a
valid certificate and an invalid value such as "MIIC-test-certificate".
In `@services/account_unification/app/federation.py`:
- Around line 383-407: Update the URL validation logic around invalid_text and
invalid_uri to reject every C0 control character (U+0000–U+001F) and DEL, not
only whitespace, while preserving the existing encoded-control checks. Force
validation of parsed.port and treat any ValueError from that access as an
invalid URI, including out-of-range ports such as 65536. Add regression tests
covering a raw NUL character and an out-of-range port in
test_federation_url_hardening.py.
---
Nitpick comments:
In `@docs/federation-onboarding.md`:
- Around line 8-13: Update the “Trust boundary” section to document the network
exposure boundary: instruct deployments to expose only the Keyverse admin/SCIM
APIs and permitted Keycloak OIDC endpoints through the WAF, while keeping the
federation service internal network and Keycloak Admin REST inaccessible
publicly.
🪄 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: Pro Plus
Run ID: 38c32967-a36c-4e81-bdb7-dbab605759e4
📒 Files selected for processing (13)
.github/workflows/measure-account-unification-coverage.ymlCHANGELOG.mdCLAUDE.mdREADME.mddeploy/templates/README.mddeploy/templates/saml-idp-employer-adfs.jsondocs/federation-onboarding.mddocs/superpowers/plans/2026-08-04-keyverse-federation-preflight.mddocs/superpowers/specs/2026-08-04-keyverse-federation-preflight-design.mdservices/account_unification/app/federation.pyservices/account_unification/tests/test_federation.pyservices/account_unification/tests/test_federation_preflight.pyservices/account_unification/tests/test_federation_url_hardening.py
seonghobae
enabled auto-merge (squash)
August 4, 2026 06:50
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
services/account_unification/app/federation.py (2)
37-42: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSecurity Misconfiguration (CWE-319): Cleartext Transmission of Sensitive Information
Reachability: External · Exploitability: Moderate
singleSignOnServiceUrl에는 HTTPS만 허용하십시오.
_HTTP_SCHEMES가http를 허용하고, 해당 값은 Keycloak payload에 그대로 전달됩니다. 활성 네트워크 공격자는 SSO 트래픽을 읽거나 변경할 수 있습니다._HTTP_SCHEMES에서http를 제거하고,http://입력이 400을 반환하는 회귀 테스트를 추가하십시오.🤖 Prompt for 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. In `@services/account_unification/app/federation.py` around lines 37 - 42, Restrict singleSignOnServiceUrl to HTTPS by removing “http” from the _HTTP_SCHEMES allowlist used by the federation validation flow. Add a regression test covering an http:// value and assert that the request returns 400, while preserving acceptance of https:// URLs.
37-42: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSecurity Misconfiguration (CWE-345)
Reachability: External · Exploitability: Moderate
metadataDescriptorUrl에 HTTPS 전용 정책과 redirect 재검증을 적용하십시오.
useMetadataDescriptorUrl=true이면_validate_http_url이http와https를 모두 허용하고,signingCertificate검증 없이 반환합니다. HTTP metadata와 HTTPS→HTTP redirect를 거부하고, 두 경우의 회귀 테스트를 추가하십시오.🤖 Prompt for 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. In `@services/account_unification/app/federation.py` around lines 37 - 42, useMetadataDescriptorUrl=true 경로의 _validate_http_url을 수정해 metadataDescriptorUrl과 redirect 최종 URL 모두 HTTPS만 허용하도록 재검증하고 HTTP 및 HTTPS→HTTP redirect를 거부하십시오. 해당 경로에서 signingCertificate 검증을 생략하지 않도록 기존 검증 흐름을 적용하고, 두 거부 사례에 대한 회귀 테스트를 추가하십시오.
🧹 Nitpick comments (2)
services/account_unification/tests/test_federation_preflight.py (1)
299-325: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win롤오버 인증서를 각각 redaction 검사하십시오.
현재 assertion은 쉼표로 결합한 전체
signing_certificates값만 검사합니다. 응답이 한 certificate body만 노출하면 전체 결합 문자열은 없으므로 테스트가 통과합니다. 각 비어 있지 않은 certificate body를 개별적으로 검사하십시오.수정 예시
- assert signing_certificates not in response.text + for certificate_body in signing_certificates.split(","): + certificate_body = certificate_body.strip() + if certificate_body: + assert certificate_body not in response.text🤖 Prompt for 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. In `@services/account_unification/tests/test_federation_preflight.py` around lines 299 - 325, Update test_saml_preflight_accepts_valid_manual_signing_certificates to validate redaction for each non-empty certificate body separately, rather than only checking the combined signing_certificates string. Split the parameterized value on commas and assert every individual certificate is absent from response.text while preserving the existing status, redacted configuration, and side-effect assertions.services/account_unification/tests/test_full_coverage_core.py (1)
326-390: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win공유 잠금 인수도 검증하세요.
UnificationService에 설정한 람다는 생성자 인수를 버립니다. 따라서 이 테스트는 merge 경로가app.state.user_operation_locks와 같은locks인스턴스를 받는지 확인하지 못합니다. 다른 잠금 인스턴스가 전달되면 SCIM replace와 account merge의 상호 배제가 사라질 수 있습니다. 생성자 인수를 캡처하고user_operation_locks is locks를 assert하세요.수정 예시
locks = object() unification = object() federation = object() + wired_locks: object | None = None + + def build_unification( + _api: object, + _audit: object, + _config: object, + user_operation_locks: object, + ) -> object: + """Capture the lock manager assigned to the merge service.""" + nonlocal wired_locks + wired_locks = user_operation_locks + return unification + descriptor = SimpleNamespace(namespace="runtime") @@ monkeypatch.setattr( main, "UnificationService", - lambda *args: unification, + build_unification, ) @@ assert app.state.user_operation_locks is locks + assert wired_locks is locksBased on learnings: SCIM replacement와 merge 흐름은 하나의 공유 동기화 메커니즘으로 보호해야 합니다.
🤖 Prompt for 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. In `@services/account_unification/tests/test_full_coverage_core.py` around lines 326 - 390, Update test_build_service_wires_all_state so the UnificationService test double captures its constructor arguments instead of discarding them, then assert that the user-operation-lock argument is the same locks instance assigned to app.state.user_operation_locks. Preserve the existing service wiring assertions.Source: Learnings
🤖 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.
Outside diff comments:
In `@services/account_unification/app/federation.py`:
- Around line 37-42: Restrict singleSignOnServiceUrl to HTTPS by removing “http”
from the _HTTP_SCHEMES allowlist used by the federation validation flow. Add a
regression test covering an http:// value and assert that the request returns
400, while preserving acceptance of https:// URLs.
- Around line 37-42: useMetadataDescriptorUrl=true 경로의 _validate_http_url을 수정해
metadataDescriptorUrl과 redirect 최종 URL 모두 HTTPS만 허용하도록 재검증하고 HTTP 및 HTTPS→HTTP
redirect를 거부하십시오. 해당 경로에서 signingCertificate 검증을 생략하지 않도록 기존 검증 흐름을 적용하고, 두 거부
사례에 대한 회귀 테스트를 추가하십시오.
---
Nitpick comments:
In `@services/account_unification/tests/test_federation_preflight.py`:
- Around line 299-325: Update
test_saml_preflight_accepts_valid_manual_signing_certificates to validate
redaction for each non-empty certificate body separately, rather than only
checking the combined signing_certificates string. Split the parameterized value
on commas and assert every individual certificate is absent from response.text
while preserving the existing status, redacted configuration, and side-effect
assertions.
In `@services/account_unification/tests/test_full_coverage_core.py`:
- Around line 326-390: Update test_build_service_wires_all_state so the
UnificationService test double captures its constructor arguments instead of
discarding them, then assert that the user-operation-lock argument is the same
locks instance assigned to app.state.user_operation_locks. Preserve the existing
service wiring assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e1b5c62b-044b-4638-8595-94d357f807b1
⛔ Files ignored due to path filters (2)
services/account_unification/requirements.lockis excluded by!**/*.lockservices/account_unification/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
.github/workflows/ci.ymlCHANGELOG.mddeploy/templates/README.mddocs/federation-onboarding.mddocs/superpowers/specs/2026-08-04-keyverse-federation-preflight-design.mdservices/account_unification/app/federation.pyservices/account_unification/pyproject.tomlservices/account_unification/requirements-dev.txtservices/account_unification/tests/test_federation_preflight.pyservices/account_unification/tests/test_federation_url_hardening.pyservices/account_unification/tests/test_full_coverage_core.pyservices/account_unification/tests/test_full_coverage_federation.pyservices/account_unification/tests/test_full_coverage_http_clients.pyservices/account_unification/tests/test_full_coverage_registration_scim.pyservices/account_unification/tests/test_full_coverage_remaining.py
🚧 Files skipped from review as they are similar to previous changes (4)
- services/account_unification/tests/test_federation_url_hardening.py
- deploy/templates/README.md
- docs/federation-onboarding.md
- docs/superpowers/specs/2026-08-04-keyverse-federation-preflight-design.md
Contributor
Author
|
Addressed the current CodeRabbit review on the exact implementation tree verified by workflow run
The preflight endpoint intentionally performs no DNS lookup, metadata fetch, or redirect traversal, so it cannot observe a remote final redirect URL without violating its side-effect-free/SSRF boundary. Redirect-target enforcement is therefore documented at the Keycloak outbound proxy or network-egress layer, where the redirect actually occurs. The current user-authored head preserves the verified workflow tree and exists only to obtain normal exact-head protected-branch checks. |
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
2 participants
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.
What
Add an operator-authenticated, side-effect-free federation preflight endpoint and fail-closed SAML/ADFS desired-state validation before anything is persisted or sent to Keycloak.
Buyer-visible gap
The runtime federation API previously enforced only generic size and alias bounds. An operator could persist unresolved
{{...}}placeholders, omit issuer pinning, disable signature validation, or configure no trusted certificate source and learn about the error only after convergence failed. The committed employer ADFS artifact also used a raw Keycloak representation that the closed Keyverse desired-state API could not accept.Runtime contract
POST /federation/identity-providers:validateurn:identifiersidpEntityIdso issuer validation cannot silently fall back to disabledvalidateSignature=truePUTpersistence boundaryDeployment and interoperability
saml-idp-employer-adfs.jsonto theIdentityProviderRegistrationAPI shapeready_to_apply=true→ apply and outage recoverySupply-chain security
cryptography==50.0.0, released July 31, 2026uv.lock, runtime requirements, and development requirements from the reviewed project manifestx509.load_der_x509_certificateAPI used by manual trust validationStandards basis
The design follows the OASIS SAML metadata interoperability profile’s absolute-URI entity identifier model and Keycloak’s current SAML identity-provider configuration semantics. Network-reachable SSO and metadata locations are restricted more narrowly to HTTPS, while standards-valid non-HTTP entity identifiers remain accepted.
TDD and verification
99b88fe74376ff42718660bd7dec6df38906cb11failed because the preflight route returned HTTP 4043dd95ba154823c1c9d1d344e3005d86b2c169378failed on internal whitespace, backslash, and percent-encoded control cases30884274720completed successfully and verified Ruff, interrogate 100%, compileall, the complete pytest suite, production statement and branch coverage 100%, package build, documentation shell syntax, realm validation, Compose validation, JSON validation, andgit diff --check30885938322upgraded and re-lockedcryptography50.0.0 and repeated the complete service, 100% coverage, package-build, and deployment-contract verification successfully30887527803then required HTTPS for SSO and metadata URLs, validated optional manual certificate material in metadata mode, strengthened rollover redaction and shared-lock wiring tests, updated the operator/design contracts, and repeated focused tests, complete tests, production statement/branch coverage 100%, package build, documentation shell validation, and deployment validation successfullyb90795f8078592ed2621b79048a03930977af9ee; its final user-authored commit exists only to obtain normal exact-head protected-branch checks after the one-shot workflow committed asgithub-actions[bot]Release
CHANGELOG.mdis updated under[Unreleased]. No version bump or release is included because broader 0.2.0 release criteria are not yet satisfied.Closes #3
Summary by CodeRabbit
새 기능
버그 수정
cryptography49.0.0을 50.0.0으로 교체했습니다.문서
테스트