refactor(credential): carry wire key identifiers through the accepted set#729
Merged
aaron-zeisler merged 3 commits intoJun 30, 2026
Merged
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 072ed9f. Configure here.
aaron-zeisler
force-pushed
the
aaronz/SDK-2534/credential-key-identifiers
branch
from
072ed9f to
d3d3a0e
Compare
aaron-zeisler
changed the base branch from
feat/concurrent-keys
to
aaronz/reject-primary-mobile-not-in-array
This was referenced Jun 29, 2026
keelerm84
reviewed
Jun 30, 2026
| reconcileAcceptedKeys(desired, r.acceptedSDKKeys, &r.additions, &r.expirations, r.loggers, "SDK key") | ||
| // Refresh the wire "key" identifier for every key now in the accepted set, including clearing it | ||
| // when the new payload carries none — otherwise a stale identifier would linger in /status. | ||
| for key, info := range set.sdkKeys { |
Member
There was a problem hiding this comment.
Iterating through this a second time is not ideal.
Right now, reconcileAcceptedKeys takes in a desired map of [keys]*time.Time. That function iterates the map and updates the expiration on the accepted keys.
If you updated the desired map to array the full value as I point out above, and update this function, then I think you can update both in a single iteration.
Base automatically changed from
aaronz/reject-primary-mobile-not-in-array
to
feat/concurrent-keys
June 30, 2026 16:51
… set
Replace the builder's WithExpiring{SDK,Mobile}Key methods with a
param-struct API (SDKKeyParams/MobileKeyParams) so each accepted key can
carry its optional wire "key" identifier alongside its value and expiry.
AcceptedSet map values become acceptedKeyInfo (value, expiry, and the
optional key identifier); the rotator's reconcile loops thread the
identifier through, refreshing it on every reconcile and clearing it when
a later payload carries none. Add util.PtrOrNil for the optional-field
modelling.
This is the storage layer for surfacing per-key identifiers on the status
endpoint; no externally visible behaviour changes yet.
aaron-zeisler
force-pushed
the
aaronz/SDK-2534/credential-key-identifiers
branch
from
d3d3a0e to
a094998
Compare
Per review: have the desired-set maps carry the full acceptedKeyInfo (value metadata, not just expiry) and have reconcileAcceptedKeys refresh both the expiry and the wire "key" identifier in its add/update pass. This drops the separate second loop that reconcileSDKKeys/ reconcileMobileKeys used to run to refresh identifiers. Behaviour is unchanged, including clearing a stale identifier when a later payload carries none; the anchor and primary mobile key are still forced permanent while keeping their identifiers.
aaron-zeisler
commented
Jun 30, 2026
… builder Per review: - The rotator's acceptedSDKKeys/acceptedMobileKeys maps now hold acceptedKeyInfo by value rather than by pointer. The struct is two pointer fields, so copies are cheap, the per-key heap allocation goes away, and snapshots are free of aliasing into rotator state. The reconcile add/update collapses to a single map assignment. - Drop the explicit anchor / primary-mobile "force permanent" handling in reconcileSDKKeys/reconcileMobileKeys. The builder already guarantees the designated keys are present and permanent, and Reconcile trusts a well-formed set, so the desired-set build is now a plain filter loop.
aaron-zeisler
commented
Jun 30, 2026
| // acceptedMobileKeys is the full set of accepted mobile keys with optional per-key expiry. | ||
| // A nil expiry means the key is permanent. | ||
| acceptedMobileKeys map[config.MobileKey]*acceptedKeyInfo | ||
| acceptedMobileKeys map[config.MobileKey]acceptedKeyInfo |
Contributor
Author
There was a problem hiding this comment.
Note for reviewers: Here I decided to go with values instead of pointers. This caused a handful of random-looking changes throughout the pull request.
keelerm84
approved these changes
Jun 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
Stacked PR — splitting #724 (review/merge bottom-up):
mobileKeys[]AcceptedKeyssdkKeys[]/mobileKeys[]on/statusBase: #728.
Summary
Carry the wire
keyidentifier through the credential accepted-set model so it can later be surfaced on the/statusendpoint. This is the storage/model layer only — no externally visible behaviour changes yet.Jira: SDK-2534
Background
Second in the stack splitting #724, based on #728. Each accepted key needs to carry its optional non-secret identifier alongside its value and expiry.
Because this PR sits on #728, the per-entry primary-mobile designation introduced here is paired with #728's
mobKey-not-in-mobileKeys[]rejection: a defined primary that is absent from the array is rejected outright rather than silently left undesignated.Changes
WithExpiring{SDK,Mobile}Keywith a param-struct API (SDKKeyParams/MobileKeyParams).AcceptedSetmap values becomeacceptedKeyInfo(value, expiry, optionalkey).util.PtrOrNilfor optional-field modelling.Note
Medium Risk
Changes the credential reconcile/build path that gates which keys are accepted, but authentication semantics are unchanged; the main new behavior is metadata refresh on reconcile (identifiers can clear or update).
Overview
Extends the credential accepted-set model so each SDK/mobile key stores optional wire
keyidentifiers (plus expiry) alongside the secret value, preparing later/statusexposure without changing auth behavior in this PR.The
AcceptedSetBuilderAPI is consolidated: separate expiring-key helpers are replaced bySDKKeyParams/MobileKeyParams(value, optional identifier, optional expiry).WithAnchor/WithPrimaryMobileKeystill force permanent keys and can overwrite earlier entries for the same value.BuildAcceptedSetnow designates anchor and primary mobile keys while walking the wire arrays and passes identifiers throughutil.PtrOrNil. Rename tests expectAcceptedSetequality to differ when only the display identifier changes.The rotator stores
acceptedKeyInfoby value and, on reconcile, replaces full per-key metadata so identifiers refresh and absent wirekeyfields clear stale names. Reconcile no longer re-injects anchor/primary into the desired map separately; it trusts the built set from the builder.Reviewed by Cursor Bugbot for commit 5eceeb4. Bugbot is set up for automated code reviews on this repo. Configure here.