New Feature: Clockskew #84

hostage-nl · 2016-05-25T09:50:16Z

Hi,

I experience problems with clients with a slight clock skew, even though their time is synced (the default microsoft way). The time is a tiny bit in the future so WSSE authentication always fails. The server is properly synced with ntp.

With clock skew configuration parameter i added, i've introduced a margin within which clock skew is allowed.

I would appreciate if you could merge this pull request.

Thanks!

martijn.

sagikazarmark · 2016-06-17T13:53:55Z

+10000000000000000000 with the following comments:

AFAIK this feature is called time tolerance or something, not sure if it is "offical" term.
I don't think it makes sense to modify the lifetime as well. It is perfectly fine to modify the configuration instead.

hostage-nl · 2016-06-17T14:37:23Z

I found the term clock skew in someone else's code while researching the problem. I agree that something like time tolerance sounds better and is more self explanatory.
Not sure what you mean, i think i didn't change anything lifetime related.
I fixed the failing test.

sagikazarmark · 2016-06-17T15:19:48Z

Security/Core/Authentication/Provider/Provider.php

@@ -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))


I meant the lifetime here

By adding the clock skew option Source: djoos/EscapeWSSEAuthenticationBundle#84

…kew from lifetime fixes nothing for out of sync clients and only shortness the lifetime for correctly synced clocks.

sagikazarmark · 2016-08-19T10:55:03Z

Any news? I would really love to see this merged.

hostage-nl · 2016-08-23T11:13:14Z

Same here, is there anything i need to do to get this one merged?

Once upon a 19 Aug 2016, Márk Sági-Kazár hit keys in the following order:

Any news? I would really love to see this merged.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.*

Hostage
Kleine Gartmanplantsoen 21
1017 RP Amsterdam
tel: +31 (0)20 4632 303
https://www.hostage.nl

djoos

Hi there,

here a few comments prior to getting this PR merged in.

Thanks in advance for your feedback!
David

djoos · 2016-09-19T09:24:31Z

DependencyInjection/Security/Factory/Factory.php

@@ -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()


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

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

djoos · 2016-09-19T09:41:09Z

Security/Core/Authentication/Provider/Provider.php

@@ -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());


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

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.

djoos · 2016-09-19T09:42:40Z

Tests/DependencyInjection/Security/Factory/FactoryTest.php

@@ -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;


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

…the same

hostage-nl added 4 commits May 25, 2016 09:55

Add configuration option used to mitigate clock skew

2a4f4e2

Fixed lost ->end() call in Factory.php

d544bfd

commit forgotten service.yml update, default clock_skew to 0

8467696

Fixed typo is services.yml

756d38f

Added clock_skew argument to FactoryTest when comparing created objects.

323f810

sagikazarmark reviewed Jun 17, 2016
View reviewed changes

sagikazarmark added a commit to webplates/platform-standard that referenced this pull request Jun 17, 2016

Fix WSSE future token issue

ff1a2c6

By adding the clock skew option Source: djoos/EscapeWSSEAuthenticationBundle#84

sagikazarmark mentioned this pull request Jun 17, 2016

Fix WSSE future token issue webplates/platform-standard#4

Merged

Thanks to sagikazarmark for pointing out that substracting the clocks…

0b1c36e

…kew from lifetime fixes nothing for out of sync clients and only shortness the lifetime for correctly synced clocks.

djoos requested changes Sep 19, 2016

View reviewed changes

hostage-nl added 4 commits October 19, 2016 19:11

Set the default clock skew setting to 0 so default behaviour remains …

0611766

…the same

Add default clock skew of 0 to escape_wsse_authentication.provider

e6cead3

Removed ^M's that got in the last commit somehow

7be6b9e

Set clockSkew=0 in the original FactoryTest

a7c39f1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Feature: Clockskew #84

New Feature: Clockskew #84

hostage-nl commented May 25, 2016

sagikazarmark commented Jun 17, 2016

hostage-nl commented Jun 17, 2016 •

edited

Loading

sagikazarmark Jun 17, 2016

sagikazarmark commented Aug 19, 2016

hostage-nl commented Aug 23, 2016

djoos left a comment

djoos Sep 19, 2016

hostage-nl Oct 19, 2016

djoos Sep 19, 2016 •

edited

Loading

hostage-nl Oct 19, 2016 •

edited

Loading

djoos Sep 19, 2016

New Feature: Clockskew #84

Are you sure you want to change the base?

New Feature: Clockskew #84

Conversation

hostage-nl commented May 25, 2016

sagikazarmark commented Jun 17, 2016

hostage-nl commented Jun 17, 2016 • edited Loading

sagikazarmark Jun 17, 2016

Choose a reason for hiding this comment

sagikazarmark commented Aug 19, 2016

hostage-nl commented Aug 23, 2016

djoos left a comment

Choose a reason for hiding this comment

djoos Sep 19, 2016

Choose a reason for hiding this comment

hostage-nl Oct 19, 2016

Choose a reason for hiding this comment

djoos Sep 19, 2016 • edited Loading

Choose a reason for hiding this comment

hostage-nl Oct 19, 2016 • edited Loading

Choose a reason for hiding this comment

djoos Sep 19, 2016

Choose a reason for hiding this comment

hostage-nl commented Jun 17, 2016 •

edited

Loading

djoos Sep 19, 2016 •

edited

Loading

hostage-nl Oct 19, 2016 •

edited

Loading