fix: Ranged gets with RSA keys #288
Conversation
| byte[] expectedDataCipherName = materials.algorithmSuite().cipherName().getBytes(StandardCharsets.UTF_8); | ||
| if (!Arrays.equals(expectedDataCipherName, dataCipherName)) { | ||
| throw new S3EncryptionClientException("The data cipher does not match the data cipher used for encryption. The object may be altered or corrupted"); | ||
| } |
There was a problem hiding this comment.
I would like to authenticate all the data. So removing the authentication is not the preferred solution. @lucasmcdonald3 and I are looking at options.
There was a problem hiding this comment.
| @Test | ||
| public void AsyncAesGcmV3toV3RangedGet() { | ||
| static Object[] keyMaterialProvider() { | ||
| return new Object[] { |
There was a problem hiding this comment.
nit: for extra coverage, we could add KMS here as well
There was a problem hiding this comment.
Did KMS (symmetric) key...
Are there any other ones we need to test?
There was a problem hiding this comment.
I think that's sufficient, we could possibly add v2 tests to cover each of the various RSA modes, but I don't think we should block on that.
src/test/java/software/amazon/encryption/s3/S3EncryptionClientRangedGetCompatibilityTest.java
Show resolved
Hide resolved
| @@ -156,7 +157,7 @@ private byte[] parsePseudoDataKey(DecryptionMaterials materials, byte[] pseudoDa | |||
| System.arraycopy(pseudoDataKey, 1, dataKey, 0, dataKeyLengthBytes); | |||
| System.arraycopy(pseudoDataKey, 1 + dataKeyLengthBytes, dataCipherName, 0, dataCipherNameLength); | |||
|
|
|||
| byte[] expectedDataCipherName = materials.algorithmSuite().cipherName().getBytes(StandardCharsets.UTF_8); | |||
| byte[] expectedDataCipherName = AlgorithmSuite.ALG_AES_256_GCM_IV12_TAG16_NO_KDF.cipherName().getBytes(StandardCharsets.UTF_8); | |||
There was a problem hiding this comment.
blocking - Does this cause a breaking/backwards-incompatible change when decrypting AES-CBC encrypted content? We might instead prefer to specifically switch from CTR to GCM instead of hard-coding GCM here. Adding tests for RSA / CBC should elucidate this (see other comment below).
There was a problem hiding this comment.
Yeah... I think you're right. Let's rethink what we're doing here. This might be more of a "thing" than we thought, and might fold into a broader conversation on authentication/ranged gets...
There was a problem hiding this comment.
It turns out that only RSA "OAEP" stores the content algorithm inside the data key; this mode was added in v2, which only supports AES-GCM for content encryption. So it's safe enough to assume that data keys decrypted under RSA OAEP are always used for GCM content encryption.
| @@ -146,7 +146,8 @@ private byte[] parsePseudoDataKey(DecryptionMaterials materials, byte[] pseudoDa | |||
| throw new S3EncryptionClientException("Invalid key length (" + dataKeyLengthBytes + ") in encrypted data key"); | |||
| } | |||
|
|
|||
| int dataCipherNameLength = pseudoDataKey.length - dataKeyLengthBytes - 1; | |||
| // int dataCipherNameLength = pseudoDataKey.length - dataKeyLengthBytes - 1; | |||
| int dataCipherNameLength = AlgorithmSuite.ALG_AES_256_GCM_IV12_TAG16_NO_KDF.cipherName().getBytes(StandardCharsets.UTF_8).length; | |||
There was a problem hiding this comment.
nit: this is fine, as this algorithm suite is the only one supported for encryption in this library. However, strictly speaking, we shouldn't need to change the encryption code at all, since the switch from GCM to CTR only happens on decryption.
There was a problem hiding this comment.
This is part of the "authentication" conversation with Ryan
## [3.1.3](v3.1.2...v3.1.3) (2024-06-18) ### Fixes * Ranged gets with RSA keys ([#288](#288)) ([5d7fc31](5d7fc31)) * set bufferSize to 1 when bufferSize is less than or equal to 0 in BoundedStreamBufferer ([#283](#283)) ([adb6d3b](adb6d3b)) ### Maintenance * add support policy ([#236](#236)) ([264168d](264168d))
Issue #, if available:
Description of changes:
Note: The new tests fail if the check is present.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Check any applicable: