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

Token used before issued #98

Closed
JorritSalverda opened this issue Aug 30, 2021 · 15 comments · Fixed by #236
Closed

Token used before issued #98

JorritSalverda opened this issue Aug 30, 2021 · 15 comments · Fixed by #236

Comments

@JorritSalverda
Copy link

Having switched from github.com/dgrijalva/jwt-go to github.com/golang-jwt/jwt/v4 I still get token used before issued errors due to clock skew, even though @dgrijalva said this check would be removed from v4, in line with jwt spec. See

jwt/claims.go

Lines 63 to 66 in 3f50a78

if !c.VerifyIssuedAt(now, false) {
vErr.Inner = fmt.Errorf("token used before issued")
vErr.Errors |= ValidationErrorIssuedAt
}

However I see that check is still executed at dgrijalva/jwt-go#314 (comment). Is this dependent on #16 getting resolved? Until that's resolved can the VerifyIssuedAt check just be dropped? Or any other suggested workarounds?

@oxisto
Copy link
Collaborator

oxisto commented Aug 30, 2021

Yes, this is somewhat dependent on #16. Meanwhile you can use jwt.TimeFunc to account for clock skew.

jwt/token.go

Lines 10 to 13 in 2ebb50f

// TimeFunc provides the current time when parsing token to validate "exp" claim (expiration time).
// You can override it to use another time value. This is useful for testing or if your
// server uses a different time zone than your tokens.
var TimeFunc = time.Now

@JorritSalverda
Copy link
Author

Thanks, I'll test with

jwt.TimeFunc = func() time.Time {
		return time.Now().UTC().Add(time.Second * 20)
	}

to see if it gets rid of these errors.

@mfridman
Copy link
Member

Hey @JorritSalverda did that solve your issue?

@oxisto Were we still interested in #16

@JorritSalverda
Copy link
Author

JorritSalverda commented Sep 13, 2021

Hi @mfridman for some reason it didn't help and i resorted to jumping through a lot of hoops to ignore this type of validation error, see https://github.com/estafette/estafette-ci-api/blob/main/pkg/clients/bitbucketapi/client.go#L277-L309

Looking at my logs the now value is actually larger than iat, so have to figure out how to get the actual value for now used during evaluation. I think though it's probably only off by a couple of milliseconds so not sure why overriding the TimeFunc didn't do it's job.

@mfridman
Copy link
Member

Thanks for the insight. I think introducing (configurable) leeway could help with these sorts of issues.

Side note, every-time I see folks having to resort binary operators to determine the error it makes me cringe, because it's so prone to error.

@shogo82148
Copy link
Contributor

Is the error "token used before issued" really needed?

RFC7519 says that:

https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.4
The "exp" (expiration time) claim identifies the expiration time on or after which the JWT MUST NOT be accepted for processing.

RFC7519 also says that:

https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.5
The "nbf" (not before) claim identifies the time before which the JWT MUST NOT be accepted for processing.

however, RFC7519 DOES NOT say that the "iat" (issued at) claim identifies the time before which the JWT MUST NOT be accepted for processing.
it just says that:

https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.6
The "iat" (issued at) claim identifies the time at which the JWT was issued. This claim can be used to determine the age of the JWT. Its value MUST be a number containing a NumericDate value. Use of this claim is OPTIONAL.

Some implementations of JWT have the maxAge option instead of the error "token used before issued".

https://github.com/auth0/node-jsonwebtoken/blob/74d5719bd03993fcf71e3b176621f133eb6138c0/verify.js#L199-L211

@oxisto
Copy link
Collaborator

oxisto commented Sep 27, 2021

Is the error "token used before issued" really needed?

RFC7519 says that:

https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.4
The "exp" (expiration time) claim identifies the expiration time on or after which the JWT MUST NOT be accepted for processing.

RFC7519 also says that:

https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.5
The "nbf" (not before) claim identifies the time before which the JWT MUST NOT be accepted for processing.

however, RFC7519 DOES NOT say that the "iat" (issued at) claim identifies the time before which the JWT MUST NOT be accepted for processing.
it just says that:

https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.6
The "iat" (issued at) claim identifies the time at which the JWT was issued. This claim can be used to determine the age of the JWT. Its value MUST be a number containing a NumericDate value. Use of this claim is OPTIONAL.

Some implementations of JWT have the maxAge option instead of the error "token used before issued".

https://github.com/auth0/node-jsonwebtoken/blob/74d5719bd03993fcf71e3b176621f133eb6138c0/verify.js#L199-L211

Yup, it is not really necessary to enforce it according to the specification. That is why in the long run it would be good to implement #16.

@oxisto
Copy link
Collaborator

oxisto commented Feb 18, 2022

@JorritSalverda Could you have a look at #139 to see if that mitigates your problem? Once we have that merged in, it will also pave way to a backwards-compatible way of configuration the validation. This was sort of a chicken-egg problem and we choose to merge in the egg first.

@rcmaples
Copy link

rcmaples commented Feb 26, 2022

Is there a fix or workaround for this issue I can use before the various PRs are merged? I'm not sure I completely understand how my local system issuing the token and then validating the token in a subsequent request is subject to clock skew, but really hoping to find a workaround or solution here.
disregard. I'm still new here; updating to v4 resolved the issue.

@daniel-cohen
Copy link

@oxisto : For v4, Do you think we can also add an option to enable the issued at validation (should be disabled by default) to #139 ?

Something along the lines of:

func (c *RegisteredClaims) VerifyIssuedAt(cmp time.Time, req bool, opts ...validationOption) bool {
	validator := validator{}
	for _, o := range opts {
		o(&validator)
	}
	
	if validator.validateIAT {
		if c.IssuedAt == nil {
			return verifyIat(nil, cmp, req)
		}
		return verifyIat(&c.IssuedAt.Time, cmp, req)
	}
	
	return true
}

I don't think I'd want a leeway option for Issued at. I'd just want to turn it off.
But, we can combine the two as well:

func (c *RegisteredClaims) VerifyIssuedAt(cmp time.Time, req bool, opts ...validationOption) bool {
	validator := validator{}
	for _, o := range opts {
		o(&validator)
	}
	
	if validator.validateIAT {
		if c.IssuedAt == nil {
			return verifyIat(nil, cmp, req, validator.leeway)
		}
		return verifyIat(&c.IssuedAt.Time, cmp, req, validator.leeway)
	}
	
	return true
}

@oxisto
Copy link
Collaborator

oxisto commented Mar 7, 2022

@oxisto : For v4, Do you think we can also add an option to enable the issued at validation (should be disabled by default) to #139 ?

Something along the lines of:

func (c *RegisteredClaims) VerifyIssuedAt(cmp time.Time, req bool, opts ...validationOption) bool {
	validator := validator{}
	for _, o := range opts {
		o(&validator)
	}
	
	if validator.validateIAT {
		if c.IssuedAt == nil {
			return verifyIat(nil, cmp, req)
		}
		return verifyIat(&c.IssuedAt.Time, cmp, req)
	}
	
	return true
}

I don't think I'd want a leeway option for Issued at. I'd just want to turn it off. But, we can combine the two as well:

func (c *RegisteredClaims) VerifyIssuedAt(cmp time.Time, req bool, opts ...validationOption) bool {
	validator := validator{}
	for _, o := range opts {
		o(&validator)
	}
	
	if validator.validateIAT {
		if c.IssuedAt == nil {
			return verifyIat(nil, cmp, req, validator.leeway)
		}
		return verifyIat(&c.IssuedAt.Time, cmp, req, validator.leeway)
	}
	
	return true
}

We are still waiting for some minor last fixes in that PR. Once this PR is merged in, we would welcome any additional PRs on this validation topic.

alek-sys added a commit to alek-sys/auth that referenced this issue Jun 24, 2022
golang-jwt library is trying to validate iat claim of the ID token and
due to not accounting for clock skew, validation pretty randomly fails.

