HDDS-4506. Support query parameter based v4 auth in S3g #1628

elek · 2020-11-25T14:57:16Z

What changes were proposed in this pull request?

AWS S3 accepts authorization information both from headers and query parameter.

Ozone s3g implementation parses only the headers.

This patch makes introduces the query parameter based authentication support with small reorganization of the existing logic to make it possible to extend it in this way.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-4506

How was this patch tested?

rclone uses query parameter based authentication.

rclone copy pwd ozone:bucket1/ failed without this patch, but after this patch it worked well.

...s3gateway/src/main/java/org/apache/hadoop/ozone/s3/signature/AuthorizationV4QueryParser.java

...3gateway/src/main/java/org/apache/hadoop/ozone/s3/signature/AuthorizationV4HeaderParser.java

bharatviswa504 · 2020-12-09T00:18:22Z

...ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/signature/StringToSignProducer.java

+      SignatureInfo signatureInfo,
+      ContainerRequestContext context
+  ) throws Exception {
+    return createSignatureBase(signatureInfo,


Can we combine this method and below.

And also can we call this method like createStringToSign, it is confusing as it says createSignatureBase IMO.

Sure, we can. But I kept them separated to make them more testable with separating the logic to extract information from the logic to create the sign.

I added the @VisibleForTesting annotation and simplified the test (mocks are removed) to show this concept, but I am open to merge it if you don't like it.

(See 48676ea0b)

bharatviswa504 · 2020-12-09T00:38:44Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/OzoneClientProducer.java

-      if (LOG.isDebugEnabled()) {
-        LOG.debug("Error during Client Creation: ", t);
-      }
+      //      if (LOG.isDebugEnabled()) {


Here the debug is removed, do we want to add the same when OS3Exception also?

Good, question. I am thinking what supposed to be the right error handling.

AFAIK all of these exceptions are related to the user / input errors. They are not really application errors. User errors are not required to be logged, IMHO a permission denied is part of the normal business.

I would restore the LOG.isDebugEnabled() here, if you agree... (8fe1202)

...ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/signature/StringToSignProducer.java

elek · 2021-02-08T09:59:42Z

@bharatviswa504 All of the comments are addressed, and I added more unit tests to test proper working (and X-Amz-Data handling).

Can you please and other look when you have time?

elek · 2021-02-19T15:47:20Z

@bharatviswa504 do you have time to check it by any chance?

bharatviswa504 · 2021-02-24T03:44:39Z

@elek
Sorry for the late response.
I am a little busy with other tasks, will get to it by next week. If others can help in the review, pls proceed.

elek · 2021-02-25T11:46:41Z

I am a little busy with other tasks, will get to it by next week.

Thanks a lot. Happy to give more information / explanation if you need...

elek · 2021-03-16T07:46:03Z

@bharatviswa504 @prashantpogde Did you have any time to check it by any chance? Happy to help to explain if it helps...

elek · 2021-03-22T07:28:03Z

Roses are red
Violets are blue
Eventually -- I hope
Will get a review...

cc @bharatviswa504 @prashantpogde

bharatviswa504 · 2021-03-22T13:51:15Z

Roses are red
Violets are blue
Eventually -- I hope
Will get a review...

cc @bharatviswa504 @prashantpogde

@elek Sorry for the delay, got stuck with other tasks, now they seem to be under control. I will get to this tomorrow for sure, as no one got to review this. Hope this helps.

elek · 2021-03-29T06:30:06Z

Unknown patch can make
the review process slow
but if you need more info
please: just let me know

cc @bharatviswa504 @prashantpogde

bharatviswa504 · 2021-03-23T10:48:38Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/OzoneClientProducer.java

+    }
+  }
+
+  private void getSignatureInfo() {


Minor: Unused method.

Thanks, removed.

bharatviswa504 · 2021-03-23T10:50:20Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java

 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;

+


Minor: Unnecessary change

Thanks. reverted.

bharatviswa504 · 2021-03-23T11:09:53Z

...zone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/signature/AWSSignatureProcessor.java

+
+  public SignatureInfo parseSignature() throws OS3Exception {
+
+    LowerCaseKeyStringMap headers =


Question: why do we convert header keys to lowercase?

Good question, but I am not sure. As far as I see it's added in #1098 I think I just kept the same logic what we used before...

...ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/signature/StringToSignProducer.java

bharatviswa504 · 2021-03-29T11:05:14Z

...ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/signature/StringToSignProducer.java

+    try {
+
+      return URLEncoder.encode(str, UTF_8.name())
+          .replaceAll("\\+", "%20")


Why are we doing this?

https://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-query-string-auth.html
Here there is an example urlEncode method is given, don't see such replace here?

Again, it's a good question, but I don't know exactly. I didn't modify the logic in this patch.

It was added by Ajay in apache/hadoop#561. I tried to re-organize the URLEncoder, but tests started to fail so I decided to keep all the existing logic as is.

We can try to clean up it, but I would prefer to do it outside of this patch (just to make clear what is related to the simple reorganization and what is related to a logic change...)

Yes, this is completely old code. I just see one new change.

We can try to clean up it, but I would prefer to do it outside of this patch (just to make clear what is related to the >simple reorganization and what is related to a logic change...)

Agreed, already this is a big change, no need of further refactor.

bharatviswa504 · 2021-03-29T11:12:25Z

...e/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/signature/TestStringToSignProducer.java

+            queryParameters);
+
+    Assert.assertEquals(
+        "String to sign is invalid",


Not understood here message is String to sign is invalid
But signatureBase is matching with signature.
Could you explain how/what is being tested here and how the value is generated

I may misunderstand the question, but the message is printed out when the two values are not equal:

From JUnit javadoc:

Asserts that two objects are equal. If they are not, an {@link AssertionError} is thrown with the given message.

Sorry for not being clear, my question is how you have got the siggnatureBase computed. I see in other test it is mentioned it is from AWS documentation, is this also from there?

bharatviswa504 · 2021-03-29T11:21:37Z

Thank You @elek for clear separation between Authorization Header Parsing and Sign generation.
Over all LGTM, few minor comments.

bharatviswa504 · 2021-03-29T11:23:01Z

Can we also add a docker test to test rclone tool?

elek · 2021-03-30T08:48:42Z

Thanks a lot the review @bharatviswa504

I pushed a commit with the minor fixes.

Can we also add a docker test to test rclone tool?

Good idea, I created, https://issues.apache.org/jira/browse/HDDS-5045 to track it. (We need to update the ozone-runner docker image first with rclone)

bharatviswa504 · 2021-04-01T10:36:29Z

...ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/signature/StringToSignProducer.java

+
+    StringBuilder result = new StringBuilder();
+    for (String p : params) {
+      if (!p.equals("X-Amz-Signature")) {


Question: This check is not there in the old code, newly added. Any reason for this?

This is the code which creates the canonical string of the query parameters to sign it later.

If the signature itself is added as a query parameter, it should be excluded from the string itself (to have a predictable string to sign).

TLDR: earlier we didn't support X-Amz-Signature query parameter therefore we ignored this exclusion.

bharatviswa504

@elek
Thanks for addressing the review comments.
I have one question, rest LGTM.

bharatviswa504

+1 LGTM
Thank You @elek for adding this support and for your patience.

elek · 2021-04-01T17:55:49Z

Roses are red
Violets are blue
This pr is merged
Thanks @bharatviswa504 to you

😄

* HDDS-3698-nonrolling-upgrade: (144 commits) fix project name in NOTICE.txt (apache#2112) HDDS-5066. Use fixed vesion from pnpm to build recon (apache#2115) HDDS-5014. Add non-rolling upgrade design docs. HDDS-5035. Use default config values to solve generated config file conflict (apache#2087) HDDS-5032. DN stopped to load containers on volume after a container load exception. (apache#2109) HDDS-4504. Datanode deletion config should be based on number of blocks (apache#1885) Fix ozone-ha acceptance test. HDDS-5058. Make getScmInfo retry for a duration. HDDS-4506. Support query parameter based v4 auth in S3g (apache#1628) HDDS-4553. ChunkInputStream should release buffer as soon as last byte in the buffer is read (apache#2062) HDDS-5022. SCM get roles command should provide Ratis Leader/Follower… (apache#2098) HDDS-5033. SCM may not be able to know full port list of Datanode after Datanode is started. (apache#2090) HDDS-3752. Fix o3fs list bucket contents issue when without tailing "/" (apache#2088) HDDS-4901. Remove OmOzoneAclMap from OmVolumeArgs to avoid OzoneAcl conversions (apache#1992) HDDS-4987. Import container should not delete container contents if container already exists (apache#2077) Checkstyle fix. Intialize DN layout version before security init. HDDS-4915. [SCM HA Security] Integrate CertClient. (apache#2000) HDDS-5049. Add timeout support for ratis requests in SCM HA. (apache#2099) trigger new CI check ...

* HDDS-3698-nonrolling-upgrade: (150 commits) HDDS-5056. Avoid false positiver error messages during pipeline creations (apache#2105) HDDS-5027. [SCM HA Security] Handle leader changes during bootstrap. (apache#2113) HDDS-5032. Fix findbugs (apache#2120) HDDS-5062. Add a config to bypass clusterId validation for bootstrapping SCM. (apache#2114) HDDS-5011. Introduce Java based ReplicationConfig implementation (apache#2089) HDDS-4925. Introduce ContainerBalancer in SCM with start/stop capabilities. (apache#2097) fix project name in NOTICE.txt (apache#2112) HDDS-5066. Use fixed vesion from pnpm to build recon (apache#2115) HDDS-5014. Add non-rolling upgrade design docs. HDDS-5035. Use default config values to solve generated config file conflict (apache#2087) HDDS-5032. DN stopped to load containers on volume after a container load exception. (apache#2109) HDDS-4504. Datanode deletion config should be based on number of blocks (apache#1885) Fix ozone-ha acceptance test. HDDS-5058. Make getScmInfo retry for a duration. HDDS-4506. Support query parameter based v4 auth in S3g (apache#1628) HDDS-4553. ChunkInputStream should release buffer as soon as last byte in the buffer is read (apache#2062) HDDS-5022. SCM get roles command should provide Ratis Leader/Follower… (apache#2098) HDDS-5033. SCM may not be able to know full port list of Datanode after Datanode is started. (apache#2090) HDDS-3752. Fix o3fs list bucket contents issue when without tailing "/" (apache#2088) HDDS-4901. Remove OmOzoneAclMap from OmVolumeArgs to avoid OzoneAcl conversions (apache#1992) ...

elek added 3 commits November 25, 2020 10:36

HDDS-4506. Support query parameter based v4 auth in S3g

edbd68c

fix unit tests and v2 parser

675b49a

fix rat and checkstyle problems

04b230d

elek requested a review from bharatviswa504 November 25, 2020 14:57

bharatviswa504 reviewed Dec 8, 2020

View reviewed changes

...s3gateway/src/main/java/org/apache/hadoop/ozone/s3/signature/AuthorizationV4QueryParser.java Show resolved Hide resolved

bharatviswa504 reviewed Dec 8, 2020

View reviewed changes

...3gateway/src/main/java/org/apache/hadoop/ozone/s3/signature/AuthorizationV4HeaderParser.java Outdated Show resolved Hide resolved

bharatviswa504 reviewed Dec 9, 2020

View reviewed changes

...ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/signature/StringToSignProducer.java Outdated Show resolved Hide resolved

elek added 10 commits December 9, 2020 11:11

Simplify test without mockito

fc481a3

restore debug logging

8fe1202

optimize debug log

21f7f57

stricter header check for aws v4 header parser

7dfd705

fix header conditions

8ac5026

Merge remote-tracking branch 'origin/master' into HDDS-4506

b5f8cdd

Support X-Amz-Expires header

b4938f6

Merge remote-tracking branch 'origin/master' into HDDS-4506

8380d7a

fixing query based authorization

ae95780

Merge remote-tracking branch 'origin/master' into HDDS-4506

5656624

arp7 requested a review from prashantpogde March 9, 2021 17:32

mukul1987 force-pushed the master branch from 79a9d39 to 520ba00 Compare March 25, 2021 16:05

bharatviswa504 reviewed Mar 29, 2021

View reviewed changes

address review comments

261d282

bharatviswa504 reviewed Apr 1, 2021

View reviewed changes

bharatviswa504 approved these changes Apr 1, 2021

View reviewed changes

elek merged commit 8ec5f43 into apache:master Apr 1, 2021

guihecheng pushed a commit to guihecheng/ozone that referenced this pull request Aug 3, 2021

HDDS-4506. Support query parameter based v4 auth in S3g (apache#1628)

118e1fa


		public SignatureInfo parseSignature() throws OS3Exception {

		LowerCaseKeyStringMap headers =

HDDS-4506. Support query parameter based v4 auth in S3g #1628

HDDS-4506. Support query parameter based v4 auth in S3g #1628

Uh oh!

Conversation

elek commented Nov 25, 2020

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

Uh oh!

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!

Uh oh!

elek commented Feb 8, 2021

Uh oh!

elek commented Feb 19, 2021

Uh oh!

bharatviswa504 commented Feb 24, 2021

Uh oh!

elek commented Feb 25, 2021

Uh oh!

elek commented Mar 16, 2021

Uh oh!

elek commented Mar 22, 2021

Uh oh!

bharatviswa504 commented Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elek commented Mar 29, 2021

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bharatviswa504 Apr 1, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

bharatviswa504 commented Mar 29, 2021

Uh oh!

bharatviswa504 commented Mar 29, 2021

Uh oh!

elek commented Mar 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bharatviswa504 left a comment

Choose a reason for hiding this comment

Uh oh!

bharatviswa504 left a comment

Choose a reason for hiding this comment

Uh oh!

bharatviswa504 commented Mar 22, 2021 •

edited

Loading

bharatviswa504 Apr 1, 2021 •

edited

Loading

elek commented Apr 1, 2021 •

edited

Loading