Switch to form3-tech/jwt-go. #1822

dlorenc · 2020-12-21T15:32:30Z

What does this PR achieve? Why do we need it?

The old library is abaondoned and has some severe CVEs in it. The form3-tech
fork has fixed those CVEs and is the most popular fork. CVE is here:
https://snyk.io/vuln/SNYK-GOLANG-GITHUBCOMDGRIJALVAJWTGO-596515

Does this PR fix issues?

Fixes #

Notes for the reviewer

I understand this is going toward maintenance mode. Hoping to get this update in to help unblock other libraries that import heketi from updating.

centos-ci · 2020-12-21T15:36:02Z

Can one of the admins verify this patch?

raghavendra-talur · 2020-12-21T16:22:46Z

@centos-ci ok-to-test

dlorenc · 2020-12-21T16:53:30Z

The test was failing with something that looked unrelated. I added a commit that I think addresses this.

dlorenc · 2020-12-21T19:54:11Z

The build error looks like some kind of timeout. Is there a way to rerun it?

phlogistonjohn · 2021-01-05T14:22:43Z

/retest this please

dlorenc · 2021-01-06T14:05:37Z

This time it looked like a vagrant issue:

14:30:12 + vagrant plugin install vagrant-libvirt
14:30:25 Installing the 'vagrant-libvirt' plugin. This can take a few minutes...
14:30:41 Building native extensions.  This could take a while...
14:30:43 Vagrant failed to properly resolve required dependencies. These
14:30:43 errors can commonly be caused by misconfigured plugin installations
14:30:43 or transient network issues. The reported error is:
14:30:43 
14:30:43 nokogiri requires Ruby version < 3.1.dev, >= 2.5.

iamniting · 2021-01-06T14:31:41Z

Python tests are also getting failed in Github actions. It looks like this got introduced now in the new python version.

2021-01-06T14:08:20.5728022Z >       headers['Authorization'] = b'bearer ' + token
2021-01-06T14:08:20.5728729Z E       TypeError: can't concat str to bytes

>>> 'hello' + 'hello'
'hellohello'
>>> b'hello' + 'hello'
Traceback (most recent call last):
  File "<input>", line 1, in <module>
    b'hello' + 'hello'
TypeError: can't concat str to bytes

phlogistonjohn · 2021-01-08T15:55:35Z

The changes generally look pretty reasonable. The changes to glide look minimal, which is very important, so thanks for keeping them small.

Once some of the CI issues are sorted out we can do proper reviews.

One small request would be to fold your 2 changesets together as having the test fix in with the import changes is more bisection friendly.

dlorenc · 2021-01-10T14:02:46Z

One small request would be to fold your 2 changesets together as having the test fix in with the import changes is more bisection friendly.

All set! I'll keep poking at the CI stuff, not sure if I'll be able to fix it though.

phlogistonjohn · 2021-01-19T18:38:24Z

Looks like a legitimate build failure due to one of our other dependencies, github.com/auth0/go-jwt-middleware still relying on the previous jwt lib. But looking at that project's readme on github its also been switched to use this fork. Perhaps the glide entry for that lib just needs to be tweaked too, @dlorenc ? Were you able to build with the current glide config locally?

dlorenc · 2021-01-20T13:40:53Z

Looks like a legitimate build failure due to one of our other dependencies, github.com/auth0/go-jwt-middleware still relying on the previous jwt lib. But looking at that project's readme on github its also been switched to use this fork. Perhaps the glide entry for that lib just needs to be tweaked too, @dlorenc ? Were you able to build with the current glide config locally?

All set! Looks like things are passing now.

phlogistonjohn · 2021-01-20T13:55:49Z

Great, thanks! The centos ci seems to be having an issue with glide timing out (possibly a network issue?). I want to take a little more time to see if we can sort that out, but another week passes w/o centos ci passing one of may just need to run some tests manually and then we can get on with preparing to merge

dlorenc · 2021-01-20T16:45:02Z

