Gdch oauth2 prototype#16117
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for Google Distributed Cloud Hosting (GDCH) Service Account credentials, including REST client implementations, JWT assertion generation using raw ECDSA signatures via OpenSSL, and corresponding unit and integration tests. The review feedback identifies several important issues and areas for improvement: a declared overload of MakeGDCHServiceAccountCredentials is missing its implementation; hardcoding internal::InternalError in the service account credentials discards the HTTP status code; JSON parsing needs validation to ensure the input is an object to prevent crashes; platform-dependent std::time_t should be replaced with modern C++ duration casts; lifetime safety should be improved when passing temporary strings to client->Post; and commented-out pseudo-code in unified_grpc_credentials.cc should be removed.
| std::shared_ptr<Credentials> MakeGDCHServiceAccountCredentials( | ||
| std::string json_object, std::string audience, Options opts) { | ||
| return std::make_shared<internal::GDCHServiceAccountConfig>( | ||
| std::move(json_object), std::move(audience), std::move(opts)); | ||
| } |
There was a problem hiding this comment.
The second overload of MakeGDCHServiceAccountCredentials (which takes only audience and opts) is declared in google/cloud/credentials.h but is missing its implementation in google/cloud/credentials.cc. This will lead to linker errors when users attempt to call it.
std::shared_ptr<Credentials> MakeGDCHServiceAccountCredentials(
std::string json_object, std::string audience, Options opts) {
return std::make_shared<internal::GDCHServiceAccountConfig>(
std::move(json_object), std::move(audience), std::move(opts));
}
std::shared_ptr<Credentials> MakeGDCHServiceAccountCredentials(
std::string audience, Options opts) {
return std::make_shared<internal::GDCHServiceAccountConfig>(
std::move(audience), std::move(opts));
}| // TODO(sdhart): verify this is the error we want to return. | ||
| return internal::InternalError(error_payload, GCP_ERROR_INFO()); | ||
| // return AsStatus(status_code, error_payload); |
There was a problem hiding this comment.
Hardcoding internal::InternalError discards the actual HTTP status code returned by the server (e.g., mapping a 400 Bad Request to kInternal instead of kInvalidArgument). Restoring AsStatus(status_code, error_payload) preserves the correct status code mapping. Additionally, this cleans up the commented-out code and the temporary TODO.
return AsStatus(status_code, error_payload);| auto credentials = nlohmann::json::parse(content, nullptr, false); | ||
| if (credentials.is_discarded()) { | ||
| return internal::InvalidArgumentError(absl::StrCat( | ||
| "Invalid GDCHServiceAccountCredentials, parsing failed on ", | ||
| "data loaded from ", source)); | ||
| } |
There was a problem hiding this comment.
If the input content is valid JSON but not a JSON object (e.g., a JSON array, string, or number), credentials.is_discarded() will be false, but subsequent calls like credentials.find() will throw a nlohmann::json::type_error exception. Checking !credentials.is_object() prevents these potential crashes.
auto credentials = nlohmann::json::parse(content, nullptr, false);
if (credentials.is_discarded() || !credentials.is_object()) {
return internal::InvalidArgumentError(absl::StrCat(
"Invalid GDCHServiceAccountCredentials, parsing failed on ",
"data loaded from ", source));
}| auto const now_from_epoch = | ||
| static_cast<std::intmax_t>(std::chrono::system_clock::to_time_t(now)); | ||
| auto const expiration_from_epoch = static_cast<std::intmax_t>( | ||
| std::chrono::system_clock::to_time_t(expiration)); |
There was a problem hiding this comment.
Using std::chrono::system_clock::to_time_t relies on std::time_t, which can be platform-dependent (e.g., 32-bit on some systems, leading to the Y2038 problem, or even floating-point). Using time_since_epoch() with std::chrono::duration_cast is the modern, standard, and safer C++ way to get the Unix timestamp.
auto const now_from_epoch = std::chrono::duration_cast<std::chrono::seconds>(
now.time_since_epoch()).count();
auto const expiration_from_epoch =
std::chrono::duration_cast<std::chrono::seconds>(
expiration.time_since_epoch()).count();| auto payload = CreateRefreshPayload(info_, tp); | ||
| if (!payload) return std::move(payload).status(); | ||
| rest_internal::RestContext context; | ||
| auto response = client->Post(context, request, {payload->dump()}); |
There was a problem hiding this comment.
Passing {payload->dump()} directly to client->Post creates a temporary std::string whose lifetime is bound to the full expression. While technically valid during the call, initializing an absl::Span<char const> from a temporary can trigger compiler warnings or static analysis lifetime alerts. Storing the dumped string in a local variable first is safer and cleaner.
std::string payload_str = payload->dump();
auto response = client->Post(context, request, {payload_str});| // if file path is specified, read json from it, handle errors | ||
| // else use json from cfg | ||
| // create |
No description provided.