diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index a742d593..564b4825 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1,6 +1,6 @@ # This configuration was generated by # `rubocop --auto-gen-config` -# on 2024-07-09 11:29:15 UTC using RuboCop version 1.64.1. +# on 2024-07-09 08:57:21 UTC using RuboCop version 1.64.1. # The point is for the user to remove these configuration records # one by one as the offenses are removed from the code base. # Note that changes in the inspected code, or installation of new @@ -294,7 +294,7 @@ Performance/StringReplacement: - 'lib/ruby_saml/utils.rb' - 'lib/ruby_saml/xml/document.rb' -# Offense count: 54 +# Offense count: 52 # This cop supports safe autocorrection (--autocorrect). # Configuration parameters: EnforcedStyle. # SupportedStyles: separated, grouped @@ -414,7 +414,15 @@ Style/ModuleFunction: Exclude: - 'lib/ruby_saml/logging.rb' -# Offense count: 15 +# Offense count: 1 +# This cop supports unsafe autocorrection (--autocorrect-all). +# Configuration parameters: EnforcedStyle, Autocorrect. +# SupportedStyles: module_function, extend_self, forbidden +Style/ModuleFunction: + Exclude: + - 'lib/ruby_saml/logging.rb' + +# Offense count: 16 # Configuration parameters: AllowedMethods. # AllowedMethods: respond_to_missing? Style/OptionalBooleanParameter: diff --git a/CHANGELOG.md b/CHANGELOG.md index 1772f569..65359192 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ * [#685](https://github.com/SAML-Toolkits/ruby-saml/pull/685) Move schema files from `lib/onelogin/schemas` to `lib/ruby_saml/schemas`. * [#692](https://github.com/SAML-Toolkits/ruby-saml/pull/692) Remove `XMLSecurity` namespace and replace with `RubySaml::XML`. * [#686](https://github.com/SAML-Toolkits/ruby-saml/pull/686) Use SHA-256 as the default hashing algorithm everywhere instead of SHA-1, including signatures, fingerprints, and digests. +* [#695](https://github.com/SAML-Toolkits/ruby-saml/pull/695) Deprecate `settings.compress_request` and `settings.compess_response` parameters. * [#690](https://github.com/SAML-Toolkits/ruby-saml/pull/690) Remove deprecated `settings.security[:embed_sign]` parameter. ### 1.17.0 diff --git a/UPGRADING.md b/UPGRADING.md index 465381ea..30b64dce 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -68,6 +68,18 @@ settings.idp_slo_service_binding = :redirect For clarity, the default value of both parameters is `:redirect` if they are not set. +### Deprecation of Compression Settings + +The `settings.compress_request` and `settings.compress_response` parameters have been deprecated +and are no longer functional. They will be removed in RubySaml 2.1.0. Please remove `compress_request` +and `compress_response` everywhere within your project code. + +The SAML SP request/response message compression behavior is now controlled automatically by the +`settings.idp_sso_service_binding` and `settings.idp_slo_service_binding` parameters respectively: +`HTTP-Redirect` will always use compression, while `HTTP-POST` will not. For clarity, here +"compression" is used to make redirect URLs which contain SAML messages be shorter. For POST messages, +compression may be achieved by enabling `Content-Encoding: gzip` on your webserver. + ## Updating from 1.12.x to 1.13.0 Version `1.13.0` adds `settings.idp_sso_service_binding` and `settings.idp_slo_service_binding`, and diff --git a/lib/ruby_saml/authrequest.rb b/lib/ruby_saml/authrequest.rb index 73b9fd19..a96ae94f 100644 --- a/lib/ruby_saml/authrequest.rb +++ b/lib/ruby_saml/authrequest.rb @@ -56,6 +56,7 @@ def create_params(settings, params={}) # The method expects :RelayState but sometimes we get 'RelayState' instead. # Based on the HashWithIndifferentAccess value in Rails we could experience # conflicts so this line will solve them. + binding_redirect = settings.idp_sso_service_binding == Utils::BINDINGS[:redirect] relay_state = params[:RelayState] || params['RelayState'] if relay_state.nil? @@ -71,12 +72,12 @@ def create_params(settings, params={}) Logging.debug "Created AuthnRequest: #{request}" - request = deflate(request) if settings.compress_request + request = deflate(request) if binding_redirect base64_request = encode(request) request_params = {"SAMLRequest" => base64_request} sp_signing_key = settings.get_sp_signing_key - if settings.idp_sso_service_binding == Utils::BINDINGS[:redirect] && settings.security[:authn_requests_signed] && sp_signing_key + if binding_redirect && settings.security[:authn_requests_signed] && sp_signing_key params['SigAlg'] = settings.security[:signature_method] url_string = RubySaml::Utils.build_query( type: 'SAMLRequest', diff --git a/lib/ruby_saml/logoutrequest.rb b/lib/ruby_saml/logoutrequest.rb index f1b6e1d3..a82b9393 100644 --- a/lib/ruby_saml/logoutrequest.rb +++ b/lib/ruby_saml/logoutrequest.rb @@ -53,6 +53,7 @@ def create_params(settings, params={}) # The method expects :RelayState but sometimes we get 'RelayState' instead. # Based on the HashWithIndifferentAccess value in Rails we could experience # conflicts so this line will solve them. + binding_redirect = settings.idp_slo_service_binding == Utils::BINDINGS[:redirect] relay_state = params[:RelayState] || params['RelayState'] if relay_state.nil? @@ -68,12 +69,12 @@ def create_params(settings, params={}) Logging.debug "Created SLO Logout Request: #{request}" - request = deflate(request) if settings.compress_request + request = deflate(request) if binding_redirect base64_request = encode(request) request_params = {"SAMLRequest" => base64_request} sp_signing_key = settings.get_sp_signing_key - if settings.idp_slo_service_binding == Utils::BINDINGS[:redirect] && settings.security[:logout_requests_signed] && sp_signing_key + if binding_redirect && settings.security[:logout_requests_signed] && sp_signing_key params['SigAlg'] = settings.security[:signature_method] url_string = RubySaml::Utils.build_query( type: 'SAMLRequest', diff --git a/lib/ruby_saml/saml_message.rb b/lib/ruby_saml/saml_message.rb index 9e29f896..7d38ec90 100644 --- a/lib/ruby_saml/saml_message.rb +++ b/lib/ruby_saml/saml_message.rb @@ -7,6 +7,7 @@ require 'rexml/document' require 'rexml/xpath' require 'ruby_saml/error_handling' +require 'ruby_saml/logging' # Only supports SAML 2.0 module RubySaml @@ -104,11 +105,18 @@ def decode_raw_saml(saml, settings = nil) # Deflate, base64 encode and url-encode a SAML Message (To be used in the HTTP-redirect binding) # @param saml [String] The plain SAML Message - # @param settings [RubySaml::Settings|nil] Toolkit settings + # @param settings_or_compress [true|false|RubySaml::Settings|nil] Whether or not the SAML should be deflated. + # The usage of RubySaml::Settings here is deprecated. # @return [String] The deflated and encoded SAML Message (encoded if the compression is requested) - # - def encode_raw_saml(saml, settings) - saml = deflate(saml) if settings.compress_request + def encode_raw_saml(saml, settings_or_compress = false) + if settings_or_compress.is_a?(TrueClass) + saml = deflate(saml) + elsif settings_or_compress.respond_to?(:compress_request) + Logging.deprecate('Please change the second argument of `encode_raw_saml_message` to a boolean ' \ + 'indicating whether or not to use compression. Using a boolean will be required ' \ + 'in RubySaml 2.1.0.') + saml = deflate(saml) if settings_or_compress.compress_request + end CGI.escape(encode(saml)) end diff --git a/lib/ruby_saml/settings.rb b/lib/ruby_saml/settings.rb index 06a3b9f9..e0cd93aa 100644 --- a/lib/ruby_saml/settings.rb +++ b/lib/ruby_saml/settings.rb @@ -52,8 +52,6 @@ def initialize(overrides = {}, keep_security_attributes = false) attr_accessor :name_identifier_value attr_accessor :name_identifier_value_requested attr_accessor :sessionindex - attr_accessor :compress_request - attr_accessor :compress_response attr_accessor :double_quote_xml_attribute_values attr_accessor :message_max_bytesize attr_accessor :passive @@ -274,8 +272,6 @@ def get_binding(value) assertion_consumer_service_binding: Utils::BINDINGS[:post], single_logout_service_binding: Utils::BINDINGS[:redirect], idp_cert_fingerprint_algorithm: RubySaml::XML::Document::SHA256, - compress_request: true, - compress_response: true, message_max_bytesize: 250_000, soft: true, double_quote_xml_attribute_values: false, @@ -296,8 +292,40 @@ def get_binding(value) }.freeze }.freeze + # @deprecated Will be removed in v2.1.0 + def compress_request + compress_deprecation('compress_request', 'idp_sso_service_binding') + defined?(@compress_request) ? @compress_request : true + end + + # @deprecated Will be removed in v2.1.0 + def compress_request=(value) + compress_deprecation('compress_request', 'idp_sso_service_binding') + @compress_request = value + end + + # @deprecated Will be removed in v2.1.0 + def compress_response + compress_deprecation('compress_response', 'idp_slo_service_binding') + defined?(@compress_response) ? @compress_response : true + end + + # @deprecated Will be removed in v2.1.0 + def compress_response=(value) + compress_deprecation('compress_response', 'idp_slo_service_binding') + @compress_response = value + end + private + # @deprecated Will be removed in v2.1.0 + def compress_deprecation(old_param, new_param) + Logging.deprecate "`RubySaml::Settings##{old_param}` is deprecated and no longer functional. " \ + 'It will be removed in RubySaml 2.1.0. ' \ + "Its functionality is now handled by `RubySaml::Settings##{new_param}` instead: " \ + '"HTTP-Redirect" will always be compressed, and "HTTP-POST" will always be uncompressed.' + end + # @return [Hash>>] # Build the SP certificates and private keys from the settings. Returns all # certificates and private keys, even if they are expired. diff --git a/lib/ruby_saml/slo_logoutresponse.rb b/lib/ruby_saml/slo_logoutresponse.rb index 5dd0ab6d..b17d81f7 100644 --- a/lib/ruby_saml/slo_logoutresponse.rb +++ b/lib/ruby_saml/slo_logoutresponse.rb @@ -62,6 +62,7 @@ def create_params(settings, request_id = nil, logout_message = nil, params = {}, # The method expects :RelayState but sometimes we get 'RelayState' instead. # Based on the HashWithIndifferentAccess value in Rails we could experience # conflicts so this line will solve them. + binding_redirect = settings.idp_slo_service_binding == Utils::BINDINGS[:redirect] relay_state = params[:RelayState] || params['RelayState'] if relay_state.nil? @@ -77,12 +78,12 @@ def create_params(settings, request_id = nil, logout_message = nil, params = {}, Logging.debug "Created SLO Logout Response: #{response}" - response = deflate(response) if settings.compress_response + response = deflate(response) if binding_redirect base64_response = encode(response) response_params = {"SAMLResponse" => base64_response} sp_signing_key = settings.get_sp_signing_key - if settings.idp_slo_service_binding == Utils::BINDINGS[:redirect] && settings.security[:logout_responses_signed] && sp_signing_key + if binding_redirect && settings.security[:logout_responses_signed] && sp_signing_key params['SigAlg'] = settings.security[:signature_method] url_string = RubySaml::Utils.build_query( type: 'SAMLResponse', diff --git a/test/logoutrequest_test.rb b/test/logoutrequest_test.rb index 49528a4b..a1368249 100644 --- a/test/logoutrequest_test.rb +++ b/test/logoutrequest_test.rb @@ -152,21 +152,7 @@ class RequestTest < Minitest::Test assert_match %r[], inflated end - it "create a signed logout request" do - settings.compress_request = true - - unauth_req = RubySaml::Logoutrequest.new - unauth_url = unauth_req.create(settings) - - inflated = decode_saml_request_payload(unauth_url) - assert_match %r[([a-zA-Z0-9/+=]+)], inflated - assert_match %r[], inflated - assert_match %r[], inflated - end - it "create an uncompressed signed logout request" do - settings.compress_request = false - params = RubySaml::Logoutrequest.new.create_params(settings) request_xml = Base64.decode64(params["SAMLRequest"]) @@ -176,7 +162,6 @@ class RequestTest < Minitest::Test end it "create a signed logout request with 256 digest and signature method" do - settings.compress_request = false settings.security[:signature_method] = RubySaml::XML::Document::RSA_SHA256 settings.security[:digest_method] = RubySaml::XML::Document::SHA256 @@ -188,7 +173,6 @@ class RequestTest < Minitest::Test end it "create a signed logout request with 512 digest and signature method RSA_SHA384" do - settings.compress_request = false settings.security[:signature_method] = RubySaml::XML::Document::RSA_SHA384 settings.security[:digest_method] = RubySaml::XML::Document::SHA512 @@ -201,7 +185,6 @@ class RequestTest < Minitest::Test end it "create a signed logout request using the first certificate and key" do - settings.compress_request = false settings.certificate = nil settings.private_key = nil settings.sp_cert_multi = { @@ -220,7 +203,6 @@ class RequestTest < Minitest::Test end it "create a signed logout request using the first valid certificate and key when :check_sp_cert_expiration is true" do - settings.compress_request = false settings.certificate = nil settings.private_key = nil settings.security[:check_sp_cert_expiration] = true @@ -328,7 +310,6 @@ class RequestTest < Minitest::Test it "create a signature parameter using the first certificate and key" do settings.security[:signature_method] = RubySaml::XML::Document::RSA_SHA1 - settings.compress_request = false settings.certificate = nil settings.private_key = nil settings.sp_cert_multi = { diff --git a/test/request_test.rb b/test/request_test.rb index 9fec1d85..bf47b54e 100644 --- a/test/request_test.rb +++ b/test/request_test.rb @@ -42,7 +42,7 @@ class RequestTest < Minitest::Test end it "create the SAMLRequest URL parameter without deflating" do - settings.compress_request = false + settings.idp_sso_service_binding = RubySaml::Utils::BINDINGS[:post] auth_url = RubySaml::Authrequest.new.create(settings) assert_match(/^http:\/\/example\.com\?SAMLRequest=/, auth_url) payload = CGI.unescape(auth_url.split("=").last) @@ -242,7 +242,6 @@ class RequestTest < Minitest::Test describe "#create_params signing with HTTP-POST binding" do before do - settings.compress_request = false settings.idp_sso_service_url = "http://example.com?field=value" settings.idp_sso_service_binding = :post settings.security[:authn_requests_signed] = true @@ -317,7 +316,6 @@ class RequestTest < Minitest::Test let(:cert) { OpenSSL::X509::Certificate.new(ruby_saml_cert_text) } before do - settings.compress_request = false settings.idp_sso_service_url = "http://example.com?field=value" settings.idp_sso_service_binding = :redirect settings.assertion_consumer_service_binding = "urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST-SimpleSign" @@ -362,7 +360,6 @@ class RequestTest < Minitest::Test it "create a signature parameter using the first certificate and key" do settings.security[:signature_method] = RubySaml::XML::Document::RSA_SHA1 - settings.compress_request = false settings.certificate = nil settings.private_key = nil settings.sp_cert_multi = { diff --git a/test/saml_message_test.rb b/test/saml_message_test.rb index 56dc8f6a..ef0563e3 100644 --- a/test/saml_message_test.rb +++ b/test/saml_message_test.rb @@ -15,6 +15,15 @@ class RubySamlTest < Minitest::Test end it "return encoded raw saml" do + encoded_raw = saml_message.send(:encode_raw_saml, logout_request_document, true) + assert logout_request_deflated_base64, encoded_raw + + deflated = saml_message.send(:deflate, logout_request_deflated_base64) + encoded_raw = saml_message.send(:encode_raw_saml, deflated, false) + assert logout_request_deflated_base64, encoded_raw + end + + it "DEPRECATED: return encoded raw saml using RubySaml::Settings as the second arg" do settings.compress_request = true encoded_raw = saml_message.send(:encode_raw_saml, logout_request_document, settings) assert logout_request_deflated_base64, encoded_raw diff --git a/test/settings_test.rb b/test/settings_test.rb index 865f0588..ff336290 100644 --- a/test/settings_test.rb +++ b/test/settings_test.rb @@ -17,7 +17,7 @@ class SettingsTest < Minitest::Test :idp_attribute_names, :issuer, :assertion_consumer_service_url, :single_logout_service_url, :sp_name_qualifier, :name_identifier_format, :name_identifier_value, :name_identifier_value_requested, :sessionindex, :attributes_index, :passive, :force_authn, - :compress_request, :double_quote_xml_attribute_values, :message_max_bytesize, + :double_quote_xml_attribute_values, :message_max_bytesize, :security, :certificate, :private_key, :certificate_new, :sp_cert_multi, :authn_context, :authn_context_comparison, :authn_context_decl_ref, :assertion_consumer_logout_service_url diff --git a/test/slo_logoutresponse_test.rb b/test/slo_logoutresponse_test.rb index 94f995fd..421017f3 100644 --- a/test/slo_logoutresponse_test.rb +++ b/test/slo_logoutresponse_test.rb @@ -12,7 +12,6 @@ class SloLogoutresponseTest < Minitest::Test settings.idp_entity_id = 'https://app.onelogin.com/saml/metadata/SOMEACCOUNT' settings.idp_slo_service_url = "http://unauth.com/logout" settings.name_identifier_value = "f00f00" - settings.compress_request = true settings.certificate = ruby_saml_cert_text settings.private_key = ruby_saml_key_text logout_request.settings = settings @@ -102,7 +101,6 @@ class SloLogoutresponseTest < Minitest::Test before do settings.idp_sso_service_binding = :redirect settings.idp_slo_service_binding = :post - settings.compress_response = false settings.security[:logout_responses_signed] = true end @@ -232,7 +230,6 @@ class SloLogoutresponseTest < Minitest::Test before do settings.idp_sso_service_binding = :post settings.idp_slo_service_binding = :redirect - settings.compress_response = false settings.security[:logout_responses_signed] = true end @@ -313,7 +310,6 @@ class SloLogoutresponseTest < Minitest::Test it "create a signature parameter using the first certificate and key" do settings.security[:signature_method] = RubySaml::XML::Document::RSA_SHA1 - settings.compress_request = false settings.certificate = nil settings.private_key = nil settings.sp_cert_multi = {