diff --git a/config/cmake/runProxy.cmake b/config/cmake/runProxy.cmake index dc7164c3830..77e41a026f4 100644 --- a/config/cmake/runProxy.cmake +++ b/config/cmake/runProxy.cmake @@ -28,6 +28,9 @@ # - TEST_ENV_VAR/TEST_ENV_VALUE: (optional) Environment variable to set # - TEST_FILES: (optional) List of files to upload # - TEST_ACLS: (optional) List of ACLs for files (anon/public-read) +# - TEST_KEYS: (optional) List of object keys to upload files under; each +# entry overrides the object key for the corresponding TEST_FILES entry +# (an empty entry falls back to the file name) # - TEST_NOERRDISPLAY: (optional) If set, suppress error output display on failure # ----------------------------------------------------------------------------- @@ -165,33 +168,39 @@ if (NOT TEST_RESULT EQUAL TEST_EXPECT) message (FATAL_ERROR "Failed: Create-Bucket exited != ${TEST_EXPECT}.\n${TEST_ERROR}") endif () -# Upload test files to the bucket, handling ACLs if provided +# Upload test files to the bucket, handling ACLs and object keys if provided if (TEST_FILES AND TEST_ACLS) - foreach (dfile dacls IN ZIP_LISTS TEST_FILES TEST_ACLS) + foreach (dfile dacls dkey IN ZIP_LISTS TEST_FILES TEST_ACLS TEST_KEYS) + if ("${dkey}" STREQUAL "") + set (dkey "${dfile}") + endif () + # Object keys may contain characters that are not filesystem-safe, + # so sanitize the key for use in log file names + string (MAKE_C_IDENTIFIER "${dkey}" dlog) if (dacls STREQUAL "anon") execute_process ( - COMMAND aws s3api put-object --acl public-read --endpoint-url=http://localhost:${TEST_PORT} --body ${TEST_FOLDER}/testfiles/${dfile} --bucket ${TEST_BUCKET} --key ${dfile} + COMMAND aws s3api put-object --acl public-read --endpoint-url=http://localhost:${TEST_PORT} --body ${TEST_FOLDER}/testfiles/${dfile} --bucket ${TEST_BUCKET} --key ${dkey} WORKING_DIRECTORY ${TEST_FOLDER} RESULT_VARIABLE TEST_RESULT - OUTPUT_FILE s3proxy-${dfile}.out - ERROR_FILE s3proxy-${dfile}.err + OUTPUT_FILE s3proxy-${dlog}.out + ERROR_FILE s3proxy-${dlog}.err OUTPUT_VARIABLE TEST_OUT ERROR_VARIABLE TEST_ERROR ) else () execute_process ( - COMMAND aws s3api put-object --endpoint-url=http://localhost:${TEST_PORT} --body ${TEST_FOLDER}/testfiles/${dfile} --bucket ${TEST_BUCKET} --key ${dfile} + COMMAND aws s3api put-object --endpoint-url=http://localhost:${TEST_PORT} --body ${TEST_FOLDER}/testfiles/${dfile} --bucket ${TEST_BUCKET} --key ${dkey} WORKING_DIRECTORY ${TEST_FOLDER} RESULT_VARIABLE TEST_RESULT - OUTPUT_FILE s3proxy-${dfile}.out - ERROR_FILE s3proxy-${dfile}.err + OUTPUT_FILE s3proxy-${dlog}.out + ERROR_FILE s3proxy-${dlog}.err OUTPUT_VARIABLE TEST_OUT ERROR_VARIABLE TEST_ERROR ) endif () # Print the output of the put-object command if it exists - if (EXISTS "${TEST_FOLDER}/s3proxy-${dfile}.out") - file (READ ${TEST_FOLDER}/s3proxy-${dfile}.out TEST_STREAM) + if (EXISTS "${TEST_FOLDER}/s3proxy-${dlog}.out") + file (READ ${TEST_FOLDER}/s3proxy-${dlog}.out TEST_STREAM) message (VERBOSE "Output USING ${TEST_BUCKET}:\n${TEST_STREAM}") endif () message (VERBOSE "COMMAND Put Result: ${TEST_RESULT}") @@ -199,8 +208,8 @@ if (TEST_FILES AND TEST_ACLS) # If the return value is not as expected, print error output and fail if (NOT TEST_RESULT EQUAL TEST_EXPECT) if (NOT TEST_NOERRDISPLAY) - if (EXISTS "${TEST_FOLDER}/s3proxy-${dfile}.err") - file (READ ${TEST_FOLDER}/s3proxy-${dfile}.err TEST_STREAM) + if (EXISTS "${TEST_FOLDER}/s3proxy-${dlog}.err") + file (READ ${TEST_FOLDER}/s3proxy-${dlog}.err TEST_STREAM) message (STATUS "Error output USING ${TEST_BUCKET}:\n${TEST_STREAM}") endif () endif () @@ -208,19 +217,25 @@ if (TEST_FILES AND TEST_ACLS) endif () endforeach () elseif (TEST_FILES) - foreach (dfile ${TEST_FILES}) + foreach (dfile dkey IN ZIP_LISTS TEST_FILES TEST_KEYS) + if ("${dkey}" STREQUAL "") + set (dkey "${dfile}") + endif () + # Object keys may contain characters that are not filesystem-safe, + # so sanitize the key for use in log file names + string (MAKE_C_IDENTIFIER "${dkey}" dlog) execute_process ( - COMMAND aws s3api put-object --endpoint-url=http://localhost:${TEST_PORT} --body ${TEST_FOLDER}/testfiles/${dfile} --bucket ${TEST_BUCKET} --key ${dfile} + COMMAND aws s3api put-object --endpoint-url=http://localhost:${TEST_PORT} --body ${TEST_FOLDER}/testfiles/${dfile} --bucket ${TEST_BUCKET} --key ${dkey} WORKING_DIRECTORY ${TEST_FOLDER} RESULT_VARIABLE TEST_RESULT - OUTPUT_FILE s3proxy-${dfile}.out - ERROR_FILE s3proxy-${dfile}.err + OUTPUT_FILE s3proxy-${dlog}.out + ERROR_FILE s3proxy-${dlog}.err OUTPUT_VARIABLE TEST_OUT ERROR_VARIABLE TEST_ERROR ) # Print the output of the put-object command if it exists - if (EXISTS "${TEST_FOLDER}/s3proxy-${dfile}.out") - file (READ ${TEST_FOLDER}/s3proxy-${dfile}.out TEST_STREAM) + if (EXISTS "${TEST_FOLDER}/s3proxy-${dlog}.out") + file (READ ${TEST_FOLDER}/s3proxy-${dlog}.out TEST_STREAM) message (VERBOSE "Output USING ${TEST_BUCKET}:\n${TEST_STREAM}") endif () message (VERBOSE "COMMAND Put Result: ${TEST_RESULT}") @@ -228,8 +243,8 @@ elseif (TEST_FILES) # If the return value is not as expected, print error output and fail if (NOT TEST_RESULT EQUAL TEST_EXPECT) if (NOT TEST_NOERRDISPLAY) - if (EXISTS "${TEST_FOLDER}/s3proxy-${dfile}.err") - file (READ ${TEST_FOLDER}/s3proxy-${dfile}.err TEST_STREAM) + if (EXISTS "${TEST_FOLDER}/s3proxy-${dlog}.err") + file (READ ${TEST_FOLDER}/s3proxy-${dlog}.err TEST_STREAM) message (STATUS "Error output USING ${TEST_BUCKET}:\n${TEST_STREAM}") endif () endif () diff --git a/release_docs/CHANGELOG.md b/release_docs/CHANGELOG.md index e1d2b9e71fc..c9d5ac47ad6 100644 --- a/release_docs/CHANGELOG.md +++ b/release_docs/CHANGELOG.md @@ -144,6 +144,10 @@ The `h5repack` tool now obtains its default low and high library version bounds ## Library +### HTTP 403 errors in the ROS3 VFD for object keys with special characters + + The ROS3 VFD did not URI-encode the S3 object key when building the HTTP request path, so keys containing characters that AWS Signature Version 4 requires to be percent-encoded — such as the '=' in Hive-style `key=value` partition prefixes, '+', or spaces — produced a signed request whose signature did not match S3's server-side recomputation. S3 rejects such requests with `SignatureDoesNotMatch`, which surfaces as an HTTP 403 error (indistinguishable from a permissions error on a HEAD request), even though tools like the AWS CLI could access the same object. The object key is now percent-encoded exactly once when the request path is built, matching the behavior of other S3 clients. Note that URLs must now be passed to the ROS3 VFD with their object keys unencoded; a key that was pre-encoded as a workaround for this issue will now be double-encoded and fail to resolve. + ### Fixed file descriptor leaks in stdio VFD error paths Fixed multiple resource leaks in the H5FDstdio driver where file descriptors were not properly closed on error paths. The error handling code was incorrectly attempting to close a local variable instead of the file pointer stored in the file structure, leading to file descriptor leaks. This issue affected 5 error paths in `H5FD_stdio_open()` and could cause file descriptor exhaustion in long-running applications. diff --git a/src/H5FDros3_s3comms.c b/src/H5FDros3_s3comms.c index 85f9fcad9b5..78084652644 100644 --- a/src/H5FDros3_s3comms.c +++ b/src/H5FDros3_s3comms.c @@ -1975,8 +1975,16 @@ H5FD__s3comms_format_http_request_message(const H5FD__s3comms_aws_params_t *aws_ aws_error_str(aws_last_error())); } - if (AWS_OP_SUCCESS != aws_byte_buf_append_dynamic(&path_buf, &key_cursor)) - HGOTO_ERROR(H5E_VFL, H5E_CANTALLOC, FAIL, "couldn't allocate memory for HTTP request string: %s", + /* The object key must be URI-encoded ('/' is preserved) when it is + * added to the request path. S3 computes request signatures from a + * singly URI-encoded path and the signing configuration sets + * use_double_uri_encode to false, so the path is signed and sent + * exactly as built here. If reserved characters in the key, such as + * '=' in Hive-style "key=value" partition names, are sent literally, + * S3 rejects the request with SignatureDoesNotMatch (HTTP 403). + */ + if (AWS_OP_SUCCESS != aws_byte_buf_append_encoding_uri_path(&path_buf, &key_cursor)) + HGOTO_ERROR(H5E_VFL, H5E_CANTALLOC, FAIL, "couldn't URI-encode object key for HTTP request: %s", aws_error_str(aws_last_error())); H5_WARN_AGGREGATE_RETURN_OFF diff --git a/test/CMakeTests.cmake b/test/CMakeTests.cmake index 2ccec130123..a41dbe746b6 100644 --- a/test/CMakeTests.cmake +++ b/test/CMakeTests.cmake @@ -664,7 +664,7 @@ endif () add_test (NAME H5TEST-tcheck_version-release COMMAND $ "-tr") set_tests_properties (H5TEST-tcheck_version-release PROPERTIES WORKING_DIRECTORY ${HDF5_TEST_BINARY_DIR}/H5TEST -) +) if ("H5TEST-tcheck_version-release" MATCHES "${HDF5_DISABLE_TESTS_REGEX}") set_tests_properties (H5TEST-tcheck_version-release PROPERTIES DISABLED true) endif () @@ -705,8 +705,9 @@ if (HDF5_ENABLE_ROS3_VFD_DOCKER_PROXY) -D "TEST_PORT=${h5test_s3tests_port}" -D "TEST_ARGS:STRING=s3proxy-local-fs" -D "TEST_BUCKET:STRING=hdf5ros3" - -D "TEST_FILES:STRING=t8.shakespeare.txt;Poe_Raven.txt;charsets.h5" - -D "TEST_ACLS:STRING=skip;anon;anon" + -D "TEST_FILES:STRING=t8.shakespeare.txt;Poe_Raven.txt;charsets.h5;charsets.h5" + -D "TEST_ACLS:STRING=skip;anon;anon;skip" + -D "TEST_KEYS:STRING=t8.shakespeare.txt;Poe_Raven.txt;charsets.h5;src=h5test/fmt=hive+style@cloud/charsets.h5" -D "TEST_EXPECT=0" -D "TEST_ENV_VAR:STRING=AWS_SHARED_CREDENTIALS_FILE" -D "TEST_ENV_VALUE:STRING=${CMAKE_BINARY_DIR}/credentials" diff --git a/test/ros3.c b/test/ros3.c index 746e11cfdb4..5d5b85d4348 100644 --- a/test/ros3.c +++ b/test/ros3.c @@ -46,6 +46,12 @@ #define S3_TEST_RESOURCE_H5_PUBLIC "charsets.h5" #define S3_TEST_RESOURCE_MISSING "missing.csv" +/* Object key containing characters ('=', '+') that must be URI-encoded + * when request paths are built and signed; mimics Hive-style "key=value" + * partition naming + */ +#define S3_TEST_RESOURCE_H5_HIVE_KEY "src=h5test/fmt=hive+style@cloud/charsets.h5" + #define S3_TEST_RESOURCE_TEXT_RESTRICTED_SIZE 5458199 #define S3_TEST_RESOURCE_TEXT_PUBLIC_SIZE 6464 #define S3_TEST_RESOURCE_TEXT_PUBLIC_SIZEOVER 6400 @@ -54,6 +60,7 @@ static char url_text_restricted[S3_TEST_MAX_URL_SIZE] = ""; static char url_text_public[S3_TEST_MAX_URL_SIZE] = ""; static char url_h5_public[S3_TEST_MAX_URL_SIZE] = ""; +static char url_h5_hive_key[S3_TEST_MAX_URL_SIZE] = ""; static char url_missing[S3_TEST_MAX_URL_SIZE] = ""; static char s3_test_bucket_url[S3_TEST_MAX_URL_SIZE] = ""; static bool s3_test_bucket_defined = false; @@ -1068,6 +1075,75 @@ error: return 1; } /* end test_ros3_access_modes() */ + +/*--------------------------------------------------------------------------- + * Function: test_hive_style_object_key + * + * Purpose: Test opening a file whose object key contains characters, + * such as the '=' in Hive-style "key=value" partition names, + * that must be URI-encoded when request paths are built. + * If the key is not URI-encoded exactly once, S3 rejects + * the requests with HTTP 403 (SignatureDoesNotMatch). + * + * Return: PASS : 0 + * FAIL : 1 + *--------------------------------------------------------------------------- + */ +static int +test_hive_style_object_key(void) +{ + hid_t fid = H5I_INVALID_HID; + hid_t fapl_id = H5I_INVALID_HID; + + TESTING("ros3 object keys with characters requiring URI encoding"); + + if (s3_test_credentials_loaded == 0) { + SKIPPED(); + puts(" s3 credentials are not loaded"); + fflush(stdout); + return 0; + } + + if (false == s3_test_bucket_defined) { + SKIPPED(); + puts(" environment variable HDF5_ROS3_TEST_BUCKET_URL not defined"); + fflush(stdout); + return 0; + } + + if ((fapl_id = H5Pcreate(H5P_FILE_ACCESS)) < 0) + TEST_ERROR; + if (H5Pset_fapl_ros3(fapl_id, &restricted_access_fa) < 0) + TEST_ERROR; + if (*s3_test_aws_session_token != '\0') + if (H5Pset_fapl_ros3_token(fapl_id, s3_test_aws_session_token) < 0) + TEST_ERROR; + + /* The object is not public, so all of the requests made when opening + * and reading the file must be signed + */ + if ((fid = H5Fopen(url_h5_hive_key, H5F_ACC_RDONLY, fapl_id)) < 0) + TEST_ERROR; + + if (H5Fclose(fid) < 0) + TEST_ERROR; + if (H5Pclose(fapl_id) < 0) + TEST_ERROR; + + PASSED(); + return 0; + +error: + H5E_BEGIN_TRY + { + H5Fclose(fid); + H5Pclose(fapl_id); + } + H5E_END_TRY + + return 1; + +} /* end test_hive_style_object_key() */ #endif /* H5_HAVE_ROS3_VFD */ /*------------------------------------------------------------------------- @@ -1130,6 +1206,13 @@ main(void) ret_value = EXIT_FAILURE; goto done; } + if (S3_TEST_MAX_URL_SIZE < snprintf(url_h5_hive_key, (size_t)S3_TEST_MAX_URL_SIZE, "%s/%s", + (const char *)s3_test_bucket_url, + (const char *)S3_TEST_RESOURCE_H5_HIVE_KEY)) { + printf("* ros3 setup failed (h5_hive_key) ! *\n"); + ret_value = EXIT_FAILURE; + goto done; + } if (S3_TEST_MAX_URL_SIZE < snprintf(url_missing, S3_TEST_MAX_URL_SIZE, "%s/%s", (const char *)s3_test_bucket_url, (const char *)S3_TEST_RESOURCE_MISSING)) { @@ -1231,6 +1314,7 @@ main(void) nerrors += test_noops_and_autofails(); nerrors += test_cmp(); nerrors += test_ros3_access_modes(); + nerrors += test_hive_style_object_key(); } if (H5FD__s3comms_term() < 0) {