-
Notifications
You must be signed in to change notification settings - Fork 51
FEAT: token-expiry capture and identity-aware pool key #660
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
jahnvi480
wants to merge
23
commits into
main
Choose a base branch
from
jahnvi/identity-aware-pooling
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 10 commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
cd40975
feat(auth): token-expiry capture and identity-aware pool key (#651, #…
jahnvi480 e3de108
Identity-aware connection pooling: isolate pools by identity (#651)
jahnvi480 32236ef
feat(pooling): defer token acquisition to a lazy factory on pool miss…
jahnvi480 5220262
test: cover lazy token-factory edge cases (#659)
jahnvi480 d38ca6e
docs+test: reconcile design scope with PR #660 and add cross-identity…
jahnvi480 29c84f9
refactor(pooling): remove dead code and tidy comments in identity-awa…
jahnvi480 360d407
chore: remove identity-aware pooling design doc from branch
jahnvi480 5814ee2
Add silent-first interactive auth and lazy eviction of idle identity …
jahnvi480 0181b52
Reduce pool eviction contention and document auth/pooling contracts
jahnvi480 0b5f2dc
Reset eviction-sweep throttle on reconfigure and closePools
jahnvi480 e285e25
test: cover account-keyed pool identity and warning double-check paths
jahnvi480 65dd8e4
Merge branch 'main' into jahnvi/identity-aware-pooling
jahnvi480 b4aed11
fix: isolate pool key for raw access tokens; close pools outside mana…
jahnvi480 4ba2d4f
Fail closed on token acquisition and harden pooled token refresh/evic…
jahnvi480 e158e1d
Reset _pools_closed on enable so re-enable re-arms the disable guard
jahnvi480 5700020
Surface auth-acquisition failures as InterfaceError; document token c…
jahnvi480 0636c6c
Fail closed on non-binary access token; add adversarial pool-key tests
jahnvi480 71642a3
Harden token factory and pool reuse
jahnvi480 4e6b82f
Merge branch 'main' into jahnvi/identity-aware-pooling
bewithgaurav f402613
Merge branch 'main' into jahnvi/identity-aware-pooling
jahnvi480 0a03c86
Merge branch 'main' into jahnvi/identity-aware-pooling
jahnvi480 9db2266
Merge branch 'main' into jahnvi/identity-aware-pooling
jahnvi480 6b0167f
Merge branch 'main' into jahnvi/identity-aware-pooling
jahnvi480 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Large diffs are not rendered by default.
Oops, something went wrong.
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In
isTokenNearExpiry, a_tokenExpiryEpochof 0 (unknown expiry) is treated as "not expiring," so a factory-backed msi/acct connection whose factory didn't return an expiry just gets reused forever and never comes up for refresh.What caught my eye is that the sibling helper
tokenExpirySafelyBeyondover inconnection_pool.cppdoes the opposite, it treats<= 0as not safe and fails closed. So the two functions disagree on what "unknown expiry" should mean, which felt worth a second look.I don't think this is actually dangerous in practice: real MSI and interactive tokens always come back with
expires_onpopulated, and even if a token did slip through and expire, ODBC's connection resiliency would just re-auth on the next use. Worst case is a query-time error, not one identity getting handed another's connection, the pool key still keeps identities separated. So this reads as a reliability edge, not a security hole.Two small things from me:
isTokenNearExpiryfail closed liketokenExpirySafelyBeyondfor factory-backed pools, just so the two are consistent.