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.

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

Add configurable expiration/notbefore leeways #53

Merged
merged 15 commits into from
Jun 21, 2019
Merged

Conversation

jasonodonnell
Copy link
Contributor

@jasonodonnell jasonodonnell commented Jun 13, 2019

Leeways for expiration and not before were hardcoded to 300s (5m) for logins. These are now configurable (with a default of 150s) via two new args: expiration_leeway and not_before_leeway.

Additionally added clock_skew_leeway to configure a 60s (1m) leeway time for all claims. This can be toggled off if no extra leeway is required.

path_role.go Outdated Show resolved Hide resolved
path_login_test.go Outdated Show resolved Hide resolved
path_login.go Outdated Show resolved Hide resolved
path_login.go Outdated Show resolved Hide resolved
path_login.go Outdated
// if no leeways are configured
leeway := time.Second * 0
if role.ExpirationLeeway == 0 && role.NotBeforeLeeway == 0 {
leeway = jwt.DefaultLeeway
Copy link
Member

Choose a reason for hiding this comment

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

This feels odd to me -- this means that if you don't set either of those, we're adding in 5 minutes of leeway above (in each direction), but then another minute here, so it's actually a 12 minute validity period. Granted this leeway accounts for clock skew in either direction, which the above calculations don't, but in the same vein, if you define the leeways above, or if you have iat/nbf defined, now you're not accounting for clock skew.

I wonder if the right logic is that we should always use the library's default leeway value to account for clock skew, but reduce the defaults we use above from 300 seconds down to, say, 150 seconds for each. That gives a validity window of 300 seconds total plus leeway.

If we add leeway all the time we should document it in CHANGES as it's a behavioral change. It may be worth another parameter, to be honest -- something like clock_skew_leeway that gets applied regardless of claims and in both directions, where we default to the library's default but people can turn it off if they don't want an extra minute of validity (which they may well not want).

path_role.go Show resolved Hide resolved
Copy link
Member

@jefferai jefferai left a comment

Choose a reason for hiding this comment

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

Looks good, pending putting Defaults in the schema

"expiration_leeway": {
Type: framework.TypeDurationSecond,
Description: `Duration in seconds of leeway when validating expiration of a token to account for clock skew.
Defaults to 150 (2.5 minutes), minimum of 1 second.`,
Copy link
Member

Choose a reason for hiding this comment

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

Post beta1, can you add text here (and in the other options you added) indicating that setting to 0 will use the default value?

"not_before_leeway": {
Type: framework.TypeDurationSecond,
Description: `Duration in seconds of leeway when validating not before values of a token to account for clock skew.
Defaults to 150 (2.5 minutes), minimum of 1 second..`,
Copy link
Member

Choose a reason for hiding this comment

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

dub period

@jefferai jefferai merged commit 2c3220b into master Jun 21, 2019
@jefferai jefferai deleted the f-jwt-config branch June 21, 2019 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants