Repository navigation
Add exchange refresh token client config - #6989
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6989 +/- ##
==========================================
- Coverage 80.34% 80.29% -0.05%
==========================================
Files 848 848
Lines 86420 86481 +61
==========================================
+ Hits 69433 69444 +11
- Misses 16982 17032 +50
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ChrisJBurns
left a comment
There was a problem hiding this comment.
-
medium · correctness — DelegateClientTokenExchange.Clone shares the new ExchangeRefreshTokens pointer (
pkg/authserver/config.go)Clone promises a copy that shares nothing with the original, but the new *DelegateClientExchangeRefreshTokens is copied by pointer. Changing a cloned config would also change the source. The author knows, but it is not fixed in this diff.
3a4af72 to
d92d44d
Compare
A delegate client that exchanges a login token for a refresh token gets
a credential that outlives the login. Without per-client limits, one
leaked token can become long-term access, and if the server accepts
any self-issued subject token, one client can spend another client's
token.
Threat this closes. reporting-svc is a confidential client whose access
token AT1 (5 minutes, aud=crm.example) is issued for alice. AT1 leaks
from a log line. Without the source policy in the parent commit,
mallory-cli, another confidential client, exchanges AT1 for a token
with aud=payroll.example. If exchange can also mint refresh tokens,
the same leak becomes a refresh token whose life is limited only by
the server default, and it keeps renewing while it is used.
This commit adds the configuration that lets an operator decide, for
each client, whether exchange may issue refresh tokens and how long
they may live:
token_exchange:
self_issued_subject_tokens: restricted
api_audience: https://crm.example
exchange_refresh_tokens:
id_jag_max_lifetime: 5m
refresh_token_max_lifetime: 720h
Validation requires at least one cap, each positive and at most 720h,
api_audience set, restricted subject tokens, and a confidential client
with the token-exchange grant. An empty cap refuses that grant kind.
The 720h ceiling is a local choice; no specification sets it. The caps
are projected into ConfiguredClientPolicy keyed by token type. Nothing
reads the projection yet, so this commit changes no behaviour.
The source check reads the client_id of the subject token, which names
the client that obtained it. Only that client, or a listed source
client, may exchange it. The check does not compare sub with iss. For
a login token, sub is the user and iss is the server, so requiring them
to match would reject every valid login token.
Still open after this commit. The later commits in this stack close
these:
- Caps are not enforced when a token is issued. The commit that adds
refresh tokens through token exchange enforces them.
- A per-token cap alone does not bound a chain: each refresh could get
a new full window. Refresh tokens do not rotate, and a child grant
expires no later than its parent and cannot be a parent itself. The
root's expiry, set once at issuance, becomes the bound.
- An ID-JAG minted from a refresh token must also respect the parent's
expiry. The commit that derives child grants enforces this.
Co-authored-by: Mecatl <noreply@mecatl.dev>
The new config tests say "unparseable", which codespell rejects. Use "unparsable". The exchange_refresh_tokens block adds one schema to the OpenAPI document, so the deliberate SDK schema count moves from 204 to 205. Co-authored-by: Mecatl <noreply@mecatl.dev>
DelegateClientTokenExchange.Clone copied the ExchangeRefreshTokens pointer, so a change to a cloned policy also changed the original. Clone now copies the block, and its comment says the clone shares no slices or pointers with the original. Nothing writes to the block after cloning today, so no behaviour changes. Co-authored-by: Mecatl <noreply@mecatl.dev>
de1d0da to
cc7f5fb
Compare
|
@ChrisJBurns good catch, thanks. |
Summary
Stacked on #6988 (token-exchange source policy).
A delegate client that exchanges a login token for a refresh token gets a credential that outlives the login. Without per-client limits, one leaked token can become long-term access. This PR adds the operator-facing configuration that will bound those refresh tokens: an
exchange_refresh_tokensblock undertoken_exchange, with one lifetime cap per grant kind (id_jag_max_lifetime,refresh_token_max_lifetime).What changed:
exchange_refresh_tokensblock with two duration caps. Each cap must be positive and at most 720h, and at least one must be set.api_audience,self_issued_subject_tokens: restricted, a confidential client, and the token-exchange grant. An empty cap refuses that grant kind.ConfiguredClientPolicy, keyed by token type. Nothing reads the projection yet.Threat:
reporting-svc(confidential,aud=crm.example) exchanges alice's login token; the access token AT1 leaks from a log line. Without the source policy,mallory-clican exchange AT1 for apayroll.exampletoken. Once refresh tokens can be minted through exchange, the same leak becomes a long-lived refresh token. The caps bound that window. Enforcement is in later PRs in this stack.Type of change
Test plan
task test): not run yettask test-e2e): not run yettask lint-fix): not runRan
go vetonpkg/authserver,server/...,runner/...andstorage/...: passes. Local builds were blocked by a full disk, so the test suite has not been run on this branch. Config validation has table tests inregistration/client_test.goandconfig_test.gofrom the parent commits.API Compatibility
v1beta1APIDoes this introduce a user-facing change?
Yes, additively. RunConfig and the auth server config accept a new optional
token_exchange.exchange_refresh_tokensblock. Before: the field is rejected as unknown. After: the field is accepted and validated, and has no effect on token issuance yet.Special notes for reviewers
docs/arch/17has no subsection for this block yet, andDelegateClientTokenExchange.Clone()still shares the new pointer between copies. Both are planned before merge.🤖 Generated with Claude Code