SimonSchneider / traefik-jwt-decode

Traefik forward auth implementation for JWT tokens
Apache License 2.0
29 stars 15 forks source link

"No token" results in 200 OK #30

Closed bennesp closed 2 years ago

bennesp commented 2 years ago

As stated in the README:

If no token is present on the request traefik-jwt-decode will return 200 and set the header jwt-token-validated: false.

Is there any reason behind?

I am asking because I expect traefik-jwt-decode to fail with 401 if no Authorization header is set. I have set up traefik with traefik-jwt-decode as forward auth middleware and I would like to block any incoming request without that header set. Is there a way to do it?

Maybe this behavior could be slightly modified through an env var, as happened for #28 ? Or maybe there is a way to do it with other traefik middlewares?

Thank you very much

eduardomineo commented 2 years ago

Hi @bennesp

I just forked and pushed this exact implementation. I added a AUTH_HEADER_REQUIRED envvar (default false) that would change the behavior of the decoder, rejecting requests with no authorization header by returning 401 code. In this case, no jwt-token-validated header will be added and a warning will be logged.

In order to make it work on Kubernetes, I created a /ping route to be used as probe, otherwise the pod would be restarted constantly since requesting / without authorization would result in non-ok status.

https://github.com/eduardomineo/traefik-jwt-decode/tree/401_missing_auth_header

@SimonSchneider feel free to give any suggestion or apply my changes to the original code.

Best wishes.

bennesp commented 2 years ago

That seems pretty neat, thank you!

I don't know the position of @SimonSchneider about this, but maybe you could also try to directly send a PR to this project if you like, I suppose, so that it would also be easier for him to review and possibly merge the changes.

Thank you again

eduardomineo commented 2 years ago

Thanks. Let's see his reply, if he approves, I can push a branch with commits referring this issue and open a PR.

ps: I just fixed the unit test.

SimonSchneider commented 2 years ago

Sorry for the silence on my part. This issue was raised during stressful times and was then forgotten.

The original thought of the default 200 no token was that I didn't want to take this decision but have the user service take this decision. That might in hindsight be a little too purist and I'd agree that putting this feature behind and env var is completely valid.

@eduardomineo I'd be happy for you to create a PR for this and I'll see to getting it merged. Thanks for the contribution and helping out.

eduardomineo commented 2 years ago

Thank you Simon for your reply! I'll work on it.

eduardomineo commented 2 years ago

@SimonSchneider Can you grant me access to push a branch so I can open a PR? Thanks!

SimonSchneider commented 2 years ago

@eduardomineo Would you mind make a fork and creating a PR from there? I think you can do that without any access granted for this repo.

eduardomineo commented 2 years ago

Cool, I didn't know. Just created the PR.

SimonSchneider commented 2 years ago

Thank you @eduardomineo 🙏 The new build is available as docker pull simonschneider/traefik-jwt-decode:latest

I'll release a new tagged version later as it seems the CI step didn't release a tagged verison, but this should allow you to try it out

eduardomineo commented 2 years ago

Thank you, @SimonSchneider , this is cool!