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

Jump to bottom

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 •
edited

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 commented Oct 12, 2021

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

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

@bagder
Copy link
Member

bagder commented Oct 12, 2021

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

@outscale-mgo
Copy link
Contributor

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

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

This seems fine to me.

@theawless
Copy link
Contributor Author

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.

@theawless
aws-sigv4: fix signature when post data is in binary
b0f3ad0 
User sets the post fields size for binary data.
Hence, we should not be using strlen on it.
@theawless
aws-sigv4: add tests for HTTP POST
37e5a30
@theawless
Copy link
Contributor Author

theawless commented Oct 14, 2021 •
edited

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.

@bagder
Copy link
Member

bagder commented Oct 15, 2021 •
edited

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.

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

bagder commented Oct 15, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
authentication hacktoberfest-accepted HTTP
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@theawless @bagder @outscale-mgo @mback2k