fix(saml): forward SP RelayState in IdP-initiated flow (AWS WorkSpaces) [Buzzvil fork patch]#1
Conversation
…ession state code) WorkSpaces euc-sso appends a per-session state code via ?RelayState= to the IdP-initiated init URL; without round-tripping it, the auth code cannot be bound to the client session and the native client loops back. idp_initiated now prefers a passed relay_state over the static default_relay_state.
SAMLSSOBindingInitView now forwards request.GET[RelayState] to idp_initiated so the AWS WorkSpaces euc-sso per-session state code round-trips back to AWS.
Walkthrough이 PR은 authentik의 IdP-initiated SAML 흐름에서 요청 쿼리의 RelayState를 그대로 forward하도록 Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SAMLSSOBindingInitView
participant AuthNRequestParser
Client->>SAMLSSOBindingInitView: IdP-initiated SSO 요청 (RelayState 쿼리 포함)
SAMLSSOBindingInitView->>SAMLSSOBindingInitView: self.request.GET[REQUEST_KEY_RELAY_STATE] 조회
SAMLSSOBindingInitView->>AuthNRequestParser: idp_initiated(relay_state=값)
AuthNRequestParser->>AuthNRequestParser: relay_state 존재 시 request.relay_state에 우선 설정
AuthNRequestParser-->>SAMLSSOBindingInitView: AuthNRequest 반환
관련 이슈: 없음 (제공된 정보에 명시된 연결 이슈 없음) 관련 PR: 없음 Suggested labels: enhancement, ci, documentation Suggested reviewers: 없음 (제공된 정보 없음) 🐰 RelayState를 잃지 않고 전해주려, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…erhub/datadog secrets)
|
self hosted runner 할당이 안되고, public이기도 해서 그냥 ghcr에 올렸습니다 |
|
아 이거 ptc 셋업까진 해둘 수 있겠네요 |
|
고것은 테라폼이랑 k8s pr 다시 만드는 걸로 |
요약이 PR은 IdP-initiated SAML 로그인( 코드 로직 자체는 작고 정확하며, 아래에서 확인했듯 패치 위치도 올바르다. 다만 신뢰 경계(trust boundary) 관점에서 짚어둘 지점이 하나 있다. 발견된 이슈🟡 주의 — IdP-initiated에서 검증 없는 RelayState를 echo (신뢰 경계 확장)
기존 IdP-initiated 경로는 관리자가 설정한 정적
다만 실제 위험도는 낮다:
→ 차단 사유는 아니지만, "IdP-init RelayState는 이제 임의 입력을 신뢰해 forward한다"는 점을 README/코드 주석의 보안 노트로 남겨두길 권한다. (참고: SAML 스펙상 RelayState 권장 길이는 ≤80 bytes이나 여기서도, 기존 SP-initiated 경로에서도 강제하지 않음 — 기존과 동일.) 🔵 제안 — 새
|
|
포크라서 퍼블릭인거죠?? |
|
그렇읍니다 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
.github/workflows/build-patched-ecr.yaml (2)
68-74: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win릴리스 생성 실패를 모두 침묵 처리.
|| echo ... skipping은 "이미 존재함" 외의 실패(권한 부족, 네트워크 오류 등)까지 모두 삼켜 워크플로우를 성공으로 표시합니다. 이미지 빌드/푸시는 됐지만 Release는 실제로 생성되지 않은 채 조용히 넘어갈 수 있습니다.♻️ 제안 (에러 메시지로 분기)
- gh release create "${{ github.ref_name }}" \ + gh release create "$TAG_NAME" \ --repo "${{ github.repository }}" \ - --title "${{ github.ref_name }} — WorkSpaces SAML RelayState patch" \ + --title "$TAG_NAME — WorkSpaces SAML RelayState patch" \ --notes "..." \ --verify-tag \ - || echo "release for ${{ github.ref_name }} already exists — skipping" + || { code=$?; gh release view "$TAG_NAME" --repo "${{ github.repository }}" >/dev/null 2>&1 && echo "release already exists — skipping" || exit $code; }🤖 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 @.github/workflows/build-patched-ecr.yaml around lines 68 - 74, The release creation step in the workflow is swallowing every failure via the fallback echo, which can hide real errors. Update the gh release create block to distinguish the “already exists” case from actual failures by checking the command’s stderr/exit condition and only skipping when the release truly exists. Keep the logic localized to the release creation step in the build-patched-ecr job, and let unexpected errors fail the workflow instead of being treated as success.
33-40: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
persist-credentials: false추가 권장.zizmor가 artipacked 위험(자격증명 지속)을 지적했습니다. 이후 단계가 신뢰할 수 있는 액션들로 제한되어 즉각적 위험은 낮지만, 표준 보안 관행으로 명시적으로 비활성화하는 것을 권장합니다.
🔒 제안
- name: Checkout (patch files only) uses: actions/checkout@v4 with: + persist-credentials: false sparse-checkout: | .buzzvil authentik/providers/saml/processors/authn_request_parser.py authentik/providers/saml/views/sso.py sparse-checkout-cone-mode: false🤖 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 @.github/workflows/build-patched-ecr.yaml around lines 33 - 40, The checkout step in the build-patched-ecr workflow should explicitly disable persisted credentials to avoid carrying git auth beyond the checkout. Update the actions/checkout@v4 configuration in the patch-files-only step to include persist-credentials set to false, keeping the sparse-checkout behavior intact. Use the existing checkout step name as the anchor when editing this workflow.Source: Linters/SAST tools
README.md (1)
14-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value패치 설명 표에서 실제 구현과의 사소한 표현 차이.
16행은
request.GET["RelayState"]로 기술되어 있지만, 실제sso.py코드는self.request.GET.get(REQUEST_KEY_RELAY_STATE)를 사용합니다(존재하지 않을 때KeyError대신None을 반환하는 더 안전한 방식). 문서 정확도를 위해 실제 구현과 맞추는 것을 권장합니다.🤖 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 `@README.md` around lines 14 - 17, README의 패치 설명 표에서 `SAMLSSOBindingInitView`의 RelayState 조회 방식이 실제 구현과 다릅니다. `sso.py`에서 `request.GET["RelayState"]`처럼 단정적으로 쓰지 말고, 실제로는 `self.request.GET.get(REQUEST_KEY_RELAY_STATE)`를 사용해 `None`을 반환하는 동작임을 반영하도록 문구를 수정하세요. `authentik/providers/saml/views/sso.py`와 `SAMLSSOBindingInitView`를 기준으로 설명을 맞추면 됩니다..buzzvil/Dockerfile (1)
1-14: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueTrivy DS-0002(USER 미지정)는 대부분 false positive로 보임 — 선택적으로 명시적
USER추가 권장.공식 authentik 서버 이미지는 기본적으로 The server container runs as user 1000:1000 Dockerfile165이므로, 별도
USER지시자가 없어도 이 overlay는 base image의 non-root 설정을 상속합니다. 다만 Trivy는 base image를 고려하지 않고 이 Dockerfile 단독으로 스캔하므로 경고가 발생합니다. 향후 base image가 바뀌더라도 안전하도록 명시적으로USER를 지정하는 것도 고려할 수 있습니다.🛡️ 명시적 USER 지정 예시
COPY --chown=authentik:authentik authentik/providers/saml/processors/authn_request_parser.py /authentik/providers/saml/processors/authn_request_parser.py COPY --chown=authentik:authentik authentik/providers/saml/views/sso.py /authentik/providers/saml/views/sso.py + +USER 1000:1000🤖 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 @.buzzvil/Dockerfile around lines 1 - 14, The Dockerfile relies on the base image’s non-root default but does not explicitly declare it, which triggers the Trivy USER warning. Update the Dockerfile around the FROM/COPY block to add an explicit USER directive matching the authentik runtime user from the base image, so the image’s intended non-root execution is clear even when scanned standalone.Source: Linters/SAST tools
🤖 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/build-patched-ecr.yaml:
- Around line 64-74: The Create GitHub Release step is interpolating
github.ref_name and github.repository directly inside the run script, which
creates a shell-injection risk. Update the workflow so the release name and repo
are passed through env variables on the Create GitHub Release step, then
reference only those environment variables inside the gh release create command.
Keep the change localized to the Create GitHub Release block and avoid direct
expression expansion in the shell script.
---
Nitpick comments:
In @.buzzvil/Dockerfile:
- Around line 1-14: The Dockerfile relies on the base image’s non-root default
but does not explicitly declare it, which triggers the Trivy USER warning.
Update the Dockerfile around the FROM/COPY block to add an explicit USER
directive matching the authentik runtime user from the base image, so the
image’s intended non-root execution is clear even when scanned standalone.
In @.github/workflows/build-patched-ecr.yaml:
- Around line 68-74: The release creation step in the workflow is swallowing
every failure via the fallback echo, which can hide real errors. Update the gh
release create block to distinguish the “already exists” case from actual
failures by checking the command’s stderr/exit condition and only skipping when
the release truly exists. Keep the logic localized to the release creation step
in the build-patched-ecr job, and let unexpected errors fail the workflow
instead of being treated as success.
- Around line 33-40: The checkout step in the build-patched-ecr workflow should
explicitly disable persisted credentials to avoid carrying git auth beyond the
checkout. Update the actions/checkout@v4 configuration in the patch-files-only
step to include persist-credentials set to false, keeping the sparse-checkout
behavior intact. Use the existing checkout step name as the anchor when editing
this workflow.
In `@README.md`:
- Around line 14-17: README의 패치 설명 표에서 `SAMLSSOBindingInitView`의 RelayState 조회
방식이 실제 구현과 다릅니다. `sso.py`에서 `request.GET["RelayState"]`처럼 단정적으로 쓰지 말고, 실제로는
`self.request.GET.get(REQUEST_KEY_RELAY_STATE)`를 사용해 `None`을 반환하는 동작임을 반영하도록 문구를
수정하세요. `authentik/providers/saml/views/sso.py`와 `SAMLSSOBindingInitView`를 기준으로
설명을 맞추면 됩니다.
🪄 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
Run ID: cad14722-c52c-42b8-9038-5033e5eb24af
📒 Files selected for processing (5)
.buzzvil/Dockerfile.github/workflows/build-patched-ecr.yamlREADME.mdauthentik/providers/saml/processors/authn_request_parser.pyauthentik/providers/saml/views/sso.py
| - name: Create GitHub Release | ||
| if: startsWith(github.ref, 'refs/tags/') | ||
| env: | ||
| GH_TOKEN: ${{ github.token }} | ||
| run: | | ||
| gh release create "${{ github.ref_name }}" \ | ||
| --repo "${{ github.repository }}" \ | ||
| --title "${{ github.ref_name }} — WorkSpaces SAML RelayState patch" \ | ||
| --notes "패치 이미지: \`ghcr.io/buzzvil/authentik:${{ github.ref_name }}\` (multi-arch amd64/arm64, public). 패치 내용·빌드·제거 조건은 저장소 README 참고." \ | ||
| --verify-tag \ | ||
| || echo "release for ${{ github.ref_name }} already exists — skipping" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
run: 블록의 템플릿 보간이 셸 인젝션 위험(zizmor error) — env 변수 경유로 전달 권장.
${{ github.ref_name }}와 ${{ github.repository }}가 run: 셸 스크립트에 직접 보간됩니다. 태그 push 권한이 있는 사용자만 ref_name을 제어할 수 있어 공격 표면은 제한적이지만, 특수문자(백틱, $(), ; 등)가 포함된 태그명이 생성되면 셸 인젝션으로 이어질 수 있습니다. GitHub Actions 표준 완화책은 표현식을 env:로 넘기고 셸에서는 환경변수로만 참조하는 것입니다.
🛡️ 제안
- name: Create GitHub Release
if: startsWith(github.ref, 'refs/tags/')
env:
GH_TOKEN: ${{ github.token }}
+ TAG_NAME: ${{ github.ref_name }}
+ REPO_NAME: ${{ github.repository }}
run: |
- gh release create "${{ github.ref_name }}" \
- --repo "${{ github.repository }}" \
- --title "${{ github.ref_name }} — WorkSpaces SAML RelayState patch" \
- --notes "패치 이미지: \`ghcr.io/buzzvil/authentik:${{ github.ref_name }}\` (multi-arch amd64/arm64, public). 패치 내용·빌드·제거 조건은 저장소 README 참고." \
+ gh release create "$TAG_NAME" \
+ --repo "$REPO_NAME" \
+ --title "$TAG_NAME — WorkSpaces SAML RelayState patch" \
+ --notes "패치 이미지: \`ghcr.io/buzzvil/authentik:$TAG_NAME\` (multi-arch amd64/arm64, public). 패치 내용·빌드·제거 조건은 저장소 README 참고." \
--verify-tag \
- || echo "release for ${{ github.ref_name }} already exists — skipping"
+ || echo "release for $TAG_NAME already exists — skipping"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Create GitHub Release | |
| if: startsWith(github.ref, 'refs/tags/') | |
| env: | |
| GH_TOKEN: ${{ github.token }} | |
| run: | | |
| gh release create "${{ github.ref_name }}" \ | |
| --repo "${{ github.repository }}" \ | |
| --title "${{ github.ref_name }} — WorkSpaces SAML RelayState patch" \ | |
| --notes "패치 이미지: \`ghcr.io/buzzvil/authentik:${{ github.ref_name }}\` (multi-arch amd64/arm64, public). 패치 내용·빌드·제거 조건은 저장소 README 참고." \ | |
| --verify-tag \ | |
| || echo "release for ${{ github.ref_name }} already exists — skipping" | |
| - name: Create GitHub Release | |
| if: startsWith(github.ref, 'refs/tags/') | |
| env: | |
| GH_TOKEN: ${{ github.token }} | |
| TAG_NAME: ${{ github.ref_name }} | |
| REPO_NAME: ${{ github.repository }} | |
| run: | | |
| gh release create "$TAG_NAME" \ | |
| --repo "$REPO_NAME" \ | |
| --title "$TAG_NAME — WorkSpaces SAML RelayState patch" \ | |
| --notes "패치 이미지: \`ghcr.io/buzzvil/authentik:$TAG_NAME\` (multi-arch amd64/arm64, public). 패치 내용·빌드·제거 조건은 저장소 README 참고." \ | |
| --verify-tag \ | |
| || echo "release for $TAG_NAME already exists — skipping" |
🧰 Tools
🪛 zizmor (1.26.1)
[error] 69-69: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 71-71: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 72-72: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 74-74: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 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 @.github/workflows/build-patched-ecr.yaml around lines 64 - 74, The Create
GitHub Release step is interpolating github.ref_name and github.repository
directly inside the run script, which creates a shell-injection risk. Update the
workflow so the release name and repo are passed through env variables on the
Create GitHub Release step, then reference only those environment variables
inside the gh release create command. Keep the change localized to the Create
GitHub Release block and avoid direct expression expansion in the shell script.
Source: Linters/SAST tools
|
라는 의견인데 https://github.com/Buzzvil/RxSwift 도 퍼블릭이군요. |
|
그래도 오픈소스 가져다 쓰는데 오픈소스 문화 맞춰주는 게 좋지 않나 싶기도 하고 사실 잘은 모르겠지만 오픈소스 업스트림 관계 깨는 것보단 인프라를 오픈소스에 맞춰주는 게 장기적으로 좀 더 저렴하지 않을까 하는 생각입니다 |
|
LGTM 입니다.~ |
2 participants
Summary
Buzzvil 포크 전용 패치. AWS WorkSpaces SAML SSO 를 위해 IdP-initiated init 흐름이 SP 가 넘긴⚠️ 업스트림(goauthentik/authentik) 반영용 아님 — Buzzvil/authentik 포크에서 패치 이미지 빌드용.)
RelayState를 그대로 사용하도록 수정한다. (문제
SAMLSSOBindingInitView는 IdP-initiated 흐름에서AuthNRequestParser.idp_initiated()를 호출하는데, 이 경로는 요청의?RelayState=쿼리를 무시하고 provider 의 정적default_relay_state만 사용한다(SAMLSSOBindingRedirect/POSTView와 달리).AWS WorkSpaces 는 native client 로그인 시 euc-sso 가 init URL 에 per-session state code 를
?RelayState=로 붙여 IdP 로 보내고, IdP 가 이를 SAML Response 에 echo 해야 euc-sso 가 auth code 를 그 client 세션에 바인딩한다. 정적default_relay_state로는 state code 가 유실되어 auth code 미발급 → native client 가 sign-in 으로 loop-back 한다. (upstream goauthentik#17542 동일 증상)변경 (2 files)
authentik/providers/saml/processors/authn_request_parser.py:idp_initiated(relay_state=None)— 전달된 relay_state 를 default_relay_state 보다 우선 사용.authentik/providers/saml/views/sso.py:SAMLSSOBindingInitView가request.GET["RelayState"]를idp_initiated()로 전달.검증
adfit VDI(directory d-956793e49d) 에서 이 이미지 배포 후 native client 로그인 → auth code 발급 → 데스크톱 진입 확인 예정.