fix: handle S3 server encoding of non-US-ASCII object metadata (#375)

kessplas · web-flow · commit b90774384dd7 · 2024-10-01T13:35:08.000-07:00
* fix: decode S3 double encoding of non-US-ASCII object metadata
* add separate dependency for MimeUtility
* add for testvectors, and a simple implementation for unicode ec decryption
diff --git a/cfn/S3EC-GitHub-CF-Template.yml b/cfn/S3EC-GitHub-CF-Template.yml
@@ -14,6 +14,20 @@ Resources:
             Action: 'kms:*'
             Resource: '*'
 
+  S3ECGitHubKMSKeyIDTestVectors:
+    Type: 'AWS::KMS::Key'
+    Properties:
+      Description: KMS Key for GitHub Action Workflow
+      Enabled: true
+      KeyPolicy:
+        Version: 2012-10-17
+        Statement:
+          - Effect: Allow
+            Principal:
+              AWS: !Sub 'arn:aws:iam::${AWS::AccountId}:root'
+            Action: 'kms:*'
+            Resource: '*'
+
   S3ECGitHubKMSKeyIDAlternate:
     Type: 'AWS::KMS::Key'
     Properties:
@@ -34,6 +48,12 @@ Resources:
       AliasName: alias/S3EC-Github-KMS-Key
       TargetKeyId: !Ref S3ECGitHubKMSKeyID
 
+  S3ECGitHubKMSKeyAliasTestVectors:
+    Type: 'AWS::KMS::Alias'
+    Properties:
+      AliasName: alias/S3EC-Github-KMS-Key-TestVectors
+      TargetKeyId: !Ref S3ECGitHubKMSKeyIDTestVectors
+
   S3ECGitHubTestS3Bucket:
     Type: 'AWS::S3::Bucket'
     Properties:
@@ -94,6 +114,36 @@ Resources:
             Resource:
               - !Join [ "", [ !GetAtt S3ECGitHubTestS3BucketAlternate.Arn, '/*'] ]
 
+  S3ECGitHubTestS3BucketTestVectors:
+    Type: 'AWS::S3::Bucket'
+    Properties:
+      BucketName: s3ec-github-test-bucket-testvectors
+      PublicAccessBlockConfiguration:
+        BlockPublicAcls: false
+        BlockPublicPolicy: false
+        IgnorePublicAcls: false
+        RestrictPublicBuckets: false
+
+  S3ECGitHubS3BucketPolicyTestVectors:
+    Type: 'AWS::IAM::ManagedPolicy'
+    Properties:
+      ManagedPolicyName: S3EC-GitHub-S3-Bucket-Policy-testvectors
+      PolicyDocument:
+        Version: 2012-10-17
+        Statement:
+          - Effect: Allow
+            Action:
+              - 's3:ListBucket'
+            Resource:
+              - !GetAtt S3ECGitHubTestS3BucketTestVectors.Arn
+          - Effect: Allow
+            Action:
+              - 's3:PutObject'
+              - 's3:GetObject'
+              - 's3:DeleteObject'
+            Resource:
+              - !Join [ "", [ !GetAtt S3ECGitHubTestS3BucketTestVectors.Arn, '/*'] ]
+
   S3ECGitHubKMSKeyPolicy:
     Type: 'AWS::IAM::ManagedPolicy'
     Properties:
@@ -117,6 +167,29 @@ Resources:
         }
       ManagedPolicyName: S3EC-GitHub-KMS-Key-Policy
 
+  S3ECGitHubKMSKeyPolicyTestVectors:
+    Type: 'AWS::IAM::ManagedPolicy'
+    Properties:
+      PolicyDocument: !Sub |
+        {
+          "Version": "2012-10-17",
+          "Statement": [
+            {
+              "Effect": "Allow",
+              "Resource": [
+                "arn:aws:kms:*:${AWS::AccountId}:key/${S3ECGitHubKMSKeyIDTestVectors}",
+                "arn:aws:kms:*:${AWS::AccountId}:${S3ECGitHubKMSKeyAliasTestVectors}"
+              ],
+              "Action": [
+                "kms:Decrypt",
+                "kms:GenerateDataKey",
+                "kms:GenerateDataKeyPair"
+              ]
+            }
+          ]
+        }
+      ManagedPolicyName: S3EC-GitHub-KMS-Key-Policy-TestVectors
+
   S3ECGitHubKMSKeyPolicyAlternate:
     Type: 'AWS::IAM::ManagedPolicy'
     Properties:
