Adds support using temporary security credentials #4302

d2r · 2018-09-24T22:01:55Z

https://docs.aws.amazon.com/AmazonS3/latest/dev/RESTAuthentication.html

d2r · 2018-09-25T14:20:56Z

Looking at build issues.

d2r · 2018-09-25T15:45:20Z

plugins/s3_auth/unit_tests/test_aws_auth_v4.cc

+  };
+
+  ValidateBenchCanonicalRequest(api, /*signPayload */ false, &now, bench, include, exclude);
 }


This duplicates the previous test, but adds the new header field.

I am not sure if we need to add this unit-test.

As far as I understood the specs, if we are using temporary security credentials then the signature calculations also require the temporary security token header to be included (v4) but the token is not directly related to S3 auth v4 signing functionality (algorithm) which is unit-tested here.

Adding the temporary security credentials token header does not change the functionality of the S3 auth v4 signing in any way (only the result of a particular signature calculation because an extra arbitrary header is added). This is already tested with HeaderA ... HeaderF in the existing unit test.

It seems to me we would need to test the addition of the corresponding amz-security-token header in the plugin which this unit-test-bench does not test (it tests the v4 signing utility and not the plugin as a whole)

I hope it makes sense.

Remove the added test, because the signing of arbitrary headers is already covered by existing tests.

Add a test to confirm that the new header is added when the plugin is configured to add it.

Do I have that right?

Remove the added test, because the signing of arbitrary headers is already covered by existing tests.

right.

Add a test to confirm that the new header is added when the plugin is configured to add it.

The Catch unit tests test only the v4 calculation. Your changes are in the plugin which will be hard to test from the unit-test we have. I guess you would need to create AuTest test which you will have to be created from scratch for this plugin (it will be great but it is up to you).

OK I'll try it if there is value in having a test for this change. We have tested in a (v4) environment manually.

d2r · 2018-09-25T15:49:16Z

plugins/s3_auth/unit_tests/test_aws_auth_v4.cc

+  "bg9zErVSjEXAMPLE//////////TnBjaVKC0y/9Y3zSyB8FtGpsu5Lf83k1exfP2VEereNnx1SxaFtzxk7f61/Eq81ud4Opuo/tslm5xrufFOKa34SvDfBaBl5r1V73/"
+  "7APNC4E0f0tK4u+"
+  "uOoqyq32YXJX2yNJ0K4Ud3MLwH9LfFgDkdpFW0J4eh0Ag8kxURpqHVOWlnIzSXHuDIZwx5Oq9WiWpNJqyEPPYNgjAkKVgvdf7g56tnfkuhyMFt1LN8xwOt2dPgiUjDhe"
+  "x/3O+1LqlAPUQYzzpcn8Mqgj5tvsDz0wTMtUbTQov1hFKGuTMxScD5m16QZv3A7vSde1aGa9Vgckg5c2TU91dQdtDodmPUpMZ24HZ11oBgv0PpREXAMPLE";


This is what clang-format wants.

may be cutting the string into smaller substring would make it look nicer?

I'll try it and see.

d2r · 2018-09-25T15:59:43Z

plugins/s3_auth/aws_auth_v4.h

  const char *_awsSecretAccessKey = nullptr;
  size_t _awsSecretAccessKeyLen   = 0;
+  const char *_awsSessionToken    = nullptr;
+  size_t _awsSessionTokenLen      = 0;


This class may not need to store the token itself.

d2r · 2018-09-25T16:16:58Z

plugins/s3_auth/unit_tests/test_aws_auth_v4.cc

  };

-  ValidateBench(api, /*signePayload */ true, &now, bench, defaultIncludeHeaders, defaultExcludeHeaders);
+  ValidateBench(api, /*signPayload */ true, &now, bench, defaultIncludeHeaders, defaultExcludeHeaders);


I am willing to split these code comment changes into a separate PR if that's what we want.

When looking at the the commit at a later date one could clearly see that this is a cleanup (comment) change, unrelated to the main feature. Creating a separate PR seems unnecessary overhead.

gtenev

This seems a useful feature, thanks for working on it!

As far as I understood the specs, if we are using temporary security credentials then the temporary security token header is to be included in the signature calculations (explicitly required according to v4 spec and I assume it is also required in v2 but could not find the exact statement).

This patch assumes that temporary security credentials will be always signed only with of s3 auth v4.

Only the authorizeV4() (using class AwsAuthV4) would automatically include amz-security-token in the signature calculation depending on the --v4-include-headers and --v4-exclude-headers settings but not authorizeV2().

Do we need to worry about including amz-security-token in v2 signature? Have you tested / played with actual AWS v2 setup ?

gtenev · 2018-09-28T01:20:46Z

plugins/s3_auth/unit_tests/test_aws_auth_v4.cc

  };

-  ValidateBench(api, /*signePayload */ true, &now, bench, defaultIncludeHeaders, defaultExcludeHeaders);
+  ValidateBench(api, /*signPayload */ true, &now, bench, defaultIncludeHeaders, defaultExcludeHeaders);


When looking at the the commit at a later date one could clearly see that this is a cleanup (comment) change, unrelated to the main feature. Creating a separate PR seems unnecessary overhead.

gtenev · 2018-09-28T01:43:17Z

plugins/s3_auth/unit_tests/test_aws_auth_v4.cc

+  };
+
+  ValidateBenchCanonicalRequest(api, /*signPayload */ false, &now, bench, include, exclude);
 }


I am not sure if we need to add this unit-test.

As far as I understood the specs, if we are using temporary security credentials then the signature calculations also require the temporary security token header to be included (v4) but the token is not directly related to S3 auth v4 signing functionality (algorithm) which is unit-tested here.

Adding the temporary security credentials token header does not change the functionality of the S3 auth v4 signing in any way (only the result of a particular signature calculation because an extra arbitrary header is added). This is already tested with HeaderA ... HeaderF in the existing unit test.

It seems to me we would need to test the addition of the corresponding amz-security-token header in the plugin which this unit-test-bench does not test (it tests the v4 signing utility and not the plugin as a whole)

I hope it makes sense.

gtenev · 2018-09-28T01:44:55Z

plugins/s3_auth/unit_tests/test_aws_auth_v4.cc

+  "bg9zErVSjEXAMPLE//////////TnBjaVKC0y/9Y3zSyB8FtGpsu5Lf83k1exfP2VEereNnx1SxaFtzxk7f61/Eq81ud4Opuo/tslm5xrufFOKa34SvDfBaBl5r1V73/"
+  "7APNC4E0f0tK4u+"
+  "uOoqyq32YXJX2yNJ0K4Ud3MLwH9LfFgDkdpFW0J4eh0Ag8kxURpqHVOWlnIzSXHuDIZwx5Oq9WiWpNJqyEPPYNgjAkKVgvdf7g56tnfkuhyMFt1LN8xwOt2dPgiUjDhe"
+  "x/3O+1LqlAPUQYzzpcn8Mqgj5tvsDz0wTMtUbTQov1hFKGuTMxScD5m16QZv3A7vSde1aGa9Vgckg5c2TU91dQdtDodmPUpMZ24HZ11oBgv0PpREXAMPLE";


may be cutting the string into smaller substring would make it look nicer?

gtenev · 2018-09-28T02:03:34Z

plugins/s3_auth/s3_auth.cc

        TSError("[%s] region map is not used with AWS auth v2, parameter ignored", PLUGIN_NAME);
      }
