Skip to content

ReEncryptInstructionFile Implementation #470

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 41 commits into
base: aniravk/ReEncrypt-feature
Choose a base branch
from

Conversation

akareddy04
Copy link
Contributor

Issue #, if available:

Description of changes:
Implemented the reEncryptInstructionFile feature and I also modified the current API to help support this.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Check any applicable:

  • Were any files moved? Moving files changes their URL, which breaks all hyperlinks to the files.

Anirav Kareddy added 21 commits June 17, 2025 15:16
…on files with AES + RSA keyrings and both of these tests pass
… third-party client is unable toretrieve the encrypted object in s3 if they do not include the overrideConfiguration with their custom instruction file suffix since it will by default use the original instruction file, not theirs.:
…ion object, not the object itself. Also reworded one of the exception messages to be clearer for when instruction file suffix request is specified for AES keyring (which should not happen)
@akareddy04 akareddy04 requested a review from a team as a code owner July 7, 2025 21:17
@akareddy04 akareddy04 changed the base branch from main to aniravk/ReEncrypt-feature July 7, 2025 21:17
Anirav Kareddy added 14 commits July 10, 2025 21:10
… support both the default and custom instruction file suffixes
… since secureRandom should now be initialized by default. I also removed all the unused imports
…file suffix + legacy wrapping algorithm upgrades from V1/V2 to V3
…eSuffix to mention that the . prefix is automatically added to the suffix + in reEncryptInstructionFileResponse, the . prefix will be removed from the suffix and return what the user requested the suffix be
Copy link
Contributor

@kessplas kessplas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some nits about comments.

* This example demonstrates generating a custom instruction file to enable access to encrypted object by a third party.
* This enables secure sharing of encrypted objects without sharing private keys.
* This example demonstrates re-encrypting the encrypted data key in an instruction file with a new RSA wrapping key.
* The other cryptographic parameters in the instruction file such as the IV and wrapping algorithm remain unchanged.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* The other cryptographic parameters in the instruction file such as the IV and wrapping algorithm remain unchanged.

This refers to an implementation detail that customers shouldn't need to worry about. It's a useful comment for internal stuff but not necessary in the examples.

assertTrue(e.getMessage().contains("Unable to RSA-OAEP-SHA1 unwrap"));
}

// Create a new client with the rotated AES key
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Create a new client with the rotated AES key
// Create a new client with the rotated RSA key

* - Default instruction file suffix required for AES keyrings
* - Legacy to V3: Can rotate same wrapping key from legacy wrapping algorithms to fully supported wrapping algorithms
* - Within V3: When rotating the wrapping key, the new keyring must be different from the current keyring
* - Enforce Rotation: When enabled, ensures old keyring cannot decrypt data encrypted by new keyring
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* - Enforce Rotation: When enabled, ensures old keyring cannot decrypt data encrypted by new keyring

nit: this PR doesn't have Enforce Rotation yet

*
* @param reEncryptInstructionFileRequest the request containing bucket, object key, new keyring, and optional instruction file suffix
* @return ReEncryptInstructionFileResponse containing the bucket, object key, and instruction file suffix used
* @throws S3EncryptionClientException if the new keyring has the same materials description as the current one
*/
public ReEncryptInstructionFileResponse reEncryptInstructionFile(ReEncryptInstructionFileRequest reEncryptInstructionFileRequest) {
GetObjectRequest request = GetObjectRequest.builder()
//GetObjectRequest MUST be kept the same
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//GetObjectRequest MUST be kept the same

these comments aren't terribly helpful, final enforces this

EncryptedDataKey originalEncryptedDataKeys = contentMetadata.encryptedDataKey();
Map<String, String> currentKeyringMaterialsDescription = contentMetadata.encryptedDataKeyContext();
byte[] iv = contentMetadata.contentIv();
// Algorithm Suite MUST be kept the same
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto, redundant comment (each of these)

.s3Request(request)
.build()
);
byte[] plaintextDataKey = decryptedMaterials.plaintextDataKey();

//Plaintext Data Key MUST be kept the same
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto, redundant comment

EncryptionMaterials encryptedMaterials = newKeyring.onEncrypt(encryptionMaterials);

Map<String, String> newMaterialsDescription = encryptedMaterials.materialsDescription().getMaterialsDescription();
//New Keyring MUST be kept the same
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto, redundant comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants