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

Make issued at (iat) claim validation optional #175

Closed
wants to merge 14 commits into from

Conversation

ksegun
Copy link
Contributor

@ksegun ksegun commented Mar 8, 2022

Based on https://datatracker.ietf.org/doc/html/rfc7519/#section-4.1.6 we should only validate the type for map claims.

The current behavior remains unchanged and this allows the caller to disable the check.

This PR also adds in the audience validation functional options from the V4 version of the original library.

Mitigates #98

Kolawole Segun added 4 commits March 8, 2022 17:46
Based on https://datatracker.ietf.org/doc/html/rfc7519\#section-4.1.6 we should only validate the type for map claims.

This should permit anyone that relies on the time validation to continue doing so.
Copy link
Collaborator

@oxisto oxisto left a comment

Choose a reason for hiding this comment

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

Nice! Thank you for this PR, this has also been one of the longer outstanding issues and it seems to work quite nicely with our nice non-breaking validation options.

I will dedicate some time over the next days to look through it more carefully. One remark up front: Is it possible to make this the default (as this is what existing users of the library are expecting).

@ksegun
Copy link
Contributor Author

ksegun commented Mar 9, 2022

You should consider adding leeway support if it continues to be time based, effectively this is similar to the 'nbf' check.

Just to clarify, you want the default to be that this is no longer checked? If so that is what this PR does.

@oxisto
Copy link
Collaborator

oxisto commented Mar 9, 2022

You should consider adding leeway support if it continues to be time based, effectively this is similar to the 'nbf' check.

Just to clarify, you want the default to be that this is no longer checked? If so that is what this PR does.

Sorry, I was in a rush this morning when I typed this. I meant the default to be the "old" behaviour. I would propose the following: Keeping IAT checking enabled (per default) in the v4 branch, with an extra remark that we will deprecate this behaviour in v5 and then switch the default to disabled in v5. Does that make sense?

@ksegun
Copy link
Contributor Author

ksegun commented Mar 9, 2022

Ok, would you like for me to add leeway support?

@@ -130,6 +134,27 @@ func (c *RegisteredClaims) VerifyIssuer(cmp string, req bool) bool {
return verifyIss(c.Issuer, cmp, req)
}

func (c *RegisteredClaims) validateAudience(req bool, opts ...validationOption) bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it somehow possible to move this function to our validation struct? Otherwise we are duplicating a lot of code. This would also make sense to move the other validate/verify functions there in the long run.

Copy link
Contributor Author

@ksegun ksegun Mar 14, 2022

Choose a reason for hiding this comment

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

Unfortunately, it cannot be moved as each VerifyAudience call is a receiver method on the respective struct. I refactored most of the code into a validator function to reduce duplication.

parser_option.go Outdated
// WithoutIssuedAt is an option to disable the validation of the issued at (iat) claim.
// The current `iat` time based validation is planned to be deprecated in v5

func WithoutIssuedAt() ParserOption {
Copy link
Collaborator

Choose a reason for hiding this comment

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

WithoutIssuedAtValidation? Although that is a bit long

@oxisto oxisto mentioned this pull request Mar 15, 2022
@b4k3r
Copy link

b4k3r commented Mar 28, 2022

@oxisto any update on it?

@oxisto
Copy link
Collaborator

oxisto commented Mar 28, 2022

@oxisto any update on it?

Unfortunately, we are stuck a little bit here because the changes necessary to the Claims interface turned out not to be backwards compatible. So it looks like we need to freeze v4 development at some point and think about how we can do a cleaner solution for v5.

@oxisto oxisto mentioned this pull request Aug 27, 2022
5 tasks
@oxisto
Copy link
Collaborator

oxisto commented Feb 20, 2023

Closing this in favour of our new v5 branch (#234) which will include this feature.

@oxisto oxisto closed this Feb 20, 2023
@ksegun ksegun deleted the iat-validation branch May 12, 2023 02:30
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 this pull request may close these issues.

3 participants