Honour nbf claim if present in ID token #210
Conversation
verify.go
Outdated
| return nil, fmt.Errorf("oidc: failed to unmarshal claims: %v", err) | ||
| } | ||
| if nbfRaw := claimsMap["nbf"]; nbfRaw != nil { | ||
| nbf, ok := nbfRaw.(float64) |
There was a problem hiding this comment.
There was a problem hiding this comment.
My rationale was that the nbf claim is not ID Token specific but a more "common" claim which none of the other ones listed so far is - perhaps there is some value of keeping a distinction between the two? If not I'll add a NotBeforeField as you suggest.
There was a problem hiding this comment.
That struct isn't exported, it's solely used for this method and validation :) Feel free to add any fields to it.
You can also use a pointer to determine presence
type idToken struct {
IssuedAt jsonTime `json:"iat"`
NotBefore *jsonTime `json:"nbf"`
}
// ...
var t idToken
// ... unmarshal
if t.NotBefore != nil {
// "nbf" is present. Evaluate it
}There was a problem hiding this comment.
Nice one! Thanks 👍 Check looking so much simpler now.
verify.go
Outdated
| @@ -276,6 +279,32 @@ 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 !v.config.SkipNbfCheck { | |||
There was a problem hiding this comment.
@ericchiang is this worth a seperate check or should we fold this into SkipExpiryCheck?
There was a problem hiding this comment.
Yeah, just group it under SkipExpiryCheck. No need to add another public flag.
There was a problem hiding this comment.
Done - simpler for sure, though the tests got a little more verbose as I now had to include an exp in all of the nbf test claims or they'd fail.
verify.go
Outdated
| } | ||
| nowTime := now() | ||
|
|
||
| if nowTime.Before(nbfTime) { |
There was a problem hiding this comment.
Add a minute of leeway here to forgive some clock skew. nowTime.Add(1*time.Minute).Before(nbfTime).
There was a problem hiding this comment.
Thanks, did so and added a test case for that.
verify_test.go
Outdated
| @@ -107,6 +107,16 @@ func TestVerify(t *testing.T) { | |||
| }, | |||
| signKey: newRSAKey(t), | |||
| }, | |||
| { | |||
| name: "nbf", | |||
There was a problem hiding this comment.
we need a positive and negative test.
|
Thanks for all the great feedback guys! I think I've addressed your points - let me know if not :) |
verify.go
Outdated
| leeway := 1 * time.Minute | ||
|
|
||
| if nowTime.Add(leeway).Before(nbfTime) { | ||
| return nil, fmt.Errorf("oidc: current time %d before the nbf (not before) time: %d", nowTime.Unix(), nbfTime.Unix()) |
There was a problem hiding this comment.
nit: for consistency with the token expiry above, just used %v for the time formatting
return nil, fmt.Errorf("oidc: current time %v before nbf (not before) time: %v", nowTime, nbfTime)
There was a problem hiding this comment.
Yeah I think there's a point to be made for printing the values in the same form as they come in the token, but consistency is key of course. Fixed.
verify.go
Outdated
| // If nbf claim is provided in token, ensure that it is indeed in the past. | ||
| if token.NotBefore != nil { | ||
| nbfTime := time.Time(*token.NotBefore) | ||
| nowTime := now() |
There was a problem hiding this comment.
Another nit: make sure nowTime is the same for the Expiry and NotBefore check
nowTime := now()
if t.Expiry.Before(nowTime) {
// handle error
}
if token.NotBefore != nil {
nbfTime := time.Time(*token.NotBefore)
leeay := 1 * time.Minute
if nowTime.Add(leeway).Before(nbfTime) {
// handle error.
}
}
|
Thanks for guiding me through this one @ericchiang @mikedanese . Much appreciated 👍 |
As promised :) Not new to the domain but very new to Go programming, so bear with me.