Skip to content

Conversation

@pdavide
Copy link
Contributor

@pdavide pdavide commented Oct 5, 2019

This PR adds support for a strict check about IssueInstant attributes in the requests and the subsequent responses. An accepted clock skew is configurable with the new clockSkewTolerance setting.

All the changes are backward compatible as the IssueInstant check is an opt-in feature.

All the changes are tested and docs updated accordingly.

@pdavide
Copy link
Contributor Author

pdavide commented Oct 5, 2019

@pdavide pdavide force-pushed the issue-instant-check branch from 5a7a06d to c06041b Compare October 5, 2019 11:38
@pitbulk
Copy link
Contributor

pitbulk commented Oct 6, 2019

This feature adds a lot of complexity to the toolkit (new settings and new methods for a lot of methods).

If you want to force that an AuthNRequest or a LogoutRequest has a reply in X time, I think you can do that at a high level, just saving the IDs of the request and timestamp and rejecting "expired" responses.

As far as a understand, SAML does not define a valid time between requests and responses.

Also, take in mind that some authentication process with 2FA and biometrics process can take time, so not sure about the convenience of this kind of restriction.

@pdavide
Copy link
Contributor Author

pdavide commented Oct 7, 2019

Hi @pitbulk, the new check is not about the time between the request and the subsequent response, but for ensuring that the issue instant of the response in not earlier than the one the request.
As suggested in the SAML specifications I introduced a clock skew parameter to allow a tolerance window and avoid false positives.

Summary of changes:

  • no new methods are introduced, only three simple getters;
  • one new setting, not required;
  • 2 optional parameters added to isValid methods in the responses;
  • test added;
  • docs added;
  • no breaking changes;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants