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

Backwards-compatible implementation of RFC7519's registered claim's structure #15

Merged
merged 28 commits into from
Aug 22, 2021

Conversation

oxisto
Copy link
Collaborator

@oxisto oxisto commented May 28, 2021

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.

Closes #11

@oxisto oxisto changed the title rfc7519 compliance RFC7519 Compliance May 28, 2021
@oxisto oxisto changed the title RFC7519 Compliance Backwards-compatible implementation of RFC7519 compliance May 28, 2021
@oxisto
Copy link
Collaborator Author

oxisto commented May 28, 2021

Now this introduces a whole range of other problems, specifically: Should we serialise the time as float including the fractions or not? There seems to a lot of confusion about that particular sentence in the RFC, whether actually any form of timestamp like 12345678.123456 should be supported or actually just things like 12345678.000000

Not sure about the state of other JWT libraries, I fear that if we start to serialise as float, this might break more things in the end than it does good.

Update: I saw, that the original author tried to tackle the problem in the v4 branch and was moving in the same direction that I have started. Plus he introduced a TimePrecision variable, which can be used to modify the precision.

The question is: What would be a sensible default? Just seconds to basically ignore the fractions, but allowing developers to be more precise, if needed?

@oxisto oxisto changed the title Backwards-compatible implementation of RFC7519 compliance Backwards-compatible implementation of RFC7519's claim's structure May 28, 2021
@oxisto oxisto marked this pull request as ready for review May 28, 2021 12:51
@oxisto
Copy link
Collaborator Author

oxisto commented May 28, 2021

There is still a weird flaky test failure on Go 1.15, but otherwise, this would be ready for a first review. @mfridman did you have something like this in mind?

@lggomez
Copy link
Member

lggomez commented May 29, 2021

Now this introduces a whole range of other problems, specifically: Should we serialise the time as float including the fractions or not? There seems to a lot of confusion about that particular sentence in the RFC, whether actually any form of timestamp like 12345678.123456 should be supported or actually just things like 12345678.000000

In this regard, RFC 3339 which is supersedes ISO 8601 as the standard for the HTTP date data type clearly states that dates must be serialized at complete precision, even at units of magnitude with zero value or in the absence of a timezone

Not sure about the state of other JWT libraries, I fear that if we start to serialize as float, this might break more things in the end than it does good.

Well, that's the ugly side of standards. As developers we still cope with the misuse of IEEE 754 and its border cases that are by design and using it in scenarios which are not clearly suitable for it like currencies. Enforcing standards can have unexpected side effects, so I get that in this scenario we do not want to break existing projects, and I agree to support extra precisions only if wanted. Kinda contradicts my previous point but it is a way to maintain the backwards compatibility in this current version

@oxisto oxisto force-pushed the rfc7519-compliance branch 2 times, most recently from 67be5e3 to 7a54692 Compare May 29, 2021 10:48
@oxisto oxisto force-pushed the rfc7519-compliance branch from 7a54692 to 29000da Compare May 29, 2021 10:53
@oxisto oxisto changed the title Backwards-compatible implementation of RFC7519's claim's structure Backwards-compatible implementation of RFC7519's registered claim's structure May 29, 2021
@oxisto oxisto added this to the v3.3.0 milestone May 29, 2021
@oxisto oxisto force-pushed the rfc7519-compliance branch from 78d8693 to 5149342 Compare May 29, 2021 15:11
@oxisto
Copy link
Collaborator Author

oxisto commented Aug 3, 2021

I merged in v4, there were some conflicts but everything should be ready now.

@oxisto
Copy link
Collaborator Author

oxisto commented Aug 9, 2021

I know everyone is busy (including myself). Giving a small nudge to @mfridman, @lggomez, @Waterdrips, @ripienaar. There are several request for this coming up in the issues. However it is a big PR so I definitely want to have some reviews here before we merge it :)

Copy link
Member

@lggomez lggomez left a comment

Choose a reason for hiding this comment

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

I'd look to replicate #83 into this new implementation, otherwise it LGTM

@mfridman
Copy link
Member

I know everyone is busy (including myself). Giving a small nudge to @mfridman, @lggomez, @Waterdrips, @ripienaar. There are several request for this coming up in the issues. However it is a big PR so I definitely want to have some reviews here before we merge it :)

Apologies, I have been quite busy as of late. I'll have some more time this week and can take a look if others haven't.

types.go Outdated Show resolved Hide resolved
@lggomez
Copy link
Member

lggomez commented Aug 21, 2021

Sorry for the delay. I left a comment on the documentation some days prior, and now I came back with some feedback in relation to a possible quirk on the TimePrecision nanoseconds case which, should not be a bug, must be documented for the users so they can be aware. Otherwise, aside from these 2 comments I have no other comments from my side

mfridman
mfridman previously approved these changes Aug 21, 2021
Copy link
Member

@mfridman mfridman left a comment

Choose a reason for hiding this comment

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

lgtm.

types.go Show resolved Hide resolved
types.go Outdated Show resolved Hide resolved
@oxisto oxisto requested a review from lggomez August 21, 2021 20:49
@oxisto
Copy link
Collaborator Author

oxisto commented Aug 21, 2021

I'd look to replicate #83 into this new implementation, otherwise it LGTM

Done

@oxisto oxisto requested a review from mfridman August 21, 2021 20:49
lggomez
lggomez previously approved these changes Aug 21, 2021
Copy link
Member

@lggomez lggomez left a comment

Choose a reason for hiding this comment

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

LGTM

@oxisto
Copy link
Collaborator Author

oxisto commented Aug 22, 2021

Should be good to go then, I had to resolve one last merge conflict. Sorry @lggomez this dismissed your review for some reason... I need another approve for it to merge :)

@oxisto oxisto requested a review from lggomez August 22, 2021 08:22
@mfridman
Copy link
Member

Lgtm

@oxisto oxisto merged commit 80625fb into golang-jwt:main Aug 22, 2021
@mfridman
Copy link
Member

mfridman commented Sep 3, 2021

@oxisto I saw a few mentions to this ticket, are we safe to tag a new release 4.1.0 ?

This was on of the main changes since the prior release, figured I'd ask just in case.

@oxisto
Copy link
Collaborator Author

oxisto commented Sep 4, 2021

@oxisto I saw a few mentions to this ticket, are we safe to tag a new release 4.1.0 ?

This was on of the main changes since the prior release, figured I'd ask just in case.

We might need to wait until we resolve #86

@mfridman
Copy link
Member

mfridman commented Sep 11, 2021

I'm not sure how best to coordinate releases, maybe discussions or a tracking "releases" issue?

Just confirming now that #86 is landed we can cut a release: 4.1.0

cc @oxisto

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.

Incorporate string array in aud field from the original v4 branch Compliance with published RFC7519
6 participants