Persistence token cache for Mac & Linux with new MSAL ext by jianghaolu · Pull Request #9188 · Azure/azure-sdk-for-java

jianghaolu · 2020-03-18T07:29:11Z

Here lists the set of design decisions we need to make, to go from the MSAL shared token cache accessors to a user-friendly SharedTokenCacheCredential design that works seamlessly and effortlessly across platforms and environments.

User interface for credentials

scope

Q: What credentials are we supporting shared token cache?

A: All user account credentials and service principal credentials.

user account credentials (public client applications)

Q: Enable or disable reading from the shared token cache by default?

A: Disable.

Q: Enable or disable writing to the shared token cache by default?

A: Disable.

Q: What environment variable to specify which account if there are multiple?

A: AZURE_USERNAME.

service principal credentials (confidential client applications)

Q: Do we want to support token cache for this?

A: Yes.

Q: Enable or disable reading from the shared token cache by default?

A: Disable.

Q: Enable or disable writing from the shared token cache by default?

A: Disable.

Q: What environment variable to specify which service principal if there are multiple?

A: N/A – will be specified on the ConfidentialClientApplication

Q: If both AZURE_USERNAME and AZURE_CLIENT_ID are present, what does it do?

A: N/A

configurations

Q: Do we have one method to enable both read & write, or separate methods?

A: One method.

Q: Do we start writing to cache in SharedTokenCacheCredential now?

A: Yes

Q: Do we allow configuring where the token cache is stored?

A: No

operating system specifics

Q: Do we automatically fallback to unprotected file on Linux if Gnome keyring item is unavailable?

A: No. Allow through a property “allowUnencryptedCache”.

Shared token cache implementation

storage locations for user accounts

Q: Where to store the DPAPI encrypted file on Windows?

A: {user.home}\AppData\Local.IdentityService\msal.cache

Q: Where to store the secret item in Keychain on Mac?

A: Service = Microsoft.Developer.IdentityService, Account = MSALCache

Q: Where to store the secret item in Gnome Keyring?

A: Under default keyring, with item label “MSALCache”, and schema “msal.cache”.

Q: Where to store the secret item as an unprotected file on Linux?

A: $HOME/.IdentityService/msal.cache

(Document all these to be implementations and shouldn’t be taken as a hard dependency)

storage locations for service principals

Q: Do we merge the token cache data for service principals with the user accounts?

A: Hopefully no, to avoid service principals peeking into the user tokens

Q: If the answer is no for the above question, specify the locations for all questions in Article 2.1.

A: MSALConfidentialCache, msal.confidential.cache

Error handling & logging

exceptions

Q: What exceptions do we throw if DPAPI encrypted file cannot be found on Windows, or Keychain item is unavailable on Mac, or Gnome Keyring item is unavailable on Linux, or unprotected file is unavailable?

A: CredentialUnavailableException for SharedTokenCacheCredential, ClientAuthenticationException for others if enabled

Q: What exceptions do we throw if the file, or the Keychain/Keyring item is available, but cannot be parsed or error out for any other reasons?

A: Wrap them in ClientAuthenticationException / AuthenticationFailedException.

logging

Q: What information do we log in this process?

A: 1) What platform we are on and 2) if the token cache access succeeded or failed. No information of where the token cache is located will be logged.

for sure, 1) potentially in the future

transient errors

Q: Is there a plan to detect and retry transient errors, like a race condition that MSAL fails to prevent?

A: for now no.

sdk/identity/azure-identity/src/main/java/com/azure/identity/DefaultAzureCredential.java

jianghaolu · 2020-04-29T20:27:19Z

Thanks for the review Alan! I've addressed the comments.

sdk/identity/azure-identity/src/main/java/com/azure/identity/implementation/IdentityClient.java

conniey · 2020-04-30T23:55:19Z

