Problem: SHA-1 is considered insecure and should not be used by default, see https://shattered.io/
We should enable strict checking of responses and use SHA-256 instead.
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | interdiff-4-5.txt | 2.92 KB | roderik |
| #6 | samlauth-sha256-3043713-5.patch | 2.66 KB | roderik |
| #4 | samlauth-sha256-3043713-4.patch | 3.35 KB | nironan |
| #2 | samlauth-sha256-3043713.patch | 982 bytes | klausi |
Comments
Comment #2
klausiPatch.
Comment #3
smfsh commentedI think this is great, and its true that this is recommended. The D8 module implements this on a per-site bases though as a configuration option and I think we should do the same. Checkbox for strict checking and drop down for algorithm (like the d8 setup.)
I can get this implemented along with a couple other changes I was planning to make to the form unless you get around to it first.
Comment #4
nironan commentedHere's an attempt to make the settings configurable. I made SHA-256 the default... is it a good idea? Or should we leave the (insecure) SHA-1 as default for backward compatibility?
Comment #5
roderik(I'm taking over D7 maintenance.) Thank you for raising this; contributions like these still require me to look more closely at the library code which is a good thing.
I have a question though. But given that this has been sitting in the queue so long, I'll already commit the non-question part.
On the original issue / patch:
On patch #4: thanks, I agree that it's good to have both be configurable. (Also, I now see that the D8 version's default choice is still SHA1 because that was the default in 2017. Sigh.) But
Setting Needs Info for the first bullet point (and sure I'll commit a patch with 'security']['signMetadata' if someone wants / provides it), committing the rest.
Comment #6
roderikComment #10
roderikResponding to myself:
1) This is not true, I can see it getting used (if I just take care of activating xdebug properly on those post requests...)
2) (I'm assuming that) I just realized what this the advantage of this... it sets the host/port/etc correctly if the SP is behind a reverse proxy.
In the D8 version I committed #2912717: Use the `Forwarded-For-*` headers for proxy for this, which calls
\OneLogin_Saml2_Utils::setProxyVars(TRUE)(public static function, meh) and adds a note to the README to configure settings. That may not be needed after we set 'baseurl'.So I'll commit this and put a TODO in the D8 version to test/change that sometime.