[Identity] Adding regional STS support #15778

sadasant · 2021-06-16T20:01:31Z

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.

sadasant · 2021-06-16T20:06:13Z

sdk/identity/identity/package.json

@@ -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\"",


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.

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.

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

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.

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?

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

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

xirzec

Looks nice and clean to me. Some minor comments, but I defer to @schaabs for the final naming/shape of things.

xirzec · 2021-06-16T20:11:56Z

sdk/identity/identity/src/msal/nodeFlows/nodeCommon.ts


  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;


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.

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.

sdk/identity/identity/src/regionalAuthority.ts

xirzec · 2021-06-16T20:14:03Z

sdk/identity/identity/package.json

@@ -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\"",


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.

schaabs · 2021-06-17T16:18:12Z

sdk/identity/identity/review/identity.api.md

+    AustraliaCentralValue = "australiacentral",
+    AustraliaEastValue = "australiaeast",
+    AustraliaSouthEastValue = "australiasoutheast",
+    AutoDiscoverRegion = "AUTO_DISCOVER",


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.

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.

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

sdk/identity/identity/src/credentials/clientCertificateCredentialOptions.ts

sdk/identity/identity/src/credentials/clientSecretCredentialOptions.ts

sdk/identity/identity/test/public/node/clientSecretCredential.spec.ts

schaabs

Just a couple of minor comments. Otherwise, looks great!

…ialOptions.ts Co-authored-by: Scott Schaab <[email protected]>

…tions.ts Co-authored-by: Scott Schaab <[email protected]>

witemple-msft

Couple of nits.

sdk/identity/identity/review/identity.api.md

sdk/identity/identity/src/msal/nodeFlows/nodeCommon.ts

Co-authored-by: Will Temple <[email protected]>

ghost · 2021-06-18T00:34:55Z

Hello @sadasant!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Microsoft.SecurityInsights 2021-09-01-preview (Azure#16933) * Adds base for updating Microsoft.SecurityInsights from version preview/2021-03-01-preview to version 2021-09-01-preview * Updates readme * Updates API version in new specs and examples * Microsoft.security insights 2021 09 01 preview add missing resources (Azure#15531) * Copy missing resources specs and examples from 2019-01-01-preview * Update added resources specs and examples and extract common types * Update readme * Extract ClientInfo, UserInfo and Lable to common types * Fix SpellCheck and Avocado * Return ThreatIntelligence to readme * Fix broken refs in Watchlists * Resolve duplicate schema errors * Run prettier * Make common types prettier * Add required property to operations according to ARM requirments * Fix readme * Add file separators to readme * Rename example file * Supress OBJECT_ADDITIONAL_PROPERTIES * Add 'where' to OBJECT_ADDITIONAL_PROPERTIES supression * Move OBJECT_ADDITIONAL_PROPERTIES supression under general Supression section. * Copy dataConnectors from 2021-03-01-preview * Update version of dataConnectors (this was done as there were errors when trying to generate C# client. Copying and changing version again fixed the problem). * Add dataConnectorsCheckRequirments path, parameters and definitions from 2019-01-01-preveiw Co-authored-by: Anat Gilenson <[email protected]> * Use newest common types in new 2021-09-01-preview API version (Azure#15778) * Use newest common types in AlertRules * Use newest common types in AutomationRules * Use newest common types in Bookmarks * Use newest common types in dataConnectors * Use newest common types in Enrichment * Use newest common types in Entities * Use newest common types in EntityQueries * Use newest common types in Incidents * Use newest common types in Metadata * Use newest common types in OfficeConsents * Use newest common types in OnboardingStates * Use newest common types in operations * Use newest common types in Settings * Use newest common types in SourceControls * Use newest common types in ThreatIntelligence * Use newest common types in Watchlist * Use newest common types in EntityTypes * Use newest common types in RelationTypes * Fix ThreatIntelligence Co-authored-by: Anat Gilenson <[email protected]> * Add template version to the scheduled alert rule + scheduled template (Azure#15919) * Add template version to the scheduled alert rule * Update AlertRules.json * Update AlertRules.json * Update AlertRules.json * Update AlertRules.json * Update GetAlertRuleTemplates.json * Update GetAlertRuleTemplateById.json * add aws s3 connector (Azure#15844) * Add a new kind of alert rules - NRT (Azure#15980) * add NRT rule * add NRT rule * add NRT rule * add NRT rule * fix typo * fix typo * fix * Align new Metadata feature with 2021-03-01-preview (Azure#16304) Co-authored-by: Anat Gilenson <[email protected]> * Add fixes from 2021-03-01-preview (Azure#16238) Co-authored-by: Anat Gilenson <[email protected]> * Add entity query templates (Azure#16269) * Add entity query templates from 2021-03-01-preview * Update version * Use newest common types and update readme * Fix conflicting common types Co-authored-by: Anat Gilenson <[email protected]> * Fix bookmark relations operatinIds to be consistent with other operationIds. (Azure#16519) Co-authored-by: Anat Gilenson <[email protected]> * Add corrections from 2021-03-01-preview (Azure#16490) Co-authored-by: Anat Gilenson <[email protected]> * Remove unused parameters (Azure#16619) Co-authored-by: Anat Gilenson <[email protected]> * Update readme default readme tag for client generation (Azure#16620) Co-authored-by: Anat Gilenson <[email protected]> * Use CloudError instead of ErrorResponse to avoid breaking change (Azure#16691) Co-authored-by: Anat Gilenson <[email protected]> * Add data connectors polling ccp api support (Azure#16293) * adding dataConnectors polling CCP api Support. (witout tests validations) * azure sentinel dataconnectors update examples * azure sentinel dataConnectors examples update and fix * azure sentinel dataConnectors prettier * azure sentinel dataConnectors add connect disconnect examples update path * azure sentinel dataConnectors add connect disconnect examples fix * azure sentinel dataConnectors add connect disconnect examples fix 2 * azure sentinel dataConnectors rebase dataConnectors dev * azure sentinel dataconnectors - fix put to post on connect and disconnect endpoints * azure sentinel dataconnectors - adding x-ms-secret to password on connect * azure sentinel dataconnectors - connect/disconnect endpoint remove unnedded 201 return * azure sentinel dataConnectors - remove empty body DataConnectorDisconnectBody Co-authored-by: Alon Danoch <[email protected]> * Add office IRM Connector (Azure#16764) * Add office IRM * fix * fix * fix * fix Co-authored-by: omerhaimov <[email protected]> * Add teamInformation to IncidentProperties 2021-09-01-preview (Azure#16787) * Fix Swagger for SecurityInsights - Add teamInformation to IncidentProperties * Try change description as advised by Swagger reviewer Yuchao Yan to fix the validation error. * Revert change in ntDomain description as it has nothing to do with validations Co-authored-by: Anat Gilenson <[email protected]> * Make CloudError and CloudErrorBody external resources (already exist under Microsoft.Rest.Azure namespace) (Azure#16872) Co-authored-by: Anat Gilenson <[email protected]> * Remove operational insights parameter 2021 09 01 preview (Azure#16891) * Remove operationalInsightsResourceProvider parameter from specs * Remove parameter from examples Co-authored-by: Anat Gilenson <[email protected]> * Update EntityTypes.json (Azure#16972) Co-authored-by: Anat Gilenson <[email protected]> Co-authored-by: Amit Bergman <[email protected]> Co-authored-by: sagamzu <[email protected]> Co-authored-by: necoh <[email protected]> Co-authored-by: alondanoch <[email protected]> Co-authored-by: Alon Danoch <[email protected]> Co-authored-by: omerhaimov <[email protected]> Co-authored-by: omerhaimov <[email protected]> Co-authored-by: Yuchao Yan <[email protected]>

sadasant requested review from schaabs and witemple-msft June 16, 2021 20:01

sadasant requested review from bterlson, chradek, deyaaeldeen, mikeharder, praveenkuttappan and xirzec as code owners June 16, 2021 20:01

ghost added the Azure.Identity label Jun 16, 2021

sadasant added 3 commits June 16, 2021 20:01

most of the code changes

4cd97eb

changes, before running record test

41991f6

tested and cleaned up

c8cf76e

sadasant force-pushed the identity/fix15762 branch from a8e90ca to c8cf76e Compare June 16, 2021 20:05

sadasant commented Jun 16, 2021

View reviewed changes

xirzec approved these changes Jun 16, 2021

View reviewed changes

better comments on the RegionalAuthority type

fcf5d5d

sadasant mentioned this pull request Jun 16, 2021

No way to configure azureRegion on the msal layer when using credentials with the identity library #15714

Closed

schaabs reviewed Jun 17, 2021

View reviewed changes

sdk/identity/identity/src/credentials/clientCertificateCredentialOptions.ts Outdated Show resolved Hide resolved

schaabs reviewed Jun 17, 2021

View reviewed changes

sdk/identity/identity/src/credentials/clientSecretCredentialOptions.ts Outdated Show resolved Hide resolved

schaabs reviewed Jun 17, 2021

View reviewed changes

sdk/identity/identity/test/public/node/clientSecretCredential.spec.ts Show resolved Hide resolved

schaabs approved these changes Jun 17, 2021

View reviewed changes

sadasant and others added 3 commits June 17, 2021 13:19

Update sdk/identity/identity/src/credentials/clientCertificateCredent…

f75bb74

…ialOptions.ts Co-authored-by: Scott Schaab <[email protected]>

Update sdk/identity/identity/src/credentials/clientSecretCredentialOp…

96f200b

…tions.ts Co-authored-by: Scott Schaab <[email protected]>

skipping regional authority public test on live mode

0b24db8

witemple-msft reviewed Jun 17, 2021

View reviewed changes

sdk/identity/identity/review/identity.api.md Outdated Show resolved Hide resolved

sdk/identity/identity/src/msal/nodeFlows/nodeCommon.ts Outdated Show resolved Hide resolved

witemple-msft approved these changes Jun 17, 2021

View reviewed changes

sadasant and others added 3 commits June 17, 2021 19:21

Update sdk/identity/identity/src/msal/nodeFlows/nodeCommon.ts

59d7fc3

Co-authored-by: Will Temple <[email protected]>

property name and standard value

158acd3

CHANGELOG clarification

76515f8

last remaining autoDiscoverRegion

81de984

sadasant added the auto-merge label Jun 18, 2021

ghost merged commit 32c780f into Azure:master Jun 18, 2021

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Identity] Adding regional STS support #15778

[Identity] Adding regional STS support #15778

sadasant commented Jun 16, 2021 •

edited

Loading

sadasant Jun 16, 2021

xirzec Jun 16, 2021

sadasant Jun 16, 2021

schaabs Jun 16, 2021

sadasant Jun 17, 2021

sadasant Jun 17, 2021

sadasant Jun 17, 2021

xirzec left a comment

xirzec Jun 16, 2021

schaabs Jun 17, 2021

xirzec Jun 16, 2021

schaabs Jun 17, 2021

sadasant Jun 17, 2021

sadasant Jun 17, 2021

schaabs left a comment

witemple-msft left a comment

ghost commented Jun 18, 2021

[Identity] Adding regional STS support #15778

[Identity] Adding regional STS support #15778

Conversation

sadasant commented Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xirzec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schaabs left a comment

Choose a reason for hiding this comment

witemple-msft left a comment

Choose a reason for hiding this comment

ghost commented Jun 18, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

sadasant commented Jun 16, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.