sdk/identity/azure-identity/src/main/java/com/azure/identity/DeviceCodeCredential.java

            if (cachedToken.get() != null) {
-                return identityClient.authenticateWithUserRefreshToken(request, cachedToken.get())
+                return identityClient.authenticateWithMsalAccount(request, cachedToken.get().getAccount())
                    .onErrorResume(t -> Mono.empty());


Would you want this to resume on an error and return an empty Mono? I'd expect the error to be propagated downstream and allow downstream subscribers to do with that error what they will. In globalSetupAsync this is call is chained with .then(), so it would hide the error.

There are similar instances in other credentials below.

Yeah it should be probably be handled downstream - it's out of the scope of this PR but I can see if it can be done with minimal footprint

sdk/identity/azure-identity/src/main/java/com/azure/identity/implementation/IdentityClient.java

conniey · 2020-05-01T05:32:57Z

...identity/src/test/java/com/azure/identity/implementation/IdentityClientIntegrationTests.java

        Assert.assertNotNull(token.getExpiresAt());
        Assert.assertFalse(token.isExpired());
-        token = client.authenticateWithUserRefreshToken(new TokenRequestContext().addScopes("https://vault.azure.net/.default"), token).block();
+        token = client.authenticateWithMsalAccount(new TokenRequestContext().addScopes("https://vault.azure.net/.default"), token.getAccount()).block();


These tests could be using StepVerifier rather than block.

// Arrange var context = new TokenRequestContext().addScopes("https://vault.azure.net/.default"); // Act & Assert StepVerifier.create(client.authenticateWithMsalAccount(context, token.getAccount())) .assertNext(token -> { assertNotNull(token.getToken()); assertNotNull(token.getExpiresAt()); }) .verifyComplete();

This is out of scope - if this PR stays open next week I can fix these. But for now I'm planning to get some sleep and enjoy my day off 😸

jianghaolu · 2020-05-01T08:28:31Z

Connie's 2 feedback are out of scope of this PR. Shouldn't block this PR so I plan to fix in a separate PR.

sdk/identity/azure-identity/src/main/java/com/azure/identity/DeviceCodeCredentialBuilder.java

...ity/azure-identity/src/main/java/com/azure/identity/InteractiveBrowserCredentialBuilder.java

sdk/identity/azure-identity/src/main/java/com/azure/identity/implementation/IdentityClient.java

sdk/identity/azure-identity/src/main/java/com/azure/identity/SharedTokenCacheCredential.java

...ntity/azure-identity/src/main/java/com/azure/identity/UsernamePasswordCredentialBuilder.java

jianghaolu · 2020-05-02T06:25:29Z

Thanks for the comments. They are extremely helpful. However, some comments may be better suited as general issues for the Identity library for Java and should be considered out of scope for this PR.

I'd like to narrow down the comments to only applicable for the changes in this PR. For the others, thanks @g2vinay for opening separate issues.

…sistence

jianghaolu added 3 commits March 16, 2020 17:11

Depend on msal persistence extension

898a02f

Initial draft of shared token cache for Mac and Linux

132c01a

Clean up

3c03443

jianghaolu commented Mar 18, 2020

View reviewed changes

sdk/identity/azure-identity/src/main/java/com/azure/identity/DefaultAzureCredential.java Outdated Show resolved Hide resolved

jianghaolu commented Mar 18, 2020

View reviewed changes

sdk/identity/azure-identity/src/main/java/com/azure/identity/DefaultAzureCredential.java Outdated Show resolved Hide resolved

jianghaolu commented Mar 18, 2020

View reviewed changes

sdk/identity/azure-identity/src/main/java/com/azure/identity/DefaultAzureCredential.java Outdated Show resolved Hide resolved

jianghaolu commented Mar 18, 2020

View reviewed changes

sdk/identity/azure-identity/src/main/java/com/azure/identity/DefaultAzureCredential.java Outdated Show resolved Hide resolved

jianghaolu requested review from chlowell, g2vinay, joshfree, schaabs and srnagar March 18, 2020 07:37

jianghaolu added 12 commits March 18, 2020 14:18

Add msal ext to module-info

49acab7

Wrap MSAL error and fix module-info

f50d8b7

checkstyle

4e4c467

Fix spotbugs and naming

fca0133

Fix default azure credential test

571b44c

Add initial perf test

e2c5dc3

more perf testing work

dd341e7

Move default to identity client options

7cba579

Fix array access in identity client options

600fc6b

Fix libsecret on Windows

1c53b24

Remove shared token cache configurations

3539779

Clean up

80bb671

jianghaolu marked this pull request as ready for review April 7, 2020 22:05

jianghaolu requested review from JimSuplizio, danieljurek, mitchdenny, samvaity and vcolin7 as code owners April 7, 2020 22:05

jianghaolu added 2 commits April 29, 2020 13:30

Merge error message

806539a

CI says IdentityClient doesn't compile

066d413

chlowell mentioned this pull request Apr 29, 2020

Enable persistent cache for user credentials on Linux and macOS Azure/azure-sdk-for-python#11134

Closed

5 tasks

Checkstyle: indentation

6cc8c1b

g2vinay reviewed Apr 30, 2020

View reviewed changes

sdk/identity/azure-identity/src/main/java/com/azure/identity/implementation/IdentityClient.java Outdated Show resolved Hide resolved

g2vinay reviewed Apr 30, 2020

View reviewed changes

sdk/identity/azure-identity/src/main/java/com/azure/identity/implementation/IdentityClient.java Outdated Show resolved Hide resolved

Update shared token cache look up with master

d5c4662

conniey reviewed Apr 30, 2020

View reviewed changes

conniey reviewed May 1, 2020

View reviewed changes

conniey approved these changes May 1, 2020

View reviewed changes

Address Connie's feedback

0bf3f76