Add support for disabling instance discovery for AzureStack scenarios

[sarathys]

Add support for disabling instance discovery for AzureStack and Custom Cloud scenarios

[chlowell]

Do you have tests for this?

[bgavrilMS]

I think 2 extra things need to be checked:

1. HomeAccountId property of Account objects from should be in format "oid.oid" (for AAD it's "oid.tid" but we don't have "tid" in ADFS). This is pretty important, otherwise other MSALs might not be able to read from the cache.
2. Account object (which is populated from the id token) should have the username set. I believe ADFS will expose this in the "upn" claim and AAD in the "preferred_username" claim. IMO, I would set Username to idToken.preferred_username ?? idToken.upn

[bgavrilMS]

CC @chlowell @rayluo

[jonnguyen1]

We are setting HomeAccountID like this currently. Do we have to make changes to this? And is this something that should go through in this pr?

// HomeAccountID creates the home account ID.
func (c ClientInfo) HomeAccountID() string {
if c.UID == "" || c.UTID == "" {
return ""
}
return fmt.Sprintf("%s.%s", c.UID, c.UTID)
}

[bgavrilMS]

I think so, otherwise you risk introducing a security issue or a cache miss issue for ADFS only. If home account Id is "", MSAL might not be able to retrieve the correct token from the cache (or it might not be able to retrieve one at all).

if think the logic is:

• if both are emtpy, homeAccountId is empty (this is probably for client_credentials)
• if c.UTID == "", then use fmt.Sprintf("%s.%s", c.UID, c.UID)

[rayluo]

2 extra things need to be checked:

• HomeAccountId property of Account objects from should be in format "oid.oid" (for AAD it's "oid.tid" but we don't have "tid" in ADFS). This is pretty important, otherwise other MSALs might not be able to read from the cache.

AFAIK, this is an undefined corner case in the original Token Cache Schema. Existing MSALs probably already have inconsistent behaviors here, ranging from "oid"-only to "oid.adfs". The effect is indeed different MSALs would not hit the same account in the cache. Not sure whether it is prevalent in the wild. We did not hear report on this. It is ADFS, anyway.

But if MSAL Go currently returns a completely empty string as HomeAccountID, that sounds too extreme. Better still return something, at least the oid.

• Account object (which is populated from the id token) should have the username set. I believe ADFS will expose this in the "upn" claim and AAD in the "preferred_username" claim. IMO, I would set Username to idToken.preferred_username ?? idToken.upn

Agreed. MSAL Python did it the same way, too.

And is this something that should go through in this pr?

Not necessarily in this PR, but if it would break Azure Stack scenarios, we may need to fix it sooner or later.

[jonnguyen1]

Then there would be cases where we would have UID.UID in AAD, when we are currently getting "" right? Does this pose some type of security issue for AAD? If so, sounds like we should distinguish between ADFS and AAD in the function/make a function for each auth type.

[bgavrilMS]

In AAD, the client info has uid and utid claims. In ADFS, it only has uid claim. You don't need to know if authority is ADFS to see that the client_info does not have utid claim.

Just avoid setting HomeAccountId to "" if the uid claim is present. This is a potential security issue, because the cache logic will not be able to differentiate between users.

[rayluo]

2 extra things need to be checked:

• HomeAccountId property of Account objects from should be in format "oid.oid" (for AAD it's "oid.tid" but we don't have "tid" in ADFS). This is pretty important, otherwise other MSALs might not be able to read from the cache.

AFAIK, this is an undefined corner case in the original Token Cache Schema. Existing MSALs probably already have inconsistent behaviors here, ranging from "oid"-only to "oid.adfs". The effect is indeed different MSALs would not hit the same account in the cache. Not sure whether it is prevalent in the wild. We did not hear report on this. It is ADFS, anyway.

But if MSAL Go currently returns a completely empty string as HomeAccountID, that sounds too extreme. Better still return something, at least the oid.

• Account object (which is populated from the id token) should have the username set. I believe ADFS will expose this in the "upn" claim and AAD in the "preferred_username" claim. IMO, I would set Username to idToken.preferred_username ?? idToken.upn

Agreed. MSAL Python did it the same way, too.

And is this something that should go through in this pr?

Not necessarily in this PR, but if it would break Azure Stack scenarios, we may need to fix it sooner or later.

[chlowell]

Just a minor nitpick--including the expected and actual values in failure messages helps future debuggers figure out what went wrong:

Suggested change

	if account.HomeAccountID != "123-456.123-456" {
	t.Fatal("Unexpected Account.HomeAccountId")
	}
	if actual := account.HomeAccountID; actual != "123-456.123-456" {
	t.Fatalf("expected %q, got %q", "123-456.123-456", actual)
	}

(tidier if the expected value comes from a variable instead of a literal)

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information