Conversation
| # Re-raise our own exceptions without wrapping | ||
| raise | ||
| except Exception as e: | ||
| raise S3EncryptionClientError(f"Failed to encryption object: {str(e)}") from e |
There was a problem hiding this comment.
fail
| raise S3EncryptionClientError(f"Failed to encryption object: {str(e)}") from e | |
| raise S3EncryptionClientError(f"Failed to encrypt object: {str(e)}") from e |
| header_key = f"{S3_METADATA_PREFIX}{key}" | ||
| headers[header_key] = value | ||
|
|
||
| params["headers"] = headers |
There was a problem hiding this comment.
I don't understand enough of the business logic to know if this was intentional or not, but you may wind up overwriting existing headers here
There was a problem hiding this comment.
Hmm... is the ask to check for header key collision?
That is a reasonable ask, and it might prevent double wrapping of the pipeline.
i.e: a user manages to wrap a encryption client with a encryption client.
There was a problem hiding this comment.
It's not an ask - it's me not understanding. If you want to override it, it's great as is :)
If you're concerned with users setting custom headers, you can always just update the above to
if header_key not in headers:
headers[header_key] = value
There was a problem hiding this comment.
The headers set by the encryption client are reserved and should not be set by the customer. If the customer does this, those values must be overwritten. This is not explicitly specified, but this is how Java behaves.
texastony
left a comment
texastony
left a comment
There was a problem hiding this comment.
I think checking for header key collision is worth refactoring this PR for.
Also, for TODOs, if we can create GHI issues for them, that would be groovy; though we can punt on that until pre-launch and triage for launch blockers if you would prefer.
| body_bytes = body.read() | ||
| else: | ||
| # Unexpected body type - should not happen as boto3 validates before this point | ||
| raise S3EncryptionClientError("Unexpected type of body parameter!") |
There was a problem hiding this comment.
Question: any chance the request ID is in scope here?
There was a problem hiding this comment.
Not sure if this answers you're question:
If you're asking if we have access to the x-amz-request-id or x-amz-id-2 headers, those come from the service as a part of the response. Since the request hasn't been sent yet at this point in code execution, so those values don't exist yet. You'd need to wait until we have sent the request and the service has responded.
If you only need it for client side tracking, you can delay this to the request-created event and take a look at the amz-sdk-invocation-id header. If you're looking to add any sort of logic around the request id that's connected with the server's request id, you'll need to hook into after-call or later.
There was a problem hiding this comment.
I was asking to improve the error message.
I frequent customer ask is for request IDs to be in error messages,
but since the request ID is not in scope,
let it roll.
| header_key = f"{S3_METADATA_PREFIX}{key}" | ||
| headers[header_key] = value | ||
|
|
||
| params["headers"] = headers |
There was a problem hiding this comment.
Hmm... is the ask to check for header key collision?
That is a reasonable ask, and it might prevent double wrapping of the pipeline.
i.e: a user manages to wrap a encryption client with a encryption client.
| } | ||
|
|
||
| # Create a pipeline and decrypt the data | ||
| pipeline = GetEncryptedObjectPipeline(self.config.cmm) |
There was a problem hiding this comment.
Issue: To handle instruction files, we are going to need the S3EC client to be passed to the GetEncryptedObjectPipeline.
Is there an S3 Client in scope here?
Note: This does not have to be resolved for this PR, but I am asking @kessplas about this, as Kess has context on the plugin nature that I do not.
There was a problem hiding this comment.
Short answer, no.
Long answer, there can be. I think the easiest way to deal with this is to make the original wrapped client an instance variable of the plugin so that the plugin can call out to get_object for the instruction file if needed. IIRC Python uses references everywhere so it should be too heavy, and the client should be thread safe enough to reuse.
3 participants
Issue #, if available: #130
Description of changes: The main reason to do this is that we don't need to worry about how the data is serialized. It will already have been serialized by the time the S3EC needs to encrypt/decrypt it.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.