Skip to content
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

feat(storage): Respect custom endpoint for SignedUrl #14179

Merged
merged 16 commits into from
May 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 17 additions & 1 deletion google/cloud/storage/client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include "google/cloud/internal/make_status.h"
#include "google/cloud/internal/opentelemetry.h"
#include "google/cloud/log.h"
#include "absl/strings/str_split.h"
#include <fstream>
#include <memory>
#include <thread>
Expand Down Expand Up @@ -389,7 +390,7 @@ StatusOr<std::string> Client::SignUrlV2(
std::string signature = curl.MakeEscapedString(encoded).get();

std::ostringstream os;
os << "https://storage.googleapis.com/" << request.bucket_name();
os << Endpoint() << '/' << request.bucket_name();
if (!request.object_name().empty()) {
os << '/' << curl.MakeEscapedString(request.object_name()).get();
}
Expand Down Expand Up @@ -481,6 +482,21 @@ std::string CreateRandomPrefixName(std::string const& prefix) {
"abcdefghijklmnopqrstuvwxyz");
}

std::string Client::Endpoint() const {
return connection_->options().get<RestEndpointOption>();
}

// This can be optimized to not have a lot of string copies.
// But the code is rarely used and not in any critical path.
std::string Client::EndpointAuthority() const {
auto endpoint = Endpoint();
auto endpoint_authority = absl::string_view(endpoint);
if (!absl::ConsumePrefix(&endpoint_authority, "https://")) {
absl::ConsumePrefix(&endpoint_authority, "http://");
}
return std::string(endpoint_authority);
}

namespace internal {

Client ClientImplDetails::CreateWithDecorations(
Expand Down
12 changes: 10 additions & 2 deletions google/cloud/storage/client.h
Original file line number Diff line number Diff line change
Expand Up @@ -2996,7 +2996,8 @@ class Client {
google::cloud::internal::OptionsSpan const span(
SpanOptions(std::forward<Options>(options)...));
internal::V4SignUrlRequest request(std::move(verb), std::move(bucket_name),
std::move(object_name));
std::move(object_name),
EndpointAuthority());
request.set_multiple_options(std::forward<Options>(options)...);
return SignUrlV4(std::move(request));
}
Expand Down Expand Up @@ -3082,7 +3083,8 @@ class Client {
PolicyDocumentV4 document, Options&&... options) {
google::cloud::internal::OptionsSpan const span(
SpanOptions(std::forward<Options>(options)...));
internal::PolicyDocumentV4Request request(std::move(document));
internal::PolicyDocumentV4Request request(std::move(document),
EndpointAuthority());
request.set_multiple_options(std::forward<Options>(options)...);
return SignPolicyDocumentV4(std::move(request));
}
Expand Down Expand Up @@ -3492,6 +3494,12 @@ class Client {
StatusOr<PolicyDocumentV4Result> SignPolicyDocumentV4(
internal::PolicyDocumentV4Request request);

// The configured endpoint, including any scheme and port.
std::string Endpoint() const;

bajajneha27 marked this conversation as resolved.
Show resolved Hide resolved
// The hostname:port part of the configured endpoint.
std::string EndpointAuthority() const;

std::shared_ptr<internal::StorageConnection> connection_;
};

Expand Down
20 changes: 17 additions & 3 deletions google/cloud/storage/client_sign_policy_document_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ using ::google::cloud::internal::CurrentOptions;
using ::google::cloud::storage::testing::canonical_errors::TransientError;
using ::testing::HasSubstr;
using ::testing::Return;
using ::testing::StartsWith;

constexpr char kJsonKeyfileContents[] = R"""({
"type": "service_account",
Expand All @@ -58,9 +59,12 @@ std::string Dec64(std::string const& s) {
return std::string(res.begin(), res.end());
};

Client CreateClientForTest() {
return Client(Options{}.set<UnifiedCredentialsOption>(
MakeServiceAccountCredentials(kJsonKeyfileContents)));
Client CreateClientForTest(
std::string endpoint = "https://storage.googleapis.com") {
return Client(Options{}
.set<UnifiedCredentialsOption>(
MakeServiceAccountCredentials(kJsonKeyfileContents))
.set<RestEndpointOption>(std::move(endpoint)));
}

/**
Expand Down Expand Up @@ -255,6 +259,16 @@ TEST(CreateSignedPolicyDocTest, SignV4VirtualHostname) {
EXPECT_EQ("https://test-bucket.storage.googleapis.com/", actual->url);
}

TEST(CreateSignedPolicyDocTest, SignV4CustomEndpoint) {
auto const custom_endpoint = std::string{"https://storage.mydomain.com"};
auto client = CreateClientForTest(custom_endpoint);
auto actual =
client.GenerateSignedPostPolicyV4(CreatePolicyDocumentV4ForTest());

ASSERT_STATUS_OK(actual);
EXPECT_THAT(actual->url, StartsWith(custom_endpoint));
}

} // namespace
GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
} // namespace storage
Expand Down
76 changes: 76 additions & 0 deletions google/cloud/storage/client_sign_url_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "google/cloud/storage/testing/canonical_errors.h"
#include "google/cloud/storage/testing/client_unit_test.h"
#include "google/cloud/testing_util/status_matchers.h"
#include "google/cloud/universe_domain_options.h"
#include <gmock/gmock.h>

namespace google {
Expand All @@ -28,9 +29,11 @@ namespace {

using ::google::cloud::internal::CurrentOptions;
using ::google::cloud::storage::testing::canonical_errors::TransientError;
using ::google::cloud::testing_util::IsOkAndHolds;
using ::google::cloud::testing_util::StatusIs;
using ::testing::HasSubstr;
using ::testing::Return;
using ::testing::StartsWith;

constexpr char kJsonKeyfileContents[] = R"""({
"type": "service_account",
Expand Down Expand Up @@ -107,6 +110,36 @@ TEST_F(CreateSignedUrlTest, V2SignRemote) {
EXPECT_THAT(*actual, HasSubstr(expected_signed_blob_safe));
}

/// @test Verify that CreateV2SignedUrl() respects the custom endpoint.
TEST_F(CreateSignedUrlTest, V2SignCustomEndpoint) {
auto const custom_endpoint = std::string{"https://storage.mydomain.com"};

Options options = Options{}
.set<UnifiedCredentialsOption>(
MakeServiceAccountCredentials(kJsonKeyfileContents))
.set<RestEndpointOption>(custom_endpoint);
Client client(options);
StatusOr<std::string> actual =
client.CreateV2SignedUrl("GET", "test-bucket", "test-object");
EXPECT_THAT(actual, IsOkAndHolds(StartsWith(custom_endpoint)));
}

/// @test Verify that CreateV2SignedUrl() respects the custom universe domain.
TEST_F(CreateSignedUrlTest, V2SignCustomUniverseDomain) {
auto const custom_ud = std::string{"mydomain.com"};

Options options =
Options{}
.set<UnifiedCredentialsOption>(
MakeServiceAccountCredentials(kJsonKeyfileContents))
.set<google::cloud::internal::UniverseDomainOption>(custom_ud);
Client client(options);

StatusOr<std::string> actual =
client.CreateV2SignedUrl("GET", "test-bucket", "test-object");
EXPECT_THAT(actual, IsOkAndHolds(HasSubstr(custom_ud)));
}

// This is a placeholder service account JSON file that is inactive. It's fine
// for it to be public.
constexpr char kJsonKeyfileContentsForV4[] = R"""({
Expand Down Expand Up @@ -231,6 +264,49 @@ TEST_F(CreateSignedUrlTest, V4SignRemote) {
EXPECT_THAT(*actual, HasSubstr(expected_signed_blob_hex));
}

/// @test Verify that CreateV4SignedUrl() respects the custom endpoint.
TEST_F(CreateSignedUrlTest, V4SignCustomEndpoint) {
auto const custom_endpoint = std::string{"https://storage.mydomain.com"};
std::string const bucket_name = "test-bucket";
std::string const object_name = "test-object";
std::string const date = "2019-02-01T09:00:00Z";
auto const valid_for = std::chrono::seconds(10);

Options options =
Options{}
.set<UnifiedCredentialsOption>(
MakeServiceAccountCredentials(kJsonKeyfileContentsForV4))
.set<RestEndpointOption>(custom_endpoint);
Client client(options);

auto actual = client.CreateV4SignedUrl(
"GET", bucket_name, object_name,
SignedUrlTimestamp(google::cloud::internal::ParseRfc3339(date).value()),
SignedUrlDuration(valid_for));
EXPECT_THAT(actual, IsOkAndHolds(StartsWith(custom_endpoint)));
}

/// @test Verify that CreateV4SignUrl() respects the custom universe domain.
TEST_F(CreateSignedUrlTest, V4SignCustomUniverseDomain) {
auto const custom_ud = std::string{"mydomain.com"};
std::string const bucket_name = "test-bucket";
std::string const object_name = "test-object";
std::string const date = "2019-02-01T09:00:00Z";
auto const valid_for = std::chrono::seconds(10);

Options options =
Options{}
.set<UnifiedCredentialsOption>(
MakeServiceAccountCredentials(kJsonKeyfileContentsForV4))
.set<google::cloud::internal::UniverseDomainOption>(custom_ud);
Client client(options);
auto actual = client.CreateV4SignedUrl(
"GET", bucket_name, object_name,
SignedUrlTimestamp(google::cloud::internal::ParseRfc3339(date).value()),
SignedUrlDuration(valid_for));
EXPECT_THAT(actual, IsOkAndHolds(HasSubstr(custom_ud)));
}

TEST_F(CreateSignedUrlTest, V4SignRemoteNoSigningEmail) {
EXPECT_CALL(*mock_, SignBlob).Times(0);
auto client = ClientForMock();
Expand Down
11 changes: 6 additions & 5 deletions google/cloud/storage/internal/policy_document_request.cc
Original file line number Diff line number Diff line change
Expand Up @@ -275,11 +275,11 @@ std::string PolicyDocumentV4Request::Url() const {
return scheme_ + "://" + *bucket_bound_domain_ + "/";
}
if (virtual_host_name_) {
return scheme_ + "://" + policy_document().bucket +
".storage.googleapis.com/";
return scheme_ + "://" + policy_document().bucket + "." +
endpoint_authority_ + "/";
}
return scheme_ + "://storage.googleapis.com/" + policy_document().bucket +
"/";
return scheme_ + "://" + endpoint_authority_ + "/" +
policy_document().bucket + "/";
}

std::string PolicyDocumentV4Request::Credentials() const {
Expand Down Expand Up @@ -342,7 +342,8 @@ std::map<std::string, std::string> PolicyDocumentV4Request::RequiredFormFields()
}

std::ostream& operator<<(std::ostream& os, PolicyDocumentV4Request const& r) {
return os << "PolicyDocumentRequest={" << r.StringToSign() << "}";
return os << "PolicyDocumentRequest={endpoint_authority="
<< r.endpoint_authority() << "," << r.StringToSign() << "}";
}

} // namespace internal
Expand Down
13 changes: 12 additions & 1 deletion google/cloud/storage/internal/policy_document_request.h
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,15 @@ std::ostream& operator<<(std::ostream& os, PolicyDocumentRequest const& r);

class PolicyDocumentV4Request {
public:
PolicyDocumentV4Request() : scheme_("https") {}
PolicyDocumentV4Request()
: PolicyDocumentV4Request("storage.googleapis.com") {}
explicit PolicyDocumentV4Request(std::string endpoint_authority)
: scheme_("https"), endpoint_authority_(std::move(endpoint_authority)) {}
PolicyDocumentV4Request(PolicyDocumentV4 document,
std::string endpoint_authority)
: PolicyDocumentV4Request(std::move(endpoint_authority)) {
document_ = std::move(document);
}
explicit PolicyDocumentV4Request(PolicyDocumentV4 document)
: PolicyDocumentV4Request() {
document_ = std::move(document);
Expand All @@ -125,6 +133,8 @@ class PolicyDocumentV4Request {
return signing_account_delegates_;
}

std::string endpoint_authority() const { return endpoint_authority_; }

void SetOption(SigningAccount const& o) { signing_account_ = o; }

void SetOption(SigningAccountDelegates const& o) {
Expand Down Expand Up @@ -191,6 +201,7 @@ class PolicyDocumentV4Request {
absl::optional<std::string> bucket_bound_domain_;
std::string scheme_;
bool virtual_host_name_{false};
std::string endpoint_authority_;
};

std::ostream& operator<<(std::ostream& os, PolicyDocumentV4Request const& r);
Expand Down
25 changes: 24 additions & 1 deletion google/cloud/storage/internal/policy_document_request_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ namespace {
using ::google::cloud::testing_util::IsOkAndHolds;
using ::google::cloud::testing_util::StatusIs;
using ::testing::ElementsAre;
using ::testing::StartsWith;

TEST(PolicyDocumentRequest, SigningAccount) {
PolicyDocumentRequest request;
Expand Down Expand Up @@ -119,7 +120,8 @@ TEST(PolicyDocumentV4Request, Printing) {
std::stringstream stream;
stream << req;
EXPECT_EQ(
"PolicyDocumentRequest={{\"conditions\":[{\"bucket\":\"test-bucket\"},{"
"PolicyDocumentRequest={endpoint_authority=storage.googleapis.com,"
"{\"conditions\":[{\"bucket\":\"test-bucket\"},{"
"\"key\":\"test-object\"},{\"x-goog-date\":\"20100616T111111Z\"},{\"x-"
"goog-credential\":\"/20100616/auto/storage/"
"goog4_request\"},{\"x-goog-algorithm\":\"GOOG4-RSA-SHA256\"}],"
Expand Down Expand Up @@ -153,6 +155,27 @@ TEST(PolicyDocumentV4Request, RequiredFormFields) {
EXPECT_EQ(expected_fields, req.RequiredFormFields());
}

TEST(PolicyDocumentV4Request, Url) {
PolicyDocumentV4 doc;
doc.bucket = "test-bucket";
auto const custom_endpoint_authority = std::string{"storage.mydomain.com"};
PolicyDocumentV4Request request(doc, custom_endpoint_authority);
EXPECT_THAT(request.Url(),
StartsWith("https://" + custom_endpoint_authority));
}

TEST(PolicyDocumentV4Request, UrlWithVirtualHostName) {
PolicyDocumentV4 doc;
doc.bucket = "test-bucket";
auto const custom_endpoint_authority = std::string{"storage.mydomain.com"};
PolicyDocumentV4Request request(doc, custom_endpoint_authority);
request.SetOption(VirtualHostname(true));

auto const expected_url = std::string{"https://" + doc.bucket + "." +
custom_endpoint_authority + "/"};
EXPECT_EQ(request.Url(), expected_url);
}

} // namespace
} // namespace internal
GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
Expand Down
10 changes: 6 additions & 4 deletions google/cloud/storage/internal/signed_url_requests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,8 @@ std::string V2SignUrlRequest::StringToSign() const {
}

std::ostream& operator<<(std::ostream& os, V2SignUrlRequest const& r) {
return os << "SingUrlRequest={" << r.StringToSign() << "}";
return os << "SingUrlRequest={" << r.endpoint_authority() << ","
<< r.StringToSign() << "}";
}

namespace {
Expand Down Expand Up @@ -235,13 +236,14 @@ Status V4SignUrlRequest::Validate() {
}

std::string V4SignUrlRequest::Hostname() {
auto endpoint_authority = common_request_.endpoint_authority();
if (virtual_host_name_) {
return common_request_.bucket_name() + ".storage.googleapis.com";
return common_request_.bucket_name() + "." + endpoint_authority;
}
if (domain_named_bucket_) {
return *domain_named_bucket_;
}
return "storage.googleapis.com";
return endpoint_authority;
}

std::string V4SignUrlRequest::HostnameWithBucket() {
Expand Down Expand Up @@ -318,7 +320,7 @@ std::string V4SignUrlRequest::PayloadHashValue() const {
}

std::ostream& operator<<(std::ostream& os, V4SignUrlRequest const& r) {
return os << "V4SignUrlRequest={"
return os << "V4SignUrlRequest={" << r.endpoint_authority() << ","
<< r.CanonicalRequest("placeholder-client-id") << ","
<< r.StringToSign("placeholder-client-id") << "}";
}
Expand Down