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

Tolerate IDP time drift in devise config #53

Merged
merged 1 commit into from
Jan 9, 2024

Conversation

alanmacd06
Copy link
Contributor

A running instance of the app experienced clock drift between it's host and the trusted IdP. The tight default tolerances in the default devise meant this broke SAML response validation when the IdP drifted ahead of the app host.

This PR relaxes the default config which, given the other controls in place in the only known deployment, should be pragmatic and safe

@@ -281,5 +281,5 @@

config.saml_session_index_key = :session_index

config.allowed_clock_drift_in_seconds = 1.second
config.allowed_clock_drift_in_seconds = 5.minutes
Copy link
Contributor

@kenny-lee-1 kenny-lee-1 Jan 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the incident email thread and slack, can I confirm it resolved our end by this approach? I don't believe this occurred before, so the configuration in the email suggests clock drift was always a risk we have had all this time?
is 5 minutes excessive if we had 7 seconds of clock drift?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this resolved the incident. It is arguably excessive, but given the anecdotal tales of ESXi guest behaviour with no NTP config (out by 30 minutes or more), I've made a judgement call. My own view is that, if someone is faking or high-jacked SAML responses given the trust relationship, we have bigger problems than extending the window of abuse by 5 mins.

Our own experience contradicts the anecdotal horror stories; we've always been running this 1.second tolerance and are not aware of any significant outages where the IdP has drifted beyond this, but given we're relying on user-reported issues, rather than granular data from the app itself, I'd be cautious of claiming "we've never seen clock drift before".

@alanmacd06 alanmacd06 merged commit ee8e42e into develop Jan 9, 2024
9 checks passed
@alanmacd06 alanmacd06 deleted the config/tolerate_idp_timedrift branch January 9, 2024 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants