Merge pull request SAML-Toolkits#397 from geneshuman/master

added SubjectConfirmation Recipient validation

Author: pitbulk

README.md

@@ -193,11 +193,12 @@ def saml_settings
 end
 ```
 
-Some assertion validations can be skipped by passing parameters to `OneLogin::RubySaml::Response.new()`.  For example, you can skip the `Conditions` validation or the `SubjectConfirmation` validations by initializing the response with different options:
+Some assertion validations can be skipped by passing parameters to `OneLogin::RubySaml::Response.new()`.  For example, you can skip the `Conditions`, `Recipient`, or the `SubjectConfirmation` validations by initializing the response with different options:
 
 ```ruby
 response = OneLogin::RubySaml::Response.new(params[:SAMLResponse], {skip_conditions: true}) # skips conditions
 response = OneLogin::RubySaml::Response.new(params[:SAMLResponse], {skip_subject_confirmation: true}) # skips subject confirmation
+response = OneLogin::RubySaml::Response.new(params[:SAMLResponse], {skip_recipient_check: true}) # doens't skip subject confirmation, but skips the recipient check which is a sub check of the subject_confirmation check
 ```
 
 All that's left is to wrap everything in a controller and reference it in the initialization and consumption URLs in OneLogin. A full controller example could look like this:

lib/onelogin/ruby-saml/response.rb

@@ -32,7 +32,7 @@ class Response < SamlMessage
 
       # Constructs the SAML Response. A Response Object that is an extension of the SamlMessage class.
       # @param response [String] A UUEncoded SAML response from the IdP.
-      # @param options  [Hash]   :settings to provide the OneLogin::RubySaml::Settings object 
+      # @param options  [Hash]   :settings to provide the OneLogin::RubySaml::Settings object
       #                          Or some options for the response validation process like skip the conditions validation
       #                          with the :skip_conditions, or allow a clock_drift when checking dates with :allowed_clock_drift
       #                          or :matches_request_id that will validate that the response matches the ID of the request,
@@ -41,6 +41,7 @@ def initialize(response, options = {})
         raise ArgumentError.new("Response cannot be nil") if response.nil?
 
         @errors = []
+
         @options = options
         @soft = true
         unless options[:settings].nil?
@@ -189,7 +190,7 @@ def session_expires_at
 
       # Checks if the Status has the "Success" code
       # @return [Boolean] True if the StatusCode is Sucess
-      # 
+      #
       def success?
         status_code == "urn:oasis:names:tc:SAML:2.0:status:Success"
       end
@@ -376,15 +377,15 @@ def validate(collect_errors = false)
       #
       def validate_success_status
         return true if success?
-          
+
         error_msg = 'The status code of the Response was not Success'
         status_error_msg = OneLogin::RubySaml::Utils.status_error_msg(error_msg, status_code, status_message)
         append_error(status_error_msg)
       end
 
       # Validates the SAML Response against the specified schema.
       # @return [Boolean] True if the XML is valid, otherwise False if soft=True
-      # @raise [ValidationError] if soft == false and validation fails 
+      # @raise [ValidationError] if soft == false and validation fails
       #
       def validate_structure
         structure_error_msg = "Invalid SAML Response. Not match the saml-schema-protocol-2.0.xsd"
@@ -417,7 +418,7 @@ def validate_response_state
         true
       end
 
-      # Validates that the SAML Response contains an ID 
+      # Validates that the SAML Response contains an ID
       # If fails, the error is added to the errors array.
       # @return [Boolean] True if the SAML Response contains an ID, otherwise returns False
       #
@@ -706,8 +707,9 @@ def validate_session_expiration(soft = true)
       end
 
       # Validates if exists valid SubjectConfirmation (If the response was initialized with the :allowed_clock_drift option,
-      # timimg validation are relaxed by the allowed_clock_drift value. If the response was initialized with the 
+      # timimg validation are relaxed by the allowed_clock_drift value. If the response was initialized with the
       # :skip_subject_confirmation option, this validation is skipped)
+      # There is also an optional Recipient check
       # If fails, the error is added to the errors array
       # @return [Boolean] True if exists a valid SubjectConfirmation, otherwise False if soft=True
       # @raise [ValidationError] if soft == false and validation fails
@@ -717,7 +719,7 @@ def validate_subject_confirmation
         valid_subject_confirmation = false
 
         subject_confirmation_nodes = xpath_from_signed_assertion('/a:Subject/a:SubjectConfirmation')
-        
+
         now = Time.now.utc
         subject_confirmation_nodes.each do |subject_confirmation|
           if subject_confirmation.attributes.include? "Method" and subject_confirmation.attributes['Method'] != 'urn:oasis:names:tc:SAML:2.0:cm:bearer'
@@ -735,8 +737,9 @@ def validate_subject_confirmation
           attrs = confirmation_data_node.attributes
           next if (attrs.include? "InResponseTo" and attrs['InResponseTo'] != in_response_to) ||
                   (attrs.include? "NotOnOrAfter" and (parse_time(confirmation_data_node, "NotOnOrAfter") + allowed_clock_drift) <= now) ||
-                  (attrs.include? "NotBefore" and parse_time(confirmation_data_node, "NotBefore") > (now + allowed_clock_drift))
-          
+                  (attrs.include? "NotBefore" and parse_time(confirmation_data_node, "NotBefore") > (now + allowed_clock_drift)) ||
+                  (attrs.include? "Recipient" and !options[:skip_recipient_check] and settings and attrs['Recipient'] != settings.assertion_consumer_service_url)
+
           valid_subject_confirmation = true
           break
         end
@@ -808,7 +811,7 @@ def validate_signature
         opts[:cert] = settings.get_idp_cert
         fingerprint = settings.get_fingerprint
 
-        unless fingerprint && doc.validate_document(fingerprint, @soft, opts)          
+        unless fingerprint && doc.validate_document(fingerprint, @soft, opts)
           return append_error(error_msg)
         end
 

lib/onelogin/ruby-saml/settings.rb

@@ -94,7 +94,7 @@ def single_logout_service_binding
       end
 
       # Setter for Single Logout Service Binding.
-      # 
+      #
       # (Currently we only support "urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect")
       # @param url [String]
       #

test/response_test.rb

@@ -17,6 +17,7 @@ class RubySamlTest < Minitest::Test
     let(:response_wrapped) { OneLogin::RubySaml::Response.new(response_document_wrapped) }
     let(:response_multiple_attr_values) { OneLogin::RubySaml::Response.new(fixture(:response_with_multiple_attribute_values)) }
     let(:response_valid_signed) { OneLogin::RubySaml::Response.new(response_document_valid_signed) }
+    let(:response_valid_signed_without_recipient) { OneLogin::RubySaml::Response.new(response_document_valid_signed, {:skip_recipient_check => true })}
     let(:response_valid_signed_without_x509certificate) { OneLogin::RubySaml::Response.new(response_document_valid_signed_without_x509certificate) }
     let(:response_no_id) { OneLogin::RubySaml::Response.new(read_invalid_response("no_id.xml.base64")) }
     let(:response_no_version) { OneLogin::RubySaml::Response.new(read_invalid_response("no_saml2.xml.base64")) }
@@ -241,19 +242,19 @@ class RubySamlTest < Minitest::Test
         end
 
         it "return true when the response is initialized with valid data" do
-          response_valid_signed.stubs(:conditions).returns(nil)
-          response_valid_signed.settings = settings
-          response_valid_signed.settings.idp_cert_fingerprint = ruby_saml_cert_fingerprint
-          assert response_valid_signed.is_valid?
-          assert_empty response_valid_signed.errors
+          response_valid_signed_without_recipient.stubs(:conditions).returns(nil)
+          response_valid_signed_without_recipient.settings = settings
+          response_valid_signed_without_recipient.settings.idp_cert_fingerprint = ruby_saml_cert_fingerprint
+          assert response_valid_signed_without_recipient.is_valid?
+          assert_empty response_valid_signed_without_recipient.errors
         end
 
         it "return true when the response is initialized with valid data and using certificate instead of fingerprint" do
-          response_valid_signed.stubs(:conditions).returns(nil)
-          response_valid_signed.settings = settings
-          response_valid_signed.settings.idp_cert = ruby_saml_cert_text
-          assert response_valid_signed.is_valid?
-          assert_empty response_valid_signed.errors
+          response_valid_signed_without_recipient.stubs(:conditions).returns(nil)
+          response_valid_signed_without_recipient.settings = settings
+          response_valid_signed_without_recipient.settings.idp_cert = ruby_saml_cert_text
+          assert response_valid_signed_without_recipient.is_valid?
+          assert_empty response_valid_signed_without_recipient.errors
         end
 
         it "return false when response is initialized with blank data" do
@@ -282,11 +283,11 @@ class RubySamlTest < Minitest::Test
         end
 
         it "should be idempotent when the response is initialized with valid data" do
-          response_valid_signed.stubs(:conditions).returns(nil)
-          response_valid_signed.settings = settings
-          response_valid_signed.settings.idp_cert_fingerprint = ruby_saml_cert_fingerprint
-          assert response_valid_signed.is_valid?
-          assert response_valid_signed.is_valid?
+          response_valid_signed_without_recipient.stubs(:conditions).returns(nil)
+          response_valid_signed_without_recipient.settings = settings
+          response_valid_signed_without_recipient.settings.idp_cert_fingerprint = ruby_saml_cert_fingerprint
+          assert response_valid_signed_without_recipient.is_valid?
+          assert response_valid_signed_without_recipient.is_valid?
         end
 
         it "not allow signature wrapping attack" do
@@ -382,6 +383,7 @@ class RubySamlTest < Minitest::Test
 
         it "return true when a nil URI is given in the ds:Reference" do
           settings.idp_cert = ruby_saml_cert_text
+          settings.assertion_consumer_service_url = "http://localhost:9001/v1/users/authorize/saml"
           response_without_reference_uri.settings = settings
           response_without_reference_uri.stubs(:conditions).returns(nil)
           response_without_reference_uri.is_valid?
@@ -676,6 +678,28 @@ class RubySamlTest < Minitest::Test
         assert_includes response_invalid_subjectconfirmation_noa.errors, "A valid SubjectConfirmation was not found on this Response"
       end
 
+      it "return true when valid subject confirmation recipient" do
+        response_valid_signed.settings = settings
+        response_valid_signed.settings.assertion_consumer_service_url = 'recipient'
+        assert response_valid_signed.send(:validate_subject_confirmation)
+        assert_empty response_valid_signed.errors
+        assert_empty response_valid_signed.errors
+      end
+
+      it "return false when invalid subject confirmation recipient" do
+        response_valid_signed.settings = settings
+        response_valid_signed.settings.assertion_consumer_service_url = 'not-the-recipient'
+        assert !response_valid_signed.send(:validate_subject_confirmation)
+        assert_includes response_valid_signed.errors, "A valid SubjectConfirmation was not found on this Response"
+      end
+
+      it "return false when invalid subject confirmation recipient, but skipping the check(default)" do
+        response_valid_signed_without_recipient.settings = settings
+        response_valid_signed_without_recipient.settings.assertion_consumer_service_url = 'not-the-recipient'
+        assert response_valid_signed_without_recipient.send(:validate_subject_confirmation)
+        assert_empty response_valid_signed_without_recipient.errors
+      end
+
       it "return true when the skip_subject_confirmation option is passed and the subject confirmation is valid" do
         opts = {}
         opts[:skip_subject_confirmation] = true
@@ -1123,6 +1147,7 @@ class RubySamlTest < Minitest::Test
         document.sign_document(private_key, cert)
 
         signed_response = OneLogin::RubySaml::Response.new(document.to_s)
+        settings.assertion_consumer_service_url = "http://recipient"
         settings.idp_cert = ruby_saml_cert_text
         signed_response.settings = settings
         Timecop.freeze(Time.parse("2015-03-18T04:50:24Z")) do