Commit 4a50e50

mo <mo.khan@gmail.com>
2018-02-16 02:35:07
delegate to signature class for signature validation.
1 parent 2b1bad5
lib/saml/kit/locales/en.yml
@@ -33,6 +33,7 @@ en:
         certificate: "Not valid before %{not_before}. Not valid after %{not_after}."
         digest_value: "is invalid."
         empty: "is missing."
+        signature: "is invalid."
       SPSSODescriptor:
         invalid: "must contain SPSSODescriptor."
         invalid_signature: "invalid signature."
lib/saml/kit/assertion.rb
@@ -10,6 +10,7 @@ module Saml
 
       validate :must_match_issuer
       validate :must_be_active_session
+      validate :must_have_valid_signature
       attr_reader :name
       attr_accessor :occurred_at
 
@@ -19,8 +20,11 @@ module Saml
         @xml_hash = hash_from(node)['Response'] || {}
         @configuration = configuration
         @occurred_at = Time.current
-        @private_keys = (configuration.private_keys(use: :encryption) + private_keys).uniq
-        decrypt!
+        decrypt!(::Xml::Kit::Decryption.new(
+          private_keys: (
+            configuration.private_keys(use: :encryption) + private_keys
+          ).uniq
+        ))
       end
 
       def issuer
@@ -36,8 +40,7 @@ module Saml
       end
 
       def signature
-        xml_hash = assertion.fetch('Signature', nil)
-        xml_hash ? Signature.new(at_xpath('//ds:Signature')) : nil
+        @signature ||= Signature.new(at_xpath('./ds:Signature'))
       end
 
       def expired?(now = occurred_at)
@@ -92,7 +95,6 @@ module Saml
       private
 
       attr_reader :configuration
-      attr_reader :private_keys
 
       def assertion
         @assertion ||=
@@ -103,17 +105,11 @@ module Saml
           end
       end
 
-      def decrypt!
+      def decrypt!(decryptor)
         return unless encrypted?
-        decryptor = ::Xml::Kit::Decryption.new(private_keys: private_keys)
-        encrypted_assertion = @node.document.at_xpath(
-          '/samlp:Response/saml:EncryptedAssertion/xmlenc:EncryptedData',
-          'xmlenc' => ::Xml::Kit::Namespaces::XMLENC,
-          "saml": ::Saml::Kit::Namespaces::ASSERTION,
-          "samlp": ::Saml::Kit::Namespaces::PROTOCOL
-        )
+
+        encrypted_assertion = @node.at_xpath('./xmlenc:EncryptedData', Saml::Kit::Document::NAMESPACES)
         @node = decryptor.decrypt_node(encrypted_assertion)
-        #(hash_from(@node)['Response'] || {})['Assertion']
       end
 
       def parse_date(value)
@@ -134,6 +130,14 @@ module Saml
         errors[:base] << error_message(:expired)
       end
 
+      def must_have_valid_signature
+        if signed? && signature.invalid?
+          signature.errors.each do |attribute, message|
+            errors.add(attribute, message)
+          end
+        end
+      end
+
       def at_xpath(xpath)
         @node.at_xpath(xpath, Saml::Kit::Document::NAMESPACES)
       end
lib/saml/kit/document.rb
@@ -8,6 +8,7 @@ module Saml
         "md": ::Saml::Kit::Namespaces::METADATA,
         "saml": ::Saml::Kit::Namespaces::ASSERTION,
         "samlp": ::Saml::Kit::Namespaces::PROTOCOL,
+        "xmlenc" => ::Xml::Kit::Namespaces::XMLENC,
       }.freeze
       include ActiveModel::Validations
       include XsdValidatable
lib/saml/kit/signature.rb
@@ -33,6 +33,10 @@ module Saml
         @xml_hash
       end
 
+      def present?
+        @node
+      end
+
       private
 
       def validate_signature
spec/saml/assertion_spec.rb
@@ -147,7 +147,8 @@ XML
   describe "#valid?" do
     let(:entity_id) { FFaker::Internet.uri("https") }
     let(:request) { instance_double(Saml::Kit::AuthenticationRequest, id: ::Xml::Kit::Id.generate, issuer: entity_id, assertion_consumer_service_url: FFaker::Internet.http_url, name_id_format: Saml::Kit::Namespaces::PERSISTENT, provider: nil, signed?: true, trusted?: true) }
-    let(:user) { double(:user, name_id_for: SecureRandom.uuid, assertion_attributes_for: { id: SecureRandom.uuid }) }
+    let(:name_id) { SecureRandom.uuid }
+    let(:user) { double(:user, name_id_for: name_id, assertion_attributes_for: { id: SecureRandom.uuid }) }
     let(:registry) { double(:registry, metadata_for: idp) }
     let(:idp) { Saml::Kit::IdentityProviderMetadata.build(configuration: configuration) }
     let(:configuration) do
