Support clock skew in claim validation

[mpage23]

Preflight checklist

• [X] I could not find a solution in the existing issues, docs, nor discussions.
• [X] I agree to follow this project's Code of Conduct.
• [X] I have read and am following this repository's Contribution Guidelines.
• [ ] This issue affects my Ory Cloud project.
• [X] I have joined the Ory Community Slack.
• [X] I am signed up to the Ory Security Patch Newsletter.

Describe your problem

Using private_key_jwt authentication via Hydra's token endpoint, Fosite validates the signed object without any consideration for clock skew. We've seen problems specifically with the iat claim. If the client machine's time is a second or two (or more) ahead of the server hosting Hydra, the token request is rejected with "Token used before issued". Similar issues could be encountered with the nbf and exp claims.

Describe your ideal solution

It would be good if Fosite allowed a clock skew that would be applied in the claim validations.

• For iat: verify that now >= iat - ClockSkewSeconds
• For nbf: verify that now >= nbf - ClockSkewSeconds
• For exp: verify that now <= exp + ClockSkewSeconds

Ideally ClockSkewSeconds above would be configurable.

Workarounds or alternatives

The best we could do is make sure that our services are synced to a time service and try to get all clients synced. However, this is not perfect as some clients are mobile devices and may have more variability in time.

Version

v0.40.3-0.20211013150831-5027277a8297 (through Hydra 1.11.2))

Additional Context

I'd be willing to help with this. I can see in Fosite where the clock skew would need to be accommodated in map_claims.go. I'm not sure if there are any other affected places in Fosite. However, it looks like making a clock skew value to be configurable might require changes to some of the MapClaims apis as there are multiple layers between the config object and the MapClaims implementation. If this feature request is accepted, I'd appreciate some guidance in this area.

[aeneasr]

Hey, that sounds like a good idea to resolve! We can probably tolerate ~10 seconds of clock skew per default

[aeneasr]

Any help appreciated :)

[mpage23]

I'm definitely interested in helping out with this. What's the best way to bounce ideas off of someone prior to starting down the path of code changes?

[james-d-elliott]

Just food for thought - and I'm not associated with ory in any way, I think there area couple ways to implement this, either via the configuration, or via an interface that is used to validate several sections of the JWT.

Due to several other areas of the library I somewhat learn personally towards the interface and a default if it's not provided.

[aeneasr]

The linked commit above looks pretty good I think. It would just need to be configurable :)

[github-actions[bot]]

Hello contributors!

I am marking this issue as stale as it has not received any engagement from the community or maintainers for a year. That does not imply that the issue has no merit! If you feel strongly about this issue

• open a PR referencing and resolving the issue;
• leave a comment on it and discuss ideas on how you could contribute towards resolving it;
• leave a comment and describe in detail why this issue is critical for your use case;
• open a new issue with updated details and a plan for resolving the issue.

Throughout its lifetime, Ory has received over 10.000 issues and PRs. To sustain that growth, we need to prioritize and focus on issues that are important to the community. A good indication of importance, and thus priority, is activity on a topic.

Unfortunately, burnout has become a topic of concern amongst open-source projects.

It can lead to severe personal and health issues as well as opening catastrophic attack vectors.

The motivation for this automation is to help prioritize issues in the backlog and not ignore, reject, or belittle anyone.

If this issue was marked as stale erroneously you can exempt it by adding the backlog label, assigning someone, or setting a milestone for it.

Thank you for your understanding and to anyone who participated in the conversation! And as written above, please do participate in the conversation if this topic is important to you!

Thank you 🙏✌️

[james-d-elliott]

Does this look someone adequate (would obviously need to make this available where the functions are used)?

