search

unknwon / com

This is an open source project for commonly used functions for the Go programming language.
Apache License 2.0
755 stars 133 forks source link

Use AES in GCM mode instead of CFB mode #12

Closed aaronjwood closed 7 years ago

aaronjwood commented 7 years ago

11

aaronjwood commented 7 years ago

Do you have any tests anywhere for this stuff?

unknwon commented 7 years ago

Test file is https://github.com/Unknwon/com/blob/master/string_test.go (no tests for this AESEncrypt/AESDecrypt yet), and you got compilation error. Make sure code compiles...

aaronjwood commented 7 years ago

Oops, my mistake...thought I caught and committed that already. Just fixed it.

unknwon commented 7 years ago

All good, merging.

unknwon commented 7 years ago

Just realized we would have a API break here (Macaron won't compile correctly now)... maybe we should leave AESEncrypt as it was and make a new function...

leonklingele commented 7 years ago

What's the problem with changing the API? If one intends to use the most recent version of this library, then changes to the code base are required. There's no need to have AESEncrypt anymore as it is/was totally broken.

unknwon commented 7 years ago

@leonklingele it is not possible to accept/remember the nonce here: https://github.com/go-macaron/macaron/blob/master/context.go#L421-L429 without breaking the API.

leonklingele commented 7 years ago

That's why I call to prepend / append the nonce directly to the resulting ciphertext in AESEncrypt. That way, the caller doesn't need to care about what the function does internally.

aaronjwood commented 7 years ago

@Unknwon I'm making changes to Macaron right now that will fix the API break. Will send over a PR in a sec...

aaronjwood commented 7 years ago

@Unknwon @leonklingele https://github.com/go-macaron/macaron/pull/119

aaronjwood commented 7 years ago

There are definitely some limitations/reduced security with keeping the API the same. For example, there is only ever one nonce generated with my PR above. Also, the key used for AES is still a basic hash of data instead of a derived key...

aaronjwood commented 7 years ago

@leonklingele it is not possible to accept/remember the nonce here: https://github.com/go-macaron/macaron/blob/master/context.go#L421-L429 without breaking the API.

One issue with this is that the nonce used by setting a secure cookie will need to be used when getting it. I think ideally those funcs (or at least GetSuperSecureCookie) would have an extra input to take in the nonce that was used when first setting the secure cookie.

leonklingele commented 7 years ago

Good luck implementing this. Why make life so hard?

aaronjwood commented 7 years ago

@leonklingele I think it would be fine if we could change the API and move away from the hashing stuff. There just needs to be coordination in place for the nonce and key, and the key needs to be generated/managed differently. Why would that be so hard?

aaronjwood commented 7 years ago

@Unknwon are you okay if I make more breaking API changes that were discussed here?