Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Identity] Adding regional STS support #15778

Merged
11 commits merged into from
Jun 18, 2021
8 changes: 8 additions & 0 deletions sdk/identity/identity/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,14 @@

- Removed the protected method `getAzureCliAccessToken` from the public API of the `AzureCliCredential`. While it will continue to be available as part of v1, we won't be supporting this method as part of v2's public API.

### New Features

- Added regional STS support to client credential types.
- Added the `RegionalAuthority` type, that allows specifying Azure regions.
- Added `regionalAuthority` property to `ClientSecretCredentialOptions` and `ClientCertificateCredentialOptions`.
- If instead of a region, `autoDiscoverRegion` is specified as the value for `regionalAuthority`, MSAL will be used to attempt to discover the region.
- A region can also be specified through the `AZURE_REGIONAL_AUTHORITY_NAME` environment variable.

## 2.0.0-beta.3 (2021-05-12)

### New features
Expand Down
7 changes: 4 additions & 3 deletions sdk/identity/identity/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@
"test:node": "npm run clean && npm run build:test && npm run unit-test:node && npm run integration-test:node",
"test": "npm run clean && npm run build:test && npm run unit-test && npm run integration-test",
"unit-test:browser": "karma start --single-run",
"unit-test:node": "mocha -r esm -r ts-node/register --reporter ../../../common/tools/mocha-multi-reporter.js --timeout 180000 --full-trace --exclude \"test/**/browser/**/*.spec.ts\" \"test/**/*.spec.ts\"",
"unit-test:node": "mocha -r esm -r ts-node/register --reporter ../../../common/tools/mocha-multi-reporter.js --timeout 300000 --full-trace --exclude \"test/**/browser/**/*.spec.ts\" \"test/**/*.spec.ts\"",
"unit-test:node:no-timeouts": "mocha -r esm -r ts-node/register --reporter ../../../common/tools/mocha-multi-reporter.js --no-timeouts --full-trace --exclude \"test/**/browser/**/*.spec.ts\" \"test/**/*.spec.ts\"",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve increased the timeout because the live regional test is very time consuming. I’ve added a no-timeouts line here to help testing. We have something similar on Key Vault.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is we can't fix the tests to be fast because it depends on MSAL and MSAL is slow, even for unit tests? In theory these shouldn't be unit tests, but live tests if they need external deps.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are live tests. On playback they are pretty fast. In the three tests I’m adding here, only one does finish. The one that finishes takes 27 seconds

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason these tests are taking so long is because the test is using the region RegionalAuthority.AutoDiscoverRegion. This results in MSAL trying to discover the region it is running in which it does, in part, by trying to query IMDS. I'm assuming this is likely timing out if you're running in a local environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are alternatives that may work that seem complicated in my mind. The alternative that seems simple is to remove the live test. We have two other tests that verify that the parameter is sent through MSAL, which should be enough. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I can record this test with a specific region, then only enable it for playback 🤔 I’ll do that for now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s not timing out though, the recordings show all 200s 🤔

"unit-test": "npm run unit-test:node && npm run unit-test:browser",
"docs": "typedoc --excludePrivate --excludeNotExported --excludeExternals --stripInternal --mode file --out ./dist/docs ./src"
},
Expand Down Expand Up @@ -81,8 +82,8 @@
"@azure/core-tracing": "1.0.0-preview.11",
"@azure/logger": "^1.0.0",
"@azure/abort-controller": "^1.0.0",
"@azure/msal-common": "^4.0.0",
"@azure/msal-node": "^1.0.2",
"@azure/msal-common": "^4.3.0",
"@azure/msal-node": "^1.1.0",
"@azure/msal-browser": "^2.0.0",
"@types/stoppable": "^1.1.0",
"events": "^3.0.0",
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

59 changes: 59 additions & 0 deletions sdk/identity/identity/review/identity.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ export class ClientCertificateCredential implements TokenCredential {

// @public
export interface ClientCertificateCredentialOptions extends TokenCredentialOptions {
regionalAuthority?: string;
sendCertificateChain?: boolean;
}

Expand All @@ -103,6 +104,7 @@ export class ClientSecretCredential implements TokenCredential {

// @public
export interface ClientSecretCredentialOptions extends TokenCredentialOptions {
regionalAuthority?: string;
}

// @public
Expand Down Expand Up @@ -210,6 +212,63 @@ export class ManagedIdentityCredential implements TokenCredential {
getToken(scopes: string | string[], options?: GetTokenOptions): Promise<AccessToken>;
}

// @public
export enum RegionalAuthority {
AsiaEastValue = "eastasia",
sadasant marked this conversation as resolved.
Show resolved Hide resolved
AsiaSouthEastValue = "southeastasia",
AustraliaCentral2Value = "australiacentral2",
AustraliaCentralValue = "australiacentral",
AustraliaEastValue = "australiaeast",
AustraliaSouthEastValue = "australiasoutheast",
AutoDiscoverRegion = "AUTO_DISCOVER",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's interesting that MSAL for node seems to use a different constant for this than .NET, which uses the string constant "TryAutoDetect" which it exposes as a constant AttemptRegionDiscovery. Does MSAL for node have a constant defined for this? If so I think we should use it to ensure we're always in sync with this value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t see it here: https://github.com/AzureAD/microsoft-authentication-library-for-js/blob/dev/lib/msal-node/docs/regional-authorities.md

I’ll ask the MSAL team.

What is the value of the AutoDiscoverRegion in .Net? I don’t quite understand it in the .NET’s PR. What would be the equivalent for JS on the public API perspective? I can hide the internals.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, I’ll use the value AutoDiscoverRegion for now.

BrazilSouthValue = "brazilsouth",
CanadaCentralValue = "canadacentral",
CanadaEastValue = "canadaeast",
ChinaEast2Value = "chinaeast2",
ChinaEastValue = "chinaeast",
ChinaNorth2Value = "chinanorth2",
ChinaNorthValue = "chinanorth",
EuropeNorthValue = "northeurope",
EuropeWestValue = "westeurope",
FranceCentralValue = "francecentral",
FranceSouthValue = "francesouth",
GermanyCentralValue = "germanycentral",
GermanyNorthEastValue = "germanynortheast",
GermanyNorthValue = "germanynorth",
GermanyWestCentralValue = "germanywestcentral",
GovernmentUSArizonaValue = "usgovarizona",
GovernmentUSDodCentralValue = "usdodcentral",
GovernmentUSDodEastValue = "usdodeast",
GovernmentUSIowaValue = "usgoviowa",
GovernmentUSTexasValue = "usgovtexas",
GovernmentUSVirginiaValue = "usgovvirginia",
IndiaCentralValue = "centralindia",
IndiaSouthValue = "southindia",
IndiaWestValue = "westindia",
JapanEastValue = "japaneast",
JapanWestValue = "japanwest",
KoreaCentralValue = "koreacentral",
KoreaSouthValue = "koreasouth",
NorwayEastValue = "norwayeast",
NorwayWestValue = "norwaywest",
SouthAfricaNorthValue = "southafricanorth",
SouthAfricaWestValue = "southafricawest",
SwitzerlandNorthValue = "switzerlandnorth",
SwitzerlandWestValue = "switzerlandwest",
UAECentralValue = "uaecentral",
UAENorthValue = "uaenorth",
UKSouthValue = "uksouth",
UKWestValue = "ukwest",
USCentralValue = "centralus",
USEast2Value = "eastus2",
USEastValue = "eastus",
USNorthCentralValue = "northcentralus",
USSouthCentralValue = "southcentralus",
USWest2Value = "westus2",
USWestCentralValue = "westcentralus",
USWestValue = "westus"
}

// @public
export function serializeAuthenticationRecord(record: AuthenticationRecord): string;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,10 @@ export interface ClientCertificateCredentialOptions extends TokenCredentialOptio
* Set this option to send base64 encoded public certificate in the client assertion header as an x5c claim
*/
sendCertificateChain?: boolean;
/**
* Specifies a regional authority. Please refer to the {@link RegionalAuthority} type for the accepted values.
* If {@link RegionalAuthority.AutoDiscoverRegion} is specified, we will try to discover the regional authority endpoint.
* If the property is not specified, the credential uses the global authority endpoint.
*/
regionalAuthority?: string;
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,11 @@ import { TokenCredentialOptions } from "../client/identityClient";
/**
* Optional parameters for the {@link ClientSecretCredential} class.
*/
export interface ClientSecretCredentialOptions extends TokenCredentialOptions {}
export interface ClientSecretCredentialOptions extends TokenCredentialOptions {
/**
* Specifies a regional authority. Please refer to the {@link RegionalAuthority} type for the accepted values.
* If {@link RegionalAuthority.AutoDiscoverRegion} is specified, we will try to discover the regional authority endpoint.
* If the property is not specified, the credential uses the global authority endpoint.
*/
regionalAuthority?: string;
}
1 change: 1 addition & 0 deletions sdk/identity/identity/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ export { AuthenticationRecord } from "./msal/types";
export { AuthenticationRequiredError } from "./msal/errors";
export { serializeAuthenticationRecord, deserializeAuthenticationRecord } from "./msal/utils";
export { TokenCredentialOptions } from "./client/identityClient";
export { RegionalAuthority } from "./regionalAuthority";
export { InteractiveCredentialOptions } from "./credentials/interactiveCredentialOptions";

export { ChainedTokenCredential } from "./credentials/chainedTokenCredential";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,8 @@ export class MsalClientCertificate extends MsalNode {
try {
const result = await this.confidentialApp!.acquireTokenByClientCredential({
scopes,
correlationId: options.correlationId
correlationId: options.correlationId,
azureRegion: this.azureRegion
});
// Even though we're providing the same default in memory persistence cache that we use for DeviceCodeCredential,
// The Client Credential flow does not return the account information from the authentication service,
Expand Down
3 changes: 2 additions & 1 deletion sdk/identity/identity/src/msal/nodeFlows/msalClientSecret.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ export class MsalClientSecret extends MsalNode {
try {
const result = await this.confidentialApp!.acquireTokenByClientCredential({
scopes,
correlationId: options.correlationId
correlationId: options.correlationId,
azureRegion: this.azureRegion
});
// The Client Credential flow does not return an account,
// so each time getToken gets called, we will have to acquire a new token through the service.
Expand Down
8 changes: 8 additions & 0 deletions sdk/identity/identity/src/msal/nodeFlows/nodeCommon.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,12 @@ import {
*/
export interface MsalNodeOptions extends MsalFlowOptions {
tokenCredentialOptions: TokenCredentialOptions;
/**
* Specifies a regional authority. Please refer to the {@link RegionalAuthority} type for the accepted values.
* If {@link RegionalAuthority.AutoDiscoverRegion} is specified, we will try to discover the regional authority endpoint.
* If the property is not specified, uses a non-regional authority endpoint.
*/
regionalAuthority?: string;
}

/**
Expand All @@ -45,11 +51,13 @@ export abstract class MsalNode extends MsalBaseUtilities implements MsalFlow {
protected clientId: string;
protected identityClient?: IdentityClient;
protected requiresConfidential: boolean = false;
protected azureRegion?: string;

constructor(options: MsalNodeOptions) {
super(options);
this.msalConfig = this.defaultNodeMsalConfig(options);
this.clientId = this.msalConfig.auth.clientId;
this.azureRegion = options.regionalAuthority || process.env.AZURE_REGIONAL_AUTHORITY_NAME;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like the right priority, but I wonder if the env variable should trump the auto discover setting or not. I don't have strong feelings about it though, since it's easiest to say the option passed always wins.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see your point about auto discover, however, I think we should stick to the convention that code configuration always wins as you put it. We follow this convention with all our other supported environment variables. Altering this behavior based of the value specified adds a lot of complexity without much benefit.

sadasant marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down
Loading