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

outscale-mgo · 2020-07-20T12:59:29Z

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 · 2020-07-21T08:09:37Z

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

outscale-mgo · 2020-07-21T10:08:55Z

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 · 2020-07-21T10:13:13Z

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

new alerts:

1 for Use of potentially dangerous function

lgtm-com · 2020-07-24T17:23:17Z

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

new alerts:

1 for Use of potentially dangerous function

bagder · 2020-07-27T12:25:42Z

Note that lgtm is correct: we must not use gmtime() in curl code! Use Curl_gmtime() instead.
Merge conflicts

bagder · 2020-07-27T22:22:39Z

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

bagder

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.

bagder · 2020-07-27T22:25:09Z

lib/http_v4_signature.c

+  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 is not safe code. What exactly is surl, shouldn't it rather use a decomposed URL part?

bagder · 2020-07-27T22:25:25Z

lib/http_v4_signature.c

+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 */


s/chat/bytes ?

bagder · 2020-07-27T22:28:59Z

lib/http_v4_signature.c

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


Can we be sure this string always has a colon?

bagder · 2020-07-27T22:30:38Z

lib/http_v4_signature.c

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


this leaks memory in api_type here, right?

bagder · 2020-07-27T22:31:45Z

lib/http_v4_signature.c

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


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

bagder · 2020-07-27T22:48:37Z

lib/http_v4_signature.c

+  sha_str[64] = 0;
+
+  canonical_request = curl_maprintf(
+                     "%s\n" /* Methode */


unusual indent level

bagder · 2020-07-27T22:49:43Z

lib/http_v4_signature.c

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


Isn't this the exact same loop again?

lib/http_v4_signature.c

bagder · 2020-07-27T22:50:03Z

lib/http_v4_signature.c

+                       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);


snprintf, please

lib/http_v4_signature.c

bagder · 2020-08-03T15:45:21Z

lib/setopt.c

+     */
+    if(data->set.str[STRING_V4_SIGNATURE] &&
+       (data->set.httpauth == CURLAUTH_NONE ||
+        data->set.httpauth == CURLAUTH_BASIC))


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

bagder · 2020-08-03T15:48:04Z

docs/cmdline-opts/v4-signature.d

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


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

bagder · 2020-08-03T15:51:26Z

docs/libcurl/opts/CURLOPT_V4_SIGNATURE.3

+.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.


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.

bagder · 2020-08-03T15:51:51Z

docs/libcurl/opts/CURLOPT_V4_SIGNATURE.3

+struct curl_slist *list = NULL;
+
+if(curl) {
+  curl_easy_setopt(curl, CURLOPT_URL, "https://api.eu-west-2.outscale.com/api/latest/ReadVms");


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

bagder · 2020-08-03T15:53:11Z

docs/libcurl/opts/CURLOPT_V4_SIGNATURE.3

+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"


What happens if you set another?

bagder · 2020-08-03T15:54:33Z

lib/http_v4_signature.c

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


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...

bagder · 2020-10-01T15:23:21Z

lib/http_v4_signature.c

+  }
+
+  if(!customrequest)
+    customrequest = "POST";


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

bagder · 2020-10-01T15:24:12Z

docs/libcurl/symbols-in-versions

@@ -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 is still pending. Why the sudden reversed order, and now we're looking at landing in time for 7.74.0

bagder · 2020-10-01T15:25:27Z

lib/http_v4_signature.c

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


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 ?

bagder · 2020-10-01T15:28:33Z

lib/http_v4_signature.c

