Remove the deprecated settings.security[:embed_sign] parameter. It ca…

…n be migrated to idp_sso/slo_service_binding. (#690)
SAML-Toolkits · Jul 9, 2024 · e9c7f97 · e9c7f97
1 parent 38d41fe
commit e9c7f97
Show file tree

Hide file tree

Showing 8 changed files with 24 additions and 200 deletions.
diff --git 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.
+* [#690](https://github.com/SAML-Toolkits/ruby-saml/pull/690) Remove deprecated `settings.security[:embed_sign]` parameter.
 
 ### 1.17.0
 * [#673](https://github.com/SAML-Toolkits/ruby-saml/pull/673) Add `Settings#sp_cert_multi` paramter to facilitate SP certificate and key rotation.

diff --git a/UPGRADING.md b/UPGRADING.md
@@ -2,6 +2,8 @@
 
 ## Updating from 1.17.x to 2.0.0
 
+**IMPORTANT: Please read this section carefully as it contains breaking changes!**
+
 ### Before upgrading
 
 Before attempting to upgrade to `2.0.0`:
@@ -48,6 +50,24 @@ settings.security[:digest_method] = RubySaml::XML::Document::SHA1
 settings.security[:signature_method] = RubySaml::XML::Document::RSA_SHA1
 ```
 
+### Removal of embed_sign Setting
+
+The deprecated `settings.security[:embed_sign]` parameter has been removed. If you were using it, please instead switch
+to using both the `settings.idp_sso_service_binding` and `settings.idp_slo_service_binding` parameters as show below.
+(This new syntax is supported on version 1.13.0 and later.)
+
+```ruby
+# Replace settings.security[:embed_sign] = true with
+settings.idp_sso_service_binding = :post
+settings.idp_slo_service_binding = :post
+
+# Replace settings.security[:embed_sign] = false with
+settings.idp_sso_service_binding = :redirect
+settings.idp_slo_service_binding = :redirect
+```
+
+For clarity, the default value of both parameters is `:redirect` if they are not set.
+
 ## 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/settings.rb b/lib/ruby_saml/settings.rb
@@ -93,7 +93,7 @@ def idp_slo_service_url
     # @return [String] IdP Single Sign On Service Binding
     #
     def idp_sso_service_binding
-      @idp_sso_service_binding || idp_binding_from_embed_sign
+      @idp_sso_service_binding || Utils::BINDINGS[:redirect]
     end
 
     # Setter for IdP Single Sign On Service Binding
@@ -106,7 +106,7 @@ def idp_sso_service_binding=(value)
     # @return [String] IdP Single Logout Service Binding
     #
     def idp_slo_service_binding
-      @idp_slo_service_binding || idp_binding_from_embed_sign
+      @idp_slo_service_binding || Utils::BINDINGS[:redirect]
     end
 
     # Setter for IdP Single Logout Service Binding
@@ -264,10 +264,6 @@ def get_sp_cert_new
       node[0] if node
     end
 
-    def idp_binding_from_embed_sign
-      security[:embed_sign] ? Utils::BINDINGS[:post] : Utils::BINDINGS[:redirect]
-    end
-
     def get_binding(value)
       return unless value
 
@@ -291,7 +287,6 @@ def get_binding(value)
         want_assertions_encrypted: false,
         want_name_id: false,
         metadata_signed: false,
-        embed_sign: false, # Deprecated
         digest_method: RubySaml::XML::Document::SHA256,
         signature_method: RubySaml::XML::Document::RSA_SHA256,
         check_idp_cert_expiration: false,

diff --git a/test/idp_metadata_parser_test.rb b/test/idp_metadata_parser_test.rb
@@ -654,8 +654,8 @@ def initialize; end
       assert_equal("https://app.onelogin.com/saml/metadata/383123", @settings.idp_entity_id)
       assert_equal("urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress", @settings.name_identifier_format)
       assert_equal("https://app.onelogin.com/trust/saml2/http-post/sso/383123", @settings.idp_sso_service_url)
+      assert_equal("urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect", @settings.idp_sso_service_binding)
       assert_nil(@settings.idp_slo_service_url)
-      # TODO: next line can be changed to `assert_nil @settings.idp_slo_service_binding` after :embed_sign is removed.
       assert_nil(@settings.instance_variable_get('@idp_slo_service_binding'))
     end
   end
@@ -726,7 +726,6 @@ def initialize; end
       assert_equal "https://app.onelogin.com/trust/saml2/http-post/sso/383123", @settings.idp_sso_service_url
       assert_equal "urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect", @settings.idp_sso_service_binding
       assert_nil @settings.idp_slo_service_url
-      # TODO: next line can be changed to `assert_nil @settings.idp_slo_service_binding` after :embed_sign is removed.
       assert_nil @settings.instance_variable_get('@idp_slo_service_binding')
     end
   end

diff --git a/test/logoutrequest_test.rb b/test/logoutrequest_test.rb
@@ -362,59 +362,6 @@ class RequestTest < Minitest::Test
       end
     end
 
-    describe "DEPRECATED: signing with HTTP-POST binding via :embed_sign" do
-
-      before do
-        # sign the logout request
-        settings.security[:logout_requests_signed] = true
-        settings.security[:embed_sign] = true
-        settings.certificate = ruby_saml_cert_text
-        settings.private_key = ruby_saml_key_text
-      end
-
-      it "created 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[<ds:SignatureValue>([a-zA-Z0-9/+=]+)</ds:SignatureValue>], inflated
-        assert_match %r[<ds:SignatureMethod Algorithm='http://www.w3.org/2001/04/xmldsig-more#rsa-sha256'/>], inflated
-        assert_match %r[<ds:DigestMethod Algorithm='http://www.w3.org/2001/04/xmlenc#sha256'/>], inflated
-      end
-    end
-
-    describe "DEPRECATED: signing with HTTP-Redirect binding via :embed_sign" do
-
-      let(:cert) { OpenSSL::X509::Certificate.new(ruby_saml_cert_text) }
-
-      before do
-        settings.security[:logout_requests_signed] = true
-        settings.security[:embed_sign] = false
-        settings.certificate = ruby_saml_cert_text
-        settings.private_key = ruby_saml_key_text
-      end
-
-      it "create a signature parameter with RSA_SHA1 / SHA1 and validate it" do
-        settings.security[:signature_method] = RubySaml::XML::Document::RSA_SHA1
-
-        params = RubySaml::Logoutrequest.new.create_params(settings, :RelayState => 'http://example.com')
-        assert params['SAMLRequest']
-        assert params[:RelayState]
-        assert params['Signature']
-        assert_equal params['SigAlg'], RubySaml::XML::Document::RSA_SHA1
-
-        query_string = "SAMLRequest=#{CGI.escape(params['SAMLRequest'])}"
-        query_string << "&RelayState=#{CGI.escape(params[:RelayState])}"
-        query_string << "&SigAlg=#{CGI.escape(params['SigAlg'])}"
-
-        signature_algorithm = RubySaml::XML::BaseDocument.new.algorithm(params['SigAlg'])
-        assert_equal signature_algorithm, OpenSSL::Digest::SHA1
-        assert cert.public_key.verify(signature_algorithm.new, Base64.decode64(params['Signature']), query_string)
-      end
-    end
-
     describe "#manipulate request_id" do
       it "be able to modify the request id" do
         logoutrequest = RubySaml::Logoutrequest.new

diff --git a/test/request_test.rb b/test/request_test.rb
@@ -430,57 +430,6 @@ class RequestTest < Minitest::Test
       assert auth_doc.to_s =~ /<saml:AuthnContextDeclRef>example\/decl\/ref<\/saml:AuthnContextDeclRef>/
     end
 
-    describe "DEPRECATED: #create_params signing with HTTP-POST binding via :embed_sign" do
-      before do
-        settings.compress_request = false
-        settings.idp_sso_service_url = "http://example.com?field=value"
-        settings.security[:authn_requests_signed] = true
-        settings.security[:embed_sign] = true
-        settings.certificate = ruby_saml_cert_text
-        settings.private_key = ruby_saml_key_text
-      end
-
-      it "create a signed request" do
-        params = RubySaml::Authrequest.new.create_params(settings)
-        request_xml = Base64.decode64(params["SAMLRequest"])
-        assert_match %r[<ds:SignatureValue>([a-zA-Z0-9/+=]+)</ds:SignatureValue>], request_xml
-        assert_match %r[<ds:SignatureMethod Algorithm='http://www.w3.org/2001/04/xmldsig-more#rsa-sha256'/>], request_xml
-      end
-    end
-
-    describe "DEPRECATED: #create_params signing with HTTP-Redirect binding via :embed_sign" do
-      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.assertion_consumer_service_binding = "urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST-SimpleSign"
-        settings.security[:authn_requests_signed] = true
-        settings.security[:embed_sign] = false
-        settings.certificate = ruby_saml_cert_text
-        settings.private_key = ruby_saml_key_text
-      end
-
-      it "create a signature parameter with RSA_SHA1 and validate it" do
-        settings.security[:signature_method] = RubySaml::XML::Document::RSA_SHA1
-
-        params = RubySaml::Authrequest.new.create_params(settings, :RelayState => 'http://example.com')
-        assert params['SAMLRequest']
-        assert params[:RelayState]
-        assert params['Signature']
-        assert_equal params['SigAlg'], RubySaml::XML::Document::RSA_SHA1
-
-        query_string = "SAMLRequest=#{CGI.escape(params['SAMLRequest'])}"
-        query_string << "&RelayState=#{CGI.escape(params[:RelayState])}"
-        query_string << "&SigAlg=#{CGI.escape(params['SigAlg'])}"
-
-        signature_algorithm = RubySaml::XML::BaseDocument.new.algorithm(params['SigAlg'])
-        assert_equal signature_algorithm, OpenSSL::Digest::SHA1
-
-        assert cert.public_key.verify(signature_algorithm.new, Base64.decode64(params['Signature']), query_string)
-      end
-    end
-
     describe "#manipulate request_id" do
       it "be able to modify the request id" do
         authnrequest = RubySaml::Authrequest.new

diff --git a/test/settings_test.rb b/test/settings_test.rb
@@ -55,30 +55,6 @@ class SettingsTest < Minitest::Test
       end
     end
 
-    it "idp_sso/slo_service_binding should fallback to :embed_sign inferred value" do
-      accessors = [:idp_sso_service_binding, :idp_slo_service_binding]
-
-      accessors.each do |accessor|
-        @settings.security[:embed_sign] = true
-
-        value = Kernel.rand.to_s
-        @settings.send("#{accessor}=".to_sym, value)
-        assert_equal value, @settings.send(accessor)
-
-        @settings.send("#{accessor}=".to_sym, :redirect)
-        assert_equal "urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect", @settings.send(accessor)
-
-        @settings.send("#{accessor}=".to_sym, :post)
-        assert_equal "urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST", @settings.send(accessor)
-
-        @settings.send("#{accessor}=".to_sym, nil)
-        assert_equal "urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST", @settings.send(accessor)
-
-        @settings.security[:embed_sign] = false
-        assert_equal "urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect", @settings.send(accessor)
-      end
-    end
-
     it "create settings from hash" do
       config = {
           :assertion_consumer_service_url => "http://app.muda.no/sso",
@@ -117,13 +93,11 @@ class SettingsTest < Minitest::Test
     it "does not modify default security settings" do
       settings = RubySaml::Settings.new
       settings.security[:authn_requests_signed] = true
-      settings.security[:embed_sign] = true
       settings.security[:digest_method] = RubySaml::XML::Document::SHA512
       settings.security[:signature_method] = RubySaml::XML::Document::RSA_SHA512
 
       new_settings = RubySaml::Settings.new
       assert_equal new_settings.security[:authn_requests_signed], false
-      assert_equal new_settings.security[:embed_sign], false
       assert_equal new_settings.security[:digest_method], RubySaml::XML::Document::SHA256
       assert_equal new_settings.security[:signature_method], RubySaml::XML::Document::RSA_SHA256
     end

diff --git a/test/slo_logoutresponse_test.rb b/test/slo_logoutresponse_test.rb
@@ -347,67 +347,6 @@ class SloLogoutresponseTest < Minitest::Test
       end
     end
 
-    describe "DEPRECATED: signing with HTTP-POST binding via :embed_sign" do
-      before do
-        settings.compress_response = false
-        settings.security[:logout_responses_signed] = true
-        settings.security[:embed_sign] = true
-      end
-
-      it "doesn't sign through create_xml_document" do
-        unauth_res = RubySaml::SloLogoutresponse.new
-        inflated = unauth_res.create_xml_document(settings).to_s
-
-        refute_match %r[<ds:SignatureValue>([a-zA-Z0-9/+=]+)</ds:SignatureValue>], inflated
-        refute_match %r[<ds:SignatureMethod Algorithm='http://www.w3.org/2001/04/xmldsig-more#rsa-sha256'/>], inflated
-        refute_match %r[<ds:DigestMethod Algorithm='http://www.w3.org/2001/04/xmlenc#sha256'/>], inflated
-      end
-
-      it "sign unsigned request" do
-        unauth_res = RubySaml::SloLogoutresponse.new
-        unauth_res_doc = unauth_res.create_xml_document(settings)
-        inflated = unauth_res_doc.to_s
-
-        refute_match %r[<ds:SignatureValue>([a-zA-Z0-9/+=]+)</ds:SignatureValue>], inflated
-        refute_match %r[<ds:SignatureMethod Algorithm='http://www.w3.org/2001/04/xmldsig-more#rsa-sha256'/>], inflated
-        refute_match %r[<ds:DigestMethod Algorithm='http://www.w3.org/2001/04/xmlenc#sha256'/>], inflated
-
-        inflated = unauth_res.sign_document(unauth_res_doc, settings).to_s
-
-        assert_match %r[<ds:SignatureValue>([a-zA-Z0-9/+=]+)</ds:SignatureValue>], inflated
-        assert_match %r[<ds:SignatureMethod Algorithm='http://www.w3.org/2001/04/xmldsig-more#rsa-sha256'/>], inflated
-        assert_match %r[<ds:DigestMethod Algorithm='http://www.w3.org/2001/04/xmlenc#sha256'/>], inflated
-      end
-    end
-
-    describe "DEPRECATED: signing with HTTP-Redirect binding via :embed_sign" do
-      let(:cert) { OpenSSL::X509::Certificate.new(ruby_saml_cert_text) }
-
-      before do
-        settings.compress_response = false
-        settings.security[:logout_responses_signed] = true
-        settings.security[:embed_sign] = false
-      end
-
-      it "create a signature parameter with RSA_SHA1 and validate it" do
-        settings.security[:signature_method] = RubySaml::XML::Document::RSA_SHA1
-
-        params = RubySaml::SloLogoutresponse.new.create_params(settings, logout_request.id, "Custom Logout Message", :RelayState => 'http://example.com')
-        assert params['SAMLResponse']
-        assert params[:RelayState]
-        assert params['Signature']
-        assert_equal params['SigAlg'], RubySaml::XML::Document::RSA_SHA1
-
-        query_string = "SAMLResponse=#{CGI.escape(params['SAMLResponse'])}"
-        query_string << "&RelayState=#{CGI.escape(params[:RelayState])}"
-        query_string << "&SigAlg=#{CGI.escape(params['SigAlg'])}"
-
-        signature_algorithm = RubySaml::XML::BaseDocument.new.algorithm(params['SigAlg'])
-        assert_equal signature_algorithm, OpenSSL::Digest::SHA1
-        assert cert.public_key.verify(signature_algorithm.new, Base64.decode64(params['Signature']), query_string)
-      end
-    end
-
     describe "#manipulate response_id" do
       it "be able to modify the response id" do
         logoutresponse = RubySaml::SloLogoutresponse.new