Summary:

Implement replay attack mitigation for SAML auth.

Review Request #12476 — Created July 18, 2022 and submitted Aug. 9, 2022, 12:59 p.m.

Information
Owner:	david
Repository:	Review Board
Branch:	release-5.0.x
Bugs:
Depends On:
Reviewers
Groups:	reviewboard
People:

Description

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.

Testing Done


Ran unit tests.
Verified that I could still log in with SAML.
Crafted a replay and saw that it was successfully blocked.

Commits

Summary
	Implement replay attack mitigation for SAML auth.

Screenshots

Issues

Description	From	Last Updated
Can get rid of this TODO. Should we be checking the request ID too?	maubin	July 21, 2022, 10:28 a.m.
The docstring says POST but the method is GET.	maubin	July 21, 2022, 10:31 a.m.
Here we're checking the message and request, but in the other view we check the message and assertion. What's the …	maubin	July 21, 2022, 10:31 a.m.
Can get rid of this.	maubin	July 21, 2022, 10:43 a.m.
True is a literal here, but False is not.	chipx86	Aug. 3, 2022, 2:28 p.m.
Can we go with the usual form of Testing <thing> with <conditions>? Same below.	chipx86	Aug. 3, 2022, 2:28 p.m.

flake8 passed.

JSHint passed.

reviewboard/accounts/sso/backends/saml/views.py (Diff revision 1)

Show all issues

Can get rid of this TODO. Should we be checking the request ID too?

david July 21, 2022, 10:35 a.m.

Based on my reading of the onelogin.saml2 library code (since there's no docs on this), the request ID doesn't get set in this code path, only the message and assertion.

reviewboard/accounts/sso/backends/saml/views.py (Diff revision 1)
Show all issues
```
The docstring says POST but the method is GET.
```

reviewboard/accounts/sso/backends/saml/views.py (Diff revision 1)

Show all issues

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?

david July 21, 2022, 10:35 a.m.

Just a difference in the implementation. Message happens everywhere, but request is for SLS and assertion is for ACS.

Commits:

	Summary
-		Implement replay attack mitigation for SAML auth.
+		Implement replay attack mitigation for SAML auth.

Diff:

Revision 2 (+284 -20)

Show changes

	reviewboard/accounts/sso/backends/saml/views.py
	reviewboard/accounts/tests/test_saml_views.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

reviewboard/accounts/tests/test_saml_views.py (Diff revisions 1 - 2)
Show all issues
```
Can get rid of this.
```

Commits:

	Summary
-		Implement replay attack mitigation for SAML auth.
+		Implement replay attack mitigation for SAML auth.

Diff:

Revision 3 (+278 -20)

Show changes

	reviewboard/accounts/sso/backends/saml/views.py
	reviewboard/accounts/tests/test_saml_views.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/accounts/sso/backends/saml/views.py (Diff revision 3)
Show all issues
```
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?

david Aug. 3, 2022, 2:30 p.m.

I don't think so. There are message/assertion IDs but from what I can tell that's pretty much kept as an internal implementation detail within the SAML protocol and there's not really a log of them generally. This should be enough that people can match it up to requests in their access log and track down the offending host.

reviewboard/accounts/tests/test_saml_views.py (Diff revision 3)

Show all issues

Can we go with the usual form of Testing <thing> with <conditions>?
Same below.

Commits:

	Summary
-		Implement replay attack mitigation for SAML auth.
+		Implement replay attack mitigation for SAML auth.

Diff:

Revision 4 (+278 -20)

Show changes

	reviewboard/accounts/sso/backends/saml/views.py
	reviewboard/accounts/tests/test_saml_views.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-5.0.x (65840cc)