chore: Cdk plus basic ci test by kessplas · Pull Request #1 · aws/amazon-s3-encryption-client-python

kessplas · 2025-08-06T22:26:34Z

Issue #, if available:

Description of changes:

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

lucasmcdonald3

Initial skim, I'd also recommend adding linters sooner rather than later. I think the "cool" linters these days are a combination of ruff and black: https://github.com/aws/aws-database-encryption-sdk-dynamodb/blob/python-poc/DynamoDbEncryption/runtimes/python/pyproject.toml#L59-L61

lucasmcdonald3 · 2025-08-06T23:58:20Z

+# There is a newer version, but MPL wants this one.
+cryptography = "^43.0.1"


Suggested change

# There is a newer version, but MPL wants this one.

cryptography = "^43.0.1"

This is included transitively via MPL

Ok -- given that you don't need the MPL let me also save you from a mistake I made and set this as a range

Suggested change

# There is a newer version, but MPL wants this one.

cryptography = "^43.0.1"

# There is a newer version, but MPL wants this one.

cryptography = ">=45.0.1, <46"

the "^43.0.1" syntax resolves to >=43.0.1, <44 which restricts customers.

When launching MPL-Python, we decided the best thing to do was to drop support for as many older versions as possible of cryptography, so ideally we're launching with the newest support possible. It's easy to decrease the minimum version later on but hard to increase it.

Do we need to specify an upper bound on future major versions? I feel like it's reasonable to trust the future MVs will be backwards compatible (unless there's a serious security issue in which case we'd need to do some work to upgrade). Any reason we shouldn't just go with cryptography = ">=45.0.6"?

lucasmcdonald3 · 2025-08-06T23:59:29Z

+boto3 = "^1.37.2"
+# There is a newer version, but MPL wants this one.
+cryptography = "^43.0.1"
+aws-cryptographic-material-providers = "^1.7.4"


Suggested change

aws-cryptographic-material-providers = "^1.7.4"

aws-cryptographic-material-providers = "^1.11.1"

Bump to latest when you get a chance, there are traps in earlier versions

Wait -- does this need the MPL?

No, it doesn't, not sure how/why it ended up there. Will remove.

lucasmcdonald3 · 2025-08-07T00:00:08Z

+cryptography = "^43.0.1"
+aws-cryptographic-material-providers = "^1.7.4"
+attrs = "^25.1.0"
+pytest = "^8.4.1"


Include this as a test dependency -- check out DBESDK for a good example here

https://github.com/aws/aws-database-encryption-sdk-dynamodb/blob/python-poc/DynamoDbEncryption/runtimes/python/pyproject.toml

lucasmcdonald3 · 2025-08-07T00:01:21Z

@@ -0,0 +1,22 @@
+[tool.poetry]


You might consider using uv instead of poetry. All the cool projects are using uv these days: https://github.com/astral-sh/uv

poetry is not a great package management system, but it solved some needs specific to Dafny-Python issues. Since you're not generating any Dafny code, you can probably branch out.

lucasmcdonald3 · 2025-08-07T00:05:38Z

+    # TODO: I don't know exactly how boto3 works,
+    # we maybe instead prefer only using kwargs?
+    # Do we need to provide specific arg overloads?


Imo, ideally any clients we write should be able to act as drop-in replacements for the boto3 equivalents, which means just using kwargs.

lucasmcdonald3 · 2025-08-07T00:06:12Z

+from typing import List, Dict, Any, Union
+
+# API Stub for CMM
+class AbstractCryptoMaterialsManager():


consider https://docs.python.org/3/library/abc.html

thanks, now I know my abc's

lucasmcdonald3 · 2025-08-07T00:11:07Z

+from attrs import define
+from .keyring import AbstractKeyring
+from .materials import EncryptionMaterials, DecryptionMaterials
+from typing import List, Dict, Any, Union


You can use the built-int dict and list types now (3.9+)

lucasmcdonald3

Just a few more notes

lucasmcdonald3 · 2025-08-07T22:44:35Z

+
+# API Stub for CMM
+class AbstractCryptoMaterialsManager:
+    def getEncryptionMaterials(self, encMatsRequest):


style nit that I'm sure a linter will pick up -- snakecase methods and field names

Suggested change

def getEncryptionMaterials(self, encMatsRequest):

def get_encryption_materials(self, enc_mats_request):

lucasmcdonald3 · 2025-08-08T19:09:23Z

+        encrypted_data_key (bytes): The encrypted data key
+    """
+
+    key_provider_info: str = field()


I think the field()s are redundant on a class annotated with define? Not terribly familiar with this though, and you may have plans to expand the behavior in the field calls here

that's correct, since none of these has a default, we can remove them

lucasmcdonald3 · 2025-08-08T19:14:21Z

+# There is a newer version, but MPL wants this one.
+cryptography = "^43.0.1"


Ok -- given that you don't need the MPL let me also save you from a mistake I made and set this as a range

Suggested change

# There is a newer version, but MPL wants this one.

cryptography = "^43.0.1"

# There is a newer version, but MPL wants this one.

cryptography = ">=45.0.1, <46"

the "^43.0.1" syntax resolves to >=43.0.1, <44 which restricts customers.

When launching MPL-Python, we decided the best thing to do was to drop support for as many older versions as possible of cryptography, so ideally we're launching with the newest support possible. It's easy to decrease the minimum version later on but hard to increase it.

lucasmcdonald3 · 2025-08-08T19:19:21Z

+    config: S3EncryptionClientConfig = field()
+
+    # TODO: rename Data-> Body to match boto
+    def put_object(self, Bucket, Key, Data, EncryptionContext=None, **kwargs):


Opening a debate here: I'd suggest just this instead:

Suggested change

def put_object(self, Bucket, Key, Data, EncryptionContext=None, **kwargs):

def put_object(self, **kwargs):

imo, our clients' API arguments should align with the boto3 API arguments.
Note that I might expect customers to provide EncryptionContext just in kwargs.
I could be wrong here -- what you have seems totally fine but I think it's worth discussing.

lucasmcdonald3 · 2025-08-08T19:21:31Z

+
+@define
+class KmsKeyring(S3Keyring):
+    kms_client = field()


Just a note that I've used botocore.client.BaseClient for the boto3 client typehint before

ideally it'd be a KmsClient specifically,
but i suppose BaseClient is better than nothing.

nvm, it breaks things: https://github.com/aws/amazon-s3-encryption-client-python/actions/runs/16893374403/job/47858202780

I think it's actually some default arg shenanigans from line 58 in those logs:

self = KmsKeyring(kms_key_id=<botocore.client.KMS object at 0x7f38961ff5d0>, enable_legacy_wrapping_algorithms='arn:aws:kms:us-west-2:370957321024:alias/S3EC-Python-Github-KMS-Key')

lucasmcdonald3 · 2025-08-08T19:24:01Z

+# Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
+# SPDX-License-Identifier: Apache-2.0
+
+import unittest


I might suggest pytest instead of unittest:

more robust feature support

we have pretty extensive examples of tests using pytest in ESDK and DBESDK doing some complicated things (ex parametrization/matrices, fixtures) so if you ever want more complicated behavior in tests you can just refer to those

lucasmcdonald3 · 2025-08-08T19:25:00Z

@@ -0,0 +1,656 @@
+# This file is automatically @generated by Poetry 1.7.1 and should not be changed by hand.


You should gitignore this (and other files)

lucasmcdonald3 · 2025-08-12T16:44:34Z

+        # Extract required parameters from kwargs
+        bucket = kwargs.pop("Bucket")
+        key = kwargs.pop("Key")
+        body = kwargs.pop("Body")


This implies Body is a required parameter, but boto3 docs imply it is optional: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/s3/client/put_object.html

Is this intentional?

Good call, as senseless as it seems, omitting the Body produces a 0 byte object. This is technically fine to do with AES-GCM encryption, so we should support it.

lucasmcdonald3 · 2025-08-12T16:47:18Z

+        """Get encryption materials from the keyring.
+
+        Args:
+            enc_mats_request (Dict[str, Any]): Request containing encryption parameters


