Switch to SHA256 and generate a nonce for AES operations - #119
Switch to SHA256 and generate a nonce for AES operations#119aaronjwood wants to merge 3 commits into
Conversation
|
Wow, this is just bad. ;) Calling |
| // This method would need to take in a salt and pass it to PBKDF2 if we want to switch to PBKDF2. | ||
| hash := sha256.Sum256([]byte(secret)) | ||
| secret = hex.EncodeToString(hash[:]) | ||
| text, err := com.AESEncrypt([]byte(secret), ctx.nonce, []byte(value)) // The nonce should really be unique per cookie. |
There was a problem hiding this comment.
Wait. You hash the secret, encode it to a HEX string, then use the byte repesentation of that HEX string?
Why not plug in hash directly?
There was a problem hiding this comment.
It should be as you say, unless there are dependencies on other libraries/packages that are expecting it in this way. Then they'd all need to be changed. @unknwon would know more about that than me.
|
@leonklingele I agree, we really need to switch to PBKDF2 and change the API to fix the issues you're mentioning. If the API can't be changed and we can't move to PBKDF2 then I don't know what else to do. |
|
What you just said doesn't make sense at all. You're already breaking the API by switching from MD5 to SHA256. Then why not do it properly (i.e. without HEX encoding / decoding)? |
|
@leonklingele I don't think this breaks the API. It's internal to |
|
It breaks the API because everyone who still has one of the old MD5-stuff can't use it anymore. |
|
@unknwon I think it's best to close this out if the API can't be changed. I don't think what I've done here really helps anything. At the very least you can keep the changes to work with the new AES |
|
Why? IMO it's fine to break the API if the old one was broken security-wise. But then please do it properly. |
|
@unknwon how about changing the API and have something in the next release notes about a bunch of flawed crypto getting fixed which causes incompatibility? |
|
OK, I agree to break SetSuperSecureCookie's API as long as it allows accept arbitrary length of salt, and use it to generate desired nonce. The other big use case of this function is https://github.com/grafana/grafana/tree/master/vendor/gopkg.in/macaron.v1, but it is in vendor so shouldn't break their build. But we should warn them (maybe file an issue) about this. |
|
Okay, let me close this out and open up a new, proper one. |
3 participants
#118