[feat] External Signature param for Redirect Binding #203
Conversation
Zogoo
changed the title
| when 384 then OpenSSL::Digest::SHA384 | ||
| when 512 then OpenSSL::Digest::SHA512 | ||
| else | ||
| OpenSSL::Digest::SHA1 |
There was a problem hiding this comment.
should this default value be configurable ?
There was a problem hiding this comment.
The request will have an extra parameter that should be used for validation.
If the value is not expected (other than SHA) it fails back to SHA1, as we do in other places.
There was a problem hiding this comment.
SHA-1 is not anymore enabled on modern distributions (like RHEL 9)
|
the long (44) commit list looks weird, I hope this gets squash merged, unless a rebase is done before hand ? |
| @@ -6,7 +6,7 @@ module SamlIdp | |||
| end | |||
|
|
|||
| it "signs valid xml" do | |||
| expect(Saml::XML::Document.parse(subject.signed).valid_signature?(Default::FINGERPRINT)).to be_truthy | |||
| expect(Saml::XML::Document.parse(subject.signed).valid_signature?("", Default::FINGERPRINT)).to be_truthy | |||
There was a problem hiding this comment.
this does not need to be better validated?
There was a problem hiding this comment.
The first attribute of this method is optional if the XML document already contains the certificate with the document. I understand that the code may look a bit strange, and using hash keys might be better, but that would require a lot of changes across different layers, which I want to avoid with these new changes.
There was a problem hiding this comment.
Reviewed !!!
Only a few minor questions, I guess. Looks like new tests we're added.
To be sure, I'd like to see #207 being merged and rebased here to ensure we don't lose important coverage.
Also, make sure those 44+ commits gets squashed
Zogoo
force-pushed
the
bug/custom_signed_algorithm
branch
from
adaa69b
to
3e58ea2
Compare
|
@jphenow could you please take a look at this one, I already tested these changes to a real Idp project. |
| return true if valid_saml_request? | ||
|
|
||
| head :forbidden if defined?(::Rails) | ||
| false | ||
| end | ||
|
|
||
| def decode_request(raw_saml_request) | ||
| @saml_request = Request.from_deflated_request(raw_saml_request) | ||
| def decode_request(raw_saml_request, signature, sig_algorithm, relay_state) |
There was a problem hiding this comment.
There are a few notes in the README that are broken by these signature changes - can you take a quick pass at updating those?
There was a problem hiding this comment.
It might(?) just be this one
There was a problem hiding this comment.
I have checked README, there was no specific section on how controller.rb methods could be help. But I have added a section that explains the starting point of this gem.
SAML 2.0 redirect binding has 2 possibilities to provide a signature one is embedded and the other one is external (as query param).
The embedded one works fine because it's XML in the URL. But external signature requires a validated signature against a combined version of other parameters.
The original idea of this PR was #111 , but request validation should not belong to XML validation (XML security) because those are 2 different layers.
And this PR also contains one bug fix.
Currently, the SP metadata certificate should used for AuthnRequest validation if SP requires validation for AuthnRequest but AuthnRequest itself doesn't contain any certificate.
IdP always needs to fall back to SP Metadata (Service Provider) attributes if AuthnRequest doesn't contain the attribute.
The reason of I'm putting those 2 changes into PR is that it comes to my repo's latest main branch.