[identity] log warning for users trying to access cloud shell using user assigned identity #20226

KarishmaGhiya · 2022-02-05T02:27:35Z

Packages impacted by this PR

@Azure/identity

Issues associated with this PR

Fixes #20142

Describe the problem that is addressed by this PR

Since Azure Managed Identities aren't available in the Azure Cloud Shell, we log a warning for users that try to access cloud shell using user assigned identity.

I am moving the upgrading of version of identity to a separate PR - because of the issues it exposes with the identity package references in the identity plugin packages. - #20340

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?

Throwing an exception will tie the client side behaviour to Service side impl details, if they change impl on service side in future, our logic of throwing exceptions can break. Instead, we will do some logging here to warn users on known targeted platforms and add some details to docs too.

Checklists

Added impacted package name to the issue description
Added a changelog (if necessary)

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

check-enforcer · 2022-02-08T01:22:31Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run js - [service] - ci

sadasant · 2022-02-08T02:19:23Z

sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts

+      authDetails.error?.message,
+      "ManagedIdentityCredential: Authentication failed. Message No responses left."
+    );
+    assert.equal(authDetails.requests.length, 0);


This test is identical to other tests that we have in this file, except for having enabled the logs through setLogLevel. Let’s try something fun: In the cloudShellMsi, export the logger, then here use Sinon to spy the logger and see if the logger.warning function has been invoked. How does that sound?

witemple-msft · 2022-02-08T16:38:49Z

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

+    if (clientId) {
+      logger.warning(
+        `${msiName}: Unavailable. Azure Managed Identities aren't available in the Azure Cloud Shell.`
+      );
+      return false;
+    }


I'm a bit confused by this. I think my confusion comes from three intersecting concerns:

The warning is logged in the isAvailable test. Doesn't that mean this warning will pretty much always trigger if the user specifies a managedIdentityClientId when the ManagedIdentityCredential tries to cache its available MSI providers? I guess what I'm saying is that it feels weird to have a warning-level log that will happen any time you provide a client ID.

The warning says "managed identities aren't available" in the cloud shell, but really the problem is that you can't pick the assigned managed identity, as the issue title says, is that right? I think the warning message could mislead someone into thinking that no form of Managed Identity is supported in the cloud shell.

Previously, we just ignored this option, correct? So, what if the user had passed in the client ID of the identity that their cloud shell instance is actually assigned (is that possible)? We'd log a warning and return false now, even though the option was semantically correct, but ignored and appeared to work previously. Feels a bit like a breaking change.

CC @sadasant WDYT?

Doesn't that mean this warning will pretty much always trigger if the user specifies a managedIdentityClientId

Yep. However, SDK logs are not automatically logged. They’re only logged when they’re enabled.

warning-level log that will happen any time you provide a client ID

We have a bunch of info level logs which are far more pervasive. When I think of whether this should be an info level log or a warning level log, I think this is most likely a warning level log, since if the user provided a custom client ID and they’re in the cloud shell environment, an info log may not be sufficient indication that there was something wrong with the authentication.

The key point is that we can disable this credential on cloud shell by setting this isAvailable to false. I can think of other less clean approaches, like throwing on getToken or throwing on some other place if this is the only MSI available and the user specified a client ID. The alternatives seem much less clean to me than just making this MSI unavailable if clientId is provided.

The warning says "managed identities aren't available" in the cloud shell

Yeah this doesn’t look good! thank you for pointing it out. I have suggested some alternatives in another thread.

appeared to work previously. Feels a bit like a breaking change

This is fundamentally a bug fix, because in the past providing a client ID in cloud shell would have ignored the client ID and instead picked an entirely different identity (the system identity). We don’t want to default to the system identity if the user provided a client ID.

Well, I disagree that it's a bug fix. It's not making something broken work correctly; it's disabling a buggy thing by making it an error case. I think that's an important difference. In this case, someone could have functional code in which the client_id parameter just doesn't do anything. Or: they could be testing something in Cloud Shell, but the code provides client_id for a prod use case that is deployed to App Service. Both of those cases now fail because no MSI provider is considered to be "available." I don't think we have good options here, but this also doesn't feel good.

On logging, I really feel that an info-level log is more appropriate, and maybe a warning-level log in getToken for this MSI provider if it is actually selected and client_id is set (I understand that in the current implementation in this PR, that couldn't happen, but if it were possible then a warning-level log would make sense). Consider someone who is using App Service MSI and passing client_id. Maybe that person forwards logs to Azure Monitor, so they have AZURE_LOG_LEVEL=warning. They will get warnings even though they never actually used cloud shell MSI if it was even tested for availability. It feels a bit strange for the code to warn the developer about that, but an info-level log ("hey FYI this parameter doesn't work with this provider") would be perfectly normal.

It's not making something broken work correctly

It is making something broken work correctly since if users passed the clientId on the cloud shell environment they would have authenticated with the wrong client.

someone could have functional code in which the client_id parameter just doesn't do anything

The fact that it seemed to work doesn’t mean that it really worked. It didn’t work since the client ID wasn’t being used.

Or: they could be testing something in Cloud Shell, but the code provides client_id for a prod use case that is deployed to App Service

In this case their test was wrong, but the deployed service would continue to work.

Consider someone who is using App Service MSI and passing client_id. Maybe that person forwards logs to Azure Monitor, so they have AZURE_LOG_LEVEL=warning. They will get warnings even though they never actually used cloud shell MSI if it was even tested for availability. It feels a bit strange for the code to warn the developer about that, but an info-level log ("hey FYI this parameter doesn't work with this provider") would be perfectly normal.

Ok I like this reasoning! @KarishmaGhiya let’s make this an info log then.

@KarishmaGhiya Wait, let’s bring this conversation with other languages. They’re using a warning, but perhaps not in the same place? Let’s make sure we pick info or warning for the right reason.

I have some good news that will go well with your feedback, @witemple-msft !

After reading a conversation we had with the Identity crew, and this post by @KarishmaGhiya here: #20142 (comment) it is clear to me that this PR should be about adding a warning on getToken and not about preventing getToken 🙂 which means no arguably breaking changes.

sadasant · 2022-02-08T19:58:40Z

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

    const resource = mapScopesToResource(scopes);
    if (!resource) {
      logger.info(`${msiName}: Unavailable. Multiple scopes are not supported.`);
      return false;
    }
+    if (clientId) {
+      logger.warning(
+        `${msiName}: Unavailable. Azure Managed Identities aren't available in the Azure Cloud Shell.`


This log should be specifically that specifying a client ID won’t work when authenticating the ManagedIdentityCredential on a Cloud Shell environment. Client IDs are usually provided when users make user-assigned identities, so the phrase user-assigned might be useful. Have we checked at what other languages are doing?

So, perhaps ManagedIdentityCredential does not support user-assigned identities in the Cloud Shell environment? What do you think?

sadasant · 2022-02-08T20:08:45Z

sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts

      secureResponses: [createResponse(200, { access_token: "token" })],
    });
-
+    console.dir(authDetails);


Remember to remove this.

sdk/identity/identity/CHANGELOG.md

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

sadasant

Looks good to me! Let’s try to get a review from someone implementing this in another language before we merge this 🙂

sdk/identity/identity/CHANGELOG.md

christothes

LGTM - just suggested a tweak to the message text.

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

KarishmaGhiya · 2022-02-10T22:02:27Z

/azp run js-identity-ci

azure-pipelines · 2022-02-10T22:02:34Z

No pipelines are associated with this pull request.

KarishmaGhiya · 2022-02-10T22:42:04Z

/check-enforcer evaluate

…d identity

…id in Cloudshell env

…st to verify logger message

…l/cloudShellMsi.ts Co-authored-by: Christopher Scott <chriscott@hotmail.com>

sdk/identity/identity/CHANGELOG.md

sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts

…tial.spec.ts

KarishmaGhiya requested review from sadasant, schaabs and witemple-msft as code owners February 5, 2022 02:27

ghost added the Azure.Identity label Feb 5, 2022

KarishmaGhiya requested review from sadasant and removed request for sadasant February 5, 2022 02:28

sadasant reviewed Feb 5, 2022

View reviewed changes

sdk/identity/identity/src/credentials/managedIdentityCredential/cloudShellMsi.ts Show resolved Hide resolved

sadasant reviewed Feb 8, 2022

View reviewed changes

KarishmaGhiya force-pushed the cloudshell-identity branch from af247a4 to 52833cc Compare February 8, 2022 08:44

witemple-msft reviewed Feb 8, 2022

View reviewed changes

sadasant reviewed Feb 8, 2022

View reviewed changes

KarishmaGhiya requested review from deyaaeldeen, mikeharder and praveenkuttappan as code owners February 9, 2022 00:54

sadasant reviewed Feb 9, 2022

View reviewed changes

sdk/identity/identity/CHANGELOG.md Outdated Show resolved Hide resolved

sadasant reviewed Feb 9, 2022

View reviewed changes

sdk/identity/identity/src/credentials/managedIdentityCredential/cloudShellMsi.ts Outdated Show resolved Hide resolved

sadasant approved these changes Feb 9, 2022

View reviewed changes

sadasant reviewed Feb 9, 2022

View reviewed changes

sdk/identity/identity/CHANGELOG.md Show resolved Hide resolved

KarishmaGhiya force-pushed the cloudshell-identity branch from ab439e3 to 684ff38 Compare February 9, 2022 23:24

christothes approved these changes Feb 9, 2022

View reviewed changes

sdk/identity/identity/src/credentials/managedIdentityCredential/cloudShellMsi.ts Outdated Show resolved Hide resolved

KarishmaGhiya force-pushed the cloudshell-identity branch from f1ad7b8 to 49f89aa Compare February 10, 2022 21:50

KarishmaGhiya added 4 commits February 11, 2022 12:08

update changelog

d7b1112

log warning for users trying to access cloud shell using user assigne…

3ebf3dc

…d identity

added a test for managed identity credetnial with and without client …

357d6f7

…id in Cloudshell env

update versions

7f19a47

KarishmaGhiya and others added 5 commits February 11, 2022 12:10

update the logger warning to be called on getToken and updated the te…

64e9284

…st to verify logger message

update isAvailable

60f0067

update changelog formatting

0963a79

Update sdk/identity/identity/src/credentials/managedIdentityCredentia…

e5d5026

…l/cloudShellMsi.ts Co-authored-by: Christopher Scott <chriscott@hotmail.com>

update package.json

59da7ef

KarishmaGhiya force-pushed the cloudshell-identity branch from d58c0c7 to 59da7ef Compare February 11, 2022 20:15

KarishmaGhiya commented Feb 11, 2022

View reviewed changes

sdk/identity/identity/CHANGELOG.md Outdated Show resolved Hide resolved

KarishmaGhiya added 2 commits February 11, 2022 12:16

Update sdk/identity/identity/CHANGELOG.md

1560f86

Update CHANGELOG.md

c3fe27a

KarishmaGhiya commented Feb 11, 2022

View reviewed changes

sdk/identity/identity/CHANGELOG.md Outdated Show resolved Hide resolved

Update sdk/identity/identity/CHANGELOG.md

18458f4

KarishmaGhiya commented Feb 11, 2022

View reviewed changes

sdk/identity/identity/CHANGELOG.md Outdated Show resolved Hide resolved

KarishmaGhiya added 2 commits February 11, 2022 12:43

Update sdk/identity/identity/CHANGELOG.md

eaf10cd

Update CHANGELOG.md

f17ec19

KarishmaGhiya commented Feb 11, 2022

View reviewed changes

sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts Outdated Show resolved Hide resolved

Update sdk/identity/identity/test/internal/node/managedIdentityCreden…

b92b510

…tial.spec.ts

KarishmaGhiya enabled auto-merge (squash) February 11, 2022 20:50

KarishmaGhiya merged commit 6d5568c into Azure:main Feb 11, 2022

KarishmaGhiya mentioned this pull request Feb 11, 2022

Cloud Shell doesn't support user assigned identities #20142

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[identity] log warning for users trying to access cloud shell using user assigned identity #20226

[identity] log warning for users trying to access cloud shell using user assigned identity #20226

KarishmaGhiya commented Feb 5, 2022 •

edited

Loading

check-enforcer bot commented Feb 8, 2022

sadasant Feb 8, 2022

witemple-msft Feb 8, 2022

sadasant Feb 8, 2022

witemple-msft Feb 8, 2022

sadasant Feb 8, 2022

sadasant Feb 8, 2022

sadasant Feb 8, 2022

sadasant Feb 8, 2022

sadasant Feb 8, 2022

sadasant Feb 8, 2022 •

edited

Loading

sadasant Feb 8, 2022

sadasant Feb 8, 2022

sadasant left a comment

christothes left a comment

KarishmaGhiya commented Feb 10, 2022

azure-pipelines bot commented Feb 10, 2022

KarishmaGhiya commented Feb 10, 2022

[identity] log warning for users trying to access cloud shell using user assigned identity #20226

[identity] log warning for users trying to access cloud shell using user assigned identity #20226

Conversation

KarishmaGhiya commented Feb 5, 2022 • edited Loading

Packages impacted by this PR

Issues associated with this PR

Describe the problem that is addressed by this PR

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?

Checklists

check-enforcer bot commented Feb 8, 2022

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

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 Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadasant left a comment

Choose a reason for hiding this comment

christothes left a comment

Choose a reason for hiding this comment

KarishmaGhiya commented Feb 10, 2022

azure-pipelines bot commented Feb 10, 2022

KarishmaGhiya commented Feb 10, 2022

KarishmaGhiya commented Feb 5, 2022 •

edited

Loading

sadasant Feb 8, 2022 •

edited

Loading