Subject: [PATCH] 651
---
Index: token/jwt/map_claims.go
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/token/jwt/map_claims.go b/token/jwt/map_claims.go
--- a/token/jwt/map_claims.go	(revision 45a6785cc54fcbd9195b0de4b821bb5fed6a41be)
+++ b/token/jwt/map_claims.go	(date 1681264281101)
@@ -27,6 +27,10 @@
 // Compares the aud claim against cmp.
 // If required is false, this method will return true if the value matches or is unset
 func (m MapClaims) VerifyAudience(cmp string, req bool) bool {
+	return m.VerifyAudienceWithVerifier(NewCommonMapClaimsVerifier(0), cmp, req)
+}
+
+func (m MapClaims) VerifyAudienceWithVerifier(verifier MapClaimsVerifier, cmp string, req bool) bool {
 	var aud []string
 	switch v := m["aud"].(type) {
 	case []string:
@@ -44,14 +48,20 @@
 	default:
 		return false
 	}
-	return verifyAud(aud, cmp, req)
+	return verifier.VerifyAudience(aud, cmp, req)
 }
 
 // Compares the exp claim against cmp.
 // If required is false, this method will return true if the value matches or is unset
 func (m MapClaims) VerifyExpiresAt(cmp int64, req bool) bool {
+	return m.VerifyExpiresAtWithVerifier(NewCommonMapClaimsVerifier(0), cmp, req)
+}
+
+// Compares the exp claim against cmp.
+// If required is false, this method will return true if the value matches or is unset
+func (m MapClaims) VerifyExpiresAtWithVerifier(verifier MapClaimsVerifier, cmp int64, req bool) bool {
 	if v, ok := m.toInt64("exp"); ok {
-		return verifyExp(v, cmp, req)
+		return verifier.VerifyExpiresAt(v, cmp, req)
 	}
 	return !req
 }
@@ -59,8 +69,12 @@
 // Compares the iat claim against cmp.
 // If required is false, this method will return true if the value matches or is unset
 func (m MapClaims) VerifyIssuedAt(cmp int64, req bool) bool {
+	return m.VerifyIssuedAtWithVerifier(NewCommonMapClaimsVerifier(0), cmp, req)
+}
+
+func (m MapClaims) VerifyIssuedAtWithVerifier(verifier MapClaimsVerifier, cmp int64, req bool) bool {
 	if v, ok := m.toInt64("iat"); ok {
-		return verifyIat(v, cmp, req)
+		return verifier.VerifyIssuedAt(v, cmp, req)
 	}
 	return !req
 }
