-
Notifications
You must be signed in to change notification settings - Fork 59
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
New Feature: Clockskew #84
Open
hostage-nl
wants to merge
10
commits into
djoos:master
Choose a base branch
from
hostage-nl:clockskew
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
2a4f4e2
Add configuration option used to mitigate clock skew
hostage-nl d544bfd
Fixed lost ->end() call in Factory.php
hostage-nl 8467696
commit forgotten service.yml update, default clock_skew to 0
hostage-nl 756d38f
Fixed typo is services.yml
hostage-nl 323f810
Added clock_skew argument to FactoryTest when comparing created objects.
hostage-nl 0b1c36e
Thanks to sagikazarmark for pointing out that substracting the clocks…
hostage-nl 0611766
Set the default clock skew setting to 0 so default behaviour remains …
hostage-nl e6cead3
Add default clock skew of 0 to escape_wsse_authentication.provider
hostage-nl 7be6b9e
Removed ^M's that got in the last commit somehow
hostage-nl a7c39f1
Set clockSkew=0 in the original FactoryTest
hostage-nl File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we take clock skew (offset) into consideration upon determination whether the token is from the future (by subtracting the skew from the created time), shouldn't we also take the same clock skew (offset) into consideration when determining whether the token is valid (by adding the skew to the created time)?
Perhaps extracting https://github.com/hostage-nl/EscapeWSSEAuthenticationBundle/blob/0b1c36e8151049aba7beeaa27045dd19ca30d65d/Security/Core/Authentication/Provider/Provider.php#L125-L129 into a isTokenExpired-method, where clock skew is used as follows?
strtotime($this->getCurrentTime()) - strtotime($created) - $this->clockSkew > $this->lifetime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, i initially implemented something like this, but the opposite. Your solution would limit the lifetime with correctly synced clocks. I considered the clock skew to be a margin of acceptable error, the allowed time difference between the client clock and the systems clock. So the clock skew would have to be added to the current time, so that a client clock which would be in de past, would have an extended lifetime of clock skew seconds.
Then i thought, is this really a nice to have feature? For a client clock being set to 29 secs in to the future and a clock skew setting of 30 secs, this would mean extending the lifetime by 59 seconds, which didn't feel like something that would be desirable.
I came to the conclusion it would be better to determine the clock skew automatically by determining the difference between the "Created" timestamp of the requesting WSSE header and the servers current time. I was confused about if it would be possible to create something like this, and because the project i was working on was fixed by my patch, i moved on and forgot about it.
So for now i would suggest not using the clock skew for expiration time and maybe file a feature request for dynamic determination of the clock skew.