@@ -235,7 +308,7 @@ Resources:
         for testing
       ManagedPolicyArns:
         - !Ref S3ECGitHubKMSKeyPolicy
+        - !Ref S3ECGitHubKMSKeyPolicyTestVectors
         - !Ref S3ECGitHubS3BucketPolicy
         - !Ref S3ECGitHubAssumeAlternatePolicy
-
-
+        - !Ref S3ECGitHubS3BucketPolicyTestVectors
diff --git a/cfn/release.yml b/cfn/release.yml
@@ -95,7 +95,9 @@ Resources:
         - !Ref SecretsManagerPolicyRelease
         - !Ref ParameterStorePolicy
         - !Ref S3ECReleaseTestKMSKeyPolicy
+        - !Ref S3ECReleaseTestKMSKeyPolicyTestVectors
         - !Ref S3ECReleaseS3BucketPolicy
+        - !Ref S3ECReleaseS3BucketPolicyTestVectors
         - "arn:aws:iam::aws:policy/AWSCodeArtifactReadOnlyAccess"
         - "arn:aws:iam::aws:policy/AWSCodeArtifactAdminAccess"
 
@@ -269,12 +271,85 @@ Resources:
             Action: 'kms:*'
             Resource: '*'
 
+  S3ECReleaseKMSKeyIDTestVectors:
+    Type: 'AWS::KMS::Key'
+    Properties:
+      Description: KMS Key for S3EC Test Vectors
+      Enabled: true
+      KeyPolicy:
+        Version: 2012-10-17
+        Statement:
+          - Effect: Allow
+            Principal:
+              AWS: !Sub 'arn:aws:iam::${AWS::AccountId}:root'
+            Action: 'kms:*'
+            Resource: '*'
+
+  S3ECReleaseKMSKeyAliasTestVectors:
+    Type: 'AWS::KMS::Alias'
+    Properties:
+      AliasName: alias/S3EC-Release-KMS-Key-TestVectors
+      TargetKeyId: !Ref S3ECReleaseKMSKeyIDTestVectors
+
   S3ECReleaseKMSKeyAlias:
     Type: 'AWS::KMS::Alias'
     Properties:
       AliasName: alias/S3EC-Release-Testing-KMS-Key
       TargetKeyId: !Ref S3ECReleaseTestingKMSKeyID
 
+  S3ECReleaseKMSKeyPolicyTestVectors:
+    Type: 'AWS::IAM::ManagedPolicy'
+    Properties:
+      PolicyDocument: !Sub |
+        {
+          "Version": "2012-10-17",
+          "Statement": [
+            {
+              "Effect": "Allow",
+              "Resource": [
+                "arn:aws:kms:*:${AWS::AccountId}:key/${S3ECReleaseKMSKeyIDTestVectors}",
+                "arn:aws:kms:*:${AWS::AccountId}:${S3ECReleaseKMSKeyAliasTestVectors}"
+              ],
+              "Action": [
+                "kms:Decrypt",
+                "kms:GenerateDataKey",
+                "kms:GenerateDataKeyPair"
+              ]
+            }
+          ]
+        }
+      ManagedPolicyName: S3EC-Release-KMS-Key-Policy-TestVectors
+
+  S3ECReleaseTestS3BucketTestVectors:
+    Type: 'AWS::S3::Bucket'
+    Properties:
+      BucketName: s3ec-release-test-bucket-testvectors
+      PublicAccessBlockConfiguration:
+        BlockPublicAcls: false
+        BlockPublicPolicy: false
+        IgnorePublicAcls: false
+        RestrictPublicBuckets: false
+
+  S3ECReleaseS3BucketPolicyTestVectors:
+    Type: 'AWS::IAM::ManagedPolicy'
+    Properties:
+      ManagedPolicyName: S3EC-Release-S3-Bucket-Policy-testvectors
+      PolicyDocument:
+        Version: 2012-10-17
+        Statement:
+          - Effect: Allow
+            Action:
+              - 's3:ListBucket'
+            Resource:
+              - !GetAtt S3ECReleaseTestS3BucketTestVectors.Arn
+          - Effect: Allow
+            Action:
+              - 's3:PutObject'
+              - 's3:GetObject'
+              - 's3:DeleteObject'
+            Resource:
+              - !Join [ "", [ !GetAtt S3ECReleaseTestS3BucketTestVectors.Arn, '/*'] ]
+
   S3ECReleaseTestS3Bucket:
     Type: 'AWS::S3::Bucket'
     Properties:
