github-actions[bot] commented on code in PR #68540: URL: https://github.com/apache/doris/pull/68540#discussion_r4215142986
########## fe/fe-filesystem/fe-filesystem-gcs/src/main/java/org/apache/doris/filesystem/gcs/GcsObjStorage.java: ########## @@ -0,0 +1,58 @@ +// 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.doris.filesystem.gcs; + +import org.apache.doris.filesystem.s3.S3FileSystemProperties; +import org.apache.doris.filesystem.s3.S3ObjStorage; + +import software.amazon.awssdk.services.s3.S3Client; + +import java.io.IOException; + +/** S3-compatible GCS client with refreshable Google OAuth credentials. */ +final class GcsObjStorage extends S3ObjStorage { + private final GcsFileSystemProperties properties; + + GcsObjStorage(S3FileSystemProperties delegate, GcsFileSystemProperties properties) { + super(delegate, properties.getSupportedSchemes()); + this.properties = properties; + } + + @Override + public String getPresignedUrl(String objectKey) throws IOException { + if (properties.getAuth().getNativeCredential().isPresent()) { Review Comment: [P1] Provide an upload URL for native GCP internal stages. The `/copy/upload` handler resolves a GCP internal stage to this filesystem and calls `getPresignedUrl(fileName)` before redirecting the client to upload. Every native GCP credential reaches this unconditional exception, so the handler returns an error and users cannot upload files to that stage for COPY. Add a native GCS V4 signed PUT path (or another working upload path) and cover the native internal-stage API flow. ########## common/cpp/obj-client/auth/gcp/gcs_signed_url.cpp: ########## @@ -0,0 +1,194 @@ +// 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. + +#include "cpp/obj-client/auth/gcp/gcs_signed_url.h" + +#include <fmt/format.h> +#include <openssl/sha.h> + +#include <algorithm> +#include <array> +#include <ctime> +#include <optional> +#include <utility> + +#include "cpp/obj-client/auth/gcp/gcp_auth.h" + +namespace doris { +namespace { + +constexpr std::string_view SIGNING_ALGORITHM = "GOOG4-RSA-SHA256"; +constexpr std::string_view SIGNED_HEADERS = "host"; +constexpr std::string_view SIGNING_REGION = "auto"; +constexpr std::string_view SIGNING_SERVICE = "storage"; +constexpr std::string_view SIGNING_TERMINATOR = "goog4_request"; +constexpr int64_t MAX_EXPIRATION_SECONDS = 7 * 24 * 60 * 60; + +struct Endpoint { + std::string scheme; + std::string authority; +}; + +bool is_unreserved(unsigned char c) { + return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '-' || + c == '_' || c == '.' || c == '~'; +} + +std::string percent_encode(std::string_view value, bool preserve_slash) { + constexpr char HEX[] = "0123456789ABCDEF"; + std::string encoded; + encoded.reserve(value.size()); + for (unsigned char c : value) { + if (is_unreserved(c) || (preserve_slash && c == '/')) { + encoded.push_back(static_cast<char>(c)); + continue; + } + encoded.push_back('%'); + encoded.push_back(HEX[c >> 4]); + encoded.push_back(HEX[c & 0x0F]); + } + return encoded; +} + +std::string hex_encode(std::string_view value) { + constexpr char HEX[] = "0123456789abcdef"; + std::string encoded; + encoded.reserve(value.size() * 2); + for (unsigned char c : value) { + encoded.push_back(HEX[c >> 4]); + encoded.push_back(HEX[c & 0x0F]); + } + return encoded; +} + +std::string sha256_hex(std::string_view value) { + std::array<unsigned char, SHA256_DIGEST_LENGTH> digest {}; + SHA256(reinterpret_cast<const unsigned char*>(value.data()), value.size(), digest.data()); + return hex_encode( + std::string_view(reinterpret_cast<const char*>(digest.data()), digest.size())); +} + +std::optional<std::string> parse_endpoint(std::string endpoint, Endpoint* parsed) { + if (endpoint.empty()) { + return "GCS endpoint must not be empty"; + } + auto scheme_end = endpoint.find("://"); + if (scheme_end == std::string::npos) { + parsed->scheme = "https"; + } else { + parsed->scheme = endpoint.substr(0, scheme_end); + endpoint.erase(0, scheme_end + 3); + } + if (parsed->scheme != "http" && parsed->scheme != "https") { + return fmt::format("unsupported GCS endpoint scheme: {}", parsed->scheme); + } + while (endpoint.ends_with('/')) { + endpoint.pop_back(); + } + if (endpoint.empty() || endpoint.find_first_of("/?#") != std::string::npos) { + return fmt::format( + "GCS signed URL endpoint must not contain a path, query, " + "or fragment: {}", + endpoint); + } + parsed->authority = std::move(endpoint); + return std::nullopt; +} + +std::optional<std::string> format_signing_time(std::chrono::system_clock::time_point now, + std::string* timestamp, std::string* date) { + auto time = std::chrono::system_clock::to_time_t(now); + std::tm utc {}; +#if defined(_WIN32) + if (gmtime_s(&utc, &time) != 0) { + return "failed to convert GCS signing time to UTC"; + } +#else + if (gmtime_r(&time, &utc) == nullptr) { + return "failed to convert GCS signing time to UTC"; + } +#endif + char timestamp_buffer[17]; + char date_buffer[9]; + if (std::strftime(timestamp_buffer, sizeof(timestamp_buffer), "%Y%m%dT%H%M%SZ", &utc) == 0 || + std::strftime(date_buffer, sizeof(date_buffer), "%Y%m%d", &utc) == 0) { + return "failed to format GCS signing time"; + } + *timestamp = timestamp_buffer; + *date = date_buffer; + return std::nullopt; +} + +} // namespace + +GcsV4SignedUrlResult build_gcs_v4_signed_url(const GcsV4SignedUrlOptions& options, + std::chrono::system_clock::time_point now, + const GcsSignBlobFunction& sign_blob) { + if (options.bucket.empty()) { + return {.error = "GCS bucket must not be empty"}; + } + if (!is_valid_gcp_service_account_email(options.signer_email)) { + return {.error = "invalid GCS signing service account email"}; + } + if (options.expiration_secs <= 0 || options.expiration_secs > MAX_EXPIRATION_SECONDS) { + return {.error = fmt::format("GCS signed URL expiration must be in [1, {}] seconds", + MAX_EXPIRATION_SECONDS)}; + } + if (!sign_blob) { + return {.error = "GCS signBlob callback must not be empty"}; + } + + Endpoint endpoint; + if (auto error = parse_endpoint(options.endpoint, &endpoint); error.has_value()) { + return {.error = std::move(*error)}; + } + std::string timestamp; + std::string date; + if (auto error = format_signing_time(now, ×tamp, &date); error.has_value()) { + return {.error = std::move(*error)}; + } + + const std::string canonical_uri = Review Comment: [P2] Sign the same key that the error-log upload wrote. For a native GCP Cloud instance with an empty storage prefix (accepted by create_instance), RuntimeState uploads `error_log/<id>`, but S3FileSystem passes `/error_log/<id>` into this new signer. The literal canonical URI becomes `/bucket//error_log/<id>`, which names a different GCS object from the uploaded `error_log/<id>`, so the returned load error-log link fails. The former AWS presigner normalized that leading separator. Normalize the key here or derive it with the same path logic as upload, and cover an empty-prefix error-log link. ########## cloud/src/meta-service/meta_service_resource.cpp: ########## @@ -1325,7 +1419,10 @@ static ObjectStoreInfoPB object_info_pb_factory(ObjectStorageDesc& obj_desc, last_item.set_role_arn(role_arn); last_item.set_external_id(external_id); } - last_item.set_cred_provider_type(get_cred_provider_type(obj)); + if (obj.has_cred_provider_type() || has_non_empty_role_arn(obj)) { + last_item.set_cred_provider_type(get_cred_provider_type(obj)); + } + copy_obj_credential(obj, &last_item); Review Comment: [P2] Include the native GCP credential in the legacy duplicate decision. ADD_OBJ_INFO now accepts a native GCP credential, but two requests for the same bucket/prefix/endpoint with different impersonation accounts both have empty AK/SK. The unchanged duplicate scan rejects the second request before this credential is copied, and ALTER_OBJ_INFO has no native-credential update path, so a legacy instance cannot rotate the identity for that location. Compare the credential envelope too and cover a second add with a different account. ########## common/cpp/obj-client/auth/gcp/gcs_signed_url.cpp: ########## @@ -0,0 +1,194 @@ +// 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. + +#include "cpp/obj-client/auth/gcp/gcs_signed_url.h" + +#include <fmt/format.h> +#include <openssl/sha.h> + +#include <algorithm> +#include <array> +#include <ctime> +#include <optional> +#include <utility> + +#include "cpp/obj-client/auth/gcp/gcp_auth.h" + +namespace doris { +namespace { + +constexpr std::string_view SIGNING_ALGORITHM = "GOOG4-RSA-SHA256"; +constexpr std::string_view SIGNED_HEADERS = "host"; +constexpr std::string_view SIGNING_REGION = "auto"; +constexpr std::string_view SIGNING_SERVICE = "storage"; +constexpr std::string_view SIGNING_TERMINATOR = "goog4_request"; +constexpr int64_t MAX_EXPIRATION_SECONDS = 7 * 24 * 60 * 60; + +struct Endpoint { + std::string scheme; + std::string authority; +}; + +bool is_unreserved(unsigned char c) { + return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '-' || + c == '_' || c == '.' || c == '~'; +} + +std::string percent_encode(std::string_view value, bool preserve_slash) { + constexpr char HEX[] = "0123456789ABCDEF"; + std::string encoded; + encoded.reserve(value.size()); + for (unsigned char c : value) { + if (is_unreserved(c) || (preserve_slash && c == '/')) { + encoded.push_back(static_cast<char>(c)); + continue; + } + encoded.push_back('%'); + encoded.push_back(HEX[c >> 4]); + encoded.push_back(HEX[c & 0x0F]); + } + return encoded; +} + +std::string hex_encode(std::string_view value) { + constexpr char HEX[] = "0123456789abcdef"; + std::string encoded; + encoded.reserve(value.size() * 2); + for (unsigned char c : value) { + encoded.push_back(HEX[c >> 4]); + encoded.push_back(HEX[c & 0x0F]); + } + return encoded; +} + +std::string sha256_hex(std::string_view value) { + std::array<unsigned char, SHA256_DIGEST_LENGTH> digest {}; + SHA256(reinterpret_cast<const unsigned char*>(value.data()), value.size(), digest.data()); + return hex_encode( + std::string_view(reinterpret_cast<const char*>(digest.data()), digest.size())); +} + +std::optional<std::string> parse_endpoint(std::string endpoint, Endpoint* parsed) { + if (endpoint.empty()) { + return "GCS endpoint must not be empty"; + } + auto scheme_end = endpoint.find("://"); + if (scheme_end == std::string::npos) { + parsed->scheme = "https"; + } else { + parsed->scheme = endpoint.substr(0, scheme_end); + endpoint.erase(0, scheme_end + 3); + } + if (parsed->scheme != "http" && parsed->scheme != "https") { + return fmt::format("unsupported GCS endpoint scheme: {}", parsed->scheme); + } + while (endpoint.ends_with('/')) { + endpoint.pop_back(); + } + if (endpoint.empty() || endpoint.find_first_of("/?#") != std::string::npos) { + return fmt::format( + "GCS signed URL endpoint must not contain a path, query, " + "or fragment: {}", + endpoint); + } + parsed->authority = std::move(endpoint); + return std::nullopt; +} + +std::optional<std::string> format_signing_time(std::chrono::system_clock::time_point now, + std::string* timestamp, std::string* date) { + auto time = std::chrono::system_clock::to_time_t(now); + std::tm utc {}; +#if defined(_WIN32) + if (gmtime_s(&utc, &time) != 0) { + return "failed to convert GCS signing time to UTC"; + } +#else + if (gmtime_r(&time, &utc) == nullptr) { + return "failed to convert GCS signing time to UTC"; + } +#endif + char timestamp_buffer[17]; + char date_buffer[9]; + if (std::strftime(timestamp_buffer, sizeof(timestamp_buffer), "%Y%m%dT%H%M%SZ", &utc) == 0 || + std::strftime(date_buffer, sizeof(date_buffer), "%Y%m%d", &utc) == 0) { + return "failed to format GCS signing time"; + } + *timestamp = timestamp_buffer; + *date = date_buffer; + return std::nullopt; +} + +} // namespace + +GcsV4SignedUrlResult build_gcs_v4_signed_url(const GcsV4SignedUrlOptions& options, + std::chrono::system_clock::time_point now, + const GcsSignBlobFunction& sign_blob) { + if (options.bucket.empty()) { + return {.error = "GCS bucket must not be empty"}; + } + if (!is_valid_gcp_service_account_email(options.signer_email)) { + return {.error = "invalid GCS signing service account email"}; + } + if (options.expiration_secs <= 0 || options.expiration_secs > MAX_EXPIRATION_SECONDS) { + return {.error = fmt::format("GCS signed URL expiration must be in [1, {}] seconds", + MAX_EXPIRATION_SECONDS)}; + } + if (!sign_blob) { + return {.error = "GCS signBlob callback must not be empty"}; + } + + Endpoint endpoint; + if (auto error = parse_endpoint(options.endpoint, &endpoint); error.has_value()) { + return {.error = std::move(*error)}; + } + std::string timestamp; + std::string date; + if (auto error = format_signing_time(now, ×tamp, &date); error.has_value()) { + return {.error = std::move(*error)}; + } + + const std::string canonical_uri = + "/" + percent_encode(options.bucket, false) + "/" + percent_encode(options.key, true); + const std::string credential_scope = + fmt::format("{}/{}/{}/{}", date, SIGNING_REGION, SIGNING_SERVICE, SIGNING_TERMINATOR); + const std::string credential = options.signer_email + "/" + credential_scope; + const std::string canonical_query = fmt::format( + "X-Goog-Algorithm={}&X-Goog-Credential={}&X-Goog-Date={}&" + "X-Goog-Expires={}&X-Goog-SignedHeaders={}", + SIGNING_ALGORITHM, percent_encode(credential, false), timestamp, + options.expiration_secs, SIGNED_HEADERS); + const std::string canonical_request = + fmt::format("GET\n{}\n{}\nhost:{}\n\n{}\nUNSIGNED-PAYLOAD", canonical_uri, Review Comment: [P2] Normalize the default HTTPS port when signing the host. `https://storage.googleapis.com:443` passes the native endpoint validator, but this canonical request signs `host:storage.googleapis.com:443` and emits a URL with `:443`. A normal curl fetch of that URL sends `Host: storage.googleapis.com` because 443 is the default HTTPS port, so GCS reconstructs a different canonical request and rejects the otherwise valid error-log link. Normalize the authority used for both the URL and signature, and cover a `:443` endpoint with an actual client retrieval. ########## common/cpp/obj-client/auth/gcp/gcp_auth.cpp: ########## @@ -0,0 +1,108 @@ +// 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. + +#include "cpp/obj-client/auth/gcp/gcp_auth.h" + +#include <algorithm> +#include <cctype> +#include <regex> +#include <utility> + +namespace doris { + +bool is_valid_gcp_storage_endpoint(std::string_view endpoint) { + // Match the FE GcpAuth policy: Google-owned XML API hosts, HTTPS and port 443 only. + static const std::regex pattern( + R"(https://([a-z0-9-]+\.)*(storage\.googleapis\.com|[a-z0-9-]+-storage\.googleapis\.com|storage\.[a-z0-9-]+\.rep\.googleapis\.com)(:443)?/?)", Review Comment: [P2] Handle bucket-prefixed GCS endpoints before passing them to the S3 clients. This regex accepts `https://bucket.storage.googleapis.com` (and the new test expects it); FE `GcpAuth` accepts it too. The S3 endpoint resolvers still insert the request bucket: virtual mode targets `bucket.bucket.storage.googleapis.com`, while path mode requests `/bucket/key` under a host already bound to `bucket`. Both differ from GCS's `bucket.storage.googleapis.com/key` form, so native object I/O fails or reaches the wrong key for an allowed endpoint. Restrict overrides to service-base hosts or normalize the bucket-prefixed form consistently, including signed URLs. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
