-
Notifications
You must be signed in to change notification settings - Fork 625
HDDS-12870. Fix listObjects corner cases #8307
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
Changes from 4 commits
6f61f55
365c67b
7bd5677
19f7ba5
f283cc0
5035f36
7611798
228bacd
a4e074d
beffb26
c8d7db3
7e2d5d4
ab8092b
edcb530
93f6cb8
ff1408f
aaf2783
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,181 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.hadoop.ozone.s3.rest; | ||
|
|
||
| import java.net.HttpURLConnection; | ||
| import java.nio.charset.StandardCharsets; | ||
| import java.security.MessageDigest; | ||
| import java.text.SimpleDateFormat; | ||
| import java.util.Date; | ||
| import java.util.TimeZone; | ||
| import javax.crypto.Mac; | ||
| import javax.crypto.spec.SecretKeySpec; | ||
|
|
||
| /** | ||
| * Utility for generating AWS S3 Signature V4 headers for HTTP requests. | ||
| * Only supports unsigned payloads (useful for GET/HEAD). | ||
| */ | ||
| public final class S3V4Signer { | ||
| private S3V4Signer() { | ||
| throw new AssertionError("Utility class"); | ||
| } | ||
|
|
||
| /** | ||
| * Signs the given HttpURLConnection with AWS S3 Signature V4 headers. | ||
| * | ||
| * @param conn The HttpURLConnection to sign | ||
| * @param accessKey AWS access key | ||
| * @param secretKey AWS secret key | ||
| * @param region AWS region (e.g., "us-east-1") | ||
| * @param service AWS service (e.g., "s3") | ||
| * @param bucket S3 bucket name | ||
| * @param queryString The query string (e.g., "max-keys=-1") | ||
| */ | ||
| public static void signRequest(HttpURLConnection conn, String accessKey, String secretKey, String region, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you have any reference for this signing process? |
||
| String service, String bucket, String queryString) { | ||
| try { | ||
| String method = conn.getRequestMethod(); | ||
| String host = conn.getURL().getHost(); | ||
| int port = conn.getURL().getPort(); | ||
| if (port != -1 && port != 80 && port != 443) { | ||
| host = host + ":" + port; | ||
| } | ||
| String amzDate = getAmzDate(); | ||
| String dateStamp = getDateStamp(); | ||
|
|
||
| String payloadHash = hash(""); // SHA256("") for empty payload | ||
| String canonicalUri = "/" + bucket + "/"; | ||
| String canonicalHeaders = "host:" + host + "\n" + | ||
| "x-amz-content-sha256:" + payloadHash + "\n" + | ||
| "x-amz-date:" + amzDate + "\n"; | ||
| String signedHeaders = "host;x-amz-content-sha256;x-amz-date"; | ||
|
|
||
| String canonicalRequest = method + "\n" + | ||
| canonicalUri + "\n" + | ||
| queryString + "\n" + | ||
| canonicalHeaders + "\n" + | ||
| signedHeaders + "\n" + | ||
| payloadHash; | ||
|
|
||
| String algorithm = "AWS4-HMAC-SHA256"; | ||
| String credentialScope = dateStamp + "/" + region + "/" + service + "/aws4_request"; | ||
| String stringToSign = algorithm + "\n" + | ||
| amzDate + "\n" + | ||
| credentialScope + "\n" + | ||
| hash(canonicalRequest); | ||
|
|
||
| byte[] signingKey = getSignatureKey(secretKey, dateStamp, region, service); | ||
| String signature = bytesToHex(hmacSHA256(stringToSign, signingKey)); | ||
|
|
||
| String authorizationHeader = algorithm + " " + | ||
| "Credential=" + accessKey + "/" + credentialScope + ", " + | ||
| "SignedHeaders=" + signedHeaders + ", " + | ||
| "Signature=" + signature; | ||
|
|
||
| conn.setRequestProperty("x-amz-date", amzDate); | ||
| conn.setRequestProperty("x-amz-content-sha256", payloadHash); | ||
| conn.setRequestProperty("Authorization", authorizationHeader); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException("S3V4Signer failed to sign request: " + e.getMessage(), e); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Returns the current date in the format "yyyyMMdd'T'HHmmss'Z'". | ||
| * | ||
| * @return The current date in the format "yyyyMMdd'T'HHmmss'Z'" | ||
| */ | ||
| private static String getAmzDate() { | ||
| SimpleDateFormat fmt = new SimpleDateFormat("yyyyMMdd'T'HHmmss'Z'"); | ||
| fmt.setTimeZone(TimeZone.getTimeZone("UTC")); | ||
| return fmt.format(new Date()); | ||
| } | ||
|
|
||
| /** | ||
| * Returns the current date in the format "yyyyMMdd". | ||
| * | ||
| * @return The current date in the format "yyyyMMdd" | ||
| */ | ||
| private static String getDateStamp() { | ||
| SimpleDateFormat fmt = new SimpleDateFormat("yyyyMMdd"); | ||
| fmt.setTimeZone(TimeZone.getTimeZone("UTC")); | ||
| return fmt.format(new Date()); | ||
| } | ||
|
|
||
| /** | ||
| * Returns the SHA-256 hash of the given string. | ||
| * | ||
| * @param text The string to hash | ||
| * @return The SHA-256 hash of the given string | ||
| * @throws Exception If hashing fails | ||
| */ | ||
| private static String hash(String text) throws Exception { | ||
| MessageDigest md = MessageDigest.getInstance("SHA-256"); | ||
| md.update(text.getBytes(StandardCharsets.UTF_8)); | ||
| byte[] digest = md.digest(); | ||
| return bytesToHex(digest); | ||
| } | ||
|
|
||
| /** | ||
| * Returns the HMAC-SHA256 of the given data using the given key. | ||
| * | ||
| * @param data The data to sign | ||
| * @param key The key to use for signing | ||
| * @return The HMAC-SHA256 of the given data using the given key | ||
| * @throws Exception If signing fails | ||
| */ | ||
| private static byte[] hmacSHA256(String data, byte[] key) throws Exception { | ||
| String algorithm = "HmacSHA256"; | ||
| Mac mac = Mac.getInstance(algorithm); | ||
| mac.init(new SecretKeySpec(key, algorithm)); | ||
| return mac.doFinal(data.getBytes(StandardCharsets.UTF_8)); | ||
| } | ||
|
|
||
| /** | ||
| * Returns the signature key for the given secret key, date stamp, region, and service. | ||
| * | ||
| * @param key The secret key | ||
| * @param dateStamp The date stamp | ||
| * @param regionName The region name | ||
| * @param serviceName The service name | ||
| * @return The signature key for the given secret key, date stamp, region, and service | ||
| * @throws Exception If key derivation fails | ||
| */ | ||
| private static byte[] getSignatureKey(String key, String dateStamp, String regionName, String serviceName) | ||
| throws Exception { | ||
| byte[] kSecret = ("AWS4" + key).getBytes(StandardCharsets.UTF_8); | ||
| byte[] kDate = hmacSHA256(dateStamp, kSecret); | ||
| byte[] kRegion = hmacSHA256(regionName, kDate); | ||
| byte[] kService = hmacSHA256(serviceName, kRegion); | ||
| return hmacSHA256("aws4_request", kService); | ||
| } | ||
|
|
||
| /** | ||
| * Returns the hexadecimal representation of the given bytes. | ||
| * | ||
| * @param bytes The bytes to convert | ||
| * @return The hexadecimal representation of the given bytes | ||
| */ | ||
| private static String bytesToHex(byte[] bytes) { | ||
| StringBuilder sb = new StringBuilder(); | ||
| for (byte b : bytes) { | ||
| sb.append(String.format("%02x", b)); | ||
| } | ||
| return sb.toString(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.hadoop.ozone.s3.rest; | ||
|
|
||
| import static org.apache.hadoop.ozone.OzoneConsts.LOCALHOST; | ||
| import static org.apache.hadoop.ozone.s3.S3GatewayConfigKeys.OZONE_S3G_HTTP_ADDRESS_KEY; | ||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
|
|
||
| import java.net.HttpURLConnection; | ||
| import java.net.URL; | ||
| import org.apache.hadoop.hdds.conf.OzoneConfiguration; | ||
| import org.apache.hadoop.hdds.scm.ScmConfigKeys; | ||
| import org.apache.hadoop.ozone.MiniOzoneCluster; | ||
| import org.apache.hadoop.ozone.s3.S3GatewayService; | ||
| import org.apache.ozone.test.OzoneTestBase; | ||
| import org.junit.jupiter.api.AfterAll; | ||
| import org.junit.jupiter.api.BeforeAll; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| /** | ||
| * Integration tests for S3 REST edge cases that cannot be triggered by AWS SDK clients. | ||
| * For example: negative/zero max-keys, or other invalid parameters that SDK would block client-side. | ||
| */ | ||
| public class TestS3RestNonSdkCases extends OzoneTestBase { | ||
|
|
||
| private static MiniOzoneCluster cluster = null; | ||
| private static S3GatewayService s3g = null; | ||
| private static final String ACCESS_KEY = "testuser"; | ||
| private static final String SECRET_KEY = "testpass"; | ||
|
|
||
| @BeforeAll | ||
| public static void startCluster() throws Exception { | ||
| OzoneConfiguration conf = new OzoneConfiguration(); | ||
| conf.setInt(ScmConfigKeys.OZONE_SCM_PIPELINE_OWNER_CONTAINER_COUNT, 1); | ||
| s3g = new S3GatewayService(); | ||
|
|
||
| cluster = MiniOzoneCluster.newBuilder(conf) | ||
| .addService(s3g) | ||
| .setNumDatanodes(3) | ||
| .build(); | ||
| cluster.waitForClusterToBeReady(); | ||
| cluster.newClient().getObjectStore().createS3Bucket(getTestBucketName()); | ||
| } | ||
|
|
||
| @AfterAll | ||
| public static void shutdownCluster() { | ||
| if (cluster != null) { | ||
| cluster.shutdown(); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void testListObjectsWithNegativeMaxKeys() throws Exception { | ||
| final String bucketName = getTestBucketName(); | ||
| String s3Endpoint = getS3EndpointURL(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can move to @BeforeAll |
||
| String queryString = "max-keys=-1"; | ||
| String url = s3Endpoint + "/" + bucketName + "/?" + queryString; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you build this with url builder?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Revert the whole integration test as smoke test might be more suitable for this case. |
||
|
|
||
| HttpURLConnection conn = (HttpURLConnection) new URL(url).openConnection(); | ||
| conn.setRequestMethod("GET"); | ||
| S3V4Signer.signRequest(conn, ACCESS_KEY, SECRET_KEY, "us-east-1", "s3", bucketName, queryString); | ||
|
|
||
| int code = conn.getResponseCode(); | ||
| assertEquals(HttpURLConnection.HTTP_BAD_REQUEST, code, | ||
| "Should return 400 Bad Request (InvalidArgument) for max-keys=-1"); | ||
| } | ||
|
|
||
| private static String getS3EndpointURL() { | ||
| String addr = s3g.getConf().get(OZONE_S3G_HTTP_ADDRESS_KEY); | ||
| String hostPort = addr.replace("0.0.0.0", LOCALHOST); | ||
| return "http://" + hostPort; | ||
| } | ||
|
|
||
| private static String getTestBucketName() { | ||
| return ("testrestnegmaxkeys" + System.currentTimeMillis()).toLowerCase(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -142,6 +142,8 @@ public Response get( | |||||
| return listMultipartUploads(bucketName, prefix, keyMarker, uploadIdMarker, maxUploads); | ||||||
| } | ||||||
|
|
||||||
| maxKeys = validateMaxKeys(maxKeys); | ||||||
|
|
||||||
| if (prefix == null) { | ||||||
| prefix = ""; | ||||||
| } | ||||||
|
|
@@ -292,6 +294,14 @@ public Response get( | |||||
| return Response.ok(response).build(); | ||||||
| } | ||||||
|
|
||||||
| private int validateMaxKeys(int maxKeys) throws OS3Exception { | ||||||
| if (maxKeys <= 0) { | ||||||
| throw S3ErrorTable.newError(S3ErrorTable.INVALID_ARGUMENT, "maxKeys must be > 0"); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| } | ||||||
|
|
||||||
| return Math.min(maxKeys, 1000); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same behaviour as AWS S3 does?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In AWS S3 ListObjects & ListObjectsV2 API reference, they both mention below.
Therefore I think add the maxKeys up to 1000 is appropriate. reference:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This sever side max needs to be configurable. Depending on deployment scale and resources, a larger number maybe desired for performance.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I implement this function but not yet commit. If below statement is not wrong, should config maxKeys still need? Or for the purpose to reduce maxKeys like limit to 500(some number is smaller than 1000)?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that AWS has a limit it enforces. For Ozone, we should set the default to 1000, but in some high-performance use cases, it might make sense to have it higher as well, where the listing needs to be completed in one round trip. If we make it configurable, the customer will have a choice.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Get it now. I kind of stuck at AWS S3 protocol, but in Ozone we can make it more flexible.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
| } | ||||||
|
|
||||||
| @PUT | ||||||
| public Response put(@PathParam("bucket") String bucketName, | ||||||
| @QueryParam("acl") String aclMarker, | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -521,6 +521,28 @@ public void testEncodingTypeException() throws IOException { | |
| assertEquals(S3ErrorTable.INVALID_ARGUMENT.getCode(), e.getCode()); | ||
| } | ||
|
|
||
| @Test | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should add another test case to check if the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@Jimmyweng006 Use setConfig of the builder to set the config with custom lower max key limit, then call
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the advice @peterxcli , I update the PR but local CI will take some time.
The new commit achieves above testing scenario with a lower max key limit, please help have a look later. Thank you. |
||
| public void testListObjectsWithInvalidMaxKeys() throws Exception { | ||
| OzoneClient client = createClientWithKeys("file1"); | ||
| BucketEndpoint bucketEndpoint = EndpointBuilder.newBucketEndpointBuilder() | ||
| .setClient(client) | ||
| .build(); | ||
|
|
||
| // maxKeys < 0 | ||
| OS3Exception e1 = assertThrows(OS3Exception.class, () -> | ||
| bucketEndpoint.get("bucket", null, null, null, -1, null, | ||
| null, null, null, null, null, null, 1000, null) | ||
| ); | ||
| assertEquals(S3ErrorTable.INVALID_ARGUMENT.getCode(), e1.getCode()); | ||
|
|
||
| // maxKeys == 0 | ||
| OS3Exception e2 = assertThrows(OS3Exception.class, () -> | ||
| bucketEndpoint.get("bucket", null, null, null, 0, null, | ||
| null, null, null, null, null, null, 1000, null) | ||
| ); | ||
| assertEquals(S3ErrorTable.INVALID_ARGUMENT.getCode(), e2.getCode()); | ||
| } | ||
|
|
||
| private void assertEncodingTypeObject( | ||
| String exceptName, String exceptEncodingType, EncodingTypeObject object) { | ||
| assertEquals(exceptName, object.getName()); | ||
|
|
||

Uh oh!
There was an error while loading. Please reload this page.