diff --git a/lib/saml_idp.rb b/lib/saml_idp.rb
index abd40d52..a48e760c 100644
--- a/lib/saml_idp.rb
+++ b/lib/saml_idp.rb
@@ -76,7 +76,7 @@ def options_have_signature(options)
def valid_signature?(fingerprint, options = {})
(signed? || options_have_signature(options)) &&
- signed_document.validate(fingerprint, :soft, options)
+ signed_document.validate(fingerprint, true, options)
end
def signed_document
diff --git a/lib/saml_idp/request.rb b/lib/saml_idp/request.rb
index 291cee9c..10e0df1d 100644
--- a/lib/saml_idp/request.rb
+++ b/lib/saml_idp/request.rb
@@ -132,7 +132,7 @@ def valid?
unless service_provider?
log "Unable to find service provider for issuer #{issuer}"
- errors.push(:issuer_missing_or_invald)
+ errors.push(:issuer_missing_or_invalid)
end
if authn_request? && logout_request?
@@ -141,7 +141,7 @@ def valid?
end
unless authn_request? || logout_request?
- log "One and only one of authnrequest and logout request is required. authnrequest: #{authn_request?} logout_request: #{logout_request?} "
+ log 'One and only one of authnrequest and logout request is required. '
errors.push(:no_auth_or_logout_request)
end
@@ -150,10 +150,13 @@ def valid?
errors.push(:no_response_url)
end
- unless service_provider? && valid_signature?
- log "Signature is invalid in #{raw_xml}"
- # TODO: We should get more specific errors
- errors.push(:invalid_signature)
+ if service_provider?
+ begin
+ valid_signature?
+ rescue SamlIdp::XMLSecurity::SignedDocument::ValidationError => e
+ log e.message
+ errors.push(e.error_code)
+ end
end
errors.blank?
diff --git a/lib/saml_idp/service_provider.rb b/lib/saml_idp/service_provider.rb
index 56dff010..83b0b0f3 100644
--- a/lib/saml_idp/service_provider.rb
+++ b/lib/saml_idp/service_provider.rb
@@ -21,17 +21,33 @@ def valid?
end
def valid_signature?(doc, require_signature = false, options = {})
- Array(certs).any? do |cert|
- if doc.valid_signature?(fingerprint_cert(cert), options.merge(cert: cert))
- @matching_cert = cert
- end
+ return true unless require_signature || should_validate_signature?
+
+ if cert_array.empty?
+ raise SamlIdp::XMLSecurity::SignedDocument::ValidationError.new(
+ 'No cert',
+ :no_cert_registered
+ )
+ end
+
+ @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
- if require_signature || should_validate_signature?
- !!@matching_cert
- else
- true
+ 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
@@ -84,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 533aab0f..07e9a2f6 100644
--- a/lib/saml_idp/xml_security.rb
+++ b/lib/saml_idp/xml_security.rb
@@ -32,11 +32,20 @@
module SamlIdp
module XMLSecurity
class SignedDocument < REXML::Document
- ValidationError = Class.new(StandardError)
+ class ValidationError < StandardError
+ attr_reader :error_code
+
+ def initialize(msg=nil, error_code=nil)
+ @error_code = error_code
+ super(msg)
+ end
+ end
+
C14N = "http://www.w3.org/2001/10/xml-exc-c14n#"
DSIG = "http://www.w3.org/2000/09/xmldsig#"
attr_accessor :signed_element_id
+ attr_accessor :raise_request_errors
def initialize(response)
super(response)
@@ -44,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)
@@ -51,7 +62,10 @@ def validate(idp_cert_fingerprint, soft = true, options = {})
begin
OpenSSL::X509::Certificate.new(cert_text)
rescue OpenSSL::X509::CertificateError => e
- return soft ? false : (raise ValidationError.new("Invalid certificate"))
+ return soft ? false : (raise ValidationError.new(
+ 'Invalid certificate',
+ :invalid_certificate
+ ))
end
# check cert matches registered idp cert
@@ -60,7 +74,10 @@ def validate(idp_cert_fingerprint, soft = true, options = {})
plain_idp_cert_fingerprint = idp_cert_fingerprint.gsub(/[^a-zA-Z0-9]/,"").downcase
if fingerprint != plain_idp_cert_fingerprint && sha1_fingerprint != plain_idp_cert_fingerprint
- return soft ? false : (raise ValidationError.new("Fingerprint mismatch"))
+ return soft ? false : (raise ValidationError.new(
+ 'Fingerprint mismatch',
+ :fingerprint_mismatch
+ ))
end
validate_doc(base64_cert, soft, options)
@@ -100,17 +117,25 @@ def find_base64_cert(options)
if cert_element
return cert_element.text unless cert_element.text.blank?
- raise ValidationError.new("Certificate element present in response (ds:X509Certificate) but evaluating to nil")
+ raise ValidationError.new(
+ "Certificate element present in response (ds:X509Certificate) but evaluating to nil",
+ :present_but_nil
+ )
elsif options[:cert]
if options[:cert].is_a?(String)
options[:cert]
elsif options[:cert].is_a?(OpenSSL::X509::Certificate)
Base64.encode64(options[:cert].to_pem)
else
- raise ValidationError.new("options[:cert] must be Base64-encoded String or OpenSSL::X509::Certificate")
+ raise ValidationError.new(
+ 'options[:cert] must be Base64-encoded String or OpenSSL::X509::Certificate',
+ :not_base64_or_cert
+ )
end
else
- raise ValidationError.new("Certificate element missing in response (ds:X509Certificate) and not provided in options[:cert]")
+ raise ValidationError.new(
+ 'Certificate element missing in response (ds:X509Certificate) and not provided in options[:cert]',
+ :cert_missing)
end
end
@@ -178,7 +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"))
+ if @raise_request_errors || !soft
+ raise ValidationError.new(
+ "Digest mismatch",
+ :digest_mismatch
+ )
+ end
+ return false
end
end
@@ -195,8 +226,18 @@ 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 && @raise_request_errors
+ raise ValidationError.new(
+ 'All signatures must use RSA SHA-256',
+ :require_sha256
+ )
+ end
+
unless cert.public_key.verify(signature_algorithm.new, signature, canon_string)
- return soft ? false : (raise ValidationError.new("Key validation error"))
+ return soft ? false : (raise ValidationError.new(
+ 'Key validation error',
+ :key_validation_error
+ ))
end
return true
diff --git a/spec/lib/saml_idp/request_spec.rb b/spec/lib/saml_idp/request_spec.rb
index 8691b021..6e1ada55 100644
--- a/spec/lib/saml_idp/request_spec.rb
+++ b/spec/lib/saml_idp/request_spec.rb
@@ -4,145 +4,146 @@ module SamlIdp
let(:aal) { 'http://idmanagement.gov/ns/assurance/aal/3' }
let(:default_aal) { 'urn:gov:gsa:ac:classes:sp:PasswordProtectedTransport:duo' }
let(:ial) { 'http://idmanagement.gov/ns/assurance/ial/2' }
- let(:password) { 'urn:oasis:names:tc:SAML:2.0:ac:classes:Password' }
- let(:authn_context_classref) { build_authn_context_classref(password) }
- let(:issuer) { 'localhost:3000' }
- let(:raw_authn_request) { "#{issuer}#{authn_context_classref}" }
-
- let(:raw_authn_unspecified_name_id_format) { "#{issuer}urn:oasis:names:tc:SAML:2.0:ac:classes:Password" }
-
- let(:raw_authn_forceauthn_present) { "#{issuer}urn:oasis:names:tc:SAML:2.0:ac:classes:Password" }
-
- let(:raw_authn_forceauthn_false) { "#{issuer}urn:oasis:names:tc:SAML:2.0:ac:classes:Password" }
-
- let(:raw_authn_enveloped_signature) { "localhost:3000urn:oasis:names:tc:SAML:2.0:ac:classes:Password" }
-
- let(:raw_logout_request) { "http://example.comsome_name_idabc123index" }
+ let(:options) { {} }
+ let(:deflated_request) { make_saml_request }
+ subject { described_class.from_deflated_request deflated_request, options }
describe 'deflated request' do
- let(:deflated_request) { Base64.encode64(Zlib::Deflate.deflate(raw_authn_request, 9)[2..-5]) }
-
- subject { described_class.from_deflated_request deflated_request }
-
it 'inflates' do
- expect(subject.request_id).to eq('_af43d1a0-e111-0130-661a-3c0754403fdb')
+ expect(subject.issuer).to eq(saml_settings.issuer)
end
- it 'handles invalid SAML' do
- req = described_class.from_deflated_request 'bang!'
- expect(req.valid?).to eq(false)
+ describe 'invalid SAML' do
+ let(:deflated_request) { 'bang!' }
+ it 'does not set attributes' do
+ expect(subject.issuer).to be nil
+ end
end
- end
- describe 'authn request' do
- subject { described_class.new raw_authn_request }
+ describe 'no request passed in' do
+ let(:deflated_request) { nil }
- it 'has a valid request_id' do
- expect(subject.request_id).to eq('_af43d1a0-e111-0130-661a-3c0754403fdb')
- end
-
- it 'has a valid acs_url' do
- expect(subject.acs_url).to eq('http://localhost:3000/saml/consume')
+ it 'sets raw_xml to an empty string' do
+ expect(subject.raw_xml).to eq ''
+ end
end
- it 'has a valid service_provider' do
- expect(subject.service_provider).to be_a ServiceProvider
- end
+ describe 'authn request methods' do
+ it 'has a valid acs_url' do
+ expect(subject.acs_url).to eq(saml_settings.assertion_consumer_service_url)
+ end
- it 'has a valid service_provider' do
- expect(subject.service_provider).to be_truthy
- end
+ it 'has a valid service_provider' do
+ expect(subject.service_provider).to be_a ServiceProvider
+ end
- it 'has a valid issuer' do
- expect(subject.issuer).to eq('localhost:3000')
- end
+ it 'has a valid service_provider' do
+ expect(subject.service_provider.valid?).to be true
+ end
- it 'has a valid valid_signature' do
- expect(subject.valid_signature?).to be_truthy
- end
+ it 'has a valid issuer' do
+ expect(subject.issuer).to eq(saml_settings.issuer)
+ end
- it "correctly indicates that it isn't signed" do
- expect(subject.signed?).to be_falsey
- end
+ it 'has a valid valid_signature' do
+ expect(subject.valid_signature?).to be true
+ end
- context 'with signature in params' do
- subject do
- described_class.new(raw_authn_request, get_params: { Signature: 'abc' })
+ it 'correctly indicates that it is not signed' do
+ expect(subject.signed?).to be false
end
- it 'correctly indicates that it is signed (even invalidly)' do
- expect(subject.signed?).to be_truthy
+ context 'with signature in params' do
+ let(:deflated_request) { signed_auth_request(embed: false) }
+ let(:options) do
+ { get_params: signed_auth_request_options.with_indifferent_access }
+ end
+
+ it 'correctly indicates that it is signed (even invalidly)' do
+ expect(subject.signed?).to be true
+ end
end
- end
- context 'with an enveloped signature' do
- subject { described_class.new raw_authn_enveloped_signature }
+ context 'with an enveloped signature' do
+ let(:deflated_request) { signed_auth_request(embed: true) }
- it 'correctly indicates that it is signed (even invalidly)' do
- expect(subject.signed?).to be_truthy
+ it 'correctly indicates that it is signed (even invalidly)' do
+ expect(subject.signed?).to be true
+ end
end
- end
- it 'should return acs_url for response_url' do
- expect(subject.response_url).to eq(subject.acs_url)
- end
+ it 'should return acs_url for response_url' do
+ expect(subject.response_url).to eq(subject.acs_url)
+ end
- it 'is a authn request' do
- expect(subject.authn_request?).to eq(true)
- end
+ it 'is a authn request' do
+ expect(subject.authn_request?).to eq(true)
+ end
- it 'fetches internal request' do
- expect(subject.request['ID']).to eq(subject.request_id)
- end
+ it 'fetches internal request' do
+ expect(subject.request['ID']).to eq(subject.request_id)
+ end
- it 'has a valid name id format' do
- expect(subject.name_id_format).to eq('urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress')
- end
+ it 'has a valid name id format' do
+ expect(subject.name_id_format).to eq(saml_settings.name_identifier_format)
+ end
- it 'has a valid requested authn context comparison' do
- expect(subject.requested_authn_context_comparison).to eq('exact')
- end
+ it 'has a valid requested authn context comparison' do
+ expect(subject.requested_authn_context_comparison).to eq(saml_settings.authn_context_comparison)
+ end
- it 'has a valid authn context' do
- expect(subject.requested_authn_context).to eq('urn:oasis:names:tc:SAML:2.0:ac:classes:Password')
- end
+ it 'has a valid authn context' do
+ expect(subject.requested_authn_context).to eq(saml_settings.authn_context)
+ end
- context 'empty issuer' do
- let(:issuer) { nil }
+ context 'empty issuer' do
+ let(:deflated_request) { make_invalid_saml_request}
- it 'does not permit empty issuer' do
- expect(subject.issuer).not_to eq('')
- expect(subject.issuer).to eq(nil)
+ it 'does not permit empty issuer' do
+ expect(subject.issuer).not_to eq('')
+ expect(subject.issuer).to eq(nil)
+ end
end
- end
- it 'defaults to force_authn = false' do
- expect(subject.force_authn?).to be_falsey
- end
+ describe 'force_authn?' do
+ describe 'it is not set' do
+ it 'defaults to false' do
+ expect(subject.force_authn?).to be false
+ end
+ end
- it 'properly parses ForceAuthn="true" if passed' do
- authn_request = described_class.new raw_authn_forceauthn_present
+ describe 'ForceAuthn is set' do
+ let(:force_authn) { true }
+ let(:deflated_request) { make_saml_request_with_options({force_authn: force_authn}) }
- expect(authn_request.force_authn?).to be_truthy
- end
+ it 'returns true' do
+ expect(subject.force_authn?).to be true
+ end
- it 'properly parses ForceAuthn="false" if passed' do
- authn_request = described_class.new raw_authn_forceauthn_false
+ describe 'it is set to false' do
+ let(:force_authn) { false }
- expect(authn_request.force_authn?).to be_falsey
- end
+ it 'returns false' do
+ expect(subject.force_authn?).to be false
+ end
+ end
+ end
+ end
- describe 'unspecified name id format' do
- subject { described_class.new raw_authn_unspecified_name_id_format }
+ describe 'unspecified name id format' do
+ let(:deflated_request) { make_saml_request_with_options({name_identifier_format: nil}) }
- it 'returns nil for name id format' do
- expect(subject.name_id_format).to eq(nil)
+ it 'returns nil for name id format' do
+ expect(subject.name_id_format).to eq(nil)
+ end
end
end
end
describe 'logout request' do
- subject { described_class.new raw_logout_request }
+ let(:deflated_request) { make_saml_logout_request }
+
+ subject { described_class.from_deflated_request deflated_request }
it 'has a valid request_id' do
expect(subject.request_id).to eq('_some_response_id')
@@ -157,7 +158,7 @@ module SamlIdp
end
it 'should have a session index' do
- expect(subject.session_index).to eq('abc123index')
+ expect(subject.session_index).to eq('_some_response_id')
end
it 'should have a valid issuer' do
@@ -174,7 +175,9 @@ module SamlIdp
end
describe '#requested_aal_authn_context' do
- subject { described_class.new raw_authn_request }
+ let(:authn_context_classref) { '' }
+ let(:deflated_request) { make_saml_request_with_options({authn_context: authn_context_classref}) }
+ subject { described_class.from_deflated_request deflated_request }
context 'no aal context requested' do
let(:authn_context_classref) { '' }
@@ -185,7 +188,7 @@ module SamlIdp
end
context 'context requested is default aal' do
- let(:authn_context_classref) { build_authn_context_classref(default_aal) }
+ let(:authn_context_classref) { default_aal }
it 'should return the aal uri' do
expect(subject.requested_aal_authn_context).to eq(default_aal)
@@ -193,7 +196,7 @@ module SamlIdp
end
context 'only context requested is aal' do
- let(:authn_context_classref) { build_authn_context_classref(aal) }
+ let(:authn_context_classref) { aal }
it 'should return the aal uri' do
expect(subject.requested_aal_authn_context).to eq(aal)
@@ -201,7 +204,7 @@ module SamlIdp
end
context 'multiple contexts requested including aal' do
- let(:authn_context_classref) { build_authn_context_classref([ial, aal]) }
+ let(:authn_context_classref) { [ial, aal] }
it 'should return the aal uri' do
expect(subject.requested_aal_authn_context).to eq(aal)
@@ -210,7 +213,9 @@ module SamlIdp
end
describe '#requested_ial_authn_context' do
- subject { described_class.new raw_authn_request }
+ let(:authn_context_classref) { '' }
+ let(:deflated_request) { make_saml_request_with_options({authn_context: authn_context_classref}) }
+ subject { described_class.from_deflated_request deflated_request }
context 'no ial context requested' do
let(:authn_context_classref) { '' }
@@ -221,7 +226,7 @@ module SamlIdp
end
context 'only context requested is ial' do
- let(:authn_context_classref) { build_authn_context_classref(ial) }
+ let(:authn_context_classref) { ial }
it 'should return the ial uri' do
expect(subject.requested_ial_authn_context).to eq(ial)
@@ -229,7 +234,7 @@ module SamlIdp
end
context 'multiple contexts requested including ial' do
- let(:authn_context_classref) { build_authn_context_classref([aal, ial]) }
+ let(:authn_context_classref) { [aal, ial] }
it 'should return the ial uri' do
expect(subject.requested_ial_authn_context).to eq(ial)
@@ -238,8 +243,8 @@ module SamlIdp
end
describe '#valid?' do
- let(:request_saml) { raw_authn_request }
- subject { described_class.new request_saml }
+ let(:request_saml) { make_saml_request }
+ subject { described_class.from_deflated_request(request_saml, options) }
context 'a valid request' do
it 'returns true' do
@@ -253,7 +258,7 @@ module SamlIdp
context 'an invalid request' do
describe 'a request with no issuer' do
- let(:issuer) { nil }
+ let(:request_saml) { make_invalid_saml_request }
it 'is not valid' do
expect(subject.valid?).to eq(false)
@@ -261,14 +266,15 @@ module SamlIdp
it 'adds an error to the request object' do
subject.valid?
- expect(subject.errors.first).to eq :issuer_missing_or_invald
+ expect(subject.errors.first).to eq :issuer_missing_or_invalid
end
end
describe 'no authn_request OR logout_request tag' do
let(:request_saml) do
- "localhost:3000#{authn_context_classref}"
+ "localhost:3000http://idmanagement.gov/ns/assurance/aal/3"
end
+ subject { described_class.new(request_saml, options) }
it 'is not valid' do
expect(subject.valid?).to eq false
@@ -281,10 +287,11 @@ module SamlIdp
end
describe 'both an authn_request AND logout_request tag' do
+ let(:request_saml) { make_saml_request }
let(:logout_saml) { "" }
- let(:request_saml) do
- logout_saml + raw_authn_request + ''
+ before do
+ subject.raw_xml = logout_saml + subject.raw_xml + ''
end
it 'is not valid' do
@@ -299,9 +306,7 @@ module SamlIdp
describe 'there is no response url' do
describe 'authn_request' do
- let(:request_saml) do
- raw_authn_request.gsub("AssertionConsumerServiceURL='http://localhost:3000/saml/consume'", '')
- end
+ let(:request_saml) { make_saml_request_with_options({assertion_consumer_service_url: ''}) }
it 'is not valid' do
expect(subject.valid?).to eq false
@@ -314,7 +319,7 @@ module SamlIdp
end
describe 'logout_request' do
- let(:request_saml) { raw_logout_request }
+ let(:request_saml) { make_saml_logout_request }
before do
subject.service_provider.assertion_consumer_logout_service_url = nil
end
@@ -331,27 +336,148 @@ module SamlIdp
end
describe 'invalid signature' do
- subject do
- # the easiest way to "force" a signature check is to make it a logout request
- described_class.new(raw_logout_request, get_params: { Signature: 'abc' })
+ let(:options) { { get_params: signed_auth_request_options.with_indifferent_access } }
+ let(:cert) { saml_settings.certificate }
+ let(:registered_cert) do
+ OpenSSL::X509::Certificate.new(
+ "-----BEGIN CERTIFICATE-----\n" +
+ cert +
+ "\n-----END CERTIFICATE-----"
+ )
end
+ let(:expected_cert) { Base64.encode64(registered_cert.to_pem) }
- it 'is not valid' do
- expect(subject.valid?).to eq false
+ let(:request_saml) { signed_auth_request_options['SAMLRequest'] }
+
+ before do
+ # force signature validation
+ subject.service_provider.validate_signature = true
+ subject.service_provider.certs = [registered_cert]
end
- it 'adds an error to request object' do
- subject.valid?
- expect(subject.errors.include?(:invalid_signature)).to be true
+ describe 'specific errors' do
+ describe 'invalid certificate' do
+ before do
+ expect(Base64).to receive(:decode64).with(expected_cert) { 'invalid certificate' }
+ end
+
+ it 'is not valid' do
+ expect(subject.valid?).to eq false
+ end
+ end
+
+ describe 'none of the service provider certs match the signed document' do
+ let(:registered_cert) { OpenSSL::X509::Certificate.new(cloudhsm_idp_x509_cert) }
+ it 'is not valid' do
+ expect(subject.valid?).to eq false
+ end
+ end
+
+ describe 'service provider has no certificate registered' do
+ before { subject.service_provider.certs = [] }
+
+ it 'is not valid' do
+ expect(subject.valid?).to eq false
+ end
+
+ it 'adds a no_cert_registered error to request object' do
+ subject.valid?
+ expect(subject.errors.include?(:no_cert_registered)).to be true
+ end
+ end
+
+ describe 'fingerprint mismatch' do
+ before do
+ expect(Base64).to receive(:decode64).with(expected_cert) { cloudhsm_idp_x509_cert }
+ end
+
+ it 'is not valid' do
+ expect(subject.valid?).to eq false
+ end
+
+ it 'adds a fingerprint mismatch error' do
+ subject.valid?
+ expect(subject.errors.include?(:no_matching_cert)).to be true
+ end
+ end
+
+ describe 'present but nil cert tag in request' do
+ # TODO: This could happen if a cert tag was present in the request document
+ # and the service_provider had a cert registered. should we
+ # refactor the code to ignore the certificate in the request document?
+ before do
+ allow(subject.service_provider).to receive(:valid_signature?).and_raise(
+ SamlIdp::XMLSecurity::SignedDocument::ValidationError.new(
+ 'Certificate element present in response (ds:X509Certificate) but evaluating to nil',
+ :present_but_nil
+ )
+ )
+ end
+
+ it 'is not valid' do
+ expect(subject.valid?).to eq false
+ end
+
+ it 'adds a present_but_nil_cert error' do
+ subject.valid?
+ expect(subject.errors.include?(:present_but_nil)).to be true
+ end
+ end
+
+ describe 'cert is not a cert' do
+ # TODO: Not sure if this is possible based on our setup.
+ before do
+ allow(subject.service_provider).to receive(:valid_signature?).and_raise(
+ SamlIdp::XMLSecurity::SignedDocument::ValidationError.new(
+ 'options[:cert] must be Base64-encoded String or OpenSSL::X509::Certificate',
+ :not_base64_or_cert
+ )
+ )
+ end
+
+ it 'is not valid' do
+ expect(subject.valid?).to eq false
+ end
+
+ it 'adds a not_base64_or_cert error' do
+ subject.valid?
+ expect(subject.errors.include?(:not_base64_or_cert)).to be true
+ end
+ end
+
+ describe 'digest mismatch' do
+ let(:options) { {} }
+ let(:request_saml) { signed_auth_request }
+ before do
+ allow(OpenSSL::Digest::SHA256).to receive(:digest) { 'incorrect_hash' }
+ end
+
+ it 'is not valid' do
+ expect(subject.valid?).to eq false
+ end
+
+ it 'adds a digest mismatch error' do
+ subject.valid?
+ expect(subject.errors.include?(:digest_mismatch)).to be true
+ end
+ end
+
+ describe 'request not using SHA256 hashing algorithm' do
+ let(:options) { {} }
+ let(:request_saml) { signed_auth_request(algo: "sha1") }
+
+ it 'is not valid' do
+ expect(subject.valid?).to eq false
+ end
+
+ it 'adds a require_sha256 error' do
+ subject.valid?
+ expect(subject.errors.include?(:require_sha256)).to be true
+ end
+ end
end
end
end
end
-
- def build_authn_context_classref(contexts)
- [contexts].flatten.map do |c|
- "#{c}"
- end.join
- end
end
end
diff --git a/spec/lib/saml_idp/service_provider_spec.rb b/spec/lib/saml_idp/service_provider_spec.rb
index 5b6b950d..a89b09e0 100644
--- a/spec/lib/saml_idp/service_provider_spec.rb
+++ b/spec/lib/saml_idp/service_provider_spec.rb
@@ -3,19 +3,157 @@ module SamlIdp
describe ServiceProvider do
subject { described_class.new attributes }
let(:attributes) { {} }
+ let(:cert) { saml_settings.get_sp_cert }
+ let(:options) { {} }
it { is_expected.to respond_to :metadata_url }
it { is_expected.not_to be_valid }
- describe "with attributes" do
+ describe 'with attributes' do
let(:attributes) { { metadata_url: metadata_url } }
- let(:metadata_url) { "http://localhost:3000/metadata" }
+ let(:metadata_url) { 'http://localhost:3000/metadata' }
- it "has a valid metadata_url" do
+ it 'has a valid metadata_url' do
expect(subject.metadata_url).to eq(metadata_url)
end
it { is_expected.to be_valid }
end
+
+ describe '#valid_signature' do
+ let(:raw_xml) do
+ SamlIdp::Request.from_deflated_request(
+ make_saml_request
+ ).raw_xml
+ end
+
+ let(:doc) { Saml::XML::Document.parse(raw_xml) }
+
+ describe 'the signature is not required' do
+ describe 'the service provider has no certs' do
+ it 'returns true' do
+ expect(subject.valid_signature?(doc)).to be true
+ end
+ end
+
+ describe 'the service provider has certs' do
+ before { subject.certs = [cert] }
+ it 'returns true' do
+ expect(subject.valid_signature?(doc)).to be true
+ end
+ end
+ end
+
+ describe 'the signature is required' do
+ 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) 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 'raises an error' do
+ expect { subject.valid_signature?(doc, true, options) }.to raise_error(SamlIdp::XMLSecurity::SignedDocument::ValidationError, 'No cert')
+ end
+ end
+
+ describe 'the service provider has one or more certs' do
+ describe 'one valid cert' do
+ before { subject.certs = [cert] }
+
+ it 'returns true' do
+ expect(subject.valid_signature?(doc, true, options)).to be true
+ end
+ end
+
+ describe 'one invalid cert' do
+ before { subject.certs = [invalid_cert]}
+ 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] }
+
+ describe "one is valid" do
+ it 'returns true' do
+ expect(subject.valid_signature?(doc, true, options)).to be true
+ end
+ end
+ end
+ end
+ end
+
+ describe 'a cert is present in the document' do
+ let(:raw_xml) do
+ SamlIdp::Request.from_deflated_request(
+ signed_auth_request
+ ).raw_xml
+ end
+
+ describe 'the service provider has no certs' do
+ it 'raises an error' do
+ expect { subject.valid_signature?(doc, true, options) }.to raise_error(SamlIdp::XMLSecurity::SignedDocument::ValidationError, 'No cert')
+ end
+ end
+
+ describe 'the service provider has one or more certs' do
+ describe 'one valid cert' do
+ before { subject.certs = [cert] }
+
+ it 'returns true' do
+ expect(subject.valid_signature?(doc)).to be true
+ end
+ end
+
+ describe 'one invalid cert' do
+ before { subject.certs = [invalid_cert]}
+
+ 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
+ let(:other_cert) do
+ OpenSSL::X509::Certificate.new(cloudhsm_idp_x509_cert)
+ end
+
+ describe 'the valid cert is registered in the idp' do
+ before { subject.certs = [other_cert, invalid_cert, cert] }
+
+ it 'returns true' do
+ expect(subject.valid_signature?(doc, true)).to be true
+ end
+
+ it 'matches the right cert' do
+ subject.valid_signature?(doc)
+ expect(subject.matching_cert).to eq cert
+ end
+ end
+
+ describe 'the valid cert is not registered in the idp' do
+ before { subject.certs = [other_cert, invalid_cert] }
+
+ 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
+ end
+ end
+ end
+ end
end
end
diff --git a/spec/support/certificate_helpers.rb b/spec/support/certificate_helpers.rb
index f49ca879..53db42f0 100644
--- a/spec/support/certificate_helpers.rb
+++ b/spec/support/certificate_helpers.rb
@@ -37,4 +37,8 @@ def encrypted_secret_key
def encrypted_secret_key_password
'im a secret password.'
end
+
+ def invalid_cert
+ OpenSSL::X509::Certificate.new(File.read('spec/support/certificates/too_short_cert.crt'))
+ end
end
diff --git a/spec/support/certificates/too_short_cert.crt b/spec/support/certificates/too_short_cert.crt
new file mode 100644
index 00000000..c9d170d4
--- /dev/null
+++ b/spec/support/certificates/too_short_cert.crt
@@ -0,0 +1,3 @@
+-----BEGIN CERTIFICATE-----
+MIICnTCCAgagAwIBAgIGAWVhrSugMA0GCSqGSIb3DQEBCwUAMIGHMR0wGwYDVQQDDBRzZWN1cmVwb3J0YWwub3BtLmdvdjEnMCUGA1UECgweT2ZmaWNlIG9mIFBlcnNvbm5lbCBNYW5hZ2VtZW50MQ4wDAYDVQQLDAVPQ0lTTzETMBEGA1UEBwwKV2FzaGluZ3RvbjELMAkGA1UECAwCREMxCzAJBgNVBAYTAlVTMB4XDTE4MDgyMjEyNDc0M1oXDTIzMDgyMjA0MDAwMFowgYcxHTAbBgNVBAMMFHNlY3VyZXBvcnRhbC5vcG0uZ292MScwJQYDVQQKDB5PZmZpY2Ugb2YgUGVyc29ubmVsIE1hbmFnZW1lbnQxDjAMBgNVBAsMBU9DSVNPMRMwEQYDVQQHDApXYXNoaW5ndG9uMQswCQYDVQQIDAJEQzELMAkGA1UEBhMCVVMwgZ8wDQYJKoZIhvcNAQEBBQADgY0AMIGJAoGBAIpUzkPbQdNQIRA3/64quioyOIetu4101bWg2NJFFu1Iy/xOAI7+hyGIbnvP1ZwlzB65+0f6yzZfrBy78uMIaPBQycDUXjrGAe9JS6eoJynSZiFvlUs5OOSQmoEZ1MfajH0EDhGV0drWTHFoPCdcckbdTd9TnyZdolVoC/3L93fbAgMBAAGjEjAQMA4GA1UdDwEB/wQEAwIC9DANBgkqhkiG9w0BAQsFAAOBgQBlqriiUBu0iiF+hMHqws5oHY+d6x66rR52XoBgnfIgKuwuQqWU57HCYXw8fho4ykefn+F7fdgAOOHNXjdTYVSlPihsU0pXqEA5NBDyCg5rzyvoLIz9jLDiIrmgZ8Tjog2wRm5aOV02fp4gcnjjBB+vNe8U+lmiJaBhY3AeiA7gSA==
+-----END CERTIFICATE-----
diff --git a/spec/support/saml_request_macros.rb b/spec/support/saml_request_macros.rb
index f65e62e5..ee6ae0be 100644
--- a/spec/support/saml_request_macros.rb
+++ b/spec/support/saml_request_macros.rb
@@ -2,16 +2,29 @@
module SamlRequestMacros
- def make_saml_request(requested_saml_acs_url = "https://foo.example.com/saml/consume")
- auth_request = OneLogin::RubySaml::Authrequest.new
- auth_url = auth_request.create(saml_settings(requested_saml_acs_url))
- CGI.unescape(auth_url.split("=").last)
+ def make_saml_request(requested_saml_acs_url = 'https://foo.example.com/saml/consume')
+ auth_url = url(saml_settings(requested_saml_acs_url))
+ CGI.unescape(auth_url.split('=').last)
end
- def make_invalid_saml_request(requested_saml_acs_url = "https://foo.example.com/saml/consume")
+ def url(saml_settings)
auth_request = OneLogin::RubySaml::Authrequest.new
- auth_url = auth_request.create(invalid_saml_settings)
- CGI.unescape(auth_url.split("=").last)
+ auth_request.create(saml_settings)
+ end
+
+ def signed_auth_request(embed: true, algo: "sha256")
+ CGI.unescape(url(signed_saml_settings(embed: embed, algo: algo)).split('=').last)
+ end
+
+ def signed_auth_request_options
+ signed_auth_request_options ||=
+ uri = URI(url(signed_saml_settings(embed: false)))
+ Rack::Utils.parse_nested_query uri.query
+ end
+
+ def make_invalid_saml_request
+ auth_url = url(invalid_saml_settings)
+ CGI.unescape(auth_url.split('=').last)
end
def make_saml_logout_request(requested_saml_logout_url = 'https://foo.example.com/saml/logout')
@@ -32,26 +45,52 @@ def make_sp_logout_request(requested_saml_logout_url = 'https://foo.example.com/
OneLogin::RubySaml::Logoutrequest.new.create(settings)
end
- def saml_settings(saml_acs_url = "https://foo.example.com/saml/consume")
- settings = OneLogin::RubySaml::Settings.new
- settings.assertion_consumer_service_url = saml_acs_url
- settings.issuer = "http://example.com/issuer"
- settings.idp_sso_target_url = "http://idp.com/saml/idp"
- settings.idp_slo_target_url = "http://idp.com/saml/idp-slo"
- settings.idp_cert_fingerprint = SamlIdp::Default::FINGERPRINT
- settings.name_identifier_format = SamlIdp::Default::NAME_ID_FORMAT
- settings.certificate = SamlIdp::Default::X509_CERTIFICATE
- settings.private_key = SamlIdp::Default::SECRET_KEY
+ def make_saml_request_with_options(options)
+ settings = saml_settings
+ options.each do |prop_name, prop_value|
+ settings.send("#{prop_name}=",prop_value)
+ end
+
+ auth_url = url(settings)
+ CGI.unescape(auth_url.split('=').last)
+ end
+
+ def saml_settings(requested_saml_acs_url = 'https://foo.example.com/saml/consume')
+ @saml_settings ||= begin
+ settings = OneLogin::RubySaml::Settings.new
+ settings.assertion_consumer_service_url = requested_saml_acs_url
+ settings.authn_context = 'urn:oasis:names:tc:SAML:2.0:ac:classes:Password'
+ settings.authn_context_comparison = 'exact'
+ settings.issuer = 'http://example.com/issuer'
+ settings.idp_sso_target_url = 'http://idp.com/saml/idp'
+ settings.idp_slo_target_url = 'http://idp.com/saml/idp-slo'
+ settings.idp_cert_fingerprint = SamlIdp::Default::FINGERPRINT
+ settings.name_identifier_format = SamlIdp::Default::NAME_ID_FORMAT
+ settings.certificate = SamlIdp::Default::X509_CERTIFICATE
+ settings.private_key = SamlIdp::Default::SECRET_KEY
+ settings.security = {
+ embed_sign: false,
+ logout_requests_signed: true,
+ digest_method: 'http://www.w3.org/2001/04/xmlenc#sha256',
+ signature_method: 'http://www.w3.org/2001/04/xmldsig-more#rsa-sha256'
+ }
+ settings
+ end
+ end
+
+ def signed_saml_settings(embed: true, algo: 'sha256')
+ settings = saml_settings('https://foo.example.com/saml/consume')
settings.security = {
- embed_sign: false,
- logout_requests_signed: true,
- digest_method: 'http://www.w3.org/2001/04/xmlenc#sha256',
- signature_method: 'http://www.w3.org/2001/04/xmldsig-more#rsa-sha256'
+ embed_sign: embed,
+ authn_requests_signed: true,
+ want_assertions_signed: true,
+ digest_method: "http://www.w3.org/2001/04/xmlenc#sha256",
+ signature_method: "http://www.w3.org/2001/04/xmldsig-more##{algo}"
}
settings
end
- def invalid_saml_settings(saml_acs_url = "https://foo.example.com/saml/consume")
+ def invalid_saml_settings
settings = saml_settings.dup
settings.issuer = ''
settings
@@ -59,7 +98,7 @@ def invalid_saml_settings(saml_acs_url = "https://foo.example.com/saml/consume")
def print_pretty_xml(xml_string)
doc = REXML::Document.new xml_string
- outbuf = ""
+ outbuf = ''
doc.write(outbuf, 1)
puts outbuf
end
diff --git a/spec/support/security_helpers.rb b/spec/support/security_helpers.rb
index b3e70ebd..666e0b9f 100644
--- a/spec/support/security_helpers.rb
+++ b/spec/support/security_helpers.rb
@@ -8,6 +8,9 @@ def fixture(document, base64 = true)
end
end
+ # TODO: go back and name these methods and files in semantically meaningful ways.
+ # and/or update and use the SamlRequestMacros to give tests more control over
+ # values being passed in
def response_document
@response_document ||= File.read(File.join(File.dirname(__FILE__), 'responses', 'response1.xml.base64'))
end
diff --git a/spec/xml_security_spec.rb b/spec/xml_security_spec.rb
index 6a603eb7..d4e96095 100644
--- a/spec/xml_security_spec.rb
+++ b/spec/xml_security_spec.rb
@@ -41,10 +41,26 @@ module SamlIdp
)
end
+ 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
response = Base64.decode64(response_document)
response.sub!('pJQ7MS/ek4KRRWGmv/H43ReHYMs=',
'b9xsAXLsynugg3Wc1CI3kpWku+0=')
+ response.gsub!('', '')
document = XMLSecurity::SignedDocument.new(response)
base64cert = document.elements['//ds:X509Certificate'].text
expect { document.validate_doc(base64cert, false) }.to(