Different approach on fixing issue of pull request #69 #74
Conversation
|
+1 |
|
Hey @SamDecrock thanks for the PR. This is an interesting approach and seems valid to me, but I'd love to get another pair of eyes on it tomorrow before calling it good to go. Meanwhile - would be great to add a test for this behavior. Look forward to getting this merged. |
| if !ignore_signature | ||
| for cert in idp_certificates or [] | ||
| signed_data_from_complete_saml_response = check_saml_signature(saml_response.toString(), cert) | ||
| if signed_data_from_complete_saml_response.length is 1 and signed_data_from_complete_saml_response[0] == saml_response.toString() |
There was a problem hiding this comment.
When there are multiple Signature nodes in the response, this throws
TypeError: Cannot read property 'length' of null
at parse_authn_response (/Users/mdolan/Repos/saml-test/node_modules/saml2-js/lib-js/saml2.js:603:50)(because check_saml_signature returns null https://github.com/Clever/saml2/blob/master/lib/saml2.coffee#L178)
Maybe just need this to be signed_data_from_complete_saml_response?.length is 1?
There was a problem hiding this comment.
Also, is there a reason for the string comparison? There are slight differences in ordering of the fields for me
|
Any plans on merging this solution ? I think I am affected by this bug as well, will be happy to make a new PR with the fix @mgduk commented about. |
|
@sylido @SamDecrock I think the comments thus far
are (still) blockers on this PR. Not sure what @SamDecrock's plans are but if @sylido wants to take the reins on this - sounds good to me. In general would be happy to merge this, but not if there are bugs (which it seems like there currently are). Feel like tests would be the best way to catch bugs like that. Let me know how this sounds. |
|
Is there any reason why this PR is still open and awaiting review? This should solve an issue I'm having trying to use this library since it appears that the enveloped XML signature is being ignored and is only checking signatures contained within each assertion. |
|
Has this been updated at all? I'm facing a similar issue now and this seems to be the best provided fix |
|
@nkumarcc I think @cozmo outlined the issues surrounding this, I tried making some tests to see if we can solidify the behavior but I falied at it - if any of you is willing to give it a try, they might merge it. I've been using this PR in my fork for quite a while now and haven't encountered a problem yet, but that's not necessarily true for all SAML cases. |
|
Hmmm @sylido I'd be down to figure it out, but I actually face an issue with the code as is. However, for this line: The |
|
@sylido honestly, that's what I did and it works properly (and I think it makes sense) |
|
Hi, It's been 3 years since I posted this fix. I'm not working on my saml related project anymore, so I cannot help you guys with the merge. If someone wants to take it up, feel free. Sam |
|
@SamDecrock can you add me as contributor? Unsure if I can contribute right away, but some of these PR's have come in handy in my private branch. Would love to help merge them in at some point down the line |
Hi,
This is my take at fixing the issue of pull request #69
I'm solving this by doing a signature verification of the complete response (a so called enveloped signature) at the very beginning (before decrypting anything). If that succeeds, it means that the complete document (except for the signature itself off course) is verified.
I then continue with the rest but without checking for signatures anymore.