labstack / echo

High performance, minimalist Go web framework
https://echo.labstack.com
MIT License
29.93k stars 2.23k forks source link

Upgrade golang-jwt to v4 #2699

Open inge4pres opened 1 week ago

inge4pres commented 1 week ago

Issue Description

The golang-jwt library imported in the middleware package suffers from a CVE.

A fix is present in v5 or v5 of the library, but upgrading to v5 changes the API. An upgrade to v4.5.1 is enough to fix the vuln.

Checklist

Expected behaviour

A SCA scan does not surface any vulnerabilities.

Actual behaviour

Vulnerabilty is flagged.

Version/commit

v4.12.0

aldas commented 1 week ago

@vishr, @lammel maybe it is time to delete JWT middleware from core (this repo) and direct everyone to https://github.com/labstack/echo-jwt I really dislike the idea of major version bump for this.

I do not think upgrading JWT deps makes sense in this repo. It is a breaking change anyway. Upgrading to jwt to v4/v5 here is even worse as you might not notice the change until your casts to Token start panicing in handlers - (during runtime). At least removing the middleware would be impossible not do notice as your builds would fail after Echo version bump.

aldas commented 1 week ago

@inge4pres as a immediate remedy, please switch to using https://github.com/labstack/echo-jwt NB: make sure to create at lease one test in you app that uses jwt mw + you handler that checks for JWT Token.

This is because people often have user = c.Get("user").(*jwt.Token) or similar line in our handler to extract JWT Token from echo context and cast it to *jwt.Token. Now when Echo would silently upgrade to v4 or v5 in your handler go file, in imports you would still have github.com/golang-jwt/jwt but not github.com/golang-jwt/jwt/v5. That later one is actual version for jwt.Token type.

something like that

func TestMyHandlerWithJWTMW(t *testing.T) {
    e := echo.New()

    // replace this with https://github.com/labstack/echo-jwt
    e.Use(middleware.JWTWithConfig(middleware.JWTConfig{
        SigningKey: []byte("secret"),
    }))

    req := httptest.NewRequest(http.MethodGet, "/", nil)
    req.Header.Set(echo.HeaderAuthorization, "bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiYWRtaW4iOnRydWV9.TJVA95OrM7E2cBab30RMHrHDcEfxjoYZgeFONFh7HgQ")
    res := httptest.NewRecorder()

    // ADD HERE YOUR HANDLER TO ECHO to test
    // `user = c.Get("user").(*jwt.Token)` not panicing due different JWT version
    // That line would definetely panic if Echo would bump JWT version to v4/v5 but you did not upgrade JWT import in this handle .go file
    //e.GET("/", myHandler)

    e.ServeHTTP(res, req)

    assert.Equal(t, http.StatusOK, res.Code)
}
lammel commented 1 week ago

@vishr, @lammel maybe it is time to delete JWT middleware from core (this repo) and direct everyone to https://github.com/labstack/echo-jwt I really dislike the idea of major version bump for this.

I do not think upgrading JWT deps makes sense in this repo. It is a breaking change anyway. Upgrading to jwt to v4/v5 here is even worse as you might not notice the change until your casts to Token start panicing in handlers - (during runtime). At least removing the middleware would be impossible not do notice as your builds would fail after Echo version bump.

Guess this is the way to go. The echo core should require as view dependencies as possible to avoid backwards compatibility issues. Bumping versions for echo-jwt will be far easier.

stevenwhitehead commented 1 week ago

@lammel @aldas made https://github.com/labstack/echo/pull/2701 to remove the middleware instead

inge4pres commented 2 days ago

Thanks for your inputs folks. I'll go ahead and close #2700 .