Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2要素認証におけるブルートフォース対策の実装 #6035

Merged
merged 6 commits into from
Sep 29, 2023

Conversation

KenTanaka
Copy link
Contributor

@KenTanaka KenTanaka commented Sep 13, 2023

概要(Overview・Refs Issue)

  • 総当り攻撃で2要素認証のトークンを突破されないために、防御策としてスロットリングを適用する。

方針(Policy)

  • 管理画面2段階認証トークン入力画面へのスロットリング設定を追加
  • ログインユーザ単位での回数制限であるべきなので、RateLimiterListenerへMemberタイプの処理を追加

実装に関する補足(Appendix)

テスト(Test)

  • 2段階認証を挟むため、E2Eでのテストコードなし
  • 2段階認証有効状態でスロットリング閾値回数での以下のパターンは試験済み
    回数超過
    回数超過前にログイン可能
    回数超過 > キャッシュ削除 > 2段階認証画面表示

相談(Discussion)

  • 現状、スロットリングの設定タイプは ip or customerの二択であるが、memberを追加すべきか?

マイナーバージョン互換性保持のための制限事項チェックリスト

  • 既存機能の仕様変更はありません
  • [ 〇] フックポイントの呼び出しタイミングの変更はありません
  • [〇] フックポイントのパラメータの削除・データ型の変更はありません
  • [〇] twigファイルに渡しているパラメータの削除・データ型の変更はありません
  • [〇] Serviceクラスの公開関数の、引数の削除・データ型の変更はありません
  • [〇] 入出力ファイル(CSVなど)のフォーマット変更はありません

レビュワー確認項目

  • [] 動作確認
  • コードレビュー
  • E2E/Unit テスト確認(テストの追加・変更が必要かどうか)
  • 互換性が保持されているか
  • セキュリティ上の問題がないか
    • 権限を超えた操作が可能にならないか
    • 不要なファイルアップロードがないか
    • 外部へ公開されるファイルや機能の追加ではないか
    • テンプレートでのエスケープ漏れがないか

@KenTanaka KenTanaka changed the title Feature/add rate limit for admin tfa 2要素認証におけるブルートフォース対策の実装 Sep 13, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 13, 2023

Codecov Report

Merging #6035 (9eece6d) into 4.2 (ac096e5) will increase coverage by 0.00%.
Report is 10 commits behind head on 4.2.
The diff coverage is 100.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff            @@
##                4.2    #6035   +/-   ##
=========================================
  Coverage     82.55%   82.56%           
- Complexity     6425     6426    +1     
=========================================
  Files           475      475           
  Lines         25859    25859           
=========================================
+ Hits          21348    21350    +2     
+ Misses         4511     4509    -2     
Flag Coverage Δ
E2E 69.48% <33.33%> (+0.01%) ⬆️
Unit 79.74% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/Eccube/DependencyInjection/Configuration.php 100.00% <100.00%> (ø)
src/Eccube/EventListener/RateLimiterListener.php 92.68% <100.00%> (ø)

... and 1 file with indirect coverage changes

@dotani1111
Copy link
Contributor

@KenTanaka
PRありがとうございます 👍

現状、スロットリングの設定タイプは ip or customerの二択であるが、memberを追加すべきか?

こちらについては新しくuser を追加して、既存のcustomerと今回追加する管理者の処理をまとめる方が良いかと思います。
注意点として互換性を維持するため、既存のcustomerでも動作するようにお願い致します。

テストに関しては、以下のテスト仕様書の更新をお願い致します。
https://github.com/EC-CUBE/eccube-specification/blob/4.2/IntegrationTest/EF09_%E3%82%B9%E3%83%AD%E3%83%83%E3%83%88%E3%83%AA%E3%83%B3%E3%82%B0_%E7%B5%90%E5%90%88%E8%A9%A6%E9%A8%93%E9%A0%85%E7%9B%AE%E6%9B%B8.md

@xuelian311 xuelian311 added this to the 4.2.3 milestone Sep 15, 2023
@xuelian311 xuelian311 added the security security label Sep 15, 2023
ji-eunsoo
ji-eunsoo previously approved these changes Sep 19, 2023
@ji-eunsoo ji-eunsoo self-requested a review September 19, 2023 02:38
@dotani1111
Copy link
Contributor

@KenTanaka
テスト仕様書とdoc4の追加をお願いします。

shinya
shinya previously approved these changes Sep 22, 2023
ji-eunsoo
ji-eunsoo previously approved these changes Sep 22, 2023
dotani1111
dotani1111 previously approved these changes Sep 22, 2023
@shinya shinya dismissed stale reviews from dotani1111, ji-eunsoo, and themself via 9eece6d September 28, 2023 06:02
@ji-eunsoo ji-eunsoo merged commit 3b0883e into EC-CUBE:4.2 Sep 29, 2023
194 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants