diff --git a/lib/saml_idp.rb b/lib/saml_idp.rb index a1d8256f..a9c559f5 100644 --- a/lib/saml_idp.rb +++ b/lib/saml_idp.rb @@ -75,9 +75,8 @@ def options_have_signature(options) private :options_have_signature def valid_signature?(fingerprint, options = {}) - soft_fail = options[:soft].nil? ? true : options[:soft] (signed? || options_have_signature(options)) && - signed_document.validate(fingerprint, soft_fail, options) + signed_document.validate(fingerprint, soft = true, options) end def signed_document diff --git a/lib/saml_idp/request.rb b/lib/saml_idp/request.rb index 52a48e7d..10e0df1d 100644 --- a/lib/saml_idp/request.rb +++ b/lib/saml_idp/request.rb @@ -152,11 +152,9 @@ def valid? if service_provider? begin - unless valid_signature? - log "Signature is invalid in #{raw_xml}" - errors.push(:invalid_signature) - end + valid_signature? rescue SamlIdp::XMLSecurity::SignedDocument::ValidationError => e + log e.message errors.push(e.error_code) end end @@ -171,7 +169,7 @@ def signed? def valid_signature? # Force signatures for logout requests because there is no other # protection against a cross-site DoS. - service_provider.valid_signature?(document, logout_request?, options.merge(soft: false)) + service_provider.valid_signature?(document, logout_request?, options) end def service_provider? diff --git a/lib/saml_idp/service_provider.rb b/lib/saml_idp/service_provider.rb index be7cdb55..83b0b0f3 100644 --- a/lib/saml_idp/service_provider.rb +++ b/lib/saml_idp/service_provider.rb @@ -21,23 +21,33 @@ def valid? end def valid_signature?(doc, require_signature = false, options = {}) - @matching_cert = Array(certs).find do |cert| - doc.valid_signature?(fingerprint_cert(cert), options.merge(cert: cert)) - end + return true unless require_signature || should_validate_signature? - no_raising_errors = options[:soft].nil? ? true : options[:soft] + if cert_array.empty? + raise SamlIdp::XMLSecurity::SignedDocument::ValidationError.new( + 'No cert', + :no_cert_registered + ) + end - if require_signature || should_validate_signature? - if certs.blank? && !no_raising_errors - raise SamlIdp::XMLSecurity::SignedDocument::ValidationError.new( - 'No certificate registered', - :no_cert_registered + @matching_cert = cert_array.find do |cert| + doc.valid_signature?(fingerprint_cert(cert), options.merge( + { + cert: cert, + # we only want to raise request errors, not individual certificate errors + raise_request_errors: true} ) - end - !!@matching_cert - else - true + ) + end + + if @matching_cert.nil? + # if no ValidationErrors, but no matching certs, the request was signed with an + # unregistered certificate + raise SamlIdp::XMLSecurity::SignedDocument::ValidationError.new( + 'No matching cert', + :no_matching_cert) end + true end # @param [OpenSSL::X509::Certificate] ssl_cert @@ -90,5 +100,11 @@ def request_metadata metadata_url.present? ? Faraday.get(metadata_url).body : "" end private :request_metadata + + private + + def cert_array + Array(certs) + end end end diff --git a/lib/saml_idp/xml_security.rb b/lib/saml_idp/xml_security.rb index 6ab2dfcb..07e9a2f6 100644 --- a/lib/saml_idp/xml_security.rb +++ b/lib/saml_idp/xml_security.rb @@ -45,6 +45,7 @@ def initialize(msg=nil, error_code=nil) DSIG = "http://www.w3.org/2000/09/xmldsig#" attr_accessor :signed_element_id + attr_accessor :raise_request_errors def initialize(response) super(response) @@ -52,6 +53,8 @@ def initialize(response) end def validate(idp_cert_fingerprint, soft = true, options = {}) + @raise_request_errors = options[:raise_request_errors] + log 'Validate the fingerprint' base64_cert = find_base64_cert(options) cert_text = Base64.decode64(base64_cert) @@ -132,8 +135,7 @@ def find_base64_cert(options) else raise ValidationError.new( 'Certificate element missing in response (ds:X509Certificate) and not provided in options[:cert]', - :cert_missing - ) + :cert_missing) end end @@ -201,10 +203,13 @@ def validate_doc_embedded_signature(base64_cert, soft = true) digest_value = Base64.decode64(REXML::XPath.first(ref, "//ds:DigestValue", sig_namespace_hash).text) unless digests_match?(hash, digest_value) - return soft ? false : (raise ValidationError.new( - "Digest mismatch", - :digest_mismatch - )) + if @raise_request_errors || !soft + raise ValidationError.new( + "Digest mismatch", + :digest_mismatch + ) + end + return false end end @@ -221,10 +226,10 @@ def verify_signature(base64_cert, sig_alg, signature, canon_string, soft) cert = OpenSSL::X509::Certificate.new(cert_text) signature_algorithm = algorithm(sig_alg) - if signature_algorithm != OpenSSL::Digest::SHA256 && !soft - raise ValidationError.new( - 'All signatures must use RSA SHA-256', - :require_sha256 + if signature_algorithm != OpenSSL::Digest::SHA256 && @raise_request_errors + raise ValidationError.new( + 'All signatures must use RSA SHA-256', + :require_sha256 ) end diff --git a/spec/lib/saml_idp/request_spec.rb b/spec/lib/saml_idp/request_spec.rb index 069611ee..6e1ada55 100644 --- a/spec/lib/saml_idp/request_spec.rb +++ b/spec/lib/saml_idp/request_spec.rb @@ -364,11 +364,6 @@ module SamlIdp it 'is not valid' do expect(subject.valid?).to eq false end - - it 'adds an error to request object' do - subject.valid? - expect(subject.errors.include?(:invalid_certificate)).to be true - end end describe 'none of the service provider certs match the signed document' do @@ -376,11 +371,6 @@ module SamlIdp it 'is not valid' do expect(subject.valid?).to eq false end - - it 'adds a key validation error to request object' do - subject.valid? - expect(subject.errors.include?(:key_validation_error)).to be true - end end describe 'service provider has no certificate registered' do @@ -398,12 +388,7 @@ module SamlIdp describe 'fingerprint mismatch' do before do - allow(subject.service_provider).to receive(:valid_signature?).and_raise( - SamlIdp::XMLSecurity::SignedDocument::ValidationError.new( - 'Fingerprint mismatch', - :fingerprint_mismatch - ) - ) + expect(Base64).to receive(:decode64).with(expected_cert) { cloudhsm_idp_x509_cert } end it 'is not valid' do @@ -412,7 +397,7 @@ module SamlIdp it 'adds a fingerprint mismatch error' do subject.valid? - expect(subject.errors.include?(:fingerprint_mismatch)).to be true + expect(subject.errors.include?(:no_matching_cert)).to be true end end @@ -461,13 +446,10 @@ module SamlIdp end describe 'digest mismatch' do + let(:options) { {} } + let(:request_saml) { signed_auth_request } before do - allow(subject.service_provider).to receive(:valid_signature?).and_raise( - SamlIdp::XMLSecurity::SignedDocument::ValidationError.new( - 'Digest mismatch', - :digest_mismatch - ) - ) + allow(OpenSSL::Digest::SHA256).to receive(:digest) { 'incorrect_hash' } end it 'is not valid' do diff --git a/spec/lib/saml_idp/service_provider_spec.rb b/spec/lib/saml_idp/service_provider_spec.rb index 0a012e75..a89b09e0 100644 --- a/spec/lib/saml_idp/service_provider_spec.rb +++ b/spec/lib/saml_idp/service_provider_spec.rb @@ -48,16 +48,22 @@ module SamlIdp before { subject.validate_signature = true } describe 'a cert is not present in the document' do + let(:raise_request_errors) { nil } let(:raw_xml) do SamlIdp::Request.from_deflated_request( signed_auth_request_options['SAMLRequest'] ).raw_xml end - let(:options) { {get_params: signed_auth_request_options}.with_indifferent_access } + let(:options) do + { + get_params: signed_auth_request_options, + raise_request_errors: raise_request_errors + }.with_indifferent_access + end describe 'the service provider has no certs' do - it 'returns false' do - expect(subject.valid_signature?(doc, true, options)).to be false + it 'raises an error' do + expect { subject.valid_signature?(doc, true, options) }.to raise_error(SamlIdp::XMLSecurity::SignedDocument::ValidationError, 'No cert') end end @@ -72,17 +78,18 @@ module SamlIdp describe 'one invalid cert' do before { subject.certs = [invalid_cert]} - - it 'returns false' do - expect(subject.valid_signature?(doc, true, options)).to be false + it 'raises an error' do + expect { subject.valid_signature?(doc, true, options) }.to raise_error(SamlIdp::XMLSecurity::SignedDocument::ValidationError, 'No matching cert') end end describe 'multiple certs' do before { subject.certs = [invalid_cert, cert] } - it 'returns true' do - expect(subject.valid_signature?(doc, true, options)).to be true + describe "one is valid" do + it 'returns true' do + expect(subject.valid_signature?(doc, true, options)).to be true + end end end end @@ -96,14 +103,8 @@ module SamlIdp end describe 'the service provider has no certs' do - it 'returns false' do - expect(subject.valid_signature?(doc, true)).to be false - end - - describe 'the requirement is passed through the method' do - it 'returns false' do - expect(subject.valid_signature?(doc, true)).to be false - end + it 'raises an error' do + expect { subject.valid_signature?(doc, true, options) }.to raise_error(SamlIdp::XMLSecurity::SignedDocument::ValidationError, 'No cert') end end @@ -119,8 +120,8 @@ module SamlIdp describe 'one invalid cert' do before { subject.certs = [invalid_cert]} - it 'returns false' do - expect(subject.valid_signature?(doc, true)).to be false + it 'raises an error' do + expect { subject.valid_signature?(doc, true, options) }.to raise_error(SamlIdp::XMLSecurity::SignedDocument::ValidationError, 'No matching cert') end end @@ -145,13 +146,8 @@ module SamlIdp describe 'the valid cert is not registered in the idp' do before { subject.certs = [other_cert, invalid_cert] } - it 'returns false' do - expect(subject.valid_signature?(doc, true)).to be false - end - - it 'matches the right cert' do - subject.valid_signature?(doc) - expect(subject.matching_cert).to be nil + it 'raises an error' do + expect { subject.valid_signature?(doc, true, options) }.to raise_error(SamlIdp::XMLSecurity::SignedDocument::ValidationError, 'No matching cert') end end end diff --git a/spec/xml_security_spec.rb b/spec/xml_security_spec.rb index d614a985..d4e96095 100644 --- a/spec/xml_security_spec.rb +++ b/spec/xml_security_spec.rb @@ -41,15 +41,19 @@ module SamlIdp ) end - it 'raises require SHA256 error' do - response = Base64.decode64(response_document) - response.sub!('pJQ7MS/ek4KRRWGmv/H43ReHYMs=', - 'b9xsAXLsynugg3Wc1CI3kpWku+0=') - document = XMLSecurity::SignedDocument.new(response) - base64cert = document.elements['//ds:X509Certificate'].text - expect { document.validate_doc(base64cert, false) }.to( - raise_error(SamlIdp::XMLSecurity::SignedDocument::ValidationError, 'All signatures must use RSA SHA-256') - ) + describe 'when raising request errors' do + it 'raises require SHA256 error' do + response = Base64.decode64(response_document) + response.sub!('pJQ7MS/ek4KRRWGmv/H43ReHYMs=', + 'b9xsAXLsynugg3Wc1CI3kpWku+0=') + document = XMLSecurity::SignedDocument.new(response) + base64cert = document.elements['//ds:X509Certificate'].text + + document.raise_request_errors = true + expect { document.validate_doc(base64cert, false, {raise_request_errors: true}) }.to( + raise_error(SamlIdp::XMLSecurity::SignedDocument::ValidationError, 'All signatures must use RSA SHA-256') + ) + end end it 'raises Key validation error' do