Redo supporting multiple certs per ServiceProvider#4898
Conversation
This reverts commit 1b290f0.
- The logout codepath is different from the login one
| expect( | ||
| certificate.public_key.verify( | ||
| OpenSSL::Digest::SHA256.new, | ||
| Base64.decode64(signature), | ||
| canon_string, | ||
| ), | ||
| ).to eq(true) |
There was a problem hiding this comment.
this passes, but the equivalent .verify inside the gem is failing:
all the puts statements confirmed of for me that the string, the signature, and the serial are all the same, and the outside verificatin works but the gem says no
There was a problem hiding this comment.
Turns out Hash#to_query does not preserve order, fixed in a31640f
| else | ||
| current_service_provider.encryption_opts | ||
| elsif current_service_provider.encrypt_responses? | ||
| cert = saml_request.service_provider.matching_cert || current_service_provider.ssl_certs.first |
There was a problem hiding this comment.
So in updating the gem to try multiple certs... I figured maybe we just have the gem keep track of which one for us... it's a nasty side effect but I am getting desperate:
https://github.com/18F/saml_idp/pull/39/files#diff-29ac8f422d334e3198b369e7cd6d2139af467645af1128deb0def18de1c05409R27
There was a problem hiding this comment.
I think it's reasonable technical debt to take on - the gem is already a mess and tbh since the signature validation all happens there it feels like it will be cleaner.
|
|
||
| expect(response).to_not be_redirect | ||
|
|
||
| expect { xmldoc.saml_response(first_cert_settings) }.to raise_error |
There was a problem hiding this comment.
this is the assertion I was relying on the most to make sure that I wasn't magically defaulting to a wrong cert, ex if we change matching_cert || service_provider.ssl_certs.last this will fail
zachmargolis
requested review from
achapm,
jmhooper,
mitchellhenke,
orenyk and
stevegsa
zachmargolis
changed the title
orenyk
left a comment
orenyk
left a comment
There was a problem hiding this comment.
Looked at the new diff and it looks good to me but given the complexity I'd feel better with another set of eyes on it 😄. Nice work!
| else | ||
| current_service_provider.encryption_opts | ||
| elsif current_service_provider.encrypt_responses? | ||
| cert = saml_request.service_provider.matching_cert || current_service_provider.ssl_certs.first |
There was a problem hiding this comment.
I think it's reasonable technical debt to take on - the gem is already a mess and tbh since the signature validation all happens there it feels like it will be cleaner.
4 participants
This is a draft, I'm putting it down for now and taking a breakThe change this time are:
Trying to actually construct a working signed logout request... as far as I can tell neither the gem nor our codebase has tests that pass the(update: got it working)SignatureandSigAlgparams.... this is where I'm stuckTo see just the "redo" diff:
c43651a00e474a8507fb551125e34a13ef2ad547...margolis-redo-multiple-certs