add credential caching#154
Conversation
Testing so far is [a] confirming that existing behavior works, and [b] manually checking call counts to oath2. This works via: ``` AZURE_LOG_LEVEL=verbose <run-test> 2>&1 | tee test.log grep -c test.log ``` Specifically, spn_auth.test drops from 3 -> 1 oauth2 calls, and across the entire test suite (non-slow), oauth2 calls drop from 260 -> 26. So it's definitely working within context. Additionally tested credential invalidations when SECRET replaced, confirmed that the SECRET forces the minting of a new token.
samansmink
left a comment
samansmink
left a comment
There was a problem hiding this comment.
Hey @benfleis! thanks for the PR! this looks really good, the results sound really nice. I have added some comments
| auto transport_options = GetTransportOptions(opener, secret); | ||
| auto mi_cred = GetManagedIdentityCredential(opener, secret, transport_options); | ||
|
|
||
| auto fp = SecretFingerprint(secret); |
There was a problem hiding this comment.
This feels like it could be DRY-d up a bit
There was a problem hiding this comment.
It was on the edge for me whether to template a function to handle this... felt just below the line. I'll try one and see whether it helps (enough).
| static std::string SecretFingerprint(const KeyValueSecret &secret) { | ||
| std::string fp; | ||
| // NOTE: this key ordering is safe because secret_map is an ordered tree | ||
| for (const auto &kv : secret.secret_map) { |
There was a problem hiding this comment.
this will use potentially sensitive data (e.g. token) as a fingerprint that is now stored in the clientcontext state. I feel this would make it quite easy to lose track of things and accidentially have this sensitive data be printed to the screen or otherwise accidentally leaked somewhere.
There was a problem hiding this comment.
My thinking was less paranoid here, but I can also easily hash it and call it a day.
| @@ -388,8 +432,14 @@ static Azure::Storage::Blobs::BlobServiceClient | |||
| GetBlobStorageAccountClientFromCredentialChainProvider(optional_ptr<FileOpener> opener, const KeyValueSecret &secret, | |||
| const AzureParsedUrl &azure_parsed_url) { | |||
| auto transport_options = GetTransportOptions(opener, secret); | |||
There was a problem hiding this comment.
the transport options fetch both from a secret and settings. Since you only use the fingerprint based on the secret, this would now result in stale caching potentially
There was a problem hiding this comment.
ay ay ay, good eye! I'll fix this.
- rework caching/invalidation a bit - add CACHE_TOKEN_CREDENTIAL flag to disable for e.g. `az account logout` - DRY up cache handling
benfleis
changed the title
|
This would be really useful for us. Can I do anything to help move it along? |
|
Thanks @jbunting , good to know. Nothing atm; it's on my backlog to finish but not forgotten! |
3 participants
Cache token credentials (by default), so that they endure between queries.
Add
CACHE_TOKEN_CREDENTIALflag (default=true) to SECRET creation, to opt-out in unusual cases, e.g. expectingaz account logoutto invalidate cache.Testing so far is [a] confirming that existing behavior works, and [b] manually checking call counts to oauth2. This works via:
Specifically:
So it's definitely working within context.
Additionally tested credential invalidations when SECRET replaced, confirmed that the SECRET forces the minting of a new token.
If/when I get around to supporting httplib, I can use http logs to automate these tests