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

Compliance with published RFC7519 #11

Closed
4 tasks
oxisto opened this issue May 28, 2021 · 2 comments · Fixed by #15
Closed
4 tasks

Compliance with published RFC7519 #11

oxisto opened this issue May 28, 2021 · 2 comments · Fixed by #15
Assignees

Comments

@oxisto
Copy link
Collaborator

oxisto commented May 28, 2021

When the library was initially started, RFC7519 did not exist - only its drafts. This has lead to some shortcomings over the years where the implementation, especially of the claims, diverged from the now published RFC.

For example, the draft at the time of the writing of the claims struct actually stated that iat, exp and so on must be an IntDate, which did not include the possibility of non-integer date representations. It got then later changed to a NumericDate format in draft version 26, which - as @dunglas correctly pointed out - included also float representations (also strings).

The aim would be to be as backwards compatible as possible, e.g. with the introduction of a new struct as suggested by @mfridman, which I will name for the lack of a better name jwt.RFC7159Claims for now.

This also was the original problem behind the audience mixup in #6

Points to consider (and possibly replaced with a new struct or function):

  • The fields ExpiresAt, IssuedAt, NotBefore in jwt.StandardClaims are int64 instead of a (to be defined) NumericDate type. Probably solvable with a new struct
  • The functions VerifyIssuedAt, VerifyNotBefore, VerifyIssuedAt of jwt.StandardClaims only compare against an int64 instead of the NumericDate. Also probably solvable with a new struct
  • The functions VerifyIssuedAt, VerifyNotBefore, VerifyIssuedAt of jwt.MapClaims only compare against an int64 instead of the NumericDate. Trickier. I do not want to touch jwt.MapClaims too much. Probably the introduction of a new set of functions can be made and the old ones set to deprecated.
  • Audience of jwt.StandardClaims should be a []string instead of string. Could also be part of the new struct

Anything else that comes to mind?

At some point, it would also probably a good idea to check, whether RFC8725 is also properly reflected in the implementation.

Originally posted by @oxisto in #9 (comment)

@oxisto oxisto changed the title RFC7519 Compliance Published RFC7519 Compliance May 28, 2021
@oxisto oxisto self-assigned this May 28, 2021
@oxisto oxisto changed the title Published RFC7519 Compliance Compliance with published RFC7519 May 28, 2021
@qbiqing
Copy link

qbiqing commented Aug 6, 2021

Audience of jwt.StandardClaims should be a []string instead of string. Could also be part of the new struct

This would be useful to me.

@oxisto
Copy link
Collaborator Author

oxisto commented Aug 6, 2021

Audience of jwt.StandardClaims should be a []string instead of string. Could also be part of the new struct

This would be useful to me.

Yeah, I created a new type ClaimStrings which can be either serialized/deserialized as a []string or string. The PR is ready (#15), just waiting for an approval by another maintainer before I can merge this in.

oxisto added a commit that referenced this issue Aug 22, 2021
…tructure (#15)

This PR aims at implementing compliance to RFC7519, as documented in #11 without breaking the public API. It creates a new struct `RegisteredClaims` and deprecates (but not removes) the `StandardClaims`. It introduces a new type `NumericDate`, which represents a JSON numeric date value as specified in the RFC. This allows us to handle float as well as int-based time fields in `aud`, `exp` and `nbf`. Additionally, it introduces the type `StringArray`, which is basically a wrapper around `[]string` to deal with the oddities of the JWT `aud` field.
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.

2 participants