Suggested change

enc_mats_request (Dict[str, Any]): Request containing encryption parameters

enc_mats_request (Dict[str, Any] or EncryptionMaterials): Request containing encryption parameters

I think this type syntax is still a little off, but you might need to play around with pydocs to find the right syntax

lucasmcdonald3 · 2025-08-12T16:51:08Z

+
+@define
+class KmsKeyring(S3Keyring):
+    kms_client = field()


I think it's actually some default arg shenanigans from line 58 in those logs:

self = KmsKeyring(kms_key_id=<botocore.client.KMS object at 0x7f38961ff5d0>, enable_legacy_wrapping_algorithms='arn:aws:kms:us-west-2:370957321024:alias/S3EC-Python-Github-KMS-Key')

lucasmcdonald3 · 2025-08-12T16:55:28Z

+            encryption_context_from_request=materials_dict.get(
+                "encryption_context_from_request", {}
+            ),
+            plaintext_data_key=materials_dict.get("PDK"),


Why "PDK"? Why not plaintext_data_key? That's more idiomatic

kessplas added 13 commits August 6, 2025 13:32

chore: add CDK for basic CI resources

374c3bc

add basic S3EC implementation

df36ec1

include decryptionMaterials change

62edf8a

add Github workflows

d4baef1

permissions

fae5819

fix test

99c9591

fix region

52dc9ab

debug

61096c6

or dict

23362e3

debug

8c76a5b

github env vars

24bcb48

vars not secrets

ef416de

remove debug, raise error on mismatch

480a398

kessplas changed the base branch from main to staging August 6, 2025 23:58

lucasmcdonald3 reviewed Aug 7, 2025

View reviewed changes

run formatter, add linting

21b5906

lucasmcdonald3 reviewed Aug 8, 2025

View reviewed changes

kessplas added 13 commits August 11, 2025 13:15

address feedback (ruff, uv, etc)

454c0eb

GHA stuff

351431a

uv venv

c15f2da

black

5e3f122

only ruff, remove isort

dac7ab7

use makefile

3cd450b

fix CI

be51b70

split out integ tests

8bbae1a

match boto3, use abc

090c8a4

snake case renaming

15d2144

format

cac76ce

remove lock file, format, simplify attrs

f231ab2

remove typehint

2d686bc

kessplas added 5 commits August 11, 2025 15:21

use pytest

aed461e

remove uv lock

2f6bc1e

ruff fixes

fa000a5

enforce ruff for src

6c0b92a

now fix black

bee9218

lucasmcdonald3 reviewed Aug 12, 2025

View reviewed changes

kessplas added 4 commits August 12, 2025 10:45

empty body plus tests

4b5ab01

fix type hints

c440af4

try client type hint again

d113d54

PDK to plaintext_data_key

a73d544

lucasmcdonald3 approved these changes Aug 12, 2025

View reviewed changes

kessplas merged commit 7a3b929 into staging Aug 12, 2025
2 checks passed

kessplas deleted the cdk-plus-basic-ci-test branch August 12, 2025 19:01

		# There is a newer version, but MPL wants this one.
		cryptography = "^43.0.1"

	aws-cryptographic-material-providers = "^1.7.4"
	aws-cryptographic-material-providers = "^1.11.1"

	def getEncryptionMaterials(self, encMatsRequest):
	def get_encryption_materials(self, enc_mats_request):

	def put_object(self, Bucket, Key, Data, EncryptionContext=None, **kwargs):
	def put_object(self, **kwargs):

		@@ -0,0 +1,656 @@
		# This file is automatically @generated by Poetry 1.7.1 and should not be changed by hand.

	enc_mats_request (Dict[str, Any]): Request containing encryption parameters
	enc_mats_request (Dict[str, Any] or EncryptionMaterials): Request containing encryption parameters

Conversation

kessplas commented Aug 6, 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!

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!

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!

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 Aug 11, 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kessplas Aug 11, 2025 •

edited

Loading