search

transip / gotransip

TransIP API client for Go
MIT License
28 stars 14 forks source link

Please consider lowering "expirationSkew" #29

Closed gaaf closed 4 years ago

gaaf commented 4 years ago

(Problem) Description

I'm generating tokens with an expiry of 30 minutes. I noticed that I got multiple tokens created despite the token being in the cache. After investigating, I found the lib assumes time can skew as much as half an hour. This results in every token I just generated to immediately be invalidated.

Steps to reproduce

  1. Generate a token with expiry of 30 minutes
  2. Store token in cache
  3. Retrieve token from cache 1 second later
  4. Try to use that token
  5. A new token is generated

There also seems to be some rate limiting going on, as request fail lots of times with tokens generated in quick succession.

Expected behaviour

To have the token usable for (close to) the configured expiry

Actual behaviour

Non-expired token invalidated immediately

Suggested fix

Lower the expirationSkew to a more sensible value (where I see "sensible" as a reasonable and acceptable expected time skew). Having a more-or-less synchronised time is not exactly rocket-science for a few decades now.

I suggest 1/2 the minimum allowed expiry, if such thing exists. A few second up to 60 sec otherwise. Or make it (package-wide) configurable instead of const.

dionbosschieter commented 4 years ago

Hi Alex,

Thanks for the feedback. The skew in combination with the expiry validation is solely a token thing, the authenticator has the expiration time because it requests new tokens. The validation of it happens in the jwt package and I would like to keep that logic there. What we could do is load the timezone of the api server token expiration date and in combination with that lower the skew.

gaaf commented 4 years ago

I fail to see how timezones are involved as the exp is an epoch. Nor do I see how I might have suggested to move the expiry validation away from the jwt package.

Lowering the acceptable skew is a matter of adjusting the value of a const to a "saner" value. Or turning it into an exported var to make it user adjustable.

dionbosschieter commented 4 years ago

Hi Alex,

I don't entirely agree, in theory yes, but this depends on the server implementation of the validation. I just checked the server validation and it seems that lowering the skew shouldn't be an issue, so you are right and we indeed don't have to load a location before checking the ExiryDate value :).

Perhaps you can include this (and test) in your open pull request? Otherwise I would be happy to change it.

gaaf commented 4 years ago

Also, it is a bit weird that the API documentation says:

Access tokens have a default expiration time of 30 minutes

And that in the Go client, tokens with that exact default expiration are almost unusable.

gaaf commented 4 years ago

Perhaps you can include this (and test) in your open pull request?

I don't think it fits there very well. The PR is about creating tokens via this package. This issue can very well be exposed when using tokens generated via the control panel.

Otherwise I would be happy to change it.

Please do ;)

dionbosschieter commented 4 years ago

Hi Alex,

I just committed https://github.com/transip/gotransip/commit/9defadb50daea3d11821aed85498078b9aff4986 and merged it into master.

Thanks for noticing this, I hope this helps you out. I'll close this and monitor your changes on the pull request.