impl(oauth2): port service account credential prerequisites #8343

scotthart · 2022-02-11T02:29:11Z

This PR brings over several oauth2/ssl utility files from google/cloud/storage. These are all necessary for implementing OAuth2.0 service account credentials.

Other than changes to namespaces and include paths, the contents of these files have been copied as-is from google/cloud/storage. Ultimately these file locations will be their permanent homes after GCS has been refactored to use the new REST library.

This change is

google-cloud-cpp-bot · 2022-02-11T02:30:40Z

Google Cloud Build Logs
For commit: 78733446ec2719ce3bc2c90ab7aa9ee26ea6b607

Public Access Build Logs
Cloud Build Console (requires auth)
Kokoro Logs (requires auth)

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

google-cloud-cpp-bot · 2022-02-11T02:38:06Z

Google Cloud Build Logs
For commit: caf13d665d3c6bd12f076f3600a17b88da3a001f

Public Access Build Logs
Cloud Build Console (requires auth)
Kokoro Logs (requires auth)

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

coryan

Reviewed 13 of 13 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @scotthart)

google/cloud/internal/oauth2_credential_constants.h, line 33 at r1 (raw file):

 */
// NOLINTNEXTLINE(readability-identifier-naming)
enum class JwtSigningAlgorithms { RS256 };

Let's remove this enum. It serves no purpose, if we ever need a new signing algorithm we can create a function or enum at the time.

google/cloud/internal/openssl_util.h, line 65 at r1 (raw file):

