uhttp: allow OAuth2JWT.CreateJWTConfig to be re-invoked per token fetch - #1100
uhttp: allow OAuth2JWT.CreateJWTConfig to be re-invoked per token fetch#1100sergiocorral-conductorone wants to merge 1 commit into
Conversation
Previously OAuth2JWT.GetClient called CreateJWTConfig exactly once and baked the resulting *jwt.Config (including its PrivateClaims map) into the oauth2.ReuseTokenSource for the client's whole lifetime. Any connector whose token endpoint requires a fresh per-request claim in PrivateClaims -- most commonly a jti nonce for RFC 7523 replay protection -- got the SAME static claim on every token refresh after the first, defeating the nonce. Wrap the token source in a reCreatingJWTSource that rebuilds the *jwt.Config via CreateJWTConfig on each Token() call, then wrap that in oauth2.ReuseTokenSource so normal caching is preserved: CreateJWTConfig is only re-invoked when the cached token has expired, not on every HTTP request. Backward compatible: the CreateJWTConfig and NewOAuth2JWT signatures are unchanged; callers returning a static config behave identically (idempotent rebuild = same result), while callers can now emit per-fetch dynamic claims (e.g. a fresh jti) from inside their callback. Found via baton-oracle-fccs needing a per-request jti nonce. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| func (o *OAuth2JWT) GetClient(ctx context.Context, options ...Option) (*http.Client, error) { | ||
| httpClient, err := getHttpClient(ctx, options...) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("creating JWT config failed: %w", err) | ||
| return nil, err | ||
| } | ||
|
|
||
| ctx = context.WithValue(ctx, oauth2.HTTPClient, httpClient) | ||
| ts := jwt.TokenSource(ctx) | ||
| ts := oauth2.ReuseTokenSource(nil, reCreatingJWTSource{ | ||
| ctx: ctx, | ||
| createfn: o.CreateJWTConfig, | ||
| credentials: o.Credentials, | ||
| scopes: o.Scopes, | ||
| }) |
There was a problem hiding this comment.
🟡 Suggestion: GetClient no longer surfaces CreateJWTConfig failures — the callback is now only reached lazily from Token(), so malformed credentials produce a non-nil client and a nil error, and a nil CreateJWTConfig panics inside client.Do instead of at construction. Connectors that call GetClient in New() and rely on it to reject bad config lose their fail-fast check; a connector whose Validate() makes no HTTP call would now report invalid credentials as valid. Consider invoking the callback once eagerly to keep the old contract while still rebuilding per fetch:
if _, err := o.CreateJWTConfig(o.Credentials, o.Scopes...); err != nil {
return nil, fmt.Errorf("creating JWT config failed: %w", err)
}
ctx = context.WithValue(ctx, oauth2.HTTPClient, httpClient)(Confidence: high that the behavior changed; medium on downstream impact.)
| // reCreatingJWTSource is an oauth2.TokenSource that rebuilds the *jwt.Config via | ||
| // CreateJWTConfig on every token fetch. This lets callers inject per-request | ||
| // claims (e.g. a fresh jti nonce in PrivateClaims for RFC 7523 replay | ||
| // protection) that must change on each token acquisition. Wrapping this in an | ||
| // oauth2.ReuseTokenSource preserves normal token caching: CreateJWTConfig is | ||
| // only re-invoked when the cached token has expired, not on every HTTP request. |
There was a problem hiding this comment.
🟡 Suggestion: the new "invoked per token acquisition" contract is documented on the unexported reCreatingJWTSource, where callers won't see it. The exported CreateJWTConfig type (line 114) silently changed from called-exactly-once to called-once-per-token-acquisition; a callback that fetches a signing key from a secret manager, rotates a key, or otherwise has side effects now runs repeatedly for the life of the client. Please move/duplicate a // Deprecated-style contract note onto type CreateJWTConfig and OAuth2JWT.CreateJWTConfig stating that it must be safe to call repeatedly, and flag the default-behavior change in the release notes / pkg/sdk/version.go bump per the repo's compatibility criteria. (Confidence: medium.)
| func jwtBearerServer(t *testing.T, expiresIn int) (*httptest.Server, *[]string) { | ||
| t.Helper() | ||
| assertions := make([]string, 0) | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| require.Equal(t, "POST", r.Method) | ||
| require.Equal(t, "urn:ietf:params:oauth:grant-type:jwt-bearer", r.FormValue("grant_type")) | ||
| assertions = append(assertions, r.FormValue("assertion")) | ||
|
|
||
| w.Header().Set("Content-Type", "application/json") | ||
| _, _ = w.Write([]byte(fmt.Sprintf(`{"access_token": "test-access-token", "token_type": "bearer", "expires_in": %d}`, expiresIn))) | ||
| })) | ||
| return server, &assertions | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: assertions is appended from the httptest handler goroutine and read from the test goroutine (require.Len(t, *assertions, ...)) with no synchronization — there's no formal happens-before edge across the socket, so this can trip -race. Also, require.* inside a handler goroutine calls t.FailNow()/runtime.Goexit() off the test goroutine, which testify documents as unsupported (it kills the handler and leaves the test hanging on a truncated response rather than failing cleanly). Guarding the slice with a sync.Mutex and returning a func() []string snapshot accessor, plus switching the in-handler checks to assert.*, makes the helper race-free. (Confidence: medium on the race tripping -race, high on the require-in-goroutine issue.)
General PR Review: uhttp: allow OAuth2JWT.CreateJWTConfig to be re-invoked per token fetchBlocking Issues: 0 | Suggestions: 3 | Threads Resolved: 0 Review SummaryScanned the full PR diff ( Risk triage (per
Security IssuesNone found. No credential material is logged; the Correctness IssuesNone found. Verified against vendored Suggestions
Prompt for AI agents |
1 participant
Motivation
OAuth2JWT.GetClient(RFC 7523 two-legged JWT-bearer OAuth) invokedCreateJWTConfigexactly once and baked the resulting*jwt.Config—including its
PrivateClaimsmap — into anoauth2.ReuseTokenSourcefor theclient's whole lifetime. Any connector whose token endpoint requires a fresh
per-request claim in
PrivateClaims— most commonly ajtinonce forreplay protection, a real recurring pattern for RFC 7523 JWT-bearer APIs — got
the same static claim on every token refresh after the first, defeating the
nonce.
This was discovered blocking a real connector (baton-oracle-fccs) from
using
OAuth2JWTas designed.Fix
Wrap the token source in a small
reCreatingJWTSourcethat rebuilds the*jwt.ConfigviaCreateJWTConfigon eachToken()call, then wrap that inoauth2.ReuseTokenSource. The result:CreateJWTConfigis re-invoked on eachtoken acquisition (when the cached token has expired), not once at
GetClient-time — while normal token caching between expirations is preserved.Backward compatibility
CreateJWTConfigandNewOAuth2JWTsignatures are unchanged — noexported surface change.
CreateJWTConfigreturns a static config behave identically(idempotent rebuild = same result), and still get cached tokens between
expirations.
jti) simply bygenerating them inside their existing callback.
One minor behavior note: an error from
CreateJWTConfignow surfaces on thefirst token fetch rather than at
GetClienttime (the token source is lazy).The error is still wrapped with the same
"creating JWT config failed"message.Tests
Added three unit tests in
pkg/uhttp/authcredentials_test.go:ReinvokesCreateJWTConfig— with a short-lived token,CreateJWTConfigis invoked more than once across multiple fetches.
FreshJTIPerFetch— a callback that stamps a freshPrivateClaims["jti"]produces a distinct
jtiin each assertion the token endpoint receives(decoded from the signed JWT).
CachesTokenForStaticConfig— with a long-lived token,CreateJWTConfigruns exactly once and the token endpoint is hit once across several requests,
proving
ReuseTokenSourcecaching is intact for the common case.go build ./...,go vet ./pkg/uhttp/...,gofmt, and the fullpkg/uhttpsuite all pass. (An unrelated pre-existing flake,
TestC1ZConcurrentCloseinpkg/dotc1z, passes on isolated re-run and is untouched by this change.)was opened by an automated assistant and must not be self-merged or
auto-approved.
🤖 Generated with Claude Code