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.

Sign up for GitHub

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

Jump to bottom

The Implicit Flow: Part 1 #107

Closed
wants to merge 10 commits into from
Closed

The Implicit Flow: Part 1 #107

wants to merge 10 commits into from

Conversation

hhorikawa
Copy link

@hhorikawa hhorikawa commented Feb 5, 2022 •
edited
Loading

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:

  • response_type: ['id_token','token']

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

  • new methods to parse IdP's public keys for discovery:false.
  • public_key() is restricted to only public keys.
  • call public_key() from key_or_secret() at necessary
  • flexibility of configured_response_type()

others:

  • coveralls dependency removed due to version conflict.

@hhorikawa hhorikawa changed the title wip: the Implicit Flow. The Implicit Flow: Part 1 Feb 6, 2022
@hhorikawa hhorikawa marked this pull request as ready for review February 6, 2022 10:22
stanhu
stanhu reviewed Feb 7, 2022
View reviewed changes
lib/omniauth/openid_connect.rb Outdated
@@ -2,4 +2,5 @@

require 'omniauth/openid_connect/errors'
require 'omniauth/openid_connect/version'
require 'omniauth/strategies/openid_connect'
#require 'omniauth/strategies/openid_connect'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this commented out?

Copy link
Author

Choose a reason for hiding this comment

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

Because omniauth/strategies/openid_connect is not a subdirectory. This is required from lib/omniauth_openid_connect.

stanhu
stanhu reviewed Feb 7, 2022
View reviewed changes
test/test_helper.rb Outdated Show resolved Hide resolved
stanhu
stanhu reviewed Feb 7, 2022
View reviewed changes
README.md Outdated Show resolved Hide resolved
hhorikawa and others added 4 commits February 7, 2022 21:14
@hhorikawa @stanhu
Apply suggestions from code review
2fca0c6 
Co-authored-by: Stan Hu <[email protected]>
@hhorikawa
fix by advices.
3165e6b
@hhorikawa
Merge branch 'master' into implicit-flow-work
63f7192 
# Conflicts:
#	test/lib/omniauth/strategies/openid_connect_test.rb
#	test/test_helper.rb
@hhorikawa
too strong rubocop.
87e77df
@hhorikawa hhorikawa requested a review from stanhu February 7, 2022 13:39
stanhu
stanhu reviewed Feb 8, 2022
View reviewed changes
lib/omniauth/strategies/openid_connect.rb
raise ArgumentError, 'Not supported response_type'
end

if configured_response_type == 'id_token token' &&
Copy link
Contributor

Choose a reason for hiding this comment

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

response_type can also be:

  1. token
  2. token id_token token

Copy link
Author

Choose a reason for hiding this comment

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

No. token (raw OAuth 2.0) MUST NOT be used for the authentication purpose. There are many explanations, for example, see: http://www.thread-safe.com/2012/01/problem-with-oauth-for-authentication.html

Acceptable:

  • code - Authorization Code Flow
  • id_token token - Implicit Flow
  • code id_token - Hybrid Flow -- Financial-grade API Security Profile 1.0 - Part 2: Advanced

However, I think i will implement the Implicit Flow first. After finishing, I will consider whether to implement the Hybrid Flow.

Copy link
Contributor

Choose a reason for hiding this comment

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

This method name setup_phase is confusing; maybe validate_response_type! is better. My point is there are many types of response_type values, and it's not clear to me that this is checking specific types for different flows.

We may want to link: https://openid.net/specs/oauth-v2-multiple-response-types-1_0-03.html

Copy link
Author

Choose a reason for hiding this comment

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

setup_phase is a override method. It's OK.
As you say, there are various combinations of response_type. However, they cannot be implemented without checking the use cases and risks one by one.
If you want to implement the Hybrid Flow, I won't stop it and welcome you to do it.

Copy link
Contributor

