Closed peterthomassen closed 4 months ago
Hello π
I was not at the initial design decision but I guess the main reason was still to increase security. You do raise valid arguments though.
I think the rainbow attack mitigation given by salting still stands valid especially as we have potentially long lived token while the auto-refresh feature is a thing.
I also professionally faced many security audit companies who would require you to have every possible security bits in place to meet their requirements, salting would definitely be in.
Why would you like to see it removed?
I would like to challenge that the salt is a security improvement. In fact, it is a downgrade. Here's why:
The bottomline is that rainbow tables are not an attack vector for strong tokens, and therefore the salt is not a mitigation of anything, as it is a no-op from a security standpoint. It further puts the user under the false impression of having 256 bits of entropy although it is only 224.
If someone were to set AUTH_TOKEN_CHARACTER_LENGTH
to a lower value, such as 32 hex digits (128 bits), then only 96 bits of entropy would remain after stripping the lookup prefix. This would actually significantly degrade the quality of an otherwise more or less acceptable 128 bit token.
However, it comes at great extra complexity:
I would like to stress that these points are meant purely from a factual standpoint, and not intended at all to criticize anyone's work! Knox is a great project, and it has a great community. I'd like to contribute my two cents to remove some unnecessary weaknesses and complexities that it currently has.
(In case this discussion leads to a code change, I would also like to pitch moving from SHA-512 to SHA-256. As there are only 256 bits of entropy in the token, approximately every other bit of the SHA-512 output is "non-entropic". The input currently just gets stretched, but the token does not get better. This change would a) improve runtime, b) save storage in the database, c) establish a better cryptographic correspondence between the token and its digest. There is no concern about SHA-256 being easier to break than SHA-512, as both are based on SHA-2, i.e. they are algorithmically the same.)
Thank you so much for the detailed explanation. This makes a lot more sense now. I agree with everything stated to the limit of my current knowledge and I very much like the idea of simplifying the code base!
Iβd like to have others opinion before going forward though especially those who took the design decision.
Thanks again ππ»
One complication is certainly that dropping the salt is not straightforward for existing tokens, as the hash cannot be verified once the salt is gone. So, either everybody would have to be logged out, or there would have to get some sort of transition period (something around the max. token lifetime, but that may mean dragging along the technical debt for a long time).
Sure we need a breaking change and to bump to a major version but nothing we canβt describe in the changelog.
@peterthomassen Everything you say makes sense. To answer the question about why the salt was originally included, the answer is I honestly don't remember. I'm sure I had a good reason in the first version, but I can't think of one now.
On moving to SHA-256, my original plan was that the algorithm should be configurable. If we never implemented that then that's by oversight rather than design. I seem to remember it being possible at one time to run with MD5 as an option (which was simply included to increase the run speed of tests and not intended to be used as a serious option).
@James1345 Regarding salt -- thanks for your feedback. I'm sure there was some reason at the time, but sometimes things change :-) I don't think any archaeology needs to be done.
Regarding algorithm -- it is possible to configure the algorithm. My suggestion was to change the default, as the current one has no advantage over SHA-256, but a few (smaller) disadvantages.
Seems like we could go with a simplified, more efficient, salt less token implementation then π
@peterthomassen would you like / have time to submit a PR implementing the salt less version?
I suggest we split the "configurable algorithm" feature in a separate PR.
Unfortunately, I currently don't have time to create a PR. (We have found another solution for our own purposes.)
Closing as the salt has now been removed
Salt is commonly added to weak passwords so that if reused, they do not produce the same hash. It also exponentially increases the storage burden on those undertaking to create rainbow tables. If entropy is high in the hashed secret itself, these pro-salt arguments no longer apply.
By default, Knox creates 64-digit hex tokens, corresponding to 256 bits of entropy. This is by far sufficient to both exclude accidental reuse, and rainbow table generation. In fact, adding another 16 hex digits salt, corresponding to 64 bit, appears like a rather "random" choice, with questionable benefit.
So, why use salt at all?