Add support for key based encryption and decryption #3
Conversation
|
Clearly the README.md will need updating if this is merged. I held off doing that until we agreed on the PR. |
|
Oh wow! Thank you @dmjones500, this is great! I'll took a look as soon as I can 🎉 🎉 |
|
Hey @dmjones500, sorry for the delay. All these changes look great to me! 👍 Do you have any spare time to update the readme? Thanks again for getting this implemented, it's been a while since I touched this library, so I'm glad it's getting some improvements :) |
|
I'm happy to make a few updates. What are your thoughts about moving towards using godoc (e.g. https://godoc.org/github.com/RNCryptor/RNCryptor-go) rather than documenting each function individually on the README.md? See my other library for an example of just linking to the docs. |
|
@dmjones500 I think moving the docs to godoc would be great! I should have done that the first time around 😄 |
|
Ping to the maintainer @stevenschobert – I know this library hasn't been updated in forever, but would this be OK to merge? I can sign up for updates/godoc changes as a separate PR if you'd like those added. |
|
Hey @mrosales! Thanks for the reminder -- I would love to get this merged and released. It's been a few years since I worked on this project though, and it might be a while before I get the time to give this the thorough review it needs. If anyone has time to help me review (@dmjones @mrosales) this and sure up any testing/documentation gaps, I'd greatly appreciate it! |
There was a problem hiding this comment.
It seems like the code here is a solid addition. The go code seems to be on-par with the initial implementation with the bonus of being formatted with go fmt.
Similarly, the test cases are probably a little light on hitting the error cases, but they have the same level of depth as the original cases. It seems like a valuable addition to add these changes and address other concerns separately.
My one structural piece of feedback here is that there is a fair amount of code duplication between these two implementations given that the logic after deriving the password-based key is the same as the logic for key-based decryption. This PR seems like a good incremental change though and I don't think trying to extract out more of the shared functionality to test separately would need to hold up a merge here.
| unpadding := int(decrypted[length-1]) | ||
|
|
||
| return decrypted[:(length - unpadding)], nil | ||
| version := data[0] |
There was a problem hiding this comment.
This is one of the causes of #1 I think, but it might be nice to check the length up front and return an error if the length is shorter than the expected header instead of causing a panic.
Possibly something like:
if len(data) < versionLength + optionsLen + saltLength + hmacSaltLength + ivLength + hmacLength {
return nil, errors.New("invalid data: does not contain valid header")
}
| version := data[0] | ||
|
|
||
| if version != supportedVersion { | ||
| return nil, errors.New(fmt.Sprintf("unsupported version: %d", version)) |
There was a problem hiding this comment.
| return nil, errors.New(fmt.Sprintf("unsupported version: %d", version)) | |
| return nil, fmt.Errorf("unsupported version: %d", version) |
| } | ||
|
|
||
| func DecryptWithKey(decKey, hmacKey, data []byte) ([]byte, error) { | ||
| version := data[0] |
There was a problem hiding this comment.
same suggestion as above about checking the length before accessing the slice.
This PR introduces key-based encryption and decryption, per the V3 RNCryptor spec. I've added test cases based on the V3 example vectors.
I took the opportunity to tidy some of the existing code, extracting constants and removing some redundant code. I hope that's ok.
My first commit uses gofmt to tidy up the formatting, so the real changes can be seen by diffing 215959a and 938f1ce.
(FYI - I'm the developer for JNCryptor, the Java version of RNCryptor)