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

Due to the use of fillBytes golang-jwt now requires go1.15 #50

Closed
zeripath opened this issue Aug 1, 2021 · 12 comments
Closed

Due to the use of fillBytes golang-jwt now requires go1.15 #50

zeripath opened this issue Aug 1, 2021 · 12 comments

Comments

@zeripath
Copy link

zeripath commented Aug 1, 2021

jwt/ecdsa.go

Lines 135 to 136 in 4bbdd8a

r.FillBytes(out[0:keyBytes]) // r is assigned to the first half of output.
s.FillBytes(out[keyBytes:]) // s is assigned to the second half of output.

from #33, means that golang-jwt/jwt now requires go1.15.

Is this intentional?

@zeripath
Copy link
Author

zeripath commented Aug 1, 2021

Increasing the required version of go to 1.15 is not an ideal thing to do especially given this library is used elsewhere where the version cannot be increased. Would it be possible to provide a go <=1.14 variant?

@oxisto
Copy link
Collaborator

oxisto commented Aug 1, 2021

jwt/ecdsa.go

Lines 135 to 136 in 4bbdd8a

r.FillBytes(out[0:keyBytes]) // r is assigned to the first half of output.
s.FillBytes(out[keyBytes:]) // s is assigned to the second half of output.

from #33, means that golang-jwt/jwt now requires go1.15.

Is this intentional?

Yes, it was. We had a discussion in this PR here: #28 and somewhere in the initial discussions when we moved over from the original repo. v3.2.1 is the last version that did not have any explicit version requirements. Supported versions were always a little bit vague but it probably required Go 1.13 in some form. It was important for us to secure a release that does not break anything and will fix the CVE that the original repo had.

After the discussion in the PR, for security reasons, we decided to support only the versions of Go officially supported; meaning that they will get security fixes. So starting from the v3.2.2 release, only 1.15 and 1.16 are supported.

@zeripath
Copy link
Author

zeripath commented Aug 1, 2021

This is a fundamental library used in many places that have lower requirements on Go versions and have promised to maintain compatibility with earlier versions of Go. (With suitable caveats about how earlier versions of go are subject to vulnerabilities.)

This change means that we cannot backport golang-jwt/jwt 3.2.2 to Gitea 1.14.6 (which although built with go 1.16 by us has a minimum go version of 1.14) without breaking an API promise and we will have to increase the minimum version of our current pre-release (1.15.0) to go 1.15.

It's placing us in a difficult position of either: not provide a fix containing #40 (!), breaking a promise to maintain compatibility with little or no warning (!), or else having to fork golang-jwt/jwt ourselves (!).

Many other downstream users of the library are going to face the same problem.

I'm happy to provide a sub go 1.15 compatible version of the file in question if it would be acceptable.

@mfridman
Copy link
Member

mfridman commented Aug 1, 2021

You're right, and we should have been more careful with changes like #33. It's an optimization of the code but the tradeoff is reducing compatibility, and IMO not worth it.

Not sure how others feel, but I'd rather not sacrifice compatibility for performance, and we should remove FillBytes to make this library compatible with older versions of Go. Just on this note, I know our stance is to support "the officially supported versions of Go" .. but that doesn't mean we should use newer features unless it really makes sense. In this case FillBytes does not.

@oxisto
Copy link
Collaborator

oxisto commented Aug 1, 2021

Well, we keep going back and forth on this. As you know, I was initially against raising the Go minimum version, but I was convinced by the discussion we had, to raise it to Go 1.15. The arguments were sound and we decided to drop support for lower versions. Thus, I also merged in the PR in question (#33) because we agreed on having 1.15 as the minimum version. In my opinion developers of the library should be able to use all the features of that particular minimum version, otherwise what is the point of "allowing" the version, but not be able to use it? This makes it very complicated for maintainers to judge which PRs are ok and it makes it impossible for developers to know which features one is "allowed" to use.

The more sensible way should have probably been to stick with 1.14 as the minimum version for now, given its still existing presence. But, to be blunt, if we backpaddle again, we start to loose our credibility on these discussions. So I am not quite sure how to proceed now. This will get even worse, because 1.17 is to be arrived in August, so following the original idea, even support for 1.15 will be dropped then.

zeripath added a commit to zeripath/jwt that referenced this issue Aug 1, 2021
Adds simple reimplementation of fillBytesInt for go 1.14 support

Fix golang-jwt#50

Signed-off-by: Andrew Thornton <[email protected]>
@mfridman
Copy link
Member

mfridman commented Aug 1, 2021

But, to be blunt, if we backpaddle again, we start to loose our credibility on these discussions.

Agreed, if we cannot take a stance (and stick to it) this does not reflect well on the maintainers. It would probably help if we created/documented the goals of this repository and roadmap.

I put together a set of items I thought were reasonable for the project as we migrated. #1 (comment)

  1. patch any outstanding security issues
  2. add module support
  3. maintain the existing jwt-go API in its current form
  4. setup GitHub Actions as CI for existing tests
  5. have a few independent maintainers to spread the load and trust of maintenance with the ultimate goal of the Go > community having access to a stable JWT package.

Of course, all of this is open for discussion. I still believe the current v3.x.y (non-module) and /v4 (module) versions should be as backwards compatible as possible. Adding newer features of the std lib should be used sparingly unless it cannot be avoided.

But coming back to this issue, I'd agree with @zeripath and removing FillBytes, even if this means more allocations and decreased performance.

@zeripath
Copy link
Author

zeripath commented Aug 1, 2021

I understand I think it's completely reasonable to increase version requirements - Gitea certainly does, but we make it clear when we're about to abandon support for a version.

But coming back to this issue, I'd agree with @zeripath and removing FillBytes, even if this means more allocations and decreased performance.

There's no need for worsened performance take a look at my PR.

@ripienaar
Copy link

We discussed this before and agreed to support the 2 most recent go versions.

Older go versions do not get security updates and we said we will not promote or support their use.

@mfridman
Copy link
Member

mfridman commented Aug 1, 2021

We discussed this before and agreed to support the 2 most recent go versions.

Older go versions do not get security updates and we said we will not promote or support their use.

Indeed we said we'd follow the Go project, and I still think it's reasonable in terms of security updates.

But if we can avoid it, shouldn't we try to reduce the usage of newer std lib features unless there is no other choice and/or it's a security requirement?

@mfridman
Copy link
Member

mfridman commented Aug 1, 2021

FWIW I'm not keen on keeping this extra code with build tags, as implemented in #51

I guess I'm trying to gauge where others stand on adding newer standard library features (such as FillBytes, added in 1.15 ) that block older versions of Go, even though they are "not officially supported"?

@ripienaar
Copy link

I am fine using new stdlib features of 1.15 when 1.16 is the latest.

We have to draw the line somewhere and that’s where we chose to draw it. If we are copying and pasting backporting new go features it’s a wrong choice.

@zeripath
Copy link
Author

zeripath commented Aug 2, 2021

Which leaves no other option but to fork ☹️

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.

4 participants