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

Create a setting for the allowed assertion offset #979

Merged
merged 3 commits into from Nov 21, 2018

Conversation

Projects
None yet
4 participants
@Wittev1
Contributor

Wittev1 commented Nov 7, 2018

This was an hardcoded setting that we would like to change. Therefore making it configurable seems the right thing.

We have a customer with an Microsoft ADFS server that's about one minute ahead of time a few times per day. With this setting, we can allow them to be ahead instead of having the customers of the customers complain.

@tvdijen

This comment has been minimized.

Member

tvdijen commented Nov 7, 2018

I suggest we add a filter to this to remain SAML2INT-compliant even when the user enters some stupid value here.. Standard mandates a maximum offset of 5 minutes..

$options = [
    'options' => [
        'default' => 60, // value to return if the filter fails
        // other options here
        'min_range' => 0,
        'max_range' => 300,
    ],
];
$var = filter_var($allowed_assertion_offset, FILTER_VALIDATE_INT, $options);
@Wittev1

This comment has been minimized.

Contributor

Wittev1 commented Nov 7, 2018

Sounds good. It's added.

@tvdijen

tvdijen approved these changes Nov 7, 2018

@tvdijen

This comment has been minimized.

Member

tvdijen commented Nov 7, 2018

Thanks @Wittev1! A second pair of eyes will review this PR and hopefully merge it

@jaimeperez

This comment has been minimized.

Member

jaimeperez commented Nov 8, 2018

Hi guys!

First of all, thanks a lot for your contributtion @Wittev1. I'm a bit skeptical about this though. I'm not sure I like the idea, for two main reasons:

  • The issue here is the ADFS server, which is not keeping its own time properly. It should be the IdP fixing its own issue, not the rest of the world accommodating for their broken setup. In a way, fixing this on our side is a way to encourage them not to fix their broken setup.
  • I would like to avoid adding configuration options if they are not really necessary. We already have way more than I would like, as that makes it more difficult to configure the software. So if we are going to increase the allowed offset, I think it might be better to just set it to the maximum allowed by SAML2Int.
@Wittev1

This comment has been minimized.

Contributor

Wittev1 commented Nov 12, 2018

@jaimeperez You've got two valid points!

  1. I agree this should be solved at de IdP, on the other hand is the IdP not always under your own control. Therefore the SAM2INT states:

Deployments MUST allow between three (3) and five (5) minutes of clock skew — in either direction — when interpreting xsd:dateTime values in assertions and when enforcing security policies based thereupon.

To be SAM2INT compliant this value must be editted anyway (between 180 and 300 seconds, now it's hardcoded on 60).

  1. To prevent an configuration overflow, you can also implement the option in Message.php and ignore the configuration-template. That way someone can alter the value without having to modify the code, but it won't confuse people when configuring.

To remain fully SAML2INT compliant, I changed the following things:

  • Renamed the variable to allowed_clock_skew
  • Altered the range to 180 - 300
  • Altered the default to 180
@thijskh

Yes, I think Jaimes comments have been sufficiently addressed by @Wittev1. I would be in favour of merging.

@thijskh thijskh requested a review from jaimeperez Nov 15, 2018

@jaimeperez jaimeperez merged commit 9a2786d into simplesamlphp:master Nov 21, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@thijskh thijskh added this to the 1.17 milestone Nov 21, 2018

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