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

Introduce v4 signature used by AWS, Outscale and GCP #5703

Closed
wants to merge 6 commits into from

Conversation

@outscale-mgo
Copy link
Contributor

@outscale-mgo outscale-mgo commented Jul 20, 2020

This Pull Request add support for V4 signature as describe here:
https://wiki.outscale.net/display/EN/About+Signatures+of+API+Requests and here:
https://docs.aws.amazon.com/general/latest/gr/signature-version-4.html

I didn't test with GCP as it's not the default authentication method.

I've try to describe the algorithm in 6e7186d commit message.

here is some examples how to use curl with this patch to either call Outscale API from CURL or AWS:

# ReadVms Outscale API with filters
curl -X POST https://api.eu-west-2.outscale.com/api/latest/ReadVms -H 'Content-Type:  application/json' -k --user XXXXACCESSKEYXXXX:XXXXXXXXXSECRETKEYXXXXXXXXX --v4-signature "osc" --data '{"Filters" : { "VmIds" : [ "i-ca54d28e" ] } }'

# OUTSCALE with AWS API using POST
curl  -X POST https://fcu.eu-west-2.outscale.com/ -H 'Content-Type: application/x-www-form-urlencoded' --user XXXXACCESSKEYXXXX:XXXXXXXXXSECRETKEYXXXXXXXXX --v4-signature  "aws:amz" --data-urlencode "Version=2020-01-24" --data-urlencode "Action=DescribeInstances"

# OUTSCALE with AWS API using GET
curl -X GET 'https://fcu.eu-west-2.outscale.com?Version=2020-01-24&Action=DescribeInstances' -k --user XXXXACCESSKEYXXXX:XXXXXXXXXSECRETKEYXXXXXXXXX --v4-signature  "aws:amz"

# AWS
curl -X GET 'https://ec2.eu-west-3.amazonaws.com?Action=DescribeInstances&Version=2013-10-15' -k --user XXXXACCESSKEYXXXX:XXXXXXXXXSECRETKEYXXXXXXXXX --v4-signature  "aws:amz" -v
@bagder
Copy link
Member

@bagder bagder commented Jul 21, 2020

build warnings/errors in almost every CI job...

@outscale-mgo outscale-mgo force-pushed the outscale-dev:introduce-v4-signature branch from 7caa17c to 9d55d38 Jul 21, 2020
@outscale-mgo
Copy link
Contributor Author

@outscale-mgo outscale-mgo commented Jul 21, 2020

build warnings/errors in almost every CI job...

It seems I've forgot to add docs/cmdline-opts/v4-signature.d file,
and had some warning that my compiler miss ...
I've just fix that and push -f, so it should be fix now.

Sorry for the inconvenience.

@lgtm-com
Copy link

@lgtm-com lgtm-com bot commented Jul 21, 2020

This pull request introduces 1 alert when merging 9d55d38 into ff8b6ce - view on LGTM.com

new alerts:

  • 1 for Use of potentially dangerous function
@outscale-mgo outscale-mgo force-pushed the outscale-dev:introduce-v4-signature branch 2 times, most recently from bafd917 to 70b3004 Jul 24, 2020
@lgtm-com
Copy link

@lgtm-com lgtm-com bot commented Jul 24, 2020

This pull request introduces 1 alert when merging 70b3004 into 13030d0 - view on LGTM.com

new alerts:

  • 1 for Use of potentially dangerous function
@outscale-mgo outscale-mgo force-pushed the outscale-dev:introduce-v4-signature branch from 70b3004 to 0d3e2c5 Jul 27, 2020
@bagder
Copy link
Member

@bagder bagder commented Jul 27, 2020

  1. Note that lgtm is correct: we must not use gmtime() in curl code! Use Curl_gmtime() instead.
  2. Merge conflicts
@outscale-mgo outscale-mgo force-pushed the outscale-dev:introduce-v4-signature branch from 0d3e2c5 to d385931 Jul 27, 2020
@bagder
Copy link
Member

@bagder bagder commented Jul 27, 2020

Test 971 and 1119 still fail. They should be easy to fix:

  • For 971, add the new cmdline flag to docs/options-in-versions
  • For 1119, add the new curl symbols to docs/libcurl/symbols-in-versions
Copy link
Member

@bagder bagder left a comment

There are details left to polish here. I stopped remarking at some point, but I'm sure you get the gist of what needs to be done. When you add a test case or two for this authentication, many of my notes would be noticed immediately as (torture) test failures.

What exactly is the official name for this algorithm? Isn't it "AWS HTTP v4 Signature" or something like that? I find calling it just "v4 signature" seems a bit... non-descriptive.

char sk[45]; /* secret key is 40 chat long + 'OSC' + \0 */
struct Curl_easy *data = conn->data;
const char *customrequest = data->set.str[STRING_CUSTOMREQUEST];
const char *surl = strstr(data->set.str[STRING_SET_URL], "://") + 3;

This comment has been minimized.

@bagder

bagder Jul 27, 2020
Member

This is not safe code. What exactly is surl, shouldn't it rather use a decomposed URL part?

CURLcode Curl_output_v4_signature(struct connectdata *conn, bool proxy)
{
CURLcode ret = CURLE_OK;
char sk[45]; /* secret key is 40 chat long + 'OSC' + \0 */

This comment has been minimized.

@bagder

bagder Jul 27, 2020
Member

s/chat/bytes ?

if(!customrequest)
customrequest = "POST";
if(content_type) {
content_type = strchr(content_type, ':') + 1;

This comment has been minimized.

@bagder

bagder Jul 27, 2020
Member

Can we be sure this string always has a colon?


tmp = strchr(api_type, '.');
if(!tmp)
return CURLE_FAILED_INIT;

This comment has been minimized.

@bagder

bagder Jul 27, 2020
Member

this leaks memory in api_type here, right?

if(tmp) {
for(i = 0; provider != tmp; ++provider, ++i) {
if(i > PROVIDER_MAX_L)
return CURLE_FAILED_INIT;

This comment has been minimized.

@bagder

bagder Jul 27, 2020
Member

That seems like a bad error code. Maybe also add an infof() to aid users?

sha_str[64] = 0;

canonical_request = curl_maprintf(
"%s\n" /* Methode */

This comment has been minimized.

@bagder

bagder Jul 27, 2020
Member

unusual indent level

strlen(canonical_request));
for(i = 0; i < 32; ++i) {
curl_msprintf(sha_str + (i * 2), "%02x", sha_d[i]);
}

This comment has been minimized.

@bagder

bagder Jul 27, 2020
Member

Isn't this the exact same loop again?

lib/http_v4_signature.c Outdated Show resolved Hide resolved
up_provider, data->set.str[STRING_USERNAME], cred_scope,
signed_headers, sha_str);

curl_msprintf(date_str, "X-%s-Date: %s", mid_provider, date_iso);

This comment has been minimized.

@bagder

bagder Jul 27, 2020
Member

snprintf, please

lib/http_v4_signature.c Outdated Show resolved Hide resolved
@outscale-mgo outscale-mgo force-pushed the outscale-dev:introduce-v4-signature branch 2 times, most recently from b4fd3f7 to 73e1059 Jul 31, 2020
*/
if(data->set.str[STRING_V4_SIGNATURE] &&
(data->set.httpauth == CURLAUTH_NONE ||
data->set.httpauth == CURLAUTH_BASIC))

This comment has been minimized.

@bagder

bagder Aug 3, 2020
Member

Why does it require none or basic set to switch to signature_v4 ?

@@ -0,0 +1,20 @@
Long: v4-signature
Arg: <provider1[:provider2]>
Help: Performe AWS V4 signature on HTTP header

This comment has been minimized.

@bagder

bagder Aug 3, 2020
Member

Spelling, and shouldn't it rather be *provides AWS V4 signature authentication" or similar?

.SH DESCRIPTION
performe v4 signature as decribe here: https://docs.aws.amazon.com/general/latest/gr/signature-version-4.html
or here https://wiki.outscale.net/display/EN/About+Signatures+of+API+Requests
pass a string that contain a provider prefix that will be used to forge the signature.

This comment has been minimized.

@bagder

bagder Aug 3, 2020
Member

Please describe this as detailed as possible without adding URLs. URLs should be avoided in the man page, but if they really add a value they could be added below somewhere perhaps with its own subtitle.

struct curl_slist *list = NULL;

if(curl) {
curl_easy_setopt(curl, CURLOPT_URL, "https://api.eu-west-2.outscale.com/api/latest/ReadVms");

This comment has been minimized.

@bagder

bagder Aug 3, 2020
Member

Please use an "example.com" URL or similar, not a real one.

performe v4 signature as decribe here: https://docs.aws.amazon.com/general/latest/gr/signature-version-4.html
or here https://wiki.outscale.net/display/EN/About+Signatures+of+API+Requests
pass a string that contain a provider prefix that will be used to forge the signature.
For Amazon use "aws:amz", for outscale use "osc"

This comment has been minimized.

@bagder

bagder Aug 3, 2020
Member

What happens if you set another?

Curl_infof(data, msg); \
ret = error; \
goto free_all; \
}

This comment has been minimized.

@bagder

bagder Aug 3, 2020
Member

I would rather that you don't sprinkle the code with this macro but rather actually inline the code. That makes the code much more readable - and if that feels like too much repetitive code, then maybe that says something...

@outscale-mgo outscale-mgo force-pushed the outscale-dev:introduce-v4-signature branch 6 times, most recently from c72b8e4 to abdda3f Aug 3, 2020
@outscale-mgo outscale-mgo force-pushed the outscale-dev:introduce-v4-signature branch 7 times, most recently from 090be14 to 59f9005 Aug 11, 2020
}

if(!customrequest)
customrequest = "POST";

This comment has been minimized.

@bagder

bagder Oct 1, 2020
Member

If no custom method is set, data->set.method knows it. It can't be nothing.

@@ -30,6 +30,7 @@ CURLAUTH_NONE 7.10.6
CURLAUTH_NTLM 7.10.6
CURLAUTH_NTLM_WB 7.22.0
CURLAUTH_ONLY 7.21.3
CURLAUTH_SIGNATURE_V4 7.72.0

This comment has been minimized.

@bagder

bagder Oct 1, 2020
Member

This is still pending. Why the sudden reversed order, and now we're looking at landing in time for 7.74.0

int i;

for(i = 0; i < 32; ++i) {
curl_msnprintf(dst + (i * 2), 3, "%02x", sha[i]);

This comment has been minimized.

@bagder

bagder Oct 1, 2020
Member

The point with curl_msnprintf is using the actual target size in the second argument, and not just a fixed length. How about passing in the target buffer size as an argument to the function and use that, plus deduct the size in the loop?

Maybe call it _hex instead of _str ?

return ret;
}

if(!strftime(date_iso, 17, "%Y%m%dT%H%M%SZ", &info)) {

This comment has been minimized.

@bagder

bagder Oct 1, 2020
Member

Can we have the 17 instead be sizeof() the target buffer?

if(!strftime(date_iso, 17, "%Y%m%dT%H%M%SZ", &info)) {
return CURLE_OUT_OF_MEMORY;
}
date_iso[16] = 0;

This comment has been minimized.

@bagder

bagder Oct 1, 2020
Member

Here too, can we have 16 be sizeof -1? Hardcoded numbers are asking for future problems,

date_iso[16] = 0;

memcpy(date, date_iso, 8);
date[8] = 0;

This comment has been minimized.

@bagder

bagder Oct 1, 2020
Member

... and here

sha256_to_str(sha_str, sha_d);

canonical_request = curl_maprintf(
"%s\n" /* Methode */

This comment has been minimized.

@bagder

bagder Oct 1, 2020
Member

trailing e in the comment

uri = data->state.up.path;

curl_msnprintf(request_type, REQUEST_TYPE_L, "%s4_request", low_provider0);
request_type[REQUEST_TYPE_L - 1] = 0;

This comment has been minimized.

@bagder

bagder Oct 1, 2020
Member

msnprintf() already zero terminates

Long: v4-signature
Arg: <provider1[:provider2]>
Help: provides AWS V4 signature authentication
Added: 7.73.0

This comment has been minimized.

@bagder

bagder Oct 1, 2020
Member

7.74.0 is the aim now!

- compute hmac from "string to sign" using above hmac as key
- create Authorization HTTP header using "Algorithm", access key,
"credential scope", "signed header" and above hash
- Push the "date" and the Authorization to the HTTP header

This comment has been minimized.

@bagder

bagder Oct 1, 2020
Member

You don't need to describe how the auth works here, that's out of scope for the man page.

@bagder
Copy link
Member

@bagder bagder commented Oct 1, 2020

I still have some questions around how this fits into CURLOPT_HTTPAUTH. Your code now sets the v4sig bit exclusively if the v4sig option is set, which seems to imply that this auth is more important than the others or doesn't play on the same level. HTTP authorization is typically the client using one out of possibly a set of the different ones that the server offers.

@outscale-mgo outscale-mgo force-pushed the outscale-dev:introduce-v4-signature branch 2 times, most recently from 8c62245 to 80d25a8 Oct 2, 2020
@vszakats
Copy link
Member

@vszakats vszakats commented Oct 4, 2020

I believe the name of the option shall make it clear this is an AWS-specific type (which is apparently supported by other vendors by now?) and that terminology used consistently accross docs, option name, source, filenames, constants, etc. [ Even though I generally prefer something vendor-agnostic if there is any such option — if someone knows about an RFC or something, we should consider it. ]

E.g.:

  • --aws-sigv4
  • CURLOPT_AWS_SIGV4
  • CURLAUTH_AWS_SIGV4
  • http_aws_sigv4.c
  • http_aws_sigv4.h
  • etc.
@outscale-mgo outscale-mgo force-pushed the outscale-dev:introduce-v4-signature branch 2 times, most recently from 63b8505 to 8492977 Oct 5, 2020
@outscale-mgo
Copy link
Contributor Author

@outscale-mgo outscale-mgo commented Oct 6, 2020

shall

That's a good point, I've rename the v4-signature as aws-sigv4, note that's I'm bad at naming thing, so I've just reuse your suggestion

@outscale-mgo outscale-mgo force-pushed the outscale-dev:introduce-v4-signature branch from 8492977 to 754bba6 Oct 6, 2020
@outscale-mgo outscale-mgo requested a review from bagder Oct 9, 2020
@outscale-mgo outscale-mgo force-pushed the outscale-dev:introduce-v4-signature branch from 754bba6 to be311d5 Dec 3, 2020
@outscale-mgo
Copy link
Contributor Author

@outscale-mgo outscale-mgo commented Dec 4, 2020

I've just rebase so ping :)

@@ -241,6 +241,7 @@
--use-ascii (-B) 5.0
--user (-u) 4.0
--user-agent (-A) 4.5.1
--aws-sigv4 7.74.0

This comment has been minimized.

@bagder

bagder Dec 14, 2020
Member

These version mentions now need to be 7.75.0...

lib/http_aws_sigv4.c Show resolved Hide resolved
goto free_all;
}

curl_msnprintf(sk, FULL_SK_L - 1, "%s4%s", up_provider,

This comment has been minimized.

@bagder

bagder Dec 14, 2020
Member

how about using sizeof(sk) instead of the define? Makes it crystal clear it won't overwrrite the buffer.

(unsigned int)strlen(request_type),
tmp_sign1);
HMAC_SHA256(tmp_sign1, sizeof(tmp_sign1), (void *)str_to_sign,
(unsigned int)strlen(str_to_sign), tmp_sign0);

This comment has been minimized.

@bagder

bagder Dec 14, 2020
Member

Since this HMAX_SHA256 is a macro anyway, can't you move the typecasts to the macro away from "the main code" ?

"SignedHeaders=content-type;host;x-try-date" for "signed headers"

If you use just "test", instead of "test:try",
test will be use for every generated string.

This comment has been minimized.

@bagder

bagder Dec 14, 2020
Member

I propose you remove everything below the first three lines of the description. The details about the request bytes are just too specific for ordinary users. Remember that this goes into the man page. Regular command line users will read and use this.

If you use just "test", instead of "test:try",
test will be use for every strings generated


This comment has been minimized.

@bagder

bagder Dec 14, 2020
Member

No need to add extra blank lines before the next .SH.

Missing mention: this option overrides the other auth types you might have set in CURL_HTTPAUTH which should be highlighted as this makes this auth method special. It could probably also be mentioned that this method can't be combined with other auth types.

curl_easy_setopt(c, CURLOPT_AWS_SIGV4, "xxx:yyy");
curl_easy_setopt(c, CURLOPT_USERPWD, "MY_ACCESS_KEY:MY_SECRET_KEY");
list = curl_slist_append(list, "Content-Type: application/json");
curl_easy_setopt(c, CURLOPT_HTTPHEADER, list);

This comment has been minimized.

@bagder

bagder Dec 14, 2020
Member

I propose you remove the CURLOPT_HTTPHEADER from the example, as it's unrelated.

struct curl_slist *list = NULL;

if(curl) {
curl_easy_setopt(curl, CURLOPT_URL, "https://api_type.region.site.domaine/uri/uri");

This comment has been minimized.

@bagder

bagder Dec 14, 2020
Member

please use example.com so the name gets shorter and won't wrap for most users

This comment has been minimized.

@outscale-mgo

outscale-mgo Dec 15, 2020
Author Contributor

I've change it to https://api_type.region.example.com/uri, and split the line to keep it under 80 columes.
I've keep "api_type.region" because those information are used by the algorithm.

/* secret key is 40 bytes long + PROVIDER_MAX_L + \0 */
#define FULL_SK_L (PROVIDER_MAX_L + 40 + 1)

static void sha256_to_hex(char *dst, unsigned char *sha, int dst_l)

This comment has been minimized.

@bagder

bagder Dec 14, 2020
Member

This should probably take a size_t for the size, as you'll be passing sizeof() to it.

@outscale-mgo outscale-mgo force-pushed the outscale-dev:introduce-v4-signature branch 2 times, most recently from 1c2c63f to 39d54f3 Dec 15, 2020
@outscale-mgo outscale-mgo requested a review from bagder Dec 17, 2020
@bagder
bagder approved these changes Dec 20, 2020
Copy link
Member

@bagder bagder left a comment

I'm ready to merge this. Can you just take a look at the new merge conflicts due to other merges that landed?

outscale-mgo added 6 commits Jul 3, 2020
It seems current hmac implementation use md5 for the hash,
V4 signature require sha256, so I've added the needed struct in
this commit.

I've added the functions that do the hmac in v4 signature file
as a static function ,in the next patch of the serie,
because it's used only by this file.

Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com>
It is a security process for HTTP.

It doesn't seems to be standard, but it is used by some cloud providers.

Aws:
https://docs.aws.amazon.com/general/latest/gr/signature-version-4.html
Outscale:
https://wiki.outscale.net/display/EN/Creating+a+Canonical+Request
GCP (I didn't test that this code work with GCP though):
https://cloud.google.com/storage/docs/access-control/signing-urls-manually

most of the code is in lib/http_v4_signature.c

Information require by the algorithm:
- The URL
- Current time
-  some prefix that are append to some of the signature parameters.

The data extracted from the URL are: the URI, the region,
the host and the API type

example:
https://api.eu-west-2.outscale.com/api/latest/ReadNets
        ~~~ ~~~~~~~~               ~~~~~~~~~~~~~~~~~~~
        ^       ^                          ^
       /         \                        URI
   API type     region

Small description of the algorithm:
- make canonical header using content type, the host, and the date
- hash the post data
- make canonical_request using custom request, the URI,
  the get data, the canonical header, the signed header
  and post data hash
- hash canonical_request
- make str_to_sign using one of the prefix pass in parameter,
  the date, the credential scope and the canonical_request hash
- compute hmac from date, using secret key as key.
- compute hmac from region, using above hmac as key
- compute hmac from api_type, using above hmac as key
- compute hmac from request_type, using above hmac as key
- compute hmac from str_to_sign using above hmac as key
- create Authorization header using above hmac, prefix pass in parameter,
  the date, and above hash

Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com>
This patch allow to call the v4 signature introduce in previous commit

Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com>
Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com>
Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com>
@outscale-mgo outscale-mgo force-pushed the outscale-dev:introduce-v4-signature branch from 39d54f3 to 426269d Dec 21, 2020
@outscale-mgo
Copy link
Contributor Author

@outscale-mgo outscale-mgo commented Dec 21, 2020

I'm ready to merge this. Can you just take a look at the new merge conflicts due to other merges that landed?

Thanks, it should be ok now

@bagder bagder closed this in 08e8455 Dec 21, 2020
@bagder
Copy link
Member

@bagder bagder commented Dec 21, 2020

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

3 participants