Gracefully handle bad SAML logout data

[zachmargolis]

I had some trouble building a request that generated the same error we saw in prod, so I went the route of stubbing something inside the IDP gem and then catching all the places

[mitchellhenke]

I don't fully understand the library, but to avoid the magic string, could we do

if matching_cert
  saml_request.service_provider.fingerprint = Fingerprinter.fingerprint_cert(matching_cert)
end

[zachmargolis]

the library gets cranky if .fingerprint is nil, so that's what I'm trying to avoid at all costs

[mitchellhenke]

I guess my confusion is we weren't setting fingerprint before, so it was getting set somewhere? If we need to override we can with the if, and if there's no matching cert, it feels like we should error earlier in the request.

[zachmargolis]

We used to pass fingerprint every time: https://github.com/18F/identity-idp/pull/4851/files#diff-fdfe3f731c3c1bb52752814efbd9b353dec85fca38e11f0ccc05e8622b9893beL29-L31

But since fingerprint is tied to a specific cert, we can't do that for multiple certs

[zachmargolis]

(wrong place to comment)

[aduth]

Before multi-cert, we had this line which checked cert.blank? before parsing it. Would this be something we would have wanted to have kept in #4851?

https://github.com/18F/identity-idp/pull/4851/files#diff-fdfe3f731c3c1bb52752814efbd9b353dec85fca38e11f0ccc05e8622b9893beL35

[aduth]

Or change:

identity-idp/app/models/service_provider.rb

Line 35 in d6446cf

@ssl_certs ||= (certs.presence || Array(cert)).map do |cert|

...to:

@ssl_certs ||= (certs.presence || Array(cert).compact).map do |cert|

[orenyk]

@aduth Array(cert) will always only have a single element, I think since the cert attribute is the old column (I had this same confusion when I initially reviewed this feature).

[zachmargolis]

Before multi-cert, we had this line which checked cert.blank? before parsing it. Would this be something we would have wanted to have kept in #4851?

Based on the stack trace from the errors, I think that the nil/blank data is coming from the request when we try to validate it, not from us having blank data inside here

[aduth]

@aduth Array(cert) will always only have a single element, I think since the cert attribute is the old column (I had this same confusion when I initially reviewed this feature).

My thinking was that if cert is nil, Array(cert) results in [nil], whereas Array(cert).compact would result in [].

And [nil].map { |cert| OpenSSL::X509::Certificate.new(load_cert(cert)) } may error?

[zachmargolis]

My thinking was that if cert is nil, Array(cert) results in [nil], whereas Array(cert).compact would result in [].

Not exactly! 😬

irb(main):008:0> Array(nil)
=> []

[aduth]

Not exactly! 😬

🤦

[orenyk]

Lol - whee Ruby!

[zachmargolis]

Closing in favor of #4898