-
-
Notifications
You must be signed in to change notification settings - Fork 560
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
More specific error messages for request certificate validation #545
Changes from all commits
a4dc9d3
229221e
1c442e4
60aa020
11145ab
c3d0b71
52868c4
9dbfe66
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -241,7 +241,7 @@ def validate_document(idp_cert_fingerprint, soft = true, options = {}) | |
validate_signature(base64_cert, soft) | ||
end | ||
|
||
def validate_document_with_cert(idp_cert) | ||
def validate_document_with_cert(idp_cert, soft = true) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the |
||
# get cert from response | ||
cert_element = REXML::XPath.first( | ||
self, | ||
|
@@ -255,12 +255,12 @@ def validate_document_with_cert(idp_cert) | |
begin | ||
cert = OpenSSL::X509::Certificate.new(cert_text) | ||
rescue OpenSSL::X509::CertificateError => _e | ||
return append_error("Certificate Error", soft) | ||
return append_error("Document certificate error", soft) | ||
end | ||
|
||
# check saml response cert matches provided idp cert | ||
if idp_cert.to_pem != cert.to_pem | ||
return false | ||
return append_error("Document certificate does not match idp certificate", soft) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In an environment where multiple idps cert are registered , that error gonna be registered for each validation executed. I'm ok raising an error that none of the registered certs matched the certificate in the document which will require some refactor in the code, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please let me know what you think about the new changes |
||
end | ||
else | ||
base64_cert = Base64.encode64(idp_cert.to_pem) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -395,25 +395,51 @@ class XmlSecurityTest < Minitest::Test | |
end | ||
|
||
describe '#validate_document_with_cert' do | ||
let(:document_data) { read_response('response_with_signed_message_and_assertion.xml') } | ||
let(:document) { OneLogin::RubySaml::Response.new(document_data).document } | ||
let(:idp_cert) { OpenSSL::X509::Certificate.new(ruby_saml_cert_text) } | ||
let(:fingerprint) { '4b68c453c7d994aad9025c99d5efcf566287fe8d' } | ||
Comment on lines
+398
to
+401
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I moved these up a couple of levels to reuse them in the other tests at this level |
||
|
||
describe 'with valid document ' do | ||
describe 'when response has cert' do | ||
let(:document_data) { read_response('response_with_signed_message_and_assertion.xml') } | ||
let(:document) { OneLogin::RubySaml::Response.new(document_data).document } | ||
let(:idp_cert) { OpenSSL::X509::Certificate.new(ruby_saml_cert_text) } | ||
let(:fingerprint) { '4b68c453c7d994aad9025c99d5efcf566287fe8d' } | ||
it 'is valid' do | ||
assert document.validate_document_with_cert(idp_cert), 'Document should be valid' | ||
end | ||
end | ||
|
||
it 'is valid' do | ||
assert document.validate_document_with_cert(idp_cert), 'Document should be valid' | ||
end | ||
describe 'when response has no cert but you have local cert' do | ||
let(:document_data) { response_document_valid_signed_without_x509certificate } | ||
|
||
it 'is valid' do | ||
assert document.validate_document_with_cert(idp_cert), 'Document should be valid' | ||
end | ||
|
||
describe 'when response has no cert but you have local cert' do | ||
let(:document) { OneLogin::RubySaml::Response.new(response_document_valid_signed_without_x509certificate).document } | ||
let(:idp_cert) { OpenSSL::X509::Certificate.new(ruby_saml_cert_text) } | ||
end | ||
|
||
it 'is valid' do | ||
assert document.validate_document_with_cert(idp_cert), 'Document should be valid' | ||
describe 'when response cert is invalid' do | ||
let(:document_data) do | ||
contents = read_response('response_with_signed_message_and_assertion.xml') | ||
contents.sub(/<ds:X509Certificate>.*<\/ds:X509Certificate>/, | ||
"<ds:X509Certificate>an-invalid-certificate</ds:X509Certificate>") | ||
end | ||
|
||
it 'is not valid' do | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this would fail on the current version of the gem (and it should not) |
||
assert !document.validate_document_with_cert(idp_cert), 'Document should be valid' | ||
assert_equal(["Document certificate error"], document.errors) | ||
end | ||
end | ||
|
||
describe 'when response cert is different from idp cert' do | ||
let(:idp_cert) { OpenSSL::X509::Certificate.new(ruby_saml_cert_text2) } | ||
|
||
it 'is not valid' do | ||
exception = assert_raises(OneLogin::RubySaml::ValidationError) do | ||
document.validate_document_with_cert(idp_cert, false) | ||
end | ||
assert_equal("Document certificate does not match idp certificate", exception.message) | ||
end | ||
|
||
it 'is not valid (soft = true)' do | ||
document.validate_document_with_cert(idp_cert) | ||
assert_equal(["Document certificate does not match idp certificate"], document.errors) | ||
end | ||
end | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be used
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we cannot use the
@soft
variable here, as it would raise if any of the certificates is invalid (when@soft
is false). this validation is required to be soft in the loop.I will adjust the code as per your second comment. and using the soft variable to determine if it should raise after validating all certificates