From c2e579be4029229a2daabfa7afa82fe3184da1d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Pi=C3=A1lek?= Date: Wed, 6 Apr 2022 10:12:19 +0200 Subject: [PATCH] Make nonce stick to the session It should be tied to the session. Will get rid of most nonce validation issues stemming from the users using the back-button or multiple tabs. Version bump --- .rubocop.yml | 3 + lib/omniauth/openid_connect/version.rb | 2 +- lib/omniauth/strategies/openid_connect.rb | 132 ++++++++++------------ 3 files changed, 61 insertions(+), 76 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index 4121b8a6..28d1d663 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -50,6 +50,9 @@ Metrics/BlockLength: Metrics/MethodLength: Max: 45 +Metrics/ClassLength: + Max: 500 + AllCops: Exclude: - bin/**/* diff --git a/lib/omniauth/openid_connect/version.rb b/lib/omniauth/openid_connect/version.rb index 99abc099..d38712eb 100644 --- a/lib/omniauth/openid_connect/version.rb +++ b/lib/omniauth/openid_connect/version.rb @@ -2,6 +2,6 @@ module OmniAuth module OpenIDConnect - VERSION = '0.9.8' + VERSION = '0.9.9' end end diff --git a/lib/omniauth/strategies/openid_connect.rb b/lib/omniauth/strategies/openid_connect.rb index 64a6936b..1d9648ce 100644 --- a/lib/omniauth/strategies/openid_connect.rb +++ b/lib/omniauth/strategies/openid_connect.rb @@ -1,12 +1,11 @@ -# -*- coding:utf-8 -*- # frozen_string_literal: true -#require 'addressable/uri' +# require 'addressable/uri' require 'timeout' require 'net/http' require 'omniauth' require 'openid_connect' -#require 'jwt' +# require 'jwt' require 'forwardable' module OmniAuth @@ -114,9 +113,9 @@ class OpenIDConnect option :max_age # not option, but by request.params - #option :ui_locales - #option :id_token_hint - #option :login_hint + # option :ui_locales + # option :id_token_hint + # option :login_hint # Authentication Request: [OPTIONAL] option :acr_values @@ -127,7 +126,7 @@ class OpenIDConnect option :send_nonce, true # Must verify the id_token. So remove this option. - #option :verify_id_token, nil + # option :verify_id_token, nil option :ux @@ -199,8 +198,8 @@ def uid image: user_info.picture, phone: user_info.phone_number, urls: { - website: user_info.website - } + website: user_info.website, + }, } end @@ -209,7 +208,7 @@ def uid {} else { raw_info: fix_user_info(user_info).raw_attributes } - end + end end credentials do @@ -221,7 +220,7 @@ def uid token: access_token.access_token, refresh_token: access_token.refresh_token, expires_in: access_token.expires_in, - scope: access_token.scope + scope: access_token.scope, } end end @@ -239,9 +238,7 @@ def initialize(app, *args, &block) # client_options から OpenIDConnect::Client インスタンスを構築. # @return [OpenIDConnect::Client] サーバとのconnection def client - if !client_options.host - raise TypeError, 'internal error: call setup_phase() first' - end + raise TypeError, 'internal error: call setup_phase() first' unless client_options.host @client ||= ::OpenIDConnect::Client.new(client_options) end @@ -261,14 +258,14 @@ def config # request_phase() と callback_phase() の開始前に呼び出される. def setup_phase super - raise TypeError if !options.issuer.is_a?(String) + raise TypeError unless options.issuer.is_a?(String) unless (uri = URI.parse(options.issuer)) && - %w[http https].include?(uri.scheme) + %w[http https].include?(uri.scheme) raise ArgumentError, 'Invalid issuer URI scheme' end @issuer = options.issuer - if !client_options.host + unless client_options.host client_options.scheme = uri.scheme client_options.host = uri.host client_options.port = uri.port @@ -282,7 +279,7 @@ def setup_phase discover! if options.discovery if configured_response_type != 'code' && - configured_response_type != 'id_token token' + configured_response_type != 'id_token token' raise ArgumentError, 'Invalid response_type' end if configured_response_type == 'id_token token' @@ -313,16 +310,16 @@ def callback_phase error_description = params['error_description'] || params['error_reason'] raise CallbackError.new( - params['error'], - error_description, # optional - params['error_uri'] - ) # optional + params['error'], + error_description, # optional + params['error_uri'] + ) # optional end if session['omniauth.state'] && - ( - params['state'].to_s.empty? || - params['state'] != session.delete('omniauth.state') - ) + ( + params['state'].to_s.empty? || + params['state'] != session.delete('omniauth.state') + ) # RFC 6749 4.1.2: クライアントからの認可リクエストに stateパラメータ # が含まれていた場合は, そのまま返ってくる. [REQUIRED] raise CallbackError.new(:csrf_detected, "Invalid 'state' parameter") @@ -366,9 +363,9 @@ def other_phase call_app! end - #def authorization_code ... params.delete() しないといかん. Remove. + # def authorization_code ... params.delete() しないといかん. Remove. # params['code'] - #end + # end def end_session_uri return unless end_session_endpoint_is_valid? @@ -391,19 +388,12 @@ def authorize_params # Others state: new_state, response_mode: options.response_mode, - nonce: - ( - if options.send_nonce || - configured_response_type == 'id_token token' - new_nonce - end - ), - hd: options.hd + nonce: nonce(options.send_nonce || + configured_response_type == 'id_token token'), + hd: options.hd, } - unless options.extra_authorize_params.empty? - opts.merge!(options.extra_authorize_params) - end + opts.merge!(options.extra_authorize_params) unless options.extra_authorize_params.empty? # Optional params. %i[display max_age acr_values ux].each do |key| @@ -420,10 +410,10 @@ def authorize_params 'email', 'realm', 'cid', - 'chem' + 'chem', ].each { |key| opts[key.to_sym] = params[key] if params[key] } - return opts.reject { |_k, v| v.nil? } + opts.reject { |_k, v| v.nil? } end # @return [JSON::JWK::Set or JSON::JWK] IdP's RSA public keys. NOT client's. @@ -460,7 +450,7 @@ def public_key(kid = nil) attr_reader :issuer def discover! - raise 'internal bug' if !options.discovery + raise 'internal bug' unless options.discovery # config() 内で, issuer を引数にして, 実際に discover! している. client_options.authorization_endpoint = config.authorization_endpoint @@ -469,22 +459,20 @@ def discover! # OpenIDConnect::Discovery::Provider::Config::Response に expires_in は # ない. - #client_options.expires_in = config().expires_in + # client_options.expires_in = config().expires_in # client_options に jwks_uri, end_session_endpoint はない. options.jwks_uri = config.jwks_uri - if config.respond_to?(:end_session_endpoint) - options.end_session_endpoint = config.end_session_endpoint - end + options.end_session_endpoint = config.end_session_endpoint if config.respond_to?(:end_session_endpoint) if config.respond_to?(:token_endpoint_auth_methods_supported) if config.token_endpoint_auth_methods_supported.include?( - 'client_secret_basic' - ) + 'client_secret_basic' + ) options.client_auth_method = :basic elsif config.token_endpoint_auth_methods_supported.include?( - 'client_secret_post' - ) + 'client_secret_post' + ) options.client_auth_method = :secret_in_body end end @@ -506,25 +494,23 @@ def user_info return @user_info if @user_info # openidconnect package: openid_connect/access_token.rb - return {} if !access_token - if !access_token.is_a?(::OpenIDConnect::AccessToken) - raise TypeError, 'internal error' - end + return {} unless access_token + raise TypeError, 'internal error' unless access_token.is_a?(::OpenIDConnect::AccessToken) @user_info = access_token.userinfo! - return @user_info + @user_info end # Google sends the string "true" as the value for the field # 'email_verified' while a boolean is expected. def fix_user_info(user_info) - raise TypeError if !user_info + raise TypeError unless user_info if user_info.email_verified.is_a? String user_info.email_verified = (user_info.email_verified.casecmp('true') == 0) end - #user_info.gender = nil # in case someone picks something else than male or female, we don't need it anyway + # user_info.gender = nil # in case someone picks something else than male or female, we don't need it anyway user_info end @@ -548,12 +534,10 @@ def authorization_code_flow_callback_phase # 仕様では grant_type, code, redirect_uri パラメータ opts = { scope: (options.scope if options.send_scope_to_token_endpoint), - client_auth_method: options.client_auth_method + client_auth_method: options.client_auth_method, } @access_token = client.access_token! opts - if !@access_token.is_a?(Rack::OAuth2::AccessToken) - raise TypeError, 'internal error' - end + raise TypeError, 'internal error' unless @access_token.is_a?(Rack::OAuth2::AccessToken) # 鍵を選ぶ。"{ヘッダ部}.{ペイロード部}.{シグネチャ部}" と、ピリオドで # 区切られている。ヘッダ部にアルゴリズムが書かれている. @@ -563,7 +547,7 @@ def authorization_code_flow_callback_phase :skip_verification ).header - #header = ::JWT.decoded_segments(actoken.id_token, false)[0] + # header = ::JWT.decoded_segments(actoken.id_token, false)[0] key = key_or_secret header # このなかで署名の検証も行う. => JSON::JWS::VerificationFailed @@ -582,7 +566,7 @@ def authorization_code_flow_callback_phase # [Security issue] On Authorization Code Flow, use key_or_secret, not # public_key. On the other hand, you have to use public_key on Implicit # Flow. - #def decode_id_token(id_token) + # def decode_id_token(id_token) def client_options options.client_options @@ -600,8 +584,8 @@ def new_state session['omniauth.state'] = state || SecureRandom.hex(16) end - def new_nonce - session['omniauth.nonce'] = SecureRandom.hex(16) + def nonce(force = false) + session['omniauth.nonce'] ||= force ? SecureRandom.hex(16) : nil end # @override @@ -616,7 +600,7 @@ def session # HMAC-SHA256 の場合は, client_secret を共通鍵とする # RSAの場合は, 認証サーバの公開鍵を使う def key_or_secret(header) - raise TypeError if !header + raise TypeError unless header case header['alg'].to_sym when :HS256, :HS384, :HS512 @@ -632,7 +616,7 @@ def key_or_secret(header) end # [Security issue] Do not use params['redirect_uri'] - #def redirect_uri + # def redirect_uri # @return [String] クエリパラメータ. def encoded_post_logout_redirect_uri @@ -640,7 +624,7 @@ def encoded_post_logout_redirect_uri # post_logout_redirect_uri を指定する場合は, id_token_hint 必須. URI.encode_www_form( -# id_token_hint: raise('Not Implemented Error'), # TODO: impl. [REQUIRED] hintなのに必須とはどういうこと? + # id_token_hint: raise('Not Implemented Error'), # TODO: impl. [REQUIRED] hintなのに必須とはどういうこと? post_logout_redirect_uri: options.post_logout_redirect_uri ) end @@ -690,7 +674,7 @@ def implicit_flow_callback_phase end def configured_response_type - if !@configured_response_type + unless @configured_response_type ary = case options.response_type when Array @@ -702,17 +686,15 @@ def configured_response_type end @configured_response_type = ary.sort.join(' ') end - return @configured_response_type + @configured_response_type end def verify_id_token!(decoded_id_token) - if !decoded_id_token.is_a?(::OpenIDConnect::ResponseObject::IdToken) - raise TypeError - end + raise TypeError unless decoded_id_token.is_a?(::OpenIDConnect::ResponseObject::IdToken) decoded_id_token.verify!( issuer: issuer, client_id: client_options.identifier, - nonce: session.delete('omniauth.nonce') + nonce: nonce ) end @@ -724,7 +706,7 @@ class CallbackError < StandardError # @param error_reason [OPTIONAL] Human-readable text. # @param error_uri [OPTIONAL] def initialize(error, error_reason = nil, error_uri = nil) - raise TypeError if !error + raise TypeError unless error @error = error @error_reason = error_reason