There is an open issue golang-jwt/jwt#98 and
seems like that is fixed in v4. However it is still unclear why iat is
validation in the first place, that's not required by RFC and doesn't
seem like the right thing to do. Only nbf and exp claims should be used
for token lifetime validity check.

Also, update README to show how to configure OpenID providers.
alek-sys added a commit to alek-sys/auth that referenced this issue Jun 28, 2022
golang-jwt library is trying to validate iat claim of the ID token and
due to not accounting for clock skew, validation pretty randomly fails.

There is an open issue golang-jwt/jwt#98 and
seems like that is fixed in v4. However it is still unclear why iat is
validation in the first place, that's not required by RFC and doesn't
seem like the right thing to do. Only nbf and exp claims should be used
for token lifetime validity check.

Also, update README to show how to configure OpenID providers.
@oxisto oxisto linked a pull request Aug 27, 2022 that will close this issue
5 tasks
This was referenced Aug 27, 2022
@alexellis
Copy link

I landed here after getting "token used before issued" from this library when consuming the OIDC token from GitHub Actions. I'm not sure what kind of skew there may be, but the receiving machine has a correct date/time AFAIK.

What's the best workaround for this?

tendant pushed a commit to tendant/auth that referenced this issue Oct 10, 2022
golang-jwt library is trying to validate iat claim of the ID token and
due to not accounting for clock skew, validation pretty randomly fails.

There is an open issue golang-jwt/jwt#98 and
seems like that is fixed in v4. However it is still unclear why iat is
validation in the first place, that's not required by RFC and doesn't
seem like the right thing to do. Only nbf and exp claims should be used
for token lifetime validity check.

Also, update README to show how to configure OpenID providers.
@mdjarv
Copy link

mdjarv commented Oct 12, 2022

@alexellis I ran in to this problem earlier this week and solved it by making a custom Claims type like this:

type Claims struct {
	ValidFunc func() error
	jwt.StandardClaims
}

func (c Claims) Valid() error {
	if c.ValidFunc == nil {
		return c.StandardClaims.Valid()
	}
	return c.ValidFunc()
}

func main() {
	claims := Claims{}

	claims.ValidFunc = func() error {
		claims.IssuedAt = 0
		return claims.StandardClaims.Valid()
	}

	jwt.ParseWithClaims(tokenString, &claims, keyFunc)
}

The reason this works is that iat is not marked as required in StandardClaims Valid() function and if set to 0 it will be ignored. This way I don't have to reimplement all the other validations myself.

jwt/claims.go

Lines 122 to 127 in 2ebb50f

func verifyIat(iat int64, now int64, required bool) bool {
if iat == 0 {
return !required
}
return now >= iat
}

@oxisto
Copy link
Collaborator

oxisto commented Oct 12, 2022

@alexellis I ran in to this problem earlier this week and solved it by making a custom Claims type like this:

type Claims struct {
	ValidFunc func() error
	jwt.StandardClaims
}

func (c Claims) Valid() error {
	if c.ValidFunc == nil {
		return c.StandardClaims.Valid()
	}
	return c.ValidFunc()
}

func main() {
	claims := Claims{}

	claims.ValidFunc = func() error {
		claims.IssuedAt = 0
		return claims.StandardClaims.Valid()
	}

	jwt.ParseWithClaims(tokenString, &claims, keyFunc)
}

The reason this works is that iat is not marked as required in StandardClaims Valid() function and if set to 0 it will be ignored. This way I don't have to reimplement all the other validations myself.

jwt/claims.go

Lines 122 to 127 in 2ebb50f

func verifyIat(iat int64, now int64, required bool) bool {
if iat == 0 {
return !required
}
return now >= iat
}

While this works, I personally would not recommend that as this is going a route that will probably be changed in the upcoming v5 version. If you are interested, you can check out the branch of the PR mentioned above. It has support for skipping IAT / currently defaults to skipping IAT.

@oxisto
Copy link
Collaborator

oxisto commented Feb 21, 2023

Fixed by #234

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 a pull request may close this issue.

8 participants