Require minimum length for secret#8720
Conversation
marcospereira
left a comment
There was a problem hiding this comment.
Looking good to me.
The only thing I'm thinking is how can we bring this information to dev mode and make people aware of this change sooner. Also, we need to add this to the migration guide.
marcospereira
left a comment
There was a problem hiding this comment.
Two minor suggestions and then it looks good to me.
Thanks, @wsargent.
documentation/manual/releases/release27/migration27/Migration27.md
Outdated
Show resolved
Hide resolved
documentation/manual/releases/release27/migration27/Migration27.md
Outdated
Show resolved
Hide resolved
…7.md Co-Authored-By: wsargent <will.sargent@gmail.com>
…7.md Co-Authored-By: wsargent <will.sargent@gmail.com>
marcospereira
left a comment
There was a problem hiding this comment.
I've added some changes (and made comments to explain them) and there is also some minor points.
documentation/manual/working/commonGuide/configuration/ApplicationSecret.md
Show resolved
Hide resolved
documentation/manual/working/javaGuide/advanced/embedding/JavaEmbeddingPlay.md
Show resolved
Hide resolved
framework/src/play/src/main/scala/play/api/http/HttpConfiguration.scala
Outdated
Show resolved
Hide resolved
|
@wsargent you may want to do a small review here since I've added some commits. 😄 |
ignasi35
left a comment
There was a problem hiding this comment.
Made a few comments wrt the length of the validations to trigger the warnings.
In some occasion, I made code suggestions (which now I think are wrong) but then realised the actual issues:
8and15are magic numbers- in both cases the comparison is lower-or-equal and that's not properly setup or worded in tests.
...ork/src/play-integration-test/src/test/scala/play/it/api/SecretConfigurationParserSpec.scala
Outdated
Show resolved
Hide resolved
framework/src/play/src/test/scala/play/api/http/HttpConfigurationSpec.scala
Outdated
Show resolved
Hide resolved
...ork/src/play-integration-test/src/test/scala/play/it/api/SecretConfigurationParserSpec.scala
Outdated
Show resolved
Hide resolved
...ork/src/play-integration-test/src/test/scala/play/it/api/SecretConfigurationParserSpec.scala
Outdated
Show resolved
Hide resolved
|
@ignasi35 should be good now |
Fixes
Fixes #8554
Purpose
Specifies a minimum length for the secret, below which the application will report an error. This prevents low entropy strings from being used.
Note that this is a) set intentionally low so that no genuine random string is used and b) is hardcoded and can't be disabled.
Unfinished work
This needs to be mentioned in the migration notes, as it will prevent some users from updating (and will require logins as changing the salt will invalidate all sessions) and will also require changes for users who use a "prod" application in their test suites.
Background Context
Salts aren't required to be incredibly strong, but they are required to be unique... and if you don't have a unique salt (i.e. you've been typing "password" in) then the application secret can be guessed and then the session data can be spoofed.