Seebees/instruction files by seebees · Pull Request #103 · aws/amazon-s3-encryption-client-python

seebees · 2025-11-17T16:02:57Z

Adding failing instruction file tests

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Update Go for instruction file failures

Update PHP v3 to support failure on bad instruction files

Fail loud!

lucasmcdonald3

The high-level test coverage of ranged gets and re-encrypt LGTM

lucasmcdonald3 · 2025-12-04T23:41:12Z

@@ -1,23 +1,89 @@
+/*


Seems like slop

maybe. I left it on purpose because adding concurrency to cpp was such a PITA

lucasmcdonald3 · 2025-12-05T20:59:17Z

+            );
+        }
+
+        @ParameterizedTest(name = "{0}: Fail to decrypt RSA duplicate commitment with FORBID_ENCRYPT_ALLOW_DECRYPT policy")


Why should explicit FORBID_ENCRYPT_ALLOW_DECRYPT be a separate test from the tests above that don't configure commitment policy? Is this redundant or is it incomplete (i.e. should we remove this, or should we add 2 more explicit tests for the other commitment policies?)

When I first went over the tests, I added it from paranoia. So I would lean towards adding the other copies. But I would prefer to do all this work as we review a complete test plan. As opposed to dealing with individual items here in this already large heap of tests :)

kessplas · 2025-12-05T23:26:35Z

+            .filter(target -> RAW_RSA_SUPPORTED.contains(((LanguageServerTarget) target.get()[0]).getLanguageName()));
+        return Stream.concat(improved, transition);
+    }
+


nit/unrelated to this specific PR: a bunch of the parameterization methods below don't seem to be used e.g.

encryptImprovedDecryptTransition encryptTransitionDecryptImproved encryptImprovedDecryptCurrent

We should clean those up / consolidate client providers if possible.

I agree. I would leave this for the full test plan review. I want to work out a good way to organize things so that our projects with large numbers of tests can be understood.

kessplas · 2025-12-05T23:30:01Z

+
+        @ParameterizedTest(name = "{0}: Improved configured with RequireEncryptRequireDecrypt should encrypt KC-GCM (instruction file)")
+        @MethodSource("software.amazon.encryption.s3.TestUtils#improvedClientsForTest")
+        void improved_configured_with_require_encrypt_require_decrypt_should_encrypt_kc_gcm_ins_file(


Suggested change

void improved_configured_with_require_encrypt_require_decrypt_should_encrypt_kc_gcm_ins_file(

void improved_configured_with_require_encrypt_require_decrypt_should_encrypt_kc_gcm_ins_file_rsa(

kessplas · 2025-12-05T23:30:27Z

+
+        @ParameterizedTest(name = "{0}: Improved configured with RequireEncryptAllowDecrypt should encrypt KC-GCM")
+        @MethodSource("software.amazon.encryption.s3.TestUtils#improvedClientsForTest")
+        void improved_configured_with_require_encrypt_allow_decrypt_should_encrypt_kc_gcm(


Suggested change

void improved_configured_with_require_encrypt_allow_decrypt_should_encrypt_kc_gcm(

void improved_configured_with_require_encrypt_allow_decrypt_should_encrypt_kc_gcm_kms(

kessplas · 2025-12-05T23:31:03Z

+
+        @ParameterizedTest(name = "{0}: Improved configured with RequireEncryptRequireDecrypt should encrypt KC-GCM")
+        @MethodSource("software.amazon.encryption.s3.TestUtils#improvedClientsForTest")
+        void improved_configured_with_require_encrypt_require_decrypt_should_encrypt_kc_gcm(


Suggested change

void improved_configured_with_require_encrypt_require_decrypt_should_encrypt_kc_gcm(

void improved_configured_with_require_encrypt_require_decrypt_should_encrypt_kc_gcm_kms(

kessplas · 2025-12-05T23:31:36Z

+            CreateClientOutput clientOutput = client.createClient(CreateClientInput.builder()
+                .config(S3ECConfig.builder()
+                    .keyMaterial(kmsKeyArn)
+                    // .commitmentPolicy(CommitmentPolicy.FORBID_ENCRYPT_ALLOW_DECRYPT)


Suggested change

// .commitmentPolicy(CommitmentPolicy.FORBID_ENCRYPT_ALLOW_DECRYPT)

I like this -- makes it clear what default should be

kessplas · 2025-12-05T23:31:57Z

+
+        @ParameterizedTest(name = "{0}: Transition configured with the default should decrypt KC-GCM")
+        @MethodSource("software.amazon.encryption.s3.TestUtils#transitionClientsForTest")
+        void transition_configured_with_the_default_should_decrypt_kc_gcm(


Suggested change

void transition_configured_with_the_default_should_decrypt_kc_gcm(

void transition_configured_with_the_default_should_decrypt_kc_gcm_kms(

kessplas · 2025-12-05T23:32:27Z

+
+        @ParameterizedTest(name = "{0}: Transition configured with default should decrypt KC-GCM (instruction file)")
+        @MethodSource("software.amazon.encryption.s3.TestUtils#transitionClientsForTest")
+        void transition_configured_with_require_encrypt_require_decrypt_should_decrypt_kc_gcm_ins_file(


Suggested change

void transition_configured_with_require_encrypt_require_decrypt_should_decrypt_kc_gcm_ins_file(

void transition_configured_with_require_encrypt_require_decrypt_should_decrypt_kc_gcm_ins_file_rsa(

kessplas · 2025-12-05T23:34:54Z

+        String instructionFileSuffix
+    ) {
+        if (crossLanguageObjects.isEmpty()) {
+            throw new AssertionError("There is nothing to decrypt");


nit:

Suggested change

throw new AssertionError("There is nothing to decrypt");

fail("There is nothing to decrypt");

kessplas · 2025-12-05T23:38:46Z

      Set.of(PYTHON_V3);

+    // Languages that support custom instruction file suffix on GetObject
+    // Only Java, Ruby, and PHP servers have been updated with this feature


I believe Go is the only other S3EC implementation that supports custom suffix on Get,
C++ and .NET client don't support it.
We might want to comment that here to make it clear what is a current gap vs. not expected to be implemented.

added "a current gap". I don't know how Go works off the top of my head. I'd like to come back and add Go if it is supported and I missed it.

Go does not support custom instruction file suffixes on get

kessplas · 2025-12-05T23:41:20Z

+                materialsDescription = builder.build();
+            }
+
+            // Check for AES key


a lot of this code is duplicated between here are CreateClientOperation

(and across servers...though that would be a longer term effort)

Yup. I agree. It could be cleaner.

kessplas · 2025-12-05T23:45:24Z

+            S3Client s3Client = clientCache_.get(input.getClientID());
+
+            // Ensure we have an S3EncryptionClient, not just a plain S3Client
+            if (!(s3Client instanceof S3EncryptionClient)) {


muse: I don't think we need to do this now but probably we should have a S3EncryptionClient specific cache and a S3Client cache that validates against S3EC instances being added. In any case I don't think we ever put non-encrypted clients in the cache so this seems somewhat superfluous (but fine I guess)

kessplas · 2025-12-05T23:57:28Z

+            S3EncryptionClient s3EncryptionClient = (S3EncryptionClient) s3Client;
+
+            // Get the keyring from cache and cast to RawKeyring
+            software.amazon.encryption.s3.materials.Keyring cachedKeyring = keyringCache_.get(input.getClientID());


Erm, why does only java-v3-transition use the keyring cache,
(which is the same keyring the client was created with, which doesn't make sense?)
while the other Java servers construct the keyring from the material in the ReEncrypt request?

this is troublesome. I'm going to look more into this. Grrrrrrr.

submodules

kessplas

LGTM

seebees and others added 19 commits November 14, 2025 13:55

ruby spec updates

2b8f937

Start on instruction failures

d0f16e0

Instruction files that pass and fail

496a8db

starting to add raw

54853fa

adding ruby to raw support

f0aa6f7

update names

7c4ddb8

Merge branch 'fireegg-test-servers' into seebees/instruction-files

39c7799

Merge branch 'fireegg-test-servers' into seebees/instruction-files

7c0ef74

speed up the build

aeb359c

speed up wait

270f2d5

update client building

271fcba

fix: Go in instruction files tests (#104)

231e3ad

Update Go for instruction file failures

better failures?

0a4ef95

bump php sdk to fixed instruction file (#106)

4d73663

Update PHP v3 to support failure on bad instruction files

faster parallel tests (#107)

63e0ed1

enable cpp

91933b7

fix c++?

490ffb2

Make the instruction file tests parallel

d1366a9

fix cpp

c7f19ec

seebees force-pushed the seebees/instruction-files branch from 8564447 to c7f19ec Compare November 20, 2025 05:48

seebees and others added 10 commits November 20, 2025 10:19

go concurency

ab1ba3a

cpp is hard

99f05fe

try this

35e8772

next cut

954b559

Enable address sanitizer during tests (#105)

ff1a84a

Fail loud!

try not deleting the body

0858f67

simplify?

5bfc9af

try less work

8d9ff30

try a different direction

03d3151

cpp build update

b4416dc

seebees added 4 commits December 3, 2025 17:15

Some updates

f70745f

Add ReEncrypt to Java V3

cde9779

Strings to constants

05a0cfb

make the makefile fail if it fails to build

12d8fec

lucasmcdonald3 reviewed Dec 5, 2025

View reviewed changes

lucasmcdonald3 previously approved these changes Dec 5, 2025

View reviewed changes

kessplas reviewed Dec 5, 2025

View reviewed changes

Comment thread test-server/java-tests/src/it/java/software/amazon/encryption/s3/InstructionFileFailures.java

kessplas reviewed Dec 5, 2025

View reviewed changes

seebees added 4 commits December 8, 2025 10:58

Updates to merged code

a49ff98

submodules

update Display name

eb00a7b

names and feedback

c3d9b67

ReEncrypt does not cache keyrings

91a24a5

seebees dismissed lucasmcdonald3’s stale review via 91a24a5 December 8, 2025 20:04

Merge branch 'fireegg-test-servers' into seebees/instruction-files

7c0c490

lucasmcdonald3 approved these changes Dec 8, 2025

View reviewed changes

kessplas approved these changes Dec 8, 2025

View reviewed changes

seebees merged commit fe13052 into fireegg-test-servers Dec 8, 2025
4 checks passed

seebees deleted the seebees/instruction-files branch December 8, 2025 22:41

imabhichow mentioned this pull request Dec 16, 2025

chore: add ranged get tests #84

Closed

	void improved_configured_with_require_encrypt_require_decrypt_should_encrypt_kc_gcm_ins_file(
	void improved_configured_with_require_encrypt_require_decrypt_should_encrypt_kc_gcm_ins_file_rsa(

	void transition_configured_with_the_default_should_decrypt_kc_gcm(
	void transition_configured_with_the_default_should_decrypt_kc_gcm_kms(

	void transition_configured_with_require_encrypt_require_decrypt_should_decrypt_kc_gcm_ins_file(
	void transition_configured_with_require_encrypt_require_decrypt_should_decrypt_kc_gcm_ins_file_rsa(

	throw new AssertionError("There is nothing to decrypt");
	fail("There is nothing to decrypt");

Conversation

seebees commented Nov 17, 2025

Uh oh!

lucasmcdonald3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kessplas Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kessplas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kessplas Dec 5, 2025 •

edited

Loading