omniauth / omniauth-saml

A generic SAML strategy for OmniAuth
https://github.com/omniauth/omniauth-saml
Other
338 stars 207 forks source link

Is :idp_cert_fingerprint_validator required? #109

Open pitbulk opened 8 years ago

pitbulk commented 8 years ago

I checked omniauth-saml's settings/code and I don't understand the use of

 :idp_cert_fingerprint_validator     => lambda { |fingerprint| fingerprint },

At the ruby toolkit, in order to check embedded Signatures (of the HTTP-POST binding), when you add a :idp_cert_fingerprint instead the :idp_cert, doesn't matter what you use, at the end the idp_cert is turned in a idp_cert_fingerprint to validate the document.

The certificate of the SAMLResponse is fingerprinted and compared with the value of the idp_cert_fingerprint.

I think this is already done at omniauth here

P.S I always recommend to set the idp_cert and not the idp_cert_fingerprint because HTTP-Redirect binding signature validations requires it (since the IdP's public certificate is not at the SAML Message). As you plan to add SLO soon, recommend the use of certificates vs fingerprints. Related topic: certFingerprint versus certificate/certData - simpleSAMLphp

md5 commented 8 years ago

idp_cert_fingerprint_validator is not required. It was added in #31, so you can read more about the use case there.

pitbulk commented 8 years ago

Ok, is an optional parameter, but why exist? since you can already provide dynamically a value for idp_cert_fingerprint, or do you want to support multiple idp_cert_fingerprint at once?

md5 commented 8 years ago

@pitbulk I'm not sure beyond what's described in #31. Perhaps it wasn't possible to dynamically provide the value of idp_cert_fingerprint when that PR was merged in late 2014?

Perhaps @bpedro can provide some context, but he hasn't been active on this project for a while.

pitbulk commented 8 years ago

ruby-saml requires to be set a certificate or a fingerprint (only 1 value) and it will validate it againstthe cert on the SAMLResponse, so if a wrong value is set, the SAMLResponse will be consider invalid, so the extra check of the idp_cert_fingerprint_validator I think is useless since already it was invalidated.

md5 commented 8 years ago

@pitbulk The code that deals with this "validator" is at https://github.com/omniauth/omniauth-saml/blob/59eeeb1eb030d8f4910c224598de21e2a25fdcab/lib/omniauth/strategies/saml.rb#L52-L59 (and in the response_fingerprint method).

Since that code is extracting the fingerprint from the SAML response directly, it is happening before ruby-saml does its own validation of the response.

I agree with you that this feature is rather suspect and equivalent functionality could be provided by the OmniAuth setup phase functionality in any use case I could think of. That being said, I could be missing something.