Skip to content

Commit fab1ebd

Browse files
Fix double-caching of OAuth tokens in Azure client secret credentials (#1573)
## Summary Fixes the double-caching of OAuth tokens in Azure client secret credentials (#1549) and unifies the internal token source infrastructure on `auth.TokenSource`. ## Why `AzureClientSecretCredentials.tokenSourceFor` called `clientcredentials.Config{}.TokenSource(ctx)`, which returns an `oauth2.ReuseTokenSource` with a hardcoded 10-second `expiryDelta`. This token source was then wrapped in `azureReuseTokenSource` (a `cachedTokenSource`) and again in `serviceToServiceVisitor` (another `cachedTokenSource`). The inner `ReuseTokenSource` swallowed proactive refresh attempts from the outer layers -- it kept returning its cached token until only ~10 seconds remained before expiry, defeating the 20-minute async refresh window and causing bursts of 401s at token expiry. This is the same class of bug as #1550 (M2M OAuth), but in the Azure client secret path. ## What changed ### Interface changes None. All changes are to unexported types and functions. ### Behavioral changes - `AzureClientSecretCredentials` no longer double-caches tokens. Each call to the underlying token source results in an HTTP request to the token endpoint; caching is purely controlled by the outer `azureReuseTokenSource` and `serviceToServiceVisitor` layers. ### Internal changes - **`azureHostResolver` interface** now returns `auth.TokenSource` instead of `oauth2.TokenSource`, and no longer takes a `context.Context` parameter. Context is passed per-call via `Token(ctx)`. - **`serviceToServiceVisitor` and `refreshableVisitor`** now accept `auth.TokenSource` directly, removing the `authconv.AuthTokenSource` wrapping at call sites. - **`azureReuseTokenSource` and `wrap`** operate on `auth.TokenSource` instead of `oauth2.TokenSource`, eliminating the `authconv` round-trip. - **`AzureClientSecretCredentials`**: removed the duplicate `tokenSourceFor` (which used `clientcredentials.Config.TokenSource` with inner caching). The former `authTokenSourceFor` -- which calls `clientcredentials.Config.Token(ctx)` without inner caching -- is now the sole `tokenSourceFor`. - **`AzureCliCredentials.tokenSourceFor`**: returns an `auth.TokenSource` that creates a fresh `azureCliTokenSource` per call, properly threading the caller's context. - **`AzureMsiCredentials.tokenSourceFor`**: wraps `NewAzureMsiTokenSource` with `authconv.AuthTokenSource`. - **GCP and M2M credential providers**: callers wrap their `oauth2.TokenSource` with `authconv.AuthTokenSource` before passing to the visitor functions. ## How is this tested? - `TestAzureClientSecretCredentials_tokenSourceFor_noCaching`: calls `Token()` N times and asserts N HTTP requests hit the token endpoint. Fails when the fix is reverted (inner caching collapses N calls into 1). - `TestAzureClientSecretCredentials_Configure`: happy-path test for the full Azure client secret credential flow. - All existing `config/` tests pass. Signed-off-by: Ubuntu <renaud.hartert@databricks.com>
1 parent 603757b commit fab1ebd

File tree

3 files changed

+125
-10
lines changed

3 files changed

+125
-10
lines changed

NEXT_CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313

1414
### Bug Fixes
1515

16+
* Fix double-caching of OAuth tokens in Azure client secret credentials ([#1549](https://github.com/databricks/databricks-sdk-go/issues/1549)).
1617
* Disable async token refresh for GCP credential providers to avoid wasted refresh attempts caused by double-caching with Google's internal `oauth2.ReuseTokenSource` ([#1549](https://github.com/databricks/databricks-sdk-go/issues/1549)).
1718

1819
### Documentation

config/auth_azure_client_secret.go

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,11 @@ import (
55
"fmt"
66
"net/url"
77

8+
"golang.org/x/oauth2"
89
"golang.org/x/oauth2/clientcredentials"
910

1011
"github.com/databricks/databricks-sdk-go/config/credentials"
1112
"github.com/databricks/databricks-sdk-go/config/experimental/auth"
12-
"github.com/databricks/databricks-sdk-go/config/experimental/auth/authconv"
1313
"github.com/databricks/databricks-sdk-go/logger"
1414
)
1515

@@ -20,16 +20,24 @@ func (c AzureClientSecretCredentials) Name() string {
2020
return "azure-client-secret"
2121
}
2222

23+
// tokenSourceFor returns an auth.TokenSource that creates a fresh
24+
// clientcredentials.Config on each call and uses the provided context for the
25+
// token exchange. This does not include the inner caching layer from
26+
// clientcredentials.Config.TokenSource so that caching is purely controlled
27+
// upstream.
2328
func (c AzureClientSecretCredentials) tokenSourceFor(
24-
ctx context.Context, cfg *Config, aadEndpoint, resource string) auth.TokenSource {
25-
return authconv.AuthTokenSource((&clientcredentials.Config{
26-
ClientID: cfg.AzureClientID,
27-
ClientSecret: cfg.AzureClientSecret,
28-
TokenURL: fmt.Sprintf("%s%s/oauth2/token", aadEndpoint, cfg.AzureTenantID),
29-
EndpointParams: url.Values{
30-
"resource": []string{resource},
31-
},
32-
}).TokenSource(ctx))
29+
_ context.Context, cfg *Config, aadEndpoint, resource string) auth.TokenSource {
30+
return auth.TokenSourceFn(func(ctx context.Context) (*oauth2.Token, error) {
31+
ctx = cfg.refreshClient.InContextForOAuth2(ctx)
32+
return (&clientcredentials.Config{
33+
ClientID: cfg.AzureClientID,
34+
ClientSecret: cfg.AzureClientSecret,
35+
TokenURL: fmt.Sprintf("%s%s/oauth2/token", aadEndpoint, cfg.AzureTenantID),
36+
EndpointParams: url.Values{
37+
"resource": []string{resource},
38+
},
39+
}).Token(ctx)
40+
})
3341
}
3442

3543
// TODO: We need to expose which authentication mechanism is used to Terraform,
Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
package config
2+
3+
import (
4+
"context"
5+
"fmt"
6+
"net/http"
7+
"sync/atomic"
8+
"testing"
9+
"time"
10+
11+
"github.com/databricks/databricks-sdk-go/httpclient"
12+
"github.com/databricks/databricks-sdk-go/httpclient/fixtures"
13+
)
14+
15+
// tokenCountingTransport wraps an http.RoundTripper and counts the number of
16+
// round trips made. It is used to verify that the token source does not cache
17+
// tokens internally, ensuring that each Token() call results in an HTTP
18+
// request.
19+
type tokenCountingTransport struct {
20+
inner http.RoundTripper
21+
count atomic.Int32
22+
}
23+
24+
func (t *tokenCountingTransport) RoundTrip(req *http.Request) (*http.Response, error) {
25+
t.count.Add(1)
26+
return t.inner.RoundTrip(req)
27+
}
28+
29+
func (t *tokenCountingTransport) SkipRetryOnIO() bool {
30+
return true
31+
}
32+
33+
// TestAzureClientSecretCredentials_tokenSourceFor_noCaching verifies that
34+
// the auth.TokenSource returned by tokenSourceFor does not cache tokens
35+
// internally. Before the fix, the code used clientcredentials.Config.TokenSource
36+
// which returns an oauth2.ReuseTokenSource with a 10s expiryDelta. This inner
37+
// cache swallowed proactive refresh attempts from the outer caching layers
38+
// (azureReuseTokenSource and serviceToServiceVisitor), causing bursts
39+
// of 401s at token expiry.
40+
// See https://github.com/databricks/databricks-sdk-go/issues/1549.
41+
func TestAzureClientSecretCredentials_tokenSourceFor_noCaching(t *testing.T) {
42+
counter := &tokenCountingTransport{
43+
inner: fixtures.MappingTransport{
44+
"POST /test-tenant-id/oauth2/token": {
45+
Response: map[string]any{
46+
"token_type": "Bearer",
47+
"access_token": "test-token",
48+
"expires_in": 3600,
49+
},
50+
},
51+
},
52+
}
53+
54+
cfg := &Config{
55+
AzureClientID: "test-client-id",
56+
AzureClientSecret: "test-client-secret",
57+
AzureTenantID: "test-tenant-id",
58+
}
59+
cfg.refreshClient = httpclient.NewApiClient(httpclient.ClientConfig{
60+
Transport: counter,
61+
})
62+
63+
c := AzureClientSecretCredentials{}
64+
ts := c.tokenSourceFor(context.Background(), cfg, "https://login.microsoftonline.com/", "https://management.azure.com/")
65+
66+
const numCalls = 3
67+
for i := range numCalls {
68+
tok, err := ts.Token(context.Background())
69+
if err != nil {
70+
t.Fatalf("Token() call %d: unexpected error: %v", i+1, err)
71+
}
72+
if tok.AccessToken != "test-token" {
73+
t.Errorf("Token() call %d: got access token %q, want %q", i+1, tok.AccessToken, "test-token")
74+
}
75+
}
76+
77+
if got := int(counter.count.Load()); got != numCalls {
78+
t.Errorf("token endpoint was called %d times, want %d (inner caching is present)", got, numCalls)
79+
}
80+
}
81+
82+
// TestAzureClientSecretCredentials_Configure verifies the happy path of the
83+
// full Azure client secret credential flow, including both the inner
84+
// (workspace) token and the management token.
85+
func TestAzureClientSecretCredentials_Configure(t *testing.T) {
86+
tokenExpiry := time.Now().Add(time.Hour).Unix()
87+
assertHeaders(t, &Config{
88+
Host: "https://adb-123.azuredatabricks.net",
89+
AzureClientID: "test-client-id",
90+
AzureClientSecret: "test-client-secret",
91+
AzureTenantID: "test-tenant-id",
92+
AuthType: "azure-client-secret",
93+
HTTPTransport: fixtures.MappingTransport{
94+
"POST /test-tenant-id/oauth2/token": {
95+
Response: map[string]any{
96+
"token_type": "Bearer",
97+
"access_token": "workspace-token",
98+
"expires_on": fmt.Sprintf("%d", tokenExpiry),
99+
},
100+
},
101+
},
102+
}, map[string]string{
103+
"Authorization": "Bearer workspace-token",
104+
"X-Databricks-Azure-Sp-Management-Token": "workspace-token",
105+
})
106+
}

0 commit comments

Comments
 (0)