More specific error messages for request certificate validation #545
Conversation
| @@ -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.
the softvariable (from line 258) was missing on the scope of this method
FabioCareMonkey
changed the title
| 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' } |
There was a problem hiding this comment.
I moved these up a couple of levels to reuse them in the other tests at this level
FabioCareMonkey
changed the title
FabioCareMonkey
force-pushed
the
bugfix-missing-soft-variable
branch
from
9551f0c
to
52868c4
Compare
| "<ds:X509Certificate>an-invalid-certificate</ds:X509Certificate>") | ||
| end | ||
|
|
||
| it 'is not valid' do |
There was a problem hiding this comment.
this would fail on the current version of the gem (and it should not)
| @@ -863,8 +863,12 @@ def validate_signature | |||
| valid = false | |||
| expired = false | |||
| idp_certs[:signing].each do |idp_cert| | |||
| valid = doc.validate_document_with_cert(idp_cert) | |||
| valid = doc.validate_document_with_cert(idp_cert, true) | |||
There was a problem hiding this comment.
It should be used
valid = doc.validate_document_with_cert(idp_cert, @soft)
instead
valid = doc.validate_document_with_cert(idp_cert, true)
There was a problem hiding this comment.
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
| 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.
In an environment where multiple idps cert are registered , that error gonna be registered for each validation executed.
In my opinion, this error should not be registered here because can't confuse the admin reviewing the logs.
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.
Please let me know what you think about the new changes
…he same error are not displayed
|
Followed a similar approach at: 59b9ade |
When validations find that the SAML request certificate does not match idp certificate they report the generic error message "Response signature validation failed";
This PR adds a more specific message for this scenario.
additionally the 'soff' variable which is required when calling the "append_error" method was not passed down to the 'validate_document_with_cert' method. This is also fixed