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

aws-sigv4: fix signature when post data is in binary #7844

Closed
wants to merge 2 commits into from
Closed

aws-sigv4: fix signature when post data is in binary #7844

wants to merge 2 commits into from

Conversation

@theawless
Copy link
Contributor

@theawless theawless commented Oct 12, 2021

User sets the post fields size for binary data. Hence, we should not be using strlen on it.
The code is very similar to the one here in mqtt.c.
I have performed basic sanity testing but do let me know if anything more is required.

Python code to create a binary test file with null character:

data = "testPo\0stData".encode()
assert(len(data), 13)
with open("testPostFile.bin", 'wb+') as file:
    file.write(data)

Build and installation:

./configure --with-secure-transport && make && make install

Curl command to test:

/usr/local/bin/curl -vvv --aws-sigv4 "testProviderOne:testProviderTwo:testRegion:testService" -u testKeyId:testSecretKey -X POST http://localhost:8080 --data-binary '@testPostFile.bin'

I put a log statement (for testing) to confirm the fix:

...

Post data str-len:6 post-field-size:13

...

> POST / HTTP/1.1
> Host: localhost:8080
> Authorization: TESTPROVIDERONE4-HMAC-SHA256 Credential=testKeyId/20211012/testRegion/testService/testproviderone4_request, SignedHeaders=host;x-testprovidertwo-date, Signature=14fced2b34bae056b9bc361e02e93525595977a28645f283d73999dfe8527248
> X-Testprovidertwo-Date: 20211012T101558Z
> User-Agent: curl/7.80.0-DEV
> Accept: */*
> Content-Length: 13
> Content-Type: application/x-www-form-urlencoded

...
@bagder
Copy link
Member

@bagder bagder commented Oct 12, 2021

Have you looked into adding a test case that verifies this?

/cc @outscale-mgo this PR looks fine right?

Loading

@bagder
Copy link
Member

@bagder bagder commented Oct 12, 2021

Wait, what happens if you decide to post data using the read callback?

Loading

@outscale-mgo
Copy link
Contributor

@outscale-mgo outscale-mgo commented Oct 12, 2021

Have you looked into adding a test case that verifies this?

/cc @outscale-mgo this PR looks fine right?

This seems fine to me.

Loading

@theawless
Copy link
Contributor Author

@theawless theawless commented Oct 13, 2021

Wait, what happens if you decide to post data using the read callback?

I think the read callback might not supported by sig-v4 because we need to calculate the signature on all of the post data and send that in the header.
For the CLI though, I noticed that both data and data-binary options setup the post fields so it works fine (after the fix).

Have you looked into adding a test case that verifies this?

Let me try doing that.

Loading

theawless added 2 commits Oct 14, 2021
User sets the post fields size for binary data.
Hence, we should not be using strlen on it.
@theawless
Copy link
Contributor Author

@theawless theawless commented Oct 14, 2021

Added tests 1937 for regular POST data and 1938 for binary POST data. Both of them are very similar to 1936 which already existed for GET.

Loading

@bagder
Copy link
Member

@bagder bagder commented Oct 15, 2021

I think the read callback might not supported by sig-v4 because we need to calculate the signature on all of the post data and send that in the header.

I suspected so. It doesn't feel like you have to fix that issue in this PR, but it feels like a) the code should then return some sort of error early instead of continuing and pretending it might actually work - and b) the limitation should probably be documented clearly.

Loading

@bagder bagder closed this in 8c6f126 Oct 15, 2021
@bagder
Copy link
Member

@bagder bagder commented Oct 15, 2021

Thanks!

Loading

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

Successfully merging this pull request may close these issues.

None yet

4 participants