Skip to content

Conversation

@jiasli
Copy link
Member

@jiasli jiasli commented Nov 30, 2020

Description
az ad sp create-for-rbac creates a Contributor role assignment at subscription scope if --skip-assignment or --role is not provided. This is a dangerous behavior and contradicts the principle of least privilege.

This PR adds the warning to help message and command execution that:

In a future release, this command will NOT create any role assignment by default. (--skip-assignment will be the default behavior.) If needed, always use --role to explicitly create a role assignment.

Testing Guide

> az ad sp create-for-rbac --help

        By default, this command assigns the 'Contributor' role to the service principal at the
        subscription scope. To reduce your risk of a compromised service principal, use --skip-
        assignment to avoid creating a role assignment, then assign a more specific role and narrow
        the scope to a resource or resource group. See [steps to add a role
        assignment](https://aka.ms/azadsp-more) for more information.

        WARNING: In a future release, this command will NOT create 'Contributor' role assignment by
        default. If needed, always use --role to explicitly create a role assignment.

> az ad sp create-for-rbac
WARNING: In a future release, this command will NOT create 'Contributor' role assignment by default. If needed, 
always use --role to explicitly create a role assignment.
WARNING: Creating 'Contributor' role assignment under scope '/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590'
WARNING:   Retrying role assignment creation: 1/36
{
  "appId": "bc26a0e0-a351-403a-9605-f97424e35dd2",
  "displayName": "azure-cli-2020-11-30-06-01-01",
  "name": "http://azure-cli-2020-11-30-06-01-01",
  "password": "...",
  "tenant": "54826b22-38d6-4fb2-bad9-b7b93a3e9c5a"
}

See email:

  • az ad sp create-for-rbac: Deprecate creating role assignment by default
  • Updating az ad sp create-for-rbac behavior and warning

@jiasli jiasli requested a review from evelyn-ys November 30, 2020 06:13
@jiasli jiasli self-assigned this Nov 30, 2020
def create_service_principal_for_rbac(
# pylint:disable=too-many-statements,too-many-locals, too-many-branches
cmd, name=None, years=None, create_cert=False, cert=None, scopes=None, role='Contributor',
cmd, name=None, years=None, create_cert=False, cert=None, scopes=None, role=None,
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to detect whether role is explicitly set and show the warning if not.

If role='Contributor' is used, it won't be possible to detect if role is provided by the user or as the default value.

See https://stackoverflow.com/a/14749388

Copy link
Member Author

Choose a reason for hiding this comment

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

Another option is to check role.is_default, and show the warning message accordingly. If role is assigned a default value, its type will be knack.validators.DefaultStr which has is_default field indicating whether it is the default value.

logger.warning(ROLE_ASSIGNMENT_CREATE_WARNING)
for scope in scopes:
logger.warning('Creating a role assignment under the scope of "%s"', scope)
logger.warning("Creating '%s' role assignment under scope '%s'", role, scope)
Copy link
Member Author

@jiasli jiasli Nov 30, 2020

Choose a reason for hiding this comment

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

Echo the role name to be more explicit and informative.

@yonzhan
Copy link
Collaborator

yonzhan commented Nov 30, 2020

Role

@yonzhan yonzhan added this to the S179 milestone Nov 30, 2020
@evelyn-ys
Copy link
Member

evelyn-ys commented Nov 30, 2020

Shall we mark --skip-assignment as deprecated this time?

@jiasli
Copy link
Member Author

jiasli commented Dec 1, 2020

Shall we mark --skip-assignment as deprecated this time?

No, as it will be become the default behavior, instead of deprecated. We now recommend the user to use it!

Marking it as deprecated will further confuse the user. We can marked it as deprecated AFTER it becomes the default behavior.

# Conflicts:
#	src/azure-cli/azure/cli/command_modules/role/_help.py
#	src/azure-cli/azure/cli/command_modules/role/custom.py
@jiasli jiasli requested a review from yonzhan December 3, 2020 02:34
Copy link
Collaborator

@yonzhan yonzhan left a comment

Choose a reason for hiding this comment

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

LGTM

@yonzhan yonzhan modified the milestones: S179, S180 Dec 5, 2020
@jiasli jiasli changed the title [Role] az ad sp create-for-rbac: Deprecate creating role assignment by default [Role] az ad sp create-for-rbac: Deprecate creating Contributor role assignment by default Dec 22, 2020
@jiasli jiasli merged commit 00c59c2 into Azure:dev Dec 22, 2020
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.

4 participants