feat: reEncryptInstructionFile Implementation #478
Conversation
* I created the MaterialsDescription class to help identify AES + RSA keys for reEncryptInstructionFile feature Co-authored-by: Anirav Kareddy <[email protected]>
--------- Co-authored-by: Anirav Kareddy <[email protected]>
Implemented a stronger level of validation for reEncryptInstructionFile where, when enabled, the client will attempt to decrypt the re-encrypted instruction file with the old key material and throw an exception when decryption succeeds. This is a stronger level of validation that the wrapping key has been rotated than the standard assertion that the materials descriptions are different. --------- Co-authored-by: Anirav Kareddy <[email protected]>
akareddy04
changed the title
| .materialsDescription( | ||
| MaterialsDescription | ||
| .builder() | ||
| .put("version", "1.0") | ||
| .put("rotated", "no") | ||
| .build() |
There was a problem hiding this comment.
As a customer, do I need to set this? If my keyring doesn't already set this, do I need to go back and update this? How is this used?
(I think I already know the answers but you could add code comments answering this for customers reading this example)
| .put("isOwner", "yes") | ||
| .put("access-level", "admin") |
There was a problem hiding this comment.
Can you 1) expand on the scenario you describe in this example's javadoc, then 2) add comments explaining how these attributes connect to the example?
You could also clarify that this is just example metadata; this example only uses keys to enforce access, not this metadata.
|
|
||
| // If enforceRotation is set to true, ensure that the old keyring cannot decrypt the newly encrypted data key | ||
| if (reEncryptInstructionFileRequest.enforceRotation()) { | ||
| enforceRotation(encryptedMaterials, request); |
There was a problem hiding this comment.
nit: IMO, enforce doesn't seem right -- to me, it implies this method is taking some action to perform the rotation. But it's really just validating that the rotation was done correctly.
I think verify or validate would have been better, but it seems like a fair amount of work to change. I'll leave it up to you/Kess to decide if this rename makes sense and/or is worth the effort to change.
There was a problem hiding this comment.
Kess and I discussed this and we decided to keep enforceRotation since it is an optional attribute in the reEncryptInstructionFile request and customers are only interacting with it by setting the boolean value. The internal method "enforceRotation" is not being called by the client and so the API isn't being affected.
There was a problem hiding this comment.
I meant that we should change enforce anywhere it exists, including in the request object, since it isn't a great word to describe the effect of setting that flag. But this is fine as-is.
| .s3Request(request) | ||
| .build() | ||
| ); | ||
| } catch (S3EncryptionClientException e) { |
There was a problem hiding this comment.
Is this catch too general? Could there be other S3Exceptions that are surfaced for other reasons that would get swallowed here?
My (very mild) concern would be that some unrelated S3Exception gets raised, then this method swallows it and raises no exception, indicating "rotation was done correctly".
Let me know if this isn't a valid concern.
There was a problem hiding this comment.
The catch won't be too general and it would fail closed. This is because we already validated the DecryptionMaterials after we decrypted the original keyring in the CMM and so in "onDecrypt" all that will be checked is whether the old keyring can decrypt the data key and if it is able to, then "key rotation" did not happen.
## [3.4.0](v3.3.5...v3.4.0) (2025-07-30) ### Features * put object with instruction file configured ([#466](#466)) ([99077dc](99077dc)) * reEncryptInstructionFile Implementation ([#475](#475)) ([ff66e72](ff66e72)) * reEncryptInstructionFile Implementation ([#478](#478)) ([f7e6fa5](f7e6fa5)) ### Fixes * Revert "feat: reEncryptInstructionFile Implementation ([#475](#475))" ([#477](#477)) ([6d45ec5](6d45ec5)) ### Maintenance * guard against properties conflicts ([#479](#479)) ([793c73b](793c73b)) * **pom:** fix scm url ([#469](#469)) ([1bc2ca3](1bc2ca3)) * **release:** Migrate release to Central Portal ([#468](#468)) ([da71231](da71231)) * validate against legacy wrapping on client but customer passes keyring with no legacy wrapping ([#473](#473)) ([bb898d1](bb898d1))
Description of changes:
Implemented the following feature: re-encrypting an instruction file with a new keyring while preserving the original encrypted object in S3. This enables:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Check any applicable: