Latency from instance discovery calls while retrieving the token using certificate credential call

[daxakp]

In our PoC testing with AzIdentity for go (which in turns uses MSAL for go) we found that client secret/certificate credential call for retrieving the token almost double the time when compared with ADAL call for the same. The added latency is from the additional instance discovery calls.
Considering the recommendation from the SDK team to keep instance discovery enabled in public clouds,

1. Is the default added latency when the trusted authority verification is enabled expected and has been reviewed.
2. Are there any benchmarks or data available for performance and latency comparison with ADAL.
3. Is my understanding correct that disabling instance discovery is acceptable if the authority host is always known, for example, https://login.microsoftonline.com? Is it recommended to disable it for 1P service?
4. Once instance discovery is disabled, is there any case where that flag can be overwritten and instance discovery will still be performed (adding latency potentially)?
5. is ESTS-R still supported when instance discovery disabled? The code seems to override the disablement of instance discovery.

Do note that our service run in air gapped cloud. Instance discovery will not work.

[bgavrilMS]

Instance discovery is used for several purposes:

1. authority validation
2. traffic shaping via CNAME manipulation at SDK side (e.g. if you configure MSAL to use login.windows.net, MSAL will actually use login.microsoftonline.com)
3. elimination of SSO islands, because MSAL knows that a token for login.windows.net is the same as a token for login.microsoft.com.

Note that instance discovery should have been implemented in ADAL as well, it's not an MSAL specific issue.

There is a flag to disable instance discovery, see AzureAD/microsoft-authentication-library-for-go@dac2c13

To answer your questions:

1. yes, although several optimizations can be made (I logged this feature request on MSAL GO for opimizations)
2. MSAL should be as fast as ADAL, but do note that ADAL Go probably doesn't implement instance discovery.
3. No, for reasons 2 and 3 above
4. There is only 1 flag - AzureAD/microsoft-authentication-library-for-go@dac2c13
5. the code at which you point to is not related to instance discovery, but to OIDC discovery. These are different concepts.

Instance discovery
OIDC discovery

ESTS-R should work in non-public clouds, irrespective of instance discovery state.

6. If your service runs in airgapped cloud, pls disable instance discovery

[weinong]

Hi @bgavrilMS ,
Specifically, for Azure 1P resource provider, where we typically use client certificate/secret flow, what does instance discovery get us? We have been running without this for 6-7 years in ADAL. Besides, for consistency across clouds including airgapped, can we just diable it across the boards?

[bgavrilMS]

I agree that until we make the optimization in MSAL GO, disabling will have minimal impact for your client.

[daxakp]

Thank you @bgavrilMS
If we cache ClientCertificateCredential per tenant, will that cache need a refresh in any case?
Do we need to validate access tokens for staleness when retrieved using credential cache?

[bgavrilMS]

I don't know about the first sentence, I'll let @chlowell chime in.
The underlying library, MSAL.Go, guarantees to give you tokens that are at least 5 min fresh.

[chlowell]

If we cache ClientCertificateCredential per tenant, will that cache need a refresh in any case?

What cache? If you mean your cache of ClientCertificateCredential instances, you would need to replace any instance whose cert you roll. New instances would start with an empty authentication data cache.

Do we need to validate access tokens for staleness when retrieved using credential cache?

No

[github-actions]

Hi @daxakp. Thank you for opening this issue and giving us the opportunity to assist. We believe that this has been addressed. If you feel that further discussion is needed, please add a comment with the text "/unresolve" to remove the "issue-addressed" label and continue the conversation.

[github-actions]

Hi @daxakp, since you haven’t asked that we /unresolve the issue, we’ll close this out. If you believe further discussion is needed, please add a comment /unresolve to reopen the issue.