From ca8b3400b5317a3b1bbbaf8375b95da0d2878994 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 2 Nov 2020 15:33:08 -0800 Subject: [PATCH 1/7] Storing the user assigned managed identity in the scaleset table --- .../__app__/onefuzzlib/agent_authorization.py | 10 ++++---- src/api-service/__app__/onefuzzlib/pools.py | 24 +++++++++++++++---- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/agent_authorization.py b/src/api-service/__app__/onefuzzlib/agent_authorization.py index ce8fa54dd6..5fcadfc8c5 100644 --- a/src/api-service/__app__/onefuzzlib/agent_authorization.py +++ b/src/api-service/__app__/onefuzzlib/agent_authorization.py @@ -55,14 +55,14 @@ def try_get_token_auth_header(request: func.HttpRequest) -> Union[Error, TokenDa @cached(ttl=60) def is_authorized(token_data: TokenData) -> bool: - # verify object_id against the user assigned managed identity - if get_scaleset_principal_id() == token_data.object_id: - return True - # backward compatibility case for scalesets deployed before the migration # to user assigned managed id scalesets = Scaleset.get_by_object_id(token_data.object_id) - return len(scalesets) > 0 + if len(scalesets) > 0: + return True + + # verify object_id against the user assigned managed identity + return get_scaleset_principal_id() == token_data.object_id def verify_token( diff --git a/src/api-service/__app__/onefuzzlib/pools.py b/src/api-service/__app__/onefuzzlib/pools.py index 7e43ea4733..4d92169f59 100644 --- a/src/api-service/__app__/onefuzzlib/pools.py +++ b/src/api-service/__app__/onefuzzlib/pools.py @@ -5,7 +5,7 @@ import datetime import logging -from typing import Dict, List, Optional, Tuple, Union +from typing import Any, Dict, List, Optional, Tuple, Union from uuid import UUID, uuid4 from onefuzztypes.enums import ( @@ -712,14 +712,30 @@ def setup(self) -> None: logging.info("creating scaleset: %s", self.scaleset_id) elif vmss.provisioning_state == "Creating": logging.info("Waiting on scaleset creation: %s", self.scaleset_id) - if vmss.identity and vmss.identity.principal_id: - self.client_object_id = vmss.identity.principal_id + self.set_identity(vmss) else: logging.info("scaleset running: %s", self.scaleset_id) + self.set_identity(vmss) self.state = ScalesetState.running - self.client_object_id = vmss.identity.principal_id self.save() + def set_identity(self, vmss: Any) -> None: + if ( + vmss.identity + and vmss.user_assigned_identities + and (len(vmss.identity.user_assigned_identities) != 1) + ): + self.error = Error( + code=ErrorCode.VM_CREATE_FAILED, + errors=[ + "The scaleset is expected to have exactly 1 user assigned identity" + ], + ) + self.state = ScalesetState.creation_failed + + self.client_object_id = list(vmss.identity.user_assigned_identities.values())[0] + return None + # result = 'did I modify the scaleset in azure' def cleanup_nodes(self) -> bool: if self.state == ScalesetState.halt: From 3c7bc80fe4d7e89cbc05acea89af5c5cfc007a3e Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 2 Nov 2020 15:52:02 -0800 Subject: [PATCH 2/7] fix assignment --- src/api-service/__app__/onefuzzlib/pools.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/api-service/__app__/onefuzzlib/pools.py b/src/api-service/__app__/onefuzzlib/pools.py index 4d92169f59..9c4732a9dd 100644 --- a/src/api-service/__app__/onefuzzlib/pools.py +++ b/src/api-service/__app__/onefuzzlib/pools.py @@ -733,7 +733,8 @@ def set_identity(self, vmss: Any) -> None: ) self.state = ScalesetState.creation_failed - self.client_object_id = list(vmss.identity.user_assigned_identities.values())[0] + user_assinged_identities = list(vmss.identity.user_assigned_identities.values()) + self.client_object_id = user_assinged_identities[0].principal_id return None # result = 'did I modify the scaleset in azure' From ba6fc157b4ff66b8430a660823aa41604b9bec58 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 2 Nov 2020 15:58:31 -0800 Subject: [PATCH 3/7] more checks --- src/api-service/__app__/onefuzzlib/pools.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/api-service/__app__/onefuzzlib/pools.py b/src/api-service/__app__/onefuzzlib/pools.py index 9c4732a9dd..70377cd312 100644 --- a/src/api-service/__app__/onefuzzlib/pools.py +++ b/src/api-service/__app__/onefuzzlib/pools.py @@ -734,7 +734,8 @@ def set_identity(self, vmss: Any) -> None: self.state = ScalesetState.creation_failed user_assinged_identities = list(vmss.identity.user_assigned_identities.values()) - self.client_object_id = user_assinged_identities[0].principal_id + if user_assinged_identities[0].principal_id: + self.client_object_id = user_assinged_identities[0].principal_id return None # result = 'did I modify the scaleset in azure' From af2c11ea5a511cb80b4853799330ebb0306a8aa9 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 2 Nov 2020 16:05:10 -0800 Subject: [PATCH 4/7] mypy error fix --- src/api-service/__app__/onefuzzlib/agent_authorization.py | 3 ++- src/api-service/__app__/onefuzzlib/azure/creds.py | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/agent_authorization.py b/src/api-service/__app__/onefuzzlib/agent_authorization.py index 5fcadfc8c5..38cce6de02 100644 --- a/src/api-service/__app__/onefuzzlib/agent_authorization.py +++ b/src/api-service/__app__/onefuzzlib/agent_authorization.py @@ -62,7 +62,8 @@ def is_authorized(token_data: TokenData) -> bool: return True # verify object_id against the user assigned managed identity - return get_scaleset_principal_id() == token_data.object_id + principal_id: UUID = get_scaleset_principal_id() + return principal_id == token_data.object_id def verify_token( diff --git a/src/api-service/__app__/onefuzzlib/azure/creds.py b/src/api-service/__app__/onefuzzlib/azure/creds.py index ea065019fc..fdb80d91e1 100644 --- a/src/api-service/__app__/onefuzzlib/azure/creds.py +++ b/src/api-service/__app__/onefuzzlib/azure/creds.py @@ -151,7 +151,7 @@ def get_scaleset_identity_resource_path() -> str: @cached def get_scaleset_principal_id() -> UUID: - api_version = "2018-11-30" # matches the apiversion in the deplyoment template + api_version = "2018-11-30" # matches the apiversion in the deployment template client = mgmt_client_factory(ResourceManagementClient) uid = client.resources.get_by_id(get_scaleset_identity_resource_path(), api_version) return UUID(uid.properties["principalId"]) From 8233790600fd6d54f8acc376146e901ced657987 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 2 Nov 2020 17:33:22 -0800 Subject: [PATCH 5/7] bug fix --- src/api-service/__app__/onefuzzlib/pools.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/api-service/__app__/onefuzzlib/pools.py b/src/api-service/__app__/onefuzzlib/pools.py index 70377cd312..e9860c5508 100644 --- a/src/api-service/__app__/onefuzzlib/pools.py +++ b/src/api-service/__app__/onefuzzlib/pools.py @@ -722,7 +722,7 @@ def setup(self) -> None: def set_identity(self, vmss: Any) -> None: if ( vmss.identity - and vmss.user_assigned_identities + and vmss.identity.user_assigned_identities and (len(vmss.identity.user_assigned_identities) != 1) ): self.error = Error( From f96248ba8058a91dea89a38afcff29edc036163d Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 3 Nov 2020 16:48:10 -0800 Subject: [PATCH 6/7] running state should not override creation failed --- src/api-service/__app__/onefuzzlib/pools.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/api-service/__app__/onefuzzlib/pools.py b/src/api-service/__app__/onefuzzlib/pools.py index e9860c5508..945e324f07 100644 --- a/src/api-service/__app__/onefuzzlib/pools.py +++ b/src/api-service/__app__/onefuzzlib/pools.py @@ -715,11 +715,13 @@ def setup(self) -> None: self.set_identity(vmss) else: logging.info("scaleset running: %s", self.scaleset_id) - self.set_identity(vmss) self.state = ScalesetState.running + self.set_identity(vmss) self.save() def set_identity(self, vmss: Any) -> None: + if self.client_object_id: + return if ( vmss.identity and vmss.identity.user_assigned_identities From bfff31d1c3af034e82cd416036443878a3529fa1 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 3 Nov 2020 17:17:35 -0800 Subject: [PATCH 7/7] set the state to creation failed if there are no user identity on the scaleset --- src/api-service/__app__/onefuzzlib/pools.py | 35 +++++++++++++-------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/pools.py b/src/api-service/__app__/onefuzzlib/pools.py index 945e324f07..9c229d3cdd 100644 --- a/src/api-service/__app__/onefuzzlib/pools.py +++ b/src/api-service/__app__/onefuzzlib/pools.py @@ -712,33 +712,42 @@ def setup(self) -> None: logging.info("creating scaleset: %s", self.scaleset_id) elif vmss.provisioning_state == "Creating": logging.info("Waiting on scaleset creation: %s", self.scaleset_id) - self.set_identity(vmss) + self.try_set_identity(vmss) else: logging.info("scaleset running: %s", self.scaleset_id) - self.state = ScalesetState.running - self.set_identity(vmss) + error = self.try_set_identity(vmss) + if error: + self.state = ScalesetState.creation_failed + self.error = error + else: + self.state = ScalesetState.running self.save() - def set_identity(self, vmss: Any) -> None: + def try_set_identity(self, vmss: Any) -> Optional[Error]: + def get_error() -> Error: + return Error( + code=ErrorCode.VM_CREATE_FAILED, + errors=[ + "The scaleset is expected to have exactly 1 user assigned identity" + ], + ) + if self.client_object_id: - return + return None if ( vmss.identity and vmss.identity.user_assigned_identities and (len(vmss.identity.user_assigned_identities) != 1) ): - self.error = Error( - code=ErrorCode.VM_CREATE_FAILED, - errors=[ - "The scaleset is expected to have exactly 1 user assigned identity" - ], - ) - self.state = ScalesetState.creation_failed + return get_error() user_assinged_identities = list(vmss.identity.user_assigned_identities.values()) + if user_assinged_identities[0].principal_id: self.client_object_id = user_assinged_identities[0].principal_id - return None + return None + else: + return get_error() # result = 'did I modify the scaleset in azure' def cleanup_nodes(self) -> bool: