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

http: improve AWS HTTP v4 Signature #6524

Closed
wants to merge 23 commits into from
Closed

http: improve AWS HTTP v4 Signature #6524

wants to merge 23 commits into from

Conversation

dwagin
Copy link
Contributor

@dwagin dwagin commented Jan 25, 2021

  • Add support services without region and service prefixes in
    the URL endpoint (ex. Min.IO, GCP, Yandex Cloud, Mail.Ru Cloud Solutions, etc)
    by providing region and service parameters via aws-sigv4 option.
  • Add [:region[:service]] suffix to aws-sigv4 option;
  • Refactor memory management.
  • Use Curl_http_method() instead STRING_CUSTOMREQUEST.
  • Refactor canonical headers generating.
  • Remove repeated sha256_to_hex() usage.
  • Add some docs fixes.
  • Add some codestyle fixes.
  • Add providing credentials via url.
  • Update tests.

- Add support services without region and service prefixes in
the URL endpoint (ex. Min.IO, GCP, Yandex Cloud, Mail.Ru Cloud Solutions, etc)
by providing region and service parameters via aws-sigv4 option.
- Add [:region[:service]] suffix to aws-sigv4 option;
- Fix memory allocation errors.
- Refactor memory management.
- Use Curl_http_method instead() STRING_CUSTOMREQUEST.
- Refactor canonical headers generating.
- Remove repeated sha256_to_hex() usage.
- Add some docs fixes.
- Add some codestyle fixes.
- Add overloaded strndup() for debug - curl_dbg_strndup().
- Update tests.
docs/libcurl/opts/CURLOPT_AWS_SIGV4.3 Outdated Show resolved Hide resolved
docs/libcurl/opts/CURLOPT_AWS_SIGV4.3 Outdated Show resolved Hide resolved
docs/libcurl/opts/CURLOPT_AWS_SIGV4.3 Outdated Show resolved Hide resolved
docs/libcurl/opts/CURLOPT_AWS_SIGV4.3 Outdated Show resolved Hide resolved
lib/http_aws_sigv4.c Show resolved Hide resolved
lib/http_aws_sigv4.c Show resolved Hide resolved
curl_msnprintf(date_str, sizeof(date_str), "X-%s-Date: %s",
mid_provider, date_iso);
data->set.headers = curl_slist_append(data->set.headers, date_str);
data->set.headers = curl_slist_append(data->set.headers, auth_header);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize you didn't actually add this, but the code must not assign data->set.headers. That's the set of custom headers passed to libcurl from the application and it should not be interfered with by internals. It will make repeated transfers and handle duplications misbehave.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to fix it tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if(!curl_slist_append(data->set.headers, timestamp)) {
  ret = CURLE_FAILED_INIT;
  goto fail;
}
if(!curl_slist_append(data->set.headers, date_header)) {
  ret = CURLE_FAILED_INIT;
  goto fail;
}

would this work ?
Note: if I ask you to split your commit, it's partially because this would enable @bagder to merge bug fix rapidly (like this one) and give us more time to review other improvement .
Edit: dwagin fix seems to be the good thing to do, and work on my machine

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize you didn't actually add this, but the code must not assign data->set.headers. That's the set of custom headers passed to libcurl from the application and it should not be interfered with by internals. It will make repeated transfers and handle duplications misbehave.

@bagder, see my fix

docs/cmdline-opts/aws-sigv4.d Show resolved Hide resolved
@bagder
Copy link
Member

bagder commented Jan 25, 2021

@outscale-mgo, you might want to give this a review/comment too

Copy link
Contributor

@outscale-mgo outscale-mgo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer if you split "http: improve AWS HTTP v4 Signature" commit
by making one commit for each "improvements"
Also I find it a little odd, to modify test1933 in one commit and modify the data in another one.

added providing credentials via url
docs/libcurl/opts/CURLOPT_AWS_SIGV4.3 Outdated Show resolved Hide resolved
lib/http_aws_sigv4.c Outdated Show resolved Hide resolved
lib/http_aws_sigv4.c Outdated Show resolved Hide resolved
lib/http_aws_sigv4.c Outdated Show resolved Hide resolved
@outscale-mgo
Copy link
Contributor

Overall I like the changes, thanks for improving this code :)

lib/http_aws_sigv4.c Show resolved Hide resolved
@dwagin
Copy link
Contributor Author

dwagin commented Jan 27, 2021

@bagder, strndup() implementation turned out to be a big deal :).
I can finish it as part of this pull request or roll back these changes and rewrite without stndup().
Please tell me how best to do it?

convert strndup() -> malloc() + memcpy()
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An additional request: please also fix the names in the four tests! 1933 1934 1935 1936 all use the same non-telling names. They should better identify what makes them unique. Compare with other tests.

docs/libcurl/opts/CURLOPT_AWS_SIGV4.3 Outdated Show resolved Hide resolved
lib/http_aws_sigv4.c Outdated Show resolved Hide resolved
@bagder
Copy link
Member

bagder commented Jan 28, 2021

@bagder, strdup() implementation turned out to be a big deal :).

You mean strndup?

I can finish it as part of this pull request or roll back these changes and rewrite without stndup().

I don't know what you want from me on this. Maybe you can survive with using Curl_memdup() instead?

@dwagin
Copy link
Contributor Author

dwagin commented Jan 28, 2021

@bagder, strdup() implementation turned out to be a big deal :).

You mean strndup?

Yes.

I can finish it as part of this pull request or roll back these changes and rewrite without stndup().

I don't know what you want from me on this. Maybe you can survive with using Curl_memdup() instead?

I did everything without strndup()

Copy link
Contributor

@outscale-mgo outscale-mgo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once that fix, I don't think I will have any more comment.
But please note that having only one patch that contain all your change, make it really hard to read, as we have only one big diff, understand what change between to the old version and your version take extra effort.
Especially as you move some code (and it was the right move), but because of that the diff really don't understand what's going on and give you unrelated visual.

lib/http_aws_sigv4.c Outdated Show resolved Hide resolved
lib/http_aws_sigv4.c Outdated Show resolved Hide resolved
lib/http_aws_sigv4.c Outdated Show resolved Hide resolved
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. I'll give a little time for everyone else to also comment before I merge. I would like to have this done asap so that this is ready for the release on Wednesday, when this feature is first exposed to the world!

@dwagin
Copy link
Contributor Author

dwagin commented Jan 30, 2021

After this refactoring, this authentication method became fully usable. Thank you all for your help! Hello, world )

@bagder
Copy link
Member

bagder commented Jan 30, 2021

Thanks!

@bagder bagder closed this in 796ce29 Jan 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

6 participants