@@ -68,15 +82,23 @@
 // Compares the iss claim against cmp.
 // If required is false, this method will return true if the value matches or is unset
 func (m MapClaims) VerifyIssuer(cmp string, req bool) bool {
+	return m.VerifyIssuerWithVerifier(NewCommonMapClaimsVerifier(0), cmp, req)
+}
+
+func (m MapClaims) VerifyIssuerWithVerifier(verifier MapClaimsVerifier, cmp string, req bool) bool {
 	iss, _ := m["iss"].(string)
-	return verifyIss(iss, cmp, req)
+	return verifier.VerifyIssuer(iss, cmp, req)
 }
 
 // Compares the nbf claim against cmp.
 // If required is false, this method will return true if the value matches or is unset
 func (m MapClaims) VerifyNotBefore(cmp int64, req bool) bool {
+	return m.VerifyNotBeforeWithVerifier(NewCommonMapClaimsVerifier(0), cmp, req)
+}
+
+func (m MapClaims) VerifyNotBeforeWithVerifier(verifier MapClaimsVerifier, cmp int64, req bool) bool {
 	if v, ok := m.toInt64("nbf"); ok {
-		return verifyNbf(v, cmp, req)
+		return verifier.VerifyNotBefore(v, cmp, req)
 	}
 
 	return !req
@@ -108,20 +130,28 @@
 // As well, if any of the above claims are not in the token, it will still
 // be considered a valid claim.
 func (m MapClaims) Valid() error {
+	return m.ValidWithVerifier(NewCommonMapClaimsVerifier(0))
+}
+
+func (m MapClaims) ValidWithVerifier(verifier MapClaimsVerifier) error {
+	if verifier == nil {
+		return errors.New("Verifier was not supplied")
+	}
+
 	vErr := new(ValidationError)
 	now := TimeFunc().Unix()
 
-	if !m.VerifyExpiresAt(now, false) {
+	if !m.VerifyExpiresAtWithVerifier(verifier, now, false) {
 		vErr.Inner = errors.New("Token is expired")
 		vErr.Errors |= ValidationErrorExpired
 	}
 
-	if !m.VerifyIssuedAt(now, false) {
+	if !m.VerifyIssuedAtWithVerifier(verifier, now, false) {
 		vErr.Inner = errors.New("Token used before issued")
 		vErr.Errors |= ValidationErrorIssuedAt
 	}
 
-	if !m.VerifyNotBefore(now, false) {
+	if !m.VerifyNotBeforeWithVerifier(verifier, now, false) {
 		vErr.Inner = errors.New("Token is not valid yet")
 		vErr.Errors |= ValidationErrorNotValidYet
 	}
@@ -131,6 +161,7 @@
 	}
 
 	return vErr
+
 }
 
 func (m MapClaims) UnmarshalJSON(b []byte) error {
@@ -149,7 +180,40 @@
 	return nil
 }
 
-func verifyAud(aud []string, cmp string, required bool) bool {
+type MapClaimsVerifier interface {
+	VerifyIssuer(iss string, cmp string, required bool) bool
+	VerifyAudience(aud []string, cmp string, required bool) bool
+	VerifyExpiresAt(exp int64, now int64, required bool) bool
+	VerifyIssuedAt(iat int64, now int64, required bool) bool
+	VerifyNotBefore(nbf int64, now int64, required bool) bool
+}
+
+func NewCommonMapClaimsVerifier(skew int) *CommonMapClaimsVerifier {
+	verifier := &CommonMapClaimsVerifier{skew: int64(skew)}
+
+	if verifier.skew < 0 {
+		verifier.skew = 10
+	}
+
+	return verifier
+}
+
+type CommonMapClaimsVerifier struct {
+	skew int64
+}
+
+func (verifier *CommonMapClaimsVerifier) VerifyIssuer(iss string, cmp string, required bool) bool {
+	if iss == "" {
+		return !required
+	}
+	if subtle.ConstantTimeCompare([]byte(iss), []byte(cmp)) != 0 {
+		return true
+	} else {
+		return false
+	}
+}
+
+func (verifier *CommonMapClaimsVerifier) VerifyAudience(aud []string, cmp string, required bool) bool {
 	if len(aud) == 0 {
 		return !required
 	}
@@ -162,34 +226,23 @@
 	return false
 }
 
-func verifyExp(exp int64, now int64, required bool) bool {
+func (verifier *CommonMapClaimsVerifier) VerifyExpiresAt(exp int64, now int64, required bool) bool {
 	if exp == 0 {
 		return !required
 	}
-	return now <= exp
+	return now <= exp+verifier.skew
 }
 
-func verifyIat(iat int64, now int64, required bool) bool {
+func (verifier *CommonMapClaimsVerifier) VerifyIssuedAt(iat int64, now int64, required bool) bool {
 	if iat == 0 {
 		return !required
 	}
-	return now >= iat
-}
-
-func verifyIss(iss string, cmp string, required bool) bool {
-	if iss == "" {
-		return !required
-	}
-	if subtle.ConstantTimeCompare([]byte(iss), []byte(cmp)) != 0 {
-		return true
-	} else {
-		return false
-	}
+	return now >= iat-verifier.skew
 }
 
-func verifyNbf(nbf int64, now int64, required bool) bool {
+func (verifier *CommonMapClaimsVerifier) VerifyNotBefore(nbf int64, now int64, required bool) bool {
 	if nbf == 0 {
 		return !required
 	}
-	return now >= nbf
+	return now >= nbf-verifier.skew
 }

[github-actions[bot]]

Hello contributors!

I am marking this issue as stale as it has not received any engagement from the community or maintainers for a year. That does not imply that the issue has no merit! If you feel strongly about this issue

• open a PR referencing and resolving the issue;
• leave a comment on it and discuss ideas on how you could contribute towards resolving it;
• leave a comment and describe in detail why this issue is critical for your use case;
• open a new issue with updated details and a plan for resolving the issue.

Throughout its lifetime, Ory has received over 10.000 issues and PRs. To sustain that growth, we need to prioritize and focus on issues that are important to the community. A good indication of importance, and thus priority, is activity on a topic.

Unfortunately, burnout has become a topic of concern amongst open-source projects.

It can lead to severe personal and health issues as well as opening catastrophic attack vectors.

The motivation for this automation is to help prioritize issues in the backlog and not ignore, reject, or belittle anyone.

If this issue was marked as stale erroneously you can exempt it by adding the backlog label, assigning someone, or setting a milestone for it.

Thank you for your understanding and to anyone who participated in the conversation! And as written above, please do participate in the conversation if this topic is important to you!

Thank you 🙏✌️