template <typename Collection>
inline std::string UrlsafeBase64Encode(Collection const& bytes) {
  std::string b64str = Base64Encode(bytes);

Optional: I think it might be cleaner to just use google::cloud::internal::Base64Encoder directly here, and remove the Base64Encode() functions?

google/cloud/internal/openssl_util.cc, line 75 at r1 (raw file):


  EVP_MD const* digest_type = nullptr;
  switch (alg) {

If we remove the alg argument we can cut all this code, maybe it requires renaming the function SignUsingSha256, the type of the arguments are implicit and don't need to be in the name again.

google/cloud/internal/openssl_util.cc, line 173 at r1 (raw file):

}

std::vector<std::uint8_t> MD5Hash(std::string const& payload) {

I suspect this is very GCS specific, I doubt it will see usage in other services. Please remove.

google/cloud/internal/write_base64.h, line 31 at r1 (raw file):

 * If it fails, it will throw an exception on badbit.
 */
void WriteBase64AsBinary(std::string const& filename, char const* data);

AFAICT this is only used in the tests for .p12 key files. We have no plans to support those with GUAC (they don't work with gRPC see #5116). I think we should remove this code and all the support for PKCS #12.

scotthart

Reviewable status: 4 of 13 files reviewed, 5 unresolved discussions (waiting on @coryan)

google/cloud/internal/openssl_util.h, line 65 at r1 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

Optional: I think it might be cleaner to just use google::cloud::internal::Base64Encoder directly here, and remove the Base64Encode() functions?

Done.

google/cloud/internal/openssl_util.cc, line 75 at r1 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

If we remove the alg argument we can cut all this code, maybe it requires renaming the function SignUsingSha256, the type of the arguments are implicit and don't need to be in the name again.

alg removed; function renamed

google/cloud/internal/openssl_util.cc, line 173 at r1 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

I suspect this is very GCS specific, I doubt it will see usage in other services. Please remove.

removed

google/cloud/internal/oauth2_credential_constants.h, line 33 at r1 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

Let's remove this enum. It serves no purpose, if we ever need a new signing algorithm we can create a function or enum at the time.

removed

google/cloud/internal/write_base64.h, line 31 at r1 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

AFAICT this is only used in the tests for .p12 key files. We have no plans to support those with GUAC (they don't work with gRPC see #5116). I think we should remove this code and all the support for PKCS #12.

Done.

google-cloud-cpp-bot · 2022-02-11T21:59:10Z

Google Cloud Build Logs
For commit: 856bcc9e8191c244f90a224f0b0fc7554929795d

Public Access Build Logs
Cloud Build Console (requires auth)
Kokoro Logs (requires auth)

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

google-cloud-cpp-bot · 2022-02-11T22:02:46Z

Google Cloud Build Logs
For commit: 0a679ca783f2dc27c9f8530ca624fd986cc78c7e

Public Access Build Logs
Cloud Build Console (requires auth)
Kokoro Logs (requires auth)

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

google-cloud-cpp-bot · 2022-02-11T22:19:07Z

Google Cloud Build Logs
For commit: 605ef1b4f9c0c0796284c16019d46828d21c777f

Public Access Build Logs
Cloud Build Console (requires auth)
Kokoro Logs (requires auth)

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

google-cloud-cpp-bot · 2022-02-11T22:27:14Z

Google Cloud Build Logs
For commit: 701a380170aa86a637924167a213492467432206

Public Access Build Logs
Cloud Build Console (requires auth)
Kokoro Logs (requires auth)

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

coryan

Reviewed 11 of 11 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @scotthart)

google/cloud/testing_util/credentials_constants.h, line 1 at r6 (raw file):

// Copyright 2020 Google LLC

Is this file used anywhere? It is only for testing and does not appear to be used in openssl_utils_test.cc?

google/cloud/testing_util/credentials_constants.h, line 41 at r6 (raw file):

//    Delete the service account ID:
//      gcloud iam service-accounts delete --quiet ${SERVICE_ACCOUNT}
char const kP12ServiceAccountId[] = "104849618361176160538";

Are any of these two constants used in the new code? I would expect they are not?

codecov · 2022-02-11T22:52:47Z

Codecov Report

Merging #8343 (174e3ac) into main (e0b818b) will decrease coverage by 0.00%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##             main    #8343      +/-   ##
==========================================
- Coverage   94.99%   94.98%   -0.01%     
==========================================
  Files        1355     1360       +5     
  Lines      120827   120925      +98     
==========================================
+ Hits       114774   114859      +85     
- Misses       6053     6066      +13

Impacted Files	Coverage Δ
google/cloud/internal/openssl_util.cc	`73.07% <73.07%> (ø)`
google/cloud/internal/make_jwt_assertion.cc	`100.00% <100.00%> (ø)`
google/cloud/internal/make_jwt_assertion_test.cc	`100.00% <100.00%> (ø)`
google/cloud/internal/openssl_util.h	`100.00% <100.00%> (ø)`
google/cloud/internal/openssl_util_test.cc	`100.00% <100.00%> (ø)`
google/cloud/examples/grpc_credential_types.cc	`88.71% <0.00%> (-1.03%)`	⬇️
...le/cloud/internal/default_completion_queue_impl.cc	`97.15% <0.00%> (-0.57%)`	⬇️
...cloud/pubsub/internal/subscription_session_test.cc	`97.75% <0.00%> (-0.25%)`	⬇️
google/cloud/completion_queue_test.cc	`96.95% <0.00%> (-0.20%)`	⬇️
.../cloud/storage/benchmarks/throughput_experiment.cc	`74.87% <0.00%> (+0.50%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e0b818b...174e3ac. Read the comment docs.

google-cloud-cpp-bot · 2022-02-11T23:02:57Z

Google Cloud Build Logs
For commit: 752c47cf8dc55c1f716d2e19f37884f99536e62e

Public Access Build Logs
Cloud Build Console (requires auth)
Kokoro Logs (requires auth)

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

scotthart

Reviewable status: 12 of 16 files reviewed, 2 unresolved discussions (waiting on @coryan)

google/cloud/testing_util/credentials_constants.h, line 1 at r6 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

Is this file used anywhere? It is only for testing and does not appear to be used in openssl_utils_test.cc?

it's used in make_jwt_assertion_test and another test file "Not Yet Appearing in this Film", but we don't need a separate file for this constant used in 2 places.

google/cloud/testing_util/credentials_constants.h, line 41 at r6 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

Are any of these two constants used in the new code? I would expect they are not?

No, they are not.

coryan

Reviewed 4 of 4 files at r7.
Reviewable status: 15 of 16 files reviewed, all discussions resolved (waiting on @coryan)

google-cloud-cpp-bot · 2022-02-11T23:06:59Z

Google Cloud Build Logs
For commit: 174e3ac39a9c44f85bd62adb97c0a933d184cc44

Public Access Build Logs
Cloud Build Console (requires auth)
Kokoro Logs (requires auth)

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

coryan

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @scotthart)

scotthart requested a review from a team as a code owner February 11, 2022 02:29

scotthart force-pushed the oauth2_refactor_review6 branch from 7873344 to caf13d6 Compare February 11, 2022 02:30

coryan suggested changes Feb 11, 2022

View reviewed changes

scotthart commented Feb 11, 2022

View reviewed changes

impl(oauth2): port service account credential prerequisites

c1b39a2

scotthart force-pushed the oauth2_refactor_review6 branch from 856bcc9 to 0a679ca Compare February 11, 2022 21:59

address review comments

605ef1b

scotthart force-pushed the oauth2_refactor_review6 branch from 0a679ca to 605ef1b Compare February 11, 2022 22:17

fix bzl file

701a380

coryan suggested changes Feb 11, 2022

View reviewed changes

delete credentials_constants.h file

752c47c

scotthart commented Feb 11, 2022

View reviewed changes

formatting

174e3ac

coryan approved these changes Feb 11, 2022

View reviewed changes

scotthart enabled auto-merge (squash) February 11, 2022 23:34

scotthart merged commit 5f0c674 into googleapis:main Feb 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

impl(oauth2): port service account credential prerequisites #8343

impl(oauth2): port service account credential prerequisites #8343

scotthart commented Feb 11, 2022 •

edited by coryan

Loading

google-cloud-cpp-bot commented Feb 11, 2022

google-cloud-cpp-bot commented Feb 11, 2022

coryan left a comment

scotthart left a comment

google-cloud-cpp-bot commented Feb 11, 2022

google-cloud-cpp-bot commented Feb 11, 2022

google-cloud-cpp-bot commented Feb 11, 2022

google-cloud-cpp-bot commented Feb 11, 2022

coryan left a comment

codecov bot commented Feb 11, 2022 •

edited

Loading

google-cloud-cpp-bot commented Feb 11, 2022

scotthart left a comment

coryan left a comment

google-cloud-cpp-bot commented Feb 11, 2022

coryan left a comment

impl(oauth2): port service account credential prerequisites #8343

impl(oauth2): port service account credential prerequisites #8343

Conversation

scotthart commented Feb 11, 2022 • edited by coryan Loading

google-cloud-cpp-bot commented Feb 11, 2022

google-cloud-cpp-bot commented Feb 11, 2022

coryan left a comment

Choose a reason for hiding this comment

scotthart left a comment

Choose a reason for hiding this comment

google-cloud-cpp-bot commented Feb 11, 2022

google-cloud-cpp-bot commented Feb 11, 2022

google-cloud-cpp-bot commented Feb 11, 2022

google-cloud-cpp-bot commented Feb 11, 2022

coryan left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 11, 2022 • edited Loading

Codecov Report

google-cloud-cpp-bot commented Feb 11, 2022

scotthart left a comment

Choose a reason for hiding this comment

coryan left a comment

Choose a reason for hiding this comment

google-cloud-cpp-bot commented Feb 11, 2022

coryan left a comment

Choose a reason for hiding this comment

scotthart commented Feb 11, 2022 •

edited by coryan

Loading

codecov bot commented Feb 11, 2022 •

edited

Loading