Skip to content

fix: Ranged gets with RSA keys #288

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jun 12, 2024
Merged

fix: Ranged gets with RSA keys #288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
25dd424
fix: Ranged gets with RSA keys
lucasmcdonald3 Jun 4, 2024
a13ef23
checkstyle
lucasmcdonald3 Jun 4, 2024
ca2deef
test
lucasmcdonald3 Jun 5, 2024
900d2a9
more tests
lucasmcdonald3 Jun 11, 2024
e32a03e
more tests
lucasmcdonald3 Jun 11, 2024
653653e
more tests
lucasmcdonald3 Jun 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/main/java/software/amazon/encryption/s3/internal/ContentMetadataStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,12 @@ private static ContentMetadata readFromMap(Map<String, String> metadata, GetObje
|| contentEncryptionAlgorithm.equals(AlgorithmSuite.ALG_AES_256_CBC_IV16_NO_KDF.cipherName())) {
algorithmSuite = AlgorithmSuite.ALG_AES_256_CBC_IV16_NO_KDF;
} else if (contentEncryptionAlgorithm.equals(AlgorithmSuite.ALG_AES_256_GCM_IV12_TAG16_NO_KDF.cipherName())) {
algorithmSuite = (contentRange == null ) ? AlgorithmSuite.ALG_AES_256_GCM_IV12_TAG16_NO_KDF : AlgorithmSuite.ALG_AES_256_CTR_IV16_TAG16_NO_KDF;
// If contentRange is provided, this is a ranged get.
// ranged gets require legacy unauthenticated modes.
// Change AES-GCM to AES-CTR to disable authentication when reading this message.
algorithmSuite = (contentRange == null)
? AlgorithmSuite.ALG_AES_256_GCM_IV12_TAG16_NO_KDF
: AlgorithmSuite.ALG_AES_256_CTR_IV16_TAG16_NO_KDF;
} else {
throw new S3EncryptionClientException(
"Unknown content encryption algorithm: " + contentEncryptionAlgorithm);
Expand Down
9 changes: 5 additions & 4 deletions src/main/java/software/amazon/encryption/s3/materials/RsaKeyring.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
package software.amazon.encryption.s3.materials;

import software.amazon.encryption.s3.S3EncryptionClientException;
import software.amazon.encryption.s3.algorithms.AlgorithmSuite;
import software.amazon.encryption.s3.internal.CryptoFactory;

import javax.crypto.Cipher;
Expand Down Expand Up @@ -117,8 +118,7 @@ public byte[] encryptDataKey(SecureRandom secureRandom,

// Create a pseudo-data key with the content encryption appended to the data key
byte[] dataKey = materials.plaintextDataKey();
byte[] dataCipherName = materials.algorithmSuite().cipherName().getBytes(
StandardCharsets.UTF_8);
byte[] dataCipherName = AlgorithmSuite.ALG_AES_256_GCM_IV12_TAG16_NO_KDF.cipherName().getBytes(StandardCharsets.UTF_8);
byte[] pseudoDataKey = new byte[1 + dataKey.length + dataCipherName.length];

pseudoDataKey[0] = (byte)dataKey.length;
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is part of the "authentication" conversation with Ryan

if (dataCipherNameLength <= 0) {
throw new S3EncryptionClientException("Invalid data cipher name length (" + dataCipherNameLength + ") in encrypted data key");
}
Expand All @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

@lucasmcdonald3 lucasmcdonald3 Jun 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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...

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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");
}
Expand Down
Loading