[Identity] Cleanups #18532

sadasant · 2021-11-04T14:30:26Z

This PR contains several cleanups on the Identity project. These include:

Moved the test/manual files to test/manual/interactive-browser-credential.
samples/manual to test/manual/authorization-code-credential.
TokenCredentialOptions to its own file.
ManagedIdentityCredential cleanups (mostly that I removed the msiGenericGetToken).
Very small cleanups on the credentials (mostly comments).
MSAL common files to start with "msal" (msalBrowserCommon, and msalNodeCommon), to make them easier to find. I always had a hard time finding them since they are named differently than the other files in those folders.
Removed any types from the internal implementation of the ManagedIdentityCredential.

This PR is part of the work mentioned in #17489 , but it won’t fully solve it.

sdk/identity/identity/test/manual/dist/index.html

witemple-msft

Some comments where I think extra explanation could be useful or where I think there's some extra code that doesn't necessarily need to be there.

If we're doing code cleanups, could we also just brush over all the uses of any in these files and find better types for them -- even if that's just unknown in some cases?

witemple-msft · 2021-11-05T15:55:53Z

sdk/identity/identity/src/credentials/managedIdentityCredential/appServiceMsi2017.ts


 const msiName = "ManagedIdentityCredential - AppServiceMSI 2017";
 const logger = credentialLogger(msiName);

+/**
+ * Formats the expiration date of the received token into the number of milliseconds between that date and midnight, January 1, 1970.
+ */
 function expiresInParser(requestBody: any): number {


This function is expiresIn but the property name is expires_on.

And moreover, do we even need this abstraction (especially given that we have an any type here)? Could we not just replace all calls to expiresInParser(x) with Date.parse(x.expires_on)?

Both expires_in and expires_on can exist in the payloads. Expires in meaning how much time is left, and expires on meaning at what time it will expire. We do this transformation on a case by case basis according to the target authentication environment. We try to narrow this down to the expiresOnTimestamp property, so I’ll change the expiresInParser functions to expiresOnParser since on average we use expires.[oO]n more often.

If it's case by case, then I'm just saying that this specific case ought to be expiresOn. I noticed that one of the other MSI instances checks expires_in, but not this one and not several others.

witemple-msft · 2021-11-05T16:00:40Z

sdk/identity/identity/src/credentials/managedIdentityCredential/fabricMsi.ts

 function expiresInParser(requestBody: any): number {
  // Parses a string representation of the seconds since epoch into a number value
  return Number(requestBody.expires_on);
 }


Same comment here as expiresInParser.

Something doesn't align here. The comment says "seconds since epoch" and the doc comment of the function says "milliseconds since". Which is the actual value and can we just do a parseInt where we need this function instead?

There’s a default expires on parser function. This case is different. The comment should say “milliseconds since”, so I’ll update that.

witemple-msft · 2021-11-05T16:02:53Z

sdk/identity/identity/src/credentials/managedIdentityCredential/fabricMsi.ts

+    request.agent = new https.Agent({
+      // This is necessary because Service Fabric provides a self-signed certificate.
+      // The alternative path is to verify the certificate using the IDENTITY_SERVER_THUMBPRINT env variable.
+      rejectUnauthorized: false
+    });


This makes me very nervous. Can we try to validate the service fabric cert instead? Is there some kind of technical reason that the cert is self-signed instead of using a real trusted cert?

Service fabric MSI certificates are self-signed since they’re only meant to be reached form the internal network. We have in our roadmaps to strengthen security of this authentication flow, however across languages we have agreed to skip the verification of the certificates. This is a local-network (cluster) endpoint, so it’s not as unsafe as a public domain endpoint, if that helps.

If that's what folks have agreed to do, then I suppose that's OK, but I really wish we could at least check the thumbprint against a list or something. If not, then I guess it's just a limitation of the environment, but still makes me nervous.

witemple-msft · 2021-11-05T16:04:40Z

sdk/identity/identity/src/credentials/managedIdentityCredential/imdsMsi.ts

 import { AuthenticationError } from "../../errors";

 const msiName = "ManagedIdentityCredential - IMDS";
 const logger = credentialLogger(msiName);

+/**
+ * Formats the expiration date of the received token into the number of milliseconds between that date and midnight, January 1, 1970.
+ */
 function expiresInParser(requestBody: any): number {


Why is this the only copy of this function where we validate the existence of the expires_on property? Also, can we give requestBody an accurate type here?

For sure! I’ll give requestBody a type! Thank you for pointing me to this. By this point it was invisible to me that there was an “any” type here.

Why is this the only copy of this function where we validate the existence of the expires_on property?

We assume that the other authentication environments will always provide an expires_on property.

In the cases where we don’t have a specific method, we always use expires_in, which should always be present.

witemple-msft · 2021-11-05T16:06:57Z

sdk/identity/identity/src/credentials/managedIdentityCredential/imdsMsi.ts

+        const request = createPipelineRequest({
+          abortSignal: getTokenOptions.abortSignal,
+          ...prepareRequestOptions(scopes, clientId),
+          allowInsecureConnection: true


I think we ought to document the exact reasoning for any settings like this where we disable an otherwise default security setting.

Most of the IMDS endpoints are http endpoints. The default one is http://169.254.169.254/, but environment variables can specify another endpoint. I will add a comment mentioning this.

sdk/identity/identity/src/credentials/managedIdentityCredential/index.ts

sdk/identity/identity/src/credentials/managedIdentityCredential/tokenExchangeMsi.ts

witemple-msft · 2021-11-05T16:14:59Z

sdk/identity/identity/src/credentials/managedIdentityCredential/tokenExchangeMsi.ts

+      const request = createPipelineRequest({
+        abortSignal: getTokenOptions.abortSignal,
+        ...prepareRequestOptions(scopes, assertion, clientId || process.env.AZURE_CLIENT_ID!),
+        allowInsecureConnection: true


Same comment about documenting why this is necessary.

IMDS endpoints are http, which are blocked by default by our pipelines.

witemple-msft

Every time we remove an any, an angel gets their wings.

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

…18289) * Initial Rest Client work for Synapse Access Control * Update ci pipeline * Update tests * Update pagination helper and test * Add samples * Fix Access Control rest * Update broken links * Fix format in agrifood

* Purview Administration * Purview Catalog * Purview Account * Purview scanning * Farmbeats * ConfidentialLedger * DocumentTranslator

Last PR some had 2.0.0 and some 2.0.1. Updating so all of them depend on ^2.0.1

Data was in the incorrect format, fixing it for release

)

* added autorest readme and generated files * rush.json modified and rush update-build ran for communication-short-codes. Imported samples and ran dev-tool for sample creation. * copied commincation-phone-numbers .gitignore into communication-short-codes * added README.md for short codes * fix @azure-tools reference * removed temporary fix for dist-esm and types files not generated in src subfolder. Previous commit from Erica fixed root issue. * modified sample to new changes (removed signup and fixed useCase) * changed createUSProgramBrief to upserUSProgramBrief in README.md - further polishing of readme probably needed * fixed wrong references to short codes package imports in readme.md * regenerated from latest swagger * regenerated from swagger from now merged PR. Added necessary modifications. Pending on a possible swagger mistake, that's why it is not building. * updated swagger, updated sdk, referencing latest commit in master for swagger. * built samples * Added compiling tests with meat but it is VERY RAW. Like the animal is still alive. * added autorest readme and generated files * rush.json modified and rush update-build ran for communication-short-codes. Imported samples and ran dev-tool for sample creation. * copied commincation-phone-numbers .gitignore into communication-short-codes * added README.md for short codes * fix @azure-tools reference * removed temporary fix for dist-esm and types files not generated in src subfolder. Previous commit from Erica fixed root issue. * modified sample to new changes (removed signup and fixed useCase) * changed createUSProgramBrief to upserUSProgramBrief in README.md - further polishing of readme probably needed * fixed wrong references to short codes package imports in readme.md * regenerated from latest swagger * regenerated from swagger from now merged PR. Added necessary modifications. Pending on a possible swagger mistake, that's why it is not building. * updated swagger, updated sdk, referencing latest commit in master for swagger. * built samples * Added compiling tests with meat but it is VERY RAW. Like the animal is still alive. * meat ready to cook * Regenerated with autorest from latest changes. Swagger now points to Rodolfo * updated sample from swagger * unit tests running and passing. unit tests with AAD failing. * updating tests to get env variables and splitting upsert in update & create * removed AAD tests * removed non-existing links to public documentation in sample readmes * added pnpm-lock.yml for short codes and removed unnecessary imports because of removed AAD tests. * reverted accidental change * added short codes to communication ci.yml * wrapping calls to the generated client in try catch finally blocks. automatic formatting * refactoring us program brief for better accessing and not to repeat data * changing let that should be const and unnecessary id check on test USPB retrieval. * grabbed pnpm-lock.yaml from azure/main and used rush update * missing pnpm-lock.yaml changes * fixing lint issues and building * run rushx format * fixes for recorded tests * running rushx format * added changelog * ran prepare release script * resolving comments from PR. Deleting unused variables, fixing references, renaming variables, updating changelog * run the format command * Update ci.yml * Update ci.yml * trying change from rest to tools * updating samples * added missing pnpm-lock.yaml update * updating release date Co-authored-by: Erica Sponsler <[email protected]> Co-authored-by: 0rlanand0Wats0n <[email protected]>

* [ai-form-recognizer] Lazy iterator for words of a line * Use method instead of property * Regenerate API * Polished, wrote changelog, added some more tests, samples * Updated API MD * Improved docs * Apply changes from review

* adding retry to docker container start Co-authored-by: scbedd <[email protected]>

This PR contains several cleanups on the Identity project. These include: - Moved the `test/manual` files to `test/manual/interactive-browser-credential`. - `samples/manual` to `test/manual/authorization-code-credential`. - `TokenCredentialOptions` to its own file. - `ManagedIdentityCredential` cleanups (mostly that I removed the `msiGenericGetToken`). - Very small cleanups on the credentials (mostly comments). - MSAL common files to start with "msal" (`msalBrowserCommon`, and `msalNodeCommon`), to make them easier to find. I always had a hard time finding them since they are named differently than the other files in those folders. - Removed `any` types from the internal implementation of the `ManagedIdentityCredential`. This PR is part of the work mentioned in #17489 , but it won’t fully solve it.

sadasant added 6 commits November 4, 2021 12:10

samples/manual to test/manual/authorization-code-credential

23df608

TokenCredentialOptions to its own file

944fcf7

ManagedIdentityCredential cleanups

002e389

very small cleanups on the credentials

a5d480d

msal common files to start with "msal", to make them easier to find

78fb37d

moved test/manual files to test/manual/interactive-browser-credential

e2641d7

sadasant requested review from KarishmaGhiya and witemple-msft November 4, 2021 14:30

sadasant self-assigned this Nov 4, 2021

sadasant requested review from deyaaeldeen and schaabs as code owners November 4, 2021 14:30

ghost added the Azure.Identity label Nov 4, 2021

sadasant added 2 commits November 4, 2021 19:36

fixing bad references on identity-cache-persistence

10ba7b9

formatting

4cdc4bf

sadasant mentioned this pull request Nov 4, 2021

[Identity] Quality improvements #17489

Closed

6 tasks

KarishmaGhiya reviewed Nov 4, 2021

View reviewed changes

sdk/identity/identity/test/manual/dist/index.html Show resolved Hide resolved

KarishmaGhiya approved these changes Nov 4, 2021

View reviewed changes

witemple-msft reviewed Nov 5, 2021

View reviewed changes

sadasant added 2 commits November 5, 2021 22:55

feedback from will

b5190e1

missed this one

73fd7a1

witemple-msft approved these changes Nov 5, 2021

View reviewed changes

sadasant enabled auto-merge (squash) November 5, 2021 23:31

witemple-msft and others added 8 commits November 6, 2021 02:36

[dev-tool] Added support for deep imports in samples (Azure#18511)

8386880

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

Prepare beta.8 release (Azure#18533)

8f5986a

Update @azure/identity dependency on REST Clients to v2 (Azure#18536)

7460bd1

* Purview Administration * Purview Catalog * Purview Account * Purview scanning * Farmbeats * ConfidentialLedger * DocumentTranslator

Post release automated changes for azure-rest-core-client (Azure#18538)

f3be3a9

Make identity dependencies consistent across RLCs (Azure#18539)

02e5bb8

Last PR some had 2.0.0 and some 2.0.1. Updating so all of them depend on ^2.0.1

[core-rest-pipeline] Fix Changelog date (Azure#18540)

7ffc110

Data was in the incorrect format, fixing it for release

Post release automated changes for azure-core-rest-pipeline (Azure#18541

acc939e

)

AlonsoMondal and others added 3 commits November 6, 2021 02:37

Sync eng/common directory with azure-sdk-tools for PR 2219 (Azure#18557)

f3c62b8

* adding retry to docker container start Co-authored-by: scbedd <[email protected]>

Merge remote-tracking branch 'Azure/main' into identity/cleanups

6e68f99

sadasant merged commit 746bb4f into Azure:main Nov 6, 2021

sadasant deleted the identity/cleanups branch November 6, 2021 14:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Identity] Cleanups #18532

[Identity] Cleanups #18532

sadasant commented Nov 4, 2021 •

edited

Loading

witemple-msft left a comment

witemple-msft Nov 5, 2021

sadasant Nov 5, 2021

witemple-msft Nov 5, 2021

witemple-msft Nov 5, 2021

sadasant Nov 5, 2021

witemple-msft Nov 5, 2021

sadasant Nov 5, 2021

witemple-msft Nov 5, 2021

witemple-msft Nov 5, 2021

sadasant Nov 5, 2021

sadasant Nov 5, 2021 •

edited

Loading

witemple-msft Nov 5, 2021

sadasant Nov 5, 2021

witemple-msft Nov 5, 2021

sadasant Nov 5, 2021

witemple-msft left a comment

[Identity] Cleanups #18532

[Identity] Cleanups #18532

Conversation

sadasant commented Nov 4, 2021 • edited Loading

witemple-msft 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

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

sadasant Nov 5, 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

witemple-msft left a comment

Choose a reason for hiding this comment

sadasant commented Nov 4, 2021 •

edited

Loading

sadasant Nov 5, 2021 •

edited

Loading