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.

Sign up for GitHub

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

Migrate to golang-jwt v3.2.1 #166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/upbound-agent/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import (
"strings"

"github.com/alecthomas/kong"
"github.com/dgrijalva/jwt-go"
"github.com/golang-jwt/jwt"
"github.com/google/uuid"
"github.com/pkg/errors"
corev1 "k8s.io/api/core/v1"
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ require (
github.com/aws/aws-sdk-go-v2/config v1.1.4
github.com/aws/aws-sdk-go-v2/service/marketplacemetering v1.2.1
github.com/crossplane/crossplane-runtime v0.13.1-0.20210504165942-53874539b310
github.com/dgrijalva/jwt-go v3.2.0+incompatible
github.com/go-resty/resty/v2 v2.5.0
github.com/golang-jwt/jwt v3.2.1+incompatible
github.com/golang/mock v1.5.0
github.com/google/addlicense v0.0.0-20210428195630-6d92264d7170
github.com/google/go-cmp v0.5.5
Expand Down
2 changes: 2 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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=
Copy link

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 •
edited
Loading

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.

Copy link

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.

Copy link

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.

Copy link

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 •
edited
Loading

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.com/golang-jwt/jwt v3.2.1+incompatible/go.mod h1:8pz2t5EyA70fFQQSrl6XZXzqecmYZeUEB8OUGHkxJ+I=
github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b/go.mod h1:SBH7ygxi8pfUlaOkMMuAQtPIUF8ecWP5IEl/CR7VP2Q=
github.com/golang/groupcache v0.0.0-20160516000752-02826c3e7903/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc=
github.com/golang/groupcache v0.0.0-20190129154638-5b532d6fd5ef/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc=
Expand Down
2 changes: 1 addition & 1 deletion internal/controllers/billing/aws/aws.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import (

"github.com/aws/aws-sdk-go-v2/aws"
"github.com/aws/aws-sdk-go-v2/service/marketplacemetering"
"github.com/dgrijalva/jwt-go"
"github.com/golang-jwt/jwt"
"github.com/pkg/errors"
v1 "k8s.io/api/core/v1"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand Down
2 changes: 1 addition & 1 deletion internal/upboundagent/internal/upboundapi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

package internal

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

// CrossplaneAccessor is the struct holding accessor info in JWT custom claims
type CrossplaneAccessor struct {
Expand Down
2 changes: 1 addition & 1 deletion internal/upboundagent/proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import (
"syscall"
"time"

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

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.

"github.com/google/uuid"
"github.com/labstack/echo-contrib/jaegertracing"
"github.com/labstack/echo-contrib/prometheus"
Expand Down
2 changes: 1 addition & 1 deletion internal/upboundagent/proxy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import (

"github.com/crossplane/crossplane-runtime/pkg/logging"

"github.com/dgrijalva/jwt-go"
"github.com/golang-jwt/jwt"
"github.com/google/go-cmp/cmp"
"github.com/labstack/echo/v4"
"github.com/pkg/errors"
Expand Down