Skip to content

Conversation

@dbradish-microsoft
Copy link
Contributor

@dbradish-microsoft dbradish-microsoft commented Nov 6, 2020

Description

  • az ad sp create-for-rbac
  • az ad sp credential reset
  • az ad app credential reset

These commands may

  • show password in the terminal, or
  • generate a certificate and save it to the local file system

The user may accidentally check the credentials into the source control.

> az ad sp create-for-rbac
Creating a role assignment under the scope of "/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590"
{
  "appId": "d488e1e6-1622-46fd-949b-91c1b51f7d10",
  "displayName": "azure-cli-2020-11-17-08-17-11",
  "name": "http://azure-cli-2020-11-17-08-17-11",
  "password": "uLbYRPXc8~...",                        ## credential here
  "tenant": "54826b22-38d6-4fb2-bad9-b7b93a3e9c5a"
}

> az ad sp credential reset --name 8873edb1-8a68-48b6-b374-a598b4b0a21e
{
  "appId": "8873edb1-8a68-48b6-b374-a598b4b0a21e",
  "name": "8873edb1-8a68-48b6-b374-a598b4b0a21e",
  "password": "uLbYRPXc8~...",                        ## credential here
  "tenant": "54826b22-38d6-4fb2-bad9-b7b93a3e9c5a"
}

> az ad app credential reset --id f924487a-8bde-4fe1-96f5-c2124d082e51
{
  "appId": "f924487a-8bde-4fe1-96f5-c2124d082e51",
  "name": "f924487a-8bde-4fe1-96f5-c2124d082e51",
  "password": "uLbYRPXc8~...",                        ## credential here
  "tenant": "54826b22-38d6-4fb2-bad9-b7b93a3e9c5a"
}

This PRs explicitly tells the user about the behavior via

help

The output includes credentials that you must protect. Be sure that you do not include these
credentials in your code or check the credentials into your source control. As an
alternative, consider using [managed identities](https://aka.ms/azadsp-managed-identities)
if available to avoid the need to use credentials.

warning

The output includes credentials that you must protect. Be sure that you do not include these 
credentials in your code or check the credentials into your source control. For more information, 
see https://aka.ms/azadsp-cli

Testing Guide

az ad sp create-for-rbac -h
az ad sp create-for-rbac
az ad sp credential reset -h
az ad sp credential reset --name <appID>
az ad app credential reset -h
az ad app credential reset --id <appID>

ℹ Whether az ad sp create-for-rbac should assign Contributor role by default is still under debate. (#16081)

@dbradish-microsoft
Copy link
Contributor Author

dbradish-microsoft commented Nov 6, 2020

@jiasli , We've been asked by Robert Lyon [email protected], to add the following warnings to our reference content:


Warning for az ad sp create
When you create a service principal using the az ad sp create command, the output includes credentials that you must protect. Be sure that you do not include these credentials in your code or check the credentials into your source control. As an alternative, consider using managed identities (https://docs.microsoft.com/en-us/azure/active-directory/managed-identities-azure-resources/overview) to avoid the need to use credentials.

Warning for az ad sp create-for-rbac
When you create a service principal using the az ad sp create-for-rbac command, the output includes credentials that you must protect. Be sure that you do not include these credentials in your code or check the credentials into your source control. As an alternative, consider using managed identities (https://docs.microsoft.com/en-us/azure/active-directory/managed-identities-azure-resources/overview) to avoid the need to use credentials.

By default, az ad sp create-for-rbac assigns the Contributor role to the service principal at the subscription scope. To reduce your risk of a compromised service principal, assign a more specific role and narrow the scope to a resource or resource group. See https://docs.microsoft.com/en-us/azure/role-based-access-control/role-assignments-steps for more information.


Could you please verify that I have entered this in the correct location and format? The create-for-rbac message also needs a line break (two separate paragraphs) if possible.

Thank you.

@yonzhan yonzhan requested a review from evelyn-ys November 6, 2020 18:14
@yonzhan yonzhan added this to the S178 milestone Nov 6, 2020
@yonzhan
Copy link
Collaborator

yonzhan commented Nov 6, 2020

add to S178

@evelyn-ys
Copy link
Member

For my perspective, it may not be enough to add these warnings to help only. Customers won't see this if they don't use -h. How about post a hint when they do run az ad sp create and az ad sp create-for-rbac?

helps['ad sp create'] = """
type: command
short-summary: Create a service principal.
long-summary: When you create a service principal using the `az ad sp create` command, the output includes credentials that you must protect. Be sure that you do not include these credentials in your code or check the credentials into your source control. As an alternative, consider using `managed identities` (https://docs.microsoft.com/en-us/azure/active-directory/managed-identities-azure-resources/overview) to avoid the need to use credentials.
Copy link
Member

@jiasli jiasli Nov 10, 2020

Choose a reason for hiding this comment

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

ad sp create simply creates a Service Principal from an app and won't show any credentials.

> az ad app create --display-name myapp20201110
  ...
  "appId": "d990dd3e-f5fe-4d3c-bb62-3e568a5b1209",

> az ad sp create --id d990dd3e-f5fe-4d3c-bb62-3e568a5b1209
  ...
  "appId": "d990dd3e-f5fe-4d3c-bb62-3e568a5b1209",

This help message should go to az ad app/sp credential reset instead.

> az ad app credential reset --id d990dd3e-f5fe-4d3c-bb62-3e568a5b1209
{
  "appId": "d990dd3e-f5fe-4d3c-bb62-3e568a5b1209",
  "name": "d990dd3e-f5fe-4d3c-bb62-3e568a5b1209",
  "password": "3f~XjdIB0AML4...",
  "tenant": "54826b22-38d6-4fb2-bad9-b7b93a3e9c5a"
}

@jiasli
Copy link
Member

jiasli commented Nov 10, 2020

Please make sure that the PR title complies with our convention so that the CI can pass.

@jiasli
Copy link
Member

jiasli commented Nov 10, 2020

For my perspective, it may not be enough to add these warnings to help only. Customers won't see this if they don't use -h. How about post a hint when they do run az ad sp create and az ad sp create-for-rbac?

Good point. However, post-output hint is used to parse the output JSON and give the user more friendly information. This warning doesn't rely on the output of az ad sp create-for-rbac (independent).

In addition, the output of this command is not too long. A pre-output hint should suffice. Something like

> az ad sp create-for-rbac
Creating a role assignment under the scope of "/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590"
  Retrying role assignment creation: 1/36
WARNING: The output includes credentials that you must protect. Be sure that you do not include these credentials 
in your code or check the credentials into your source control.
{
  "appId": "73f977e9-5611-44c0-8991-db42a7fdd2f6",
  "displayName": "azure-cli-2020-11-10-05-54-54",
  "name": "http://azure-cli-2020-11-10-05-54-54",
  "password": "vcq0j_vGRI1Z...",
  "tenant": "54826b22-38d6-4fb2-bad9-b7b93a3e9c5a"
}

@dbradish-microsoft dbradish-microsoft changed the title Added long-summary for az ad sp create and az ad sp create-for-rbac [Active Directory] az ad sp create and az ad sp create-for-rbac: Add long-summary Nov 12, 2020
@yonzhan yonzhan modified the milestones: S178, S179 Nov 14, 2020
@jiasli jiasli changed the title [Active Directory] az ad sp create and az ad sp create-for-rbac: Add long-summary {Role} Add long-summary for commands which show passwords Nov 17, 2020
@jiasli jiasli changed the title {Role} Add long-summary for commands which show passwords {Role} Add long-summary/warning for commands generating credentials Nov 17, 2020
@jiasli jiasli marked this pull request as draft November 17, 2020 08:43
@jiasli jiasli marked this pull request as ready for review November 30, 2020 03:24
Copy link
Member

@evelyn-ys evelyn-ys left a comment

Choose a reason for hiding this comment

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

LGTM

@jiasli
Copy link
Member

jiasli commented Nov 30, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jiasli jiasli changed the title {Role} Add long-summary/warning for commands generating credentials [Role] Add long-summary/warning for commands generating credentials Dec 2, 2020
@jiasli jiasli merged commit d5d1fd3 into dev Dec 2, 2020
@jiasli jiasli deleted the dbradish_LongDescForAzAdSp branch December 2, 2020 03:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants