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.

Sign up for GitHub

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

Jump to bottom

fix(storage): scopes should disable self-signed JWTs #10369

Merged
merged 2 commits into from
Dec 5, 2022
Merged

fix(storage): scopes should disable self-signed JWTs #10369

merged 2 commits into from
Dec 5, 2022

Conversation

coryan
Copy link
Contributor

@coryan coryan commented Dec 5, 2022 •
edited
Loading

The storage service does not support self-signed JWTs with scopes. With this change, self-signed JWTs will be automatically disabled when using the legacy storage::oauth2::Credentials.

This change is Reviewable

@product-auto-label product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label Dec 5, 2022
@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: 4769779e17729ed1bf351bc437d95a2a68b8dd83

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

@codecov
Copy link

codecov bot commented Dec 5, 2022 •
edited
Loading

Codecov Report

Base: 93.86% // Head: 93.86% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (57d0432) compared to base (293a4c0).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #10369      +/-   ##
==========================================
- Coverage   93.86%   93.86%   -0.01%     
==========================================
  Files        1600     1600              
  Lines      145421   145412       -9     
==========================================
- Hits       136504   136492      -12     
- Misses       8917     8920       +3
Impacted Files Coverage Δ
...loud/internal/oauth2_service_account_credentials.h 100.00% <ø> (ø)
...oud/internal/oauth2_service_account_credentials.cc 93.03% <100.00%> (+0.07%) ⬆️
...cloud/storage/internal/unified_rest_credentials.cc 94.87% <100.00%> (-0.97%) ⬇️
...loud/storage/oauth2/service_account_credentials.cc 100.00% <100.00%> (ø)
...cloud/storage/oauth2/service_account_credentials.h 85.45% <100.00%> (-1.44%) ⬇️
...storage/oauth2/service_account_credentials_test.cc 97.72% <100.00%> (+0.01%) ⬆️
...cloud/pubsub/internal/subscription_session_test.cc 97.99% <0.00%> (-0.84%) ⬇️
...le/cloud/storage/internal/curl_download_request.cc 89.29% <0.00%> (-0.67%) ⬇️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@coryan coryan force-pushed the fix-storage-any-scopes-disable-self-signed-jwt branch from 4769779 to c8afd89 Compare December 5, 2022 17:47
@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: c8afd89ca651de715cd4843308022bfc3be9e600

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

@coryan coryan marked this pull request as ready for review December 5, 2022 18:22
@coryan coryan requested a review from a team as a code owner December 5, 2022 18:22
scotthart
scotthart approved these changes Dec 5, 2022
View reviewed changes
Copy link
Member

@scotthart scotthart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @coryan)

google/cloud/storage/oauth2/service_account_credentials.cc line 137 at r1 (raw file):

  // Storage has more stringent requirements w.r.t. self-signed JWTs
  // than most services (which the base class
  // Disable them in the implementation class

Either some formatting or wording is off in this comment block.

Code quote:

  // Storage has more stringent requirements w.r.t. self-signed JWTs
  // than most services (which the base class
  // Disable them in the implementation class

coryan added 2 commits December 5, 2022 21:55
@coryan
fix(storage): scopes should disable self-signed JWTs
349476d 
The storage service does not support self-signed JWTs with scopes. With
this change, self-signed JWTs will be automatically disabled when using
the legacy `storage::oauth2::Credentials`.
@coryan
Address review comments
57d0432
@coryan coryan force-pushed the fix-storage-any-scopes-disable-self-signed-jwt branch from c8afd89 to 57d0432 Compare December 5, 2022 22:13
coryan
coryan commented Dec 5, 2022
View reviewed changes
Copy link
Contributor Author

@coryan coryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 4 of 6 files reviewed, all discussions resolved (waiting on @scotthart)

google/cloud/storage/oauth2/service_account_credentials.cc line 137 at r1 (raw file):

Previously, scotthart (Scott Hart) wrote…

Either some formatting or wording is off in this comment block.

Fixed I think, PTAL.

@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: 57d04323be768279278a6c60e1732f78ad060a75

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

@coryan coryan enabled auto-merge (squash) December 5, 2022 22:31
@coryan coryan merged commit d0baafb into googleapis:main Dec 5, 2022
@coryan coryan deleted the fix-storage-any-scopes-disable-self-signed-jwt branch December 6, 2022 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scotthart scotthart scotthart approved these changes

Assignees
No one assigned
Labels
api: storage Issues related to the Cloud Storage API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@coryan @google-cloud-cpp-bot @scotthart