Implement replay attack mitigation for SAML auth.
Review Request #12476 — Created July 18, 2022 and submitted
Information | |
---|---|
david | |
Review Board | |
release-5.0.x | |
Reviewers | |
reviewboard | |
SAML is generally quite secure by design, but if there's a man in the
middle, it is susceptible to replay attacks (within a short limited time
frame). The way this is mitigated is by giving every message and
assertion a unique ID. The service provider can then store these IDs and
make sure that they haven't already been used.This change implements that. The used IDs are kept in the cache, and
checked before we proceed. While using the cache for this isn't 100%
reliable, given the very short message lifetime, it's good enough for
our purposes.
- Ran unit tests.
- Verified that I could still log in with SAML.
- Crafted a replay and saw that it was successfully blocked.
Summary | |
---|---|
Description | From | Last Updated |
---|---|---|
Can get rid of this TODO. Should we be checking the request ID too? |
![]() |
|
The docstring says POST but the method is GET. |
![]() |
|
Here we're checking the message and request, but in the other view we check the message and assertion. What's the … |
![]() |
|
Can get rid of this. |
![]() |
|
True is a literal here, but False is not. |
|
|
Can we go with the usual form of Testing <thing> with <conditions>? Same below. |
|

-
-
reviewboard/accounts/sso/backends/saml/views.py (Diff revision 1) Can get rid of this TODO. Should we be checking the request ID too?
-
reviewboard/accounts/sso/backends/saml/views.py (Diff revision 1) The docstring says POST but the method is GET.
-
reviewboard/accounts/sso/backends/saml/views.py (Diff revision 1) Here we're checking the message and request, but in the other view we check the message and assertion. What's the reason for the difference?
Also should we have a test for this view?
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+284 -20) |
Checks run (2 succeeded)
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+278 -20) |
Checks run (2 succeeded)
-
-
reviewboard/accounts/sso/backends/saml/views.py (Diff revision 3) True
is a literal here, but False is not. -
reviewboard/accounts/sso/backends/saml/views.py (Diff revision 3) Is there anything more we could log here that would help and in other similar logging statements?
-
reviewboard/accounts/tests/test_saml_views.py (Diff revision 3) Can we go with the usual form of
Testing <thing> with <conditions>
?Same below.
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+278 -20) |