From 4f2287b52f0d1b97801234292a1084aafb35de85 Mon Sep 17 00:00:00 2001 From: nharper285 Date: Tue, 2 Nov 2021 10:22:19 -0700 Subject: [PATCH 01/24] Service Principal locked down by default and deploying user added. --- src/deployment/deploy.py | 9 +++++++-- src/deployment/registration.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 44b2bf614c..9b27dd2ab5 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -52,6 +52,7 @@ GraphQueryError, OnefuzzAppRole, add_application_password, + add_user, assign_instance_app_role, authorize_application, get_application, @@ -363,7 +364,7 @@ def setup_rbac(self) -> None: service_principal_params = { "accountEnabled": True, - "appRoleAssignmentRequired": False, + "appRoleAssignmentRequired": True, "servicePrincipalType": "Application", "appId": app["appId"], } @@ -397,7 +398,7 @@ def try_sp_create() -> None: else: raise error - try_sp_create() + sp = try_sp_create() else: existing_role_values = [app_role["value"] for app_role in app["appRoles"]] @@ -458,6 +459,10 @@ def try_sp_create() -> None: (password_id, password) = self.create_password(app["id"]) + if sp is not None: + logging.info("inside add_user") + add_user(sp.object_id, user.object_id) + cli_app = get_application( app_id=uuid.UUID(ONEFUZZ_CLI_APP), subscription_id=self.get_subscription_id(), diff --git a/src/deployment/registration.py b/src/deployment/registration.py index dad342ff8d..338baba8a1 100644 --- a/src/deployment/registration.py +++ b/src/deployment/registration.py @@ -646,6 +646,35 @@ def set_app_audience( raise Exception(err_str) +def add_user(objectId: str, principalId: str) -> None: + # Get principalId by retrieving owner for SP + http_body = { + "principalId": principalId, + "resourceId": objectId, + "appRoleId": "00000000-0000-0000-0000-000000000000", + } + try: + query_microsoft_graph( + method="POST", + resource="users/%s/appRoleAssignments" % principalId, + body=http_body, + ) + except GraphQueryError: + query = ( + "az rest --method post --url " + "https://graph.microsoft.com/v1.0/users/%s/appRoleAssignments " + "--body '%s' --headers \"Content-Type\"=application/json" + % (principalId, http_body) + ) + logger.warning( + "execute the following query in the azure portal bash shell and " + "run deploy.py again : \n%s", + query, + ) + err_str = "Unable to add user to SP using Microsoft Graph Query API. \n" + raise Exception(err_str) + + def main() -> None: formatter = argparse.ArgumentDefaultsHelpFormatter From bebc2f5480f52d3d78f6d7a2172563b656ab0328 Mon Sep 17 00:00:00 2001 From: nharper285 Date: Tue, 2 Nov 2021 10:44:55 -0700 Subject: [PATCH 02/24] Fixing call to client. --- src/deployment/deploy.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 9b27dd2ab5..7b267908aa 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -288,6 +288,11 @@ def setup_rbac(self) -> None: logger.info("using existing client application") return + client = get_client_from_cli_profile( + SubscriptionClient, subscription_id=self.get_subscription_id() + ) + user = client.signed_in_user.get() + app = get_application( display_name=self.application_name, subscription_id=self.get_subscription_id(), From f6eb239e1a24aeded9346db7f48d677790f76fce Mon Sep 17 00:00:00 2001 From: nharper285 Date: Tue, 2 Nov 2021 10:51:11 -0700 Subject: [PATCH 03/24] Adding multiple users. --- src/deployment/deploy.py | 9 ++++-- src/deployment/registration.py | 53 +++++++++++++++++----------------- 2 files changed, 33 insertions(+), 29 deletions(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 7b267908aa..5382fb64e7 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -52,7 +52,7 @@ GraphQueryError, OnefuzzAppRole, add_application_password, - add_user, + add_users, assign_instance_app_role, authorize_application, get_application, @@ -465,8 +465,11 @@ def try_sp_create() -> None: (password_id, password) = self.create_password(app["id"]) if sp is not None: - logging.info("inside add_user") - add_user(sp.object_id, user.object_id) + logging.info("inside add_users") + users = [user.object_id] + if self.admins: + users += self.admins + add_users(sp.object_id, user.object_id) cli_app = get_application( app_id=uuid.UUID(ONEFUZZ_CLI_APP), diff --git a/src/deployment/registration.py b/src/deployment/registration.py index 338baba8a1..000be9eb14 100644 --- a/src/deployment/registration.py +++ b/src/deployment/registration.py @@ -646,33 +646,34 @@ def set_app_audience( raise Exception(err_str) -def add_user(objectId: str, principalId: str) -> None: +def add_users(objectId: str, principalIds: List[str]) -> None: # Get principalId by retrieving owner for SP - http_body = { - "principalId": principalId, - "resourceId": objectId, - "appRoleId": "00000000-0000-0000-0000-000000000000", - } - try: - query_microsoft_graph( - method="POST", - resource="users/%s/appRoleAssignments" % principalId, - body=http_body, - ) - except GraphQueryError: - query = ( - "az rest --method post --url " - "https://graph.microsoft.com/v1.0/users/%s/appRoleAssignments " - "--body '%s' --headers \"Content-Type\"=application/json" - % (principalId, http_body) - ) - logger.warning( - "execute the following query in the azure portal bash shell and " - "run deploy.py again : \n%s", - query, - ) - err_str = "Unable to add user to SP using Microsoft Graph Query API. \n" - raise Exception(err_str) + for id in principalIds: + http_body = { + "principalId": id, + "resourceId": objectId, + "appRoleId": "00000000-0000-0000-0000-000000000000", + } + try: + query_microsoft_graph( + method="POST", + resource="users/%s/appRoleAssignments" % id, + body=http_body, + ) + except GraphQueryError: + query = ( + "az rest --method post --url " + "https://graph.microsoft.com/v1.0/users/%s/appRoleAssignments " + "--body '%s' --headers \"Content-Type\"=application/json" + % (id, http_body) + ) + logger.warning( + "execute the following query in the azure portal bash shell and " + "run deploy.py again : \n%s", + query, + ) + err_str = "Unable to add user to SP using Microsoft Graph Query API. \n" + raise Exception(err_str) def main() -> None: From aa710cf590325cd7d57d62924a7b54928843e2ae Mon Sep 17 00:00:00 2001 From: nharper285 Date: Tue, 2 Nov 2021 11:28:14 -0700 Subject: [PATCH 04/24] Retrieving SP id. --- src/deployment/deploy.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 5382fb64e7..1d6198f3b0 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -403,7 +403,7 @@ def try_sp_create() -> None: else: raise error - sp = try_sp_create() + try_sp_create() else: existing_role_values = [app_role["value"] for app_role in app["appRoles"]] @@ -464,12 +464,13 @@ def try_sp_create() -> None: (password_id, password) = self.create_password(app["id"]) - if sp is not None: + if app is not None: logging.info("inside add_users") + logging.info(app["id"]) users = [user.object_id] if self.admins: users += self.admins - add_users(sp.object_id, user.object_id) + add_users(app["id"], user.object_id) cli_app = get_application( app_id=uuid.UUID(ONEFUZZ_CLI_APP), From 31dcd1dbe3262069af5ffa06aa6981ca73b20f9a Mon Sep 17 00:00:00 2001 From: nharper285 Date: Tue, 2 Nov 2021 13:19:13 -0700 Subject: [PATCH 05/24] Refactorin gcall. --- src/deployment/deploy.py | 5 ++-- src/deployment/registration.py | 53 +++++++++++++++++----------------- 2 files changed, 29 insertions(+), 29 deletions(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 1d6198f3b0..3a8606417c 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -52,7 +52,7 @@ GraphQueryError, OnefuzzAppRole, add_application_password, - add_users, + add_user, assign_instance_app_role, authorize_application, get_application, @@ -470,7 +470,8 @@ def try_sp_create() -> None: users = [user.object_id] if self.admins: users += self.admins - add_users(app["id"], user.object_id) + for user_id in users: + add_user(app["id"], user_id) cli_app = get_application( app_id=uuid.UUID(ONEFUZZ_CLI_APP), diff --git a/src/deployment/registration.py b/src/deployment/registration.py index 000be9eb14..338baba8a1 100644 --- a/src/deployment/registration.py +++ b/src/deployment/registration.py @@ -646,34 +646,33 @@ def set_app_audience( raise Exception(err_str) -def add_users(objectId: str, principalIds: List[str]) -> None: +def add_user(objectId: str, principalId: str) -> None: # Get principalId by retrieving owner for SP - for id in principalIds: - http_body = { - "principalId": id, - "resourceId": objectId, - "appRoleId": "00000000-0000-0000-0000-000000000000", - } - try: - query_microsoft_graph( - method="POST", - resource="users/%s/appRoleAssignments" % id, - body=http_body, - ) - except GraphQueryError: - query = ( - "az rest --method post --url " - "https://graph.microsoft.com/v1.0/users/%s/appRoleAssignments " - "--body '%s' --headers \"Content-Type\"=application/json" - % (id, http_body) - ) - logger.warning( - "execute the following query in the azure portal bash shell and " - "run deploy.py again : \n%s", - query, - ) - err_str = "Unable to add user to SP using Microsoft Graph Query API. \n" - raise Exception(err_str) + http_body = { + "principalId": principalId, + "resourceId": objectId, + "appRoleId": "00000000-0000-0000-0000-000000000000", + } + try: + query_microsoft_graph( + method="POST", + resource="users/%s/appRoleAssignments" % principalId, + body=http_body, + ) + except GraphQueryError: + query = ( + "az rest --method post --url " + "https://graph.microsoft.com/v1.0/users/%s/appRoleAssignments " + "--body '%s' --headers \"Content-Type\"=application/json" + % (principalId, http_body) + ) + logger.warning( + "execute the following query in the azure portal bash shell and " + "run deploy.py again : \n%s", + query, + ) + err_str = "Unable to add user to SP using Microsoft Graph Query API. \n" + raise Exception(err_str) def main() -> None: From 259ddca9d869e25a656021ab00b91338304689e7 Mon Sep 17 00:00:00 2001 From: nharper285 Date: Tue, 2 Nov 2021 16:05:59 -0700 Subject: [PATCH 06/24] Getting closer. --- src/deployment/deploy.py | 22 ++++++++-------------- src/deployment/registration.py | 24 ++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 14 deletions(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 3a8606417c..6fea3f3912 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -51,6 +51,7 @@ from registration import ( GraphQueryError, OnefuzzAppRole, + get_signed_in_user, add_application_password, add_user, assign_instance_app_role, @@ -288,16 +289,10 @@ def setup_rbac(self) -> None: logger.info("using existing client application") return - client = get_client_from_cli_profile( - SubscriptionClient, subscription_id=self.get_subscription_id() - ) - user = client.signed_in_user.get() - app = get_application( display_name=self.application_name, subscription_id=self.get_subscription_id(), ) - app_roles = [ { "allowedMemberTypes": ["Application"], @@ -464,14 +459,13 @@ def try_sp_create() -> None: (password_id, password) = self.create_password(app["id"]) - if app is not None: - logging.info("inside add_users") - logging.info(app["id"]) - users = [user.object_id] - if self.admins: - users += self.admins - for user_id in users: - add_user(app["id"], user_id) + logger.info("inside add_users") + user = get_signed_in_user(self.subscription_id) + users = [user["id"]] + if self.admins: + users += self.admins + for user_id in users: + add_user(app["id"], user_id) cli_app = get_application( app_id=uuid.UUID(ONEFUZZ_CLI_APP), diff --git a/src/deployment/registration.py b/src/deployment/registration.py index 338baba8a1..1a6feae133 100644 --- a/src/deployment/registration.py +++ b/src/deployment/registration.py @@ -645,6 +645,30 @@ def set_app_audience( ) raise Exception(err_str) +def get_signed_in_user(subscriptionId: str) -> Optional[Any]: + # Get principalId by retrieving owner for SP + try: + app = query_microsoft_graph( + method="GET", + resource="me/", + subscription=subscriptionId, + ) + return app + + except GraphQueryError: + # query = ( + # "az rest --method post --url " + # "https://graph.microsoft.com/v1.0/users/%s/appRoleAssignments " + # "--body '%s' --headers \"Content-Type\"=application/json" + # % (principalId, http_body) + # ) + # logger.warning( + # "execute the following query in the azure portal bash shell and " + # "run deploy.py again : \n%s", + # query, + # ) + err_str = "Unable to add user to SP using Microsoft Graph Query API. \n" + raise Exception(err_str) def add_user(objectId: str, principalId: str) -> None: # Get principalId by retrieving owner for SP From eb84d0bb01f0a34a2ca4e54d4818ec971f1e90b2 Mon Sep 17 00:00:00 2001 From: nharper285 Date: Wed, 3 Nov 2021 10:46:23 -0700 Subject: [PATCH 07/24] Can retrieve object_id from create --- src/deployment/deploy.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 6fea3f3912..213ffd51c9 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -373,13 +373,13 @@ def try_sp_create() -> None: error: Optional[Exception] = None for _ in range(10): try: - query_microsoft_graph( + sp = query_microsoft_graph( method="POST", resource="servicePrincipals", body=service_principal_params, subscription=self.get_subscription_id(), ) - return + logger.info(sp) except GraphQueryError as err: # work around timing issue when creating service principal # https://github.com/Azure/azure-cli/issues/14767 @@ -465,6 +465,8 @@ def try_sp_create() -> None: if self.admins: users += self.admins for user_id in users: + #need object id of SP + # logger.info(app) add_user(app["id"], user_id) cli_app = get_application( From dd727f6e0120b3f17f919c656bb6eb95878719f1 Mon Sep 17 00:00:00 2001 From: nharper285 Date: Wed, 3 Nov 2021 12:05:22 -0700 Subject: [PATCH 08/24] New retrieve sp functionality. --- src/deployment/deploy.py | 13 ++++---- src/deployment/registration.py | 61 +++++++++++++++++++++++----------- 2 files changed, 47 insertions(+), 27 deletions(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 213ffd51c9..12633bb597 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -51,12 +51,13 @@ from registration import ( GraphQueryError, OnefuzzAppRole, - get_signed_in_user, add_application_password, add_user, assign_instance_app_role, authorize_application, get_application, + get_service_principal, + get_signed_in_user, get_tenant_id, query_microsoft_graph, register_application, @@ -373,13 +374,12 @@ def try_sp_create() -> None: error: Optional[Exception] = None for _ in range(10): try: - sp = query_microsoft_graph( + query_microsoft_graph( method="POST", resource="servicePrincipals", body=service_principal_params, subscription=self.get_subscription_id(), ) - logger.info(sp) except GraphQueryError as err: # work around timing issue when creating service principal # https://github.com/Azure/azure-cli/issues/14767 @@ -459,15 +459,14 @@ def try_sp_create() -> None: (password_id, password) = self.create_password(app["id"]) - logger.info("inside add_users") user = get_signed_in_user(self.subscription_id) + sp = get_service_principal(app["appId"], self.subscription_id) + users = [user["id"]] if self.admins: users += self.admins for user_id in users: - #need object id of SP - # logger.info(app) - add_user(app["id"], user_id) + add_user(sp["id"], user_id) cli_app = get_application( app_id=uuid.UUID(ONEFUZZ_CLI_APP), diff --git a/src/deployment/registration.py b/src/deployment/registration.py index 1a6feae133..aec387910c 100644 --- a/src/deployment/registration.py +++ b/src/deployment/registration.py @@ -645,42 +645,63 @@ def set_app_audience( ) raise Exception(err_str) -def get_signed_in_user(subscriptionId: str) -> Optional[Any]: + +def get_signed_in_user(subscription_id: str) -> Optional[Any]: # Get principalId by retrieving owner for SP try: app = query_microsoft_graph( method="GET", resource="me/", - subscription=subscriptionId, + subscription=subscription_id, ) return app - + except GraphQueryError: - # query = ( - # "az rest --method post --url " - # "https://graph.microsoft.com/v1.0/users/%s/appRoleAssignments " - # "--body '%s' --headers \"Content-Type\"=application/json" - # % (principalId, http_body) - # ) - # logger.warning( - # "execute the following query in the azure portal bash shell and " - # "run deploy.py again : \n%s", - # query, - # ) - err_str = "Unable to add user to SP using Microsoft Graph Query API. \n" + query = ( + "az rest --method post --url " + "https://graph.microsoft.com/v1.0/me " + '--headers "Content-Type"=application/json' + ) + logger.warning( + "execute the following query in the azure portal bash shell and " + "run deploy.py again : \n%s", + query, + ) + err_str = "Unable to retrieve signed-in user via Microsoft Graph Query API. \n" raise Exception(err_str) -def add_user(objectId: str, principalId: str) -> None: + +def get_service_principal(app_id: str, subscription_id: str) -> Optional[Any]: + try: + service_principals = query_microsoft_graph_list( + method="GET", + resource="servicePrincipals", + params={"$filter": f"appId eq '{app_id}'"}, + subscription=subscription_id, + ) + if len(service_principals) != 0: + return service_principals[0] + else: + raise GraphQueryError( + "Could not retrieve any service principals for App Id: %s", app_id + ) + + except GraphQueryError: + err_str = "Unable to add retrieve SP using Microsoft Graph Query API. \n" + raise Exception(err_str) + + +def add_user(object_id: str, principal_id: str) -> None: # Get principalId by retrieving owner for SP http_body = { - "principalId": principalId, - "resourceId": objectId, + "principalId": principal_id, + "resourceId": object_id, "appRoleId": "00000000-0000-0000-0000-000000000000", } try: query_microsoft_graph( method="POST", - resource="users/%s/appRoleAssignments" % principalId, + resource="users/%s/appRoleAssignments" % principal_id, body=http_body, ) except GraphQueryError: @@ -688,7 +709,7 @@ def add_user(objectId: str, principalId: str) -> None: "az rest --method post --url " "https://graph.microsoft.com/v1.0/users/%s/appRoleAssignments " "--body '%s' --headers \"Content-Type\"=application/json" - % (principalId, http_body) + % (principal_id, http_body) ) logger.warning( "execute the following query in the azure portal bash shell and " From 59ece5d840524041199a44d58b13f1135663a2f7 Mon Sep 17 00:00:00 2001 From: nharper285 Date: Wed, 3 Nov 2021 12:51:14 -0700 Subject: [PATCH 09/24] mypy fixes. --- src/deployment/deploy.py | 3 ++- src/deployment/registration.py | 37 ++++++++++++++++++---------------- 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 12633bb597..1e0f573840 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -464,7 +464,8 @@ def try_sp_create() -> None: users = [user["id"]] if self.admins: - users += self.admins + admins_str = [str(x) for x in self.admins] + users += admins_str for user_id in users: add_user(sp["id"], user_id) diff --git a/src/deployment/registration.py b/src/deployment/registration.py index aec387910c..24e6626665 100644 --- a/src/deployment/registration.py +++ b/src/deployment/registration.py @@ -646,7 +646,7 @@ def set_app_audience( raise Exception(err_str) -def get_signed_in_user(subscription_id: str) -> Optional[Any]: +def get_signed_in_user(subscription_id: Optional[str]) -> Any: # Get principalId by retrieving owner for SP try: app = query_microsoft_graph( @@ -671,7 +671,7 @@ def get_signed_in_user(subscription_id: str) -> Optional[Any]: raise Exception(err_str) -def get_service_principal(app_id: str, subscription_id: str) -> Optional[Any]: +def get_service_principal(app_id: str, subscription_id: Optional[str]) -> Any: try: service_principals = query_microsoft_graph_list( method="GET", @@ -683,7 +683,7 @@ def get_service_principal(app_id: str, subscription_id: str) -> Optional[Any]: return service_principals[0] else: raise GraphQueryError( - "Could not retrieve any service principals for App Id: %s", app_id + f"Could not retrieve any service principals for App Id: {app_id}", 400 ) except GraphQueryError: @@ -704,20 +704,23 @@ def add_user(object_id: str, principal_id: str) -> None: resource="users/%s/appRoleAssignments" % principal_id, body=http_body, ) - except GraphQueryError: - query = ( - "az rest --method post --url " - "https://graph.microsoft.com/v1.0/users/%s/appRoleAssignments " - "--body '%s' --headers \"Content-Type\"=application/json" - % (principal_id, http_body) - ) - logger.warning( - "execute the following query in the azure portal bash shell and " - "run deploy.py again : \n%s", - query, - ) - err_str = "Unable to add user to SP using Microsoft Graph Query API. \n" - raise Exception(err_str) + except GraphQueryError as ex: + if "Permission being assigned already exists" not in ex.message: + query = ( + "az rest --method post --url " + "https://graph.microsoft.com/v1.0/users/%s/appRoleAssignments " + "--body '%s' --headers \"Content-Type\"=application/json" + % (principal_id, http_body) + ) + logger.warning( + "execute the following query in the azure portal bash shell and " + "run deploy.py again : \n%s", + query, + ) + err_str = "Unable to add user to SP using Microsoft Graph Query API. \n" + raise Exception(err_str) + else: + logger.info("User already assigned to application.") def main() -> None: From aa93fc9400bc40a84aeec8af4adfec547de4b8b0 Mon Sep 17 00:00:00 2001 From: nharper285 Date: Wed, 3 Nov 2021 13:26:26 -0700 Subject: [PATCH 10/24] Separating functionality into new function. --- src/deployment/deploy.py | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 1e0f573840..4e93e6de42 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -459,15 +459,15 @@ def try_sp_create() -> None: (password_id, password) = self.create_password(app["id"]) - user = get_signed_in_user(self.subscription_id) - sp = get_service_principal(app["appId"], self.subscription_id) + # user = get_signed_in_user(self.subscription_id) + # sp = get_service_principal(app["appId"], self.subscription_id) - users = [user["id"]] - if self.admins: - admins_str = [str(x) for x in self.admins] - users += admins_str - for user_id in users: - add_user(sp["id"], user_id) + # users = [user["id"]] + # if self.admins: + # admins_str = [str(x) for x in self.admins] + # users += admins_str + # for user_id in users: + # add_user(sp["id"], user_id) cli_app = get_application( app_id=uuid.UUID(ONEFUZZ_CLI_APP), @@ -605,6 +605,22 @@ def assign_scaleset_identity_role(self) -> None: OnefuzzAppRole.ManagedNode, ) + def assign_user_access(self) -> None: + logger.info("assinging user access to service principal") + app = get_application( + display_name=self.application_name, + subscription_id=self.get_subscription_id(), + ) + user = get_signed_in_user(self.subscription_id) + sp = get_service_principal(app["appId"], self.subscription_id) + + users = [user["id"]] + if self.admins: + admins_str = [str(x) for x in self.admins] + users += admins_str + for user_id in users: + add_user(sp["id"], user_id) + def apply_migrations(self) -> None: logger.info("applying database migrations") name = self.results["deploy"]["func-name"]["value"] @@ -962,6 +978,7 @@ def main() -> None: ("rbac", Client.setup_rbac), ("arm", Client.deploy_template), ("assign_scaleset_identity_role", Client.assign_scaleset_identity_role), + ("assign_user_access", Client.assign_user_access), ] full_deployment_states = rbac_only_states + [ From f483460724cf5ac85f0dc0d28e1ca9f84a598cfd Mon Sep 17 00:00:00 2001 From: nharper285 Date: Wed, 3 Nov 2021 14:11:02 -0700 Subject: [PATCH 11/24] mypy errors. --- src/deployment/deploy.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 4e93e6de42..1087899b8a 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -612,14 +612,15 @@ def assign_user_access(self) -> None: subscription_id=self.get_subscription_id(), ) user = get_signed_in_user(self.subscription_id) - sp = get_service_principal(app["appId"], self.subscription_id) - - users = [user["id"]] - if self.admins: - admins_str = [str(x) for x in self.admins] - users += admins_str - for user_id in users: - add_user(sp["id"], user_id) + if app: + sp = get_service_principal(app["appId"], self.subscription_id) + + users = [user["id"]] + if self.admins: + admins_str = [str(x) for x in self.admins] + users += admins_str + for user_id in users: + add_user(sp["id"], user_id) def apply_migrations(self) -> None: logger.info("applying database migrations") From f92f6423de1cbed02d0db03fb766adff8c082c5b Mon Sep 17 00:00:00 2001 From: nharper285 Date: Thu, 4 Nov 2021 13:47:18 -0700 Subject: [PATCH 12/24] Logic for updating appRoleAssignemntRequired param. --- src/deployment/deploy.py | 49 ++++++++++++++++++++++++---------- src/deployment/registration.py | 6 +++-- 2 files changed, 39 insertions(+), 16 deletions(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 1087899b8a..264b20c470 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -311,6 +311,14 @@ def setup_rbac(self) -> None: "isEnabled": True, "value": OnefuzzAppRole.ManagedNode.value, }, + { + "allowedMemberTypes": ["Application"], + "description": "Allows user access from the CLI.", + "displayName": OnefuzzAppRole.UserAssignment.value, + "id": str(uuid.uuid4()), + "isEnabled": True, + "value": OnefuzzAppRole.UserAssignment.value, + }, ] if not app: @@ -423,11 +431,10 @@ def try_sp_create() -> None: # this is a requirement to update the application roles for role in app["appRoles"]: role["isEnabled"] = False - query_microsoft_graph( method="PATCH", resource=f"applications/{app['id']}", - body={"appRoles": app["AppRoles"]}, + body={"appRoles": app["appRoles"]}, subscription=self.get_subscription_id(), ) @@ -459,16 +466,6 @@ def try_sp_create() -> None: (password_id, password) = self.create_password(app["id"]) - # user = get_signed_in_user(self.subscription_id) - # sp = get_service_principal(app["appId"], self.subscription_id) - - # users = [user["id"]] - # if self.admins: - # admins_str = [str(x) for x in self.admins] - # users += admins_str - # for user_id in users: - # add_user(sp["id"], user_id) - cli_app = get_application( app_id=uuid.UUID(ONEFUZZ_CLI_APP), subscription_id=self.get_subscription_id(), @@ -612,15 +609,39 @@ def assign_user_access(self) -> None: subscription_id=self.get_subscription_id(), ) user = get_signed_in_user(self.subscription_id) + if app: sp = get_service_principal(app["appId"], self.subscription_id) - + # Update appRoleAssignmentRequired if necessary + if not sp["appRoleAssignmentRequired"]: + service_principal_params = { + "appRoleAssignmentRequired": True, + } + try: + query_microsoft_graph( + method="POST", + resource="servicePrincipals", + body=service_principal_params, + subscription=self.get_subscription_id(), + ) + except GraphQueryError as err: + if ( + "service principal was not updated with proper appRoleAssignmentRequired value (True)." + not in str(err) + ): + raise err + logger.warning("udating service principal failed with an error.") + + # Assign Roles and Add Users + roles = [ + x["id"] for x in app["appRoles"] if x["displayName"] == "UserAssignment" + ] users = [user["id"]] if self.admins: admins_str = [str(x) for x in self.admins] users += admins_str for user_id in users: - add_user(sp["id"], user_id) + add_user(sp["id"], user_id, roles[0]) def apply_migrations(self) -> None: logger.info("applying database migrations") diff --git a/src/deployment/registration.py b/src/deployment/registration.py index 24e6626665..78eebc92e3 100644 --- a/src/deployment/registration.py +++ b/src/deployment/registration.py @@ -154,6 +154,7 @@ class ApplicationInfo(NamedTuple): class OnefuzzAppRole(Enum): ManagedNode = "ManagedNode" CliClient = "CliClient" + UserAssignment = "UserAssignment" def register_application( @@ -691,12 +692,13 @@ def get_service_principal(app_id: str, subscription_id: Optional[str]) -> Any: raise Exception(err_str) -def add_user(object_id: str, principal_id: str) -> None: +def add_user(object_id: str, principal_id: str, role_id: str) -> None: # Get principalId by retrieving owner for SP + # need to add users with proper role assignment http_body = { "principalId": principal_id, "resourceId": object_id, - "appRoleId": "00000000-0000-0000-0000-000000000000", + "appRoleId": role_id, } try: query_microsoft_graph( From b7353657bf6c15554eff17b4d4926645b905a925 Mon Sep 17 00:00:00 2001 From: nharper285 Date: Thu, 4 Nov 2021 13:50:24 -0700 Subject: [PATCH 13/24] Changing to patch. --- src/deployment/deploy.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 264b20c470..6202ea050b 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -619,7 +619,7 @@ def assign_user_access(self) -> None: } try: query_microsoft_graph( - method="POST", + method="PATCH", resource="servicePrincipals", body=service_principal_params, subscription=self.get_subscription_id(), From 10beca137dfaf439127c531b7ab4cb81a0f71f0b Mon Sep 17 00:00:00 2001 From: nharper285 Date: Thu, 4 Nov 2021 13:52:39 -0700 Subject: [PATCH 14/24] Updating to Patch. --- src/deployment/deploy.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 6202ea050b..aba53006c9 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -620,7 +620,7 @@ def assign_user_access(self) -> None: try: query_microsoft_graph( method="PATCH", - resource="servicePrincipals", + resource=f"servicePrincipals/{sp['id']}", body=service_principal_params, subscription=self.get_subscription_id(), ) From d3c990602b5240b723b223ea0a9400b61de284db Mon Sep 17 00:00:00 2001 From: nharper285 Date: Thu, 4 Nov 2021 13:59:54 -0700 Subject: [PATCH 15/24] Fixing bug. --- src/deployment/deploy.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index aba53006c9..3224c0f49f 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -622,7 +622,7 @@ def assign_user_access(self) -> None: method="PATCH", resource=f"servicePrincipals/{sp['id']}", body=service_principal_params, - subscription=self.get_subscription_id(), + subscription=self.subscription_id, ) except GraphQueryError as err: if ( @@ -630,7 +630,7 @@ def assign_user_access(self) -> None: not in str(err) ): raise err - logger.warning("udating service principal failed with an error.") + logger.warning(err) # Assign Roles and Add Users roles = [ From 3edb282d799a74a8f88e85a9bc4f8a3be8513903 Mon Sep 17 00:00:00 2001 From: nharper285 Date: Thu, 4 Nov 2021 14:01:27 -0700 Subject: [PATCH 16/24] Fixing bad assignment. --- src/deployment/deploy.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 3224c0f49f..ea4725d1c0 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -630,7 +630,7 @@ def assign_user_access(self) -> None: not in str(err) ): raise err - logger.warning(err) + logger.warning(err) # Assign Roles and Add Users roles = [ From 9cd5c222767963bf7f21d44a0e640eb3e3cdee54 Mon Sep 17 00:00:00 2001 From: nharper285 Date: Thu, 4 Nov 2021 14:06:48 -0700 Subject: [PATCH 17/24] Responding to comments. --- src/deployment/deploy.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index ea4725d1c0..73e022b30b 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -312,7 +312,7 @@ def setup_rbac(self) -> None: "value": OnefuzzAppRole.ManagedNode.value, }, { - "allowedMemberTypes": ["Application"], + "allowedMemberTypes": ["User"], "description": "Allows user access from the CLI.", "displayName": OnefuzzAppRole.UserAssignment.value, "id": str(uuid.uuid4()), @@ -634,7 +634,7 @@ def assign_user_access(self) -> None: # Assign Roles and Add Users roles = [ - x["id"] for x in app["appRoles"] if x["displayName"] == "UserAssignment" + x["id"] for x in app["appRoles"] if x["displayName"] == OnefuzzAppRole.UserAssignment.value ] users = [user["id"]] if self.admins: From a7d96479aca1dfaac3cbe6124cbc87a62f6c384e Mon Sep 17 00:00:00 2001 From: nharper285 Date: Thu, 4 Nov 2021 14:29:01 -0700 Subject: [PATCH 18/24] Removing functionality for updating setting. --- src/deployment/deploy.py | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 73e022b30b..1f594e7704 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -614,27 +614,18 @@ def assign_user_access(self) -> None: sp = get_service_principal(app["appId"], self.subscription_id) # Update appRoleAssignmentRequired if necessary if not sp["appRoleAssignmentRequired"]: - service_principal_params = { - "appRoleAssignmentRequired": True, - } - try: - query_microsoft_graph( - method="PATCH", - resource=f"servicePrincipals/{sp['id']}", - body=service_principal_params, - subscription=self.subscription_id, - ) - except GraphQueryError as err: - if ( - "service principal was not updated with proper appRoleAssignmentRequired value (True)." - not in str(err) - ): - raise err - logger.warning(err) + logger.warning( + "The Service Principal does not have 'appRoleAssignmentRequired' set to True." + + " This means that any authenticated user can access the principal." + + " If you are manually upgrading an instance, and want this value set to 'True'," + + " you must manually update the setting." + ) # Assign Roles and Add Users roles = [ - x["id"] for x in app["appRoles"] if x["displayName"] == OnefuzzAppRole.UserAssignment.value + x["id"] + for x in app["appRoles"] + if x["displayName"] == OnefuzzAppRole.UserAssignment.value ] users = [user["id"]] if self.admins: From 6257a322264079b325edc8f512355f0bdf501227 Mon Sep 17 00:00:00 2001 From: Noah McGregor Harper <74685766+nharper285@users.noreply.github.com> Date: Thu, 4 Nov 2021 14:38:34 -0700 Subject: [PATCH 19/24] Update src/deployment/deploy.py Co-authored-by: Cheick Keita --- src/deployment/deploy.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 1f594e7704..6383f33718 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -616,7 +616,7 @@ def assign_user_access(self) -> None: if not sp["appRoleAssignmentRequired"]: logger.warning( "The Service Principal does not have 'appRoleAssignmentRequired' set to True." - + " This means that any authenticated user can access the principal." + + " This means that any authenticated user can access the service." + " If you are manually upgrading an instance, and want this value set to 'True'," + " you must manually update the setting." ) From f1a85c17a80b5526e42c1600e808a12d9912ea70 Mon Sep 17 00:00:00 2001 From: nharper285 Date: Thu, 4 Nov 2021 15:13:40 -0700 Subject: [PATCH 20/24] UPdating error message. --- src/deployment/deploy.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 1f594e7704..1b65d71e7c 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -615,10 +615,9 @@ def assign_user_access(self) -> None: # Update appRoleAssignmentRequired if necessary if not sp["appRoleAssignmentRequired"]: logger.warning( - "The Service Principal does not have 'appRoleAssignmentRequired' set to True." - + " This means that any authenticated user can access the principal." - + " If you are manually upgrading an instance, and want this value set to 'True'," - + " you must manually update the setting." + "The service is not currently configured to require a role assignment to access it." + + " This means that any authenticated user can access the service." + + " To change this behavior enable 'Assignment Required?' on the service principal in the AAD Portal." ) # Assign Roles and Add Users From 420973ce27600b31e339822304d1d44dd9a3f44a Mon Sep 17 00:00:00 2001 From: nharper285 Date: Fri, 5 Nov 2021 10:18:04 -0700 Subject: [PATCH 21/24] Retriggering? --- src/deployment/deploy.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 1b65d71e7c..db9ff21aad 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -616,7 +616,7 @@ def assign_user_access(self) -> None: if not sp["appRoleAssignmentRequired"]: logger.warning( "The service is not currently configured to require a role assignment to access it." - + " This means that any authenticated user can access the service." + + " This means that any authenticated user can access the service. " + " To change this behavior enable 'Assignment Required?' on the service principal in the AAD Portal." ) From f4deda1c9345c23aad58eb606b2ad1da39855887 Mon Sep 17 00:00:00 2001 From: nharper285 Date: Fri, 5 Nov 2021 10:29:44 -0700 Subject: [PATCH 22/24] Trying to emulate new file structure. --- src/deployment/deploy.py | 4 ++-- src/deployment/deploylib/__init__.py | 0 src/deployment/{ => deploylib}/registration.py | 0 src/deployment/{ => deploylib}/set_admins.py | 0 4 files changed, 2 insertions(+), 2 deletions(-) create mode 100644 src/deployment/deploylib/__init__.py rename src/deployment/{ => deploylib}/registration.py (100%) rename src/deployment/{ => deploylib}/set_admins.py (100%) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index db9ff21aad..d6416bfee3 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -48,7 +48,7 @@ from msrest.serialization import TZ_UTC from data_migration import migrate -from registration import ( +from deploylib.registration import ( GraphQueryError, OnefuzzAppRole, add_application_password, @@ -64,7 +64,7 @@ set_app_audience, update_pool_registration, ) -from set_admins import update_admins, update_allowed_aad_tenants +from deploylib.set_admins import update_admins, update_allowed_aad_tenants # Found by manually assigning the User.Read permission to application # registration in the admin portal. The values are in the manifest under diff --git a/src/deployment/deploylib/__init__.py b/src/deployment/deploylib/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/src/deployment/registration.py b/src/deployment/deploylib/registration.py similarity index 100% rename from src/deployment/registration.py rename to src/deployment/deploylib/registration.py diff --git a/src/deployment/set_admins.py b/src/deployment/deploylib/set_admins.py similarity index 100% rename from src/deployment/set_admins.py rename to src/deployment/deploylib/set_admins.py From 1a08f5a45971b5fe093175f86420f55c989fb647 Mon Sep 17 00:00:00 2001 From: nharper285 Date: Fri, 5 Nov 2021 11:21:48 -0700 Subject: [PATCH 23/24] Fixing lint issues. --- src/deployment/deploy.py | 2 -- src/deployment/deploylib/registration.py | 1 + src/deployment/deploylib/set_admins.py | 5 ++--- src/deployment/deploylib/tests/test_deploy_config.py | 2 +- 4 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 5f6cf86cda..d0dc25fd9d 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -56,7 +56,6 @@ update_nsg, ) from deploylib.data_migration import migrate - from deploylib.registration import ( GraphQueryError, OnefuzzAppRole, @@ -73,7 +72,6 @@ set_app_audience, update_pool_registration, ) -from deploylib.set_admins import update_admins, update_allowed_aad_tenants # Found by manually assigning the User.Read permission to application # registration in the admin portal. The values are in the manifest under diff --git a/src/deployment/deploylib/registration.py b/src/deployment/deploylib/registration.py index 62a46d77fb..78eebc92e3 100644 --- a/src/deployment/deploylib/registration.py +++ b/src/deployment/deploylib/registration.py @@ -724,6 +724,7 @@ def add_user(object_id: str, principal_id: str, role_id: str) -> None: else: logger.info("User already assigned to application.") + def main() -> None: formatter = argparse.ArgumentDefaultsHelpFormatter diff --git a/src/deployment/deploylib/set_admins.py b/src/deployment/deploylib/set_admins.py index e924445757..92e67d92bc 100644 --- a/src/deployment/deploylib/set_admins.py +++ b/src/deployment/deploylib/set_admins.py @@ -4,9 +4,6 @@ # Licensed under the MIT License. import argparse -import json -import logging -from typing import List, Optional from uuid import UUID from azure.common.client_factory import get_client_from_cli_profile @@ -19,6 +16,7 @@ update_allowed_aad_tenants, ) + def main() -> None: formatter = argparse.ArgumentDefaultsHelpFormatter parser = argparse.ArgumentParser(formatter_class=formatter) @@ -41,5 +39,6 @@ def main() -> None: if args.allowed_aad_tenants: update_allowed_aad_tenants(config_client, args.allowed_aad_tenants) + if __name__ == "__main__": main() diff --git a/src/deployment/deploylib/tests/test_deploy_config.py b/src/deployment/deploylib/tests/test_deploy_config.py index 6e39780ed6..dc0bdf8521 100644 --- a/src/deployment/deploylib/tests/test_deploy_config.py +++ b/src/deployment/deploylib/tests/test_deploy_config.py @@ -4,7 +4,7 @@ # Licensed under the MIT License. import unittest -from typing import Any, List +from typing import Any from deploylib.configuration import NetworkSecurityConfig From 69f1e0069796cd8b660b318ea638236542fab621 Mon Sep 17 00:00:00 2001 From: nharper285 Date: Fri, 5 Nov 2021 14:29:55 -0700 Subject: [PATCH 24/24] Fixing create sp. --- src/deployment/deploy.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index d0dc25fd9d..c8715ef863 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -397,6 +397,7 @@ def try_sp_create() -> None: body=service_principal_params, subscription=self.get_subscription_id(), ) + return except GraphQueryError as err: # work around timing issue when creating service principal # https://github.com/Azure/azure-cli/issues/14767