@@ -162,19 +163,41 @@ XML
     end
 
     it 'is invalid when the encrypted signature is invalid' do
+      xml = Saml::Kit::Response.build_xml(user, request, configuration: configuration)
+      altered = xml.gsub(name_id, 'altered')
+      document = Nokogiri::XML(altered)
+      assertion = document.at_xpath("/samlp:Response/saml:Assertion", Saml::Kit::Document::NAMESPACES)
+      key_pair = Xml::Kit::KeyPair.generate(use: :encryption)
+      encrypted = Xml::Kit::Encryption.new(assertion.to_xml, key_pair.public_key).to_xml
+      response = Saml::Kit::Response.new(encrypted, configuration: configuration)
+      expect(response.assertion([key_pair.private_key])).to be_invalid
     end
 
-    it 'is invalid when the signature is invalid' do
+    it 'is valid when the encrypted signature is valid' do
+      key_pair = Xml::Kit::KeyPair.generate(use: :encryption)
+      response = Saml::Kit::Response.build(user, request, configuration: configuration) do |x|
+        x.encrypt_with(key_pair)
+      end
+      expect(response.assertion([key_pair.private_key])).to be_valid
+    end
+
+    it 'is invalid when the assertion signature is invalid' do
       xml = Saml::Kit::Response.build_xml(user, request, configuration: configuration)
-      altered = xml.gsub(entity_id, 'altered')
+      altered = xml.gsub(name_id, 'altered')
       response = Saml::Kit::Response.new(altered, configuration: configuration)
       expect(response.assertion).to be_invalid
+      expect(response.assertion.errors[:digest_value]).to match_array(['is invalid.'])
+    end
+
+    it 'is invalid when the response signature is invalid' do
+      xml = Saml::Kit::Response.build_xml(user, request, configuration: configuration)
+      altered = xml.gsub('StatusCode', 'ALTERED')
+      response = Saml::Kit::Response.new(altered, configuration: configuration)
+      expect(response).to be_invalid
     end
 
     it 'is valid' do
-      response = Saml::Kit::Response.build(user, request, configuration: configuration) do |x|
-        x.sign_with(Xml::Kit::KeyPair.generate(use: :signing))
-      end
+      response = Saml::Kit::Response.build(user, request, configuration: configuration)
       expect(response.assertion).to be_valid
     end
   end
spec/saml/signature_spec.rb
@@ -1,13 +1,13 @@
 RSpec.describe Saml::Kit::Signature do
-  describe "#valid?" do
-    let(:key_pair) { ::Xml::Kit::KeyPair.generate(use: :signing) }
-    let(:signed_document) do
-      Saml::Kit::AuthenticationRequest.build do |x|
-        x.sign_with(key_pair)
-      end
+  let(:key_pair) { ::Xml::Kit::KeyPair.generate(use: :signing) }
+  let(:signed_document) do
+    Saml::Kit::AuthenticationRequest.build do |x|
+      x.sign_with(key_pair)
     end
-    subject { described_class.new(signed_document.at_xpath('//ds:Signature')) }
+  end
+  subject { described_class.new(signed_document.at_xpath('//ds:Signature')) }
 
+  describe "#valid?" do
     it 'returns true when the signature is valid' do
       expect(subject).to be_valid
     end
@@ -60,11 +60,25 @@ RSpec.describe Saml::Kit::Signature do
         end
       end
     end
+  end
+
+  describe "#to_h" do
+    it 'returns a hash representation of the signature' do
+      expected = Hash.from_xml(signed_document.to_s)['AuthnRequest']['Signature']
+      expect(subject.to_h).to eql(expected)
+    end
+  end
+
+  describe "#present?" do
+    context "when a signature is not present" do
+      it 'return false' do
+        expect(described_class.new(nil)).to_not be_present
+      end
+    end
 
-    describe "#to_h" do
-      it 'returns a hash representation of the signature' do
-        expected = Hash.from_xml(signed_document.to_s)['AuthnRequest']['Signature']
-        expect(subject.to_h).to eql(expected)
+    context "when a signature is present" do
+      it 'returns true' do
+        expect(subject).to be_present
       end
     end
   end
saml-kit.gemspec
@@ -24,7 +24,7 @@ Gem::Specification.new do |spec|
   spec.require_paths = ["lib"]
 
   spec.add_dependency "activemodel", ">= 4.2.0"
-  spec.add_dependency "xml-kit", ">= 0.1.8", "<= 1.0.0"
+  spec.add_dependency "xml-kit", ">= 0.1.9", "<= 1.0.0"
   spec.add_development_dependency "bundler", "~> 1.15"
   spec.add_development_dependency "ffaker", "~> 2.7"
   spec.add_development_dependency "rake", "~> 10.0"