ueberauth / guardian

Elixir Authentication
MIT License
3.44k stars 381 forks source link

JWT - Adds a custom dynamic secret fetcher that can read a tokens header #473

Closed hassox closed 6 years ago

hassox commented 6 years ago

Addresses: https://github.com/ueberauth/guardian/issues/469

hassox commented 6 years ago

@yordis if we leave it blank it would be a broken implementation. I think we either give it an implementation or leave it as a callback. I think for convenience we should have an implementation but what we could do instead is just delegate out to the default fetcher for each one rather than duplicate the code.

hassox commented 6 years ago

@yordis updated

hassox commented 6 years ago

@yordis ping

apognu commented 6 years ago

Does fetch_signing_secret only returns the secret? Signing tokens with different secrets, but not being able to identify which one from the token defeats the purpose, somewhat.

fetch_signing_secret should return the secret and a key id that would appear in the generated token header as a kid claim.

Right now, there is no way, from the token headers passed to fetch_verifying_secret, to fetch the key used to sign a token.

scrogson commented 6 years ago

@apognu in general, I agree with you. If you are doing public/private key signing/verifying and rotating keys then what you are saying makes perfect sense.

The default JWT token module that ships with Guardian is more naive and doesn't really go to great lengths to support multiple secret keys.

The great news is that you can easily create your own token strategy and use that instead of the default one provided by Guardian.

Let me know if we're missing something.

apognu commented 6 years ago

Thanks for replying on a closed issue.

I agree that the default behavior should be a naive one, and while Guardian does allow me to specify custom secret_fetcher and token_verify_module (if there is another plugging point, maybe I missed it), I believe they lack specific features that would be useful:

My specific use-case aside, I believe there is a symmetry issue between those two functions, where one can use the values in the headers and the second cannot set those values.

On the same note (but not directly related to this issue), in custom token_verify_modules, the token header is not passed to the verify_claim methods, so we still cannot map any verification rules to the key that is used (for example, "a token signed by a specific key can only include the sub to which the key belongs to").

hassox commented 6 years ago

@apognu I don't believe this has been released yet. I'd be interested to see what modifications you would put in place to make this feature more useful.

scrogson commented 6 years ago

@apognu I see. Excellent points! I think it would be great to support these things. Perhaps we can extend what's in master before the next release.

apognu commented 6 years ago

@hassox @scrogson I could give it a go, but by the time I familiarize myself with the structure of the project, it could be some time. When is the next release scheduled?

Without having looked too closely at the code, this would require:

Those are my thoughts right now, but I have not spent a lot of time working on it. And I'll be frank, I have no idea how much work that would represent on Guardian's code base.