omniauth / omniauth_openid_connect

MIT License
171 stars 187 forks source link

The Implicit Flow: Part 1 #107

Closed hhorikawa closed 2 years ago

hhorikawa commented 2 years ago

The Implicit Flow: Part 1.

It's quite a big deal, so I split it into several patches. I have checked that I don't break the Code Flow.

AS-IS: The Implicit Flow is used in a situation when the client_secret cannot be kept safe. So, the client must validate the token returned by the IdP with the IdP's public key. However, neither the strategy code nor the test cases have been validated and tested. It is vulnerable.

In this Part 1, I prepare the flow to incorporate verification.

options:

You must use always the IdP's RSA public key.

others:

formigarafa commented 2 years ago

Some people (me) may get a bit confused with the use-case you are trying to cover. If you don't mind I would appreciate if you could give a better explanation on this use. Some people won't expect to see an implicit flow using a server, understand? I am not saying you can't have such use-case. I had to deal with some very odd ones myself. I am just trying to follow what you are trying to achieve.

BTW, my expectation is that I may be just about to learn something.

hhorikawa commented 2 years ago

@formigarafa Thanks for your comment.

  1. The current implementation is not perfect, so I would like to brush up and fix it. It's a purely technical interest.
  2. The use case is free from client_secret in the server app. By using this secure library, you don't have to store the client_secret in your application. There are no trade-offs. It's simply good to make one less secret to keep safe
  3. Desktop or front-end application. However, I don't have knowledge about this point. Ruby Hyperloop (now called HYPERSTACK)?

Do you know what a use case was expected when the first Implicit Flow was introduced?

hhorikawa commented 2 years ago

The review takes a lot of time. Guess the patch is too big. OK. I'll drop this PR and reconstruct it into a bunch of smaller patches.

formigarafa commented 2 years ago

My concern is just if it would become possible to create an implicit flow on server-side with your changes which I believe is supposed to be avoided. Running the tool in the client will not complete any of the server flows.

I have been reading and it seem there are other less conventional flows using the name implicit. I wonder if what you are implementing would be one of those. It would be nice to have a link or description to give an overview of what the code is supposed to do. It will ease reviews by knowing what is new and if something is breaking in the process.