[extension/oauth2clientauthextension] Add support for Oauth2 STS mode #40850
Conversation
Co-authored-by: Andrew Wilkins <[email protected]>
There was a problem hiding this comment.
I'm not a code owner, but since you asked, here's my review :)
Probably needs to happen in another change, but IMO the Config struct should have a confighttp.ClientConfig embedded, which would replace the need for the TLS and Timeout config, which are embedded in confighttp.ClientConfig. That would also enable instrumentation of the token requests.
There was a problem hiding this comment.
Typo in filename: twoleggedclienthauth.go
| shouldError bool | ||
| expectedError string |
There was a problem hiding this comment.
| shouldError bool | |
| expectedError string | |
| expectedType any |
| if test.shouldError { | ||
| assert.ErrorContains(t, err, test.expectedError) | ||
| return | ||
| } | ||
|
|
||
| assert.NoError(t, err) | ||
| if test.settings.SubjectToken != "" { | ||
| _, ok := rc.(*stsClientAuthenticator) | ||
| assert.True(t, ok) | ||
| } else { | ||
| _, ok := rc.(*twoLeggedClientAuthenticator) | ||
| assert.True(t, ok) | ||
| } |
There was a problem hiding this comment.
| if test.shouldError { | |
| assert.ErrorContains(t, err, test.expectedError) | |
| return | |
| } | |
| assert.NoError(t, err) | |
| if test.settings.SubjectToken != "" { | |
| _, ok := rc.(*stsClientAuthenticator) | |
| assert.True(t, ok) | |
| } else { | |
| _, ok := rc.(*twoLeggedClientAuthenticator) | |
| assert.True(t, ok) | |
| } | |
| requiore.NoError(t, err) | |
| assert.IsType(t, test.expectedType, rc) |
| @@ -316,3 +231,59 @@ func TestFailContactingOAuth(t *testing.T) { | |||
| assert.ErrorIs(t, err, errFailedToGetSecurityToken) | |||
| assert.ErrorContains(t, err, serverURL.String()) | |||
| } | |||
|
|
|||
| func TestClientAuthenticatorMode(t *testing.T) { | |||
There was a problem hiding this comment.
Should all or most the other tests in this file be moved to twoleggedclientauth_test.go?
| func (o *stsClientAuthenticator) GetRequestMetadata(ctx context.Context, uri ...string) (map[string]string, error) { | ||
| tok, err := o.tokenSource.Token() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return map[string]string{ | ||
| "authorization": fmt.Sprintf("Bearer %s", tok.AccessToken), | ||
| }, nil | ||
| } | ||
|
|
||
| // Applied when authenticator is returned as PerRPCCredentials | ||
| func (o *stsClientAuthenticator) RequireTransportSecurity() bool { | ||
| return !o.config.DisableTLSOnUse | ||
| } |
There was a problem hiding this comment.
| func (o *stsClientAuthenticator) GetRequestMetadata(ctx context.Context, uri ...string) (map[string]string, error) { | |
| tok, err := o.tokenSource.Token() | |
| if err != nil { | |
| return nil, err | |
| } | |
| return map[string]string{ | |
| "authorization": fmt.Sprintf("Bearer %s", tok.AccessToken), | |
| }, nil | |
| } | |
| // Applied when authenticator is returned as PerRPCCredentials | |
| func (o *stsClientAuthenticator) RequireTransportSecurity() bool { | |
| return !o.config.DisableTLSOnUse | |
| } |
AFAICS they're not needed? These methods are for implementing credentials.PerRPCCredentials, but we're using grpcOAuth.TokenSource for that.
| mockKsaTokenFile, err := os.CreateTemp("", "test_ksa_token_path") | ||
| assert.NoError(t, err) | ||
| defer os.Remove(mockKsaTokenFile.Name()) | ||
| _, err = mockKsaTokenFile.WriteString("test-ksa-token") | ||
| assert.NoError(t, err) | ||
| err = mockKsaTokenFile.Close() | ||
| assert.NoError(t, err) |
There was a problem hiding this comment.
| mockKsaTokenFile, err := os.CreateTemp("", "test_ksa_token_path") | |
| assert.NoError(t, err) | |
| defer os.Remove(mockKsaTokenFile.Name()) | |
| _, err = mockKsaTokenFile.WriteString("test-ksa-token") | |
| assert.NoError(t, err) | |
| err = mockKsaTokenFile.Close() | |
| assert.NoError(t, err) | |
| mockKsaTokenFile := filepath.Join(t.TempDir(), "test_ksa_token_path") | |
| err := os.WriteFile(mockKsaTokenFile, "test-ksa-token", 0644) | |
| assert.NoError(t, err) |
If you use T.TempDir(), it will be removed when the test completes.
|
Please address feedback and mark ready to review again |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Description
Adds an new mode of operation for Oauth2 STS exchange. The existing 2-legged flow remains in tact.
A few new configuration options were added (e.g.
SubjectToken), but no breaking changes were made to existing config options.The extension automatically chooses which mode to use based on whether a
ClientSecretis passed in for the 2-legged flow or if aSubjectTokenwas provided for the STS flow.Link to tracking issue
Fixes #38086
Testing
Did an automated integration test against Google's sts server.
Added unit tests for the STS authenticator.
Documentation
Added descriptions for the newly introduced configs for STS.