Great, thanks! The centos ci seems to be having an issue with glide timing out (possibly a network issue?). I want to take a little more time to see if we can sort that out, but another week passes w/o centos ci passing one of may just need to run some tests manually and then we can get on with preparing to merge

Is there a way to rerun it? The glide steps take several minutes on my laptop. I think that happened earlier to me too, but a rerun fixed it.

iamniting · 2021-01-20T17:00:02Z

/retest this please

iamniting · 2021-01-20T17:01:40Z

The above comment will run the CI again. But I tried many times on the other PR it is not helping out.

phlogistonjohn · 2021-01-20T18:20:35Z

@iamniting I think @dlorenc is suggesting to run the glide command more than once in the test, not to start the CI job again.
It's worth a try I think.

phlogistonjohn · 2021-01-20T18:35:41Z

I have an experiment at #1833 to try a Fonzie-type approach to getting the CI to do anything.

iamniting · 2021-01-22T18:14:15Z

Cool this one has also passed, Now centos CI is on track

phlogistonjohn

Tests are now passing and everything seems to check out. Thanks!

phlogistonjohn · 2021-01-23T15:51:29Z

@raghavendra-talur PTAL, thanks.

raghavendra-talur · 2021-01-24T22:19:30Z

middleware/jwt.go

@@ -42,7 +42,7 @@ func init() {
 	}
 }

-// From https://github.com/dgrijalva/jwt-go/pull/139 it is understood
+// From https://github.com/form3tech-oss/jwt-go/pull/139 it is understood


This change does not make sense. The PR is only available in the dgrijalva repo. We can keep the comment as-is or modify the comment to not refer to any PR altogether.

raghavendra-talur · 2021-01-24T22:32:24Z

glide.yaml

@@ -1,10 +1,11 @@
 package: github.com/heketi/heketi
 import:
 - package: github.com/auth0/go-jwt-middleware
+  version: 36081240882bbf356af6efb152969e4b0bcf4456


This commit is from 2019 and as @phlogistonjohn mentioned, it will bring in dgrijalva/jwt-go as its dependency. I suggest that the version be bumped up to v1.0.0. v1.0.0 uses the fork "github.com/form3tech-oss/jwt-go".

raghavendra-talur · 2021-01-24T22:32:48Z

glide.yaml

@@ -1,10 +1,11 @@
 package: github.com/heketi/heketi
 import:
 - package: github.com/auth0/go-jwt-middleware
+  version: 36081240882bbf356af6efb152969e4b0bcf4456


Suggested change

version: 36081240882bbf356af6efb152969e4b0bcf4456

version: v1.0.0

phlogistonjohn · 2021-02-05T18:35:19Z

@dlorenc have you seen @raghavendra-talur 's feedback? Do you plan on making changes in reponse?

The old library is abaondoned and has some severe CVEs in it. The form3-tech fork has fixed those CVEs and is the most popular fork. CVE is here: https://snyk.io/vuln/SNYK-GOLANG-GITHUBCOMDGRIJALVAJWTGO-596515 Also switch the key size in the jwt_test to 512, this appears required for go 1.13 Signed-off-by: Dan Lorenc <dlorenc@google.com>

Signed-off-by: Raghavendra Talur <rtalur@redhat.com>

With the migration to form3-tech/jwt-go repo, one of the string replaces also edited a link in the comment to an issue in the old jwt repo. This commit reverses the edit. Signed-off-by: Raghavendra Talur <rtalur@redhat.com>

dlorenc force-pushed the jwt branch from d779c97 to ca5ab3f Compare December 21, 2020 16:38

iamniting requested review from phlogistonjohn and raghavendra-talur January 4, 2021 09:34

dlorenc force-pushed the jwt branch from c8d4c50 to 7d3dfcf Compare January 6, 2021 14:04

dlorenc force-pushed the jwt branch from 7d3dfcf to 1930520 Compare January 10, 2021 14:02

dlorenc force-pushed the jwt branch 2 times, most recently from c1ddb88 to 9ef9550 Compare January 14, 2021 13:39

dlorenc force-pushed the jwt branch from a8ee546 to 5fa889a Compare January 20, 2021 13:23

iamniting approved these changes Jan 20, 2021

View reviewed changes

phlogistonjohn approved these changes Jan 22, 2021

View reviewed changes

raghavendra-talur suggested changes Jan 24, 2021

View reviewed changes

Dan Lorenc and others added 3 commits February 23, 2021 13:17

glide: use go-jwt-middleware version based on form3tech-oss

a996087

Signed-off-by: Raghavendra Talur <rtalur@redhat.com>

middleware: revert the link to jwt issue

44ee562

With the migration to form3-tech/jwt-go repo, one of the string replaces also edited a link in the comment to an issue in the old jwt repo. This commit reverses the edit. Signed-off-by: Raghavendra Talur <rtalur@redhat.com>

raghavendra-talur force-pushed the jwt branch from d52d8ed to 44ee562 Compare February 23, 2021 18:27

Merge branch 'master' into jwt

47d1db5

raghavendra-talur approved these changes Feb 24, 2021

View reviewed changes

phlogistonjohn merged commit 69b5267 into heketi:master Feb 24, 2021

dlorenc deleted the jwt branch May 12, 2021 14:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to form3-tech/jwt-go. #1822

Switch to form3-tech/jwt-go. #1822

dlorenc commented Dec 21, 2020

centos-ci commented Dec 21, 2020

raghavendra-talur commented Dec 21, 2020

dlorenc commented Dec 21, 2020

dlorenc commented Dec 21, 2020

phlogistonjohn commented Jan 5, 2021

dlorenc commented Jan 6, 2021

iamniting commented Jan 6, 2021

phlogistonjohn commented Jan 8, 2021

dlorenc commented Jan 10, 2021

phlogistonjohn commented Jan 19, 2021

dlorenc commented Jan 20, 2021

phlogistonjohn commented Jan 20, 2021

dlorenc commented Jan 20, 2021

iamniting commented Jan 20, 2021

iamniting commented Jan 20, 2021

phlogistonjohn commented Jan 20, 2021

phlogistonjohn commented Jan 20, 2021 •

edited

iamniting commented Jan 22, 2021

phlogistonjohn left a comment

phlogistonjohn commented Jan 23, 2021

raghavendra-talur Jan 24, 2021

raghavendra-talur Jan 24, 2021

raghavendra-talur Jan 24, 2021

phlogistonjohn commented Feb 5, 2021

	version: 36081240882bbf356af6efb152969e4b0bcf4456
	version: v1.0.0

Switch to form3-tech/jwt-go. #1822

Switch to form3-tech/jwt-go. #1822

Conversation

dlorenc commented Dec 21, 2020

What does this PR achieve? Why do we need it?

Does this PR fix issues?

Notes for the reviewer

centos-ci commented Dec 21, 2020

raghavendra-talur commented Dec 21, 2020

dlorenc commented Dec 21, 2020

dlorenc commented Dec 21, 2020

phlogistonjohn commented Jan 5, 2021

dlorenc commented Jan 6, 2021

iamniting commented Jan 6, 2021

phlogistonjohn commented Jan 8, 2021

dlorenc commented Jan 10, 2021

phlogistonjohn commented Jan 19, 2021

dlorenc commented Jan 20, 2021

phlogistonjohn commented Jan 20, 2021

dlorenc commented Jan 20, 2021

iamniting commented Jan 20, 2021

iamniting commented Jan 20, 2021

phlogistonjohn commented Jan 20, 2021

phlogistonjohn commented Jan 20, 2021 • edited

iamniting commented Jan 22, 2021

phlogistonjohn left a comment

Choose a reason for hiding this comment

phlogistonjohn commented Jan 23, 2021

raghavendra-talur Jan 24, 2021

Choose a reason for hiding this comment

raghavendra-talur Jan 24, 2021

Choose a reason for hiding this comment

raghavendra-talur Jan 24, 2021

Choose a reason for hiding this comment

phlogistonjohn commented Feb 5, 2021

phlogistonjohn commented Jan 20, 2021 •

edited