chore: return generic errors for aes256cbc encrypt and decrypt #509
Conversation
developerfred
force-pushed
the
issue/499
branch
from
20febd0
to
15b0155
Compare
There was a problem hiding this comment.
Thanks @developerfred for the PR 😄 there are a few more errors that need to be returned by the Encrypt(message mc.PlainContent) (mc.EncryptedContent, error) and decryptEncryptedData(d.privateKey, encryptedData) as they still return other errors that would make it back to the end user.
Thanks
There was a problem hiding this comment.
thanks @developerfred these changes were exactly what I had asked for. I missed one method though decryptEncryptedData(d.privateKey, encryptedData) can you also do the same with that one
developerfred
force-pushed
the
issue/499
branch
from
80caf39
to
ff0c66d
Compare
There was a problem hiding this comment.
Thanks @developerfred looks great 😄 changes are good to merge 👍
| } | ||
|
|
||
| macKey, encryptionKey := generateMacKeyAndEncryptionKey(sharedSecret) | ||
| mac, err := generateMac(macKey, data.InitializationVector, *ephemeralPublicKey, data.Ciphertext) | ||
|
|
||
| if err != nil { | ||
| return nil, errors.WithMessage(err, "generateMac failed") |
There was a problem hiding this comment.
I don't think it's a good idea to mask all errors with the same error. It's okay to return the same error type, but it should not be the same error message for different types of errors, the actual error is getting masked here. It'll be harder to debug the reason just by seeing "error in decryption".
There was a problem hiding this comment.
I believe that in the future we will create a variable for each type of error, making the code more maintainable.
There was a problem hiding this comment.
@hasitpbhatt thank you for taking the time to leave a review on this PR. Sorry for the delay getting back to you I was off for a while. I would love to hear about how you found out about Mailchain and if there is anything I can do to help you. Please feel free to comment here or join our slack channel
You raise an excellent point about the outcome from a developer perspective. The reason for masking the error is to reduce the information an attacker might get from an error. This does need more thought overall so I have opened an issues #532 to discuss this.
In the meantime @developerfred I will merge this change as it resolves the issue as originally defined. Once #532 is resolved we will come back to this
fixes #499