+    return ret;
+  }
+
+  if(!strftime(date_iso, 17, "%Y%m%dT%H%M%SZ", &info)) {


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

bagder · 2020-10-01T15:29:03Z

lib/http_v4_signature.c

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


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

bagder · 2020-10-01T15:29:15Z

lib/http_v4_signature.c

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


... and here

bagder · 2020-10-01T15:30:22Z

lib/http_v4_signature.c

+  sha256_to_str(sha_str, sha_d);
+
+  canonical_request = curl_maprintf(
+    "%s\n" /* Methode */


trailing e in the comment

bagder · 2020-10-01T15:33:16Z

lib/http_v4_signature.c

+  uri = data->state.up.path;
+
+  curl_msnprintf(request_type, REQUEST_TYPE_L, "%s4_request", low_provider0);
+  request_type[REQUEST_TYPE_L - 1] = 0;


msnprintf() already zero terminates

bagder · 2020-10-01T15:35:46Z

docs/cmdline-opts/v4-signature.d

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


7.74.0 is the aim now!

bagder · 2020-10-01T15:36:53Z

docs/libcurl/opts/CURLOPT_V4_SIGNATURE.3

+- 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


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

bagder · 2020-10-01T15:39:48Z

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.

vszakats · 2020-10-04T18:15:17Z

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 · 2020-10-06T12:46:44Z

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 · 2020-12-04T09:34:28Z

I've just rebase so ping :)

bagder · 2020-12-14T09:15:49Z

docs/options-in-versions

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


These version mentions now need to be 7.75.0...

lib/http_aws_sigv4.c

bagder · 2020-12-14T09:56:29Z

lib/http_aws_sigv4.c

+    goto free_all;
+  }
+
+  curl_msnprintf(sk, FULL_SK_L - 1, "%s4%s", up_provider,


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

bagder · 2020-12-14T10:53:07Z

lib/http_aws_sigv4.c

+              (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);


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

bagder · 2020-12-14T10:59:23Z

docs/cmdline-opts/aws-sigv4.d

+"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.


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.

bagder · 2020-12-14T11:02:09Z

docs/libcurl/opts/CURLOPT_AWS_SIGV4.3

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


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.

bagder · 2020-12-14T11:03:21Z

docs/libcurl/opts/CURLOPT_AWS_SIGV4.3

+  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);


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

bagder · 2020-12-14T11:04:06Z

docs/libcurl/opts/CURLOPT_AWS_SIGV4.3

+struct curl_slist *list = NULL;
+
+if(curl) {
+  curl_easy_setopt(curl, CURLOPT_URL, "https://api_type.region.site.domaine/uri/uri");


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

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.

bagder · 2020-12-14T12:53:44Z

lib/http_aws_sigv4.c

+/* 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 should probably take a size_t for the size, as you'll be passing sizeof() to it.

bagder

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

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>

outscale-mgo · 2020-12-21T12:58:17Z

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 · 2020-12-21T15:28:31Z

Thanks!

outscale-mgo force-pushed the introduce-v4-signature branch from 7caa17c to 9d55d38 Compare July 21, 2020 10:01

outscale-mgo force-pushed the introduce-v4-signature branch 2 times, most recently from bafd917 to 70b3004 Compare July 24, 2020 17:12

outscale-mgo force-pushed the introduce-v4-signature branch from 70b3004 to 0d3e2c5 Compare July 27, 2020 11:56

outscale-mgo force-pushed the introduce-v4-signature branch from 0d3e2c5 to d385931 Compare July 27, 2020 13:31

bagder requested changes Jul 27, 2020

View reviewed changes

outscale-mgo force-pushed the introduce-v4-signature branch 2 times, most recently from b4fd3f7 to 73e1059 Compare August 3, 2020 15:30

bagder reviewed Aug 3, 2020

View reviewed changes

outscale-mgo force-pushed the introduce-v4-signature branch 6 times, most recently from c72b8e4 to abdda3f Compare August 10, 2020 13:20

outscale-mgo force-pushed the introduce-v4-signature branch 7 times, most recently from 090be14 to 59f9005 Compare August 14, 2020 09:17

bagder reviewed Oct 1, 2020

View reviewed changes

outscale-mgo force-pushed the introduce-v4-signature branch 2 times, most recently from 8c62245 to 80d25a8 Compare October 2, 2020 16:04

outscale-mgo force-pushed the introduce-v4-signature branch 2 times, most recently from 63b8505 to 8492977 Compare October 5, 2020 15:35

outscale-mgo force-pushed the introduce-v4-signature branch from 8492977 to 754bba6 Compare October 6, 2020 14:45

outscale-mgo requested a review from bagder October 9, 2020 16:52

outscale-mgo force-pushed the introduce-v4-signature branch from 754bba6 to be311d5 Compare December 3, 2020 11:00

bagder reviewed Dec 14, 2020

View reviewed changes

outscale-mgo force-pushed the introduce-v4-signature branch 2 times, most recently from 1c2c63f to 39d54f3 Compare December 15, 2020 11:05

outscale-mgo requested a review from bagder December 17, 2020 10:20

bagder approved these changes Dec 20, 2020

View reviewed changes

outscale-mgo added 6 commits December 21, 2020 11:31

http: Make the call to v4 signature

46ff4ec

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

tool: add AWS HTTP v4 Signature support

f05fb53

Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com>

docs: add AWS HTTP v4 Signature

a86b0ef

tests: add very simple AWS HTTP v4 Signature test

426269d

Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com>

outscale-mgo force-pushed the introduce-v4-signature branch from 39d54f3 to 426269d Compare December 21, 2020 10:36

bagder closed this in 08e8455 Dec 21, 2020

jonrumsey mentioned this pull request Feb 3, 2021

OS400 7.75.0 build broken - missing updated for CURLOPT_AWS_SIG4 #6560

Closed

theawless mentioned this pull request May 5, 2021

Enabling aws-sigv4 integration alexcrichton/curl-rust#392

Closed

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

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

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

Conversation

outscale-mgo commented Jul 20, 2020 • edited by vszakats

bagder commented Jul 21, 2020

outscale-mgo commented Jul 21, 2020 • edited

lgtm-com bot commented Jul 21, 2020

lgtm-com bot commented Jul 24, 2020

bagder commented Jul 27, 2020

bagder commented Jul 27, 2020

bagder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bagder Aug 3, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bagder commented Oct 1, 2020 • edited

vszakats commented Oct 4, 2020 • edited

outscale-mgo commented Oct 6, 2020

outscale-mgo commented Dec 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

outscale-mgo Dec 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bagder left a comment

Choose a reason for hiding this comment

outscale-mgo commented Dec 21, 2020

bagder commented Dec 21, 2020

outscale-mgo commented Jul 20, 2020 •

edited by vszakats

outscale-mgo commented Jul 21, 2020 •

edited

bagder Aug 3, 2020 •

edited

bagder commented Oct 1, 2020 •

edited

vszakats commented Oct 4, 2020 •

edited

outscale-mgo Dec 15, 2020 •

edited