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.

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

The Implicit Flow: Part 1 #107

Closed
wants to merge 10 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,6 @@ tmp
.ruby-version
.ruby-gemset
Gemfile.lock

# Backup files
*~
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ config.omniauth :openid_connect, {
| discovery | Should OpenID discovery be used. This is recommended if the IDP provides a discovery endpoint. See client config for how to manually enter discovered values. | no | false | one of: true, false |
| client_auth_method | Which authentication method to use to authenticate your app with the authorization server | no | Sym: basic | "basic", "jwks" |
| scope | Which OpenID scopes to include (:openid is always required) | no | Array<sym> [:openid] | [:openid, :profile, :email] |
| response_type | Which OAuth2 response type to use with the authorization request | no | String: code | one of: 'code', 'id_token' |
| `response_type` | Which OAuth2 response type to use with the authorization request | no | String: code | one of: 'code', 'id_token token' <br />**Security note:** Do not use 'token' or 'id_token'. The 'token' (raw OAuth 2.0) MUST NOT be used for the authentication purpose. The 'id_token' is used only for the Self-Issued OpenID Providers. Instead, use 'id_token token' (the Implicit Flow). |
| state | A value to be used for the OAuth2 state parameter on the authorization request. Can be a proc that generates a string. | no | Random 16 character string | Proc.new { SecureRandom.hex(32) } |
| response_mode | The response mode per [spec](https://openid.net/specs/oauth-v2-form-post-response-mode-1_0.html) | no | nil | one of: :query, :fragment, :form_post, :web_message |
| display | An optional parameter to the authorization request to determine how the authorization and consent page | no | nil | one of: :page, :popup, :touch, :wap |
Expand Down
2 changes: 1 addition & 1 deletion lib/omniauth/openid_connect.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@

require 'omniauth/openid_connect/errors'
require 'omniauth/openid_connect/version'
require 'omniauth/strategies/openid_connect'
require 'omniauth/openid_connect/util'
46 changes: 46 additions & 0 deletions lib/omniauth/openid_connect/util.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
# frozen_string_literal: true

module OmniAuth
module OpenIDConnect
hhorikawa marked this conversation as resolved.
Show resolved Hide resolved
module Util
# @param [String or Hash] key_or_hash Certificate data in PEM format
# @raise [OpenSSL::X509::CertificateError] Certificate format is incorrect
def self.parse_x509_key(key_or_hash, kid)
raise TypeError if !key_or_hash.is_a?(String) && !key_or_hash.is_a?(Hash)

if key_or_hash.is_a?(Hash)
# https://www.googleapis.com/oauth2/v1/certs format
raise TypeError unless kid

key_or_hash.each do |key, pem|
return OpenSSL::X509::Certificate.new(pem).public_key if key == kid
end
raise ArgumentError, "missing kid: #{kid}"
else
OpenSSL::X509::Certificate.new(key_or_hash).public_key
end
end

# Decode JSON Web Key (JWK) or JWK Set format.
# See RFC 7517
# @param [String or Hash] key_or_hash JSON-formatted string, or a hash.
# Sample: https://www.googleapis.com/oauth2/v3/certs
def self.parse_jwk_key(key_or_hash, _kid)
case key_or_hash
when String
json = JSON.parse(key_or_hash)
when Hash
json = key_or_hash
else
raise TypeError, "key was #{key_or_hash.class}, #{key_or_hash.inspect}"
end

if json.key?('keys')
JSON::JWK::Set.new json['keys']
else
JSON::JWK.new json
end
end
end
end
end
102 changes: 72 additions & 30 deletions lib/omniauth/strategies/openid_connect.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,13 @@ class OpenIDConnect
option :issuer
option :discovery, false
option :client_signing_alg
# Required if you set 'discovery:false'.
# IdP's public keys. NOT client's.
option :client_jwk_signing_key
option :client_x509_signing_key

option :scope, [:openid]
option :response_type, 'code' # ['code', 'id_token']
option :response_type, 'code' # 'code', ['id_token', 'token']
option :state
option :response_mode # [:query, :fragment, :form_post, :web_message]
option :display, nil # [:page, :popup, :touch, :wap]
Expand Down Expand Up @@ -98,6 +101,23 @@ def config
@config ||= ::OpenIDConnect::Discovery::Provider::Config.discover!(options.issuer)
end

# @override
# Called before both of request_phase and callback_phase
def setup_phase
super

if configured_response_type != 'code' &&
configured_response_type != 'id_token token'
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

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

client_options.secret
raise ArgumentError, 'MUST NOT set client_secret on the Implicit Flow'
end
end

# @override
def request_phase
options.issuer = issuer if options.issuer.to_s.empty?
discover!
Expand All @@ -112,15 +132,15 @@ def callback_phase
raise CallbackError, error: params['error'], reason: error_description, uri: params['error_uri'] if error
raise CallbackError, error: :csrf_detected, reason: "Invalid 'state' parameter" if invalid_state

return unless valid_response_type?
return if configured_response_type == 'code' && !valid_response_type?

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

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.

discover!
client.redirect_uri = redirect_uri

return id_token_callback_phase if configured_response_type == 'id_token'
return implicit_flow_callback_phase if configured_response_type == 'id_token token'

client.authorization_code = authorization_code
access_token
Expand Down Expand Up @@ -181,10 +201,19 @@ def authorize_uri
client.authorization_uri(opts.reject { |_k, v| v.nil? })
end

def public_key
# @return [JSON::JWK::Set or JSON::JWK] IdP's RSA public keys. NOT client's.
def public_key(kid = nil)
# [Security issue] Do not call key_or_secret() here.

return config.jwks if options.discovery

key_or_secret || config.jwks
return OmniAuth::OpenIDConnect::Util.parse_jwk_key(options.client_jwk_signing_key, kid) if options.client_jwk_signing_key

if options.client_x509_signing_key
return OmniAuth::OpenIDConnect::Util.parse_x509_key(options.client_x509_signing_key, kid)
end

raise 'internal error: missing RSA public key'
end

private
Expand Down Expand Up @@ -273,34 +302,22 @@ def session
super
end

def key_or_secret
case options.client_signing_alg
# For HMAC-SHA256, return client_secret as the common key.
# For RSA, return the public key of the authentication server
def key_or_secret(header = nil)
raise TypeError if header && !header.respond_to?(:[])

case header ? header['alg'].to_sym : options.client_signing_alg
when :HS256, :HS384, :HS512
client_options.secret
when :RS256, :RS384, :RS512
if options.client_jwk_signing_key
parse_jwk_key(options.client_jwk_signing_key)
elsif options.client_x509_signing_key
parse_x509_key(options.client_x509_signing_key)
end
public_key(header['kid'])
else
# ES256 : ECDSA using P-256 curve and SHA-256 hash
raise ArgumentError, "unsupported alg: #{header['alg']}"
end
end

def parse_x509_key(key)
OpenSSL::X509::Certificate.new(key).public_key
end

def parse_jwk_key(key)
json = JSON.parse(key)
return JSON::JWK::Set.new(json['keys']) if json.key?('keys')

JSON::JWK.new(json)
end

def decode(str)
UrlSafeBase64.decode64(str).unpack1('B*').to_i(2).to_s
end

def redirect_uri
return client_options.redirect_uri unless params['redirect_uri']

Expand All @@ -324,7 +341,21 @@ def logout_path_pattern
@logout_path_pattern ||= %r{\A#{Regexp.quote(request_path)}(/logout)}
end

def id_token_callback_phase
# The Implicit Flow:
# Get an access token at the same time as id_token. There is a risk that
# one of them has been tampered with. (Token Hijacking)
# For that reason,
# (1) You MUST verify the signature of the id_token by the public key of
# IdP. Instead of choosing the key with the response header, you have
# to use always the public key.
# (2) The access token must be validated by the id_token.
def implicit_flow_callback_phase
if !params['access_token'] || !params['id_token']
fail! :missing_id_token,
OmniAuth::OpenIDConnect::MissingIdTokenError.new(params['error'])
return
end

user_data = decode_id_token(params['id_token']).raw_attributes
env['omniauth.auth'] = AuthHash.new(
provider: name,
Expand All @@ -335,6 +366,7 @@ def id_token_callback_phase
call_app!
end

# Called only from callback_phase()
def valid_response_type?
return true if params.key?(configured_response_type)

Expand All @@ -344,8 +376,18 @@ def valid_response_type?
false
end

# Normalize options.response_type.
# @return [String] 'code' or 'id_token token'
def configured_response_type
@configured_response_type ||= options.response_type.to_s
unless @configured_response_type
ary = case options.response_type
when Array then options.response_type
when Symbol then [options.response_type]
else options.response_type.split(/[ \t]+/)
end
@configured_response_type = ary.sort.join(' ')
end
@configured_response_type
end

def verify_id_token!(id_token)
Expand Down
1 change: 1 addition & 0 deletions lib/omniauth_openid_connect.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# frozen_string_literal: true

require 'omniauth/openid_connect'
require 'omniauth/strategies/openid_connect'
127 changes: 127 additions & 0 deletions test/lib/omniauth/strategies/implicit_flow_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
# frozen_string_literal: true

require_relative '../../../test_helper'

module OmniAuth
module Strategies
class NotYet
# Implicit Flow
def test_request_phase_with_response_mode
expected_redirect = %r{^https://example\.com/authorize\?client_id=1234&nonce=\w{32}&response_mode=form_post&response_type=id_token&scope=openid&state=\w{32}$}
strategy.options.issuer = 'example.com'
strategy.options.response_mode = 'form_post'
strategy.options.response_type = 'id_token'
strategy.options.client_options.host = 'example.com'

strategy.expects(:redirect).with(regexp_matches(expected_redirect))
strategy.request_phase
end

def test_request_phase_with_response_mode_symbol
expected_redirect = %r{^https://example\.com/authorize\?client_id=1234&nonce=\w{32}&response_mode=form_post&response_type=id_token&scope=openid&state=\w{32}$}
strategy.options.issuer = 'example.com'
strategy.options.response_mode = 'form_post'
strategy.options.response_type = :id_token
strategy.options.client_options.host = 'example.com'

strategy.expects(:redirect).with(regexp_matches(expected_redirect))
strategy.request_phase
end

def test_callback_phase_with_id_token
code = SecureRandom.hex(16)
state = SecureRandom.hex(16)
nonce = SecureRandom.hex(16)
request.stubs(:params).returns('id_token' => code, 'state' => state)
request.stubs(:path).returns('')

strategy.options.issuer = 'example.com'
strategy.options.client_signing_alg = :RS256
strategy.options.client_jwk_signing_key = File.read('test/fixtures/jwks.json')
strategy.options.response_type = 'id_token'

strategy.unstub(:user_info)
access_token = stub('OpenIDConnect::AccessToken')
access_token.stubs(:access_token)
access_token.stubs(:refresh_token)
access_token.stubs(:expires_in)
access_token.stubs(:scope)
access_token.stubs(:id_token).returns(File.read('test/fixtures/id_token.txt'))

id_token = stub('OpenIDConnect::ResponseObject::IdToken')
id_token.stubs(:raw_attributes).returns('sub' => 'sub', 'name' => 'name', 'email' => 'email')
id_token.stubs(:verify!).with(issuer: strategy.options.issuer, client_id: @identifier, nonce: nonce).returns(true)
::OpenIDConnect::ResponseObject::IdToken.stubs(:decode).returns(id_token)
id_token.expects(:verify!)

strategy.call!('rack.session' => { 'omniauth.state' => state, 'omniauth.nonce' => nonce })
strategy.callback_phase
end

# Implicit Flow
def test_callback_phase_without_id_token
state = SecureRandom.hex(16)
nonce = SecureRandom.hex(16)
request.stubs(:params).returns('state' => state)
request.stubs(:path).returns('')
strategy.options.response_type = 'id_token'

strategy.call!('rack.session' => { 'omniauth.state' => state, 'omniauth.nonce' => nonce })

strategy.expects(:fail!).with(:missing_id_token, is_a(OmniAuth::OpenIDConnect::MissingIdTokenError))
strategy.callback_phase
end

# Implicit Flow
def test_callback_phase_without_id_token_symbol
state = SecureRandom.hex(16)
nonce = SecureRandom.hex(16)
request.stubs(:params).returns('state' => state)
request.stubs(:path).returns('')
strategy.options.response_type = :id_token

strategy.call!('rack.session' => { 'omniauth.state' => state, 'omniauth.nonce' => nonce })

strategy.expects(:fail!).with(:missing_id_token, is_a(OmniAuth::OpenIDConnect::MissingIdTokenError))
strategy.callback_phase
end

# Implicit Flow
def test_id_token_auth_hash
state = SecureRandom.hex(16)
nonce = SecureRandom.hex(16)
strategy.options.response_type = 'token id_token'
strategy.options.issuer = 'example.com'

id_token = stub('OpenIDConnect::ResponseObject::IdToken')
id_token.stubs(:verify!).returns(true)
id_token.stubs(:raw_attributes, :to_h).returns(
{
"iss": 'http://server.example.com',
"sub": '248289761001',
"aud": 's6BhdRkqt3',
"nonce": 'n-0S6_WzA2Mj',
"exp": 1_311_281_970,
"iat": 1_311_280_970,
}
)

request.stubs(:params).returns('state' => state, 'nounce' => nonce, 'id_token' => id_token)
request.stubs(:path).returns('')

strategy.stubs(:decode_id_token).returns(id_token)
strategy.stubs(:stored_state).returns(state)

strategy.call!('rack.session' => { 'omniauth.state' => state, 'omniauth.nonce' => nonce })
strategy.callback_phase

auth_hash = strategy.send(:env)['omniauth.auth']
assert auth_hash.key?('provider')
assert auth_hash.key?('uid')
assert auth_hash.key?('info')
assert auth_hash.key?('extra')
assert auth_hash['extra'].key?('raw_info')
end
end
end
end
Loading