Commit 9572f6a
Wire authserver DCR resolver and add structured logs (#5044)
* Wire authserver DCR resolver and add structured logs
Implements Phase 2 steps 2d/2g of the DCR story (#5039):
- EmbeddedAuthServer now owns an in-memory DCRCredentialStore and calls
resolveDCRCredentials for any OAuth2 upstream with DCRConfig. The
resolved ClientSecret is overlaid on the built upstream.OAuth2Config
after buildPureOAuth2Config (whose signature and body remain
intentionally unchanged) so that RFC 7591-obtained credentials flow
through the same execution path as file/env-resolved secrets.
- Each UpstreamRunConfig element is shallow-copied and its OAuth2
sub-config is deep-copied before DCR resolution, preserving the
caller's RunConfig.Upstreams slice per .claude/rules/go-style.md
"Copy Before Mutating Caller Input".
- resolveDCRCredentials emits structured logs: Debug on cache hit with
dcr_age_days, an additional Warn when the cached registration exceeds
dcrStaleAgeThreshold (90 days), and Error with a "step" attribute
identifying which phase failed on every error path.
- The /oauth/register handler upgrades its success log to Info with
upstream, issuer, client_id, software_id, token_endpoint_auth_method,
and scopes. SoftwareID is threaded through DCRRequest validation so
incoming "software_id" is captured. A small helper guards against a
nil embedded *fosite.Config (a legitimate test-only condition).
- isTransientNetworkError's permanent-4xx branch now emits a Warn with
a DCR remediation hint before returning false unchanged. The
MonitoredTokenSource gains an optional SetUpstreamContext setter so
the upstream and client_id fields can be threaded into the log
without breaking the existing NewMonitoredTokenSource contract.
- Integration tests exercise the full DCR boot path against a mock AS,
verify the cache-hit short-circuit issues zero additional HTTP
requests, and assert the caller's original RunConfig.Upstreams slice
element is unchanged across both calls.
Address authserver DCR review feedback
Fixes from the CODE_REVIEW_ISSUES.md review of commit 71c4f43:
Critical
- Wire upstream/client_id into MonitoredTokenSource so the DCR remediation
warning carries meaningful correlation fields. Promote the fields to
constructor parameters (replacing SetUpstreamContext) to remove the
unsynchronized writer and force callers to supply them at construction.
- Runner populates the new fields from the RemoteAuthConfig, preferring
the DCR-cached client_id over the statically configured one.
High
- /oauth/register handler drops the redundant upstream attribute that
mirrored issuer, and omits issuer when empty rather than emitting a
bare issuer="".
- Resolver no longer logs each error branch and then returns; it now
wraps failures in a DCRStepError and the boundary caller
(buildUpstreamConfigs) emits a single slog.Error via LogDCRStepError.
- The DCR-resolved ClientSecret is applied through a new
applyResolutionToOAuth2Config helper paired with applyResolution, so
the DCR application sites live side-by-side and future call sites
cannot silently drop the secret.
Medium
- Remove the Type==OAuth2 guard that duplicated needsDCR's nil check.
- Cap software_id to 256 characters and require printable ASCII in
ValidateDCRRequest; expose MaxSoftwareIDLength.
- Add TestNewEmbeddedAuthServer_DCRBoot to drive the full constructor
and assert EmbeddedAuthServer.dcrStore is populated after boot.
- Remove the nil-guard in Handler.issuer() and add TestHandler_issuer
so a real wiring bug fails loudly instead of logging issuer="".
- Sanitize error strings before logging to strip URL query parameters
that could plausibly carry tokens in a future refactor.
Preserve URL-trailing punctuation in DCR log sanitiser
The query-stripping regex in sanitizeErrorForLog matches `[^\s"']+`, so a
URL ending with sentence punctuation (`.`, `,`, `)`, `]`, etc.) pulls
that punctuation into the URL match. url.Parse then absorbs it into the
raw query, and the Strip + Reassemble step drops it along with the rest
of the query — mangling the surrounding prose.
Split the trailing run of terminal punctuation off the match before
parsing, and re-append it verbatim after the query is stripped. URL
matches without a query are returned untouched so the pass is
idempotent for URLs that are already clean. New test cases cover commas,
periods, closing parens, mixed runs, and a Go http.Client-style quoted
URL.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Bound upstream DCR /register error body before logging
A misbehaving or malicious authorization server could echo arbitrary
content into the upstream's /register error response. handleHTTPResponse
read that body via io.ReadAll with no LimitReader, then embedded it
verbatim in the returned error — which downstream callers log. Cap the
read at 8 KiB (far larger than any conformant RFC 7591 error response)
so operator log volume cannot be inflated by a non-conformant upstream.
Addresses #5044 review finding F2 (HIGH).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Mirror CIMD precedence in remoteAuthLogContext
The doc comment claimed remoteAuthLogContext mirrored the precedence of
remote.Handler.resolveClientCredentials, but the implementation skipped
the CachedCIMDClientID check entirely. For CIMD-authenticated workloads
the new DCR remediation Warn would have reported a stale or empty
client_id rather than the CIMD URL actually being sent on token refresh,
defeating the operator-correlation the field exists for.
Restore the documented precedence (CachedCIMDClientID >
CachedClientID > ClientID) and add a TestRemoteAuthLogContext case
covering the CIMD-wins path.
Addresses #5044 review findings F3 (HIGH) and F26
(MEDIUM, closed by the new test case).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Hoist DCR remediation Warn out of network-error classifier
isTransientNetworkError previously emitted the cached-DCR-client
remediation Warn from inside its classifier on every permanent 4xx.
A tight Token() loop hitting the same condition would spam the same
record on every call before the workload's Unauthenticated status
propagated. The same branch also fired for non-DCR workloads, which
saw a remediation telling them to "delete cached credentials" they
never had.
Strip the side effect from the classifier and emit the Warn from
markAsUnauthenticated, which already gates the close-monitoring
transition through stopOnce. The Warn now fires at most once per
state transition, is suppressed when no client_id context is
available, and reads honestly about the variability ("if this
workload uses cached DCR or CIMD credentials they may be stale").
Addresses #5044 review finding F5 (MEDIUM).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Validate handler config eagerly and demote DCR success log
Two related polish items in the authserver handlers package:
NewHandler did not validate that AuthorizationServerConfig (or its
embedded *fosite.Config) was non-nil. issuer() reached into the
config at request time, so a misconfigured caller would panic deep
inside an HTTP handler instead of failing at startup. Add the nil
check to the constructor and simplify issuer() to rely on the
constructor invariant. Pin the new invariant with
TestNewHandler_ErrorsOnNilConfig.
The /oauth/register success log was promoted to Info even though the
operation is neither long-running nor exceptional. Demote back to
Debug; an audit-log path is the right home for the audit signal if
that becomes a requirement.
Addresses #5044 review findings F6 and F7.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Tighten DCR resolver internals and clarify cache lifetimes
Bundles ten review-feedback items in pkg/authserver/runner; each
addresses internal-API hygiene or doc-comment drift in the DCR
resolver, no behaviour change for callers.
Sanitizer hardening (F1, F19, F18, F23): sanitizeErrorForLog now
strips userinfo and fragment in addition to the query, since either
can carry credentials or tokens (https://user:pass@host,
implicit-flow #access_token=...). queryStrippingPattern matches
http/https case-insensitively per RFC 3986 §3.1 so HTTPS://...
cannot escape sanitisation. trimURLTrailingPunctuation switches to
strings.IndexByte to match the ASCII-only terminator set without
the rune-decoding overhead. Test cases added for each.
Resolver error API (F4, F13, F20): the dcrStepRegister panic-recovery
branch no longer emits a duplicate slog.Error; the captured stack
travels with the wrapped *dcrStepError to the boundary log so a
single panic produces a single record. DCRStepError /
LogDCRStepError lowercased to dcrStepError / logDCRStepError since
no caller lives outside the package. logDCRStepError now no-ops on
nil err so the unknown-step branch cannot fire on a missing failure.
Resolution helpers (F11, F12, F25): applyResolution renamed to
consumeResolution to communicate that it is a one-shot state
transition (clearing DCRConfig is unconditional), not an idempotent
defaulting step. The applyResolutionToOAuth2Config doc now states
the paired-call invariant explicitly without referencing a specific
test.
Lifecycle docs (F21, F22): the per-instance dcrStore vs.
process-wide dcrFlight asymmetry is now stated on both sides, and
EmbeddedAuthServer.Close documents the future-Close hook for a
backend with handles.
Inline rules-file rationale (F24): production comments no longer
cite .claude/rules/... by path; the principle is inlined.
Addresses #5044 review findings F1, F4, F11, F12,
F13, F18, F19, F20, F21, F22, F23, F24, F25.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Tighten DCR remediation hint and drain capped error body
Three follow-up items from review:
isPermanentTokenEndpointError used to treat every non-5xx
RetrieveError as permanent, which meant the DCR remediation Warn
fired on transient back-pressure (408 Request Timeout, 429 Too Many
Requests). Narrow the classifier to an explicit 400 / 401 / 403
allowlist so the "delete the cached credentials and restart" hint
fires only on responses where stale cached client credentials are a
plausible cause. Retry behaviour is unaffected — the retry decision
is gated entirely by isTransientNetworkError, which already returns
false for the whole RetrieveError branch.
handleHTTPResponse in pkg/oauthproto/dcr.go now drains the response
body after the LimitReader-bounded ReadAll. Without this, an upstream
returning more than 8 KiB on /register error left the connection mid-
stream when the deferred Close fired, preventing TCP connection
reuse. Mirrors what the non-JSON content-type branch a few lines
down already does.
remoteAuthLogContext moved out of pkg/runner into pkg/auth/remote as
*Config.LogContext(), collocating it with resolveClientCredentials
so the cached-CIMD > cached-DCR > static precedence has one home.
Adding a fourth cached field in future updates both call sites in
one place. Tests moved to pkg/auth/remote/config_test.go.
Addresses #5044 review #4212656411 comments
3174540081, 3174540082, and 3174540086.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>1 parent 11fa6fb commit 9572f6a
19 files changed
Lines changed: 1363 additions & 104 deletions
File tree
- pkg
- authserver
- runner
- server
- handlers
- registration
- auth
- remote
- oauthproto
- runner
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
9 | 9 | | |
10 | 10 | | |
11 | 11 | | |
| 12 | + | |
12 | 13 | | |
13 | 14 | | |
14 | 15 | | |
| |||
129 | 130 | | |
130 | 131 | | |
131 | 132 | | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
132 | 142 | | |
133 | 143 | | |
134 | 144 | | |
| |||
212 | 222 | | |
213 | 223 | | |
214 | 224 | | |
| 225 | + | |
| 226 | + | |
215 | 227 | | |
216 | 228 | | |
217 | 229 | | |
| |||
226 | 238 | | |
227 | 239 | | |
228 | 240 | | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
229 | 254 | | |
230 | 255 | | |
231 | 256 | | |
232 | 257 | | |
| 258 | + | |
| 259 | + | |
233 | 260 | | |
234 | 261 | | |
235 | 262 | | |
236 | 263 | | |
237 | 264 | | |
| 265 | + | |
| 266 | + | |
238 | 267 | | |
239 | 268 | | |
240 | 269 | | |
241 | 270 | | |
242 | | - | |
| 271 | + | |
| 272 | + | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
243 | 277 | | |
244 | 278 | | |
245 | 279 | | |
| |||
264 | 298 | | |
265 | 299 | | |
266 | 300 | | |
267 | | - | |
| 301 | + | |
| 302 | + | |
| 303 | + | |
| 304 | + | |
268 | 305 | | |
269 | 306 | | |
270 | 307 | | |
| |||
273 | 310 | | |
274 | 311 | | |
275 | 312 | | |
276 | | - | |
| 313 | + | |
| 314 | + | |
| 315 | + | |
| 316 | + | |
277 | 317 | | |
278 | 318 | | |
279 | 319 | | |
| |||
349 | 389 | | |
350 | 390 | | |
351 | 391 | | |
| 392 | + | |
| 393 | + | |
| 394 | + | |
| 395 | + | |
| 396 | + | |
352 | 397 | | |
353 | 398 | | |
354 | 399 | | |
| |||
399 | 444 | | |
400 | 445 | | |
401 | 446 | | |
| 447 | + | |
| 448 | + | |
| 449 | + | |
| 450 | + | |
| 451 | + | |
| 452 | + | |
| 453 | + | |
| 454 | + | |
| 455 | + | |
| 456 | + | |
| 457 | + | |
| 458 | + | |
| 459 | + | |
| 460 | + | |
| 461 | + | |
| 462 | + | |
| 463 | + | |
| 464 | + | |
| 465 | + | |
| 466 | + | |
| 467 | + | |
| 468 | + | |
| 469 | + | |
| 470 | + | |
| 471 | + | |
| 472 | + | |
| 473 | + | |
| 474 | + | |
| 475 | + | |
| 476 | + | |
402 | 477 | | |
403 | 478 | | |
404 | 479 | | |
| |||
414 | 489 | | |
415 | 490 | | |
416 | 491 | | |
417 | | - | |
418 | | - | |
| 492 | + | |
| 493 | + | |
| 494 | + | |
| 495 | + | |
| 496 | + | |
| 497 | + | |
| 498 | + | |
| 499 | + | |
419 | 500 | | |
420 | 501 | | |
421 | 502 | | |
422 | 503 | | |
423 | 504 | | |
424 | 505 | | |
425 | | - | |
| 506 | + | |
| 507 | + | |
| 508 | + | |
| 509 | + | |
| 510 | + | |
| 511 | + | |
| 512 | + | |
| 513 | + | |
| 514 | + | |
| 515 | + | |
| 516 | + | |
| 517 | + | |
| 518 | + | |
| 519 | + | |
| 520 | + | |
| 521 | + | |
| 522 | + | |
| 523 | + | |
| 524 | + | |
| 525 | + | |
| 526 | + | |
426 | 527 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
118 | 118 | | |
119 | 119 | | |
120 | 120 | | |
121 | | - | |
| 121 | + | |
122 | 122 | | |
123 | 123 | | |
124 | 124 | | |
| |||
151 | 151 | | |
152 | 152 | | |
153 | 153 | | |
154 | | - | |
| 154 | + | |
155 | 155 | | |
156 | 156 | | |
157 | 157 | | |
| |||
195 | 195 | | |
196 | 196 | | |
197 | 197 | | |
198 | | - | |
| 198 | + | |
199 | 199 | | |
200 | 200 | | |
201 | 201 | | |
| |||
245 | 245 | | |
246 | 246 | | |
247 | 247 | | |
248 | | - | |
| 248 | + | |
249 | 249 | | |
250 | 250 | | |
251 | 251 | | |
| |||
297 | 297 | | |
298 | 298 | | |
299 | 299 | | |
300 | | - | |
| 300 | + | |
301 | 301 | | |
302 | 302 | | |
303 | 303 | | |
| |||
341 | 341 | | |
342 | 342 | | |
343 | 343 | | |
344 | | - | |
| 344 | + | |
345 | 345 | | |
346 | 346 | | |
347 | 347 | | |
| |||
374 | 374 | | |
375 | 375 | | |
376 | 376 | | |
377 | | - | |
| 377 | + | |
378 | 378 | | |
379 | 379 | | |
380 | 380 | | |
| |||
407 | 407 | | |
408 | 408 | | |
409 | 409 | | |
410 | | - | |
| 410 | + | |
411 | 411 | | |
412 | 412 | | |
413 | 413 | | |
| |||
627 | 627 | | |
628 | 628 | | |
629 | 629 | | |
630 | | - | |
| 630 | + | |
631 | 631 | | |
632 | 632 | | |
633 | 633 | | |
| |||
647 | 647 | | |
648 | 648 | | |
649 | 649 | | |
650 | | - | |
| 650 | + | |
651 | 651 | | |
652 | 652 | | |
653 | 653 | | |
| |||
698 | 698 | | |
699 | 699 | | |
700 | 700 | | |
701 | | - | |
| 701 | + | |
702 | 702 | | |
703 | 703 | | |
704 | 704 | | |
| |||
738 | 738 | | |
739 | 739 | | |
740 | 740 | | |
741 | | - | |
| 741 | + | |
742 | 742 | | |
743 | 743 | | |
744 | 744 | | |
| |||
793 | 793 | | |
794 | 794 | | |
795 | 795 | | |
796 | | - | |
| 796 | + | |
797 | 797 | | |
798 | 798 | | |
799 | 799 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
186 | 186 | | |
187 | 187 | | |
188 | 188 | | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
189 | 213 | | |
190 | 214 | | |
191 | 215 | | |
| |||
0 commit comments