diff --git a/pom.xml b/pom.xml
@@ -113,6 +113,12 @@
       <version>1.2</version>
     </dependency>
 
+    <dependency>
+      <groupId>com.sun.xml.messaging.saaj</groupId>
+      <artifactId>saaj-impl</artifactId>
+      <version>2.0.1</version>
+    </dependency>
+
     <!-- Test Dependencies -->
     <!-- https://mvnrepository.com/artifact/org.mockito/mockito-core -->
     <dependency>
diff --git a/src/main/java/software/amazon/encryption/s3/internal/ContentMetadataDecodingStrategy.java b/src/main/java/software/amazon/encryption/s3/internal/ContentMetadataDecodingStrategy.java
@@ -2,6 +2,7 @@
 // SPDX-License-Identifier: Apache-2.0
 package software.amazon.encryption.s3.internal;
 
+import com.sun.xml.messaging.saaj.packaging.mime.internet.MimeUtility;
 import software.amazon.awssdk.core.ResponseInputStream;
 import software.amazon.awssdk.core.async.AsyncResponseTransformer;
 import software.amazon.awssdk.protocols.jsoncore.JsonNode;
@@ -15,6 +16,10 @@
 import software.amazon.encryption.s3.materials.EncryptedDataKey;
 import software.amazon.encryption.s3.materials.S3Keyring;
 
+import java.io.ByteArrayOutputStream;
+import java.io.DataOutputStream;
+import java.io.IOException;
+import java.io.UnsupportedEncodingException;
 import java.nio.charset.StandardCharsets;
 import java.util.Base64;
 import java.util.HashMap;