@stanhu stanhu Feb 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I see your security note about id_token, but this gem supported that configuration in the past (e.g. https://docs.microsoft.com/en-us/azure/active-directory/develop/v2-protocols-oidc#send-the-sign-in-request). I'm a bit hesitant to break existing configurations.

Copy link
Author

Choose a reason for hiding this comment

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

There are too many things to point out and I am at a loss.

  • The Implicit Flow of this branch does not force the public key verification required by the specification and does not secure. So I'm trying to make it.
  • The compatibility with what does not work. What does that mean?
  • As stated in the link you shared, Azure AD disables the Implicit Flow by default.
  • I'm using Azure AD, so I checked. You can enable the implicit permissions on the Azure AD console. However, in the description there, id_token token is specified. In the case of the Hybrid Flow, enabling only id_token is instructed.
  • The Hybrid Flow is not the scope of this patch sequence.

Furthermore, if you still want to get id_token only, OK, I consider implementing it. Let me do to accept only id_token in the next patch part 2 and implement the public key validation in patch part 3.

Copy link
Author

Choose a reason for hiding this comment

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

@stanhu
Are there any additions? For reference, I would link to the implementation guide.
https://openid.net/specs/openid-connect-implicit-1_0.html

stanhu
stanhu reviewed Feb 8, 2022
View reviewed changes
lib/omniauth/strategies/openid_connect.rb

options.issuer = issuer if options.issuer.nil? || options.issuer.empty?

verify_id_token!(params['id_token']) if configured_response_type == 'id_token'
verify_id_token!(params['id_token']) if configured_response_type == 'id_token token'
Copy link
Contributor

@stanhu stanhu Feb 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think there should be some method for implicit_flow?, client_credentials_flow?, etc. There are many possibilities for configured_response_type.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, I see.
I want to organize it with the patch part 2 to push next.

m0n9oose
m0n9oose requested changes Feb 8, 2022
View reviewed changes
lib/omniauth/strategies/openid_connect.rb Outdated Show resolved Hide resolved
lib/omniauth/openid_connect/util.rb Show resolved Hide resolved
lib/omniauth/strategies/openid_connect.rb Outdated Show resolved Hide resolved
@hhorikawa
Merge branch 'master' into implicit-flow-work
f38ecb2 
# Conflicts:
#	test/lib/omniauth/strategies/openid_connect_test.rb
#	test/test_helper.rb
@hhorikawa hhorikawa linked an issue Feb 9, 2022 that may be closed by this pull request
Implicit Flow does not work #105
Open
@hhorikawa hhorikawa removed a link to an issue Feb 9, 2022
Implicit Flow does not work #105
Open
@formigarafa
Copy link
Collaborator

formigarafa commented Feb 15, 2022 •
edited
Loading

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
Copy link
Author

@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 hhorikawa linked an issue Feb 23, 2022 that may be closed by this pull request
Implicit Flow does not work #105
Open
@hhorikawa
Copy link
Author

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.

@hhorikawa hhorikawa closed this Feb 23, 2022
@formigarafa
Copy link
Collaborator

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.

seanpdoyle added a commit to seanpdoyle/omniauth_openid_connect that referenced this pull request Jan 4, 2023
@seanpdoyle
Support response_type: ["code", "id_token"]
10ca535 
Closes [omniauth#105][]
Similar to [omniauth#107][]

Some OpenID compatible IdP support hybrid authorizations that accept a
`response_type` with both `code` and `id_token`.

For example, [Microsoft Azure B2C][] accepts them as a URL-encoded
array:

> `response_type`: Must include an ID token for OpenID Connect. If your web application also needs tokens for calling a web API, you can use `code+id_token`.

This commit extends the `OmniAuth::Strategies::OpenIDConnect` to encode
the `response_type` into the query parameter as space-delimited token
list when provided as an array. Similarly, when checking for missing
keys in the response, iterate over the values as if they're an array.

For the originally supported single-value case, the previous behavior is
maintained.

[Microsoft Azure B2C]: https://learn.microsoft.com/en-us/azure/active-directory-b2c/openid-connect#send-authentication-requests
[omniauth#105]: omniauth#105
[omniauth#107]: omniauth#107
seanpdoyle added a commit to seanpdoyle/omniauth_openid_connect that referenced this pull request Jan 4, 2023
@seanpdoyle
Support response_type: ["code", "id_token"]
7965667 
Closes [omniauth#105][]
Similar to [omniauth#107][]

Some OpenID compatible IdP support hybrid authorizations that accept a
`response_type` with both `code` and `id_token`.

For example, [Microsoft Azure B2C][] accepts them as a URL-encoded
array:

> `response_type`: Must include an ID token for OpenID Connect. If your web application also needs tokens for calling a web API, you can use `code+id_token`.

This commit extends the `OmniAuth::Strategies::OpenIDConnect` to encode
the `response_type` into the query parameter as space-delimited token
list when provided as an array. Similarly, when checking for missing
keys in the response, iterate over the values as if they're an array.

For the originally supported single-value case, the previous behavior is
maintained.

[Microsoft Azure B2C]: https://learn.microsoft.com/en-us/azure/active-directory-b2c/openid-connect#send-authentication-requests
[omniauth#105]: omniauth#105
[omniauth#107]: omniauth#107
seanpdoyle added a commit to seanpdoyle/omniauth_openid_connect that referenced this pull request Jan 4, 2023
@seanpdoyle
Support response_type: ["code", "id_token"]
09af6dc 
Closes [omniauth#105][]
Similar to [omniauth#107][]

Some OpenID compatible IdP support hybrid authorizations that accept a
`response_type` with both `code` and `id_token`.

For example, [Microsoft Azure B2C][] accepts them as a URL-encoded
array:

> `response_type`: Must include an ID token for OpenID Connect. If your web application also needs tokens for calling a web API, you can use `code+id_token`.

This commit extends the `OmniAuth::Strategies::OpenIDConnect` to encode
the `response_type` into the query parameter as space-delimited token
list when provided as an array. Similarly, when checking for missing
keys in the response, iterate over the values as if they're an array.

For the originally supported single-value case, the previous behavior is
maintained.

[Microsoft Azure B2C]: https://learn.microsoft.com/en-us/azure/active-directory-b2c/openid-connect#send-authentication-requests
[omniauth#105]: omniauth#105
[omniauth#107]: omniauth#107
@seanpdoyle seanpdoyle mentioned this pull request Jan 4, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidwessman davidwessman davidwessman left review comments

@m0n9oose m0n9oose m0n9oose approved these changes

@Eric-Guo Eric-Guo Awaiting requested review from Eric-Guo

@formigarafa formigarafa Awaiting requested review from formigarafa

@stanhu stanhu Awaiting requested review from stanhu

@bufferoverflow bufferoverflow Awaiting requested review from bufferoverflow

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implicit Flow does not work
5 participants
@hhorikawa @formigarafa @stanhu @m0n9oose @davidwessman