feat(Go): support DB-ESDK in Go #1861
Conversation
rishav-karanjit
changed the title
rishav-karanjit
changed the title
For some reason some code did not got merged
rishav-karanjit
changed the title
| options.APIOptions = append(options.APIOptions, func(stack *middleware.Stack) error { | ||
| // Add request interceptor at the beginning of Initialize step | ||
| requestIntercetor := m.createRequestInterceptor() | ||
| if err := stack.Initialize.Add(requestIntercetor, middleware.Before); err != nil { |
There was a problem hiding this comment.
Q: I thought Initialize is where the request object is created. But it we add Interceptor before it, then request must exist. What does Initialize step do then?
There was a problem hiding this comment.
A request is created only after Serialize step which comes after Initialize step. Initialize step does the following:
- takes input parameters and processes them to ensure they're ready for the next steps in the pipeline.
- Sets default parameters. It fills in any default values that might be needed but weren't explicitly provided in the input.
So, putting a middleware stack before initialize step means that once the input come from application layer, the input will be encrypted then initialize step will get the ciphertext instead of the plaintext
| } | ||
|
|
||
| func (m DBEsdkMiddleware) createRequestInterceptor() middleware.InitializeMiddleware { | ||
| return middleware.InitializeMiddlewareFunc("RequestInterceptor", func( |
There was a problem hiding this comment.
The ID RequestInterceptor is too generic, maybe add something like aws-ddbec-request-interceptor ?
There was a problem hiding this comment.
I will make a PR for this.
| } | ||
|
|
||
| // handleRequestInterception handles the interception logic before the DynamoDB operation | ||
| func (m DBEsdkMiddleware) handleRequestInterception(ctx context.Context, request interface{}) (context.Context, error) { |
There was a problem hiding this comment.
I assume operation that is not handled here is passed as it-is (given that this is global interceptor)
There was a problem hiding this comment.
Yes, it is passed as it-is. We only handle 13 APIs for DB-ESDK because it makes sense to only have those and not have something like createTable.
Say, DynamoDB adds a new API which makes sense to include here, we will have to manually update all the interceptors in all the runtimes in DB-ESDK.
This is also what all other runtime does. Example (Java): https://github.com/aws/aws-database-encryption-sdk-dynamodb/blob/main/DynamoDbEncryption/runtimes/java/src/main/java/software/amazon/cryptography/dbencryptionsdk/dynamodb/DynamoDbEncryptionInterceptor.java#L326
| switch v := request.(type) { | ||
| case *dynamodb.PutItemInput: | ||
| ctx = middleware.WithStackValue(ctx, ContextKeyOriginalInput, *v) | ||
| // Note: this context is not propagated downstream into dafny layer so it's left as context.TODO() https://issues.amazon.com/CrypTool-5403 |
There was a problem hiding this comment.
Why not put the actual context here? So that if Dafny or some other tooling starts accepting it, we don't have to do anything here.
There was a problem hiding this comment.
When I was writing this middleware I thought context.TODO() made more sense as we don't support context yet. But, as I think now, if we send the context now then we don't have do to anything if dafny starts accepting one.
There was a problem hiding this comment.
I will make a PR for this.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| *v = transformedRequest.TransformedInput |
There was a problem hiding this comment.
Q: if v is pointer type, I assume we are assigning value of TransformedInput to v and not address because we don't want to mutate the TransformedInput ?
There was a problem hiding this comment.
No. Its because we want to change content of v globally not just locally. *v = transformedRequest.TransformedInput means that it take a memory location that v points to and override the contents with the transformed input.
If we had done v = &transformedRequest.TransformedInput, this does not change the original v. We are just making a local variable v point to some different address and when the function returns the change to local variable is lost.
| out middleware.FinalizeOutput, metadata middleware.Metadata, err error, | ||
| ) { | ||
| // First let the request complete | ||
| result, metadata, err := next.HandleFinalize(ctx, in) |
There was a problem hiding this comment.
Doesn't middleware.After mean the interceptor will be executed after the request is done?
There was a problem hiding this comment.
When we get a response from transportation layer, it goes back to application layer in the reverse order of request (Deserialize -> Finalize -> Build -> Serialize -> Initialize). Adding after finalize means that after the final message (which will be a ciphertext) is prepared, we decrypt the message and send it to Build step.
DynamoDbEncryption/runtimes/go/ImplementationFromDafny-go/dbesdkmiddleware/middleware.go
Show resolved
Hide resolved
rishav-karanjit
changed the title
Issue #, if available:
Description of changes:
middleware.gois the middleware to intercept a DDB call which has been reviewed twice (by Shubham and Andy)Callout:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.