Repository navigation
Skip upstream identity capture when sub_id is off - #6956
Conversation
Since #6908 the auth server stored the verified upstream identity (issuer, subject, stable claim and value) at every login, even when SubjectIdentifierAudiences was empty. Its only reader turns it into the sub_id and upstream_subject claims, and only for listed audiences. With the default empty list, every write was stored data nobody reads, and deployments that keep upstream identity out of storage had to strip it in their own storage layer. Record the identity at login only when SubjectIdentifierAudiences is not empty. Both login paths, the authorization-code callback and the device verification flow, now ask one helper, Handler.recordedUpstreamIdentity, so a future login path cannot forget the check. Nothing downstream changes: a zero value already means "no claim". Behaviour with disclosure on is unchanged. This reverses a choice made in #6908, which stored the identity so that a policy change applied at the next refresh. Now, going from an empty list to a non-empty one adds the claims only to sessions whose login happened while the list was non-empty. Other users get them from their next login. Adding an audience to a list that is already non-empty still applies at the next refresh, and emptying the list still stops the claims at the next refresh. Sessions created on v0.57.0 already hold the stored identity and keep it until their refresh token expires. There is no cleanup step. Update the SubjectIdentifierAudiences docs and the auth server storage architecture page to describe both cases, tell consumers not to switch a user's key based on whether sub_id is present, and note that a rolling restart leaves some logins unrecorded. Fixes #6955 Co-authored-by: Mecatl <noreply@mecatl.dev>
rdimitrov
left a comment
There was a problem hiding this comment.
LGTM. Verified that the only two places that turn an upstream login result into VerifiedUpstream (carryFirstUpstreamIdentity in the callback and completeDeviceLogin) now both go through recordedUpstreamIdentity; every other write just copies an existing value through storage, and the sole reader (applySubjectIdentifier in token.go) is unchanged and still requires every audience to be listed. The looser "list non-empty" gate at login vs. per-audience gate at issuance is the right split, and the new tests cover auth-code, chain leg and device paths in both states.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6956 +/- ##
==========================================
- Coverage 80.21% 80.20% -0.02%
==========================================
Files 844 844
Lines 85647 85649 +2
==========================================
- Hits 68702 68695 -7
- Misses 16940 16949 +9
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Summary
Since #6908 (v0.57.0) the auth server stores the verified upstream identity (issuer, subject, stable claim and value) at every login, even when
SubjectIdentifierAudiencesis empty, which is the default. The only reader turns it into thesub_idandupstream_subjectclaims, and only for listed audiences. With the list empty, every one of those writes is stored data nobody reads. It is a stable identifier for a user at their upstream IdP, and it stays at rest for the lifetime of a refresh token. Deployments that seal upstream identity at rest had to strip it in their own storage layer across several store methods.SubjectIdentifierAudiencesis not empty. OtherwiseVerifiedUpstreamstays zero, which downstream code already treats as "no claim".Handler.recordedUpstreamIdentity, so a new login path cannot forget the check.carryFirstUpstreamIdentitynow takes the value the helper returns.SubjectIdentifierAudiencesdocs anddocs/arch/11-auth-server-storage.mdto describe when the claims start to appear.Behaviour with disclosure on is unchanged.
Fixes #6955
Type of change
Test plan
task test)task test-e2e)task lint-fix)task testandtask lint-fixboth pass on this commit. I did not run the e2e tests or test against a kind cluster. The change does not touch RunConfig, CRDs or the operator.Changes
pkg/authserver/server/handlers/callback.gorecordedUpstreamIdentity;carryFirstUpstreamIdentitytakes the identity it returnspkg/authserver/server/handlers/device_verification.gorecordedUpstreamIdentityincompleteDeviceLoginpkg/authserver/server/handlers/subject_identifier_test.gopkg/authserver/integration_test.gopkg/authserver/config.go,runner/embeddedauthserver.go,server/session/session.go,storage/types.godocs/arch/11-auth-server-storage.mdDoes this introduce a user-facing change?
Only for Go callers that set
SubjectIdentifierAudiences. The setting is not reachable from config files, CRDs or the CLI, sothv, vMCP and operator users are not affected.sub_idandupstream_subjectto the next refresh of existing sessions.A user whose session predates the change can lack
sub_iduntil they log in again, and nothing reports why. Consumers should not switch the key they use for a user depending on whethersub_idis present. If disclosure is enabled with a rolling restart, logins served by a replica that still has the empty list record nothing. The arch doc says all of this.Special notes for reviewers
token.gostill requires every audience of a token to be listed before it adds the claims. The login-side check is looser on purpose: it only decides whether to record the value at all. I did not share a predicate between the two, because the conditions are different.ifinCallbackHandlertook it over the gocyclo limit, and two separate gates could drift apart.