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.

Sign up for GitHub

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
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

New Feature: Clockskew #84

Show file tree
Hide file tree
Changes from 5 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 May 25, 2016
d544bfd
Fixed lost ->end() call in Factory.php
hostage-nl May 25, 2016
8467696
commit forgotten service.yml update, default clock_skew to 0
hostage-nl May 25, 2016
756d38f
Fixed typo is services.yml
hostage-nl May 25, 2016
323f810
Added clock_skew argument to FactoryTest when comparing created objects.
hostage-nl Jun 17, 2016
0b1c36e
Thanks to sagikazarmark for pointing out that substracting the clocks…
hostage-nl Jun 17, 2016
0611766
Set the default clock skew setting to 0 so default behaviour remains …
hostage-nl Oct 19, 2016
e6cead3
Add default clock skew of 0 to escape_wsse_authentication.provider
hostage-nl Oct 19, 2016
7be6b9e
Removed ^M's that got in the last commit somehow
hostage-nl Oct 19, 2016
a7c39f1
Set clockSkew=0 in the original FactoryTest
hostage-nl Oct 19, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions DependencyInjection/Security/Factory/Factory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,8 @@ public function create(ContainerBuilder $container, $id, $config, $userProviderI
->replaceArgument(3, new Reference($this->encoderId))
->replaceArgument(4, new Reference($this->nonceCacheId))
->replaceArgument(5, $config['lifetime'])
->replaceArgument(6, $config['date_format']);
->replaceArgument(6, $config['date_format'])
->replaceArgument(7, $config['clock_skew']);

$entryPointId = $this->createEntryPoint($container, $id, $config, $defaultEntryPoint);

Expand Down Expand Up @@ -99,7 +100,8 @@ public function addConfiguration(NodeDefinition $node)
->scalarNode('date_format')->defaultValue(
'/^([\+-]?\d{4}(?!\d{2}\b))((-?)((0[1-9]|1[0-2])(\3([12]\d|0[1-9]|3[01]))?|W([0-4]\d|5[0-2])(-?[1-7])?|(00[1-9]|0[1-9]\d|[12]\d{2}|3([0-5]\d|6[1-6])))([T\s]((([01]\d|2[0-3])((:?)[0-5]\d)?|24\:?00)([\.,]\d+(?!:))?)?(\17[0-5]\d([\.,]\d+)?)?([zZ]|([\+-])([01]\d|2[0-3]):?([0-5]\d)?)?)?)?$/'
)->end()
->arrayNode('encoder')
->scalarNode('clock_skew')->defaultValue(60)->end()
Copy link
Owner

Choose a reason for hiding this comment

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

Could you make sure to set a default value of 0?

Copy link
Author

Choose a reason for hiding this comment

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

done, good idea, keeping the behaviour the same as without these changes.

->arrayNode('encoder')
->children()
->scalarNode('algorithm')->end()
->scalarNode('encodeHashAsBase64')->end()
Expand Down
10 changes: 9 additions & 1 deletion Resources/config/services.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,15 @@
services:
escape_wsse_authentication.provider:
class: %escape_wsse_authentication.provider.class%
arguments: ['@security.user_checker', null, null, null, null, 300, '/^([\+-]?\d{4}(?!\d{2}\b))((-?)((0[1-9]|1[0-2])(\3([12]\d|0[1-9]|3[01]))?|W([0-4]\d|5[0-2])(-?[1-7])?|(00[1-9]|0[1-9]\d|[12]\d{2}|3([0-5]\d|6[1-6])))([T\s]((([01]\d|2[0-3])((:?)[0-5]\d)?|24\:?00)([\.,]\d+(?!:))?)?(\17[0-5]\d([\.,]\d+)?)?([zZ]|([\+-])([01]\d|2[0-3]):?([0-5]\d)?)?)?)?$/']
arguments:
- '@security.user_checker'
- null
- null
- null
- null
- 300
- '/^([\+-]?\d{4}(?!\d{2}\b))((-?)((0[1-9]|1[0-2])(\3([12]\d|0[1-9]|3[01]))?|W([0-4]\d|5[0-2])(-?[1-7])?|(00[1-9]|0[1-9]\d|[12]\d{2}|3([0-5]\d|6[1-6])))([T\s]((([01]\d|2[0-3])((:?)[0-5]\d)?|24\:?00)([\.,]\d+(?!:))?)?(\17[0-5]\d([\.,]\d+)?)?([zZ]|([\+-])([01]\d|2[0-3]):?([0-5]\d)?)?)?)?$/'
- 0

escape_wsse_authentication.listener:
class: %escape_wsse_authentication.listener.class%
Expand Down
12 changes: 8 additions & 4 deletions Security/Core/Authentication/Provider/Provider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ class Provider implements AuthenticationProviderInterface
private $nonceCache;
private $lifetime;
private $dateFormat;
private $clockSkew;

/**
* Constructor.
Expand All @@ -36,15 +37,17 @@ class Provider implements AuthenticationProviderInterface
* @param Cache $nonceCache The nonce cache
* @param int $lifetime The lifetime
* @param string $dateFormat The date format
*/
* @param int $clockSkew The margin in seconds to mitigate clock skew
*/
public function __construct(
UserCheckerInterface $userChecker,
UserProviderInterface $userProvider,
$providerKey,
PasswordEncoderInterface $encoder,
Cache $nonceCache,
$lifetime=300,
$dateFormat='/^([\+-]?\d{4}(?!\d{2}\b))((-?)((0[1-9]|1[0-2])(\3([12]\d|0[1-9]|3[01]))?|W([0-4]\d|5[0-2])(-?[1-7])?|(00[1-9]|0[1-9]\d|[12]\d{2}|3([0-5]\d|6[1-6])))([T\s]((([01]\d|2[0-3])((:?)[0-5]\d)?|24\:?00)([\.,]\d+(?!:))?)?(\17[0-5]\d([\.,]\d+)?)?([zZ]|([\+-])([01]\d|2[0-3]):?([0-5]\d)?)?)?)?$/'
$dateFormat='/^([\+-]?\d{4}(?!\d{2}\b))((-?)((0[1-9]|1[0-2])(\3([12]\d|0[1-9]|3[01]))?|W([0-4]\d|5[0-2])(-?[1-7])?|(00[1-9]|0[1-9]\d|[12]\d{2}|3([0-5]\d|6[1-6])))([T\s]((([01]\d|2[0-3])((:?)[0-5]\d)?|24\:?00)([\.,]\d+(?!:))?)?(\17[0-5]\d([\.,]\d+)?)?([zZ]|([\+-])([01]\d|2[0-3]):?([0-5]\d)?)?)?)?$/',
$clockSkew = 0
)
{
if(empty($providerKey))
Expand All @@ -59,6 +62,7 @@ public function __construct(
$this->nonceCache = $nonceCache;
$this->lifetime = $lifetime;
$this->dateFormat = $dateFormat;
$this->clockSkew = $clockSkew;
}

public function authenticate(TokenInterface $token)
Expand Down Expand Up @@ -119,7 +123,7 @@ protected function validateDigest($digest, $nonce, $created, $secret, $salt)
}

//expire timestamp after specified lifetime
if(strtotime($this->getCurrentTime()) - strtotime($created) > $this->lifetime)
if(strtotime($this->getCurrentTime()) - strtotime($created) > ($this->lifetime - $this->clockSkew))
Copy link

Choose a reason for hiding this comment

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

I meant the lifetime here

{
throw new CredentialsExpiredException('Token has expired.');
}
Expand Down Expand Up @@ -158,7 +162,7 @@ protected function getCurrentTime()

protected function isTokenFromFuture($created)
{
return strtotime($created) > strtotime($this->getCurrentTime());
return strtotime($created) - $this->clockSkew > strtotime($this->getCurrentTime());
Copy link
Owner

@djoos djoos Sep 19, 2016 •
edited
Loading

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

Copy link
Author

@hostage-nl hostage-nl Oct 19, 2016 •
edited
Loading

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.

}

protected function isFormattedCorrectly($created)
Expand Down
7 changes: 5 additions & 2 deletions Tests/DependencyInjection/Security/Factory/FactoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ public function testCreate()
$profile = 'someprofile';
$lifetime = 300;
$date_format = '/^([\+-]?\d{4}(?!\d{2}\b))((-?)((0[1-9]|1[0-2])(\3([12]\d|0[1-9]|3[01]))?|W([0-4]\d|5[0-2])(-?[1-7])?|(00[1-9]|0[1-9]\d|[12]\d{2}|3([0-5]\d|6[1-6])))([T\s]((([01]\d|2[0-3])((:?)[0-5]\d)?|24\:?00)([\.,]\d+(?!:))?)?(\17[0-5]\d([\.,]\d+)?)?([zZ]|([\+-])([01]\d|2[0-3]):?([0-5]\d)?)?)?)?$/';
$clock_skew = 60;
Copy link
Owner

Choose a reason for hiding this comment

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

I'd suggest using the default (0) here in the existing tests, but adding some specific tests for the clock skew feature.


$algorithm = 'sha1';
$encodeHashAsBase64 = true;
Expand All @@ -68,7 +69,8 @@ public function testCreate()
'profile' => $profile,
'encoder' => $encoder,
'lifetime' => $lifetime,
'date_format' => $date_format
'date_format' => $date_format,
'clock_skew' => $clock_skew
),
'user_provider',
'entry_point'
Expand Down Expand Up @@ -108,7 +110,8 @@ public function testCreate()
'index_3' => new Reference($encoderId),
'index_4' => new Reference($nonceCacheId),
'index_5' => $lifetime,
'index_6' => $date_format
'index_6' => $date_format,
'index_7' => $clock_skew
),
$definition->getArguments()
);
Expand Down