fix: saml signatureAlgorithm in AuthnRequest #2792
Conversation
* This fixes a regression introduced in commit d10922a where we stopped reading the signatureAlgorithm from the properties file and only used the SHA1 default. [#187250012] Co-authored-by: Hongchol Sinn <hongchol.sinn@broadcom.com>
|
We have created an issue in Pivotal Tracker to manage this: https://www.pivotaltracker.com/story/show/187271895 The labels on this github issue will be updated when the story is started. |
| @@ -301,6 +301,10 @@ | |||
|
|
|||
| <bean id="fixedHttpMetaDataProvider" class="org.cloudfoundry.identity.uaa.provider.saml.FixedHttpMetaDataProvider" /> | |||
|
|
|||
| <bean id="defaultSamlConfig" class="org.cloudfoundry.identity.uaa.provider.saml.SamlConfigurationBean"> | |||
| <property name="signatureAlgorithm" value="${login.saml.signatureAlgorithm:SHA1}"/> | |||
There was a problem hiding this comment.
It is to be whatever you configure in TAS setting and what is in uaa.yml.
There was a problem hiding this comment.
Same thought here :) I guess the state was SHA-1 before but would be good to have a plan to adapt a secure default.
There was a problem hiding this comment.
Should the default value be SHA256. the default being SHA1 is not secure. This is not terrible since it is overridden, but the default should be closer to SHAKE256 or SHA3-512
There was a problem hiding this comment.
In production, SHA1 was never supposed to be used. In OpsManager, you can set the value to SHA256 or SHA512 only. SHA1 was exposed only because we removed the bean by mistake.
There was a problem hiding this comment.
@torsten-sap Is this a PR you can approve as acceptable?
The bean definition is the same as the old one we took out by mistake.
|
Following this fix we should consider ensuring our defaults are as secure as possible: https://en.wikipedia.org/wiki/Secure_Hash_Algorithms |
|
We considered changing the default to SHA256, but decided to keep the same default as before. Note that TAS will always set it to SHA256 or SHA512, so we do not expect this default to have any effect in TAS. It is true that a default of SHA256 would have made the bug less severe and we might want to consider changing that soon. |
Tallicia
added
in progress
accepted
[#187250012]