+      if (nullptr != _token || _token_len > 0) {
+        TSError("[%s] session token is not used with AWS auth v2, parameter ignored", PLUGIN_NAME);


UsingTemporarySecurityCredentials is possible with v2 and v4. If we plan to support only v4 may be we could update the error message and the documentation accordingly.

This check seems to contradict the documentation change. The updated example in s3_auth.en.rst shows a v2 being set up along with the --session-token

@gtenev You are correct. I misread some S3 documentation and got the idea that this was a v4+ feature. I need to either clarify error messages and the documentation here or else make sure this works with v2 as well.

I need to either clarify error messages and the documentation here or else make sure this works with v2 as well.

I think v2 is probably going out of life soon so may be you could just clarify the docs and the error message (just to make sure it is clear to the operator that v2 is not supported)

Ah, if it is the case that v2 is EOL soon, it would be better not to add code to handle that case. I'll make the change you suggest so it is clear in case anyone accidentally tries it with v2.

d2r · 2018-10-13T20:35:23Z

OK, sorry this took so long. I addressed review comments:

Squashed commits, old commit was a0d9df3
Opted not to add the functional test
Updated documentation to be consistent

d2r · 2018-10-22T15:41:32Z

@gtenev ping

gtenev · 2018-10-22T18:10:46Z

doc/admin-guide/plugins/s3_auth.en.rst

-    version=2
+    session_token=my-token
+    version=4
    virtual_host=yes


virtual_host is a v2 specific parameter. We have 2 options here:

remove virtual_host from this example (otherwise plugin will warn in the log on each realod/restart)

change the example back to v2 like it was before (you have already added a good example in the v4 specific section) and remove session_token=my-token

Thanks, opting for 2.

https://docs.aws.amazon.com/AmazonS3/latest/dev/RESTAuthentication.html

d2r · 2018-10-22T19:18:11Z

@gtenev Addressed review comment.

bryancall · 2019-03-27T22:36:29Z

Cherry picked to 8.1.0

d2r added the Plugins label Sep 24, 2018

d2r added this to the 9.0.0 milestone Sep 24, 2018

d2r self-assigned this Sep 24, 2018

d2r requested a review from gtenev September 24, 2018 22:01

d2r added the WIP label Sep 24, 2018

d2r force-pushed the s3_add_session_token branch from 7325222 to da353f6 Compare September 24, 2018 22:08

d2r added WIP and removed WIP labels Sep 24, 2018

d2r force-pushed the s3_add_session_token branch from da353f6 to 4b8f6d7 Compare September 25, 2018 15:44

d2r removed the WIP label Sep 25, 2018

d2r commented Sep 25, 2018

View reviewed changes

d2r force-pushed the s3_add_session_token branch from 4b8f6d7 to cfcc6dc Compare September 25, 2018 15:48

d2r commented Sep 25, 2018

View reviewed changes

d2r added the WIP label Sep 25, 2018

d2r force-pushed the s3_add_session_token branch from cfcc6dc to a0d9df3 Compare September 25, 2018 16:14

d2r commented Sep 25, 2018

View reviewed changes

d2r removed the WIP label Sep 26, 2018

gtenev requested changes Sep 28, 2018

View reviewed changes

d2r force-pushed the s3_add_session_token branch from a0d9df3 to c8f7cf0 Compare October 13, 2018 20:30

gtenev requested changes Oct 22, 2018

View reviewed changes

Adds v4 support using temporary security token

3e08165

https://docs.aws.amazon.com/AmazonS3/latest/dev/RESTAuthentication.html

d2r force-pushed the s3_add_session_token branch from c8f7cf0 to 3e08165 Compare October 22, 2018 19:17

gtenev approved these changes Oct 23, 2018

View reviewed changes

d2r merged commit 3ae02b1 into apache:master Oct 23, 2018

bryancall removed this from the 9.0.0 milestone Mar 27, 2019

bryancall added this to the 8.1.0 milestone Mar 27, 2019

zwoop modified the milestones: 8.1.0, 8.1.0-nogo Mar 18, 2020

Adds support using temporary security credentials #4302

Adds support using temporary security credentials #4302

Conversation

d2r commented Sep 24, 2018

Uh oh!

d2r commented Sep 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gtenev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d2r Oct 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d2r commented Oct 13, 2018

Uh oh!

d2r commented Oct 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d2r commented Oct 22, 2018

Uh oh!

bryancall commented Mar 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

d2r Oct 1, 2018 •

edited

Loading