fix: Shibboleth SP の属性の受け付けを SP のログインスクリプトからの要求に限る - #1929
Merged
Merged
Conversation
SP の属性は、Web サーバ上のログインスクリプト(secure/login.py, login.php)が WEKO へ送る。その受け付けを、送信元のアドレスで限定する。 - weko-accounts: 受け付けるアドレスを WEKO_ACCOUNTS_SHIB_SP_ALLOWED_ADDRS (既定 127.0.0.1 / ::1)に限るデコレータを付ける。それ以外は 403 - login.py / login.php: 送り先をループバックアドレスにし、Host ヘッダに 公開ホスト名を入れる - nginx: /weko/shib/login への GET 以外をループバックアドレスからだけ許可する (weko.conf / weko-ams.conf / weko-ams-restricted.conf) 既存の環境では、login.py / login.php と nginx の設定を同時に更新すること。 どちらかだけでは Shibboleth ログインが通らなくなる。 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
real_ip は信頼するプロキシ(プライベートアドレス)からの接続について X-Forwarded-For で $remote_addr を書き換える。そのため $remote_addr で ループバックかを判定すると、信頼するアドレスから X-Forwarded-For に ループバックを入れた要求を通してしまう。 /weko/shib/login の判定を $realip_remote_addr(書き換える前の接続元)で 行い、WEKO へ渡す REMOTE_ADDR も同じ値にする。 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reviewer's GuideShibboleth SP 属性の受け付けをログインスクリプト由来のリクエストに限定するため、アプリケーションと nginx の双方で送信元を検証し、スクリプトはループバック経由で POST するよう変更しています。 Sequence diagram for restricted Shibboleth attribute submissionsequenceDiagram
participant SP as ShibbolethLoginScript
participant N as Nginx
participant W as WEKO
SP->>N: POST /weko/shib/login via 127.0.0.1
N->>N: Check request_method and realip_remote_addr
alt Loopback POST
N->>W: Forward request with REMOTE_ADDR=127.0.0.1
W->>W: shib_sp_source_required
W->>W: Read Shibboleth attributes
W-->>SP: Login response
else Non-loopback POST
N-->>SP: 403 Forbidden
end
Flow diagram for Shibboleth login request authorizationflowchart TD
A[POST /weko/shib/login] --> B{Method is GET or HEAD?}
B -->|Yes| C[Forward to WEKO]
B -->|No| D{realip_remote_addr is 127.0.0.1 or ::1?}
D -->|No| E[403 Forbidden]
D -->|Yes| F[Forward with REMOTE_ADDR=realip_remote_addr]
F --> G{request.remote_addr is allowed?}
G -->|No| E
G -->|Yes| H[Read Shibboleth attributes]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've reviewed your changes and they look great!
Sourcery assessment
Needs a human reviewer. This changes the authentication trust boundary: any request accepted from an allowed address can supply Shibboleth attributes that determine the logged-in user, so an incorrect address or proxy configuration could permit account impersonation from the moment it ships. Reverting removes the new check and routing, but cannot undo any authenticated actions performed while a forged identity was accepted.
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
API インベントリ差分（件数のみ）
ベースラインとの差分API インベントリ差分レポート
判定: ✅ PASS (FAIL 0 / WARN 0)サマリ
変化はありません。 台帳との突き合わせスナップショット ↔ インベントリ 突き合わせ
判定: ✅ 一致 (0件)
ソース由来の経路検知ソース由来の経路検知
判定: ✅ 全検知が台帳に対応 (0件)
参考: 静的検知と結びつかなかった台帳行
|
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.
概要 (Summary)
shib_sp_loginに、送信元のアドレスを確認するデコレータshib_sp_source_requiredを付ける。許可するアドレスは新しい設定WEKO_ACCOUNTS_SHIB_SP_ALLOWED_ADDRS(既定127.0.0.1/::1)。それ以外は属性を読む前に 403weko.conf/weko-ams.conf/weko-ams-restricted.conf):location = /weko/shib/loginを追加し、GET / HEAD 以外はループバックアドレスからの接続だけを許可する$realip_remote_addr(real_ip で書き換える前の接続元)を使い、WEKO へ渡すREMOTE_ADDRも同じ値にする。$remote_addrで判定すると、信頼するプロキシのアドレスからX-Forwarded-Forにループバックを入れた要求を通してしまうためnginx/login.py/nginx/login.php: 送り先をループバックアドレス(https://127.0.0.1)にし、Hostヘッダに公開ホスト名を入れる(開発環境の docker-compose 向け。k8s の本番は Deployment のhostAliasesで公開ホスト名が 127.0.0.1 に解決されるため、従来のスクリプトのままでもループバックで送られる)/weko/shib/loginへの POST は 403 / GET(ログイン後の確認画面)と他の/weko/shib配下は従来どおり / SP のログインスクリプトからの POST は従来どおりhostAliasesによりループバックで送っているので、nginx の設定の更新だけでよい関連Issue / チケット (Related Issues)
変更タイプ (Type of Change)
🤖 0. CI 自動チェック (API Inventory Drift)
PR ごとに WEKO3 コンテナを起動し、
url_mapのダンプ・台帳との突き合わせ・変更行の到達可否測定を自動実行する。結果は PR コメントと Actions の artifact
(
api-inventory-summary) に出る。このリポジトリは public のため、台帳もベースラインも同梱していない。
実データはプライベートリポジトリ
RCOSDP/weko-secretにあり、CI は Secret 経由で取得する。以降この文書では、そこを単にプライベートリポジトリと呼ぶ。
台帳はブランチごとに内容が違うため、CI は weko 側と同名のブランチを
プライベートリポジトリから探して使う（head → base → 既定ブランチ の順）。
採用されたブランチ名は PR コメントの冒頭に出るので、件数を読む前にそこを見ること。
対応ブランチが無い場合は既定ブランチと比較され、コメント冒頭に警告が出る。
その件数は当てにならないので、PASS でも「確認済み」と読まないこと。
詳細:
tools/api-inventory/ci/README.md§3aSecret (
API_INVENTORY_REPO/API_INVENTORY_SSH_KEY) が未設定のリポジトリ、および fork からの PR では、このジョブは何もせずスキップされる。
API を追加・変更した場合（必須）
この PR が
fix/issue62569→develop_v2.0.4なら、プライベート側もfix/issue62569→develop_v2.0.4。同名にしておけば台帳 PR が未マージでもCI がそれを見るので、2つの PR のマージ順を気にしなくてよい。
api_snapshot.jsonを更新し、対応する PR を出したweko3_api_list_full.tsvに行を追加・更新し、build_checklist.pyで 24 列版を再生成した（未収載だと reconcile が FAIL する）（
git statusに*.tsv/api_snapshot.jsonが出ていないこと）FAIL したときの対処（要約）
まず PR コメント冒頭の台帳ブランチを見る。警告が出ていれば、件数を追う前に
プライベートリポジトリ側の対応ブランチを用意すること（比較相手が違うので件数に意味がない）。
ジョブが落ちる条件は 3 つある。PR コメントのどのセクションに件数が出ているかで切り分ける。
drift.md)reconcile.md)*_PERMISSION_FACTORY/ CSRF 保護 等が危険側の値に変わったcan_delete/can_exportがFalse→Truedata_opを更新data_opが作成/更新/削除の経路に、未認証で到達したdata_opの記載誤りなら台帳を直すurl_mapに無いreconcile B のうち、実機に存在しないことが正当な行（プラグイン未登録・config で無効等）は
プライベートリポジトリの
reconcile_allow.jsonに理由付きで登録する。理由なしの登録は不可。登録済みの行は B'（既知・許容）として集計され、E'（endpoint が実機に無い）と併せてゲート対象外になる。
W1〜W6 は WARN でゲートは通るが、レビューでは見ること
（ModelView の追加 / 実装本体の変化 / HTTP メソッド・URL の変化 / 監視対象 config の変化 /
依存パッケージの版の変化）。特に W6（依存の版）は、ベースラインを CI と異なる環境で作ると
毎回出続けて形骸化するため、ベースラインは
install.shで作った環境から生成する。🔒 1. セキュリティ & API アクセス制御チェック (必須)
認証・認可 (Authentication & Authorization)
@login_required,@pass_record,need(...), Invenio Access Action/api/*ではPermission.require(http_exception=403)を使うこと。@login_requiredは API アプリにsecurity.loginが無いため 401 ではなく 500 になる--allow-writes付きでGET / HEAD 以外も叩く）
起動した経路のみ。ワークフロー系など未解決プレースホルダの行は skip される
Noneで無効化していない*_PERMISSION_FACTORY等を監視機能クローズ・非公開化の場合 (Feature Disable)
404 Not Foundまたは403 Forbiddenが返ることを確認した🧪 2. テストコード観点チェック (pytest / Invenio Test Suite)
権限・異常系テスト (Negative & Authorization Tests)
401 Unauthorizedまたは403 Forbidden/404 Not Foundが返ることを検証するテストがある403になるテストがある404/403を返すテストがある境界値・入力バリデーションテスト (Boundary & Validation)
400 Bad Request/ バリデーションエラーが返るテストがあるデータ整合性・トランザクションテスト (Integrity & Rollback)
🛡️ 3. データ保護 & 破壊的変更防止チェック (Data Safety)
⚙️ 4. マイグレーション & システム影響チェック (Invenio / WEKO3 Stack)
データベース (DB / Alembic)
invenio alembic upgrade（適用）およびdowngrade（ロールバック）スクリプトを作成・検証した検索インデックス (Elasticsearch / OpenSearch)
設定 & 非同期処理 (Config / Celery / Cache)
invenio.cfg/ 環境変数のデフォルト値を設定したWEKO_ACCOUNTS_SHIB_SP_ALLOWED_ADDRS(既定['127.0.0.1', '::1'])を weko_accounts/config.py に追加📚 5. ドキュメント・仕様書更新チェック (weko-document)
tools/api-inventory/、台帳・調査記録はプライベートリポジトリ（public リポジトリには置かない）。
§0 のチェック項目で対応済みなら、ここは確認のみ。
weko3_api_auth_findings.md）もプライベートリポジトリに置く。台帳は二重管理しない📋 6. 動作検証エビデンス (Verification Evidence)
テスト実行結果
CI の成果物 (artifact:
api-inventory-summary)drift.mdreconcile.md明細（該当した経路名・実測結果）は公開できないため artifact に含めていない。
プライベートリポジトリ側で同じコマンドを
--summary-onlyなしで実行して確認する。手動で確認したこと
$remote_addrで判定する設定では、信頼するアドレスからX-Forwarded-For: 127.0.0.1を付けた POST が通ってしまうことを確認した(そのため$realip_remote_addrで判定している)🤖 Generated with Claude Code
Summary by Sourcery
Restrict Shibboleth SP attribute submissions to trusted loopback or configured source addresses.
Bug Fixes:
Enhancements:
Tests: