Should not be able to get key vault secret using bad credentials.

[dsteink2]

Describe the bug
First, I use the correct credentials to obtain the secret from the key vault.
Next, within the same program execution, I change the client secret to an incorrect value.
Finally, when I attempt to obtain the secret from the key vault with the incorrect credentials, I am able to obtain the secret. This should fail with 401 - unauthorized error.
NOTE: when using SDK version 12.5.0, this bug is not present.
When using latest git clone, the bug appears.
The latest commit in this version is 13213977b3f7089e8595ff116b890688bee0dacf
This new issue seems to be caused by pull request #4024

Exception or Stack Trace
no exception

To Reproduce
Steps to reproduce the behavior:
Compile the enclosed sample program.
Set the environment variables to match your key vault secret name, path and credentials.
Run the program.

Code Snippet
This code snippet is a modified version of the sample provided with the SDK:
sdk/keyvault/azure-security-keyvault-secrets/test/samples/sample1-basic-operations
(I added the ".txt" suffix to the file since github wouldn't let me upload a ".cpp" file, remove the ".txt" suffix before compiling.)

sample1_basic_operations.cpp.txt

Expected behavior
The attempt to access the key vault should fail with 401-unauthorized error when using incorrect client secret.

Screenshots
If applicable, add screenshots to help explain your problem.
sdt56068_________ ./sample1-basic-operations
Using good client secret, Secret is returned with name extfsazureconnector and value ?sv=2019-12-12&ss=b&srt=sco&sp=rwdlacx&se=2023-02-13T08:43:10Z&st=2021-01-13T00:43:10Z&spr=https&sig=A6wQT7yGR%2BSrbUTIDiDupEhkDLbDx3wv54LCAYA%2BUbw%3D
Using bad client secret, Secret is returned with name extfsazureconnector and value ?sv=2019-12-12&ss=b&srt=sco&sp=rwdlacx&se=2023-02-13T08:43:10Z&st=2021-01-13T00:43:10Z&spr=https&sig=A6wQT7yGR%2BSrbUTIDiDupEhkDLbDx3wv54LCAYA%2BUbw%3D

Setup (please complete the following information):

• OS: [e.g. iOS]
  Suse Linux SLES15, service pack 4
• IDE : [e.g. IntelliJ]
  Command line build using vcpkg
  GCC g++ (SUSE Linux) 7.5.0
  No IDE used

sdt56068_________ make
[ 59%] Built target azure-core
[ 72%] Built target azure-security-keyvault-secrets
[ 77%] Built target get-env-helper
[ 95%] Built target azure-identity
Consolidate compiler generated dependencies of target sample1-basic-operations
[ 95%] Building CXX object sdk/keyvault/azure-security-keyvault-secrets/test/samples/sample1-basic-operations/CMakeFiles/sample1-basic-operations.dir/sample1_basic_operations.cpp.o
[100%] Linking CXX executable sample1-basic-operations
[100%] Built target sample1-basic-operations

• Version of the Library used
  git clone from recent repo.
  most recent commit in this clone is 13213977b3f7089e8595ff116b890688bee0dacf

Additional context
This code used to work as expected when we were using SDK version 12.5.0
Due to a critical bug in this version, we started using a git clone version to continue our development.
This new issue seems to be caused by pull request #4024
If there are additional steps I need to take to flush the token cache, please provide them.

Information Checklist
Kindly make sure that you have added all the following information above and checkoff the required fields otherwise we will treat the issuer as an incomplete report

• Bug Description Added
• Repro Steps Added
• Setup information Added

[antkmsft]

This is a result of token caching (#4024). Until token expires, its valid value, once obtained successfully, will get reused for the same combination of TenenatId+ClientId+Scope until the token expires. Other SDKs (.NET SDK, for example) have the same behavior.

[antkmsft]

For your scenario to keep working, and working more reliably, you can remove the service principal from the key vault's access control, or to put it to deny access list. Without that, an old token still can be used to access it, even if it was tricky to reuse the old token programmatically.

Before caching was implemented, it used to be that the token could not be obtained. But should someone have the old token, they could still access it.

So depending how you use it, it really was not a secure way to be absolutely sure no client can access key vault - for example, once you initialized one client, and made a successful request to the key vault, the token was being cached inside that one client, so it could access keyvault while others could not (because of the fact they could not obtain the token, not because the service principal lost its permission).

If you make this work via managing keyvault permissions and not via managing service principal secrets, you can truly be sure that from the moment that changes got applied, no client that was using that principal is able to access the key vault. You'll write the same logic using Azure SDK for another language, and you'll get the same behavior, without relying on an implementation detail of older C++ SDK versions.

[antkmsft]

@dsteink2, I looked at your code sample, and you are right, in another case it does not work this way on other SDK.
I am referring to case when you create two credentials one after another, and the second one that gets used has the wrong secret.

In that case, it would work in other SDKs as I described (won't error on invalid secret), but only if you set TokenCredentialOptions.TokenCachePersistenceOptions. Otherwise, token cache would not be used and therefore the credential would attempt to get the token each time, without looking into cache.

I will add that option to the next beta. Thank you.

[antkmsft]

In other words, other SDKs do have per-credential-instance in-proc cache, and shared-between-credential-instances persistent cache.
When you delete secret, but keep using the same credential, it keeps working because per-credential-instance in-proc cache is used.
If you however are using a credential with very same creation parameters - TenantID, ClientID etc, they both have their own per-credential-instance caches, so if a second credential has bad client secret, GetToken() does fail.
That's unless you set TokenCredentialOptions.TokenCachePersistenceOptions. Then the behavior is like the one we have in C++ Identity 1.4.0-beta.2 (except that the cache is not an in-proc one, but it persistent and is shared between processes).

C++ Identity 1.4.0-beta.2 has it in the middle: it has shared-between-credential-instances in-proc cache, but requires no option being set to enable this behavior.

I think we should make per-credential-instance in-proc cache, and completely remove shared-between-credential-instances in-proc cache. Later, if there is a request to add it, we'll re-add the cache from 1.4.0-beta.2, but add an option to TokenCredentialOptions that is disabled by default and needs to be enabled in order for cache to work. We can even add named caches if needed (configurable via TokenCredentialOptions- the same way as in another language SDKs).

[antkmsft]

@Jinming-Hu, in context of the usage scenario described in #3835, would making per-credential-instance in-proc cache, and completely removing shared-between-credential-instances in-proc cache still be helpful?
If user reuses the same credential between multiple clients, they would still send only one auth request per credential instance across all storage clients. But if user created multiple credential instances, even if they have the same TenantId, ClientId, and ClientSecret, and basically gave each storage client its own credential instance, then there will be multiple requests. Sounds good?

Or would you right away want to keep the current shared-between-credential-instances in-proc cache from 1.4.0-beta.2 (but make it disabled by default, and let users enable it via TokenCredentialOptions)?

[Jinming-Hu]

If user reuses the same credential between multiple clients, they would still send only one auth request per credential instance across all storage clients. But if user created multiple credential instances, even if they have the same TenantId, ClientId, and ClientSecret, and basically gave each storage client its own credential instance, then there will be multiple requests. Sounds good?

This sounds to me a breaking change.
I'm curious, what's the value of such a change? What is the problem we're trying to solve?

If a user provides a wrong secret, the authorization still works just because he ever provided the right secret and token cache hasn't expired yet. Token-cache is by-design behavior. I don't see a problem here.

why do we think per-credential-instance cache is better than shared-between-credential-instances cache?

[antkmsft]

It won't be a bad kind of a breaking change since so far we've only released token cache in beta.

The reasons I'd like to change it:
It does not work exactly this way in other language SDKs.
If you delete the secret but the credential still works, it's ok. It actually works this way in other SDKs.
But this case, I see how this might look a bit unexpected to some:

auto cred1 = std::make_shared<ClientSecretCredential>(
    getenv("AZURE_TENANT_ID"),
    getenv("AZURE_CLIENT_ID"),
    getenv("AZURE_CLIENT_SECRET"));

Client client1(..., cred1);
auto result1 = client1.GetSomething(...).Value; // Works OK

auto cred2 = std::make_shared<ClientSecretCredential>(
    getenv("AZURE_TENANT_ID"),
    getenv("AZURE_CLIENT_ID"),
    "BAD");

Client client2(..., cred2);
auto result2 = client2.GetSomething(...).Value; // Also works OK, may be surprising to some.

It may also look confusing if your code happens to call client2.GetSecret() before client1.GetSecret():
client2.GetSecret() only succeeds if client1.GetSecret() was called before. Good thing if your code is linear and simple, but if you initialize in one place, and invoke in the other, there could be hard to find or hard to repro errors. Some of them may have wrong client not reveal themselves until the order changes.

auto result1 = client1.GetSomething(...).Value; // OK
auto result2 = client2.GetSomething(...).Value; // OK

auto result2 = client2.GetSomething(...).Value; // Fails
auto result1 = client1.GetSomething(...).Value; // OK

In contrast to that: customer has problem that too many auth requests are being sent. To solve it, they reuse the same credential (if not already). Sounds pretty natural and intuitive. Credential do even today get passed everywhere as shared_ptrs:

auto cred1 = std::make_shared<ClientSecretCredential>(
    getenv("AZURE_TENANT_ID"),
    getenv("AZURE_CLIENT_ID"),
    getenv("AZURE_CLIENT_SECRET"));

Client client1(..., cred1);
auto result1 = client1.GetSomething(...).Value; // Works OK

auto cred2 = std::make_shared<ClientSecretCredential>(
    getenv("AZURE_TENANT_ID"),
    getenv("AZURE_CLIENT_ID"),
    "BAD");

Client client2(..., cred2);
auto result2 = client2.GetSomething(...).Value; // Fails, and it is easy to understand, because the secret was wrong.

Client client3(..., cred1);
auto result3 = client3.GetSomething(...).Value; // Still OK, but it clearly uses cred1 and not cred2, so not surprising.
// ^^^^^ will succeed even if at this time you deleted ClientSecret in the Azure Portal (unlike azure-identity 1.3.0)

And unlike the behavior before token cache got implemented, auth request does not get sent during client3.GetSomething(...) gets invoked: the token that was obtained during client1.GetSomething(...) call gets reused.

So the question is the following - are you fine with the sample with cred1-2-3 above, or do you think we need to immediately additionally implement the following:

TokenCredentialOptions opts;
opts.UseInProcTokenCache = true; // details not finalized, the general idea is shown

auto cred1 = std::make_shared<ClientSecretCredential>(
    getenv("AZURE_TENANT_ID"),
    getenv("AZURE_CLIENT_ID"),
    getenv("AZURE_CLIENT_SECRET"),
    opts);

Client client1(..., cred1);
auto result1 = client1.GetSomething(...).Value; // Works OK

auto cred2 = std::make_shared<ClientSecretCredential>(
    getenv("AZURE_TENANT_ID"),
    getenv("AZURE_CLIENT_ID"),
    "BAD",
    opts);

Client client2(..., cred2);
auto result2 = client2.GetSomething(...).Value; // Also works OK, but since we explicitly set the token cache option, could be less surprising.

If we go one more step further, we would implement named caches. We don't have to implement it now unless we think there's a customer who needs it. Based on what other SDKs have, it would look something like this:

TokenCredentialOptions optsA;
optsA.TokenCache = "CacheA";

TokenCredentialOptions optsB;
optsB.TokenCache = "CacheB";

cred1A = ...make_shared<...Credential>(..., optsA);
cred2A = ...make_shared<...Credential>(..., optsA);
cred3A = ...make_shared<...Credential>(..., optsA);

cred1B = ...make_shared<...Credential>(..., optsB);
cred2B = ...make_shared<...Credential>(..., optsB);
cred3B = ...make_shared<...Credential>(..., optsB);

// Group of credentials 1A, 2A, 3A do share one cache, while 1B, 2B, 3B share another one.

[Jinming-Hu]

I kind of don't like the idea of TokenCredentialOptions.UseInProcTokenCache or TokenCredentialOptions.TokenCachePersistenceOptions. I feel we're making things overcomplicated.

I'm fine with either per-credential-instance cache or shared-between-credential-instances cache, but not both. I prefer keeping things simple.

The change in #3835 is still in beta right? If so, you still have the chance to make some changes.

[antkmsft]

Yes, we released shared-between-credential-instances cache, but only as beta - in 1.4.0-beta.2 (November release). In December release, 1.4.0-beta.3, we'll change it to be per-credential-instance.

I agree with keeping things simple, so I'm glad to hear from you that there is no immediate need to have shared-between-credential-instances cache, I'm happy to not have it. If it is ever needed, it will be hidden behind options as advanced feature. I agree, it would be a complication, but other SDKs have it this way as well. But it's a good thing we don't need it at this moment - maybe we'll never need it.

[dsteink2]

I also agree with keeping things simple. I don't have a preference about how the token caching works, but I believe it seems odd that I can give the correct credentials and access the key vault, then give incorrect credentials and still gain access. I suppose it depends upon the security model being used by the application. Since I've been recently assigned to work on this project, I am still learning about the Azure identity code and how our security model works. Sorry I can't be of more help...