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

Update jwt lib #769

Closed
johakoch opened this issue Sep 15, 2023 · 4 comments · Fixed by #796
Closed

Update jwt lib #769

johakoch opened this issue Sep 15, 2023 · 4 comments · Fixed by #796

Comments

@johakoch
Copy link
Collaborator

Update JWT lib to version 5. https://github.com/golang-jwt/jwt/releases/tag/v5.0.0

github.com/golang-jwt/jwt/v5

@johakoch
Copy link
Collaborator Author

In version 5, the JWT parser has options to verify issuer and audience (I think, this is similar to version 3 and unlike version 4). That implies that, if the claims for the jwt access control block are to be evaluated per request, each request must have its own parser instance (with parser options set in NewParser()).

@filex
Copy link
Contributor

filex commented Sep 20, 2023 via email

@johakoch
Copy link
Collaborator Author

Can we disable that built-in lib behavior

It's not enabled by default. You can add a specific parser/validation option to the slice of options.

and implement the same checks from our eval context?

I don't like copying the lib code to our code.

There is a validator struct handling the claim validation, which

is intentionally not exported (yet) as we want to
internally finalize its API. In the future, we might make it publicly
available.

Even if it were, newValidator() also creates a new parser with the specified options:

func newValidator(opts ...ParserOption) *validator {
	p := NewParser(opts...)
	return p.validator
}

Anyway, both Parser and validator structs are not heavy-weight:

type Parser struct {
	// If populated, only these methods will be considered valid.
	validMethods []string

	// Use JSON Number format in JSON decoder.
	useJSONNumber bool

	// Skip claims validation during token parsing.
	skipClaimsValidation bool

	validator *validator

	decodeStrict bool

	decodePaddingAllowed bool
}
type validator struct {
	// leeway is an optional leeway that can be provided to account for clock skew.
	leeway time.Duration

	// timeFunc is used to supply the current time that is needed for
	// validation. If unspecified, this defaults to time.Now.
	timeFunc func() time.Time

	// verifyIat specifies whether the iat (Issued At) claim will be verified.
	// According to https://www.rfc-editor.org/rfc/rfc7519#section-4.1.6 this
	// only specifies the age of the token, but no validation check is
	// necessary. However, if wanted, it can be checked if the iat is
	// unrealistic, i.e., in the future.
	verifyIat bool

	// expectedAud contains the audience this token expects. Supplying an empty
	// string will disable aud checking.
	expectedAud string

	// expectedIss contains the issuer this token expects. Supplying an empty
	// string will disable iss checking.
	expectedIss string

	// expectedSub contains the subject this token expects. Supplying an empty
	// string will disable sub checking.
	expectedSub string
}

So it might be ok to create a parser/validator with each request, no?

@malud
Copy link
Collaborator

malud commented Oct 12, 2023

So it might be ok to create a parser/validator with each request, no?

We should revisit this topic while working on this issue. I would like to see a benchmark for this and also to think about some object pooling to reuse the parser somehow.

@johakoch johakoch linked a pull request Jan 30, 2024 that will close this issue
@malud malud closed this as completed in #796 Feb 3, 2024
malud added a commit that referenced this issue Feb 3, 2024
* updated jwt lib to v5.2.0

* store the parsers for 3600s in memStore

* add Changelog entry

---------

Co-authored-by: Marcel Ludwig <marcel.ludwig@milecrew.com>
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.

3 participants