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

Jump to bottom

Increase nbf-leeway to 5 minutes #316

Merged
merged 1 commit into from
Oct 9, 2021
Merged

Increase nbf-leeway to 5 minutes #316

merged 1 commit into from
Oct 9, 2021

Conversation

cgostuff
Copy link
Contributor

@cgostuff cgostuff commented Sep 20, 2021 •
edited
Loading

The default clock skew for MS JWT validation is 5 minutes. Increasing the default nbf-leeway here would match that: https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/6.12.2/src/Microsoft.IdentityModel.Tokens/TokenValidationParameters.cs#L149-L153

@ericchiang
Copy link
Collaborator

The config takes a time function

https://pkg.go.dev/github.com/coreos/go-oidc/v3@v3.1.0/oidc#Config

I also believe SkipExpiryCheck will let you do your own logic here.

Clock skews more than a minute seem like it'd be better to fix the clock skew. E.g. things like TLS validation might fail as well. What value were you hoping to set this to? Can you sync your ntpd before starting your Go program instead?

@cgostuff
Copy link
Contributor Author

cgostuff commented Sep 21, 2021 •
edited
Loading

Thanks for the reply. Passing in a custom time function won't affect the nbf-constraint from what I can see.

Identityserver4 uses a default clock skew value of 5 minutes, so I figured it made sense that it's possible to configure this Go-component to a similar level.

I suppose I could use SkipExpiryCheck and do a completely custom expiry check after running the standard Verify function.

@ericchiang
Copy link
Collaborator

Do you have a link for the identityserver4 skew? Does increasing the skew to 5 minutes work for you, or would you need to set it hire? I'd be open to 5min if there are other components doing that.

@cgostuff
Copy link
Contributor Author

cgostuff commented Sep 21, 2021 •
edited
Loading

This link refers to the default value: IdentityServer/IdentityServer4.AccessTokenValidation#90.
Setting the nbf leeway to 5 min does work for me, I wouldn't need it any higher than that.

@ericchiang
Copy link
Collaborator

If you'd like to update this PR to make the default 5 minutes, I think that'd be fine. Please include the following link in the comment:

https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/6.12.2/src/Microsoft.IdentityModel.Tokens/TokenValidationParameters.cs#L149-L153

I don't think we want to make this configurable. If you need more than 5 minutes, you probably want to fix the clock skew.

@cgostuff cgostuff changed the title Make nbf-leeway configurable Increase nbf-leeway to 5 minutes Sep 22, 2021
@cgostuff
Copy link
Contributor Author

cgostuff commented Sep 22, 2021 •
edited
Loading

I updated the OP with the link you referred to (hopefully this is what you meant), as well as the title to more accurately reflect the new change.

ericchiang
ericchiang approved these changes Sep 28, 2021
View reviewed changes
Copy link
Collaborator

@ericchiang ericchiang left a comment

Choose a reason for hiding this comment

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

lgtm with one nit

Can you squash your commits? (there are 4 now, there should be one)

https://stackoverflow.com/questions/5189560/squash-my-last-x-commits-together-using-git

oidc/verify.go
@@ -274,7 +274,7 @@ func (v *IDTokenVerifier) Verify(ctx context.Context, rawIDToken string) (*IDTok
// If nbf claim is provided in token, ensure that it is indeed in the past.
if token.NotBefore != nil {
nbfTime := time.Time(*token.NotBefore)
leeway := 1 * time.Minute
leeway := 5 * time.Minute
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please include a comment for how this was picked?

// Set to 5 minutes since this is what other OpenID Connect providers do to deal
// with clock skew.
//
// https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/6.12.2/src/Microsoft.IdentityModel.Tokens/TokenValidationParameters.cs#L149-L153
leeway := 5 * time.Minute

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the comment, but messed the squash up.. I suppose I used a number too high in HEAD~X and now I'm not quite sure how to fix it. Any tips?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe try "reset --soft" and reauthor the commit? Something like:

$ git remote -v
origin  https://github.com/cgostuff/go-oidc (fetch)
origin  https://github.com/cgostuff/go-oidc (push)
upstream        https://github.com/coreos/go-oidc (fetch)
upstream        https://github.com/coreos/go-oidc (push)
$ git checkout origin/v3
HEAD is now at 5d613d0 Merge branch 'v3' of https://github.com/cgostuff/go-oidc into v3
$ git reset --soft upstream/v3
$ git status
HEAD detached from origin/v3
Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

        modified:   oidc/verify.go

$ git commit -m "Make leeway for the nbf-constraint configurable"
[detached HEAD dc8d817] Make leeway for the nbf-constraint configurable
 1 file changed, 3 insertions(+), 1 deletion(-)
$ git log -n 2
commit dc8d8177cfe75db23906befb62d442a25dce5eaf (HEAD)
Author: Eric Chiang <ericchiang@google.com>
Date:   Fri Oct 1 18:05:25 2021 +0000

    Make leeway for the nbf-constraint configurable

commit d42db69c79f2fa664fd4156e939bf27bba0d2f68 (tag: v3.1.0, upstream/v3.0.0, upstream/v3, origin/v3.0.0)
Merge: 15b94d9 916b64f
Author: Eric Chiang <ericchiang@google.com>
Date:   Thu Sep 16 17:06:04 2021 -0700

    Merge pull request #315 from ericchiang/issuer

    oidc: add option to override discovered issuer URL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ty for the reply. At "git reset --soft upstream/v3" I get:
fatal: ambiguous argument 'upstream/v3': unknown revision or path not in the working tree.

@ericchiang
Copy link
Collaborator

ericchiang commented Oct 1, 2021 via email

@cgostuff
Copy link
Contributor Author

cgostuff commented Oct 1, 2021

Somehow it has the same exact effect.. I reset to the last commit from the original repository, redo the commit, then it just re-pushes the same changes together with a merge instead of overwriting the history/squashing.

@ericchiang
Copy link
Collaborator

ericchiang commented Oct 1, 2021 via email

@cgostuff
Copy link
Contributor Author

cgostuff commented Oct 1, 2021 •
edited
Loading

If you can send it, that would be great. I apologize for the hassle, I have relied on AD to squash for me in the past. Edit: the branch should be good to go now.

@cgostuff
Copy link
Contributor Author

cgostuff commented Oct 2, 2021

Looks like I finally got it! Thanks for guiding me through this @ericchiang.

@ericchiang ericchiang merged commit 6ce86d9 into coreos:v3 Oct 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericchiang ericchiang ericchiang approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cgostuff @ericchiang