Skip to content

Commit c62b155

Browse files
committed
revert to review comments
Signed-off-by: Sanskarzz <sanskar.gur@gmail.com>
1 parent 8f33688 commit c62b155

10 files changed

Lines changed: 409 additions & 632 deletions

File tree

docs/server/docs.go

Lines changed: 77 additions & 155 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

docs/server/swagger.json

Lines changed: 77 additions & 190 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

docs/server/swagger.yaml

Lines changed: 67 additions & 104 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

pkg/auth/discovery/discovery.go

Lines changed: 17 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -519,6 +519,7 @@ type OAuthFlowConfig struct {
519519
RegistrationAccessToken string //nolint:gosec // G117: field legitimately holds sensitive data
520520
RegistrationClientURI string
521521
TokenEndpointAuthMethod string
522+
RegisteredCallbackPort int
522523
}
523524

524525
// OAuthFlowResult contains the result of an OAuth flow
@@ -543,6 +544,7 @@ type OAuthFlowResult struct {
543544
RegistrationAccessToken string //nolint:gosec // G117: field legitimately holds sensitive data
544545
RegistrationClientURI string
545546
TokenEndpointAuthMethod string
547+
RegisteredCallbackPort int
546548
}
547549

548550
func shouldDynamicallyRegisterClient(config *OAuthFlowConfig) bool {
@@ -615,20 +617,12 @@ func PerformOAuthFlow(ctx context.Context, issuer string, config *OAuthFlowConfi
615617
// exists.
616618
//
617619
// One consequence of option (b) is that the resolver's RFC 7591 §3.2.1
618-
// expiry-driven refetch does NOT participate in the CLI's cross-
619-
// invocation persistence loop: each PerformOAuthFlow call builds a fresh
620-
// in-memory store, so a "cached but expired" entry from the previous
621-
// invocation never reaches the resolver. Cross-invocation expiry is also
622-
// NOT enforced by the remote handler's gate today —
623-
// HasCachedClientCredentials only checks CachedClientID != "" and does
624-
// not consult CachedSecretExpiry, so an expired-but-still-cached client
625-
// gets reused on the next invocation and surfaces as a token-endpoint
626-
// failure rather than a clean DCR re-registration. Tightening the gate
627-
// to also check CachedSecretExpiry is open follow-up work; the
628-
// behaviour today is "cross-invocation expiry is unhandled". Within a
629-
// single invocation, the resolver's expiry check is still in the loop
630-
// and would fire if the same call site somehow registered, persisted,
631-
// and re-queried the in-memory store — but the CLI never does this today.
620+
// expiry-driven refetch does NOT participate in the CLI's cross-invocation
621+
// persistence loop: each PerformOAuthFlow call builds a fresh in-memory store,
622+
// so a cached entry from a previous invocation never reaches the resolver.
623+
// Cross-invocation client-secret expiry is handled instead by the remote
624+
// handler, which consults CachedSecretExpiry and renews through RFC 7592 before
625+
// cached credentials are used.
632626
//
633627
// Wrapping the remote handler's secretProvider into a dcr.CredentialStore
634628
// adapter (option (a)) would close that loop and is the natural follow-up;
@@ -677,27 +671,14 @@ func handleDynamicRegistration(ctx context.Context, issuer string, config *OAuth
677671
}
678672

679673
// Store DCR renewal metadata for RFC 7592 operations.
680-
// client_secret_expires_at == 0 means the secret never expires (RFC 7591 §3.2.1).
681-
if registrationResponse.ClientSecretExpiresAt > 0 {
682-
config.SecretExpiry = time.Unix(registrationResponse.ClientSecretExpiresAt, 0)
683-
}
684-
config.RegistrationAccessToken = registrationResponse.RegistrationAccessToken
685-
config.RegistrationClientURI = registrationResponse.RegistrationClientURI
686-
687-
if registrationResponse.RegistrationAccessToken != "" {
688-
slog.Debug("DCR response includes registration access token for RFC 7592 operations")
689-
}
690-
691-
// Store DCR renewal metadata for RFC 7592 operations.
692-
// client_secret_expires_at == 0 means the secret never expires (RFC 7591 §3.2.1).
693-
if registrationResponse.ClientSecretExpiresAt > 0 {
694-
config.SecretExpiry = time.Unix(registrationResponse.ClientSecretExpiresAt, 0)
695-
}
696-
config.RegistrationAccessToken = registrationResponse.RegistrationAccessToken
697-
config.RegistrationClientURI = registrationResponse.RegistrationClientURI
698-
config.TokenEndpointAuthMethod = registrationResponse.TokenEndpointAuthMethod
699-
700-
if registrationResponse.RegistrationAccessToken != "" {
674+
// A zero ClientSecretExpiresAt means the secret never expires (RFC 7591 §3.2.1).
675+
config.SecretExpiry = resolution.ClientSecretExpiresAt
676+
config.RegistrationAccessToken = resolution.RegistrationAccessToken
677+
config.RegistrationClientURI = resolution.RegistrationClientURI
678+
config.TokenEndpointAuthMethod = resolution.TokenEndpointAuthMethod
679+
config.RegisteredCallbackPort = config.CallbackPort
680+
681+
if resolution.RegistrationAccessToken != "" {
701682
slog.Debug("DCR response includes registration access token for RFC 7592 operations")
702683
}
703684

@@ -938,6 +919,7 @@ func newOAuthFlow(ctx context.Context, oauthConfig *oauth.Config, config *OAuthF
938919
RegistrationAccessToken: config.RegistrationAccessToken,
939920
RegistrationClientURI: config.RegistrationClientURI,
940921
TokenEndpointAuthMethod: config.TokenEndpointAuthMethod,
922+
RegisteredCallbackPort: config.RegisteredCallbackPort,
941923
}, nil
942924
}
943925

pkg/auth/remote/config.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,10 @@ type Config struct {
8686
// CachedTokenEndpointAuthMethod is the auth method used for the token endpoint
8787
// (e.g., "client_secret_basic", "none"). Persisted for RFC 7592 updates.
8888
CachedTokenEndpointAuthMethod string `json:"cached_token_auth_method,omitempty" yaml:"cached_token_auth_method,omitempty"`
89+
// CachedDCRCallbackPort is the callback port that was actually registered
90+
// during DCR. It may differ from CallbackPort when the requested port was
91+
// unavailable and a fallback port was selected.
92+
CachedDCRCallbackPort int `json:"cached_dcr_callback_port,omitempty" yaml:"cached_dcr_callback_port,omitempty"`
8993
}
9094

9195
// BearerTokenEnvVarName is the environment variable name used for bearer token authentication.
@@ -194,6 +198,7 @@ func (c *Config) ClearCachedClientCredentials() {
194198
c.CachedRegTokenRef = ""
195199
c.CachedRegClientURI = ""
196200
c.CachedTokenEndpointAuthMethod = ""
201+
c.CachedDCRCallbackPort = 0
197202
}
198203

199204
// LogContext returns the upstream issuer and resolved client_id for use as

0 commit comments

Comments
 (0)