Binary signing of encryption. #404
Conversation
elorest
requested review from
drujensen,
marksiemers,
eliasjpr,
fridgerator and
faustinoaq
| @@ -24,22 +28,30 @@ module Amber::Support | |||
| # Verify and Decrypt a message. We need to verify the message in order to | |||
| # avoid padding attacks. Reference: http://www.limited-entropy.com/padding-oracle-attacks. | |||
| def verify_and_decrypt(value : String) : Bytes | |||
| decrypt(verifier.verify_raw(value)) | |||
| verify_and_decrypt(Base64.decode(value)) | |||
There was a problem hiding this comment.
The base64 de-marshaling here doesn't match the marshaling that is happening when the cookie is written, does it make sense to have this class handle both Base64 encoding and decoding?
There was a problem hiding this comment.
It probably makes more sense to remove that from here entirely and have this class just take and return slices. As it is this is just an overload where if a string is passed in it de-marshals it.
What do you mean by "it doesn't match"?
There was a problem hiding this comment.
I'm only trying to point out that by my reading, the code flow for decrypting a message is:
other->MessageEncryptor->base64.decode
and the code flow for encrypting is:
other->base64.encode->MessageEncryptor
I'd rather MessageEncryptor handle both base64 directions or neither.
There was a problem hiding this comment.
Neither makes more sense, since method overloading doesn't work with return values, we'd have to create another method called encrypt_and_base64_encode or something.
I think removing it makes more sense.
|
Is there a simple way we can fingerprint the encryption method to make this more upgrade-able in the future? |
|
@robacarp Not sure it understand your meaning on this. This isn't going to change all over the place. It will be a small breaking change with the 0.4.0 release but that's sort of the point of minor and major version releases. One will just have to decrypt and save their production.yml before upgrading, and reencrypting. |
|
@elorest I think it will also invalidate all cookies signed with the previous algorithm, but I'm not familiar enough with all the code at play to say that with confidence. I'm thinking more in terms of when sha256 is broken enough that it needs to be replaced, is there a way the cookies can be marked as sha256 so that they can be decrypted when this code is updated again. Its not something necessary to put a ton of time into now, but simply prefixing the signed cookie with |
|
Generally cookies aren't important enough that if they become invalid every year or two anyone worries about it. Rails has changed the way cookies worked many times in the last 12 years. |
|
@elorest thanks, that's good background and context |
* binarry signing of encryption * added signature to encrypted secrets * SHA256 Signature for environment files * refactored a couple things to make it more readable.
Description of the Change
Benefits