From 3578ed49df83d5df427eadcea53afc022bb06067 Mon Sep 17 00:00:00 2001 From: Carlos O'Ryan Date: Sat, 27 May 2023 19:05:42 -0400 Subject: [PATCH] feat(storage): better error messages in signed URLs (#11741) The error messages for signed URL functions (`CreateV2SignedUrl()`, `CreateV4SignedUrl()` and `CreatedSignedPolicyDocument()`) were not helpful when the signing account is missing. --- google/cloud/storage/client.cc | 15 ++++++++-- .../client_sign_policy_document_test.cc | 11 ++++++-- google/cloud/storage/client_sign_url_test.cc | 28 ++++++++++++++++--- 3 files changed, 46 insertions(+), 8 deletions(-) diff --git a/google/cloud/storage/client.cc b/google/cloud/storage/client.cc index bbebb25ddc5f5..e25fd705a5112 100644 --- a/google/cloud/storage/client.cc +++ b/google/cloud/storage/client.cc @@ -22,6 +22,7 @@ #include "google/cloud/internal/algorithm.h" #include "google/cloud/internal/curl_options.h" #include "google/cloud/internal/filesystem.h" +#include "google/cloud/internal/make_status.h" #include "google/cloud/internal/opentelemetry.h" #include "google/cloud/log.h" #include @@ -357,7 +358,6 @@ StatusOr Client::SignBlobImpl( SigningAccount const& signing_account, std::string const& string_to_sign) { auto credentials = raw_client_->client_options().credentials(); - std::string signing_account_email = SigningEmail(signing_account); // First try to sign locally. auto signed_blob = credentials->SignBlob(signing_account, string_to_sign); if (signed_blob) { @@ -367,8 +367,19 @@ StatusOr Client::SignBlobImpl( // If signing locally fails that may be because the credentials do not // support signing, or because the signing account is different than the // credentials account. In either case, try to sign using the API. + // In this case, however, we want to validate the signing account, because + // otherwise the errors are almost impossible to troubleshoot. + auto signing_email = SigningEmail(signing_account); + if (signing_email.empty()) { + return google::cloud::internal::InvalidArgumentError( + "signing account cannot be empty." + " The client library was unable to fetch a valid signing email from" + " the configured credentials, and the application did not provide" + " a value in the `google::cloud::storage::SigningAccount` option.", + GCP_ERROR_INFO()); + } internal::SignBlobRequest sign_request( - signing_account_email, internal::Base64Encode(string_to_sign), {}); + std::move(signing_email), internal::Base64Encode(string_to_sign), {}); auto response = raw_client_->SignBlob(sign_request); if (!response) return response.status(); auto decoded = internal::Base64Decode(response->signed_blob); diff --git a/google/cloud/storage/client_sign_policy_document_test.cc b/google/cloud/storage/client_sign_policy_document_test.cc index ebc28465477e2..b089fc5ba2e95 100644 --- a/google/cloud/storage/client_sign_policy_document_test.cc +++ b/google/cloud/storage/client_sign_policy_document_test.cc @@ -137,6 +137,7 @@ TEST_F(CreateSignedPolicyDocRPCTest, SignRemote) { auto client = ClientForMock(); auto actual = client.CreateSignedPolicyDocument( CreatePolicyDocumentForTest(), + SigningAccount("test-only-invalid@example.com"), Options{}.set("u-p-test")); ASSERT_STATUS_OK(actual); EXPECT_THAT(actual->signature, expected_signed_blob); @@ -148,7 +149,10 @@ TEST_F(CreateSignedPolicyDocRPCTest, SignPolicyTooManyFailures) { testing::TooManyFailuresStatusTest( mock_, EXPECT_CALL(*mock_, SignBlob), [](Client& client) { - return client.CreateSignedPolicyDocument(CreatePolicyDocumentForTest()) + return client + .CreateSignedPolicyDocument( + CreatePolicyDocumentForTest(), + SigningAccount("test-only-invalid@example.com")) .status(); }, "SignBlob"); @@ -161,7 +165,10 @@ TEST_F(CreateSignedPolicyDocRPCTest, SignPolicyPermanentFailure) { testing::PermanentFailureStatusTest( client, EXPECT_CALL(*mock_, SignBlob), [](Client& client) { - return client.CreateSignedPolicyDocument(CreatePolicyDocumentForTest()) + return client + .CreateSignedPolicyDocument( + CreatePolicyDocumentForTest(), + SigningAccount("test-only-invalid@example.com")) .status(); }, "SignBlob"); diff --git a/google/cloud/storage/client_sign_url_test.cc b/google/cloud/storage/client_sign_url_test.cc index 49d95d93a95e7..e64be6acd902c 100644 --- a/google/cloud/storage/client_sign_url_test.cc +++ b/google/cloud/storage/client_sign_url_test.cc @@ -29,6 +29,7 @@ namespace { using ::google::cloud::internal::CurrentOptions; using ::google::cloud::storage::testing::canonical_errors::TransientError; +using ::google::cloud::testing_util::StatusIs; using ::testing::HasSubstr; using ::testing::Return; @@ -101,6 +102,7 @@ TEST_F(CreateSignedUrlTest, V2SignRemote) { auto client = ClientForMock(); StatusOr actual = client.CreateV2SignedUrl("GET", "test-bucket", "test-object", + SigningAccount("test-only-invalid@example.com"), Options{}.set("u-p-test")); ASSERT_STATUS_OK(actual); EXPECT_THAT(*actual, HasSubstr(expected_signed_blob_safe)); @@ -111,7 +113,9 @@ TEST_F(CreateSignedUrlTest, V2SignTooManyFailures) { testing::TooManyFailuresStatusTest( mock_, EXPECT_CALL(*mock_, SignBlob), [](Client& client) { - return client.CreateV2SignedUrl("GET", "test-bucket", "test-object") + return client + .CreateV2SignedUrl("GET", "test-bucket", "test-object", + SigningAccount("test-only-invalid@example.com")) .status(); }, "SignBlob"); @@ -123,7 +127,9 @@ TEST_F(CreateSignedUrlTest, V2SignPermanentFailure) { testing::PermanentFailureStatusTest( client, EXPECT_CALL(*mock_, SignBlob), [](Client& client) { - return client.CreateV2SignedUrl("GET", "test-bucket", "test-object") + return client + .CreateV2SignedUrl("GET", "test-bucket", "test-object", + SigningAccount("test-only-invalid@example.com")) .status(); }, "SignBlob"); @@ -247,17 +253,29 @@ TEST_F(CreateSignedUrlTest, V4SignRemote) { auto client = ClientForMock(); StatusOr actual = client.CreateV4SignedUrl("GET", "test-bucket", "test-object", + SigningAccount("test-only-invalid@example.com"), Options{}.set("u-p-test")); ASSERT_STATUS_OK(actual); EXPECT_THAT(*actual, HasSubstr(expected_signed_blob_hex)); } +TEST_F(CreateSignedUrlTest, V4SignRemoteNoSigningEmail) { + EXPECT_CALL(*mock_, SignBlob).Times(0); + auto client = ClientForMock(); + auto const actual = client.CreateV4SignedUrl( + "GET", "test-bucket", "test-object", SigningAccount("")); + EXPECT_THAT(actual, StatusIs(StatusCode::kInvalidArgument, + HasSubstr("signing account cannot be empty"))); +} + /// @test Verify that CreateV4SignedUrl() + SignBlob() respects retry policies. TEST_F(CreateSignedUrlTest, V4SignTooManyFailures) { testing::TooManyFailuresStatusTest( mock_, EXPECT_CALL(*mock_, SignBlob), [](Client& client) { - return client.CreateV4SignedUrl("GET", "test-bucket", "test-object") + return client + .CreateV4SignedUrl("GET", "test-bucket", "test-object", + SigningAccount("test-only-invalid@example.com")) .status(); }, "SignBlob"); @@ -269,7 +287,9 @@ TEST_F(CreateSignedUrlTest, V4SignPermanentFailure) { testing::PermanentFailureStatusTest( client, EXPECT_CALL(*mock_, SignBlob), [](Client& client) { - return client.CreateV4SignedUrl("GET", "test-bucket", "test-object") + return client + .CreateV4SignedUrl("GET", "test-bucket", "test-object", + SigningAccount("test-only-invalid@example.com")) .status(); }, "SignBlob");