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

Migrate to golang-jwt v3.2.1 #166

Merged
merged 1 commit into from
Jul 27, 2021
Merged

Conversation

turkenh
Copy link
Member

@turkenh turkenh commented Jul 27, 2021

Description of your changes

Migrate to golang-jwt v3.2.1 as advised in the description of the security issue CVE-2020-26160.

Fixes GHSA-w73w-5m7g-f7qc

I have:

  • Read and followed Upbound's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR, as appropriate.

How has this code been tested

make e2e.run and verify that validation passes.

@turkenh turkenh force-pushed the migrate-to-golang-jwt branch from b9c290c to 5fd3fad Compare July 27, 2021 08:01
@turkenh turkenh requested review from muvaf and ulucinar July 27, 2021 08:01
@turkenh turkenh requested a review from negz July 27, 2021 10:02
@@ -28,7 +28,7 @@ import (
"syscall"
"time"

"github.com/dgrijalva/jwt-go"
"github.com/golang-jwt/jwt"

Choose a reason for hiding this comment

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

We still refer to the old library (https://github.com/dgrijalva) in a comment at line 438) in this file in the context of a previous fix. Because it's about clock skew, so I would expect that issue to be also relevant with the new library but I have not put thought into it. Just writing here so that we do not overlook it.

Copy link
Member Author

Choose a reason for hiding this comment

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

This should be ok. We already fixed that problem on our end and the issue is referenced just to set the context.

@@ -218,6 +218,8 @@ github.com/gogo/protobuf v1.2.1/go.mod h1:hp+jE20tsWTFYpLwKvXlhS1hjn+gTNwPg2I6zV
github.com/gogo/protobuf v1.2.2-0.20190723190241-65acae22fc9d/go.mod h1:SlYgWuQ5SjCEi6WLHjHCa1yvBfUnHcTbrrZtXPKa29o=
github.com/gogo/protobuf v1.3.1 h1:DqDEcV5aeaTmdFBePNpYsp3FlcVH/2ISVVM9Qf8PSls=
github.com/gogo/protobuf v1.3.1/go.mod h1:SlYgWuQ5SjCEi6WLHjHCa1yvBfUnHcTbrrZtXPKa29o=
github.com/golang-jwt/jwt v3.2.1+incompatible h1:73Z+4BJcrTC+KczS6WvTPvRGOp1WmfEP4Q1lOd9Z/+c=

Choose a reason for hiding this comment

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

Why is github.com/dgrijalva/jwt-go still not removed from go.sum? Is it still an indirect dependency? If so, it may still pose a security threat.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, good point, let me check.

Copy link
Member Author

Choose a reason for hiding this comment

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

This comes as an indirect dependency from labstack/echo library:

go mod why github.com/dgrijalva/jwt-go
# github.com/dgrijalva/jwt-go
github.com/upbound/universal-crossplane/internal/upboundagent
github.com/labstack/echo/v4/middleware
github.com/dgrijalva/jwt-go

Unfortunately, that library does not have a version including a fix rather they claim not to be effected by the issue: labstack/echo#1916 (comment)

Opened another draft PR with replace however, it is failing with another issue during go mod tidy: #167

Not sure how to proceed here yet.

Copy link

@ulucinar ulucinar Jul 27, 2021

Choose a reason for hiding this comment

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

My understanding from that comment is that we are safe in this situation because labstack/echo claims no optional audience verification by default. However, we had better keep an eye on v5 of labstack/echo where they will get rid of the unmaintained github.com/dgrijalva/jwt-go dependency.

Because labstack/echo is the only consumer of github.com/dgrijalva/jwt-go and because they claim no optional audience verification by default, I think we are good to go.

And we had better get rid of the direct dependency asap.

Choose a reason for hiding this comment

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

And if somewhere, we were ourselves doing an optional audience verification using the vulnerable library (github.com/dgrijalva/jwt-go), then it would have been our direct dependency.

Choose a reason for hiding this comment

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

@negz, @daren-eth, we may keep an eye on labstack/echo and github.com/dgrijalva/jwt-go as I had previously observed that we depend on them elsewhere, e.g., upbound-api.

Choose a reason for hiding this comment

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

Thanks @turkenh for the investigation.

Copy link
Member Author

@turkenh turkenh Jul 27, 2021

Choose a reason for hiding this comment

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

Created this issue to track indirect dependencies: #168

@github-actions
Copy link

Successfully created backport PR #169 for release-1.2.

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.

2 participants