OAuth2 token expiry logic

[ise...@gmail.com]

Hi,

I have been looking at various packages to understand how one goes about implementing things in Go. I have been learning a great deal just by getting my head around code written by smarter devs than I'm ever likely to be ;)

Whilst looking at golang/oauth2, two lines of code handling token expiry led me to discover about time.Time's use of both wall and monotonic clocks. I realised that either I had failed to understand something or the code was wrong. I'm assuming it's the former, so hopefully somebody can explain it to me...

In oauth2/internal/token.go, doTokenRoudnTrip sets the Token.Expiry as follows:

token.Expiry = time.Now().Add(time.Duration(expires) * time.Second)

 

In oauth2/token.go, the Token.expired function returns this:

return t.Expiry.Round(0).Add(-expiryDelta).Before(timeNow())

If I have read the time package documentation correctly, the monotonic clock reading is intended for time measurements. In particular, it provides robust behaviour for functions such as Before().

So I do not understand why the monotonic reading is stripped from the time value with Round(0) before invoking Before() in the code above. As I read it, the token.Expiry has a monotonic clock reading (now + duration) which is then stripped when evaluated against Before(now) later on... does this not make the code susceptible to issues when system time is being adjusted? What is the rationale for Round(0) in this case?

-- ise

[crate...@gmail.com]

According to https://golang.org/pkg/time/#Duration.Round "If m <= 0, Round returns d unchanged". So now I'm really curious, why do the Round() at all?

s

[ISE Development]

On Monday, 20 April 2020 15:04:54 UTC+1, crate...@gmail.com wrote:

According to https://golang.org/pkg/time/#Duration.Round "If m <= 0, Round returns d unchanged". So now I'm really curious, why do the Round() at all?

But in this case, it's a time.Time value, so this applies (https://pkg.go.dev/time?tab=doc#Time.Round):

Round returns the result of rounding t to the nearest multiple of d (since the zero time). The rounding behavior for halfway values is to round up. If d <= 0, Round returns t stripped of any monotonic clock reading but otherwise unchanged. 

[crate...@gmail.com]

Whoops, you're right. I got my Time and Duration mixed up. Your question still stands, though. The section on Monotonic Clocks at https://pkg.go.dev/time?tab=doc is a bit dense, but my best guess is that stripping the monotonic clock reading from the Expiry ensures that the comparison is made against the actual wall clock time (in the past) vs some unknown adjusted time, which might be earlier or later than the intended time and thus give incorrect results.

s

[David Finkel]

On Mon, Apr 20, 2020 at 11:02 AM <crate...@gmail.com> wrote:

Whoops, you're right. I got my Time and Duration mixed up. Your question still stands, though. The section on Monotonic Clocks at https://pkg.go.dev/time?tab=doc is a bit dense, but my best guess is that stripping the monotonic clock reading from the Expiry ensures that the comparison is made against the actual wall clock time (in the past) vs some unknown adjusted time, which might be earlier or later than the intended time and thus give incorrect results.

(from the peanut gallery)

 

My guess is that it wouldn't be that the monotonic time is incorrect, (in fact it's probably more correct), it's just that it never gets serialized, so you get inconsistent results whether you check the expiration of the unserialized original struct, vs one you parsed if you compare using the monotonic clock.

 

s

On Monday, April 20, 2020 at 7:16:30 AM UTC-7, ISE Development wrote:

On Monday, 20 April 2020 15:04:54 UTC+1, crate...@gmail.com wrote:

According to https://golang.org/pkg/time/#Duration.Round "If m <= 0, Round returns d unchanged". So now I'm really curious, why do the Round() at all?

But in this case, it's a time.Time value, so this applies (https://pkg.go.dev/time?tab=doc#Time.Round):

Round returns the result of rounding t to the nearest multiple of d (since the zero time). The rounding behavior for halfway values is to round up. If d <= 0, Round returns t stripped of any monotonic clock reading but otherwise unchanged. 

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/f871298a-f06d-4e9f-ad20-ee145cdd8004%40googlegroups.com.

[ISE Development]

On Monday, 20 April 2020 16:23:10 UTC+1, David Finkel wrote:

On Mon, Apr 20, 2020 at 11:02 AM <crate...@gmail.com> wrote:

Whoops, you're right. I got my Time and Duration mixed up. Your question still stands, though. The section on Monotonic Clocks at https://pkg.go.dev/time?tab=doc is a bit dense, but my best guess is that stripping the monotonic clock reading from the Expiry ensures that the comparison is made against the actual wall clock time (in the past) vs some unknown adjusted time, which might be earlier or later than the intended time and thus give incorrect results.

(from the peanut gallery)

 

My guess is that it wouldn't be that the monotonic time is incorrect, (in fact it's probably more correct), it's just that it never gets serialized, so you get inconsistent results whether you check the expiration of the unserialized original struct, vs one you parsed if you compare using the monotonic clock.

I was wondering if it had to with serialization. However, no time serialization takes place, as far as I can tell. The OAuth2 token is returned with a time to live (expires_in expressed as seconds). So the Expiry field is set with time.Now() + the time to live duration and subsequently compared with time.Now() (- a hard-coded clock skew adjustment)... so both should have monotonic clock readings and the comparison should be consistent (?).

[Uli Kunitz]

The code of the expired function ensure that only the wall time is used for comparisons. My guess is that they developers of the code wanted to avoid any confusion what times are compared.

[Ross Light]

Round(0) ensures that the expiry is being compared to the wall clock time, since that's what the server uses to verify.

If you're interested, you can look at https://golang.org/cl/83575, the commit that introduced this logic.