chore: validate against legacy wrapping on client but customer passes keyring with no legacy wrapping #473
Conversation
Amazon S3 Encryption Client 3.3.5 Release -- 2025-05-20
…Algorithms on the S3 Encryption Client while passing in a keyring that does not have it enabled will fail
…Algorithm was enabled or not for keyring. From there, in the S3EncryptionClient Builder, I checked whether enableLegacyWrappingAlgorithm was set, but was not set for the keyring that was passed; if not, an exception is thrown. The test cases I wrote illustrates this
akareddy04
changed the title
…tButCustomerPassesKeyring
| /** | ||
| * @return true if legacy wrapping algorithms are enabled, false otherwise | ||
| */ | ||
| public boolean enableLegacyWrappingAlgorithms() { |
There was a problem hiding this comment.
| public boolean enableLegacyWrappingAlgorithms() { | |
| public boolean areLegacyWrappingAlgorithmsEnabled() { |
| if (_enableLegacyWrappingAlgorithms && _keyring !=null && _keyring instanceof S3Keyring) { | ||
| S3Keyring keyring = (S3Keyring) _keyring; | ||
| if (!keyring.enableLegacyWrappingAlgorithms()) { | ||
| throw new S3EncryptionClientException("Legacy wrapping algorithms are not enabled for this keyring"); |
There was a problem hiding this comment.
this error message can be much more helpful, maybe something like:
"enableLegacyWrappingAlgorithms is set on the client, but is not set on the keyring provided. In order to enable legacy wrapping algorithms, set enableLegacyWrappingAlgorithms to true in the keyring's builder."
also....
i'm debating if we want to throw an exception here as it would be a breaking change.
it's better to log a warning.
let's do that instead.
| } | ||
|
|
||
| @Test | ||
| public void validateAgainstSettingLegacyWrappingOnClientWithKmsKeyringPassedV1toV3() { |
There was a problem hiding this comment.
this test name doesn't make sense
| @@ -976,4 +981,71 @@ public void nullMaterialDescriptionV3() { | |||
| v3Client.close(); | |||
|
|
|||
| } | |||
|
|
|||
There was a problem hiding this comment.
can we also add a simple test (for each keyring) that when _enableLegacyWrappingAlgorithms is set on the keyring and the client that there's no error?
also if we're logging instead,
it's a bit harder to test that.
it should be possible though,
we can chat offline
| @@ -1068,6 +1069,12 @@ public S3EncryptionClient build() { | |||
| if (!onlyOneNonNull(_cryptoMaterialsManager, _keyring, _aesKey, _rsaKeyPair, _kmsKeyId)) { | |||
| throw new S3EncryptionClientException("Exactly one must be set of: crypto materials manager, keyring, AES key, RSA key pair, KMS key id"); | |||
| } | |||
| if (_enableLegacyWrappingAlgorithms && _keyring !=null && _keyring instanceof S3Keyring) { | |||
There was a problem hiding this comment.
I think we want to break out the _keyring instanceof S3Keyring check;
if the customer passes a custom keyring (i.e. not an S3Keyring instance),
then setting _enableLegacyWrappingAlgorithms on the client is still useless.
we can't check keyring.enableLegacyWrappingAlgorithms() so we should log in any case.
…ilties in the dependency + most of the dependencies latest versions only support java 11 and we are currently only supporting java 8
akareddy04
changed the title
kessplas
deleted the
aniravk/validateAgainstLegacyWrappingOnClientButCustomerPassesKeyring
branch
## [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))
Issue #, if available:
Description of changes:
Added validation for the case where enableLegacyWrappingAlgorithms is set on the client, but customer passes in a keyring to S3EC builder where enableLegacyWrappingAlgorithms is not set on keyring.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Check any applicable: