JWT limiter #221
JWT limiter #221
Conversation
| Store Store | ||
| Rate Rate | ||
| Options Options | ||
| ErrValidation error |
There was a problem hiding this comment.
Please remove this ErrValidation, if you should return an error, return it directly.
There was a problem hiding this comment.
It was the simplest way to stop the handler by calling OnError function
There was a problem hiding this comment.
This doesn't work because either you have one request at a time, and even then it doesn't work because you never reset the ErrValidation for the next request.
Or, you have multiple request at the same time, and then you have a huge data race / race condition.
This is clear now that the API for KeyGetter is not enough but, if we change it, it would break backward compatibility. My bad for this!
You'll have to wait for the v4 engine to add support for JWT.
|
Excuse me for delay, it was Nowruz 😄 |
|
I haven't forgotten. I'll probably review this the following weekend. |
| @@ -33,9 +33,32 @@ func NewMiddleware(limiter *limiter.Limiter, options ...Option) *Middleware { | |||
| return middleware | |||
| } | |||
|
|
|||
| // NewJWTMiddleware return a new instance of a JWT token middleware. | |||
| func NewJWTMiddleware(limiter *limiter.Limiter, options ...Option) *Middleware { | |||
There was a problem hiding this comment.
Please remove this function because it doesn't provide any value.
You could define KeyGetter with your JWTKeyGetter by declaring an option.
You can find inspiration on how to do this there:
limiter/drivers/middleware/stdlib/options.go
Lines 53 to 58 in c3db9d6
There was a problem hiding this comment.
Then, you could declare your middleware with something like this:
stdlib.NewMiddleware(limiter, stdlib.WithJWTKeyGetter(JWTKeyGetter(limiter)))Or, simply do a function that does something like this if you want syntactic sugar for this.
| @@ -79,6 +92,15 @@ func GetIP(r *http.Request, options ...Options) net.IP { | |||
| return net.ParseIP(host) | |||
| } | |||
|
|
|||
| // GetJWTSub returns sub from request JWT. | |||
| func GetJWTSub(r *http.Request, secret string) (string, error) { | |||
| if token, valid := getAuthorizationToken(r); valid { | |||
There was a problem hiding this comment.
Please reverse your condition handling to have the edge/corner case on the right.
| if token, valid := getAuthorizationToken(r); valid { | |
| token, valid := getAuthorizationToken(r) | |
| if !valid { | |
| return "", ErrInvalidJWT |
| @@ -26,6 +26,8 @@ github.com/go-playground/validator/v10 v10.11.1 h1:prmOlTVv+YjZjmRmNSF3VmspqJIxJ | |||
| github.com/go-playground/validator/v10 v10.11.1/go.mod h1:i+3WkQ1FvaUjjxh1kSvIA4dMGDBiPU55YFDl0WbKdWU= | |||
| github.com/goccy/go-json v0.9.11 h1:/pAaQDLHEoCq/5FFmSKBswWmK6H0e8g4159Kc/X/nqk= | |||
| github.com/goccy/go-json v0.9.11/go.mod h1:6MelG93GURQebXPDq3khkgXZkazVtN9CRI+MGFi0w8I= | |||
| github.com/golang-jwt/jwt v3.2.2+incompatible h1:IfV12K8xAKAnZqdXVzCZ+TOjboZ2keLg81eXfW3O+oY= | |||
| } | ||
|
|
||
| // Verify the token format (Bearer <token>) | ||
| lowerToken := strings.ToLower(headerToken[:len(bearer)]) |
There was a problem hiding this comment.
Your code could panic there if len(headerToken) < len(bearer)
| return "", err | ||
| } | ||
| if !token.Valid { | ||
| return "", ErrInvalidJWT |
There was a problem hiding this comment.
Could you wrap your errors there in order to create a stacktrace ?
return "", errors.Wrap(ErrInvalidJWT, "failed to extract sub from jwt")
| @@ -24,6 +24,7 @@ type Options struct { | |||
| // proxy is not configured properly to forward a trustworthy client IP. | |||
| // Please read the section "Limiter behind a reverse proxy" in the README for further information. | |||
| ClientIPHeader string | |||
| JWTSecret string | |||
There was a problem hiding this comment.
Please add a comment to explain what this option does, how/where it's used.
| // Handler handles a HTTP request. | ||
| func (middleware *Middleware) Handler(h http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| err := middleware.Limiter.ErrValidation |
There was a problem hiding this comment.
Please read my comment about data race / race condition.
It's a blocker.
Co-authored-by: Thomas LE ROUX <thomas@leroux.io>
Co-authored-by: Thomas LE ROUX <thomas@leroux.io>
Co-authored-by: Thomas LE ROUX <thomas@leroux.io>
Implement JWT token instead of using IP