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

Jwt v5 update #9

Merged
merged 6 commits into from
May 16, 2023
Merged

Jwt v5 update #9

merged 6 commits into from
May 16, 2023

Conversation

whkelvin
Copy link
Contributor

No description provided.

@sp71
Copy link

sp71 commented May 13, 2023

@aldas Can you review this change?

Copy link
Contributor

@aldas aldas left a comment

Choose a reason for hiding this comment

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

LGTM

@aldas
Copy link
Contributor

aldas commented May 15, 2023

@whkelvin please change this line

go: [1.17, 1.18, 1.19]

to go: ["1.18", "1.19", "1.20"]

lets just drop 1.17 and add 1.20
NB: quotation marks are important

@aldas
Copy link
Contributor

aldas commented May 15, 2023

allright, this line

LATEST_GO_VERSION: 1.19
needs to be "1.20" as it should test against latest version. older versions have almost always vulns

@aldas
Copy link
Contributor

aldas commented May 15, 2023

it has to be quoted "1.20" or it will be interpreted as 1.2.x

@aldas
Copy link
Contributor

aldas commented May 15, 2023

ok, I'll merge this and fix these CI things. Thanks @whkelvin

@aldas aldas merged commit 6944ffe into labstack:main May 16, 2023
@gnuletik
Copy link

@aldas I think that the v4.2.0 release should be retracted.

github.com/golang-jwt/jwt/v5 is a breaking change update. It breaks existing authentication implementation.

With the current tag, a lot of users may break authentication of their services with a simple go get -u.

A new release as github.com/labstack/echo-jwt/v5 should be done.

@aldas
Copy link
Contributor

aldas commented May 19, 2023

I understand what you say but this library (unlike Echo core) does not follow semantic versioning promises. Second paragraph on repo front page/README states that. It does not try to hide that fact.

Our intention is not to break things with "trivial" things like middleware fields/function/methods etc but to allow us to be reckless and irresponsible when following golang-jwt/jwt versions.

We have experience from core with JWT lib (using older dgrijalva/jwt-go) that promising not break ever things is not feasible. For example dgrijalva/jwt-go had security vulnerability and lib was basically unmaintained. So after 6+ months on sitting growing number of issues we did the breaking change and changed to golang-jwt/jwt (note the import paths in core, it is using v3.2.2+incompatible) and knew that this will not be the last breaking change. golang-jwt/jwt have gone already from v4 to v5. So eventually this repo was created to be the place where "bad" things happen. And you definitely think why this library did not start with 0.x.x version - this is to have vanity fixed major number for Echo versions.

p.s. being a bastard - if someone find this thread because of production outage - you can blame me but do not forget that that I am not the one who should have written integration test which will catch this thing easily (my own takeaway from post-mortem/experience with last JWT lib breaking change).

@aldas
Copy link
Contributor

aldas commented May 19, 2023

p.s.s. I'll add more clearer note about the promises to the readme and example for testing handler with middleware

@gnuletik
Copy link

Thanks for the feedback @aldas!
I did not see that this library doesn't follow semantic versioning, my bad!
Totally agree that authentication should be covered with full integration tests!

So eventually this repo was created to be the place where "bad" things happen.

Just as a small improvement, I think that this statement should be placed somewhere as the comparaison with the middleware in the core.

Thanks for your work!

@SladeThe
Copy link

Voted down.
Each and every go.mod dependency MUST respect semantic versioning v2.
Otherwise, go tools would break existing projects upon automatic dependency update, as it happens with echo-jwt.
If every lib ignores the rules, it leads to another dependency hell Go devs wanted to avoid.

@aldas
Copy link
Contributor

aldas commented Jul 25, 2023

@SladeThe there is nothing that requires you use this middleware. 50+% of it is different configuration options.

If you absolutely disagree and can not use this lib then try this

This is stripped down version of same middleware:

func jwtMW(signingKey []byte) echo.MiddlewareFunc {
	return func(next echo.HandlerFunc) echo.HandlerFunc {
		return func(c echo.Context) error {
			_, auth, ok := strings.Cut(c.Request().Header.Get("Authorization"), "bearer ")
			if !ok {
				return echo.NewHTTPError(http.StatusUnauthorized, "authorization header missing or invalid")
			}

			token, err := jwt.ParseWithClaims(auth, jwt.MapClaims{}, func(token *jwt.Token) (interface{}, error) {
				if token.Method.Alg() != "HS256" {
					return nil, echo.NewHTTPError(http.StatusUnauthorized, "unexpected jwt signing method")
				}
				return signingKey, nil
			})
			if err != nil {
				return err
			}
			if !token.Valid {
				return echo.NewHTTPError(http.StatusUnauthorized, "invalid token")
			}
			c.Set("token", token)
			return next(c)
		}
	}
}

Full example:

package main

import (
	"errors"
	"fmt"
	"github.com/golang-jwt/jwt/v5"
	"github.com/labstack/echo/v4"
	"log"
	"net/http"
	"strings"
)

func jwtMW(signingKey []byte) echo.MiddlewareFunc {
	return func(next echo.HandlerFunc) echo.HandlerFunc {
		return func(c echo.Context) error {
			_, auth, ok := strings.Cut(c.Request().Header.Get("Authorization"), "bearer ")
			if !ok {
				return echo.NewHTTPError(http.StatusUnauthorized, "authorization header missing or invalid")
			}

			token, err := jwt.ParseWithClaims(auth, jwt.MapClaims{}, func(token *jwt.Token) (interface{}, error) {
				if token.Method.Alg() != "HS256" {
					return nil, echo.NewHTTPError(http.StatusUnauthorized, "unexpected jwt signing method")
				}
				return signingKey, nil
			})
			if err != nil {
				return err
			}
			if !token.Valid {
				return echo.NewHTTPError(http.StatusUnauthorized, "invalid token")
			}
			c.Set("token", token)
			return next(c)
		}
	}
}

func main() {
	e := echo.New()

	e.Use(jwtMW([]byte("secret")))

	e.GET("/test", func(c echo.Context) error {
		token := c.Get("token")

		return c.JSON(http.StatusOK, fmt.Sprintf("token: %v", token))
	})

	if err := e.Start(":8080"); err != nil && !errors.Is(err, http.ErrServerClosed) {
		log.Fatal(err)
	}
}

Output:

x@x:~/code/echo$ curl -H "Authorization: bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiYWRtaW4iOnRydWV9.TJVA95OrM7E2cBab30RMHrHDcEfxjoYZgeFONFh7HgQ" http://localhost:8080/test
"token: \u0026{eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiYWRtaW4iOnRydWV9.TJVA95OrM7E2cBab30RMHrHDcEfxjoYZgeFONFh7HgQ 0xc0000120a8 map[alg:HS256 typ:JWT] map[admin:true name:John Doe sub:1234567890] [76 149 64 247 147 171 51 177 54 112 22 155 223 68 76 30 177 195 112 71 241 142 134 25 129 225 78 52 88 123 30 4] true}"

@aldas aldas mentioned this pull request Jul 25, 2023
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.

5 participants