diff --git a/.duvet/.gitignore b/.duvet/.gitignore new file mode 100644 index 000000000..93956e36d --- /dev/null +++ b/.duvet/.gitignore @@ -0,0 +1,3 @@ +reports/ +requirements/ +specification/ \ No newline at end of file diff --git a/.duvet/config.toml b/.duvet/config.toml new file mode 100644 index 000000000..e11fed05a --- /dev/null +++ b/.duvet/config.toml @@ -0,0 +1,27 @@ +'$schema' = "https://awslabs.github.io/duvet/config/v0.4.0.json" + +[[source]] +pattern = "src/**/*.java" + +# Include required specifications here +[[specification]] +source = "specification/s3-encryption/client.md" +[[specification]] +source = "specification/s3-encryption/decryption.md" +[[specification]] +source = "specification/s3-encryption/encryption.md" +[[specification]] +source = "specification/s3-encryption/key-commitment.md" +[[specification]] +source = "specification/s3-encryption/key-derivation.md" +[[specification]] +source = "specification/s3-encryption/data-format/content-metadata.md" +[[specification]] +source = "specification/s3-encryption/data-format/metadata-strategy.md" + +[report.html] +enabled = true + +# Enable snapshots to prevent requirement coverage regressions +[report.snapshot] +enabled = false diff --git a/.github/workflows/duvet.yml b/.github/workflows/duvet.yml new file mode 100644 index 000000000..366348689 --- /dev/null +++ b/.github/workflows/duvet.yml @@ -0,0 +1,44 @@ +name: duvet + +on: + workflow_call: + # Optional inputs that can be provided when calling this workflow + +jobs: + test: + runs-on: macos-latest + permissions: + id-token: write + contents: read + pages: write + + steps: + - name: Checkout code + uses: actions/checkout@v5 + with: + submodules: true + + - name: Setup Rust toolchain + uses: actions-rust-lang/setup-rust-toolchain@v1 + with: + toolchain: stable + + - name: Clone duvet repository + run: git clone https://github.com/awslabs/duvet.git /tmp/duvet + + - name: Build and install duvet + run: | + cd /tmp/duvet + cargo xtask build + cargo install --path ./duvet + + - name: Run duvet + run: make duvet + + - name: Upload duvet reports + uses: actions/upload-artifact@v4 + with: + name: reports + include-hidden-files: true + path: .duvet/reports/report.html + diff --git a/.gitmodules b/.gitmodules index b4e128ffa..68883895d 100644 --- a/.gitmodules +++ b/.gitmodules @@ -1,3 +1,4 @@ -[submodule "specification"] +[submodule "private_aws"] path = specification - url = git@github.com:awslabs/aws-encryption-sdk-specification.git \ No newline at end of file + url = https://github.com/awslabs/aws-encryption-sdk-specification.git + branch = tonyknap/s3ec-v3.0.1-candidate diff --git a/CONTEXT_FOR_DUVET.md b/CONTEXT_FOR_DUVET.md new file mode 100644 index 000000000..ba28f3c6b --- /dev/null +++ b/CONTEXT_FOR_DUVET.md @@ -0,0 +1,56 @@ +# Duvet Annotation Context + +## Overview +Working through incomplete Duvet requirements one at a time to add citations and tests to the codebase. + +## Key Insights +- **Duvet syntax**: Only comments with exactly two slashes (`//`) are read by Duvet + - `//=` for specification links + - `//#` for requirement descriptions + - `///` or `////` are NOT read by Duvet (effectively commented out) +- **Annotation types**: `type=implication`, `type=exception`, `type=test` +- **Report location**: `.duvet/reports/report.html` +- **Total requirements**: 198 (178 complete, 20 incomplete as of last run) + +## Completed Requirements + +### 1. V1 Format Exclusive Keys +**Requirement**: "When the object is encrypted using the V1 format, - Mapkeys exclusive to other format versions MUST NOT be present." + +**Citation added**: +- `MetadataKeyConstants.isV1Format()` at line 73 +- `ContentMetadataDecodingStrategy.isV1InObjectMetadata()` at line 424 + +**Test**: Already tested via `ContentMetadataStrategyTest.testExclusiveKeysCollision` + +### 2. V2 Format Tag Length Requirements +**Requirements**: +- "If the object is encrypted using AES-GCM for content encryption, then the mapkey 'x-amz-tag-len' MUST be present." +- "If the object is encrypted using AES-CBC for content encryption, then the mapkey 'x-amz-tag-len' MUST NOT be present." + +**Bug fixed**: Code was incorrectly writing tag length for CBC (should only be for GCM) + +**Citation added**: `ContentMetadataEncodingStrategy.addMetadataToMap()` at line 190 + +**Implementation**: Check `cipherName().contains("GCM")` to determine whether to write tag length + +**Test**: Existing tests validate tag length is written for GCM and read correctly + +## Commit History +- `9068c876`: fix(CBC): Add annotations for V1/V2 format requirements and fix CBC tag length bug + +## Files Modified +- `src/main/java/software/amazon/encryption/s3/internal/MetadataKeyConstants.java` +- `src/main/java/software/amazon/encryption/s3/internal/ContentMetadataEncodingStrategy.java` +- `src/main/java/software/amazon/encryption/s3/internal/ContentMetadataDecodingStrategy.java` + +## Process for Next Requirements +1. User provides incomplete requirement text +2. Find where requirement is implemented in code +3. Add Duvet annotation at implementation location +4. Verify test coverage exists +5. Run tests: `mvn clean compile` then `mvn test -Dtest=ContentMetadataStrategyTest,MetadataKeyConstantsTest,CipherProviderTest,AlgorithmSuiteValidationTest` +6. Stage and commit changes + +## Remaining Work +18 incomplete requirements remaining (as of last count) diff --git a/Makefile b/Makefile index 1c60b7a64..1288d27da 100644 --- a/Makefile +++ b/Makefile @@ -1,15 +1,12 @@ # Used for misc supporting functions like Duvet and prettier. Builds, tests, etc. should use the usual Java/Maven tooling. -duvet: | duvet_extract duvet_report - -duvet_extract: - rm -rf compliance - $(foreach file, $(shell find specification/s3-encryption -name '*.md'), duvet extract -o compliance -f MARKDOWN $(file);) +duvet: | duvet_clean duvet_report duvet_report: - duvet \ - report \ - --spec-pattern "compliance/**/*.toml" \ - --source-pattern "src/**/*.java" \ - --source-pattern "compliance_exceptions/*.txt" \ - --html specification_compliance_report.html + duvet report + +duvet-view-report-mac: + open .duvet/reports/report.html + +duvet_clean: + rm -rf .duvet/reports/ .duvet/requirements/ diff --git a/specification b/specification index 280a89401..2e1710a6b 160000 --- a/specification +++ b/specification @@ -1 +1 @@ -Subproject commit 280a894019cd1b4efc6b16cfb233bf1ec21bc508 +Subproject commit 2e1710a6b305484612951bc97985fd15c80f5823 diff --git a/src/main/java/software/amazon/encryption/s3/internal/CipherProvider.java b/src/main/java/software/amazon/encryption/s3/internal/CipherProvider.java index f6e2aa53e..2bb23c037 100644 --- a/src/main/java/software/amazon/encryption/s3/internal/CipherProvider.java +++ b/src/main/java/software/amazon/encryption/s3/internal/CipherProvider.java @@ -193,6 +193,17 @@ public static Cipher createAndInitCipher(final CryptographicMaterials materials, if (materials.cipherMode().opMode() == Cipher.ENCRYPT_MODE) { throw new S3EncryptionClientException("Encryption is not supported for algorithm: " + materials.algorithmSuite().cipherName()); } + //= specification/s3-encryption/decryption.md#cbc-decryption + //# If an object is encrypted with ALG_AES_256_CBC_IV16_NO_KDF and [legacy unauthenticated algorithm suites](#legacy-decryption) is enabled, + //# then the S3EC MUST create a cipher with AES in CBC Mode with PKCS5Padding or PKCS7Padding compatible padding for a 16-byte block cipher (example: for the Java JCE, this is "AES/CBC/PKCS5Padding"). + // NOTE: CBC and PKCS5Padding is specified above in CryptoFactory.createCipher(materials.algorithmSuite().cipherName(), materials.cryptoProvider()) + //= specification/s3-encryption/decryption.md#cbc-decryption + //# If the cipher object cannot be created as described above, + //# Decryption MUST fail. + //= specification/s3-encryption/decryption.md#cbc-decryption + //= type=exception + //# The error SHOULD detail why the cipher could not be initialized + //# (such as CBC or PKCS5Padding is not supported by the underlying crypto provider). cipher.init(materials.cipherMode().opMode(), materials.dataKey(), new IvParameterSpec(iv)); break; case ALG_AES_256_CTR_HKDF_SHA512_COMMIT_KEY: diff --git a/src/main/java/software/amazon/encryption/s3/internal/ContentMetadataDecodingStrategy.java b/src/main/java/software/amazon/encryption/s3/internal/ContentMetadataDecodingStrategy.java index baa819aae..60fc4ac56 100644 --- a/src/main/java/software/amazon/encryption/s3/internal/ContentMetadataDecodingStrategy.java +++ b/src/main/java/software/amazon/encryption/s3/internal/ContentMetadataDecodingStrategy.java @@ -92,6 +92,8 @@ private static ContentMetadata readFromV3FormatMap(Map metadata, if (!MetadataKeyConstants.isV3Format(metadata)) { //= specification/s3-encryption/data-format/content-metadata.md#determining-s3ec-object-status //# In general, if there is any deviation from the above format, with the exception of additional unrelated mapkeys, then the S3EC SHOULD throw an exception. + //= specification/s3-encryption/data-format/content-metadata.md#content-metadata-mapkeys + //# - If a mapkey exclusive to other (non-V3) format versions is present, the S3EC SHOULD throw an exception. throw new S3EncryptionClientException("Content metadata is tampered, required metadata to decrypt the object are missing"); } @@ -227,9 +229,6 @@ private static ContentMetadata readFromMapV1V2(Map metadata, Get //= specification/s3-encryption/data-format/content-metadata.md#content-metadata-mapkeys //= type=exception //# - The mapkey "x-amz-unencrypted-content-length" SHOULD be present for V1 format objects. - //= specification/s3-encryption/data-format/content-metadata.md#content-metadata-mapkeys - - //# - The mapkey "x-amz-cek-alg" MUST be present for V2 format objects. if (contentEncryptionAlgorithm == null || contentEncryptionAlgorithm.equals(AlgorithmSuite.ALG_AES_256_CBC_IV16_NO_KDF.cipherName())) { algorithmSuite = AlgorithmSuite.ALG_AES_256_CBC_IV16_NO_KDF; @@ -257,14 +256,27 @@ private static ContentMetadata readFromMapV1V2(Map metadata, Get //= specification/s3-encryption/data-format/content-metadata.md#algorithm-suite-and-message-format-version-compatibility //# Objects encrypted with ALG_AES_256_CBC_IV16_NO_KDF MAY use either the V1 or V2 message format version. if (metadata.containsKey(MetadataKeyConstants.ENCRYPTED_DATA_KEY_V1)) { + //= specification/s3-encryption/data-format/content-metadata.md#content-metadata-mapkeys + //# - If mapkeys exclusive to other (non-V1) format versions is present,the S3EC SHOULD throw an exception. + if (isV2InObjectMetadata(metadata) || isV3InObjectMetadata(metadata)) { + throw new S3EncryptionClientException("Object metadata is tampered, conflicting keys are present."); + } //= specification/s3-encryption/data-format/content-metadata.md#content-metadata-mapkeys //# - The mapkey "x-amz-key" MUST be present for V1 format objects. edkCiphertext = DECODER.decode(metadata.get(MetadataKeyConstants.ENCRYPTED_DATA_KEY_V1)); } else if (metadata.containsKey(MetadataKeyConstants.ENCRYPTED_DATA_KEY_V2)) { + //= specification/s3-encryption/data-format/content-metadata.md#content-metadata-mapkeys + //# - If a mapkey exclusive to other (non-V2) format versions is present, the S3EC SHOULD throw an exception. + if (isV1InObjectMetadata(metadata) || isV3InObjectMetadata(metadata)) { + throw new S3EncryptionClientException("Object metadata is tampered, conflicting keys are present."); + } //= specification/s3-encryption/data-format/content-metadata.md#algorithm-suite-and-message-format-version-compatibility //# Objects encrypted with ALG_AES_256_CBC_IV16_NO_KDF MAY use either the V1 or V2 message format version. //= specification/s3-encryption/data-format/content-metadata.md#content-metadata-mapkeys //# - The mapkey "x-amz-key-v2" MUST be present for V2 format objects. + //= specification/s3-encryption/data-format/content-metadata.md#content-metadata-mapkeys + //= type=exception + //# - The mapkey "x-amz-unencrypted-content-length" SHOULD be present for V2 format objects. edkCiphertext = DECODER.decode(metadata.get(MetadataKeyConstants.ENCRYPTED_DATA_KEY_V2)); } else { // this shouldn't happen under normal circumstances- only if out-of-band modification @@ -303,8 +315,6 @@ private static ContentMetadata readFromMapV1V2(Map metadata, Get break; case ALG_AES_256_GCM_IV12_TAG16_NO_KDF: case ALG_AES_256_CTR_IV16_TAG16_NO_KDF: - //= specification/s3-encryption/data-format/content-metadata.md#content-metadata-mapkeys - //# - The mapkey "x-amz-tag-len" MUST be present for V2 format objects. final int tagLength = Integer.parseInt(metadata.get(MetadataKeyConstants.CONTENT_CIPHER_TAG_LENGTH)); if (tagLength != algorithmSuite.cipherTagLengthBits()) { throw new S3EncryptionClientException("Expected tag length (bits) of: " @@ -419,17 +429,27 @@ public Map loadInstructionFileMetadata(GetObjectRequest request) } /** - * Determines if V1/V2 format is present in object metadata. - * All V1/V2 keys must be present in object metadata. + * Determines if V1 format is present in object metadata. */ - public static boolean isV1V2InObjectMetadata(Map objectMetadata) { + public static boolean isV1InObjectMetadata(Map objectMetadata) { //= specification/s3-encryption/data-format/content-metadata.md#determining-s3ec-object-status - //# - If the metadata contains "x-amz-iv" and "x-amz-key" then the object MUST be considered as an S3EC-encrypted object using the V1 format. + //# - If the metadata contains "x-amz-iv" and "x-amz-key" but no other version exclusive keys then the object MUST be considered as an S3EC-encrypted object using the V1 format. + return objectMetadata.containsKey(MetadataKeyConstants.CONTENT_IV) + && objectMetadata.containsKey(MetadataKeyConstants.ENCRYPTED_DATA_KEY_V1) + && !objectMetadata.containsKey(MetadataKeyConstants.ENCRYPTED_DATA_KEY_V2) + && !objectMetadata.containsKey(MetadataKeyConstants.ENCRYPTED_DATA_KEY_V3); + } + + /** + * Determines if V2 format is present in object metadata. + */ + public static boolean isV2InObjectMetadata(Map objectMetadata) { //= specification/s3-encryption/data-format/content-metadata.md#determining-s3ec-object-status - //# - If the metadata contains "x-amz-iv" and "x-amz-metadata-x-amz-key-v2" then the object MUST be considered as an S3EC-encrypted object using the V2 format. + //# - If the metadata contains "x-amz-iv" and "x-amz-key-v2" but no other version exclusive keys then the object MUST be considered as an S3EC-encrypted object using the V2 format. return objectMetadata.containsKey(MetadataKeyConstants.CONTENT_IV) - && (objectMetadata.containsKey(MetadataKeyConstants.ENCRYPTED_DATA_KEY_V1) - || objectMetadata.containsKey(MetadataKeyConstants.ENCRYPTED_DATA_KEY_V2)); + && objectMetadata.containsKey(MetadataKeyConstants.ENCRYPTED_DATA_KEY_V2) + && !objectMetadata.containsKey(MetadataKeyConstants.ENCRYPTED_DATA_KEY_V1) + && !objectMetadata.containsKey(MetadataKeyConstants.ENCRYPTED_DATA_KEY_V3); } /** @@ -438,7 +458,7 @@ public static boolean isV1V2InObjectMetadata(Map objectMetadata) */ public static boolean isV3InObjectMetadata(Map objectMetadata) { //= specification/s3-encryption/data-format/content-metadata.md#determining-s3ec-object-status - //# - If the metadata contains "x-amz-3" and "x-amz-d" and "x-amz-i" then the object MUST be considered an S3EC-encrypted object using the V3 format. + //# - If the metadata contains "x-amz-3" and "x-amz-d" and "x-amz-i" but no other version exclusive keys then the object MUST be considered an S3EC-encrypted object using the V3 format. return objectMetadata.containsKey(MetadataKeyConstants.ENCRYPTED_DATA_KEY_V3) && objectMetadata.containsKey(MetadataKeyConstants.KEY_COMMITMENT_V3) && objectMetadata.containsKey(MetadataKeyConstants.MESSAGE_ID_V3) @@ -511,24 +531,25 @@ public ContentMetadata decodeV3FromInstructionFile(GetObjectRequest request, Get public ContentMetadata decode(GetObjectRequest request, GetObjectResponse response) { Map objectMetadata = response.metadata(); - - //= specification/s3-encryption/data-format/content-metadata.md#determining-s3ec-object-status - //= type=exception - //# If there are multiple mapkeys which are meant to be exclusive, such as "x-amz-key", "x-amz-key-v2", and "x-amz-3" then the S3EC SHOULD throw an exception. - if (objectMetadata != null) { + //= specification/s3-encryption/data-format/content-metadata.md#determining-s3ec-object-status + //# If there are multiple mapkeys which are meant to be exclusive to different versions, such as "x-amz-key", "x-amz-key-v2", and "x-amz-3" then the S3EC SHOULD throw an exception. + if (MetadataKeyConstants.hasExclusiveKeyCollision(objectMetadata)) { + throw new S3EncryptionClientException("Content metadata is tampered, required metadata combination is illegal."); + } + // V1/V2 in Object Metadata - All V1/V2 keys present in object metadata //= specification/s3-encryption/data-format/content-metadata.md#determining-s3ec-object-status - //# - If the metadata contains "x-amz-iv" and "x-amz-key" then the object MUST be considered as an S3EC-encrypted object using the V1 format. + //# - If the metadata contains "x-amz-iv" and "x-amz-key" but no other version exclusive keys then the object MUST be considered as an S3EC-encrypted object using the V1 format. //= specification/s3-encryption/data-format/content-metadata.md#determining-s3ec-object-status - //# - If the metadata contains "x-amz-iv" and "x-amz-metadata-x-amz-key-v2" then the object MUST be considered as an S3EC-encrypted object using the V2 format. - if (isV1V2InObjectMetadata(objectMetadata)) { + //# - If the metadata contains "x-amz-iv" and "x-amz-key-v2" but no other version exclusive keys then the object MUST be considered as an S3EC-encrypted object using the V2 format. + if (isV1InObjectMetadata(objectMetadata) || isV2InObjectMetadata(objectMetadata)) { return readFromMapV1V2(objectMetadata, response); } // V3 in Object Metadata - c/d/i always in object metadata, x-amz-3 also in object metadata //= specification/s3-encryption/data-format/content-metadata.md#determining-s3ec-object-status - //# - If the metadata contains "x-amz-3" and "x-amz-d" and "x-amz-i" then the object MUST be considered an S3EC-encrypted object using the V3 format. + //# - If the metadata contains "x-amz-3" and "x-amz-d" and "x-amz-i" but no other version exclusive keys then the object MUST be considered an S3EC-encrypted object using the V3 format. //= specification/s3-encryption/data-format/content-metadata.md#content-metadata-mapkeys //# In the V3 format, the mapkeys "x-amz-c", "x-amz-d", and "x-amz-i" MUST be stored exclusively in the Object Metadata. else if (isV3InObjectMetadata(objectMetadata)) { diff --git a/src/main/java/software/amazon/encryption/s3/internal/ContentMetadataEncodingStrategy.java b/src/main/java/software/amazon/encryption/s3/internal/ContentMetadataEncodingStrategy.java index 13efa210a..39696f1cb 100644 --- a/src/main/java/software/amazon/encryption/s3/internal/ContentMetadataEncodingStrategy.java +++ b/src/main/java/software/amazon/encryption/s3/internal/ContentMetadataEncodingStrategy.java @@ -9,6 +9,7 @@ import java.util.HashMap; import java.util.Map; +import edu.umd.cs.findbugs.annotations.NonNull; import software.amazon.awssdk.protocols.jsoncore.JsonWriter; import software.amazon.awssdk.services.s3.model.CreateMultipartUploadRequest; import software.amazon.awssdk.services.s3.model.PutObjectRequest; @@ -53,6 +54,11 @@ public PutObjectRequest encodeMetadata(EncryptionMaterials materials, byte[] iv, } else { //= specification/s3-encryption/data-format/metadata-strategy.md#object-metadata //# By default, the S3EC MUST store content metadata in the S3 Object Metadata. + //= specification/s3-encryption/data-format/content-metadata.md#us-ascii-preferred-string + //= type=exception + //= reason=It would be a breaking change to introduce this. + //# Thus, + //# Content Metadata MapKeys SHOULD be restricted to US-ASCII. Map newMetadata = addMetadataToMap(putObjectRequest.metadata(), materials, iv); return putObjectRequest.toBuilder() .metadata(newMetadata) @@ -79,6 +85,11 @@ public CreateMultipartUploadRequest encodeMetadata(EncryptionMaterials materials return createMultipartUploadRequest.toBuilder() .metadata(objectMetadata).build(); } else { + //= specification/s3-encryption/data-format/content-metadata.md#us-ascii-preferred-string + //= type=exception + //= reason=It would be a breaking change to introduce this. + //# Thus, + //# Content Metadata MapKeys SHOULD be restricted to US-ASCII. Map newMetadata = addMetadataToMap(createMultipartUploadRequest.metadata(), materials, iv); return createMultipartUploadRequest.toBuilder() .metadata(newMetadata) @@ -158,6 +169,8 @@ private Map addMetadataToMapV3(Map map, Encrypti jsonWriter.writeFieldName(entry.getKey()).writeValue(entry.getValue()); } jsonWriter.writeEndObject(); + //= specification/s3-encryption/data-format/content-metadata.md#us-ascii-preferred-string + //# An implementation MAY support UTF-8. String jsonEncryptionContext = new String(jsonWriter.getBytes(), StandardCharsets.UTF_8); //= specification/s3-encryption/data-format/metadata-strategy.md#v3-instruction-files //# - The V3 message format MUST store the mapkey "x-amz-t" and its value (when present in the content metadata) in the Instruction File. @@ -168,6 +181,8 @@ private Map addMetadataToMapV3(Map map, Encrypti jsonWriter.writeFieldName(entry.getKey()).writeValue(entry.getValue()); } jsonWriter.writeEndObject(); + //= specification/s3-encryption/data-format/content-metadata.md#us-ascii-preferred-string + //# An implementation MAY support UTF-8. String jsonEncryptionContext = new String(jsonWriter.getBytes(), StandardCharsets.UTF_8); //= specification/s3-encryption/data-format/metadata-strategy.md#v3-instruction-files //# - The V3 message format MUST store the mapkey "x-amz-m" and its value (when present in the content metadata) in the Instruction File. @@ -178,16 +193,33 @@ private Map addMetadataToMapV3(Map map, Encrypti } return metadata; } + private Map addMetadataToMap(Map map, EncryptionMaterials materials, byte[] iv) { if (materials.algorithmSuite().isCommitting()) { return addMetadataToMapV3(map, materials, iv); } + return addMetadataToMapV2(map, materials, iv); + } + + @NonNull + private Map addMetadataToMapV2(Map map, EncryptionMaterials materials, byte[] iv) { Map metadata = new HashMap<>(map); EncryptedDataKey edk = materials.encryptedDataKeys().get(0); metadata.put(MetadataKeyConstants.ENCRYPTED_DATA_KEY_V2, ENCODER.encodeToString(edk.encryptedDatakey())); metadata.put(MetadataKeyConstants.CONTENT_IV, ENCODER.encodeToString(iv)); metadata.put(MetadataKeyConstants.CONTENT_CIPHER, materials.algorithmSuite().cipherName()); - metadata.put(MetadataKeyConstants.CONTENT_CIPHER_TAG_LENGTH, Integer.toString(materials.algorithmSuite().cipherTagLengthBits())); + // When the object is encrypted using the V2 format: + //= specification/s3-encryption/data-format/content-metadata.md#content-metadata-mapkeys + //# - The mapkey "x-amz-tag-len" MAY be present for V2 format objects. + //= specification/s3-encryption/data-format/content-metadata.md#content-metadata-mapkeys + //# - If the object is encrypted using AES-GCM for content encryption, then the the mapkey "x-amz-tag-len" MUST be present. + //= specification/s3-encryption/data-format/content-metadata.md#content-metadata-mapkeys + //# - If the object is encrypted using AES-CBC for content encryption, then the the mapkey "x-amz-tag-len" MUST NOT be present. + if (materials.algorithmSuite().cipherName().contains("GCM")) { + metadata.put(MetadataKeyConstants.CONTENT_CIPHER_TAG_LENGTH, Integer.toString(materials.algorithmSuite().cipherTagLengthBits())); + } else { + throw new S3EncryptionClientException("Only AES-GCM encryption is supported for encryption. AES-CBC is deprecated."); + } metadata.put(MetadataKeyConstants.ENCRYPTED_DATA_KEY_ALGORITHM, edk.keyProviderInfo()); try (JsonWriter jsonWriter = JsonWriter.create()) { @@ -202,11 +234,16 @@ private Map addMetadataToMap(Map map, Encryption } } jsonWriter.writeEndObject(); + //= specification/s3-encryption/data-format/content-metadata.md#us-ascii-preferred-string + //# An implementation MAY support UTF-8. String jsonEncryptionContext = new String(jsonWriter.getBytes(), StandardCharsets.UTF_8); metadata.put(MetadataKeyConstants.ENCRYPTED_DATA_KEY_MATDESC_OR_EC, jsonEncryptionContext); } catch (JsonWriter.JsonGenerationException e) { throw new S3EncryptionClientException("Cannot serialize encryption context to JSON.", e); } + //= specification/s3-encryption/data-format/content-metadata.md#content-metadata-mapkeys + //= type=exception + //# - The mapkey "x-amz-unencrypted-content-length" SHOULD be present for V2 format objects. return metadata; } } diff --git a/src/main/java/software/amazon/encryption/s3/internal/GetEncryptedObjectPipeline.java b/src/main/java/software/amazon/encryption/s3/internal/GetEncryptedObjectPipeline.java index 92fd59019..c298b3636 100644 --- a/src/main/java/software/amazon/encryption/s3/internal/GetEncryptedObjectPipeline.java +++ b/src/main/java/software/amazon/encryption/s3/internal/GetEncryptedObjectPipeline.java @@ -96,6 +96,9 @@ private DecryptionMaterials prepareMaterialsFromRequest(final GetObjectRequest g //= specification/s3-encryption/decryption.md#legacy-decryption //# If the S3EC is not configured to enable legacy unauthenticated content decryption, the client MUST throw //# an exception when attempting to decrypt an object encrypted with a legacy unauthenticated algorithm suite. + //= specification/s3-encryption/decryption.md#cbc-decryption + //# If an object is encrypted with ALG_AES_256_CBC_IV16_NO_KDF and [legacy unauthenticated algorithm suites](#legacy-decryption) is NOT enabled, + //# the S3EC MUST throw an error which details that client was not configured to decrypt objects with ALG_AES_256_CBC_IV16_NO_KDF. throw new S3EncryptionClientException("Enable legacy unauthenticated modes to use legacy content decryption: " + algorithmSuite.cipherName()); } diff --git a/src/main/java/software/amazon/encryption/s3/internal/MetadataKeyConstants.java b/src/main/java/software/amazon/encryption/s3/internal/MetadataKeyConstants.java index 8f3bd0a51..38e15039b 100644 --- a/src/main/java/software/amazon/encryption/s3/internal/MetadataKeyConstants.java +++ b/src/main/java/software/amazon/encryption/s3/internal/MetadataKeyConstants.java @@ -71,10 +71,11 @@ public class MetadataKeyConstants { public static boolean isV1Format(Map metadata) { return metadata.containsKey(CONTENT_IV) && metadata.containsKey(ENCRYPTED_DATA_KEY_MATDESC_OR_EC) && - //= specification/s3-encryption/data-format/content-metadata.md#determining-s3ec-object-status - //# If there are multiple mapkeys which are meant to be exclusive, such as "x-amz-key", "x-amz-key-v2", and "x-amz-3" then the S3EC SHOULD throw an exception. metadata.containsKey(ENCRYPTED_DATA_KEY_V1) && - !metadata.containsKey(ENCRYPTED_DATA_KEY_V2) && + ///= specification/s3-encryption/data-format/content-metadata.md#content-metadata-mapkeys + ///= type=implication + ///# - Mapkeys exclusive to other format versions MUST NOT be present. + !metadata.containsKey(ENCRYPTED_DATA_KEY_V2) && !metadata.containsKey(ENCRYPTED_DATA_KEY_V3); } @@ -85,8 +86,6 @@ public static boolean isV2Format(Map metadata) { // TODO-Post-Pentest: Objects copied without x-amz-matdesc was able be decrypted by V2 Client. // Should this mapkey be SHOULD instead of MUST? // metadata.containsKey(ENCRYPTED_DATA_KEY_MATDESC_OR_EC) && - //= specification/s3-encryption/data-format/content-metadata.md#determining-s3ec-object-status - //# If there are multiple mapkeys which are meant to be exclusive, such as "x-amz-key", "x-amz-key-v2", and "x-amz-3" then the S3EC SHOULD throw an exception. metadata.containsKey(ENCRYPTED_DATA_KEY_V2) && !metadata.containsKey(ENCRYPTED_DATA_KEY_V1) && !metadata.containsKey(ENCRYPTED_DATA_KEY_V3); @@ -98,14 +97,24 @@ public static boolean isV3Format(Map metadata) { metadata.containsKey(ENCRYPTED_DATA_KEY_ALGORITHM_V3) && metadata.containsKey(KEY_COMMITMENT_V3) && metadata.containsKey(MESSAGE_ID_V3) && - //= specification/s3-encryption/data-format/content-metadata.md#determining-s3ec-object-status - //# If there are multiple mapkeys which are meant to be exclusive, such as "x-amz-key", "x-amz-key-v2", and "x-amz-3" then the S3EC SHOULD throw an exception. metadata.containsKey(ENCRYPTED_DATA_KEY_V3) && !metadata.containsKey(ENCRYPTED_DATA_KEY_V2) && !metadata.containsKey(ENCRYPTED_DATA_KEY_V1); } + /** + * Checks if multiple version-exclusive keys are present in metadata. + */ + public static boolean hasExclusiveKeyCollision(Map metadata) { + boolean hasV1Key = metadata.containsKey(ENCRYPTED_DATA_KEY_V1); + boolean hasV2Key = metadata.containsKey(ENCRYPTED_DATA_KEY_V2); + boolean hasV3Key = metadata.containsKey(ENCRYPTED_DATA_KEY_V3); + + int exclusiveKeyCount = (hasV1Key ? 1 : 0) + (hasV2Key ? 1 : 0) + (hasV3Key ? 1 : 0); + return exclusiveKeyCount > 1; + } + /** * Compresses a wrapping algorithm name to its V3 format. */ diff --git a/src/test/java/software/amazon/encryption/s3/S3EncryptionClientBuilderValidationTest.java b/src/test/java/software/amazon/encryption/s3/S3EncryptionClientBuilderValidationTest.java index 9d39c4ebf..c544035f3 100644 --- a/src/test/java/software/amazon/encryption/s3/S3EncryptionClientBuilderValidationTest.java +++ b/src/test/java/software/amazon/encryption/s3/S3EncryptionClientBuilderValidationTest.java @@ -181,6 +181,10 @@ public void testBuilderWithLegacyAlgorithmFails() throws NoSuchAlgorithmExceptio S3EncryptionClientException exception = assertThrows(S3EncryptionClientException.class, () -> S3EncryptionClient.builderV4() .aesKey(aesKey) + //= specification/s3-encryption/decryption.md#cbc-decryption + //= type=test + //# If an object is encrypted with ALG_AES_256_CBC_IV16_NO_KDF and [legacy unauthenticated algorithm suites](#legacy-decryption) is NOT enabled, + //# the S3EC MUST throw an error which details that client was not configured to decrypt objects with ALG_AES_256_CBC_IV16_NO_KDF. .encryptionAlgorithm(AlgorithmSuite.ALG_AES_256_CBC_IV16_NO_KDF) .build() ); diff --git a/src/test/java/software/amazon/encryption/s3/S3EncryptionClientCommitmentPolicyTest.java b/src/test/java/software/amazon/encryption/s3/S3EncryptionClientCommitmentPolicyTest.java index cfcaf452c..63e441f71 100644 --- a/src/test/java/software/amazon/encryption/s3/S3EncryptionClientCommitmentPolicyTest.java +++ b/src/test/java/software/amazon/encryption/s3/S3EncryptionClientCommitmentPolicyTest.java @@ -177,7 +177,7 @@ public void testCommitmentPolicyForbidEncryptAllowDecrypt() { //= specification/s3-encryption/key-commitment.md#commitment-policy //= type=test //# When the commitment policy is FORBID_ENCRYPT_ALLOW_DECRYPT, the S3EC MUST NOT encrypt using an algorithm suite which supports key commitment. - assertTrue(ContentMetadataDecodingStrategy.isV1V2InObjectMetadata(metadata)); + assertTrue(ContentMetadataDecodingStrategy.isV1InObjectMetadata(metadata) || ContentMetadataDecodingStrategy.isV2InObjectMetadata(metadata)); assertFalse(ContentMetadataDecodingStrategy.isV3InObjectMetadata(metadata)); assertEquals(metadata.get(MetadataKeyConstants.CONTENT_CIPHER), AlgorithmSuite.ALG_AES_256_GCM_IV12_TAG16_NO_KDF.cipherName()); @@ -251,7 +251,7 @@ public void testCommitmentPolicyRequireEncryptAllowDecrypt() { //= type=test //# When the commitment policy is REQUIRE_ENCRYPT_ALLOW_DECRYPT, the S3EC MUST only encrypt using an algorithm suite which supports key commitment. assertTrue(ContentMetadataDecodingStrategy.isV3InObjectMetadata(metadata)); - assertFalse(ContentMetadataDecodingStrategy.isV1V2InObjectMetadata(metadata)); + assertFalse(ContentMetadataDecodingStrategy.isV1InObjectMetadata(metadata) || ContentMetadataDecodingStrategy.isV2InObjectMetadata(metadata)); assertEquals(metadata.get(MetadataKeyConstants.CONTENT_CIPHER_V3), AlgorithmSuite.ALG_AES_256_GCM_HKDF_SHA512_COMMIT_KEY.idAsString()); //= specification/s3-encryption/key-commitment.md#commitment-policy @@ -323,7 +323,7 @@ public void testCommitmentPolicyRequireEncryptRequireDecrypt() { //= type=test //# When the commitment policy is REQUIRE_ENCRYPT_REQUIRE_DECRYPT, the S3EC MUST only encrypt using an algorithm suite which supports key commitment. assertTrue(ContentMetadataDecodingStrategy.isV3InObjectMetadata(metadata)); - assertFalse(ContentMetadataDecodingStrategy.isV1V2InObjectMetadata(metadata)); + assertFalse(ContentMetadataDecodingStrategy.isV1InObjectMetadata(metadata) || ContentMetadataDecodingStrategy.isV2InObjectMetadata(metadata)); assertEquals(metadata.get(MetadataKeyConstants.CONTENT_CIPHER_V3), AlgorithmSuite.ALG_AES_256_GCM_HKDF_SHA512_COMMIT_KEY.idAsString()); //= specification/s3-encryption/key-commitment.md#commitment-policy diff --git a/src/test/java/software/amazon/encryption/s3/internal/CipherProviderTest.java b/src/test/java/software/amazon/encryption/s3/internal/CipherProviderTest.java index 92fc79d89..464abe2e9 100644 --- a/src/test/java/software/amazon/encryption/s3/internal/CipherProviderTest.java +++ b/src/test/java/software/amazon/encryption/s3/internal/CipherProviderTest.java @@ -300,6 +300,21 @@ public void testCreateAndInitCipherALG_AES_256_CBC_IV16_NO_KDF_DecryptionSucceed Cipher cipher = CipherProvider.createAndInitCipher(materials, iv, messageId); assertNotNull(cipher); + //= specification/s3-encryption/decryption.md#cbc-decryption + //= type=exception + //= reason=Well cipher creation failure does throw an error, we do not catch the error and throw a reasonable message + //# If the cipher object cannot be created as described above, + //# Decryption MUST fail. + //= specification/s3-encryption/decryption.md#cbc-decryption + //= type=TODO + //# The error SHOULD detail why the cipher could not be initialized + //# (such as CBC or PKCS5Padding is not supported by the underlying crypto provider). + // If we ever refactor so that the cipher creation failure is a modeled error, we should add tests for it. + + //= specification/s3-encryption/decryption.md#cbc-decryption + //= type=test + //# If an object is encrypted with ALG_AES_256_CBC_IV16_NO_KDF and [legacy unauthenticated algorithm suites](#legacy-decryption) is enabled, + //# then the S3EC MUST create a cipher with AES in CBC Mode with PKCS5Padding or PKCS7Padding compatible padding for a 16-byte block cipher (example: for the Java JCE, this is "AES/CBC/PKCS5Padding"). assertEquals("AES/CBC/PKCS5Padding", cipher.getAlgorithm()); } diff --git a/src/test/java/software/amazon/encryption/s3/internal/ContentMetadataStrategyTest.java b/src/test/java/software/amazon/encryption/s3/internal/ContentMetadataStrategyTest.java index eea456a09..a0fda00e8 100644 --- a/src/test/java/software/amazon/encryption/s3/internal/ContentMetadataStrategyTest.java +++ b/src/test/java/software/amazon/encryption/s3/internal/ContentMetadataStrategyTest.java @@ -63,9 +63,6 @@ public void setUp() { @Test public void testDetectV1Format() { - //= specification/s3-encryption/data-format/content-metadata.md#determining-s3ec-object-status - //= type=test - //# - If the metadata contains "x-amz-iv" and "x-amz-key" then the object MUST be considered as an S3EC-encrypted object using the V1 format. Map metadata = new HashMap<>(); metadata.put("x-amz-iv", "dGVzdC1pdi0xMi1i"); // base64 of "test-iv-12-b" metadata.put("x-amz-key", "ZW5jcnlwdGVkLWtleS1kYXRh"); // base64 of "encrypted-key-data" @@ -430,6 +427,25 @@ public void testMissingKeysV1() { assertTrue(exception.getMessage().contains("Content metadata is tampered, required metadata to decrypt the object are missing")); } + @Test + public void testMissingKeysV1Colliding() { + Map metadata = new HashMap<>(); + metadata.put("x-amz-iv", "dGVzdC1pdi0xMi1i"); // base64 of "test-iv-12-b" + metadata.put("x-amz-key", "ZW5jcnlwdGVkLWtleS1kYXRh"); // base64 of "encrypted-key-data" + metadata.put("x-amz-matdesc", "{}"); + metadata.put("x-amz-key-v2", "ZW5jcnlwdGVkLWtleS1kYXRh"); + + GetObjectResponse response = GetObjectResponse.builder() + .metadata(metadata) + .build(); + //= specification/s3-encryption/data-format/content-metadata.md#content-metadata-mapkeys + //= type=test + //# - If mapkeys exclusive to other (non-V1) format versions is present,the S3EC SHOULD throw an exception. + S3EncryptionClientException exception = assertThrows(S3EncryptionClientException.class, () -> decodingStrategy.decode(getObjectRequest, response)); + System.out.println(exception.getMessage()); + assertTrue(exception.getMessage().contains("Content metadata is tampered, required metadata combination is illegal.")); + } + @Test public void testExclusiveKeysCollision() { Map metadata = new HashMap<>(); @@ -447,11 +463,8 @@ public void testExclusiveKeysCollision() { .metadata(metadata) .build(); - //= specification/s3-encryption/data-format/content-metadata.md#determining-s3ec-object-status - //= type=test - //# If there are multiple mapkeys which are meant to be exclusive, such as "x-amz-key", "x-amz-key-v2", and "x-amz-3" then the S3EC SHOULD throw an exception. S3EncryptionClientException exception = assertThrows(S3EncryptionClientException.class, () -> decodingStrategy.decode(getObjectRequest, response)); - assertTrue(exception.getMessage().contains("Content metadata is tampered, required metadata to decrypt the object are missing")); + assertTrue(exception.getMessage().contains("Content metadata is tampered, required metadata combination is illegal")); } @Test @@ -515,9 +528,7 @@ static Stream provideMetadataFormatDetection() { //= type=test //# - The mapkey "x-amz-cek-alg" MUST be present for V2 format objects. v2Metadata.put("x-amz-cek-alg", "AES/GCM/NoPadding"); - //= specification/s3-encryption/data-format/content-metadata.md#content-metadata-mapkeys - //= type=test - //# - The mapkey "x-amz-tag-len" MUST be present for V2 format objects. + v2Metadata.put("x-amz-tag-len", "128"); Map v2CbcMetadata = new HashMap<>(); @@ -541,9 +552,7 @@ static Stream provideMetadataFormatDetection() { //= type=test //# - The mapkey "x-amz-cek-alg" MUST be present for V2 format objects. v2CbcMetadata.put("x-amz-cek-alg", "AES/CBC/PKCS5Padding"); - //= specification/s3-encryption/data-format/content-metadata.md#content-metadata-mapkeys - //= type=test - //# - The mapkey "x-amz-tag-len" MUST be present for V2 format objects. + v2CbcMetadata.put("x-amz-tag-len", "128"); @@ -573,30 +582,19 @@ static Stream provideMetadataFormatDetection() { //= specification/s3-encryption/data-format/content-metadata.md#algorithm-suite-and-message-format-version-compatibility //= type=test //# Objects encrypted with ALG_AES_256_CBC_IV16_NO_KDF MAY use either the V1 or V2 message format version. - //= specification/s3-encryption/data-format/content-metadata.md#determining-s3ec-object-status - //= type=test - //# - If the metadata contains "x-amz-iv" and "x-amz-key" then the object MUST be considered as an S3EC-encrypted object using the V1 format. + Arguments.of(v1Metadata, "V1", AlgorithmSuite.ALG_AES_256_CBC_IV16_NO_KDF), //= specification/s3-encryption/data-format/content-metadata.md#algorithm-suite-and-message-format-version-compatibility //= type=test //# Objects encrypted with ALG_AES_256_CBC_IV16_NO_KDF MAY use either the V1 or V2 message format version. - //= specification/s3-encryption/data-format/content-metadata.md#determining-s3ec-object-status - //= type=test - //# - If the metadata contains "x-amz-iv" and "x-amz-metadata-x-amz-key-v2" then the object MUST be considered as an S3EC-encrypted object using the V2 format. Arguments.of(v2CbcMetadata, "V2", AlgorithmSuite.ALG_AES_256_CBC_IV16_NO_KDF), //= specification/s3-encryption/data-format/content-metadata.md#algorithm-suite-and-message-format-version-compatibility //= type=test //# Objects encrypted with ALG_AES_256_GCM_IV12_TAG16_NO_KDF MUST use the V2 message format version only. - //= specification/s3-encryption/data-format/content-metadata.md#determining-s3ec-object-status - //= type=test - //# - If the metadata contains "x-amz-iv" and "x-amz-metadata-x-amz-key-v2" then the object MUST be considered as an S3EC-encrypted object using the V2 format. Arguments.of(v2Metadata, "V2", AlgorithmSuite.ALG_AES_256_GCM_IV12_TAG16_NO_KDF), //= specification/s3-encryption/data-format/content-metadata.md#algorithm-suite-and-message-format-version-compatibility //= type=test //# Objects encrypted with ALG_AES_256_GCM_HKDF_SHA512_COMMIT_KEY MUST use the V3 message format version only. - //= specification/s3-encryption/data-format/content-metadata.md#determining-s3ec-object-status - //= type=test - //# - If the metadata contains "x-amz-3" and "x-amz-d" and "x-amz-i" then the object MUST be considered an S3EC-encrypted object using the V3 format. Arguments.of(v3Metadata, "V3", AlgorithmSuite.ALG_AES_256_GCM_HKDF_SHA512_COMMIT_KEY) ); }