Skip to content
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.

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

BatchWriter attempts to re-encrypt unprocessed items. #91

Closed
woodyza opened this issue Sep 10, 2018 · 7 comments
Closed

BatchWriter attempts to re-encrypt unprocessed items. #91

woodyza opened this issue Sep 10, 2018 · 7 comments

Comments

@woodyza
Copy link

woodyza commented Sep 10, 2018

Using the current 1.0.5 packaged version of the code the BatchWriter returned by Table.batch_writer() will raise exceptions if there are any unprocessed items returned from a batch_write.

The cause appears to be the client re-encrypting the already encrypted unprocessed items that get returned, and in the process validating the attribute names against the reserved attributes which fails.

Replicating this is easy enough, use a client to put items into a table at a rate that causes the writes to be throttled. Once at least one item fails and gets returned as an unprocessed item it'll trigger that error.

@mattsb42-aws mattsb42-aws changed the title BatchWriter retries cause 'Reserved attribute name' validation errors BatchWriter attempts to re-encrypt unprocessed items. Sep 10, 2018
@mattsb42-aws
Copy link
Member

Thanks for reporting this.

It looks like this is caused by the intersection between the boto3 BatchWriter and our EncryptedClient (for my future reference: [1][2][3]).

We look for the reserved attributes to attempt to keep the user from accidentally using those attributes for other things or re-encrypting items (exactly what is happening here).

I think that a reasonable balance here might be to add a check into the encrypt_batch_write_item to determine whether the item was already encrypted. We will want to do what we can to make sure that the item was actually encrypted by this client and was not since modified.

[1] https://github.com/boto/boto3/blob/develop/boto3/dynamodb/table.py#L133-L147
[2] https://github.com/aws/aws-dynamodb-encryption-python/blob/master/src/dynamodb_encryption_sdk/internal/utils.py#L270-L298
[3] https://github.com/aws/aws-dynamodb-encryption-python/blob/master/src/dynamodb_encryption_sdk/encrypted/item.py#L67-L71

@mattsb42-aws
Copy link
Member

We will be addressing this in #92

@woodyza
Copy link
Author

woodyza commented Mar 13, 2019

Hey @mattsb42-aws any idea when this might get into an official release?

@mattsb42-aws
Copy link
Member

Hi @woodyza. Sorry, this fell through the cracks.

Since this changes the API behavior, this is going to need to be a minor-version change. Because of that I had wanted to get the change to address #90 in that release as well, but have had other things that kept taking priority.

However, after taking another look through #90, I realize now that we cannot process that silently like we could the batch operations (detailed update in that issue).

I have a couple minor linting issues that I need to address (pylint update, I think), but after that we should be good to publish. I'll update here once a new version is pushed.

@woodyza
Copy link
Author

woodyza commented Mar 13, 2019

All good, thanks for the update! 👍

@mattsb42-aws
Copy link
Member

@woodyza 1.1.0 is now live with these changes.

@woodyza
Copy link
Author

woodyza commented Mar 13, 2019

Great stuff, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants