-
Notifications
You must be signed in to change notification settings - Fork 4.5k
credentials: allow audience to be configured (#8421) #8442
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.
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
credentials: allow audience to be configured (#8421) #8442
Conversation
|
7361fa2
to
4d729c6
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8442 +/- ##
==========================================
+ Coverage 82.27% 82.31% +0.04%
==========================================
Files 414 414
Lines 40422 40425 +3
==========================================
+ Hits 33257 33277 +20
+ Misses 5799 5780 -19
- Partials 1366 1368 +2
🚀 New features to boost your workflow:
|
4d729c6
to
bb16425
Compare
test/creds_test.go
Outdated
func (a *audienceTestCreds) RequireTransportSecurity() bool { return false } | ||
|
||
func (s) TestGRPCMethodInAudienceWhenEnvironmentSet(t *testing.T) { | ||
oldAudienceIsFullPath := envconfig.AudienceIsFullPath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be changed to use the testutil function to set and unset the environment config.
testutils.SetEnvConfig(t, &envconfig.AudienceIsFullPath, true)
This will set to true and reset to original value in the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice :)
Please add release notes. |
bb16425
to
60f17dd
Compare
@eshitachandwani I'm sorry, I'm not sure what you mean by release notes, I don't see any in the repository. I have updated the documentation around the environment variable however. Oh, I think I see... it should be set in the PR description. I'm confused, I edited the PR description and the validation is still failing. Are you able to help me figure out where I've gone wrong please? Final edit: figured it out, I had a space before the *, reading the RegExp helped. |
764e4c5
to
4257d73
Compare
LGTM , adding @gtcooke94 to review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Just a few small things
4257d73
to
e75172f
Compare
There are competing specifications around whether a method should be included in a JWT audience or not. For example grpc#4713 specifically excluded the method referencing https://google.aip.dev/auth/4111 whereas GCE IAP requires the full URI https://cloud.google.com/iap/docs/authentication-howto. In order to facilitate both methods, we introduce a new environment variable, namely GRPC_AUDIENCE_IS_FULL_PATH, to allow the method stripping to be disabled. This defaults to the existing behaviour of stripping the method, but can be set to avoid this.
e75172f
to
97573c5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - by the way, the commits will automatically be squashed into 1 when the PR is merged, so you don't need to worry about squashing and force-pushing a single commit each update
Thanks for the PR!
Thanks, a lot of those were to force a re-build because the arm64 tests seem super flaky. |
Reverting this PR to allow for broader cross-language discussion prior to making a decision on incorporating this feature. |
That's rather frustrating... this has a severe effect on our ability to use Google Cloud due to its implementation of IAP. |
Fixes: #8421
There are competing specifications around whether a method should be included in a JWT audience or not. For example #4713 specifically excluded the method referencing https://google.aip.dev/auth/4111 whereas GCE IAP requires the full URI https://cloud.google.com/iap/docs/authentication-howto.
In order to facilitate both methods, we introduce a new environment variable, namely GRPC_AUDIENCE_IS_FULL_PATH, to allow the method stripping to be disabled. This defaults to the existing behaviour of stripping the method, but can be set to avoid this.
RELEASE NOTES:
GRPC_AUDIENCE_IS_FULL_PATH
is set to true.