Support Managed Identity by YanaXu · Pull Request #348 · Azure/login

Yan Xu (YanaXu) · 2023-09-26T07:50:52Z

Description

This PR is going to support both system- and user- assigned managed identity login for self-hosted runners on Azure VM.

What's new

The Action provides a parameter auth-type with value list [SERVICE_PRINCIPAL, IDENTITY] to identify the type of authentication.

If auth-type: SERVICE_PRINCIPAL with clientId, tenantId and clientSecret detected in your input, we will attempt to login by using service principal with the secret.

#login with secret
- uses: azure/login@v1
      with:
        auth-type: SERVICE_PRINCIPAL
        creds: ${{ secrets.AZURE_CREDENTIALS }}

If auth-type: SERVICE_PRINCIPAL with clientId and tenantId detected in your input, we will attempt to login by using OIDC.

#login with OIDC
- uses: azure/login@v1
    with:
        auth-type: SERVICE_PRINCIPAL
        client-id: ${{ secrets.AZURE_CLIENT_ID }}
        tenant-id: ${{ secrets.AZURE_TENANT_ID }}
        subscription-id: ${{ secrets.AZURE_SUBSCRIPTION_ID }}

If auth-type: IDENTITY with clientId detected in your input, we will attempt to login by using user-assigned managed identity.

#login with user-assigned managed identity
- uses: azure/login@v1
   with:
       auth-type: IDENTITY
       client-id: ${{ secrets.AZURE_CLIENT_ID }}
       subscription-id: ${{ secrets.AZURE_SUBSCRIPTION_ID }}

If auth-type: IDENTITY without clientId detected in your input, we will attempt to login by using system-assigned managed identity.

#login with system-assigned managed identity
- uses: azure/login@v1
 with:
       auth-type: IDENTITY
       subscription-id: ${{ secrets.AZURE_SUBSCRIPTION_ID }}

The default value of auth-type to be SERVICE_PRINCIPAL to make it backward-compatible.

Always specify the subscription-id for safety. Without doing so will make Azure CLI use the current active subscription, which can be risky if your default subscription has changed.
The changes for README.md is in Update README.md for Managed identity #344.

Yan Xu (YanaXu) · 2023-09-28T07:25:20Z

Hi Jiashuo Li (@jiasli) , Yeming Liu (@isra-fel) , Xiaogang (@dolauli) , please help us review this PR.
This design spec is Design Spec for Supporting Managed Identity in Azure Login Action.docx.
Please refer to this workflow file for the configuration of this action to support difference login methods.

The Azure CLI login commands are in src/cli/AzureCliLogin.ts.
The Azure PowerShell login cmdlets are in src/PowerShell/AzPSScriptBuilder.ts.

The readme.md of this action will be updated separately in a later PR.

Jiashuo Li (jiasli) · 2023-10-09T06:42:35Z

+            return;
+        }
+        if (!this.loginConfig.subscriptionId) {
+            core.warning('No subscription-id is given. Skip setting subscription... If there are mutiple subscriptions under the tenant, please input subscription-id to specify which subscription to use.');


No subscription-id is given. Skip setting subscription...

This statement is not accurate. Azure CLI chooses the first active subscription as the default/subscription, so an implicit setting subscription is performed.

Jiashuo Li (jiasli) · 2023-10-09T06:45:29Z

-        if (!this.subscriptionId && !this.allowNoSubscriptionsLogin) {
-            throw new Error("Not all values are present in the credentials. Ensure subscriptionId is supplied.");


The original design makes more sense to me. Without requiring the subscriptionId, the user may unexpectedly perform operations in a wrong subscription. See Azure/azure-cli#14933 (comment).

If this feature is not related to managed identity, I suggest we split this change to another PR.

Jiashuo Li (jiasli) · 2023-10-09T07:00:15Z

+                await this.loginWithUserAssignedIdentity(args);
+            }
+            else {
+                await this.loginWithSystemAssignedIdentity(args);


Can loginWithSystemAssignedIdentity function and its siblings be written inline as it only contains 1 line and is called only once?

Jiashuo Li (jiasli) · 2023-10-09T07:03:42Z

+        }
+        return result[AzPSConstants.Result];
+    }
+}


This file lacks the ending new line.

Jiashuo Li (jiasli) · 2023-10-09T07:28:31Z

In the previous code, creds is not compatible with individual parameters. We use creds for login using service principal with secret and individual parameters for OIDC login:

I personally prefer the original design as it is explicit and definitive. The new design allows using OIDC with either individual parameters or creds.

The value of creds is generated by az ad sp create-for-rbac --json-auth and users should never compose creds by themselves, so using creds with auth types other than service principal may not be very reasonable.

Yan Xu (YanaXu) · 2023-10-13T03:30:38Z

Hi Shiying Chen (@MoChilia) , Jiashuo Li (@jiasli), Yeming Liu (@isra-fel), Jin Lei (@msJinLei) ,
This PR is updated based on the review comments. Could you please review it again?

These comments are applied:

'creds' will only support login with SP+secrets.
'creds' will only be valid when there are no individual parameters and 'auth-type' is 'SERVICE_PRINCIPAL'.
if 'allow-no-subscriptions' is not set or set to false while 'subscription-id' is not set, throw an error (it's same as before).
In Azure PS login, don't use "Set-AzContext", add tenant id and subscription id in "Connect-AzAccount" instead.
Import Az.Accounts instead of adding its path to PSModulePath and print used Az.Accounts version.

This comment is skipped in this PR:

Add a warning message if 'allow-no-subscriptions' is true.

I also add the unit tests for parameter validation and Azure PS script generation. You can take them for reference.

Shiying Chen (MoChilia)

Common and Azure CLI part LGTM.

Yeming Liu (isra-fel) · 2023-10-17T06:34:46Z

    required: false
    default: 'api://AzureADTokenExchange'
+  auth-type:
+    description: 'The type of authentication. Supported values are SERVICE_PRINCIPAL, IDENTITY. Default value is SERVICE_PRINCIPAL' 


This introduced a new naming pattern "UPPER_CASE". It is different from all the existing parameters. Any particular reason?

Yeming Liu (isra-fel) · 2023-10-17T06:44:33Z

+        const runner: string = process.env.RUNNER_OS || os.type();
+        switch (runner.toLowerCase()) {
+            case "linux":
+                this.pushPSModulePath(AzPSConstants.DEFAULT_AZ_PATH_ON_LINUX);


Does this function work on a custom runner?

Yes. It'll also work on a self-hosted runner.
There are 2 options for this:

Only apply this for GitHub-Hosted runners.

I see there is an env property RUNNER_ENVIRONMENT which have 2 values "self-hosted" and "github-hosted". But I didn't find official docs for it. That's why I don't suggest to use it in our code since it's not reliable. I didn't find other ways to check if this action is running on a GitHub-Hosted runner.

The other way is to keep it as it is now.

If so, this line will be applied for "self-hosted" runner too. It could add an invalid path to PSModulePath. However, it does no harm. We have test cases for it. This is aligned with the original code.

I suggest we choose option 2.

Yeming Liu (isra-fel) · 2023-10-17T07:31:43Z

+            commands += `Add-AzEnvironment -Name '${loginConfig.environment}' -ARMEndpoint '${loginConfig.resourceManagerEndpointUrl}' | out-null;`;
+        }
+        if (loginConfig.authType === LoginConfig.AUTH_TYPE_SERVICE_PRINCIPAL) {
+            if (loginConfig.servicePrincipalKey) {


It seems the nested if-else can be refactored to something like switch (loginConfig.GetLoginFlow()) { ... } but I don't have a strong opinion here.

Anton Ryzhov (anton-delphai) · 2023-11-20T11:05:39Z

It seems like this change makes allow-no-subscriptions option useless as readParametersFromCreds() already throws an error without considering this option

Pawan Kumar Regoti (pawan-regoti) · 2023-11-20T19:43:46Z

a funny workaround is giving not null value to subscriptionId in creds and set allow-no-subscriptions to true. It will bypass the subscriptionId and not throws error saying subscriptionId is missing.

      - uses: azure/login@v1
        with:
          allow-no-subscriptions: true
          ...
          creds: '{"subscriptionId":"something"}'

github-advanced-security AI found potential problems Sep 26, 2023

View reviewed changes

Comment thread src/PowerShell/AzPSScriptBuilder.ts Fixed

Yan Xu (YanaXu) had a problem deploying to Automation test September 26, 2023 07:50 — with GitHub Actions Failure

Yan Xu (YanaXu) force-pushed the yanxu/mi branch from cc56f14 to cd07ba8 Compare September 26, 2023 08:15

Yan Xu (YanaXu) had a problem deploying to Automation test September 26, 2023 08:15 — with GitHub Actions Failure

Yan Xu (YanaXu) force-pushed the yanxu/mi branch from cd07ba8 to 5efccfa Compare September 27, 2023 07:16

Yan Xu (YanaXu) had a problem deploying to Automation test September 27, 2023 07:16 — with GitHub Actions Failure

Yan Xu (YanaXu) had a problem deploying to Automation test September 27, 2023 08:03 — with GitHub Actions Failure

Yan Xu (YanaXu) had a problem deploying to Automation test September 27, 2023 08:03 — with GitHub Actions Error

Yan Xu (YanaXu) had a problem deploying to Automation test September 27, 2023 08:03 — with GitHub Actions Failure

Yan Xu (YanaXu) had a problem deploying to Automation test September 27, 2023 08:03 — with GitHub Actions Error

Yan Xu (YanaXu) temporarily deployed to Automation test September 27, 2023 08:31 — with GitHub Actions Inactive

Shiying Chen (MoChilia) mentioned this pull request Sep 28, 2023

Allow to specify custom subject for federated login #346

Closed

Shiying Chen (MoChilia) reviewed Sep 28, 2023

View reviewed changes

Comment thread __tests__/PowerShell/AzPSScriptBuilder.test.ts Resolved

Comment thread src/PowerShell/AzPSLogin.ts Resolved

Comment thread src/PowerShell/AzPSScriptBuilder.ts Resolved

Yan Xu (YanaXu) mentioned this pull request Oct 9, 2023

azure/login@v1 does not work on MacOS runners #351

Closed

Jiashuo Li (jiasli) reviewed Oct 9, 2023

View reviewed changes

Yan Xu (YanaXu) mentioned this pull request Oct 13, 2023

remove a invalid check in pr-check workflow for supporting MI PR #352

Merged

Yan Xu (YanaXu) added 4 commits October 13, 2023 11:17

support MI for CLI

89ef558

support MI for PowerShell

1a8b55b

refactor and fix typo

ec12b5a

adjust logic according to review comments

4d29ec0

Jin Lei (msJinLei) approved these changes Oct 16, 2023

View reviewed changes

Shiying Chen (MoChilia) approved these changes Oct 16, 2023

View reviewed changes

Yeming Liu (isra-fel) reviewed Oct 17, 2023

View reviewed changes

Yan Xu (YanaXu) added 2 commits October 17, 2023 18:01

create a type for PS Script output and add some test cases

3caf02d

rename servicePrincipalKey to servicePrincipalSecret

bd9b473

		if (!this.subscriptionId && !this.allowNoSubscriptionsLogin) {
		throw new Error("Not all values are present in the credentials. Ensure subscriptionId is supplied.");

Conversation

Yan Xu (YanaXu) commented Sep 26, 2023 • edited by MoChilia Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

What's new

Uh oh!

Uh oh!

Yan Xu (YanaXu) commented Sep 28, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jiashuo Li (jiasli) Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jiashuo Li (jiasli) Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jiashuo Li (jiasli) Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jiashuo Li (jiasli) Oct 9, 2023

Choose a reason for hiding this comment

Uh oh!

Jiashuo Li (jiasli) commented Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Yan Xu (YanaXu) commented Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Shiying Chen (MoChilia) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Yeming Liu (isra-fel) Oct 17, 2023

Choose a reason for hiding this comment

Uh oh!

Yeming Liu (isra-fel) Oct 17, 2023

Choose a reason for hiding this comment

Uh oh!

Yan Xu (YanaXu) Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Yeming Liu (isra-fel) Oct 17, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Anton Ryzhov (anton-delphai) commented Nov 20, 2023

Uh oh!

Pawan Kumar Regoti (pawan-regoti) commented Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This branch was previously deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Yan Xu (YanaXu) commented Sep 26, 2023 •

edited by MoChilia

Loading

Jiashuo Li (jiasli) Oct 9, 2023 •

edited

Loading

Jiashuo Li (jiasli) Oct 9, 2023 •

edited

Loading

Jiashuo Li (jiasli) Oct 9, 2023 •

edited

Loading

Jiashuo Li (jiasli) commented Oct 9, 2023 •

edited

Loading

Yan Xu (YanaXu) commented Oct 13, 2023 •

edited

Loading

Yan Xu (YanaXu) Oct 17, 2023 •

edited

Loading

Pawan Kumar Regoti (pawan-regoti) commented Nov 20, 2023 •

edited

Loading