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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion oidc/verify.go
View file
Open in desktop
Expand Up @@ -274,7 +274,9 @@ 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
// 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
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.


if nowTime.Add(leeway).Before(nbfTime) {
return nil, fmt.Errorf("oidc: current time %v before the nbf (not before) time: %v", nowTime, nbfTime)
Expand Down