Validate signature algorithm correctly #130
Conversation
|
|
||
| cert, error = valid_cert(cert_text) | ||
| if error.present? | ||
| def validate(idp_certificate, options = {}, soft: true) |
There was a problem hiding this comment.
In general, I think changing the type (fingerprint to certificate) of an argument is risky! Like if this was a public API we'd want to like bump a major version, etc etc
For this gem where we tightly control the usage, it's probably fine
There was a problem hiding this comment.
oh agreed -- nothing that is called external to the gem actually changes (the XMLSecurity class is not called directly from the IdP -- IdP changes are all just updating the tests/removing the analytics code).
I thought about just ditching the validate method and updating the code to use validate_with_sha256 to avoid changing the type of an argument, but like you said, we control the usage, so it felt okay to do it this way.
lib/saml_idp/request.rb
Outdated
| Array(service_provider.certs).find do |cert| | ||
| document.valid_sig_with_sha256?(cert, options) | ||
| rescue SamlIdp::XMLSecurity::SignedDocument::ValidationError | ||
| document.valid_signature?(cert, options.merge(cert:)) |
There was a problem hiding this comment.
we probably no longer need to pass in cert: as an option right?
There was a problem hiding this comment.
oh, yes, i think we don't need it any longer, since we're no longer using it as a backup
lib/saml_idp.rb
Outdated
| def gather_errors(fingerprint, options = {}) | ||
| signed_document.validate(fingerprint, false, options) | ||
| def gather_errors(cert, options = {}) | ||
| signed_document.validate(cert, options, soft: false) | ||
| rescue SamlIdp::XMLSecurity::SignedDocument::ValidationError => e | ||
| { cert: options[:cert].serial.to_s, error_code: e.error_code } |
There was a problem hiding this comment.
if we remove passing in options[:cert] we can probably change this to be cert right?
| { cert: options[:cert].serial.to_s, error_code: e.error_code } | |
| { cert: cert.serial.to_s, error_code: e.error_code } |
|
|
||
| attr_accessor :document | ||
|
|
||
| def initialize(response) | ||
| @document = Nokogiri.XML(response) | ||
| end | ||
|
|
||
| def validate(idp_cert_fingerprint, soft = true, options = {}) | ||
| log 'Validate the fingerprint' |
There was a problem hiding this comment.
Minor, but glad to see somewhat noisy log items going away
This change is the follow-up to several weeks of tracking whether this change will impact any existing integrations. Running a query for the
certs_differentvalue does not produce any results, so it feels safe to move forward.This change:
validatepath in theXmlSecurityclass to stop messing around with trying to match digests; since we have the certs saved in the database, we can just compare those certs to the request cert.fingerprint_mismatcherror.