Skip to content

Commit 033fcd3

Browse files
scotthartscotthart
authored
fix(oauth2): compute engine credential query (googleapis#8421)
* fix(oauth2): compute engine credential query
1 parent da7807e commit 033fcd3

File tree

5 files changed

+51
-63
lines changed

5 files changed

+51
-63
lines changed

‎google/cloud/internal/curl_impl.cc

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -654,6 +654,8 @@ Status CurlImpl::MakeRequest(CurlImpl::HttpMethod method,
654654
using HttpMethod = CurlImpl::HttpMethod;
655655
handle_.SetOption(CURLOPT_CUSTOMREQUEST, HttpMethodAsChar(method));
656656
handle_.SetOption(CURLOPT_UPLOAD, 0L);
657+
handle_.SetOption(CURLOPT_FOLLOWLOCATION,
658+
options_.get<CurlFollowLocationOption>() ? 1L : 0L);
657659

658660
if (method == HttpMethod::kGet) {
659661
handle_.SetOption(CURLOPT_NOPROGRESS, 1L);

‎google/cloud/internal/curl_options.h

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -117,10 +117,18 @@ struct MaximumCurlSocketSendSizeOption {
117117
using Type = std::size_t;
118118
};
119119

120+
/**
121+
* Issue a new request to any Location header specified in a HTTP 3xx response.
122+
*/
123+
struct CurlFollowLocationOption {
124+
using Type = bool;
125+
};
126+
120127
using CurlOptionList = ::google::cloud::OptionList<
121128
ConnectionPoolSizeOption, EnableCurlSslLockingOption,
122129
EnableCurlSigpipeHandlerOption, MaximumCurlSocketRecvSizeOption,
123-
MaximumCurlSocketSendSizeOption, CAPathOption, HttpVersionOption>;
130+
MaximumCurlSocketSendSizeOption, CAPathOption, HttpVersionOption,
131+
CurlFollowLocationOption>;
124132

125133
GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
126134
} // namespace rest_internal

‎google/cloud/internal/oauth2_compute_engine_credentials.cc

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414

1515
#include "google/cloud/internal/oauth2_compute_engine_credentials.h"
1616
#include "google/cloud/internal/compute_engine_util.h"
17+
#include "google/cloud/internal/curl_options.h"
1718
#include "google/cloud/internal/getenv.h"
1819
#include "google/cloud/internal/oauth2_credential_constants.h"
1920
#include "google/cloud/internal/oauth2_credentials.h"
@@ -91,6 +92,7 @@ ComputeEngineCredentials::ComputeEngineCredentials(
9192
service_account_email_(std::move(service_account_email)),
9293
options_(std::move(options)) {
9394
if (!rest_client_) {
95+
options_.set<rest_internal::CurlFollowLocationOption>(true);
9496
rest_client_ = rest_internal::GetDefaultRestClient(
9597
"http://" + google::cloud::internal::GceMetadataHostname(), options_);
9698
}
@@ -127,14 +129,13 @@ ComputeEngineCredentials::DoMetadataServerGetRequest(std::string const& path,
127129
request.SetPath(path);
128130
request.AddHeader("metadata-flavor", "Google");
129131
if (recursive) request.AddQueryParameter("recursive", "true");
130-
return rest_client_->Post(request,
131-
std::vector<std::pair<std::string, std::string>>{});
132+
return rest_client_->Get(request);
132133
}
133134

134135
Status ComputeEngineCredentials::RetrieveServiceAccountInfo() const {
135136
auto response = DoMetadataServerGetRequest(
136-
"/computeMetadata/v1/instance/service-accounts/" +
137-
service_account_email_ + "/",
137+
"computeMetadata/v1/instance/service-accounts/" + service_account_email_ +
138+
"/",
138139
true);
139140
if (!response) {
140141
return std::move(response).status();
@@ -160,8 +161,8 @@ ComputeEngineCredentials::Refresh() const {
160161
}
161162

162163
auto response = DoMetadataServerGetRequest(
163-
"/computeMetadata/v1/instance/service-accounts/" +
164-
service_account_email_ + "/token",
164+
"computeMetadata/v1/instance/service-accounts/" + service_account_email_ +
165+
"/token",
165166
false);
166167
if (!response) {
167168
return std::move(response).status();

‎google/cloud/internal/oauth2_compute_engine_credentials_test.cc

Lines changed: 30 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@ using ::google::cloud::testing_util::MockHttpPayload;
3838
using ::google::cloud::testing_util::MockRestClient;
3939
using ::google::cloud::testing_util::MockRestResponse;
4040
using ::google::cloud::testing_util::StatusIs;
41-
using ::testing::A;
4241
using ::testing::ByMove;
4342
using ::testing::Contains;
4443
using ::testing::Eq;
@@ -102,26 +101,21 @@ TEST_F(ComputeEngineCredentialsTest,
102101
return std::unique_ptr<HttpPayload>(std::move(mock_http_payload));
103102
});
104103

105-
EXPECT_CALL(
106-
*mock_rest_client_,
107-
Post(A<RestRequest>(),
108-
A<std::vector<std::pair<std::string, std::string>> const&>()))
109-
.WillOnce([&](RestRequest const& request,
110-
std::vector<std::pair<std::string, std::string>> const&) {
104+
EXPECT_CALL(*mock_rest_client_, Get)
105+
.WillOnce([&](RestRequest const& request) {
111106
EXPECT_THAT(request.GetHeader("metadata-flavor"), Contains("Google"));
112107
EXPECT_THAT(request.GetQueryParameter("recursive"), Contains("true"));
113108
EXPECT_THAT(
114109
request.path(),
115-
Eq(std::string("/computeMetadata/v1/instance/service-accounts/") +
110+
Eq(std::string("computeMetadata/v1/instance/service-accounts/") +
116111
alias + "/"));
117112
return std::unique_ptr<RestResponse>(std::move(mock_response1));
118113
})
119-
.WillOnce([&](RestRequest const& request,
120-
std::vector<std::pair<std::string, std::string>> const&) {
114+
.WillOnce([&](RestRequest const& request) {
121115
EXPECT_THAT(request.GetHeader("metadata-flavor"), Contains("Google"));
122116
EXPECT_THAT(
123117
request.path(),
124-
Eq(std::string("/computeMetadata/v1/instance/service-accounts/") +
118+
Eq(std::string("computeMetadata/v1/instance/service-accounts/") +
125119
email + "/token"));
126120
return std::unique_ptr<RestResponse>(std::move(mock_response2));
127121
});
@@ -348,37 +342,31 @@ TEST_F(ComputeEngineCredentialsTest, FailedRetrieveServiceAccountInfo) {
348342
return std::unique_ptr<HttpPayload>(std::move(mock_http_payload));
349343
});
350344

351-
EXPECT_CALL(
352-
*mock_rest_client_,
353-
Post(A<RestRequest>(),
354-
A<std::vector<std::pair<std::string, std::string>> const&>()))
355-
.WillOnce([&](RestRequest const& request,
356-
std::vector<std::pair<std::string, std::string>> const&) {
345+
EXPECT_CALL(*mock_rest_client_, Get)
346+
.WillOnce([&](RestRequest const& request) {
357347
EXPECT_THAT(request.GetHeader("metadata-flavor"), Contains("Google"));
358348
EXPECT_THAT(request.GetQueryParameter("recursive"), Contains("true"));
359349
EXPECT_THAT(
360350
request.path(),
361-
Eq(std::string("/computeMetadata/v1/instance/service-accounts/") +
351+
Eq(std::string("computeMetadata/v1/instance/service-accounts/") +
362352
alias + "/"));
363353
return Status{StatusCode::kAborted, "Fake Curl error", {}};
364354
})
365-
.WillOnce([&](RestRequest const& request,
366-
std::vector<std::pair<std::string, std::string>> const&) {
355+
.WillOnce([&](RestRequest const& request) {
367356
EXPECT_THAT(request.GetHeader("metadata-flavor"), Contains("Google"));
368357
EXPECT_THAT(request.GetQueryParameter("recursive"), Contains("true"));
369358
EXPECT_THAT(
370359
request.path(),
371-
Eq(std::string("/computeMetadata/v1/instance/service-accounts/") +
360+
Eq(std::string("computeMetadata/v1/instance/service-accounts/") +
372361
alias + "/"));
373362
return std::unique_ptr<RestResponse>(std::move(mock_response2));
374363
})
375-
.WillOnce([&](RestRequest const& request,
376-
std::vector<std::pair<std::string, std::string>> const&) {
364+
.WillOnce([&](RestRequest const& request) {
377365
EXPECT_THAT(request.GetHeader("metadata-flavor"), Contains("Google"));
378366
EXPECT_THAT(request.GetQueryParameter("recursive"), Contains("true"));
379367
EXPECT_THAT(
380368
request.path(),
381-
Eq(std::string("/computeMetadata/v1/instance/service-accounts/") +
369+
Eq(std::string("computeMetadata/v1/instance/service-accounts/") +
382370
alias + "/"));
383371
return std::unique_ptr<RestResponse>(std::move(mock_response3));
384372
});
@@ -485,88 +473,78 @@ TEST_F(ComputeEngineCredentialsTest, FailedRefresh) {
485473
});
486474

487475
// Fail the first call to RetrieveServiceAccountInfo immediately.
488-
EXPECT_CALL(
489-
*mock_rest_client_,
490-
Post(A<RestRequest>(),
491-
A<std::vector<std::pair<std::string, std::string>> const&>()))
492-
.WillOnce([&](RestRequest const& request,
493-
std::vector<std::pair<std::string, std::string>> const&) {
476+
EXPECT_CALL(*mock_rest_client_, Get)
477+
.WillOnce([&](RestRequest const& request) {
494478
EXPECT_THAT(request.GetHeader("metadata-flavor"), Contains("Google"));
495479
EXPECT_THAT(request.GetQueryParameter("recursive"), Contains("true"));
496480
// For the first expected failures, the alias is used until the metadata
497481
// request succeeds.
498482
EXPECT_THAT(
499483
request.path(),
500-
Eq(std::string("/computeMetadata/v1/instance/service-accounts/") +
484+
Eq(std::string("computeMetadata/v1/instance/service-accounts/") +
501485
alias + "/"));
502486
return Status{StatusCode::kAborted, "Fake Curl error", {}};
503487
})
504488
// Make the call to RetrieveServiceAccountInfo return a good response,
505-
.WillOnce([&](RestRequest const& request,
506-
std::vector<std::pair<std::string, std::string>> const&) {
489+
.WillOnce([&](RestRequest const& request) {
507490
EXPECT_THAT(request.GetHeader("metadata-flavor"), Contains("Google"));
508491
EXPECT_THAT(request.GetQueryParameter("recursive"), Contains("true"));
509492
// For the first expected failures, the alias is used until the metadata
510493
// request succeeds.
511494
EXPECT_THAT(
512495
request.path(),
513-
Eq(std::string("/computeMetadata/v1/instance/service-accounts/") +
496+
Eq(std::string("computeMetadata/v1/instance/service-accounts/") +
514497
alias + "/"));
515498
return std::unique_ptr<RestResponse>(std::move(mock_response2));
516499
})
517500
// but fail the token request immediately.
518-
.WillOnce([&](RestRequest const& request,
519-
std::vector<std::pair<std::string, std::string>> const&) {
501+
.WillOnce([&](RestRequest const& request) {
520502
EXPECT_THAT(request.GetHeader("metadata-flavor"), Contains("Google"));
521503
EXPECT_THAT(
522504
request.path(),
523-
Eq(std::string("/computeMetadata/v1/instance/service-accounts/") +
505+
Eq(std::string("computeMetadata/v1/instance/service-accounts/") +
524506
email + "/token"));
525507
return Status{StatusCode::kAborted, "Fake Curl error", {}};
526508
})
527509
// Make the call to RetrieveServiceAccountInfo return a good response,
528-
.WillOnce([&](RestRequest const& request,
529-
std::vector<std::pair<std::string, std::string>> const&) {
510+
.WillOnce([&](RestRequest const& request) {
530511
EXPECT_THAT(request.GetHeader("metadata-flavor"), Contains("Google"));
531512
EXPECT_THAT(request.GetQueryParameter("recursive"), Contains("true"));
532513
// Now that the first request has succeeded and the metadata has been
533514
// retrieved, the the email is used for refresh.
534515
EXPECT_THAT(
535516
request.path(),
536-
Eq(std::string("/computeMetadata/v1/instance/service-accounts/") +
517+
Eq(std::string("computeMetadata/v1/instance/service-accounts/") +
537518
email + "/"));
538519
return std::unique_ptr<RestResponse>(std::move(mock_response4));
539520
})
540521
// but fail the token request with a bad HTTP error code.
541-
.WillOnce([&](RestRequest const& request,
542-
std::vector<std::pair<std::string, std::string>> const&) {
522+
.WillOnce([&](RestRequest const& request) {
543523
EXPECT_THAT(request.GetHeader("metadata-flavor"), Contains("Google"));
544524
EXPECT_THAT(
545525
request.path(),
546-
Eq(std::string("/computeMetadata/v1/instance/service-accounts/") +
526+
Eq(std::string("computeMetadata/v1/instance/service-accounts/") +
547527
email + "/token"));
548528
return std::unique_ptr<RestResponse>(std::move(mock_response5));
549529
})
550530
// Make the call to RetrieveServiceAccountInfo return a good response,
551-
.WillOnce([&](RestRequest const& request,
552-
std::vector<std::pair<std::string, std::string>> const&) {
531+
.WillOnce([&](RestRequest const& request) {
553532
EXPECT_THAT(request.GetHeader("metadata-flavor"), Contains("Google"));
554533
EXPECT_THAT(request.GetQueryParameter("recursive"), Contains("true"));
555534
// Now that the first request has succeeded and the metadata has been
556535
// retrieved, the the email is used for refresh.
557536
EXPECT_THAT(
558537
request.path(),
559-
Eq(std::string("/computeMetadata/v1/instance/service-accounts/") +
538+
Eq(std::string("computeMetadata/v1/instance/service-accounts/") +
560539
email + "/"));
561540
return std::unique_ptr<RestResponse>(std::move(mock_response6));
562541
})
563542
// but, parse with an invalid token response.
564-
.WillOnce([&](RestRequest const& request,
565-
std::vector<std::pair<std::string, std::string>> const&) {
543+
.WillOnce([&](RestRequest const& request) {
566544
EXPECT_THAT(request.GetHeader("metadata-flavor"), Contains("Google"));
567545
EXPECT_THAT(
568546
request.path(),
569-
Eq(std::string("/computeMetadata/v1/instance/service-accounts/") +
547+
Eq(std::string("computeMetadata/v1/instance/service-accounts/") +
570548
email + "/token"));
571549
return std::unique_ptr<RestResponse>(std::move(mock_response7));
572550
});
@@ -614,17 +592,13 @@ TEST_F(ComputeEngineCredentialsTest, AccountEmail) {
614592
return std::unique_ptr<HttpPayload>(std::move(mock_http_payload));
615593
});
616594

617-
EXPECT_CALL(
618-
*mock_rest_client_,
619-
Post(A<RestRequest>(),
620-
A<std::vector<std::pair<std::string, std::string>> const&>()))
621-
.WillOnce([&](RestRequest const& request,
622-
std::vector<std::pair<std::string, std::string>> const&) {
595+
EXPECT_CALL(*mock_rest_client_, Get)
596+
.WillOnce([&](RestRequest const& request) {
623597
EXPECT_THAT(request.GetHeader("metadata-flavor"), Contains("Google"));
624598
EXPECT_THAT(request.GetQueryParameter("recursive"), Contains("true"));
625599
EXPECT_THAT(
626600
request.path(),
627-
Eq(std::string("/computeMetadata/v1/instance/service-accounts/") +
601+
Eq(std::string("computeMetadata/v1/instance/service-accounts/") +
628602
alias + "/"));
629603
return std::unique_ptr<RestResponse>(std::move(mock_response1));
630604
});

‎google/cloud/internal/oauth2_google_credentials_test.cc

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
// limitations under the License.
1414

1515
#include "google/cloud/internal/oauth2_google_credentials.h"
16+
#include "google/cloud/internal/compute_engine_util.h"
1617
#include "google/cloud/internal/filesystem.h"
1718
#include "google/cloud/internal/oauth2_anonymous_credentials.h"
1819
#include "google/cloud/internal/oauth2_authorized_user_credentials.h"
@@ -281,6 +282,8 @@ TEST_F(GoogleCredentialsTest, MissingCredentialsViaGcloudFilePath) {
281282
// eventually finding no valid credentials and hitting a runtime error.
282283
ScopedEnvironment gcloud_path_override_env_var(GoogleGcloudAdcFileEnvVar(),
283284
filename);
285+
ScopedEnvironment gcloud_metadata_host_override_env_var(
286+
internal::GceMetadataHostnameEnvVar(), "nowhere.com");
284287

285288
auto creds = GoogleDefaultCredentials();
286289
EXPECT_THAT(creds, StatusIs(Not(StatusCode::kOk),

0 commit comments

Comments
 (0)