-
Notifications
You must be signed in to change notification settings - Fork 8
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6 +/- ##
==========================================
+ Coverage 94.11% 94.17% +0.06%
==========================================
Files 17 16 -1
Lines 781 772 -9
==========================================
- Hits 735 727 -8
+ Misses 46 45 -1
Continue to review full report at Codecov.
|
Moved down to section about compatibility
be used on its own. | ||
mrcrypt is a command-line tool which encrypts secrets that conform to the AWS Encryption SDK's `message format <http://docs.aws.amazon.com/encryption-sdk/latest/developer-guide/message-format.html>`__ for envelope encryption. Envelope encryption is used to encrypt a file using a KMS data key. That data key is then encrypted with regional KMS Customer Master Keys. Each regionally encrypted data key is then stored in the encrypted message. When decrypting, the appropriate regional CMK is used to decrypt the data key, and the data key is then used to decrypt the file. In other words, encrypt once - decrypt from anywhere. | ||
|
||
Because mrcrypt follows the AWS Encryption SDK's message format, files encrypted by mrcrypt can also be decrypted by the AWS Encryption SDKs for Python and Java. This allows application developers to build robust in-app decryption solutions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unfortunately not entirely accurate. mrcrypt does not support elliptic curve (EC) point compression, which is required to fully comply with the AWS Encryption SDK message format. Because of this, while mycrypt output can currently be read by the AWS Encryption SDK for Java because of a quirk in BouncyCastle, mrcrypt output cannot be read by the AWS Encryption SDK for Python.
note: Because it was an unintended side effect and introduces undesired behavior, we do not guarantee that all future versions of the AWS Encryption SDK for Java will be able to decrypt messages containing uncompressed EC points: aws/aws-encryption-sdk-java#7 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, GitHub selected more lines than I asked for.. For clarification, the innacurate part is : "Because mrcrypt follows the AWS Encryption SDK's message format, files encrypted by mrcrypt can also be decrypted by the AWS Encryption SDKs for Python and Java"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. We know about that issue. We wrote mrcrypt before the Python SDK was released. Worked really well for most use cases. But now that there's a Python SDK, we're planning using that inside mrcrypt. That should make things fully compatible with the Java SDK as well. We've pointed out the elliptical curve issue at the bottom of the README. Maybe we'll make a note about that section at the top.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, yup, I saw your initial launch as we were going through final security testing for the Python SDK. It was definitely frustrating to see duplication of effort, but awesome to see your enthusiasm for the platform.
My issue with the new wording in the readme is that someone might read that and reasonably expect to be able to use either of the Encryption SDK clients to decrypt files that they create using mrcyrpt, which is not currently the case.
Great to see continued activity here! Feel free to grab me if you run into any issues migrating the internals to the Python SDK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattsb42-aws Thanks for looking at this PR. It's great to see that you are excited about the community involvement with the AWS Encryption SDK.
This is an important point you've brought up. I hope that the limitations of mrcrypt are clear in the Compatability section of the README. Perhaps the intro paragraph should be a little clearer about this too.
For others that may come across this thread, I feel that it's important to mention that Bouncy Castle's support for uncompressed points is not a quirk. The uncompressed point serialization that is used by mrcrypt is supported by most ECC standards, including the SEC/NIST standards which is what the AWS Encryption SDK references. Uncompressed public points of the secp384r1
and secp256r1
curves used by the AWS Encryption SDK are fully supported by Bouncy Castle. I don't see mrcrypt's current implementation becoming incompatible with the AWS Encryption SDK for Java anytime soon unless there are plans to move away from Bouncy Castle.
However, I cannot speak for the AWS Encryption SDK for Python because I have not tried it as of writing. I will remove that statement from the README.
All that being said, ultimately I would like to see the files generated by mrcrypt fully comply the AWS Encryption SDK Message Format by supporting point compression. mrcrypt's main purpose is to provide a command-line tool to assist working with files that follow the AWS Encryption SDK Message Format. The easiest method is likely using the AWS Encryption SDK for Python. I'm hoping to find some time soon to work on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just submitted a new PR with some wording changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updated wording, @michaelajr; that addresses my concerns with the intro.
@austinmoore- The compatibility section does lay out the disconnect well; my concern was with a conflicting message being presented in the intro (which, let's be honest, most users are much more likely to read) that would lead a casual reader to a different conclusion.
You are absolutely correct that uncompressed points are intentionally supported by Bouncy Castle. What I meant by the "quirk" is the combination of: 1) we use Bouncy Castle to load the point, 2) Bouncy Castle has a single API for loading both uncompressed and compressed points, and 3) we don't specifically check that the point is compressed before loading it. We make no guarantees that point 1 will stay true (especially given how slow Bouncy Castle's elliptic curve crypto is) and we make no guarantees that point 3 will not change in the future (though that would require at least a minor version bump) given that the message format specification requires a compressed point.
I can confirm that the AWS Encryption SDK for Python does not support compressed points, though as I discovered in verifying this, it does not currently give a very helpful error when you try to do this: aws/aws-encryption-sdk-python#21
However, it should be fairly straightforward now (post-1.3.0) to build a cryptographic materials manager that would be able to read these messages. You would just need to subclass DefaultCryptoMaterialsManager
and override the decrypt_materials
method to load the key from an uncompressed point and return a DecryptionMaterials
with the DER-encoded key. We won't be supporting this in the official client, but custom cryptographic materials managers are fully supported by the framework.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattsb42-aws That info is really useful! After reading this thread last night, I was considering how to use the AWS Encryption SDK for Python while still supporting files that have an uncompressed point. It sounds like a custom crypto materials manager will make all that much simpler. Thanks!
No description provided.