@@ -135,9 +140,13 @@ private ContentMetadata readFromMap(Map<String, String> metadata, GetObjectRespo
         // Get encrypted data key encryption context
         final Map<String, String> encryptionContext = new HashMap<>();
         final String jsonEncryptionContext = metadata.get(MetadataKeyConstants.ENCRYPTED_DATA_KEY_CONTEXT);
+        // When the encryption context contains non-US-ASCII characters,
+        // the S3 server applies an esoteric encoding to the object metadata.
+        // Reverse that, to allow decryption.
+        final String decodedJsonEncryptionContext = decodeS3CustomEncoding(jsonEncryptionContext);
         try {
             JsonNodeParser parser = JsonNodeParser.create();
-            JsonNode objectNode = parser.parse(jsonEncryptionContext);
+            JsonNode objectNode = parser.parse(decodedJsonEncryptionContext);
 
             for (Map.Entry<String, JsonNode> entry : objectNode.asObject().entrySet()) {
                 encryptionContext.put(entry.getKey(), entry.getValue().asString());
@@ -171,6 +180,46 @@ public ContentMetadata decode(GetObjectRequest request, GetObjectResponse respon
         }
     }
 
+    private static String decodeS3CustomEncoding(final String s) {
+        final String mimeDecoded;
+        try {
+            mimeDecoded = MimeUtility.decodeText(s);
+        } catch (UnsupportedEncodingException ex) {
+            throw new S3EncryptionClientException("Unable to decode S3 object metadata: " + s, ex);
+        }
+        // Once MIME decoded, we need to recover the correct code points from the second encoding pass
+        // Otherwise, decryption fails
+        try {
+            final StringBuilder stringBuilder = new StringBuilder();
+            final ByteArrayOutputStream baos = new ByteArrayOutputStream();
+            final DataOutputStream out = new DataOutputStream(baos);
+            final byte[] sInBytes = mimeDecoded.getBytes(StandardCharsets.UTF_8);
+            final char[] sInChars = mimeDecoded.toCharArray();
+
+            int nonAsciiChars = 0;
+            for (int i = 0; i < sInChars.length; i++) {
+                if (sInChars[i] > 127) {
+                    byte[] buf = {sInBytes[i + nonAsciiChars], sInBytes[i + nonAsciiChars + 1]};
+                    // temporarily re-encode as UTF-8
+                    String wrongString = new String(buf, StandardCharsets.UTF_8);
+                    // write its code point
+                    out.write(wrongString.charAt(0));
+                    nonAsciiChars++;
+                } else {
+                    if (baos.size() > 0) {
+                        // This is not the most efficient, but we prefer to specify UTF_8
+                        stringBuilder.append(new String(baos.toByteArray(), StandardCharsets.UTF_8));
+                        baos.reset();
+                    }
+                    stringBuilder.append(sInChars[i]);
+                }
+            }
+            return stringBuilder.toString();
+        } catch (IOException exception) {
+            throw new S3EncryptionClientException("Unable to decode S3 object metadata: " + s, exception);
+        }
+    }
+
     private ContentMetadata decodeFromObjectMetadata(GetObjectRequest request, GetObjectResponse response) {
         return readFromMap(response.metadata(), response);
     }
diff --git a/src/test/java/software/amazon/encryption/s3/S3AsyncEncryptionClientTest.java b/src/test/java/software/amazon/encryption/s3/S3AsyncEncryptionClientTest.java
@@ -65,6 +65,7 @@
 import static software.amazon.encryption.s3.utils.S3EncryptionClientTestResources.BUCKET;
 import static software.amazon.encryption.s3.utils.S3EncryptionClientTestResources.KMS_KEY_ID;
 import static software.amazon.encryption.s3.utils.S3EncryptionClientTestResources.KMS_REGION;
+import static software.amazon.encryption.s3.utils.S3EncryptionClientTestResources.S3_REGION;
 import static software.amazon.encryption.s3.utils.S3EncryptionClientTestResources.appendTestSuffix;
 import static software.amazon.encryption.s3.utils.S3EncryptionClientTestResources.deleteObject;
 
@@ -89,7 +90,7 @@ public void asyncCustomConfiguration() {
         S3AsyncClient wrappedAsyncClient = S3AsyncClient
                 .builder()
                 .credentialsProvider(creds)
-                .region(Region.of(KMS_REGION.toString()))
+                .region(Region.of(S3_REGION.toString()))
                 .build();
         KmsClient kmsClient = KmsClient
                 .builder()
diff --git a/src/test/java/software/amazon/encryption/s3/S3EncryptionClientTest.java b/src/test/java/software/amazon/encryption/s3/S3EncryptionClientTest.java
@@ -970,6 +970,32 @@ public void s3EncryptionClientMixedCredentialsInstructionFileFails() {
         s3Client.close();
     }
 
+    @Test
+    public void NonUSASCIIMetadataFails() {
+        final String objectKey = appendTestSuffix("non-us-ascii-metadata-fails");
+        final String input = "This is a test.";
+        S3Client v3Client = S3EncryptionClient.builder()
+                .kmsKeyId(KMS_KEY_ALIAS)
+                .build();
+
+        Map<String, String> ec = new HashMap<>(1);
+        ec.put("ec-key", "我的源资源");
+        try {
+            v3Client.putObject(builder -> builder
+                    .bucket(BUCKET)
+                    .key(objectKey)
+                    .overrideConfiguration(withAdditionalConfiguration(ec))
+                    .build(), RequestBody.fromString(input));
+        } catch (S3EncryptionClientException exception) {
+            // The Java SDK does not support writing object metadata
+            // with non-US-ASCII characters.
+            assertTrue(exception.getCause() instanceof S3Exception);
+        }
+
+        // Cleanup
+        v3Client.close();
+    }
+
     /**
      * A simple, reusable round-trip (encryption + decryption) using a given
      * S3Client. Useful for testing client configuration.
diff --git a/src/test/java/software/amazon/encryption/s3/S3EncryptionClientTestVectorsTest.java b/src/test/java/software/amazon/encryption/s3/S3EncryptionClientTestVectorsTest.java
diff --git a/src/test/java/software/amazon/encryption/s3/utils/S3EncryptionClientTestResources.java b/src/test/java/software/amazon/encryption/s3/utils/S3EncryptionClientTestResources.java
