From bd8d425cee90df2ea2326d2e03901813c698514c Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 7 Jun 2021 11:02:45 -0700 Subject: [PATCH 001/103] migrate to msgraph --- src/deployment/registration.py | 143 ++++++++++++++++----------------- 1 file changed, 69 insertions(+), 74 deletions(-) diff --git a/src/deployment/registration.py b/src/deployment/registration.py index 8d4a82c969..6791914ae8 100644 --- a/src/deployment/registration.py +++ b/src/deployment/registration.py @@ -12,6 +12,8 @@ from typing import Any, Callable, Dict, List, NamedTuple, Optional, Tuple, TypeVar from uuid import UUID, uuid4 +# https://github.com/microsoftgraph/msgraph-sdk-python-core + import requests from azure.cli.core.azclierror import AuthenticationError from azure.common.client_factory import get_client_from_cli_profile @@ -141,39 +143,29 @@ def register_application( subscription_id: str, ) -> ApplicationInfo: logger.info("retrieving the application registration %s" % registration_name) - client = get_graph_client(subscription_id) - apps: List[Application] = list( - client.applications.list(filter="displayName eq '%s'" % registration_name) - ) - if len(apps) == 0: + app = get_application(display_name=registration_name) + + if not app: logger.info("No existing registration found. creating a new one") app = create_application_registration( onefuzz_instance_name, registration_name, approle, subscription_id ) else: - app = apps[0] logger.info( "Found existing application objectId '%s' - appid '%s'" - % (app.object_id, app.app_id) + % (app.object_id, app["appId"] ) ) - onefuzz_apps: List[Application] = list( - client.applications.list(filter="displayName eq '%s'" % onefuzz_instance_name) - ) + onefuzz_app = get_application(display_name=onefuzz_instance_name) - if len(onefuzz_apps) == 0: + if not(onefuzz_app): raise Exception("onefuzz app not found") - onefuzz_app = onefuzz_apps[0] - pre_authorized_applications = ( - onefuzz_app.pre_authorized_applications - if onefuzz_app.pre_authorized_applications is not None - else [] - ) + pre_authorized_applications = onefuzz_app["apiApplication"]["preAuthorizedApplications"] - if app.app_id not in [app.app_id for app in pre_authorized_applications]: - authorize_application(UUID(app.app_id), UUID(onefuzz_app.app_id)) + if app["appId"] not in [app["appId"] for app in pre_authorized_applications]: + authorize_application(UUID(app["appId"]), UUID(onefuzz_app["appId"])) password = create_application_credential(registration_name, subscription_id) @@ -188,14 +180,12 @@ def create_application_credential(application_name: str, subscription_id: str) - """Add a new password to the application registration""" logger.info("creating application credential for '%s'" % application_name) - client = get_graph_client(subscription_id) - apps: List[Application] = list( - client.applications.list(filter="displayName eq '%s'" % application_name) - ) + app = get_application(display_name=application_name) - app: Application = apps[0] + if not app: + raise Exception("app not found") - (_, password) = add_application_password(app.object_id, subscription_id) + (_, password) = add_application_password(app["objectId"], subscription_id) return str(password) @@ -204,36 +194,42 @@ def create_application_registration( ) -> Application: """Create an application registration""" - client = get_graph_client(subscription_id) - apps: List[Application] = list( - client.applications.list(filter="displayName eq '%s'" % onefuzz_instance_name) - ) + app = get_application(display_name=onefuzz_instance_name) + + if not app: + raise Exception("onefuzz app registration not found") - app = apps[0] resource_access = [ - ResourceAccess(id=role.id, type="Role") - for role in app.app_roles - if role.value == approle.value + { "id": "guid", "type": "string" } + for role in app["appRoles"] + if role["value"] == approle.value ] - params = ApplicationCreateParameters( - is_device_only_auth_supported=True, - display_name=name, - identifier_uris=[], - password_credentials=[], - required_resource_access=( - [ - RequiredResourceAccess( - resource_access=resource_access, - resource_app_id=app.app_id, - ) - ] - if len(resource_access) > 0 - else [] - ), - ) - registered_app: Application = client.applications.create(params) + params = { + "isDeviceOnlyAuthSupported": True, + "appRoles": [{"@odata.type": "microsoft.graph.appRole"}], + "displayName": name, + "publicClient": { + "redirectUris": ["https://%s.azurewebsites.net" % onefuzz_instance_name] + }, + "requiredResourceAccess": ( + [ + { + "resourceAccess": resource_access, + "resourceAppId":app["appId"], + } + ] + if len(resource_access) > 0 + else [] + ), + } + + registered_app:Dict = query_microsoft_graph( + method="POST", + resource="applications", + body=params, + ) logger.info("creating service principal") service_principal_params = ServicePrincipalCreateParameters( @@ -243,29 +239,20 @@ def create_application_registration( app_id=registered_app.app_id, ) - client.service_principals.create(service_principal_params) - - atttempts = 5 - while True: - if atttempts < 0: - raise Exception( - "Unable to create application registration, Please try again" - ) - - atttempts = atttempts - 1 - try: - time.sleep(5) - - update_param = ApplicationUpdateParameters( - reply_urls=["https://%s.azurewebsites.net" % onefuzz_instance_name] - ) - client.applications.patch(registered_app.object_id, update_param) + service_principal_params = { + "accountEnabled": True, + "appRoleAssignmentRequired": False, + "servicePrincipalType": "Application", + "appId": registered_app["appId"], + } - break - except Exception: - continue + query_microsoft_graph( + method="POST", + resource="applications/servicePrincipals", + body=service_principal_params, + ) - authorize_application(UUID(registered_app.app_id), UUID(app.app_id)) + authorize_application(UUID(registered_app["appId"]), UUID(app["appId"])) assign_app_role( onefuzz_instance_name, name, subscription_id, OnefuzzAppRole.ManagedNode ) @@ -333,11 +320,19 @@ def add_application_password_impl( return add_application_password_legacy(app_object_id, subscription_id) -def get_application(app_id: UUID) -> Optional[Any]: +def get_application(app_id: Optional[UUID]=None, display_name: Optional[str]=None) -> Optional[Any]: + filters = [] + if app_id: + filters.append("appId eq '%s'" % app_id) + if display_name: + filters.append("displayName eq '%s'" % display_name) + + filter_str = " and ".join(filters) + apps: Dict = query_microsoft_graph( method="GET", resource="applications", - params={"$filter": "appId eq '%s'" % app_id}, + params=filter_str, ) if len(apps["value"]) == 0: return None @@ -351,7 +346,7 @@ def authorize_application( permissions: List[str] = ["user_impersonation"], ) -> None: try: - onefuzz_app = get_application(onefuzz_app_id) + onefuzz_app = get_application(app_id = onefuzz_app_id) if onefuzz_app is None: logger.error("Application '%s' not found", onefuzz_app_id) return From 1ad0f5a28aae462398c569fb9e59c548ea7551d0 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 7 Jun 2021 14:13:15 -0700 Subject: [PATCH 002/103] add subscription id to query_microsoft_graph --- src/deployment/deploy.py | 4 ++-- src/deployment/registration.py | 29 ++++++++++++++++++++++------- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 11e1c95993..acfc914ae1 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -389,12 +389,12 @@ def try_sp_create() -> None: client.applications.patch( app.object_id, ApplicationUpdateParameters(identifier_uris=[url]) ) - set_app_audience(app.object_id, "AzureADMultipleOrgs") + set_app_audience(app.object_id, "AzureADMultipleOrgs", subscription_id=self.get_subscription_id()) elif ( not self.multi_tenant_domain and app.sign_in_audience == "AzureADMultipleOrgs" ): - set_app_audience(app.object_id, "AzureADMyOrg") + set_app_audience(app.object_id, "AzureADMyOrg", subscription_id=self.get_subscription_id()) url = "https://%s.azurewebsites.net" % self.application_name client.applications.patch( app.object_id, ApplicationUpdateParameters(identifier_uris=[url]) diff --git a/src/deployment/registration.py b/src/deployment/registration.py index 6791914ae8..0151006148 100644 --- a/src/deployment/registration.py +++ b/src/deployment/registration.py @@ -5,6 +5,7 @@ import argparse import logging +from re import S, sub import time import urllib.parse from datetime import datetime, timedelta @@ -53,10 +54,12 @@ def query_microsoft_graph( resource: str, params: Optional[Dict] = None, body: Optional[Dict] = None, + subscription: Optional[str] = None, ) -> Any: profile = get_cli_profile() (token_type, access_token, _), _, _ = profile.get_raw_token( - resource="https://graph.microsoft.com" + resource="https://graph.microsoft.com", + subscription=subscription ) url = urllib.parse.urljoin("https://graph.microsoft.com/v1.0/", resource) headers = { @@ -144,7 +147,7 @@ def register_application( ) -> ApplicationInfo: logger.info("retrieving the application registration %s" % registration_name) - app = get_application(display_name=registration_name) + app = get_application(display_name=registration_name, subscription_id=subscription_id) if not app: logger.info("No existing registration found. creating a new one") @@ -157,7 +160,7 @@ def register_application( % (app.object_id, app["appId"] ) ) - onefuzz_app = get_application(display_name=onefuzz_instance_name) + onefuzz_app = get_application(display_name=onefuzz_instance_name, subscription_id=subscription_id) if not(onefuzz_app): raise Exception("onefuzz app not found") @@ -194,7 +197,7 @@ def create_application_registration( ) -> Application: """Create an application registration""" - app = get_application(display_name=onefuzz_instance_name) + app = get_application(display_name=onefuzz_instance_name, subscription_id=subscription_id) if not app: raise Exception("onefuzz app registration not found") @@ -229,6 +232,7 @@ def create_application_registration( method="POST", resource="applications", body=params, + subscription=subscription_id ) logger.info("creating service principal") @@ -250,9 +254,10 @@ def create_application_registration( method="POST", resource="applications/servicePrincipals", body=service_principal_params, + subscription=subscription_id ) - authorize_application(UUID(registered_app["appId"]), UUID(app["appId"])) + authorize_application(UUID(registered_app["appId"]), UUID(app["appId"]), subscription_id) assign_app_role( onefuzz_instance_name, name, subscription_id, OnefuzzAppRole.ManagedNode ) @@ -314,13 +319,14 @@ def add_application_password_impl( method="POST", resource="applications/%s/addPassword" % app_object_id, body=password_request, + subscription=subscription_id ) return (str(key), password["secretText"]) except AuthenticationError: return add_application_password_legacy(app_object_id, subscription_id) -def get_application(app_id: Optional[UUID]=None, display_name: Optional[str]=None) -> Optional[Any]: +def get_application(app_id: Optional[UUID]=None, display_name: Optional[str]=None, subscription_id: Optional[str] = None) -> Optional[Any]: filters = [] if app_id: filters.append("appId eq '%s'" % app_id) @@ -333,6 +339,7 @@ def get_application(app_id: Optional[UUID]=None, display_name: Optional[str]=Non method="GET", resource="applications", params=filter_str, + subscription=subscription_id ) if len(apps["value"]) == 0: return None @@ -344,6 +351,7 @@ def authorize_application( registration_app_id: UUID, onefuzz_app_id: UUID, permissions: List[str] = ["user_impersonation"], + subscription_id: Optional[str] = None ) -> None: try: onefuzz_app = get_application(app_id = onefuzz_app_id) @@ -384,6 +392,7 @@ def add_preauthorized_app() -> None: "preAuthorizedApplications": preAuthorizedApplications.to_list() } }, + subscription=subscription_id ) retry(add_preauthorized_app, "authorize application") @@ -501,6 +510,7 @@ def assign_app_role( "$filter": "displayName eq '%s'" % onefuzz_instance_name, "$select": "appId", }, + subscription=subscription_id ) if len(onefuzz_service_appId["value"]) == 0: raise Exception("onefuzz app registration not found") @@ -509,6 +519,7 @@ def assign_app_role( method="GET", resource="servicePrincipals", params={"$filter": "appId eq '%s'" % appId}, + subscription=subscription_id ) if len(onefuzz_service_principals["value"]) == 0: @@ -518,6 +529,7 @@ def assign_app_role( method="GET", resource="servicePrincipals", params={"$filter": "displayName eq '%s'" % application_name}, + subscription=subscription_id ) if len(scaleset_service_principals["value"]) == 0: raise Exception("scaleset service principal not found") @@ -537,6 +549,7 @@ def assign_app_role( method="GET", resource="servicePrincipals/%s/appRoleAssignments" % scaleset_service_principal["id"], + subscription=subscription_id ) # check if the role is already assigned @@ -553,6 +566,7 @@ def assign_app_role( "resourceId": onefuzz_service_principal["id"], "appRoleId": managed_node_role["id"], }, + subscription=subscription_id ) except AuthenticationError: assign_app_role_manually( @@ -560,7 +574,7 @@ def assign_app_role( ) -def set_app_audience(objectId: str, audience: str) -> None: +def set_app_audience(objectId: str, audience: str, subscription_id: Optional[str]=None) -> None: # typical audience values: AzureADMyOrg, AzureADMultipleOrgs http_body = {"signInAudience": audience} try: @@ -568,6 +582,7 @@ def set_app_audience(objectId: str, audience: str) -> None: method="PATCH", resource="applications/%s" % objectId, body=http_body, + subscription=subscription_id ) except GraphQueryError: query = ( From ac0f4005f6f0523ae8ed1f4c23e881d642e7e5b2 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 8 Jun 2021 14:06:16 -0700 Subject: [PATCH 003/103] migrating remaingin references --- .../__app__/onefuzzlib/azure/creds.py | 49 ++++++++++++++++--- src/deployment/deploy.py | 7 +-- src/deployment/registration.py | 12 +++-- 3 files changed, 55 insertions(+), 13 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/azure/creds.py b/src/api-service/__app__/onefuzzlib/azure/creds.py index e3669f1317..2dc0a25440 100644 --- a/src/api-service/__app__/onefuzzlib/azure/creds.py +++ b/src/api-service/__app__/onefuzzlib/azure/creds.py @@ -4,13 +4,16 @@ # Licensed under the MIT License. import os -from typing import Any, List +import urllib.parse +from typing import Any, Dict, List, Optional from uuid import UUID +import requests from azure.graphrbac import GraphRbacManagementClient from azure.graphrbac.models import CheckGroupMembershipParameters from azure.identity import DefaultAzureCredential from azure.keyvault.secrets import SecretClient +from azure.mgmt import subscription from azure.mgmt.resource import ResourceManagementClient from azure.mgmt.subscription import SubscriptionClient from memoization import cached @@ -101,13 +104,47 @@ def get_graph_client() -> GraphRbacManagementClient: return GraphRbacManagementClient(get_msi(), get_subscription()) +def query_microsoft_graph( + method: str, + resource: str, + params: Optional[Dict] = None, + body: Optional[Dict] = None, +) -> Any: + auth = get_msi() + access_token = auth.token["access_token"] + token_type = auth.token["token_type"] + + url = urllib.parse.urljoin("https://graph.microsoft.com/v1.0/", resource) + headers = { + "Authorization": "%s %s" % (token_type, access_token), + "Content-Type": "application/json", + } + response = requests.request( + method=method, url=url, headers=headers, params=params, json=body + ) + + response.status_code + + if 200 <= response.status_code < 300: + try: + return response.json() + except ValueError: + return None + else: + error_text = str(response.content, encoding="utf-8", errors="backslashreplace") + raise Exception( + "request did not succeed: HTTP %s - %s" + % (response.status_code, error_text), + response.status_code, + ) + + def is_member_of(group_id: str, member_id: str) -> bool: - client = get_graph_client() - return bool( - client.groups.is_member_of( - CheckGroupMembershipParameters(group_id=group_id, member_id=member_id) - ).value + body = {"groupIds": [group_id]} + response = query_microsoft_graph( + method="POST", resource=f"users/{member_id}/checkMemberGroups", body=body ) + return group_id in response["value"] @cached diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index acfc914ae1..0de31f73a5 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -67,7 +67,7 @@ add_application_password, assign_app_role, authorize_application, - get_graph_client, + get_tenant_id, register_application, set_app_audience, update_pool_registration, @@ -437,10 +437,11 @@ def try_sp_create() -> None: if self.multi_tenant_domain: authority = COMMON_AUTHORITY else: - onefuzz_client = get_graph_client(self.get_subscription_id()) + + tenant_id = get_tenant_id(self.get_subscription_id()) authority = ( "https://login.microsoftonline.com/%s" - % onefuzz_client.config.tenant_id + % tenant_id ) self.cli_config = { "client_id": onefuzz_cli_app.app_id, diff --git a/src/deployment/registration.py b/src/deployment/registration.py index 0151006148..0ad6958c44 100644 --- a/src/deployment/registration.py +++ b/src/deployment/registration.py @@ -22,12 +22,8 @@ from azure.graphrbac import GraphRbacManagementClient from azure.graphrbac.models import ( Application, - ApplicationCreateParameters, - ApplicationUpdateParameters, AppRole, PasswordCredential, - RequiredResourceAccess, - ResourceAccess, ServicePrincipal, ServicePrincipalCreateParameters, ) @@ -85,6 +81,14 @@ def query_microsoft_graph( response.status_code, ) +def get_tenant_id(subscription_id: Optional[str]=None) -> str: + result = query_microsoft_graph( + method="GET", + resource="organization", + subscription=subscription_id + ) + result["value"]["id"] + OperationResult = TypeVar("OperationResult") From 4902528dbd55d88ca90f4d1a8512eb02e0c191f6 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 8 Jun 2021 15:06:42 -0700 Subject: [PATCH 004/103] formatting --- src/deployment/deploy.py | 17 ++-- src/deployment/registration.py | 151 +++++++++++++++++++-------------- 2 files changed, 96 insertions(+), 72 deletions(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 0de31f73a5..36d2acd1cc 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -389,12 +389,20 @@ def try_sp_create() -> None: client.applications.patch( app.object_id, ApplicationUpdateParameters(identifier_uris=[url]) ) - set_app_audience(app.object_id, "AzureADMultipleOrgs", subscription_id=self.get_subscription_id()) + set_app_audience( + app.object_id, + "AzureADMultipleOrgs", + subscription_id=self.get_subscription_id(), + ) elif ( not self.multi_tenant_domain and app.sign_in_audience == "AzureADMultipleOrgs" ): - set_app_audience(app.object_id, "AzureADMyOrg", subscription_id=self.get_subscription_id()) + set_app_audience( + app.object_id, + "AzureADMyOrg", + subscription_id=self.get_subscription_id(), + ) url = "https://%s.azurewebsites.net" % self.application_name client.applications.patch( app.object_id, ApplicationUpdateParameters(identifier_uris=[url]) @@ -439,10 +447,7 @@ def try_sp_create() -> None: else: tenant_id = get_tenant_id(self.get_subscription_id()) - authority = ( - "https://login.microsoftonline.com/%s" - % tenant_id - ) + authority = "https://login.microsoftonline.com/%s" % tenant_id self.cli_config = { "client_id": onefuzz_cli_app.app_id, "authority": authority, diff --git a/src/deployment/registration.py b/src/deployment/registration.py index 0ad6958c44..8ae5207783 100644 --- a/src/deployment/registration.py +++ b/src/deployment/registration.py @@ -5,16 +5,14 @@ import argparse import logging -from re import S, sub import time import urllib.parse from datetime import datetime, timedelta from enum import Enum -from typing import Any, Callable, Dict, List, NamedTuple, Optional, Tuple, TypeVar +from re import S, sub +from typing import Any, Callable, Dict, List, NamedTuple, Optional, Tuple, TypeVar, cast from uuid import UUID, uuid4 -# https://github.com/microsoftgraph/msgraph-sdk-python-core - import requests from azure.cli.core.azclierror import AuthenticationError from azure.common.client_factory import get_client_from_cli_profile @@ -30,6 +28,9 @@ from functional import seq from msrest.serialization import TZ_UTC +# https://github.com/microsoftgraph/msgraph-sdk-python-core + + FIX_URL = ( "https://ms.portal.azure.com/#blade/Microsoft_AAD_RegisteredApps/" "ApplicationMenuBlade/ProtectAnAPI/appId/%s/isMSAApp/" @@ -54,8 +55,7 @@ def query_microsoft_graph( ) -> Any: profile = get_cli_profile() (token_type, access_token, _), _, _ = profile.get_raw_token( - resource="https://graph.microsoft.com", - subscription=subscription + resource="https://graph.microsoft.com", subscription=subscription ) url = urllib.parse.urljoin("https://graph.microsoft.com/v1.0/", resource) headers = { @@ -81,13 +81,12 @@ def query_microsoft_graph( response.status_code, ) -def get_tenant_id(subscription_id: Optional[str]=None) -> str: + +def get_tenant_id(subscription_id: Optional[str] = None) -> str: result = query_microsoft_graph( - method="GET", - resource="organization", - subscription=subscription_id - ) - result["value"]["id"] + method="GET", resource="organization", subscription=subscription_id + ) + return cast(str, result["value"]["id"]) OperationResult = TypeVar("OperationResult") @@ -151,7 +150,9 @@ def register_application( ) -> ApplicationInfo: logger.info("retrieving the application registration %s" % registration_name) - app = get_application(display_name=registration_name, subscription_id=subscription_id) + app = get_application( + display_name=registration_name, subscription_id=subscription_id + ) if not app: logger.info("No existing registration found. creating a new one") @@ -161,25 +162,30 @@ def register_application( else: logger.info( "Found existing application objectId '%s' - appid '%s'" - % (app.object_id, app["appId"] ) + % (app.object_id, app["appId"]) ) - onefuzz_app = get_application(display_name=onefuzz_instance_name, subscription_id=subscription_id) + onefuzz_app = get_application( + display_name=onefuzz_instance_name, subscription_id=subscription_id + ) - if not(onefuzz_app): + if not (onefuzz_app): raise Exception("onefuzz app not found") - pre_authorized_applications = onefuzz_app["apiApplication"]["preAuthorizedApplications"] + pre_authorized_applications = onefuzz_app["apiApplication"][ + "preAuthorizedApplications" + ] if app["appId"] not in [app["appId"] for app in pre_authorized_applications]: authorize_application(UUID(app["appId"]), UUID(onefuzz_app["appId"])) password = create_application_credential(registration_name, subscription_id) + tenant_id = get_tenant_id(subscription_id=subscription_id) return ApplicationInfo( client_id=app.app_id, client_secret=password, - authority=("https://login.microsoftonline.com/%s" % client.config.tenant_id), + authority=("https://login.microsoftonline.com/%s" % tenant_id), ) @@ -201,42 +207,43 @@ def create_application_registration( ) -> Application: """Create an application registration""" - app = get_application(display_name=onefuzz_instance_name, subscription_id=subscription_id) + app = get_application( + display_name=onefuzz_instance_name, subscription_id=subscription_id + ) if not app: raise Exception("onefuzz app registration not found") resource_access = [ - { "id": "guid", "type": "string" } + {"id": "guid", "type": "string"} for role in app["appRoles"] if role["value"] == approle.value ] - params = { - "isDeviceOnlyAuthSupported": True, - "appRoles": [{"@odata.type": "microsoft.graph.appRole"}], - "displayName": name, - "publicClient": { - "redirectUris": ["https://%s.azurewebsites.net" % onefuzz_instance_name] - }, - "requiredResourceAccess": ( - [ - { - "resourceAccess": resource_access, - "resourceAppId":app["appId"], - } - ] - if len(resource_access) > 0 - else [] - ), - } + "isDeviceOnlyAuthSupported": True, + "appRoles": [{"@odata.type": "microsoft.graph.appRole"}], + "displayName": name, + "publicClient": { + "redirectUris": ["https://%s.azurewebsites.net" % onefuzz_instance_name] + }, + "requiredResourceAccess": ( + [ + { + "resourceAccess": resource_access, + "resourceAppId": app["appId"], + } + ] + if len(resource_access) > 0 + else [] + ), + } - registered_app:Dict = query_microsoft_graph( + registered_app: Dict = query_microsoft_graph( method="POST", resource="applications", body=params, - subscription=subscription_id + subscription=subscription_id, ) logger.info("creating service principal") @@ -244,24 +251,28 @@ def create_application_registration( account_enabled=True, app_role_assignment_required=False, service_principal_type="Application", - app_id=registered_app.app_id, + app_id=registered_app["appId"], ) service_principal_params = { - "accountEnabled": True, - "appRoleAssignmentRequired": False, - "servicePrincipalType": "Application", - "appId": registered_app["appId"], - } + "accountEnabled": True, + "appRoleAssignmentRequired": False, + "servicePrincipalType": "Application", + "appId": registered_app["appId"], + } query_microsoft_graph( - method="POST", - resource="applications/servicePrincipals", - body=service_principal_params, - subscription=subscription_id - ) + method="POST", + resource="applications/servicePrincipals", + body=service_principal_params, + subscription=subscription_id, + ) - authorize_application(UUID(registered_app["appId"]), UUID(app["appId"]), subscription_id) + authorize_application( + UUID(registered_app["appId"]), + UUID(app["appId"]), + subscription_id=subscription_id, + ) assign_app_role( onefuzz_instance_name, name, subscription_id, OnefuzzAppRole.ManagedNode ) @@ -323,14 +334,18 @@ def add_application_password_impl( method="POST", resource="applications/%s/addPassword" % app_object_id, body=password_request, - subscription=subscription_id + subscription=subscription_id, ) return (str(key), password["secretText"]) except AuthenticationError: return add_application_password_legacy(app_object_id, subscription_id) -def get_application(app_id: Optional[UUID]=None, display_name: Optional[str]=None, subscription_id: Optional[str] = None) -> Optional[Any]: +def get_application( + app_id: Optional[UUID] = None, + display_name: Optional[str] = None, + subscription_id: Optional[str] = None, +) -> Optional[Any]: filters = [] if app_id: filters.append("appId eq '%s'" % app_id) @@ -342,8 +357,10 @@ def get_application(app_id: Optional[UUID]=None, display_name: Optional[str]=Non apps: Dict = query_microsoft_graph( method="GET", resource="applications", - params=filter_str, - subscription=subscription_id + params={ + "$filter": filter_str, + }, + subscription=subscription_id, ) if len(apps["value"]) == 0: return None @@ -355,10 +372,10 @@ def authorize_application( registration_app_id: UUID, onefuzz_app_id: UUID, permissions: List[str] = ["user_impersonation"], - subscription_id: Optional[str] = None + subscription_id: Optional[str] = None, ) -> None: try: - onefuzz_app = get_application(app_id = onefuzz_app_id) + onefuzz_app = get_application(app_id=onefuzz_app_id) if onefuzz_app is None: logger.error("Application '%s' not found", onefuzz_app_id) return @@ -396,7 +413,7 @@ def add_preauthorized_app() -> None: "preAuthorizedApplications": preAuthorizedApplications.to_list() } }, - subscription=subscription_id + subscription=subscription_id, ) retry(add_preauthorized_app, "authorize application") @@ -514,7 +531,7 @@ def assign_app_role( "$filter": "displayName eq '%s'" % onefuzz_instance_name, "$select": "appId", }, - subscription=subscription_id + subscription=subscription_id, ) if len(onefuzz_service_appId["value"]) == 0: raise Exception("onefuzz app registration not found") @@ -523,7 +540,7 @@ def assign_app_role( method="GET", resource="servicePrincipals", params={"$filter": "appId eq '%s'" % appId}, - subscription=subscription_id + subscription=subscription_id, ) if len(onefuzz_service_principals["value"]) == 0: @@ -533,7 +550,7 @@ def assign_app_role( method="GET", resource="servicePrincipals", params={"$filter": "displayName eq '%s'" % application_name}, - subscription=subscription_id + subscription=subscription_id, ) if len(scaleset_service_principals["value"]) == 0: raise Exception("scaleset service principal not found") @@ -553,7 +570,7 @@ def assign_app_role( method="GET", resource="servicePrincipals/%s/appRoleAssignments" % scaleset_service_principal["id"], - subscription=subscription_id + subscription=subscription_id, ) # check if the role is already assigned @@ -570,7 +587,7 @@ def assign_app_role( "resourceId": onefuzz_service_principal["id"], "appRoleId": managed_node_role["id"], }, - subscription=subscription_id + subscription=subscription_id, ) except AuthenticationError: assign_app_role_manually( @@ -578,7 +595,9 @@ def assign_app_role( ) -def set_app_audience(objectId: str, audience: str, subscription_id: Optional[str]=None) -> None: +def set_app_audience( + objectId: str, audience: str, subscription_id: Optional[str] = None +) -> None: # typical audience values: AzureADMyOrg, AzureADMultipleOrgs http_body = {"signInAudience": audience} try: @@ -586,7 +605,7 @@ def set_app_audience(objectId: str, audience: str, subscription_id: Optional[str method="PATCH", resource="applications/%s" % objectId, body=http_body, - subscription=subscription_id + subscription=subscription_id, ) except GraphQueryError: query = ( From a4379c8346375d7e7042b18a60d0ac08a71e5147 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 8 Jun 2021 15:54:15 -0700 Subject: [PATCH 005/103] adding missing dependencies --- src/cli/requirements.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/cli/requirements.txt b/src/cli/requirements.txt index 9259bf2a5d..1703a8e64f 100644 --- a/src/cli/requirements.txt +++ b/src/cli/requirements.txt @@ -18,3 +18,5 @@ cryptography<3.4,>=3.2 pyjwt==1.7.1 # onefuzztypes version is set during build onefuzztypes==0.0.0 +six==1.16.0 +requests==2.25.1 \ No newline at end of file From e1fa0ad9cca32827bfb70079dedec4a57e2ee5dd Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Wed, 9 Jun 2021 12:12:15 -0700 Subject: [PATCH 006/103] flake fix --- src/api-service/__app__/onefuzzlib/azure/creds.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/azure/creds.py b/src/api-service/__app__/onefuzzlib/azure/creds.py index 2dc0a25440..5f89d5b7bb 100644 --- a/src/api-service/__app__/onefuzzlib/azure/creds.py +++ b/src/api-service/__app__/onefuzzlib/azure/creds.py @@ -10,10 +10,8 @@ import requests from azure.graphrbac import GraphRbacManagementClient -from azure.graphrbac.models import CheckGroupMembershipParameters from azure.identity import DefaultAzureCredential from azure.keyvault.secrets import SecretClient -from azure.mgmt import subscription from azure.mgmt.resource import ResourceManagementClient from azure.mgmt.subscription import SubscriptionClient from memoization import cached From c130d6d04718dcf3affbef0477d8f9b1e4aeb98e Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Wed, 9 Jun 2021 13:28:31 -0700 Subject: [PATCH 007/103] fix get_tenant_id --- src/deployment/registration.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/deployment/registration.py b/src/deployment/registration.py index 8ae5207783..06ad5afb52 100644 --- a/src/deployment/registration.py +++ b/src/deployment/registration.py @@ -86,7 +86,7 @@ def get_tenant_id(subscription_id: Optional[str] = None) -> str: result = query_microsoft_graph( method="GET", resource="organization", subscription=subscription_id ) - return cast(str, result["value"]["id"]) + return cast(str, result["value"][0]["id"]) OperationResult = TypeVar("OperationResult") From 681867e35ff97cba4af4dc1c0e464b33e543a3c9 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Wed, 9 Jun 2021 18:39:36 -0700 Subject: [PATCH 008/103] cleanup --- src/cli/requirements.txt | 4 +--- src/deployment/registration.py | 2 -- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/src/cli/requirements.txt b/src/cli/requirements.txt index 1703a8e64f..e06400e77b 100644 --- a/src/cli/requirements.txt +++ b/src/cli/requirements.txt @@ -17,6 +17,4 @@ cryptography<3.4,>=3.2 # PyJWT needs to be pinned to the version used by azure-cli-core pyjwt==1.7.1 # onefuzztypes version is set during build -onefuzztypes==0.0.0 -six==1.16.0 -requests==2.25.1 \ No newline at end of file +onefuzztypes==0.0.0 \ No newline at end of file diff --git a/src/deployment/registration.py b/src/deployment/registration.py index 06ad5afb52..58b046e1e6 100644 --- a/src/deployment/registration.py +++ b/src/deployment/registration.py @@ -28,8 +28,6 @@ from functional import seq from msrest.serialization import TZ_UTC -# https://github.com/microsoftgraph/msgraph-sdk-python-core - FIX_URL = ( "https://ms.portal.azure.com/#blade/Microsoft_AAD_RegisteredApps/" From 82300244b1ef54fe5390417447e852f3df50edce Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 14 Jun 2021 08:44:39 -0700 Subject: [PATCH 009/103] formatting --- src/deployment/registration.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/deployment/registration.py b/src/deployment/registration.py index 58b046e1e6..1a0c5adcb9 100644 --- a/src/deployment/registration.py +++ b/src/deployment/registration.py @@ -28,7 +28,6 @@ from functional import seq from msrest.serialization import TZ_UTC - FIX_URL = ( "https://ms.portal.azure.com/#blade/Microsoft_AAD_RegisteredApps/" "ApplicationMenuBlade/ProtectAnAPI/appId/%s/isMSAApp/" From 0403485d474c236ecc84bca752e3e2fb86ac9e03 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Wed, 16 Jun 2021 12:31:10 -0700 Subject: [PATCH 010/103] migrate application creation in deploy.py --- .../__app__/onefuzzlib/azure/creds.py | 6 - src/deployment/deploy.py | 183 ++++++++------ src/deployment/registration.py | 229 +++++------------- 3 files changed, 164 insertions(+), 254 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/azure/creds.py b/src/api-service/__app__/onefuzzlib/azure/creds.py index 5f89d5b7bb..68a75db943 100644 --- a/src/api-service/__app__/onefuzzlib/azure/creds.py +++ b/src/api-service/__app__/onefuzzlib/azure/creds.py @@ -9,7 +9,6 @@ from uuid import UUID import requests -from azure.graphrbac import GraphRbacManagementClient from azure.identity import DefaultAzureCredential from azure.keyvault.secrets import SecretClient from azure.mgmt.resource import ResourceManagementClient @@ -97,11 +96,6 @@ def get_regions() -> List[Region]: return sorted([Region(x.name) for x in locations]) -@cached -def get_graph_client() -> GraphRbacManagementClient: - return GraphRbacManagementClient(get_msi(), get_subscription()) - - def query_microsoft_graph( method: str, resource: str, diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 36d2acd1cc..3e641df05c 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -23,7 +23,6 @@ from azure.common.credentials import get_cli_profile from azure.core.exceptions import ResourceExistsError from azure.cosmosdb.table.tableservice import TableService -from azure.graphrbac import GraphRbacManagementClient from azure.graphrbac.models import ( Application, ApplicationCreateParameters, @@ -68,9 +67,11 @@ assign_app_role, authorize_application, get_tenant_id, + query_microsoft_graph, register_application, set_app_audience, update_pool_registration, + get_application ) # Found by manually assigning the User.Read permission to application @@ -260,43 +261,34 @@ def setup_rbac(self) -> None: logger.info("using existing client application") return - client = get_client_from_cli_profile( - GraphRbacManagementClient, subscription_id=self.get_subscription_id() + + app = get_application( + display_name=self.application_name, subscription_id=self.get_subscription_id() ) - logger.info("checking if RBAC already exists") - try: - existing = list( - client.applications.list( - filter="displayName eq '%s'" % self.application_name - ) - ) - except GraphErrorException: - logger.error("unable to query RBAC. Provide client_id and client_secret") - sys.exit(1) app_roles = [ - AppRole( - allowed_member_types=["Application"], - display_name=OnefuzzAppRole.CliClient.value, - id=str(uuid.uuid4()), - is_enabled=True, - description="Allows access from the CLI.", - value=OnefuzzAppRole.CliClient.value, - ), - AppRole( - allowed_member_types=["Application"], - display_name=OnefuzzAppRole.ManagedNode.value, - id=str(uuid.uuid4()), - is_enabled=True, - description="Allow access from a lab machine.", - value=OnefuzzAppRole.ManagedNode.value, - ), + { + "allowedMemberTypes": ["Application"], + "description": "Allows access from the CLI.", + "displayName": OnefuzzAppRole.CliClient.value, + "id": str(uuid.uuid4()), + "isEnabled": True, + "value": OnefuzzAppRole.CliClient.value + }, + { + "allowedMemberTypes": ["Application"], + "description": "Allow access from a lab machine.", + "displayName": OnefuzzAppRole.ManagedNode.value, + "id": str(uuid.uuid4()), + "isEnabled": True, + "value": OnefuzzAppRole.ManagedNode.value, + } ] - app: Optional[Application] = None + # app: Optional[Application] = None - if not existing: + if not app: logger.info("creating Application registration") if self.multi_tenant_domain: @@ -307,37 +299,49 @@ def setup_rbac(self) -> None: else: url = "https://%s.azurewebsites.net" % self.application_name - params = ApplicationCreateParameters( - display_name=self.application_name, - identifier_uris=[url], - reply_urls=[url + "/.auth/login/aad/callback"], - optional_claims=OptionalClaims(id_token=[], access_token=[]), - required_resource_access=[ - RequiredResourceAccess( - resource_access=[ - ResourceAccess(id=USER_READ_PERMISSION, type="Scope") - ], - resource_app_id=MICROSOFT_GRAPH_APP_ID, - ) - ], - app_roles=app_roles, - ) - - app = client.applications.create(params) + params = { + "displayName": self.application_name, + "identifierUris": [url], + "appRoles": app_roles, + "requiredResourceAccess": + [ + { + "resourceAccess": { + "id": USER_READ_PERMISSION, + "type": "Scope" + }, + "resourceAppId": MICROSOFT_GRAPH_APP_ID, + } + ], + } + + + app:Dict = query_microsoft_graph( + method="POST", + resource="applications", + body=params, + subscription=self.get_subscription_id(), + ) logger.info("creating service principal") - service_principal_params = ServicePrincipalCreateParameters( - account_enabled=True, - app_role_assignment_required=False, - service_principal_type="Application", - app_id=app.app_id, - ) + + service_principal_params = { + "accountEnabled": True, + "appRoleAssignmentRequired": False, + "servicePrincipalType": "Application", + "appId": app["appId"], + } def try_sp_create() -> None: error: Optional[Exception] = None for _ in range(10): try: - client.service_principals.create(service_principal_params) + query_microsoft_graph( + method="POST", + resource="applications/servicePrincipals", + body=service_principal_params, + subscription=self.get_subscription_id(), + ) return except GraphErrorException as err: # work around timing issue when creating service principal @@ -360,25 +364,36 @@ def try_sp_create() -> None: try_sp_create() else: - app = existing[0] - existing_role_values = [app_role.value for app_role in app.app_roles] + + existing_role_values = [app_role["value"] for app_role in app["appRoles"]] has_missing_roles = any( - [role.value not in existing_role_values for role in app_roles] + [role["value"] not in existing_role_values for role in app_roles] ) if has_missing_roles: # disabling the existing app role first to allow the update # this is a requirement to update the application roles - for role in app.app_roles: - role.is_enabled = False + for role in app["appRoles"]: + role["isEnabled"] = False + - client.applications.patch( - app.object_id, ApplicationUpdateParameters(app_roles=app.app_roles) + query_microsoft_graph( + method="PATCH", + resource=f"applications/{app['id']}", + body={ + "appRoles": app["AppRoles"] + }, + subscription=self.get_subscription_id(), ) # overriding the list of app roles - client.applications.patch( - app.object_id, ApplicationUpdateParameters(app_roles=app_roles) + query_microsoft_graph( + method="PATCH", + resource=f"applications/{app['id']}", + body={ + "appRoles": app_roles + }, + subscription=self.get_subscription_id(), ) if self.multi_tenant_domain and app.sign_in_audience == "AzureADMyOrg": @@ -386,9 +401,15 @@ def try_sp_create() -> None: self.multi_tenant_domain, self.application_name, ) - client.applications.patch( - app.object_id, ApplicationUpdateParameters(identifier_uris=[url]) + query_microsoft_graph( + method="PATCH", + resource=f"applications/{app['id']}", + body={ + "identifierUris": [url] + }, + subscription=self.get_subscription_id(), ) + set_app_audience( app.object_id, "AzureADMultipleOrgs", @@ -404,22 +425,24 @@ def try_sp_create() -> None: subscription_id=self.get_subscription_id(), ) url = "https://%s.azurewebsites.net" % self.application_name - client.applications.patch( - app.object_id, ApplicationUpdateParameters(identifier_uris=[url]) + query_microsoft_graph( + method="PATCH", + resource=f"applications/{app['id']}", + body={ + "identifierUris": [url] + }, + subscription=self.get_subscription_id(), ) else: logger.debug("No change to App Registration signInAudence setting") - creds = list(client.applications.list_password_credentials(app.object_id)) - client.applications.update_password_credentials(app.object_id, creds) (password_id, password) = self.create_password(app.object_id) - cli_app = list( - client.applications.list(filter="appId eq '%s'" % ONEFUZZ_CLI_APP) - ) - if len(cli_app) == 0: + cli_app = get_application(app_id=ONEFUZZ_CLI_APP, subscription_id=self.get_subscription_id()) + + if not cli_app: logger.info( "Could not find the default CLI application under the current " "subscription, creating a new one" @@ -440,8 +463,8 @@ def try_sp_create() -> None: } else: - onefuzz_cli_app = cli_app[0] - authorize_application(uuid.UUID(onefuzz_cli_app.app_id), app.app_id) + onefuzz_cli_app = cli_app + authorize_application(uuid.UUID(onefuzz_cli_app["appId"]), app["appId"]) if self.multi_tenant_domain: authority = COMMON_AUTHORITY else: @@ -449,18 +472,18 @@ def try_sp_create() -> None: tenant_id = get_tenant_id(self.get_subscription_id()) authority = "https://login.microsoftonline.com/%s" % tenant_id self.cli_config = { - "client_id": onefuzz_cli_app.app_id, + "client_id": onefuzz_cli_app["appId"], "authority": authority, } - self.results["client_id"] = app.app_id + self.results["client_id"] = app["appId"] self.results["client_secret"] = password # Log `client_secret` for consumption by CI. if self.log_service_principal: - logger.info("client_id: %s client_secret: %s", app.app_id, password) + logger.info("client_id: %s client_secret: %s", app["appId"], password) else: - logger.debug("client_id: %s client_secret: %s", app.app_id, password) + logger.debug("client_id: %s client_secret: %s", app["appId"], password) def deploy_template(self) -> None: logger.info("deploying arm template: %s", self.arm_template) diff --git a/src/deployment/registration.py b/src/deployment/registration.py index 1a0c5adcb9..2f71d3622a 100644 --- a/src/deployment/registration.py +++ b/src/deployment/registration.py @@ -17,7 +17,6 @@ from azure.cli.core.azclierror import AuthenticationError from azure.common.client_factory import get_client_from_cli_profile from azure.common.credentials import get_cli_profile -from azure.graphrbac import GraphRbacManagementClient from azure.graphrbac.models import ( Application, AppRole, @@ -120,14 +119,6 @@ def retry( raise Exception(f"failed '{description}'") -def get_graph_client(subscription_id: str) -> GraphRbacManagementClient: - client: GraphRbacManagementClient = get_client_from_cli_profile( - GraphRbacManagementClient, - subscription_id=subscription_id, - ) - return client - - class ApplicationInfo(NamedTuple): client_id: UUID client_secret: str @@ -219,7 +210,6 @@ def create_application_registration( params = { "isDeviceOnlyAuthSupported": True, - "appRoles": [{"@odata.type": "microsoft.graph.appRole"}], "displayName": name, "publicClient": { "redirectUris": ["https://%s.azurewebsites.net" % onefuzz_instance_name] @@ -244,12 +234,6 @@ def create_application_registration( ) logger.info("creating service principal") - service_principal_params = ServicePrincipalCreateParameters( - account_enabled=True, - app_role_assignment_required=False, - service_principal_type="Application", - app_id=registered_app["appId"], - ) service_principal_params = { "accountEnabled": True, @@ -289,27 +273,6 @@ def create_password() -> Tuple[str, str]: return retry(create_password, "create password") -def add_application_password_legacy( - app_object_id: UUID, subscription_id: str -) -> Tuple[str, str]: - key = str(uuid4()) - password = str(uuid4()) - - client = get_graph_client(subscription_id) - password_cred = [ - PasswordCredential( - start_date="%s" % datetime.now(TZ_UTC).strftime("%Y-%m-%dT%H:%M.%fZ"), - end_date="%s" - % (datetime.now(TZ_UTC) + timedelta(days=365)).strftime( - "%Y-%m-%dT%H:%M.%fZ" - ), - key_id=key, - value=password, - ) - ] - client.applications.update_password_credentials(app_object_id, password_cred) - return (key, password) - def add_application_password_impl( app_object_id: UUID, subscription_id: str @@ -326,16 +289,14 @@ def add_application_password_impl( } } - try: - password: Dict = query_microsoft_graph( - method="POST", - resource="applications/%s/addPassword" % app_object_id, - body=password_request, - subscription=subscription_id, - ) - return (str(key), password["secretText"]) - except AuthenticationError: - return add_application_password_legacy(app_object_id, subscription_id) + + password: Dict = query_microsoft_graph( + method="POST", + resource="applications/%s/addPassword" % app_object_id, + body=password_request, + subscription=subscription_id, + ) + return (str(key), password["secretText"]) def get_application( @@ -446,151 +407,83 @@ def update_pool_registration(onefuzz_instance_name: str, subscription_id: str) - subscription_id, ) - -def assign_app_role_manually( +def assign_app_role( onefuzz_instance_name: str, application_name: str, subscription_id: str, app_role: OnefuzzAppRole, ) -> None: + """ + Allows the application to access the service by assigning + their managed identity to the provided App Role + """ - client = get_graph_client(subscription_id) - apps: List[Application] = list( - client.applications.list(filter="displayName eq '%s'" % onefuzz_instance_name) + onefuzz_service_appId = query_microsoft_graph( + method="GET", + resource="applications", + params={ + "$filter": "displayName eq '%s'" % onefuzz_instance_name, + "$select": "appId", + }, + subscription=subscription_id, ) - - if not apps: + if len(onefuzz_service_appId["value"]) == 0: raise Exception("onefuzz app registration not found") - - app = apps[0] - appId = app.app_id - - onefuzz_service_principals: List[ServicePrincipal] = list( - client.service_principals.list(filter="appId eq '%s'" % appId) + appId = onefuzz_service_appId["value"][0]["appId"] + onefuzz_service_principals = query_microsoft_graph( + method="GET", + resource="servicePrincipals", + params={"$filter": "appId eq '%s'" % appId}, + subscription=subscription_id, ) - if not onefuzz_service_principals: + if len(onefuzz_service_principals["value"]) == 0: raise Exception("onefuzz app service principal not found") - onefuzz_service_principal = onefuzz_service_principals[0] - - scaleset_service_principals: List[ServicePrincipal] = list( - client.service_principals.list(filter="displayName eq '%s'" % application_name) + onefuzz_service_principal = onefuzz_service_principals["value"][0] + scaleset_service_principals = query_microsoft_graph( + method="GET", + resource="servicePrincipals", + params={"$filter": "displayName eq '%s'" % application_name}, + subscription=subscription_id, ) - - if not scaleset_service_principals: + if len(scaleset_service_principals["value"]) == 0: raise Exception("scaleset service principal not found") - scaleset_service_principal = scaleset_service_principals[0] - - scaleset_service_principal.app_roles - app_roles: List[AppRole] = [ - role for role in app.app_roles if role.value == app_role.value - ] + scaleset_service_principal = scaleset_service_principals["value"][0] + managed_node_role = ( + seq(onefuzz_service_principal["appRoles"]) + .filter(lambda x: x["value"] == app_role.value) + .head_option() + ) - if not app_roles: + if not managed_node_role: raise Exception( - "ManagedNode role not found in the OneFuzz application " + f"{app_role.value} role not found in the OneFuzz application " "registration. Please redeploy the instance" ) - - body = '{ "principalId": "%s", "resourceId": "%s", "appRoleId": "%s"}' % ( - scaleset_service_principal.object_id, - onefuzz_service_principal.object_id, - app_roles[0].id, - ) - - query = ( - "az rest --method post --url " - "https://graph.microsoft.com/v1.0/servicePrincipals/%s/appRoleAssignedTo " - "--body '%s' --headers \"Content-Type\"=application/json" - % (scaleset_service_principal.object_id, body) + assignments = query_microsoft_graph( + method="GET", + resource="servicePrincipals/%s/appRoleAssignments" + % scaleset_service_principal["id"], + subscription=subscription_id, ) - logger.warning( - "execute the following query in the azure portal bash shell : \n%s" % query + # check if the role is already assigned + role_assigned = seq(assignments["value"]).find( + lambda assignment: assignment["appRoleId"] == managed_node_role["id"] ) - - -def assign_app_role( - onefuzz_instance_name: str, - application_name: str, - subscription_id: str, - app_role: OnefuzzAppRole, -) -> None: - """ - Allows the application to access the service by assigning - their managed identity to the provided App Role - """ - try: - onefuzz_service_appId = query_microsoft_graph( - method="GET", - resource="applications", - params={ - "$filter": "displayName eq '%s'" % onefuzz_instance_name, - "$select": "appId", - }, - subscription=subscription_id, - ) - if len(onefuzz_service_appId["value"]) == 0: - raise Exception("onefuzz app registration not found") - appId = onefuzz_service_appId["value"][0]["appId"] - onefuzz_service_principals = query_microsoft_graph( - method="GET", - resource="servicePrincipals", - params={"$filter": "appId eq '%s'" % appId}, - subscription=subscription_id, - ) - - if len(onefuzz_service_principals["value"]) == 0: - raise Exception("onefuzz app service principal not found") - onefuzz_service_principal = onefuzz_service_principals["value"][0] - scaleset_service_principals = query_microsoft_graph( - method="GET", - resource="servicePrincipals", - params={"$filter": "displayName eq '%s'" % application_name}, - subscription=subscription_id, - ) - if len(scaleset_service_principals["value"]) == 0: - raise Exception("scaleset service principal not found") - scaleset_service_principal = scaleset_service_principals["value"][0] - managed_node_role = ( - seq(onefuzz_service_principal["appRoles"]) - .filter(lambda x: x["value"] == app_role.value) - .head_option() - ) - - if not managed_node_role: - raise Exception( - f"{app_role.value} role not found in the OneFuzz application " - "registration. Please redeploy the instance" - ) - assignments = query_microsoft_graph( - method="GET", - resource="servicePrincipals/%s/appRoleAssignments" + if not role_assigned: + query_microsoft_graph( + method="POST", + resource="servicePrincipals/%s/appRoleAssignedTo" % scaleset_service_principal["id"], + body={ + "principalId": scaleset_service_principal["id"], + "resourceId": onefuzz_service_principal["id"], + "appRoleId": managed_node_role["id"], + }, subscription=subscription_id, ) - # check if the role is already assigned - role_assigned = seq(assignments["value"]).find( - lambda assignment: assignment["appRoleId"] == managed_node_role["id"] - ) - if not role_assigned: - query_microsoft_graph( - method="POST", - resource="servicePrincipals/%s/appRoleAssignedTo" - % scaleset_service_principal["id"], - body={ - "principalId": scaleset_service_principal["id"], - "resourceId": onefuzz_service_principal["id"], - "appRoleId": managed_node_role["id"], - }, - subscription=subscription_id, - ) - except AuthenticationError: - assign_app_role_manually( - onefuzz_instance_name, application_name, subscription_id, app_role - ) - def set_app_audience( objectId: str, audience: str, subscription_id: Optional[str] = None From 62455631e26add0f390b726de8970d8ad0160233 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Wed, 16 Jun 2021 12:37:57 -0700 Subject: [PATCH 011/103] foramt --- src/deployment/deploy.py | 79 ++++++++++++++-------------------- src/deployment/registration.py | 3 +- 2 files changed, 33 insertions(+), 49 deletions(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 3e641df05c..505d0b9d2f 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -71,7 +71,7 @@ register_application, set_app_audience, update_pool_registration, - get_application + get_application, ) # Found by manually assigning the User.Read permission to application @@ -261,12 +261,11 @@ def setup_rbac(self) -> None: logger.info("using existing client application") return - app = get_application( - display_name=self.application_name, subscription_id=self.get_subscription_id() + display_name=self.application_name, + subscription_id=self.get_subscription_id(), ) - app_roles = [ { "allowedMemberTypes": ["Application"], @@ -274,7 +273,7 @@ def setup_rbac(self) -> None: "displayName": OnefuzzAppRole.CliClient.value, "id": str(uuid.uuid4()), "isEnabled": True, - "value": OnefuzzAppRole.CliClient.value + "value": OnefuzzAppRole.CliClient.value, }, { "allowedMemberTypes": ["Application"], @@ -283,7 +282,7 @@ def setup_rbac(self) -> None: "id": str(uuid.uuid4()), "isEnabled": True, "value": OnefuzzAppRole.ManagedNode.value, - } + }, ] # app: Optional[Application] = None @@ -303,25 +302,20 @@ def setup_rbac(self) -> None: "displayName": self.application_name, "identifierUris": [url], "appRoles": app_roles, - "requiredResourceAccess": - [ - { - "resourceAccess": { - "id": USER_READ_PERMISSION, - "type": "Scope" - }, - "resourceAppId": MICROSOFT_GRAPH_APP_ID, - } - ], - } - - - app:Dict = query_microsoft_graph( - method="POST", - resource="applications", - body=params, - subscription=self.get_subscription_id(), - ) + "requiredResourceAccess": [ + { + "resourceAccess": {"id": USER_READ_PERMISSION, "type": "Scope"}, + "resourceAppId": MICROSOFT_GRAPH_APP_ID, + } + ], + } + + app: Dict = query_microsoft_graph( + method="POST", + resource="applications", + body=params, + subscription=self.get_subscription_id(), + ) logger.info("creating service principal") @@ -376,13 +370,10 @@ def try_sp_create() -> None: 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(), ) @@ -390,9 +381,7 @@ def try_sp_create() -> None: query_microsoft_graph( method="PATCH", resource=f"applications/{app['id']}", - body={ - "appRoles": app_roles - }, + body={"appRoles": app_roles}, subscription=self.get_subscription_id(), ) @@ -402,12 +391,10 @@ def try_sp_create() -> None: self.application_name, ) query_microsoft_graph( - method="PATCH", - resource=f"applications/{app['id']}", - body={ - "identifierUris": [url] - }, - subscription=self.get_subscription_id(), + method="PATCH", + resource=f"applications/{app['id']}", + body={"identifierUris": [url]}, + subscription=self.get_subscription_id(), ) set_app_audience( @@ -426,21 +413,19 @@ def try_sp_create() -> None: ) url = "https://%s.azurewebsites.net" % self.application_name query_microsoft_graph( - method="PATCH", - resource=f"applications/{app['id']}", - body={ - "identifierUris": [url] - }, - subscription=self.get_subscription_id(), + method="PATCH", + resource=f"applications/{app['id']}", + body={"identifierUris": [url]}, + subscription=self.get_subscription_id(), ) else: logger.debug("No change to App Registration signInAudence setting") - (password_id, password) = self.create_password(app.object_id) - - cli_app = get_application(app_id=ONEFUZZ_CLI_APP, subscription_id=self.get_subscription_id()) + cli_app = get_application( + app_id=ONEFUZZ_CLI_APP, subscription_id=self.get_subscription_id() + ) if not cli_app: logger.info( diff --git a/src/deployment/registration.py b/src/deployment/registration.py index 2f71d3622a..8f29ab7007 100644 --- a/src/deployment/registration.py +++ b/src/deployment/registration.py @@ -273,7 +273,6 @@ def create_password() -> Tuple[str, str]: return retry(create_password, "create password") - def add_application_password_impl( app_object_id: UUID, subscription_id: str ) -> Tuple[str, str]: @@ -289,7 +288,6 @@ def add_application_password_impl( } } - password: Dict = query_microsoft_graph( method="POST", resource="applications/%s/addPassword" % app_object_id, @@ -407,6 +405,7 @@ def update_pool_registration(onefuzz_instance_name: str, subscription_id: str) - subscription_id, ) + def assign_app_role( onefuzz_instance_name: str, application_name: str, From 431200d3de9a64e1618ae57b163e825ba9875114 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Wed, 16 Jun 2021 12:58:38 -0700 Subject: [PATCH 012/103] mypy fix --- 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 505d0b9d2f..82069620a8 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -310,7 +310,7 @@ def setup_rbac(self) -> None: ], } - app: Dict = query_microsoft_graph( + app = query_microsoft_graph( method="POST", resource="applications", body=params, @@ -424,7 +424,7 @@ def try_sp_create() -> None: (password_id, password) = self.create_password(app.object_id) cli_app = get_application( - app_id=ONEFUZZ_CLI_APP, subscription_id=self.get_subscription_id() + app_id=uuid.UUID(ONEFUZZ_CLI_APP), subscription_id=self.get_subscription_id() ) if not cli_app: From 71bceef4fde1b8ec3b028aab62b107145fdec90c Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Wed, 16 Jun 2021 13:25:55 -0700 Subject: [PATCH 013/103] isort --- src/deployment/deploy.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 82069620a8..d35b1121d0 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -58,20 +58,19 @@ generate_container_sas, ) from azure.storage.queue import QueueServiceClient -from msrest.serialization import TZ_UTC - from data_migration import migrate +from msrest.serialization import TZ_UTC from registration import ( OnefuzzAppRole, add_application_password, assign_app_role, authorize_application, + get_application, get_tenant_id, query_microsoft_graph, register_application, set_app_audience, update_pool_registration, - get_application, ) # Found by manually assigning the User.Read permission to application From 7e1b93c18e2f02c3a4eb4b557d22c82d787feb90 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Wed, 16 Jun 2021 13:36:47 -0700 Subject: [PATCH 014/103] isort --- src/deployment/deploy.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index d35b1121d0..e49471bb06 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -58,8 +58,9 @@ generate_container_sas, ) from azure.storage.queue import QueueServiceClient -from data_migration import migrate from msrest.serialization import TZ_UTC + +from data_migration import migrate from registration import ( OnefuzzAppRole, add_application_password, From 7ba55defc47c009736e98f106482b9163fe2b410 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Wed, 16 Jun 2021 13:55:35 -0700 Subject: [PATCH 015/103] format --- src/deployment/deploy.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index e49471bb06..0a2ac4a6ff 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -424,7 +424,8 @@ def try_sp_create() -> None: (password_id, password) = self.create_password(app.object_id) cli_app = get_application( - app_id=uuid.UUID(ONEFUZZ_CLI_APP), subscription_id=self.get_subscription_id() + app_id=uuid.UUID(ONEFUZZ_CLI_APP), + subscription_id=self.get_subscription_id(), ) if not cli_app: From 09107aba15c89b5a38eb3545aee81e87de7e46f3 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Wed, 16 Jun 2021 17:01:38 -0700 Subject: [PATCH 016/103] bug fixes --- src/deployment/deploy.py | 34 +++++++++++------------- src/deployment/registration.py | 48 ++++++++++++++++++++++------------ 2 files changed, 47 insertions(+), 35 deletions(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 0a2ac4a6ff..131ae80da9 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -23,17 +23,7 @@ from azure.common.credentials import get_cli_profile from azure.core.exceptions import ResourceExistsError from azure.cosmosdb.table.tableservice import TableService -from azure.graphrbac.models import ( - Application, - ApplicationCreateParameters, - ApplicationUpdateParameters, - AppRole, - GraphErrorException, - OptionalClaims, - RequiredResourceAccess, - ResourceAccess, - ServicePrincipalCreateParameters, -) +from azure.graphrbac.models import GraphErrorException from azure.mgmt.applicationinsights import ApplicationInsightsManagementClient from azure.mgmt.applicationinsights.models import ( ApplicationInsightsComponentExportRequest, @@ -249,7 +239,9 @@ def check_region(self) -> None: sys.exit(1) def create_password(self, object_id: UUID) -> Tuple[str, str]: - return add_application_password(object_id, self.get_subscription_id()) + return add_application_password( + "cli_password", object_id, self.get_subscription_id() + ) def setup_rbac(self) -> None: """ @@ -301,10 +293,14 @@ def setup_rbac(self) -> None: params = { "displayName": self.application_name, "identifierUris": [url], + "signInAudience": "AzureADMyOrg", "appRoles": app_roles, + "web": {"redirectUris": [f"{url}/.auth/login/aad/callback"]}, "requiredResourceAccess": [ { - "resourceAccess": {"id": USER_READ_PERMISSION, "type": "Scope"}, + "resourceAccess": [ + {"id": USER_READ_PERMISSION, "type": "Scope"} + ], "resourceAppId": MICROSOFT_GRAPH_APP_ID, } ], @@ -332,7 +328,7 @@ def try_sp_create() -> None: try: query_microsoft_graph( method="POST", - resource="applications/servicePrincipals", + resource="servicePrincipals", body=service_principal_params, subscription=self.get_subscription_id(), ) @@ -385,7 +381,7 @@ def try_sp_create() -> None: subscription=self.get_subscription_id(), ) - if self.multi_tenant_domain and app.sign_in_audience == "AzureADMyOrg": + if self.multi_tenant_domain and app["signInAudience"] == "AzureADMyOrg": url = "https://%s/%s" % ( self.multi_tenant_domain, self.application_name, @@ -398,16 +394,16 @@ def try_sp_create() -> None: ) set_app_audience( - app.object_id, + app["id"], "AzureADMultipleOrgs", subscription_id=self.get_subscription_id(), ) elif ( not self.multi_tenant_domain - and app.sign_in_audience == "AzureADMultipleOrgs" + and app["signInAudience"] == "AzureADMultipleOrgs" ): set_app_audience( - app.object_id, + app["id"], "AzureADMyOrg", subscription_id=self.get_subscription_id(), ) @@ -421,7 +417,7 @@ def try_sp_create() -> None: else: logger.debug("No change to App Registration signInAudence setting") - (password_id, password) = self.create_password(app.object_id) + (password_id, password) = self.create_password(app["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 8f29ab7007..5e996ab3dc 100644 --- a/src/deployment/registration.py +++ b/src/deployment/registration.py @@ -9,21 +9,13 @@ import urllib.parse from datetime import datetime, timedelta from enum import Enum -from re import S, sub from typing import Any, Callable, Dict, List, NamedTuple, Optional, Tuple, TypeVar, cast from uuid import UUID, uuid4 import requests from azure.cli.core.azclierror import AuthenticationError -from azure.common.client_factory import get_client_from_cli_profile from azure.common.credentials import get_cli_profile -from azure.graphrbac.models import ( - Application, - AppRole, - PasswordCredential, - ServicePrincipal, - ServicePrincipalCreateParameters, -) +from azure.graphrbac.models import Application from functional import seq from msrest.serialization import TZ_UTC @@ -150,7 +142,7 @@ def register_application( else: logger.info( "Found existing application objectId '%s' - appid '%s'" - % (app.object_id, app["appId"]) + % (app["id"], app["appId"]) ) onefuzz_app = get_application( @@ -171,7 +163,7 @@ def register_application( tenant_id = get_tenant_id(subscription_id=subscription_id) return ApplicationInfo( - client_id=app.app_id, + client_id=app["id"], client_secret=password, authority=("https://login.microsoftonline.com/%s" % tenant_id), ) @@ -186,7 +178,9 @@ def create_application_credential(application_name: str, subscription_id: str) - if not app: raise Exception("app not found") - (_, password) = add_application_password(app["objectId"], subscription_id) + (_, password) = add_application_password( + f"{application_name}_password", app["objectId"], subscription_id + ) return str(password) @@ -244,7 +238,7 @@ def create_application_registration( query_microsoft_graph( method="POST", - resource="applications/servicePrincipals", + resource="servicePrincipals", body=service_principal_params, subscription=subscription_id, ) @@ -261,10 +255,12 @@ def create_application_registration( def add_application_password( - app_object_id: UUID, subscription_id: str + password_name: str, app_object_id: UUID, subscription_id: str ) -> Tuple[str, str]: def create_password() -> Tuple[str, str]: - password = add_application_password_impl(app_object_id, subscription_id) + password = add_application_password_impl( + password_name, app_object_id, subscription_id + ) logger.info("app password created") return password @@ -274,8 +270,28 @@ def create_password() -> Tuple[str, str]: def add_application_password_impl( - app_object_id: UUID, subscription_id: str + password_name: str, app_object_id: UUID, subscription_id: str ) -> Tuple[str, str]: + + app = query_microsoft_graph( + method="GET", + resource="applications/%s" % app_object_id, + subscription=subscription_id, + ) + + passwords = [ + x for x in app["passwordCredentials"] if x["displayName"] == password_name + ] + + if len(passwords) > 0: + key_id = passwords[0]["keyId"] + query_microsoft_graph( + method="POST", + resource="applications/%s/removePassword" % app_object_id, + body={"keyId": key_id}, + subscription=subscription_id, + ) + key = uuid4() password_request = { "passwordCredential": { From 73a135fbb7f4c02f7c35c6318d0337b61ea412a6 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Wed, 16 Jun 2021 17:12:49 -0700 Subject: [PATCH 017/103] specify the correct signInAudience --- src/deployment/deploy.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 131ae80da9..0432c8dfb9 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -287,13 +287,15 @@ def setup_rbac(self) -> None: self.multi_tenant_domain, self.application_name, ) + signInAudience = "AzureADMultipleOrgs" else: url = "https://%s.azurewebsites.net" % self.application_name + signInAudience = "AzureADMyOrg" params = { "displayName": self.application_name, "identifierUris": [url], - "signInAudience": "AzureADMyOrg", + "signInAudience": signInAudience, "appRoles": app_roles, "web": {"redirectUris": [f"{url}/.auth/login/aad/callback"]}, "requiredResourceAccess": [ From be18ab9217636152aa5c848490bb712d861ae193 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Thu, 17 Jun 2021 11:57:37 -0700 Subject: [PATCH 018/103] fix backing service principal creation fix preauthorized application --- src/deployment/deploy.py | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 0432c8dfb9..463ba77e9b 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -23,7 +23,6 @@ from azure.common.credentials import get_cli_profile from azure.core.exceptions import ResourceExistsError from azure.cosmosdb.table.tableservice import TableService -from azure.graphrbac.models import GraphErrorException from azure.mgmt.applicationinsights import ApplicationInsightsManagementClient from azure.mgmt.applicationinsights.models import ( ApplicationInsightsComponentExportRequest, @@ -52,6 +51,7 @@ from data_migration import migrate from registration import ( + GraphQueryError, OnefuzzAppRole, add_application_password, assign_app_role, @@ -297,6 +297,20 @@ def setup_rbac(self) -> None: "identifierUris": [url], "signInAudience": signInAudience, "appRoles": app_roles, + "api": { + "oauth2PermissionScopes": [ + { + "adminConsentDescription": f"Allow the application to access {self.application_name} on behalf of the signed-in user.", + "adminConsentDisplayName": f"Access {self.application_name}", + "id": str(uuid.uuid4()), + "isEnabled": True, + "type": "User", + "userConsentDescription": f"Allow the application to access {self.application_name} on your behalf.", + "userConsentDisplayName": f"Access {self.application_name}", + "value": "user_impersonation", + } + ] + }, "web": {"redirectUris": [f"{url}/.auth/login/aad/callback"]}, "requiredResourceAccess": [ { @@ -335,7 +349,7 @@ def try_sp_create() -> None: subscription=self.get_subscription_id(), ) return - except GraphErrorException as err: + except GraphQueryError as err: # work around timing issue when creating service principal # https://github.com/Azure/azure-cli/issues/14767 if ( From 295c56e1dd76e95bf6114198c0c5fe769211a20f Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Thu, 17 Jun 2021 13:29:15 -0700 Subject: [PATCH 019/103] remove remaining references to graphrbac --- src/api-service/__app__/onefuzzlib/request.py | 3 +-- src/api-service/__app__/requirements.txt | 1 - src/deployment/registration.py | 3 +-- src/deployment/requirements.txt | 1 - 4 files changed, 2 insertions(+), 6 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/request.py b/src/api-service/__app__/onefuzzlib/request.py index ebaad6d79a..e5e8d65ed5 100644 --- a/src/api-service/__app__/onefuzzlib/request.py +++ b/src/api-service/__app__/onefuzzlib/request.py @@ -10,7 +10,6 @@ from uuid import UUID from azure.functions import HttpRequest, HttpResponse -from azure.graphrbac.models import GraphErrorException from onefuzztypes.enums import ErrorCode from onefuzztypes.models import Error from onefuzztypes.responses import BaseResponse @@ -35,7 +34,7 @@ def check_access(req: HttpRequest) -> Optional[Error]: member_id = req.headers["x-ms-client-principal-id"] try: result = is_member_of(group_id, member_id) - except GraphErrorException: + except Exception: return Error( code=ErrorCode.UNAUTHORIZED, errors=["unable to interact with graph"] ) diff --git a/src/api-service/__app__/requirements.txt b/src/api-service/__app__/requirements.txt index 2a9869ebc3..76a45154e3 100644 --- a/src/api-service/__app__/requirements.txt +++ b/src/api-service/__app__/requirements.txt @@ -4,7 +4,6 @@ azure-cosmosdb-nspkg==2.0.2 azure-cosmosdb-table==1.0.6 azure-devops==6.0.0b4 azure-functions==1.5.0 -azure-graphrbac~=0.61.1 azure-identity==1.6.0 azure-keyvault-keys~=4.3.1 azure-keyvault-secrets~=4.2.0 diff --git a/src/deployment/registration.py b/src/deployment/registration.py index 5e996ab3dc..f39447e925 100644 --- a/src/deployment/registration.py +++ b/src/deployment/registration.py @@ -15,7 +15,6 @@ import requests from azure.cli.core.azclierror import AuthenticationError from azure.common.credentials import get_cli_profile -from azure.graphrbac.models import Application from functional import seq from msrest.serialization import TZ_UTC @@ -186,7 +185,7 @@ def create_application_credential(application_name: str, subscription_id: str) - def create_application_registration( onefuzz_instance_name: str, name: str, approle: OnefuzzAppRole, subscription_id: str -) -> Application: +) -> Any: """Create an application registration""" app = get_application( diff --git a/src/deployment/requirements.txt b/src/deployment/requirements.txt index 3779de2cff..77ea638718 100644 --- a/src/deployment/requirements.txt +++ b/src/deployment/requirements.txt @@ -1,7 +1,6 @@ azure-cli-core==2.23.0 azure-cli==2.23.0 azure-cosmosdb-table==1.0.6 -azure-graphrbac==0.60.0 azure-mgmt-eventgrid==3.0.0rc9 azure-mgmt-resource==12.1.0 azure-mgmt-storage==17.1.0 From 9c471590534a8112986c4cdd88887fc1aa7f836a Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Fri, 18 Jun 2021 11:05:04 -0700 Subject: [PATCH 020/103] fix ms graph authentication --- src/api-service/__app__/onefuzzlib/azure/creds.py | 10 ++++++---- src/api-service/__app__/onefuzzlib/request.py | 5 +++-- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/azure/creds.py b/src/api-service/__app__/onefuzzlib/azure/creds.py index 68a75db943..ae1dcc9fd2 100644 --- a/src/api-service/__app__/onefuzzlib/azure/creds.py +++ b/src/api-service/__app__/onefuzzlib/azure/creds.py @@ -14,7 +14,7 @@ from azure.mgmt.resource import ResourceManagementClient from azure.mgmt.subscription import SubscriptionClient from memoization import cached -from msrestazure.azure_active_directory import MSIAuthentication +from msrestazure.azure_active_directory import AZURE_PUBLIC_CLOUD, MSIAuthentication from msrestazure.tools import parse_resource_id from onefuzztypes.primitives import Container, Region @@ -22,10 +22,12 @@ @cached -def get_msi() -> MSIAuthentication: +def get_ms_graph_msi() -> MSIAuthentication: allow_more_workers() reduce_logging() - return MSIAuthentication() + return MSIAuthentication( + resource=AZURE_PUBLIC_CLOUD.endpoints.microsoft_graph_resource_id + ) @cached @@ -102,7 +104,7 @@ def query_microsoft_graph( params: Optional[Dict] = None, body: Optional[Dict] = None, ) -> Any: - auth = get_msi() + auth = get_ms_graph_msi() access_token = auth.token["access_token"] token_type = auth.token["token_type"] diff --git a/src/api-service/__app__/onefuzzlib/request.py b/src/api-service/__app__/onefuzzlib/request.py index e5e8d65ed5..34c9e6844e 100644 --- a/src/api-service/__app__/onefuzzlib/request.py +++ b/src/api-service/__app__/onefuzzlib/request.py @@ -34,9 +34,10 @@ def check_access(req: HttpRequest) -> Optional[Error]: member_id = req.headers["x-ms-client-principal-id"] try: result = is_member_of(group_id, member_id) - except Exception: + except Exception as e: return Error( - code=ErrorCode.UNAUTHORIZED, errors=["unable to interact with graph"] + code=ErrorCode.UNAUTHORIZED, + errors=["unable to interact with graph", str(e)], ) if not result: logging.error("unauthorized access: %s is not in %s", member_id, group_id) From 128c7f8201c7068139f39f8b1ee33325f8fb9d33 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Fri, 9 Jul 2021 13:53:43 -0700 Subject: [PATCH 021/103] formatting --- 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 e7f9cce06a..7a8539f253 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -121,7 +121,7 @@ def __init__( multi_tenant_domain: str, upgrade: bool, subscription_id: Optional[str], - admins: List[UUID] + admins: List[UUID], ): self.subscription_id = subscription_id self.resource_group = resource_group From 83a29040c50073659279c6c49697c7070b9698c0 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Thu, 15 Jul 2021 12:44:36 -0700 Subject: [PATCH 022/103] add a data structure to store permission for each url --- .../__app__/onefuzzlib/request_trie.py | 104 ++++++++++++++++++ 1 file changed, 104 insertions(+) create mode 100644 src/api-service/__app__/onefuzzlib/request_trie.py diff --git a/src/api-service/__app__/onefuzzlib/request_trie.py b/src/api-service/__app__/onefuzzlib/request_trie.py new file mode 100644 index 0000000000..7c0c111802 --- /dev/null +++ b/src/api-service/__app__/onefuzzlib/request_trie.py @@ -0,0 +1,104 @@ +from typing import Dict, List +from uuid import UUID +import uuid + + +class Permissions: + allowed_groups_ids: List[UUID] + allowed_members_ids: List[UUID] + + def __init__(self, allowed_groups_ids: List[UUID] = [], allowed_members_ids: List[UUID] = []) -> None: + self.allowed_groups_ids = allowed_groups_ids + self.allowed_members_ids = allowed_members_ids + +class RequestTrieNode: + permissions: Permissions + children: Dict[str, "RequestTrieNode"] + + def __init__(self) -> None: + self.permissions = Permissions() + self.children = {} + pass + + +class RequestTrie: + root: RequestTrieNode + + def __init__(self) -> None: + self.root = RequestTrieNode() + + def add_url( + self, path: str, permissions: Permissions + ): + segments = path.split("/") + if len(segments) == 0: + return + + current_node = self.root + current_segment_index = 0 + + while current_segment_index < len(segments): + current_segment = segments[current_segment_index] + if current_segment in current_node.children: + current_node = current_node.children[current_segment] + current_segment_index = current_segment_index + 1 + else: + break + + # we found a node matching this exact path + # This means that there is an existing rule causing a conflict + if current_segment_index == len(segments): + raise Exception(f"Conflicting path {path}") + + while current_segment_index < len(segments): + current_segment = segments[current_segment_index] + current_node.children[current_segment] = RequestTrieNode() + current_node = current_node.children[current_segment] + current_segment_index = current_segment_index+1 + + current_node.permissions = permissions + + + def get_matching_rules(self, path: str) -> Permissions: + segments = path.split("/") + if len(segments) == 0: + return Permissions() + + current_node = self.root + current_segment_index = 0 + + while current_segment_index < len(segments): + current_segment = segments[current_segment_index] + if (current_segment in current_node.children) or ( + "*" in current_node.children + ): + current_node = current_node.children[current_segment] + current_segment_index = current_segment_index + 1 + else: + break + + return current_node.permissions + + + + +import unittest +class TestRequestTrie(unittest.TestCase): + def test(self) -> None: + + guid1 = uuid.uuid4() + guid2 = uuid.uuid4() + + request_trie = RequestTrie() + request_trie.add_url("a/b/c", Permissions(allowed_groups_ids=[guid1])) + request_trie.add_url("b/*/c", Permissions(allowed_groups_ids=[guid2])) + + permissions = request_trie.get_matching_rules("a/b/c") + self.assertNotEqual(len(permissions.allowed_groups_ids), 0, "empty allowed groups") + self.assertEqual(permissions.allowed_groups_ids[0], guid1) + + + # permission + + + From f536f77ca1d3bd666dee7a45088143cf5103bcac Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Thu, 15 Jul 2021 17:56:18 -0700 Subject: [PATCH 023/103] adding logic --- .../__app__/onefuzzlib/azure/creds.py | 4 +- src/api-service/__app__/onefuzzlib/request.py | 70 +++++++++++++++++- .../__app__/onefuzzlib/request_trie.py | 73 +++++++++---------- 3 files changed, 103 insertions(+), 44 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/azure/creds.py b/src/api-service/__app__/onefuzzlib/azure/creds.py index ae1dcc9fd2..1de46c9545 100644 --- a/src/api-service/__app__/onefuzzlib/azure/creds.py +++ b/src/api-service/__app__/onefuzzlib/azure/creds.py @@ -133,8 +133,8 @@ def query_microsoft_graph( ) -def is_member_of(group_id: str, member_id: str) -> bool: - body = {"groupIds": [group_id]} +def is_member_of(group_ids: List[str], member_id: str) -> bool: + body = {"groupIds": group_ids} response = query_microsoft_graph( method="POST", resource=f"users/{member_id}/checkMemberGroups", body=body ) diff --git a/src/api-service/__app__/onefuzzlib/request.py b/src/api-service/__app__/onefuzzlib/request.py index 34c9e6844e..08c7424533 100644 --- a/src/api-service/__app__/onefuzzlib/request.py +++ b/src/api-service/__app__/onefuzzlib/request.py @@ -6,17 +6,23 @@ import json import logging import os -from typing import TYPE_CHECKING, Optional, Sequence, Type, TypeVar, Union +from typing import List, TYPE_CHECKING, Optional, Sequence, Type, TypeVar, Union from uuid import UUID +import uuid from azure.functions import HttpRequest, HttpResponse from onefuzztypes.enums import ErrorCode from onefuzztypes.models import Error from onefuzztypes.responses import BaseResponse from pydantic import ValidationError +from pydantic.tools import parse_obj_as from .azure.creds import is_member_of from .orm import ModelMixin +from .request_auth import RequestAuthorization + +from memoization import cached +import urllib # We don't actually use these types at runtime at this time. Rather, # these are used in a bound TypeVar. MyPy suggests to only import these @@ -26,6 +32,66 @@ from pydantic import BaseModel # noqa: F401 +class RuleDefinition(BaseModel): + methods: List[str] + endpoint: str + allowed_groups: List[UUID] + + +@cached +def get_rules() -> Optional[RequestAuthorization]: + # todo: move to instacewide configuration + rules_data = os.environ["ONEFUZZ_AAD_GROUP_RULES"] + if not rules_data: + return None + + rules = parse_obj_as(List[RuleDefinition], rules_data) + request_auth = RequestAuthorization() + for rule in rules: + request_auth.add_url( + rule.endpoint, + RequestAuthorization.Rules(allowed_groups_ids=rule.allowed_groups), + ) + + request_auth + + +# todo: +# - check the verb +# +def check_access2(req: HttpRequest) -> Optional[Error]: + rules = get_rules() + + if not rules: + return None + + path = urllib.parse.urlparse(req.url).path + rule = rules.get_matching_rules(path) + if not rule: + return None + else: + member_id = req.headers["x-ms-client-principal-id"] + + try: + result = is_member_of(rule.allowed_groups_ids, member_id) + except Exception as e: + return Error( + code=ErrorCode.UNAUTHORIZED, + errors=["unable to interact with graph", str(e)], + ) + if not result: + logging.error( + "unauthorized access: %s is not authorized to access in %s", + member_id, + req.url, + ) + return Error( + code=ErrorCode.UNAUTHORIZED, + errors=["not approved to use this instance of onefuzz"], + ) + return None + + def check_access(req: HttpRequest) -> Optional[Error]: if "ONEFUZZ_AAD_GROUP_ID" not in os.environ: return None @@ -33,7 +99,7 @@ def check_access(req: HttpRequest) -> Optional[Error]: group_id = os.environ["ONEFUZZ_AAD_GROUP_ID"] member_id = req.headers["x-ms-client-principal-id"] try: - result = is_member_of(group_id, member_id) + result = is_member_of([group_id], member_id) except Exception as e: return Error( code=ErrorCode.UNAUTHORIZED, diff --git a/src/api-service/__app__/onefuzzlib/request_trie.py b/src/api-service/__app__/onefuzzlib/request_trie.py index 7c0c111802..c3e9bb4350 100644 --- a/src/api-service/__app__/onefuzzlib/request_trie.py +++ b/src/api-service/__app__/onefuzzlib/request_trie.py @@ -1,35 +1,30 @@ -from typing import Dict, List +from typing import Dict, List, Optional from uuid import UUID import uuid -class Permissions: - allowed_groups_ids: List[UUID] - allowed_members_ids: List[UUID] +class RequestAuthorization: + class Rules: + allowed_groups_ids: List[UUID] - def __init__(self, allowed_groups_ids: List[UUID] = [], allowed_members_ids: List[UUID] = []) -> None: - self.allowed_groups_ids = allowed_groups_ids - self.allowed_members_ids = allowed_members_ids + def __init__(self, allowed_groups_ids: List[UUID] = []) -> None: + self.allowed_groups_ids = allowed_groups_ids -class RequestTrieNode: - permissions: Permissions - children: Dict[str, "RequestTrieNode"] - - def __init__(self) -> None: - self.permissions = Permissions() - self.children = {} - pass + class Node: + rules: "RequestAuthorization.Rules" + children: Dict[str, "RequestAuthorization.Node"] + def __init__(self) -> None: + self.rules = RequestAuthorization.Rules() + self.children = {} + pass -class RequestTrie: - root: RequestTrieNode + root: Node def __init__(self) -> None: - self.root = RequestTrieNode() + self.root = RequestAuthorization.Node() - def add_url( - self, path: str, permissions: Permissions - ): + def add_url(self, path: str, rules: Rules) -> None: segments = path.split("/") if len(segments) == 0: return @@ -52,17 +47,16 @@ def add_url( while current_segment_index < len(segments): current_segment = segments[current_segment_index] - current_node.children[current_segment] = RequestTrieNode() + current_node.children[current_segment] = RequestAuthorization.Node() current_node = current_node.children[current_segment] - current_segment_index = current_segment_index+1 - - current_node.permissions = permissions + current_segment_index = current_segment_index + 1 + current_node.rules = rules - def get_matching_rules(self, path: str) -> Permissions: + def get_matching_rules(self, path: str) -> Optional[Rules]: segments = path.split("/") if len(segments) == 0: - return Permissions() + return None current_node = self.root current_segment_index = 0 @@ -77,28 +71,27 @@ def get_matching_rules(self, path: str) -> Permissions: else: break - return current_node.permissions + return current_node.rules +import unittest -import unittest -class TestRequestTrie(unittest.TestCase): +class TestRequestAuthorization(unittest.TestCase): def test(self) -> None: guid1 = uuid.uuid4() guid2 = uuid.uuid4() - request_trie = RequestTrie() - request_trie.add_url("a/b/c", Permissions(allowed_groups_ids=[guid1])) - request_trie.add_url("b/*/c", Permissions(allowed_groups_ids=[guid2])) - - permissions = request_trie.get_matching_rules("a/b/c") - self.assertNotEqual(len(permissions.allowed_groups_ids), 0, "empty allowed groups") - self.assertEqual(permissions.allowed_groups_ids[0], guid1) + request_trie = RequestAuthorization() + request_trie.add_url("a/b/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1])) + request_trie.add_url("b/*/c", RequestAuthorization.Rules(allowed_groups_ids=[guid2])) + rules = request_trie.get_matching_rules("a/b/c") + if rules: + self.assertNotEqual(len(rules.allowed_groups_ids), 0, "empty allowed groups") + self.assertEqual(rules.allowed_groups_ids[0], guid1) + else: + self.fail("no rule found") # permission - - - From e1a4918dc7431a95662c08fba71a397fc332610d Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Thu, 15 Jul 2021 18:39:34 -0700 Subject: [PATCH 024/103] reafctoring and added a couple of tests --- src/api-service/__app__/onefuzzlib/request.py | 53 +++++++++---------- .../{request_trie.py => request_auth.py} | 34 ++++++------ src/api-service/tests/test_request_auth.py | 45 ++++++++++++++++ 3 files changed, 88 insertions(+), 44 deletions(-) rename src/api-service/__app__/onefuzzlib/{request_trie.py => request_auth.py} (77%) create mode 100644 src/api-service/tests/test_request_auth.py diff --git a/src/api-service/__app__/onefuzzlib/request.py b/src/api-service/__app__/onefuzzlib/request.py index 08c7424533..a126665239 100644 --- a/src/api-service/__app__/onefuzzlib/request.py +++ b/src/api-service/__app__/onefuzzlib/request.py @@ -6,11 +6,13 @@ import json import logging import os -from typing import List, TYPE_CHECKING, Optional, Sequence, Type, TypeVar, Union -from uuid import UUID +import urllib import uuid +from typing import TYPE_CHECKING, List, Optional, Sequence, Type, TypeVar, Union +from uuid import UUID from azure.functions import HttpRequest, HttpResponse +from memoization import cached from onefuzztypes.enums import ErrorCode from onefuzztypes.models import Error from onefuzztypes.responses import BaseResponse @@ -21,9 +23,6 @@ from .orm import ModelMixin from .request_auth import RequestAuthorization -from memoization import cached -import urllib - # We don't actually use these types at runtime at this time. Rather, # these are used in a bound TypeVar. MyPy suggests to only import these # types during type checking. @@ -67,29 +66,27 @@ def check_access2(req: HttpRequest) -> Optional[Error]: path = urllib.parse.urlparse(req.url).path rule = rules.get_matching_rules(path) - if not rule: - return None - else: - member_id = req.headers["x-ms-client-principal-id"] - - try: - result = is_member_of(rule.allowed_groups_ids, member_id) - except Exception as e: - return Error( - code=ErrorCode.UNAUTHORIZED, - errors=["unable to interact with graph", str(e)], - ) - if not result: - logging.error( - "unauthorized access: %s is not authorized to access in %s", - member_id, - req.url, - ) - return Error( - code=ErrorCode.UNAUTHORIZED, - errors=["not approved to use this instance of onefuzz"], - ) - return None + + member_id = req.headers["x-ms-client-principal-id"] + + try: + result = is_member_of(rule.allowed_groups_ids, member_id) + except Exception as e: + return Error( + code=ErrorCode.UNAUTHORIZED, + errors=["unable to interact with graph", str(e)], + ) + if not result: + logging.error( + "unauthorized access: %s is not authorized to access in %s", + member_id, + req.url, + ) + return Error( + code=ErrorCode.UNAUTHORIZED, + errors=["not approved to use this instance of onefuzz"], + ) + return None def check_access(req: HttpRequest) -> Optional[Error]: diff --git a/src/api-service/__app__/onefuzzlib/request_trie.py b/src/api-service/__app__/onefuzzlib/request_auth.py similarity index 77% rename from src/api-service/__app__/onefuzzlib/request_trie.py rename to src/api-service/__app__/onefuzzlib/request_auth.py index c3e9bb4350..d426c8b502 100644 --- a/src/api-service/__app__/onefuzzlib/request_trie.py +++ b/src/api-service/__app__/onefuzzlib/request_auth.py @@ -1,6 +1,6 @@ +import uuid from typing import Dict, List, Optional from uuid import UUID -import uuid class RequestAuthorization: @@ -53,24 +53,22 @@ def add_url(self, path: str, rules: Rules) -> None: current_node.rules = rules - def get_matching_rules(self, path: str) -> Optional[Rules]: + def get_matching_rules(self, path: str) -> Rules: segments = path.split("/") - if len(segments) == 0: - return None - current_node = self.root current_segment_index = 0 while current_segment_index < len(segments): current_segment = segments[current_segment_index] - if (current_segment in current_node.children) or ( - "*" in current_node.children - ): + if current_segment in current_node.children: current_node = current_node.children[current_segment] - current_segment_index = current_segment_index + 1 + elif "*" in current_node.children: + current_node = current_node.children["*"] else: break + current_segment_index = current_segment_index + 1 + return current_node.rules @@ -84,14 +82,18 @@ def test(self) -> None: guid2 = uuid.uuid4() request_trie = RequestAuthorization() - request_trie.add_url("a/b/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1])) - request_trie.add_url("b/*/c", RequestAuthorization.Rules(allowed_groups_ids=[guid2])) + request_trie.add_url( + "a/b/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1]) + ) + request_trie.add_url( + "b/*/c", RequestAuthorization.Rules(allowed_groups_ids=[guid2]) + ) rules = request_trie.get_matching_rules("a/b/c") - if rules: - self.assertNotEqual(len(rules.allowed_groups_ids), 0, "empty allowed groups") - self.assertEqual(rules.allowed_groups_ids[0], guid1) - else: - self.fail("no rule found") + + self.assertNotEqual( + len(rules.allowed_groups_ids), 0, "empty allowed groups" + ) + self.assertEqual(rules.allowed_groups_ids[0], guid1) # permission diff --git a/src/api-service/tests/test_request_auth.py b/src/api-service/tests/test_request_auth.py new file mode 100644 index 0000000000..c29380d21f --- /dev/null +++ b/src/api-service/tests/test_request_auth.py @@ -0,0 +1,45 @@ +import unittest +import uuid + +from __app__.onefuzzlib.request_auth import RequestAuthorization + + +class TestRequestAuthorization(unittest.TestCase): + def test_exact_match(self) -> None: + + guid1 = uuid.uuid4() + guid2 = uuid.uuid4() + + request_trie = RequestAuthorization() + request_trie.add_url( + "a/b/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1]) + ) + + rules1 = request_trie.get_matching_rules("a/b/c") + rules2 = request_trie.get_matching_rules("b/b/e") + + self.assertNotEqual( + len(rules1.allowed_groups_ids), 0, "empty allowed groups" + ) + self.assertEqual(rules1.allowed_groups_ids[0], guid1) + + + self.assertEqual( + len(rules2.allowed_groups_ids), 0, "expected nothing" + ) + + + def test_wildcard(self): + guid1 = uuid.uuid4() + + request_trie = RequestAuthorization() + request_trie.add_url( + "b/*/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1]) + ) + + rules = request_trie.get_matching_rules("b/b/c") + + self.assertNotEqual( + len(rules.allowed_groups_ids), 0, "empty allowed groups" + ) + self.assertEqual(rules.allowed_groups_ids[0], guid1) From 57488b0167d0cf266ec740c4f2b76a62de3adf8a Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Fri, 16 Jul 2021 13:34:40 -0700 Subject: [PATCH 025/103] foramtting --- src/api-service/__app__/onefuzzlib/request.py | 1 + .../__app__/onefuzzlib/request_auth.py | 32 ++----------------- 2 files changed, 3 insertions(+), 30 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/request.py b/src/api-service/__app__/onefuzzlib/request.py index a126665239..ffb1a748bd 100644 --- a/src/api-service/__app__/onefuzzlib/request.py +++ b/src/api-service/__app__/onefuzzlib/request.py @@ -31,6 +31,7 @@ from pydantic import BaseModel # noqa: F401 +# todo add top level rule class RuleDefinition(BaseModel): methods: List[str] endpoint: str diff --git a/src/api-service/__app__/onefuzzlib/request_auth.py b/src/api-service/__app__/onefuzzlib/request_auth.py index d426c8b502..338658aa4d 100644 --- a/src/api-service/__app__/onefuzzlib/request_auth.py +++ b/src/api-service/__app__/onefuzzlib/request_auth.py @@ -1,5 +1,4 @@ -import uuid -from typing import Dict, List, Optional +from typing import Dict, List from uuid import UUID @@ -69,31 +68,4 @@ def get_matching_rules(self, path: str) -> Rules: current_segment_index = current_segment_index + 1 - return current_node.rules - - -import unittest - - -class TestRequestAuthorization(unittest.TestCase): - def test(self) -> None: - - guid1 = uuid.uuid4() - guid2 = uuid.uuid4() - - request_trie = RequestAuthorization() - request_trie.add_url( - "a/b/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1]) - ) - request_trie.add_url( - "b/*/c", RequestAuthorization.Rules(allowed_groups_ids=[guid2]) - ) - - rules = request_trie.get_matching_rules("a/b/c") - - self.assertNotEqual( - len(rules.allowed_groups_ids), 0, "empty allowed groups" - ) - self.assertEqual(rules.allowed_groups_ids[0], guid1) - - # permission + return current_node.rules \ No newline at end of file From f8b48a3e7096cc710b48cacba318b03cc6410288 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 19 Jul 2021 09:30:37 -0700 Subject: [PATCH 026/103] more unit tests --- .../__app__/onefuzzlib/request_auth.py | 6 +- src/api-service/tests/test_request_auth.py | 101 ++++++++++++++++-- 2 files changed, 95 insertions(+), 12 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/request_auth.py b/src/api-service/__app__/onefuzzlib/request_auth.py index 338658aa4d..fa9962e8e9 100644 --- a/src/api-service/__app__/onefuzzlib/request_auth.py +++ b/src/api-service/__app__/onefuzzlib/request_auth.py @@ -3,6 +3,10 @@ class RequestAuthorization: + """ + Stores the rules associated with a the request paths + """ + class Rules: allowed_groups_ids: List[UUID] @@ -68,4 +72,4 @@ def get_matching_rules(self, path: str) -> Rules: current_segment_index = current_segment_index + 1 - return current_node.rules \ No newline at end of file + return current_node.rules diff --git a/src/api-service/tests/test_request_auth.py b/src/api-service/tests/test_request_auth.py index c29380d21f..4b4981310a 100644 --- a/src/api-service/tests/test_request_auth.py +++ b/src/api-service/tests/test_request_auth.py @@ -8,7 +8,6 @@ class TestRequestAuthorization(unittest.TestCase): def test_exact_match(self) -> None: guid1 = uuid.uuid4() - guid2 = uuid.uuid4() request_trie = RequestAuthorization() request_trie.add_url( @@ -18,28 +17,108 @@ def test_exact_match(self) -> None: rules1 = request_trie.get_matching_rules("a/b/c") rules2 = request_trie.get_matching_rules("b/b/e") - self.assertNotEqual( - len(rules1.allowed_groups_ids), 0, "empty allowed groups" - ) + self.assertNotEqual(len(rules1.allowed_groups_ids), 0, "empty allowed groups") self.assertEqual(rules1.allowed_groups_ids[0], guid1) + self.assertEqual(len(rules2.allowed_groups_ids), 0, "expected nothing") + + def test_wildcard(self): + guid1 = uuid.uuid4() + + request_trie = RequestAuthorization() + request_trie.add_url( + "b/*/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1]) + ) + + rules = request_trie.get_matching_rules("b/b/c") + + self.assertNotEqual(len(rules.allowed_groups_ids), 0, "empty allowed groups") + self.assertEqual(rules.allowed_groups_ids[0], guid1) + + def test_adding_rule_on_same_path(self): + guid1 = uuid.uuid4() + + request_trie = RequestAuthorization() + request_trie.add_url( + "a/b/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1]) + ) + + try: + request_trie.add_url( + "a/b/c", RequestAuthorization.Rules(allowed_groups_ids=[]) + ) + self.fail("this is expected to fail") + except: + pass + + # The most specific rules takes priority over the ones containing a wildcard + def test_priority(self): + guid1 = uuid.uuid4() + guid2 = uuid.uuid4() + + request_trie = RequestAuthorization() + request_trie.add_url( + "a/*/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1]) + ) + + request_trie.add_url( + "a/b/c", RequestAuthorization.Rules(allowed_groups_ids=[guid2]) + ) + + rules = request_trie.get_matching_rules("a/b/c") self.assertEqual( - len(rules2.allowed_groups_ids), 0, "expected nothing" + rules.allowed_groups_ids[0], + guid2, + "expected to match the most specific rule", ) + # if a path has no specific rule. it inherits from the parents + # /a/b/c inherint from a/b + # todo test the wildcard behavior + def test_inherit_rule(self): + guid1 = uuid.uuid4() + guid2 = uuid.uuid4() - def test_wildcard(self): + request_trie = RequestAuthorization() + request_trie.add_url( + "a/b/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1]) + ) + + request_trie.add_url( + "f/*/c", RequestAuthorization.Rules(allowed_groups_ids=[guid2]) + ) + + rules = request_trie.get_matching_rules("a/b/c/d") + self.assertEqual( + rules.allowed_groups_ids[0], guid1, "expected to inherit rule of a/b/c" + ) + + rules = request_trie.get_matching_rules("f/b/c/d") + self.assertEqual( + rules.allowed_groups_ids[0], guid2, "expected to inherit rule of f/*/c" + ) + + # the lowest level rule override the parent rules + def test_override_rule(self): guid1 = uuid.uuid4() + guid2 = uuid.uuid4() request_trie = RequestAuthorization() request_trie.add_url( - "b/*/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1]) + "a/b/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1]) ) - rules = request_trie.get_matching_rules("b/b/c") + request_trie.add_url( + "a/b/c/d", RequestAuthorization.Rules(allowed_groups_ids=[guid2]) + ) - self.assertNotEqual( - len(rules.allowed_groups_ids), 0, "empty allowed groups" + rules = request_trie.get_matching_rules("a/b/c") + self.assertEqual( + rules.allowed_groups_ids[0], guid1, "expected to inherit rule of a/b/c" + ) + + rules = request_trie.get_matching_rules("a/b/c/d") + self.assertEqual( + rules.allowed_groups_ids[0], guid2, "expected to inherit rule of a/b/c/d" ) - self.assertEqual(rules.allowed_groups_ids[0], guid1) From e8be5fa905aaaef0a7fa2e63ecb2caa7c3c63a37 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 19 Jul 2021 09:42:39 -0700 Subject: [PATCH 027/103] build fix --- src/api-service/__app__/onefuzzlib/request.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/api-service/__app__/onefuzzlib/request.py b/src/api-service/__app__/onefuzzlib/request.py index ffb1a748bd..68f582d6a6 100644 --- a/src/api-service/__app__/onefuzzlib/request.py +++ b/src/api-service/__app__/onefuzzlib/request.py @@ -16,6 +16,7 @@ from onefuzztypes.enums import ErrorCode from onefuzztypes.models import Error from onefuzztypes.responses import BaseResponse +from pydantic import BaseModel # noqa: F401 from pydantic import ValidationError from pydantic.tools import parse_obj_as @@ -28,7 +29,6 @@ # types during type checking. if TYPE_CHECKING: from onefuzztypes.requests import BaseRequest # noqa: F401 - from pydantic import BaseModel # noqa: F401 # todo add top level rule From de717c02825a058b7a7f3927b9a85dfe86d21cda Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 19 Jul 2021 09:53:29 -0700 Subject: [PATCH 028/103] format --- src/api-service/__app__/onefuzzlib/azure/creds.py | 6 +++++- src/api-service/__app__/onefuzzlib/request.py | 1 - 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/azure/creds.py b/src/api-service/__app__/onefuzzlib/azure/creds.py index 1de46c9545..263d1bbd18 100644 --- a/src/api-service/__app__/onefuzzlib/azure/creds.py +++ b/src/api-service/__app__/onefuzzlib/azure/creds.py @@ -138,7 +138,11 @@ def is_member_of(group_ids: List[str], member_id: str) -> bool: response = query_microsoft_graph( method="POST", resource=f"users/{member_id}/checkMemberGroups", body=body ) - return group_id in response["value"] + for group_id in group_ids: + if group_id not in response["value"]: + return False + + return True @cached diff --git a/src/api-service/__app__/onefuzzlib/request.py b/src/api-service/__app__/onefuzzlib/request.py index 68f582d6a6..b938cbe2eb 100644 --- a/src/api-service/__app__/onefuzzlib/request.py +++ b/src/api-service/__app__/onefuzzlib/request.py @@ -7,7 +7,6 @@ import logging import os import urllib -import uuid from typing import TYPE_CHECKING, List, Optional, Sequence, Type, TypeVar, Union from uuid import UUID From ab147279d3ac4bcfa0248bab6309459b1a238bc2 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 19 Jul 2021 10:01:39 -0700 Subject: [PATCH 029/103] fix --- src/api-service/tests/test_request_auth.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/api-service/tests/test_request_auth.py b/src/api-service/tests/test_request_auth.py index 4b4981310a..9bea7df68a 100644 --- a/src/api-service/tests/test_request_auth.py +++ b/src/api-service/tests/test_request_auth.py @@ -48,7 +48,7 @@ def test_adding_rule_on_same_path(self): "a/b/c", RequestAuthorization.Rules(allowed_groups_ids=[]) ) self.fail("this is expected to fail") - except: + except Exception: pass # The most specific rules takes priority over the ones containing a wildcard From 8952156d32b6fb1527364667221e02da4b3927f5 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 19 Jul 2021 10:34:52 -0700 Subject: [PATCH 030/103] formatting --- src/api-service/__app__/onefuzzlib/azure/creds.py | 5 +++-- src/api-service/__app__/onefuzzlib/request.py | 6 +++--- src/api-service/tests/test_request_auth.py | 10 +++++----- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/azure/creds.py b/src/api-service/__app__/onefuzzlib/azure/creds.py index 263d1bbd18..808aa78669 100644 --- a/src/api-service/__app__/onefuzzlib/azure/creds.py +++ b/src/api-service/__app__/onefuzzlib/azure/creds.py @@ -133,13 +133,14 @@ def query_microsoft_graph( ) -def is_member_of(group_ids: List[str], member_id: str) -> bool: +def is_member_of(group_ids: List[UUID], member_id: UUID) -> bool: body = {"groupIds": group_ids} response = query_microsoft_graph( method="POST", resource=f"users/{member_id}/checkMemberGroups", body=body ) + result = map(UUID, response["value"]) for group_id in group_ids: - if group_id not in response["value"]: + if group_id not in result: return False return True diff --git a/src/api-service/__app__/onefuzzlib/request.py b/src/api-service/__app__/onefuzzlib/request.py index b938cbe2eb..40d597e7a7 100644 --- a/src/api-service/__app__/onefuzzlib/request.py +++ b/src/api-service/__app__/onefuzzlib/request.py @@ -52,7 +52,7 @@ def get_rules() -> Optional[RequestAuthorization]: RequestAuthorization.Rules(allowed_groups_ids=rule.allowed_groups), ) - request_auth + return request_auth # todo: @@ -67,7 +67,7 @@ def check_access2(req: HttpRequest) -> Optional[Error]: path = urllib.parse.urlparse(req.url).path rule = rules.get_matching_rules(path) - member_id = req.headers["x-ms-client-principal-id"] + member_id = UUID(req.headers["x-ms-client-principal-id"]) try: result = is_member_of(rule.allowed_groups_ids, member_id) @@ -93,7 +93,7 @@ def check_access(req: HttpRequest) -> Optional[Error]: if "ONEFUZZ_AAD_GROUP_ID" not in os.environ: return None - group_id = os.environ["ONEFUZZ_AAD_GROUP_ID"] + group_id = UUID(os.environ["ONEFUZZ_AAD_GROUP_ID"]) member_id = req.headers["x-ms-client-principal-id"] try: result = is_member_of([group_id], member_id) diff --git a/src/api-service/tests/test_request_auth.py b/src/api-service/tests/test_request_auth.py index 9bea7df68a..e57dcef42e 100644 --- a/src/api-service/tests/test_request_auth.py +++ b/src/api-service/tests/test_request_auth.py @@ -22,7 +22,7 @@ def test_exact_match(self) -> None: self.assertEqual(len(rules2.allowed_groups_ids), 0, "expected nothing") - def test_wildcard(self): + def test_wildcard(self) -> None: guid1 = uuid.uuid4() request_trie = RequestAuthorization() @@ -35,7 +35,7 @@ def test_wildcard(self): self.assertNotEqual(len(rules.allowed_groups_ids), 0, "empty allowed groups") self.assertEqual(rules.allowed_groups_ids[0], guid1) - def test_adding_rule_on_same_path(self): + def test_adding_rule_on_same_path(self) -> None: guid1 = uuid.uuid4() request_trie = RequestAuthorization() @@ -52,7 +52,7 @@ def test_adding_rule_on_same_path(self): pass # The most specific rules takes priority over the ones containing a wildcard - def test_priority(self): + def test_priority(self) -> None: guid1 = uuid.uuid4() guid2 = uuid.uuid4() @@ -76,7 +76,7 @@ def test_priority(self): # if a path has no specific rule. it inherits from the parents # /a/b/c inherint from a/b # todo test the wildcard behavior - def test_inherit_rule(self): + def test_inherit_rule(self) -> None: guid1 = uuid.uuid4() guid2 = uuid.uuid4() @@ -100,7 +100,7 @@ def test_inherit_rule(self): ) # the lowest level rule override the parent rules - def test_override_rule(self): + def test_override_rule(self) -> None: guid1 = uuid.uuid4() guid2 = uuid.uuid4() From 0e535407855150f7d1e6773c9586a12575eeaa2b Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 19 Jul 2021 11:29:07 -0700 Subject: [PATCH 031/103] adding testing function --- .../__app__/onefuzzlib/azure/creds.py | 23 +++++++++++++++++++ src/api-service/__app__/onefuzzlib/request.py | 12 ++++++---- 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/azure/creds.py b/src/api-service/__app__/onefuzzlib/azure/creds.py index 808aa78669..9f5c808bbd 100644 --- a/src/api-service/__app__/onefuzzlib/azure/creds.py +++ b/src/api-service/__app__/onefuzzlib/azure/creds.py @@ -133,6 +133,29 @@ def query_microsoft_graph( ) +######## FOR TESTING PURPOSE ONLY ############ +def is_member_of_test(group_ids: List[UUID], member_id: UUID) -> bool: + from pydantic import BaseModel + from pydantic.tools import parse_obj_as + + class GroupMemebership(BaseModel): + principal_id: UUID + groups: List[UUID] + + if os.environ["TEST_MSGRAPH_AAD"]: + data = parse_obj_as(List[GroupMemebership], os.environ["TEST_MSGRAPH_AAD"]) + for membership in data: + if membership.principal_id == member_id: + for group_id in group_ids: + if group_id not in membership.groups: + return False + return True + return False + + +######################################### + + def is_member_of(group_ids: List[UUID], member_id: UUID) -> bool: body = {"groupIds": group_ids} response = query_microsoft_graph( diff --git a/src/api-service/__app__/onefuzzlib/request.py b/src/api-service/__app__/onefuzzlib/request.py index 40d597e7a7..9f938c1ca0 100644 --- a/src/api-service/__app__/onefuzzlib/request.py +++ b/src/api-service/__app__/onefuzzlib/request.py @@ -19,7 +19,7 @@ from pydantic import ValidationError from pydantic.tools import parse_obj_as -from .azure.creds import is_member_of +from .azure.creds import is_member_of, is_member_of_test from .orm import ModelMixin from .request_auth import RequestAuthorization @@ -55,10 +55,11 @@ def get_rules() -> Optional[RequestAuthorization]: return request_auth +Testing = True # todo: # - check the verb # -def check_access2(req: HttpRequest) -> Optional[Error]: +def check_access(req: HttpRequest) -> Optional[Error]: rules = get_rules() if not rules: @@ -70,7 +71,10 @@ def check_access2(req: HttpRequest) -> Optional[Error]: member_id = UUID(req.headers["x-ms-client-principal-id"]) try: - result = is_member_of(rule.allowed_groups_ids, member_id) + if Testing: + result = is_member_of_test(rule.allowed_groups_ids, member_id) + else: + result = is_member_of(rule.allowed_groups_ids, member_id) except Exception as e: return Error( code=ErrorCode.UNAUTHORIZED, @@ -89,7 +93,7 @@ def check_access2(req: HttpRequest) -> Optional[Error]: return None -def check_access(req: HttpRequest) -> Optional[Error]: +def check_access_(req: HttpRequest) -> Optional[Error]: if "ONEFUZZ_AAD_GROUP_ID" not in os.environ: return None From 6bb50cdbb65a87aa07bac8cf926fa10f4e98ac95 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 19 Jul 2021 11:49:55 -0700 Subject: [PATCH 032/103] formatting --- src/api-service/__app__/onefuzzlib/azure/creds.py | 6 ++---- src/api-service/__app__/onefuzzlib/request.py | 2 ++ 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/azure/creds.py b/src/api-service/__app__/onefuzzlib/azure/creds.py index 9f5c808bbd..1bd5ed222b 100644 --- a/src/api-service/__app__/onefuzzlib/azure/creds.py +++ b/src/api-service/__app__/onefuzzlib/azure/creds.py @@ -133,7 +133,7 @@ def query_microsoft_graph( ) -######## FOR TESTING PURPOSE ONLY ############ +# FOR TESTING PURPOSE ONLY ############ def is_member_of_test(group_ids: List[UUID], member_id: UUID) -> bool: from pydantic import BaseModel from pydantic.tools import parse_obj_as @@ -151,9 +151,7 @@ class GroupMemebership(BaseModel): return False return True return False - - -######################################### +# ######################################## def is_member_of(group_ids: List[UUID], member_id: UUID) -> bool: diff --git a/src/api-service/__app__/onefuzzlib/request.py b/src/api-service/__app__/onefuzzlib/request.py index 9f938c1ca0..dbef18d556 100644 --- a/src/api-service/__app__/onefuzzlib/request.py +++ b/src/api-service/__app__/onefuzzlib/request.py @@ -59,6 +59,8 @@ def get_rules() -> Optional[RequestAuthorization]: # todo: # - check the verb # + + def check_access(req: HttpRequest) -> Optional[Error]: rules = get_rules() From a70fbd6028a39fc9292a7348165f51bce05d09a1 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 19 Jul 2021 12:04:15 -0700 Subject: [PATCH 033/103] formatting --- src/api-service/__app__/onefuzzlib/azure/creds.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/api-service/__app__/onefuzzlib/azure/creds.py b/src/api-service/__app__/onefuzzlib/azure/creds.py index 1bd5ed222b..905ffd9a5d 100644 --- a/src/api-service/__app__/onefuzzlib/azure/creds.py +++ b/src/api-service/__app__/onefuzzlib/azure/creds.py @@ -151,6 +151,8 @@ class GroupMemebership(BaseModel): return False return True return False + + # ######################################## From a9e0bf89b8a0c19278b338f8e91fe1021afa0d0e Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 19 Jul 2021 13:17:33 -0700 Subject: [PATCH 034/103] build fix --- src/api-service/__app__/onefuzzlib/azure/creds.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/api-service/__app__/onefuzzlib/azure/creds.py b/src/api-service/__app__/onefuzzlib/azure/creds.py index 905ffd9a5d..ab3afab16a 100644 --- a/src/api-service/__app__/onefuzzlib/azure/creds.py +++ b/src/api-service/__app__/onefuzzlib/azure/creds.py @@ -150,7 +150,7 @@ class GroupMemebership(BaseModel): if group_id not in membership.groups: return False return True - return False + return False # ######################################## From e4618f8331e8f0514a73c3061f10434a084516b6 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 20 Jul 2021 10:23:59 -0700 Subject: [PATCH 035/103] bug fixes --- .../__app__/onefuzzlib/azure/creds.py | 21 +++++++++++-------- src/api-service/__app__/onefuzzlib/request.py | 11 +++++----- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/azure/creds.py b/src/api-service/__app__/onefuzzlib/azure/creds.py index ab3afab16a..fae13a71e6 100644 --- a/src/api-service/__app__/onefuzzlib/azure/creds.py +++ b/src/api-service/__app__/onefuzzlib/azure/creds.py @@ -136,20 +136,23 @@ def query_microsoft_graph( # FOR TESTING PURPOSE ONLY ############ def is_member_of_test(group_ids: List[UUID], member_id: UUID) -> bool: from pydantic import BaseModel - from pydantic.tools import parse_obj_as + from pydantic.tools import parse_raw_as class GroupMemebership(BaseModel): principal_id: UUID groups: List[UUID] - if os.environ["TEST_MSGRAPH_AAD"]: - data = parse_obj_as(List[GroupMemebership], os.environ["TEST_MSGRAPH_AAD"]) - for membership in data: - if membership.principal_id == member_id: - for group_id in group_ids: - if group_id not in membership.groups: - return False - return True + memberships = os.environ.get("TEST_MSGRAPH_AAD") + if memberships is None: + return True + + data = parse_raw_as(List[GroupMemebership], memberships) + for membership in data: + if membership.principal_id == member_id: + for group_id in group_ids: + if group_id not in membership.groups: + return False + return True return False diff --git a/src/api-service/__app__/onefuzzlib/request.py b/src/api-service/__app__/onefuzzlib/request.py index dbef18d556..7c2abc5abb 100644 --- a/src/api-service/__app__/onefuzzlib/request.py +++ b/src/api-service/__app__/onefuzzlib/request.py @@ -17,7 +17,7 @@ from onefuzztypes.responses import BaseResponse from pydantic import BaseModel # noqa: F401 from pydantic import ValidationError -from pydantic.tools import parse_obj_as +from pydantic.tools import parse_obj_as, parse_raw_as from .azure.creds import is_member_of, is_member_of_test from .orm import ModelMixin @@ -39,12 +39,11 @@ class RuleDefinition(BaseModel): @cached def get_rules() -> Optional[RequestAuthorization]: - # todo: move to instacewide configuration - rules_data = os.environ["ONEFUZZ_AAD_GROUP_RULES"] - if not rules_data: + # todo: move to instancewide configuration + rules_data = os.environ.get("ONEFUZZ_AAD_GROUP_RULES") + if rules_data is None: return None - - rules = parse_obj_as(List[RuleDefinition], rules_data) + rules = parse_raw_as(List[RuleDefinition], rules_data) request_auth = RequestAuthorization() for rule in rules: request_auth.add_url( From 4d6bcd93fef7cc3729ddc776aec93239ad23c07f Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 20 Jul 2021 11:13:00 -0700 Subject: [PATCH 036/103] add support for http method --- src/api-service/__app__/onefuzzlib/request.py | 6 +-- .../__app__/onefuzzlib/request_auth.py | 34 ++++++++++---- src/api-service/tests/test_request_auth.py | 46 +++++++++++-------- 3 files changed, 56 insertions(+), 30 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/request.py b/src/api-service/__app__/onefuzzlib/request.py index 7c2abc5abb..bea69441e3 100644 --- a/src/api-service/__app__/onefuzzlib/request.py +++ b/src/api-service/__app__/onefuzzlib/request.py @@ -47,6 +47,7 @@ def get_rules() -> Optional[RequestAuthorization]: request_auth = RequestAuthorization() for rule in rules: request_auth.add_url( + rule.methods, rule.endpoint, RequestAuthorization.Rules(allowed_groups_ids=rule.allowed_groups), ) @@ -55,9 +56,6 @@ def get_rules() -> Optional[RequestAuthorization]: Testing = True -# todo: -# - check the verb -# def check_access(req: HttpRequest) -> Optional[Error]: @@ -67,7 +65,7 @@ def check_access(req: HttpRequest) -> Optional[Error]: return None path = urllib.parse.urlparse(req.url).path - rule = rules.get_matching_rules(path) + rule = rules.get_matching_rules(req.method, path) member_id = UUID(req.headers["x-ms-client-principal-id"]) diff --git a/src/api-service/__app__/onefuzzlib/request_auth.py b/src/api-service/__app__/onefuzzlib/request_auth.py index fa9962e8e9..fd89bd1ffe 100644 --- a/src/api-service/__app__/onefuzzlib/request_auth.py +++ b/src/api-service/__app__/onefuzzlib/request_auth.py @@ -1,6 +1,8 @@ -from typing import Dict, List +from typing import Dict, List, Optional from uuid import UUID +from memoization.memoization import cached + class RequestAuthorization: """ @@ -14,11 +16,13 @@ def __init__(self, allowed_groups_ids: List[UUID] = []) -> None: self.allowed_groups_ids = allowed_groups_ids class Node: - rules: "RequestAuthorization.Rules" + # http method -> rules + rules: Dict[str, "RequestAuthorization.Rules"] + # path -> node children: Dict[str, "RequestAuthorization.Node"] def __init__(self) -> None: - self.rules = RequestAuthorization.Rules() + self.rules = {} self.children = {} pass @@ -27,7 +31,8 @@ def __init__(self) -> None: def __init__(self) -> None: self.root = RequestAuthorization.Node() - def add_url(self, path: str, rules: Rules) -> None: + def add_url(self, methods: List[str], path: str, rules: Rules) -> None: + methods = map(lambda m: m.upper(), methods) segments = path.split("/") if len(segments) == 0: return @@ -46,7 +51,9 @@ def add_url(self, path: str, rules: Rules) -> None: # we found a node matching this exact path # This means that there is an existing rule causing a conflict if current_segment_index == len(segments): - raise Exception(f"Conflicting path {path}") + for method in methods: + if method in current_node.rules: + raise Exception(f"Conflicting rules on {method} {path}") while current_segment_index < len(segments): current_segment = segments[current_segment_index] @@ -54,11 +61,20 @@ def add_url(self, path: str, rules: Rules) -> None: current_node = current_node.children[current_segment] current_segment_index = current_segment_index + 1 - current_node.rules = rules + for method in methods: + current_node.rules[method] = rules - def get_matching_rules(self, path: str) -> Rules: + @cached + def get_matching_rules(self, method: str, path: str) -> Rules: + method = method.upper() segments = path.split("/") current_node = self.root + + if method in current_node.rules: + current_rule = current_node.rules[method] + else: + current_rule = RequestAuthorization.Rules() + current_segment_index = 0 while current_segment_index < len(segments): @@ -70,6 +86,8 @@ def get_matching_rules(self, path: str) -> Rules: else: break + if method in current_node.rules: + current_rule = current_node.rules[method] current_segment_index = current_segment_index + 1 - return current_node.rules + return current_rule diff --git a/src/api-service/tests/test_request_auth.py b/src/api-service/tests/test_request_auth.py index e57dcef42e..5a4e08efba 100644 --- a/src/api-service/tests/test_request_auth.py +++ b/src/api-service/tests/test_request_auth.py @@ -11,11 +11,11 @@ def test_exact_match(self) -> None: request_trie = RequestAuthorization() request_trie.add_url( - "a/b/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1]) + ["get"], "a/b/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1]) ) - rules1 = request_trie.get_matching_rules("a/b/c") - rules2 = request_trie.get_matching_rules("b/b/e") + rules1 = request_trie.get_matching_rules("get", "a/b/c") + rules2 = request_trie.get_matching_rules("get", "b/b/e") self.assertNotEqual(len(rules1.allowed_groups_ids), 0, "empty allowed groups") self.assertEqual(rules1.allowed_groups_ids[0], guid1) @@ -27,10 +27,10 @@ def test_wildcard(self) -> None: request_trie = RequestAuthorization() request_trie.add_url( - "b/*/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1]) + ["get"], "b/*/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1]) ) - rules = request_trie.get_matching_rules("b/b/c") + rules = request_trie.get_matching_rules("get", "b/b/c") self.assertNotEqual(len(rules.allowed_groups_ids), 0, "empty allowed groups") self.assertEqual(rules.allowed_groups_ids[0], guid1) @@ -40,12 +40,12 @@ def test_adding_rule_on_same_path(self) -> None: request_trie = RequestAuthorization() request_trie.add_url( - "a/b/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1]) + ["get"], "a/b/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1]) ) try: request_trie.add_url( - "a/b/c", RequestAuthorization.Rules(allowed_groups_ids=[]) + ["get"], "a/b/c", RequestAuthorization.Rules(allowed_groups_ids=[]) ) self.fail("this is expected to fail") except Exception: @@ -58,14 +58,14 @@ def test_priority(self) -> None: request_trie = RequestAuthorization() request_trie.add_url( - "a/*/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1]) + ["get"], "a/*/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1]) ) request_trie.add_url( - "a/b/c", RequestAuthorization.Rules(allowed_groups_ids=[guid2]) + ["get"], "a/b/c", RequestAuthorization.Rules(allowed_groups_ids=[guid2]) ) - rules = request_trie.get_matching_rules("a/b/c") + rules = request_trie.get_matching_rules("get", "a/b/c") self.assertEqual( rules.allowed_groups_ids[0], @@ -79,26 +79,36 @@ def test_priority(self) -> None: def test_inherit_rule(self) -> None: guid1 = uuid.uuid4() guid2 = uuid.uuid4() + guid3 = uuid.uuid4() request_trie = RequestAuthorization() request_trie.add_url( - "a/b/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1]) + ["get"], "a/b/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1]) ) request_trie.add_url( - "f/*/c", RequestAuthorization.Rules(allowed_groups_ids=[guid2]) + ["get"], "f/*/c", RequestAuthorization.Rules(allowed_groups_ids=[guid2]) ) - rules = request_trie.get_matching_rules("a/b/c/d") + request_trie.add_url( + ["post"], "a/b", RequestAuthorization.Rules(allowed_groups_ids=[guid3]) + ) + + rules = request_trie.get_matching_rules("get", "a/b/c/d") self.assertEqual( rules.allowed_groups_ids[0], guid1, "expected to inherit rule of a/b/c" ) - rules = request_trie.get_matching_rules("f/b/c/d") + rules = request_trie.get_matching_rules("get", "f/b/c/d") self.assertEqual( rules.allowed_groups_ids[0], guid2, "expected to inherit rule of f/*/c" ) + rules = request_trie.get_matching_rules("post", "a/b/c/d") + self.assertEqual( + rules.allowed_groups_ids[0], guid3, "expected to inherit rule of post a/b/c" + ) + # the lowest level rule override the parent rules def test_override_rule(self) -> None: guid1 = uuid.uuid4() @@ -106,19 +116,19 @@ def test_override_rule(self) -> None: request_trie = RequestAuthorization() request_trie.add_url( - "a/b/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1]) + ["get"], "a/b/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1]) ) request_trie.add_url( - "a/b/c/d", RequestAuthorization.Rules(allowed_groups_ids=[guid2]) + ["get"], "a/b/c/d", RequestAuthorization.Rules(allowed_groups_ids=[guid2]) ) - rules = request_trie.get_matching_rules("a/b/c") + rules = request_trie.get_matching_rules("get", "a/b/c") self.assertEqual( rules.allowed_groups_ids[0], guid1, "expected to inherit rule of a/b/c" ) - rules = request_trie.get_matching_rules("a/b/c/d") + rules = request_trie.get_matching_rules("get", "a/b/c/d") self.assertEqual( rules.allowed_groups_ids[0], guid2, "expected to inherit rule of a/b/c/d" ) From b58ac558bbd24ae19ea7fb06d98885dd030e51b4 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 20 Jul 2021 11:51:50 -0700 Subject: [PATCH 037/103] fix tests --- src/api-service/__app__/onefuzzlib/request.py | 2 +- src/api-service/__app__/onefuzzlib/request_auth.py | 6 +++--- src/api-service/tests/test_request_auth.py | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/request.py b/src/api-service/__app__/onefuzzlib/request.py index bea69441e3..c06cb62ddf 100644 --- a/src/api-service/__app__/onefuzzlib/request.py +++ b/src/api-service/__app__/onefuzzlib/request.py @@ -17,7 +17,7 @@ from onefuzztypes.responses import BaseResponse from pydantic import BaseModel # noqa: F401 from pydantic import ValidationError -from pydantic.tools import parse_obj_as, parse_raw_as +from pydantic.tools import parse_raw_as from .azure.creds import is_member_of, is_member_of_test from .orm import ModelMixin diff --git a/src/api-service/__app__/onefuzzlib/request_auth.py b/src/api-service/__app__/onefuzzlib/request_auth.py index fd89bd1ffe..2c72c69ee0 100644 --- a/src/api-service/__app__/onefuzzlib/request_auth.py +++ b/src/api-service/__app__/onefuzzlib/request_auth.py @@ -1,4 +1,4 @@ -from typing import Dict, List, Optional +from typing import Dict, List from uuid import UUID from memoization.memoization import cached @@ -32,7 +32,8 @@ def __init__(self) -> None: self.root = RequestAuthorization.Node() def add_url(self, methods: List[str], path: str, rules: Rules) -> None: - methods = map(lambda m: m.upper(), methods) + methods = list(map(lambda m: m.upper(), methods)) + segments = path.split("/") if len(segments) == 0: return @@ -47,7 +48,6 @@ def add_url(self, methods: List[str], path: str, rules: Rules) -> None: current_segment_index = current_segment_index + 1 else: break - # we found a node matching this exact path # This means that there is an existing rule causing a conflict if current_segment_index == len(segments): diff --git a/src/api-service/tests/test_request_auth.py b/src/api-service/tests/test_request_auth.py index 5a4e08efba..d9dabe9127 100644 --- a/src/api-service/tests/test_request_auth.py +++ b/src/api-service/tests/test_request_auth.py @@ -106,7 +106,7 @@ def test_inherit_rule(self) -> None: rules = request_trie.get_matching_rules("post", "a/b/c/d") self.assertEqual( - rules.allowed_groups_ids[0], guid3, "expected to inherit rule of post a/b/c" + rules.allowed_groups_ids[0], guid3, "expected to inherit rule of post a/b" ) # the lowest level rule override the parent rules From 31e5341407342e0784fb9ecbc1db6da322b8b773 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 20 Jul 2021 12:26:10 -0700 Subject: [PATCH 038/103] build fix attempt --- src/api-service/__app__/onefuzzlib/request_auth.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/request_auth.py b/src/api-service/__app__/onefuzzlib/request_auth.py index 2c72c69ee0..a4d7429bb8 100644 --- a/src/api-service/__app__/onefuzzlib/request_auth.py +++ b/src/api-service/__app__/onefuzzlib/request_auth.py @@ -1,8 +1,6 @@ from typing import Dict, List from uuid import UUID -from memoization.memoization import cached - class RequestAuthorization: """ @@ -64,7 +62,6 @@ def add_url(self, methods: List[str], path: str, rules: Rules) -> None: for method in methods: current_node.rules[method] = rules - @cached def get_matching_rules(self, method: str, path: str) -> Rules: method = method.upper() segments = path.split("/") From c3e5b38aaf5330b47d13c86cc4c703dda29c6e76 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 20 Jul 2021 13:29:30 -0700 Subject: [PATCH 039/103] fix typo --- src/deployment/registration.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/deployment/registration.py b/src/deployment/registration.py index f39447e925..db0605f18d 100644 --- a/src/deployment/registration.py +++ b/src/deployment/registration.py @@ -151,7 +151,7 @@ def register_application( if not (onefuzz_app): raise Exception("onefuzz app not found") - pre_authorized_applications = onefuzz_app["apiApplication"][ + pre_authorized_applications = onefuzz_app["api"][ "preAuthorizedApplications" ] From 174cd86f4fcc9831b7eb7e2a21a666545846dbca Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 20 Jul 2021 13:51:46 -0700 Subject: [PATCH 040/103] format --- src/deployment/registration.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/deployment/registration.py b/src/deployment/registration.py index db0605f18d..687a3c98d1 100644 --- a/src/deployment/registration.py +++ b/src/deployment/registration.py @@ -151,9 +151,7 @@ def register_application( if not (onefuzz_app): raise Exception("onefuzz app not found") - pre_authorized_applications = onefuzz_app["api"][ - "preAuthorizedApplications" - ] + pre_authorized_applications = onefuzz_app["api"]["preAuthorizedApplications"] if app["appId"] not in [app["appId"] for app in pre_authorized_applications]: authorize_application(UUID(app["appId"]), UUID(onefuzz_app["appId"])) From 088dc4db4bf4e731c20cf117ca54e1cdb43e0e70 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 20 Jul 2021 14:22:13 -0700 Subject: [PATCH 041/103] deployment fix --- src/deployment/registration.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/deployment/registration.py b/src/deployment/registration.py index 687a3c98d1..b5fe948361 100644 --- a/src/deployment/registration.py +++ b/src/deployment/registration.py @@ -176,7 +176,7 @@ def create_application_credential(application_name: str, subscription_id: str) - raise Exception("app not found") (_, password) = add_application_password( - f"{application_name}_password", app["objectId"], subscription_id + f"{application_name}_password", app["id"], subscription_id ) return str(password) From b402038e0737f9903d684628b94ed4a29aa12077 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 20 Jul 2021 17:16:22 -0700 Subject: [PATCH 042/103] set implicitGrantSettings in the deployment --- src/deployment/deploy.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 7a8539f253..7b0f706b7a 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -314,7 +314,13 @@ def setup_rbac(self) -> None: } ] }, - "web": {"redirectUris": [f"{url}/.auth/login/aad/callback"]}, + "web": { + "implicitGrantSettings": { + "enableAccessTokenIssuance": False, + "enableIdTokenIssuance": True + }, + "redirectUris": [f"{url}/.auth/login/aad/callback"] + }, "requiredResourceAccess": [ { "resourceAccess": [ From 9996678273af833fed7c6f950e97b0882848d695 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 20 Jul 2021 18:11:01 -0700 Subject: [PATCH 043/103] format --- src/deployment/deploy.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 7b0f706b7a..1481d61b43 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -315,12 +315,12 @@ def setup_rbac(self) -> None: ] }, "web": { - "implicitGrantSettings": { - "enableAccessTokenIssuance": False, - "enableIdTokenIssuance": True - }, - "redirectUris": [f"{url}/.auth/login/aad/callback"] + "implicitGrantSettings": { + "enableAccessTokenIssuance": False, + "enableIdTokenIssuance": True, }, + "redirectUris": [f"{url}/.auth/login/aad/callback"], + }, "requiredResourceAccess": [ { "resourceAccess": [ From b9fcdac30893b994a83e4f467cee8e6bb6d0387f Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 20 Jul 2021 23:04:01 -0700 Subject: [PATCH 044/103] cleanup --- src/api-service/__app__/onefuzzlib/request.py | 20 +-- .../__app__/onefuzzlib/request_auth.py | 27 +++- src/api-service/tests/test_request_auth.py | 140 +++++++++++------- 3 files changed, 115 insertions(+), 72 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/request.py b/src/api-service/__app__/onefuzzlib/request.py index c06cb62ddf..e05d1698f7 100644 --- a/src/api-service/__app__/onefuzzlib/request.py +++ b/src/api-service/__app__/onefuzzlib/request.py @@ -7,7 +7,7 @@ import logging import os import urllib -from typing import TYPE_CHECKING, List, Optional, Sequence, Type, TypeVar, Union +from typing import TYPE_CHECKING, Optional, Sequence, Type, TypeVar, Union from uuid import UUID from azure.functions import HttpRequest, HttpResponse @@ -17,7 +17,6 @@ from onefuzztypes.responses import BaseResponse from pydantic import BaseModel # noqa: F401 from pydantic import ValidationError -from pydantic.tools import parse_raw_as from .azure.creds import is_member_of, is_member_of_test from .orm import ModelMixin @@ -30,29 +29,14 @@ from onefuzztypes.requests import BaseRequest # noqa: F401 -# todo add top level rule -class RuleDefinition(BaseModel): - methods: List[str] - endpoint: str - allowed_groups: List[UUID] - - @cached def get_rules() -> Optional[RequestAuthorization]: # todo: move to instancewide configuration rules_data = os.environ.get("ONEFUZZ_AAD_GROUP_RULES") if rules_data is None: return None - rules = parse_raw_as(List[RuleDefinition], rules_data) - request_auth = RequestAuthorization() - for rule in rules: - request_auth.add_url( - rule.methods, - rule.endpoint, - RequestAuthorization.Rules(allowed_groups_ids=rule.allowed_groups), - ) - return request_auth + return RequestAuthorization.parse_rules(rules_data) Testing = True diff --git a/src/api-service/__app__/onefuzzlib/request_auth.py b/src/api-service/__app__/onefuzzlib/request_auth.py index a4d7429bb8..987b09e7a1 100644 --- a/src/api-service/__app__/onefuzzlib/request_auth.py +++ b/src/api-service/__app__/onefuzzlib/request_auth.py @@ -1,5 +1,12 @@ from typing import Dict, List from uuid import UUID +from pydantic import BaseModel, parse_raw_as + + +class RuleDefinition(BaseModel): + methods: List[str] + endpoint: str + allowed_groups: List[UUID] class RequestAuthorization: @@ -29,7 +36,7 @@ def __init__(self) -> None: def __init__(self) -> None: self.root = RequestAuthorization.Node() - def add_url(self, methods: List[str], path: str, rules: Rules) -> None: + def __add_url__(self, methods: List[str], path: str, rules: Rules) -> None: methods = list(map(lambda m: m.upper(), methods)) segments = path.split("/") @@ -86,5 +93,21 @@ def get_matching_rules(self, method: str, path: str) -> Rules: if method in current_node.rules: current_rule = current_node.rules[method] current_segment_index = current_segment_index + 1 - return current_rule + + @classmethod + def parse_rules(cls, rules_data: str) -> "RequestAuthorization": + rules = parse_raw_as(List[RuleDefinition], rules_data) + return cls.build(rules) + + @classmethod + def build(cls, rules: List[RuleDefinition]) -> "RequestAuthorization": + request_auth = RequestAuthorization() + for rule in rules: + request_auth.__add_url__( + rule.methods, + rule.endpoint, + RequestAuthorization.Rules(allowed_groups_ids=rule.allowed_groups), + ) + + return request_auth diff --git a/src/api-service/tests/test_request_auth.py b/src/api-service/tests/test_request_auth.py index d9dabe9127..f89f6efaf0 100644 --- a/src/api-service/tests/test_request_auth.py +++ b/src/api-service/tests/test_request_auth.py @@ -1,7 +1,7 @@ import unittest import uuid -from __app__.onefuzzlib.request_auth import RequestAuthorization +from __app__.onefuzzlib.request_auth import RequestAuthorization, RuleDefinition class TestRequestAuthorization(unittest.TestCase): @@ -9,13 +9,18 @@ def test_exact_match(self) -> None: guid1 = uuid.uuid4() - request_trie = RequestAuthorization() - request_trie.add_url( - ["get"], "a/b/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1]) + request_auth = RequestAuthorization.build( + [ + RuleDefinition( + methods=["get"], + endpoint="a/b/c", + allowed_groups=[guid1], + ) + ] ) - rules1 = request_trie.get_matching_rules("get", "a/b/c") - rules2 = request_trie.get_matching_rules("get", "b/b/e") + rules1 = request_auth.get_matching_rules("get", "a/b/c") + rules2 = request_auth.get_matching_rules("get", "b/b/e") self.assertNotEqual(len(rules1.allowed_groups_ids), 0, "empty allowed groups") self.assertEqual(rules1.allowed_groups_ids[0], guid1) @@ -25,12 +30,17 @@ def test_exact_match(self) -> None: def test_wildcard(self) -> None: guid1 = uuid.uuid4() - request_trie = RequestAuthorization() - request_trie.add_url( - ["get"], "b/*/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1]) + request_auth = RequestAuthorization.build( + [ + RuleDefinition( + methods=["get"], + endpoint="b/*/c", + allowed_groups=[guid1], + ) + ] ) - rules = request_trie.get_matching_rules("get", "b/b/c") + rules = request_auth.get_matching_rules("get", "b/b/c") self.assertNotEqual(len(rules.allowed_groups_ids), 0, "empty allowed groups") self.assertEqual(rules.allowed_groups_ids[0], guid1) @@ -38,15 +48,22 @@ def test_wildcard(self) -> None: def test_adding_rule_on_same_path(self) -> None: guid1 = uuid.uuid4() - request_trie = RequestAuthorization() - request_trie.add_url( - ["get"], "a/b/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1]) - ) - try: - request_trie.add_url( - ["get"], "a/b/c", RequestAuthorization.Rules(allowed_groups_ids=[]) + RequestAuthorization.build( + [ + RuleDefinition( + methods=["get"], + endpoint="a/b/c", + allowed_groups=[guid1], + ), + RuleDefinition( + methods=["get"], + endpoint="a/b/c", + allowed_groups=[], + ), + ] ) + self.fail("this is expected to fail") except Exception: pass @@ -56,16 +73,22 @@ def test_priority(self) -> None: guid1 = uuid.uuid4() guid2 = uuid.uuid4() - request_trie = RequestAuthorization() - request_trie.add_url( - ["get"], "a/*/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1]) - ) - - request_trie.add_url( - ["get"], "a/b/c", RequestAuthorization.Rules(allowed_groups_ids=[guid2]) + request_auth = RequestAuthorization.build( + [ + RuleDefinition( + methods=["get"], + endpoint="a/*/c", + allowed_groups=[guid1], + ), + RuleDefinition( + methods=["get"], + endpoint="a/b/c", + allowed_groups=[guid2], + ), + ] ) - rules = request_trie.get_matching_rules("get", "a/b/c") + rules = request_auth.get_matching_rules("get", "a/b/c") self.assertEqual( rules.allowed_groups_ids[0], @@ -81,30 +104,37 @@ def test_inherit_rule(self) -> None: guid2 = uuid.uuid4() guid3 = uuid.uuid4() - request_trie = RequestAuthorization() - request_trie.add_url( - ["get"], "a/b/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1]) - ) - - request_trie.add_url( - ["get"], "f/*/c", RequestAuthorization.Rules(allowed_groups_ids=[guid2]) - ) - - request_trie.add_url( - ["post"], "a/b", RequestAuthorization.Rules(allowed_groups_ids=[guid3]) - ) - - rules = request_trie.get_matching_rules("get", "a/b/c/d") + request_auth = RequestAuthorization.build( + [ + RuleDefinition( + methods=["get"], + endpoint="a/b/c", + allowed_groups=[guid1], + ), + RuleDefinition( + methods=["get"], + endpoint="f/*/c", + allowed_groups=[guid2], + ), + RuleDefinition( + methods=["post"], + endpoint="a/b", + allowed_groups=[guid3], + ), + ] + ) + + rules = request_auth.get_matching_rules("get", "a/b/c/d") self.assertEqual( rules.allowed_groups_ids[0], guid1, "expected to inherit rule of a/b/c" ) - rules = request_trie.get_matching_rules("get", "f/b/c/d") + rules = request_auth.get_matching_rules("get", "f/b/c/d") self.assertEqual( rules.allowed_groups_ids[0], guid2, "expected to inherit rule of f/*/c" ) - rules = request_trie.get_matching_rules("post", "a/b/c/d") + rules = request_auth.get_matching_rules("post", "a/b/c/d") self.assertEqual( rules.allowed_groups_ids[0], guid3, "expected to inherit rule of post a/b" ) @@ -114,21 +144,27 @@ def test_override_rule(self) -> None: guid1 = uuid.uuid4() guid2 = uuid.uuid4() - request_trie = RequestAuthorization() - request_trie.add_url( - ["get"], "a/b/c", RequestAuthorization.Rules(allowed_groups_ids=[guid1]) - ) - - request_trie.add_url( - ["get"], "a/b/c/d", RequestAuthorization.Rules(allowed_groups_ids=[guid2]) - ) - - rules = request_trie.get_matching_rules("get", "a/b/c") + request_auth = RequestAuthorization.build( + [ + RuleDefinition( + methods=["get"], + endpoint="a/b/c", + allowed_groups=[guid1], + ), + RuleDefinition( + methods=["get"], + endpoint="a/b/c/d", + allowed_groups=[guid2], + ), + ] + ) + + rules = request_auth.get_matching_rules("get", "a/b/c") self.assertEqual( rules.allowed_groups_ids[0], guid1, "expected to inherit rule of a/b/c" ) - rules = request_trie.get_matching_rules("get", "a/b/c/d") + rules = request_auth.get_matching_rules("get", "a/b/c/d") self.assertEqual( rules.allowed_groups_ids[0], guid2, "expected to inherit rule of a/b/c/d" ) From f52c757a8f845f160e202dc18e061836e281151a Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 20 Jul 2021 23:34:54 -0700 Subject: [PATCH 045/103] format --- src/api-service/__app__/onefuzzlib/request_auth.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/api-service/__app__/onefuzzlib/request_auth.py b/src/api-service/__app__/onefuzzlib/request_auth.py index 987b09e7a1..031fab81c5 100644 --- a/src/api-service/__app__/onefuzzlib/request_auth.py +++ b/src/api-service/__app__/onefuzzlib/request_auth.py @@ -1,5 +1,6 @@ from typing import Dict, List from uuid import UUID + from pydantic import BaseModel, parse_raw_as From 212f18a4f926387267c23eb1b2cb3cbbca881bf7 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Wed, 21 Jul 2021 09:21:55 -0700 Subject: [PATCH 046/103] renaming RequestAuthorization to RequestAccess --- src/api-service/__app__/onefuzzlib/request.py | 6 ++-- .../{request_auth.py => request_access.py} | 24 ++++++------- ...request_auth.py => test_request_access.py} | 34 +++++++++---------- 3 files changed, 32 insertions(+), 32 deletions(-) rename src/api-service/__app__/onefuzzlib/{request_auth.py => request_access.py} (82%) rename src/api-service/tests/{test_request_auth.py => test_request_access.py} (80%) diff --git a/src/api-service/__app__/onefuzzlib/request.py b/src/api-service/__app__/onefuzzlib/request.py index e05d1698f7..672ba9dc07 100644 --- a/src/api-service/__app__/onefuzzlib/request.py +++ b/src/api-service/__app__/onefuzzlib/request.py @@ -20,7 +20,7 @@ from .azure.creds import is_member_of, is_member_of_test from .orm import ModelMixin -from .request_auth import RequestAuthorization +from .request_access import RequestAccess # We don't actually use these types at runtime at this time. Rather, # these are used in a bound TypeVar. MyPy suggests to only import these @@ -30,13 +30,13 @@ @cached -def get_rules() -> Optional[RequestAuthorization]: +def get_rules() -> Optional[RequestAccess]: # todo: move to instancewide configuration rules_data = os.environ.get("ONEFUZZ_AAD_GROUP_RULES") if rules_data is None: return None - return RequestAuthorization.parse_rules(rules_data) + return RequestAccess.parse_rules(rules_data) Testing = True diff --git a/src/api-service/__app__/onefuzzlib/request_auth.py b/src/api-service/__app__/onefuzzlib/request_access.py similarity index 82% rename from src/api-service/__app__/onefuzzlib/request_auth.py rename to src/api-service/__app__/onefuzzlib/request_access.py index 031fab81c5..d73ac46e07 100644 --- a/src/api-service/__app__/onefuzzlib/request_auth.py +++ b/src/api-service/__app__/onefuzzlib/request_access.py @@ -10,7 +10,7 @@ class RuleDefinition(BaseModel): allowed_groups: List[UUID] -class RequestAuthorization: +class RequestAccess: """ Stores the rules associated with a the request paths """ @@ -23,9 +23,9 @@ def __init__(self, allowed_groups_ids: List[UUID] = []) -> None: class Node: # http method -> rules - rules: Dict[str, "RequestAuthorization.Rules"] + rules: Dict[str, "RequestAccess.Rules"] # path -> node - children: Dict[str, "RequestAuthorization.Node"] + children: Dict[str, "RequestAccess.Node"] def __init__(self) -> None: self.rules = {} @@ -35,7 +35,7 @@ def __init__(self) -> None: root: Node def __init__(self) -> None: - self.root = RequestAuthorization.Node() + self.root = RequestAccess.Node() def __add_url__(self, methods: List[str], path: str, rules: Rules) -> None: methods = list(map(lambda m: m.upper(), methods)) @@ -63,7 +63,7 @@ def __add_url__(self, methods: List[str], path: str, rules: Rules) -> None: while current_segment_index < len(segments): current_segment = segments[current_segment_index] - current_node.children[current_segment] = RequestAuthorization.Node() + current_node.children[current_segment] = RequestAccess.Node() current_node = current_node.children[current_segment] current_segment_index = current_segment_index + 1 @@ -78,7 +78,7 @@ def get_matching_rules(self, method: str, path: str) -> Rules: if method in current_node.rules: current_rule = current_node.rules[method] else: - current_rule = RequestAuthorization.Rules() + current_rule = RequestAccess.Rules() current_segment_index = 0 @@ -97,18 +97,18 @@ def get_matching_rules(self, method: str, path: str) -> Rules: return current_rule @classmethod - def parse_rules(cls, rules_data: str) -> "RequestAuthorization": + def parse_rules(cls, rules_data: str) -> "RequestAccess": rules = parse_raw_as(List[RuleDefinition], rules_data) return cls.build(rules) @classmethod - def build(cls, rules: List[RuleDefinition]) -> "RequestAuthorization": - request_auth = RequestAuthorization() + def build(cls, rules: List[RuleDefinition]) -> "RequestAccess": + request_access = RequestAccess() for rule in rules: - request_auth.__add_url__( + request_access.__add_url__( rule.methods, rule.endpoint, - RequestAuthorization.Rules(allowed_groups_ids=rule.allowed_groups), + RequestAccess.Rules(allowed_groups_ids=rule.allowed_groups), ) - return request_auth + return request_access diff --git a/src/api-service/tests/test_request_auth.py b/src/api-service/tests/test_request_access.py similarity index 80% rename from src/api-service/tests/test_request_auth.py rename to src/api-service/tests/test_request_access.py index f89f6efaf0..73ef9211a5 100644 --- a/src/api-service/tests/test_request_auth.py +++ b/src/api-service/tests/test_request_access.py @@ -1,15 +1,15 @@ import unittest import uuid -from __app__.onefuzzlib.request_auth import RequestAuthorization, RuleDefinition +from __app__.onefuzzlib.request_access import RequestAccess, RuleDefinition -class TestRequestAuthorization(unittest.TestCase): +class TestRequestAccess(unittest.TestCase): def test_exact_match(self) -> None: guid1 = uuid.uuid4() - request_auth = RequestAuthorization.build( + request_access = RequestAccess.build( [ RuleDefinition( methods=["get"], @@ -19,8 +19,8 @@ def test_exact_match(self) -> None: ] ) - rules1 = request_auth.get_matching_rules("get", "a/b/c") - rules2 = request_auth.get_matching_rules("get", "b/b/e") + rules1 = request_access.get_matching_rules("get", "a/b/c") + rules2 = request_access.get_matching_rules("get", "b/b/e") self.assertNotEqual(len(rules1.allowed_groups_ids), 0, "empty allowed groups") self.assertEqual(rules1.allowed_groups_ids[0], guid1) @@ -30,7 +30,7 @@ def test_exact_match(self) -> None: def test_wildcard(self) -> None: guid1 = uuid.uuid4() - request_auth = RequestAuthorization.build( + request_access = RequestAccess.build( [ RuleDefinition( methods=["get"], @@ -40,7 +40,7 @@ def test_wildcard(self) -> None: ] ) - rules = request_auth.get_matching_rules("get", "b/b/c") + rules = request_access.get_matching_rules("get", "b/b/c") self.assertNotEqual(len(rules.allowed_groups_ids), 0, "empty allowed groups") self.assertEqual(rules.allowed_groups_ids[0], guid1) @@ -49,7 +49,7 @@ def test_adding_rule_on_same_path(self) -> None: guid1 = uuid.uuid4() try: - RequestAuthorization.build( + RequestAccess.build( [ RuleDefinition( methods=["get"], @@ -73,7 +73,7 @@ def test_priority(self) -> None: guid1 = uuid.uuid4() guid2 = uuid.uuid4() - request_auth = RequestAuthorization.build( + request_access = RequestAccess.build( [ RuleDefinition( methods=["get"], @@ -88,7 +88,7 @@ def test_priority(self) -> None: ] ) - rules = request_auth.get_matching_rules("get", "a/b/c") + rules = request_access.get_matching_rules("get", "a/b/c") self.assertEqual( rules.allowed_groups_ids[0], @@ -104,7 +104,7 @@ def test_inherit_rule(self) -> None: guid2 = uuid.uuid4() guid3 = uuid.uuid4() - request_auth = RequestAuthorization.build( + request_access = RequestAccess.build( [ RuleDefinition( methods=["get"], @@ -124,17 +124,17 @@ def test_inherit_rule(self) -> None: ] ) - rules = request_auth.get_matching_rules("get", "a/b/c/d") + rules = request_access.get_matching_rules("get", "a/b/c/d") self.assertEqual( rules.allowed_groups_ids[0], guid1, "expected to inherit rule of a/b/c" ) - rules = request_auth.get_matching_rules("get", "f/b/c/d") + rules = request_access.get_matching_rules("get", "f/b/c/d") self.assertEqual( rules.allowed_groups_ids[0], guid2, "expected to inherit rule of f/*/c" ) - rules = request_auth.get_matching_rules("post", "a/b/c/d") + rules = request_access.get_matching_rules("post", "a/b/c/d") self.assertEqual( rules.allowed_groups_ids[0], guid3, "expected to inherit rule of post a/b" ) @@ -144,7 +144,7 @@ def test_override_rule(self) -> None: guid1 = uuid.uuid4() guid2 = uuid.uuid4() - request_auth = RequestAuthorization.build( + request_access = RequestAccess.build( [ RuleDefinition( methods=["get"], @@ -159,12 +159,12 @@ def test_override_rule(self) -> None: ] ) - rules = request_auth.get_matching_rules("get", "a/b/c") + rules = request_access.get_matching_rules("get", "a/b/c") self.assertEqual( rules.allowed_groups_ids[0], guid1, "expected to inherit rule of a/b/c" ) - rules = request_auth.get_matching_rules("get", "a/b/c/d") + rules = request_access.get_matching_rules("get", "a/b/c/d") self.assertEqual( rules.allowed_groups_ids[0], guid2, "expected to inherit rule of a/b/c/d" ) From 998829d1c2f8b65627a1cdda62bc76051248230f Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 27 Jul 2021 11:41:02 -0700 Subject: [PATCH 047/103] fix deployment --- src/deployment/registration.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/deployment/registration.py b/src/deployment/registration.py index b5fe948361..ba4cef2c19 100644 --- a/src/deployment/registration.py +++ b/src/deployment/registration.py @@ -160,7 +160,7 @@ def register_application( tenant_id = get_tenant_id(subscription_id=subscription_id) return ApplicationInfo( - client_id=app["id"], + client_id=app["appId"], client_secret=password, authority=("https://login.microsoftonline.com/%s" % tenant_id), ) @@ -194,7 +194,7 @@ def create_application_registration( raise Exception("onefuzz app registration not found") resource_access = [ - {"id": "guid", "type": "string"} + {"id": role["id"], "type": "Scope"} for role in app["appRoles"] if role["value"] == approle.value ] @@ -205,6 +205,7 @@ def create_application_registration( "publicClient": { "redirectUris": ["https://%s.azurewebsites.net" % onefuzz_instance_name] }, + "isFallbackPublicClient": True, "requiredResourceAccess": ( [ { From 63cde940adf34888f62c6d09af2effa35ae15b41 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 27 Jul 2021 16:07:17 -0700 Subject: [PATCH 048/103] fix graph authentication on the server --- src/api-service/__app__/onefuzzlib/azure/creds.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/azure/creds.py b/src/api-service/__app__/onefuzzlib/azure/creds.py index ae1dcc9fd2..d64d418039 100644 --- a/src/api-service/__app__/onefuzzlib/azure/creds.py +++ b/src/api-service/__app__/onefuzzlib/azure/creds.py @@ -104,13 +104,15 @@ def query_microsoft_graph( params: Optional[Dict] = None, body: Optional[Dict] = None, ) -> Any: - auth = get_ms_graph_msi() - access_token = auth.token["access_token"] - token_type = auth.token["token_type"] + cred = get_identity() + access_token = cred.get_token("https://graph.microsoft.com/.default") + # auth = get_ms_graph_msi() + # access_token = auth.token["access_token"] + token_type = "Bearer" url = urllib.parse.urljoin("https://graph.microsoft.com/v1.0/", resource) headers = { - "Authorization": "%s %s" % (token_type, access_token), + "Authorization": "%s %s" % (token_type , access_token.token ), "Content-Type": "application/json", } response = requests.request( From 93ee8c9fb785ed146f3ab8eb17f666d14ba30a26 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Thu, 29 Jul 2021 11:21:55 -0700 Subject: [PATCH 049/103] use the current cli logged in account to retrive the backend token cache --- src/cli/onefuzz/backend.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/cli/onefuzz/backend.py b/src/cli/onefuzz/backend.py index 77d33bf7e8..dda5db76ea 100644 --- a/src/cli/onefuzz/backend.py +++ b/src/cli/onefuzz/backend.py @@ -26,6 +26,7 @@ ) from urllib.parse import urlparse, urlunparse from uuid import UUID +from azure.common.credentials import get_cli_profile import msal import requests @@ -185,7 +186,13 @@ def device_login(self, scopes: List[str]) -> Any: token_cache=self.token_cache, ) - accounts = self.app.get_accounts() + profile = get_cli_profile() + current_user = profile.get_current_account_user() + if current_user: + accounts = self.app.get_accounts(current_user) + else: + accounts = self.app.get_accounts() + if accounts: access_token = self.app.acquire_token_silent(scopes, account=accounts[0]) if access_token: From e9372860c0d0e179aa40bdc43318942dee06e6ec Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Thu, 29 Jul 2021 12:10:42 -0700 Subject: [PATCH 050/103] assign the the msgraph app role permissions to the web app during the deployment --- src/deployment/azuredeploy.json | 5 +++ src/deployment/deploy.py | 17 +++++++++- src/deployment/registration.py | 57 +++++++++++++++++++++++++++++++-- 3 files changed, 76 insertions(+), 3 deletions(-) diff --git a/src/deployment/azuredeploy.json b/src/deployment/azuredeploy.json index 127fd3383d..69189cc9d1 100644 --- a/src/deployment/azuredeploy.json +++ b/src/deployment/azuredeploy.json @@ -821,6 +821,11 @@ "scaleset-identity": { "type": "string", "value": "[variables('scaleset_identity')]" + }, + "webapp-identity" : { + "type": "string", + + "value": "[reference(resourceId('Microsoft.Web/sites', parameters('name')), '2018-02-01', 'Full').identity.principalId]" } } } diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 1481d61b43..6c1999985a 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -54,6 +54,7 @@ GraphQueryError, OnefuzzAppRole, add_application_password, + assign_instance_app_role, assign_app_role, authorize_application, get_application, @@ -580,13 +581,26 @@ def assign_scaleset_identity_role(self) -> None: logger.info("Upgrading: skipping assignment of the managed identity role") return logger.info("assigning the user managed identity role") - assign_app_role( + assign_instance_app_role( self.application_name, self.results["deploy"]["scaleset-identity"]["value"], self.get_subscription_id(), OnefuzzAppRole.ManagedNode, ) + def assign_app_permissions(self) -> None: + if self.upgrade: + logger.info("Upgrading: skipping assignment of msgraph permissions") + return + logger.info("assigning msgraph permissions") + + assign_app_role( + principal_id=self.results["deploy"]["webapp-identity"]["value"], + application_id=MICROSOFT_GRAPH_APP_ID, + role_names=["GroupMember.Read.All"], + subscription_id=self.get_subscription_id(), + ) + def apply_migrations(self) -> None: name = self.results["deploy"]["func-name"]["value"] key = self.results["deploy"]["func-key"]["value"] @@ -954,6 +968,7 @@ def main() -> None: ("rbac", Client.setup_rbac), ("arm", Client.deploy_template), ("assign_scaleset_identity_role", Client.assign_scaleset_identity_role), + ("assign_app_permissions", Client.assign_app_permissions), ] full_deployment_states = rbac_only_states + [ diff --git a/src/deployment/registration.py b/src/deployment/registration.py index ba4cef2c19..26a1291f4f 100644 --- a/src/deployment/registration.py +++ b/src/deployment/registration.py @@ -246,7 +246,7 @@ def create_application_registration( UUID(app["appId"]), subscription_id=subscription_id, ) - assign_app_role( + assign_instance_app_role( onefuzz_instance_name, name, subscription_id, OnefuzzAppRole.ManagedNode ) return registered_app @@ -421,6 +421,59 @@ def update_pool_registration(onefuzz_instance_name: str, subscription_id: str) - def assign_app_role( + principal_id: str, + application_id: str, + role_names: List[str], + subscription_id: str, +): + application_registration = query_microsoft_graph( + method="GET", + resource="applications", + params={ + "$filter": f"displayName eq '{application_id}'", + "$select": "appId", + }, + subscription=subscription_id, + ) + + roles = seq(application_registration["appRoles"]).find( + lambda role: role["value"] in role_names + ) + if len(roles) < len(role_names): + existing_roles = [role["value"] for role in roles] + missing_roles = [ + role_name for role_name in role_names if role_name not in existing_roles + ] + raise Exception( + f"The following roles could not be found in appId '{application_id}': {missing_roles}" + ) + + expected_role_ids = [role["id"] for role in roles] + assignments = query_microsoft_graph( + method="GET", + resource=f"servicePrincipals/{principal_id}/appRoleAssignments", + subscription=subscription_id, + ) + assigned_role_ids = [assignment["appRoleId"] for assignment in assignments["value"]] + missing_assignments = [ + id for id in expected_role_ids if id not in assigned_role_ids + ] + + if missing_assignments: + for app_role_id in missing_assignments: + query_microsoft_graph( + method="POST", + resource=f"servicePrincipals/{principal_id}/appRoleAssignedTo", + body={ + "principalId": principal_id, + "resourceId": application_registration["id"], + "appRoleId": app_role_id, + }, + subscription=subscription_id, + ) + + +def assign_instance_app_role( onefuzz_instance_name: str, application_name: str, subscription_id: str, @@ -589,7 +642,7 @@ def main() -> None: args.subscription_id, ) elif args.command == "assign_scaleset_role": - assign_app_role( + assign_instance_app_role( onefuzz_instance_name, args.scaleset_name, args.subscription_id, From 642c37b605958ab05ff760c723a11235e7903bcb Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Thu, 29 Jul 2021 12:12:39 -0700 Subject: [PATCH 051/103] formatting --- src/cli/onefuzz/backend.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cli/onefuzz/backend.py b/src/cli/onefuzz/backend.py index dda5db76ea..a1cd1ed92e 100644 --- a/src/cli/onefuzz/backend.py +++ b/src/cli/onefuzz/backend.py @@ -26,10 +26,10 @@ ) from urllib.parse import urlparse, urlunparse from uuid import UUID -from azure.common.credentials import get_cli_profile import msal import requests +from azure.common.credentials import get_cli_profile from azure.storage.blob import ContainerClient from pydantic import BaseModel, Field from tenacity import RetryCallState, retry From 398af21ad554eacf8fdc4d1c5ab350e3f278d133 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Thu, 29 Jul 2021 12:38:27 -0700 Subject: [PATCH 052/103] fix build --- src/api-service/__app__/onefuzzlib/azure/creds.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/azure/creds.py b/src/api-service/__app__/onefuzzlib/azure/creds.py index 7806ab22a4..4b892b2187 100644 --- a/src/api-service/__app__/onefuzzlib/azure/creds.py +++ b/src/api-service/__app__/onefuzzlib/azure/creds.py @@ -25,12 +25,10 @@ @cached -def get_ms_graph_msi() -> MSIAuthentication: +def get_msi() -> MSIAuthentication: allow_more_workers() reduce_logging() - return MSIAuthentication( - resource=AZURE_PUBLIC_CLOUD.endpoints.microsoft_graph_resource_id - ) + return MSIAuthentication() @cached @@ -109,8 +107,6 @@ def query_microsoft_graph( ) -> Any: cred = get_identity() access_token = cred.get_token("https://graph.microsoft.com/.default") - # auth = get_ms_graph_msi() - # access_token = auth.token["access_token"] token_type = "Bearer" url = urllib.parse.urljoin("https://graph.microsoft.com/v1.0/", resource) From b58a04d1c341c222c03cafd62d6760aad9509c8f Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Thu, 29 Jul 2021 12:51:51 -0700 Subject: [PATCH 053/103] build fix --- src/api-service/__app__/onefuzzlib/azure/creds.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/api-service/__app__/onefuzzlib/azure/creds.py b/src/api-service/__app__/onefuzzlib/azure/creds.py index 4b892b2187..791022efd0 100644 --- a/src/api-service/__app__/onefuzzlib/azure/creds.py +++ b/src/api-service/__app__/onefuzzlib/azure/creds.py @@ -17,7 +17,7 @@ from azure.mgmt.resource import ResourceManagementClient from azure.mgmt.subscription import SubscriptionClient from memoization import cached -from msrestazure.azure_active_directory import AZURE_PUBLIC_CLOUD, MSIAuthentication +from msrestazure.azure_active_directory import MSIAuthentication from msrestazure.tools import parse_resource_id from onefuzztypes.primitives import Container, Region From c4902392df6c8fd172d12d6c64e241d6d3f5af12 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Thu, 29 Jul 2021 13:08:30 -0700 Subject: [PATCH 054/103] fix bandit issue --- src/api-service/__app__/onefuzzlib/azure/creds.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/azure/creds.py b/src/api-service/__app__/onefuzzlib/azure/creds.py index 791022efd0..1d758b1b85 100644 --- a/src/api-service/__app__/onefuzzlib/azure/creds.py +++ b/src/api-service/__app__/onefuzzlib/azure/creds.py @@ -107,11 +107,10 @@ def query_microsoft_graph( ) -> Any: cred = get_identity() access_token = cred.get_token("https://graph.microsoft.com/.default") - token_type = "Bearer" url = urllib.parse.urljoin("https://graph.microsoft.com/v1.0/", resource) headers = { - "Authorization": "%s %s" % (token_type, access_token.token), + "Authorization": "Bearer %s" % access_token.token, "Content-Type": "application/json", } response = requests.request( From 79b050d2e2629f151d06007a57eb3ee2ec6576b9 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Thu, 29 Jul 2021 13:27:05 -0700 Subject: [PATCH 055/103] mypy fix --- src/deployment/registration.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/deployment/registration.py b/src/deployment/registration.py index 26a1291f4f..7d68782666 100644 --- a/src/deployment/registration.py +++ b/src/deployment/registration.py @@ -425,7 +425,7 @@ def assign_app_role( application_id: str, role_names: List[str], subscription_id: str, -): +) -> None: application_registration = query_microsoft_graph( method="GET", resource="applications", From 92ad8e9649cd1e9677167073e57a482ecca63263 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Thu, 29 Jul 2021 14:06:32 -0700 Subject: [PATCH 056/103] isort --- 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 8f59d69683..ee012e2378 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -54,8 +54,8 @@ GraphQueryError, OnefuzzAppRole, add_application_password, - assign_instance_app_role, assign_app_role, + assign_instance_app_role, authorize_application, get_application, get_tenant_id, From 4f9b98f684b1a6a66a6705d9ea77a20ad1d2ee34 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Thu, 29 Jul 2021 17:47:33 -0700 Subject: [PATCH 057/103] deploy fixes --- src/deployment/deploy.py | 2 +- src/deployment/registration.py | 22 ++++++++++++---------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index ee012e2378..9bfa307ebf 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -597,7 +597,7 @@ def assign_app_permissions(self) -> None: assign_app_role( principal_id=self.results["deploy"]["webapp-identity"]["value"], application_id=MICROSOFT_GRAPH_APP_ID, - role_names=["GroupMember.Read.All"], + role_names=["GroupMember.Read.All", "User.Read.All"], subscription_id=self.get_subscription_id(), ) diff --git a/src/deployment/registration.py b/src/deployment/registration.py index 7d68782666..43b6bc36d4 100644 --- a/src/deployment/registration.py +++ b/src/deployment/registration.py @@ -247,7 +247,7 @@ def create_application_registration( subscription_id=subscription_id, ) assign_instance_app_role( - onefuzz_instance_name, name, subscription_id, OnefuzzAppRole.ManagedNode + onefuzz_instance_name, name, subscription_id, approle ) return registered_app @@ -290,10 +290,9 @@ def add_application_password_impl( subscription=subscription_id, ) - key = uuid4() password_request = { "passwordCredential": { - "displayName": "%s" % key, + "displayName": "%s" % password_name, "startDateTime": "%s" % datetime.now(TZ_UTC).strftime("%Y-%m-%dT%H:%M.%fZ"), "endDateTime": "%s" % (datetime.now(TZ_UTC) + timedelta(days=365)).strftime( @@ -308,7 +307,7 @@ def add_application_password_impl( body=password_request, subscription=subscription_id, ) - return (str(key), password["secretText"]) + return (password_name, password["secretText"]) def get_application( @@ -428,17 +427,20 @@ def assign_app_role( ) -> None: application_registration = query_microsoft_graph( method="GET", - resource="applications", + resource="servicePrincipals", params={ - "$filter": f"displayName eq '{application_id}'", - "$select": "appId", + "$filter": f"appId eq '{application_id}'", }, subscription=subscription_id, ) + if len(application_registration["value"]) == 0: + raise Exception(f"appid '{application_id}' was not found:") + app = application_registration["value"][0] - roles = seq(application_registration["appRoles"]).find( - lambda role: role["value"] in role_names + roles = ( + seq(app["appRoles"]).filter(lambda role: role["value"] in role_names).to_list() ) + if len(roles) < len(role_names): existing_roles = [role["value"] for role in roles] missing_roles = [ @@ -466,7 +468,7 @@ def assign_app_role( resource=f"servicePrincipals/{principal_id}/appRoleAssignedTo", body={ "principalId": principal_id, - "resourceId": application_registration["id"], + "resourceId": app["id"], "appRoleId": app_role_id, }, subscription=subscription_id, From 66d3bed747bd84322e0c4d97e1c52e87ac24de43 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Fri, 30 Jul 2021 13:00:28 -0700 Subject: [PATCH 058/103] formatting --- src/deployment/registration.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/deployment/registration.py b/src/deployment/registration.py index 43b6bc36d4..a95e9f66cb 100644 --- a/src/deployment/registration.py +++ b/src/deployment/registration.py @@ -246,9 +246,7 @@ def create_application_registration( UUID(app["appId"]), subscription_id=subscription_id, ) - assign_instance_app_role( - onefuzz_instance_name, name, subscription_id, approle - ) + assign_instance_app_role(onefuzz_instance_name, name, subscription_id, approle) return registered_app From c15fb28693f412a91b6e96a8437b089f54d6ce6f Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 3 Aug 2021 11:07:20 -0700 Subject: [PATCH 059/103] removing assign_app_permissions from the deployment becahse it requires admin privileges --- src/deployment/deploy.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 9bfa307ebf..7edaa36efb 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -969,7 +969,8 @@ def main() -> None: ("rbac", Client.setup_rbac), ("arm", Client.deploy_template), ("assign_scaleset_identity_role", Client.assign_scaleset_identity_role), - ("assign_app_permissions", Client.assign_app_permissions), + # this operation seems to require admin privileges + # ("assign_app_permissions", Client.assign_app_permissions), ] full_deployment_states = rbac_only_states + [ From 161d09259eb8e2a607195ac9c17079e81b72e13e Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Wed, 4 Aug 2021 07:53:25 -0700 Subject: [PATCH 060/103] remove assign_app_permissions --- src/deployment/deploy.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 7edaa36efb..ad571d7140 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -969,8 +969,6 @@ def main() -> None: ("rbac", Client.setup_rbac), ("arm", Client.deploy_template), ("assign_scaleset_identity_role", Client.assign_scaleset_identity_role), - # this operation seems to require admin privileges - # ("assign_app_permissions", Client.assign_app_permissions), ] full_deployment_states = rbac_only_states + [ From 7ca5d9b65e75e2cb30696947601ce4f151bef02d Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Fri, 6 Aug 2021 08:22:31 -0700 Subject: [PATCH 061/103] remove assign_app_permissions --- src/deployment/deploy.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 9bfa307ebf..ad571d7140 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -969,7 +969,6 @@ def main() -> None: ("rbac", Client.setup_rbac), ("arm", Client.deploy_template), ("assign_scaleset_identity_role", Client.assign_scaleset_identity_role), - ("assign_app_permissions", Client.assign_app_permissions), ] full_deployment_states = rbac_only_states + [ From e4adb979fa6494f9f7817cea3e0e9a05ea50e38e Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 30 Aug 2021 10:03:28 -0700 Subject: [PATCH 062/103] mypy fix --- src/deployment/registration.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/deployment/registration.py b/src/deployment/registration.py index 7916dc7099..11098622e0 100644 --- a/src/deployment/registration.py +++ b/src/deployment/registration.py @@ -259,7 +259,7 @@ def create_application_registration( def add_application_password( password_name: str, app_object_id: UUID, subscription_id: str ) -> Tuple[str, str]: - def create_password() -> Tuple[str, str]: + def create_password(data: Any) -> Tuple[str, str]: password = add_application_password_impl( password_name, app_object_id, subscription_id ) From c2842a44f37454b3afdd3f55d07927efa9217129 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 25 Oct 2021 16:48:54 -0700 Subject: [PATCH 063/103] format --- src/api-service/__app__/onefuzzlib/azure/creds.py | 8 ++++++-- src/deployment/deploy.py | 1 + src/deployment/registration.py | 1 + 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/azure/creds.py b/src/api-service/__app__/onefuzzlib/azure/creds.py index 8a58efab65..5d7612f156 100644 --- a/src/api-service/__app__/onefuzzlib/azure/creds.py +++ b/src/api-service/__app__/onefuzzlib/azure/creds.py @@ -147,8 +147,8 @@ def query_microsoft_graph( f"request did not succeed: HTTP {response.status_code} - {error_text}", response.status_code, ) - - + + def query_microsoft_graph_list( method: str, resource: str, @@ -176,6 +176,7 @@ class GroupMemebership(BaseModel): def is_member_of_test(group_ids: List[UUID], member_id: UUID) -> bool: from pydantic import BaseModel from pydantic.tools import parse_raw_as + memberships = os.environ.get("TEST_MSGRAPH_AAD") if memberships is None: return True @@ -188,6 +189,8 @@ def is_member_of_test(group_ids: List[UUID], member_id: UUID) -> bool: return False return True return False + + # ######################################## @@ -198,6 +201,7 @@ def is_member_of(group_id: str, member_id: str) -> bool: ) return group_id in response + @cached def get_scaleset_identity_resource_path() -> str: scaleset_id_name = "%s-scalesetid" % get_instance_name() diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index 8d47223caf..cf4ccf2ac6 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -243,6 +243,7 @@ def create_password(self, object_id: UUID) -> Tuple[str, str]: return add_application_password( "cli_password", object_id, self.get_subscription_id() ) + def get_instance_url(self) -> str: # The url to access the instance # This also represents the legacy identifier_uris of the application diff --git a/src/deployment/registration.py b/src/deployment/registration.py index 7d23d7f64f..de6dc8a7bb 100644 --- a/src/deployment/registration.py +++ b/src/deployment/registration.py @@ -105,6 +105,7 @@ def get_tenant_id(subscription_id: Optional[str] = None) -> str: f"unable to retrive tenant_id for subscription {subscription_id}" ) + OperationResult = TypeVar("OperationResult") From a7a442cb0ad2381a46495a1464b694eac4e08e5b Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 25 Oct 2021 16:52:28 -0700 Subject: [PATCH 064/103] more cleanup --- src/cli/onefuzz/backend.py | 1 - src/deployment/deploy.py | 13 ------------- src/deployment/registration.py | 1 + 3 files changed, 1 insertion(+), 14 deletions(-) diff --git a/src/cli/onefuzz/backend.py b/src/cli/onefuzz/backend.py index 8221c93a6e..19f064fb9b 100644 --- a/src/cli/onefuzz/backend.py +++ b/src/cli/onefuzz/backend.py @@ -29,7 +29,6 @@ import msal import requests -from azure.common.credentials import get_cli_profile from azure.storage.blob import ContainerClient from pydantic import BaseModel, Field from tenacity import RetryCallState, retry diff --git a/src/deployment/deploy.py b/src/deployment/deploy.py index cf4ccf2ac6..44b2bf614c 100644 --- a/src/deployment/deploy.py +++ b/src/deployment/deploy.py @@ -594,19 +594,6 @@ def assign_scaleset_identity_role(self) -> None: OnefuzzAppRole.ManagedNode, ) - def assign_app_permissions(self) -> None: - if self.upgrade: - logger.info("Upgrading: skipping assignment of msgraph permissions") - return - logger.info("assigning msgraph permissions") - - assign_app_role( - principal_id=self.results["deploy"]["webapp-identity"]["value"], - application_id=MICROSOFT_GRAPH_APP_ID, - role_names=["GroupMember.Read.All", "User.Read.All"], - subscription_id=self.get_subscription_id(), - ) - def apply_migrations(self) -> None: logger.info("applying database migrations") name = self.results["deploy"]["func-name"]["value"] diff --git a/src/deployment/registration.py b/src/deployment/registration.py index de6dc8a7bb..b6edc06e24 100644 --- a/src/deployment/registration.py +++ b/src/deployment/registration.py @@ -494,6 +494,7 @@ def assign_app_role( if len(application_registrations) == 0: raise Exception(f"appid '{application_id}' was not found:") app = application_registrations[0] + roles = ( seq(app["appRoles"]).filter(lambda role: role["value"] in role_names).to_list() ) From 2959c77ea2637de6853ee8aef1ac6b7620a4275e Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 25 Oct 2021 17:28:44 -0700 Subject: [PATCH 065/103] build fix --- src/api-service/__app__/onefuzzlib/azure/creds.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/azure/creds.py b/src/api-service/__app__/onefuzzlib/azure/creds.py index 5d7612f156..a6048cd7ab 100644 --- a/src/api-service/__app__/onefuzzlib/azure/creds.py +++ b/src/api-service/__app__/onefuzzlib/azure/creds.py @@ -167,16 +167,16 @@ def query_microsoft_graph_list( else: raise GraphQueryError("Expected data containing a list of values", None) - class GroupMemebership(BaseModel): - principal_id: UUID - groups: List[UUID] - # FOR TESTING PURPOSE ONLY ############ def is_member_of_test(group_ids: List[UUID], member_id: UUID) -> bool: from pydantic import BaseModel from pydantic.tools import parse_raw_as + class GroupMemebership(BaseModel): + principal_id: UUID + groups: List[UUID] + memberships = os.environ.get("TEST_MSGRAPH_AAD") if memberships is None: return True From fa3086a3df0f4e7b451ead5fff874c1f10282193 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 25 Oct 2021 17:32:18 -0700 Subject: [PATCH 066/103] remove change to azuredeploy.json --- src/deployment/azuredeploy.json | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/deployment/azuredeploy.json b/src/deployment/azuredeploy.json index 5546832cfd..f29d3f28dd 100644 --- a/src/deployment/azuredeploy.json +++ b/src/deployment/azuredeploy.json @@ -896,11 +896,6 @@ "type": "string", "value": "[variables('scaleset_identity')]" }, - "webapp-identity" : { - "type": "string", - - "value": "[reference(resourceId('Microsoft.Web/sites', parameters('name')), '2018-02-01', 'Full').identity.principalId]" - }, "tenant_id": { "type": "string", "value": "[subscription().tenantId]" From c361c6e7fcec70fff84f3990fe5091e5da163ead Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 25 Oct 2021 18:20:43 -0700 Subject: [PATCH 067/103] lint --- src/api-service/__app__/onefuzzlib/request.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/api-service/__app__/onefuzzlib/request.py b/src/api-service/__app__/onefuzzlib/request.py index b3c69f29ea..e01b787d97 100644 --- a/src/api-service/__app__/onefuzzlib/request.py +++ b/src/api-service/__app__/onefuzzlib/request.py @@ -18,7 +18,6 @@ from pydantic import BaseModel # noqa: F401 from pydantic import ValidationError - from .azure.creds import is_member_of, is_member_of_test from .orm import ModelMixin from .request_access import RequestAccess From 3a4fdb5fb95f49a889cd82552b6031b71c5bb70a Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Fri, 29 Oct 2021 12:17:13 -0700 Subject: [PATCH 068/103] fix mypy --- src/api-service/__app__/onefuzzlib/azure/creds.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/azure/creds.py b/src/api-service/__app__/onefuzzlib/azure/creds.py index a6048cd7ab..5eaf6b237b 100644 --- a/src/api-service/__app__/onefuzzlib/azure/creds.py +++ b/src/api-service/__app__/onefuzzlib/azure/creds.py @@ -194,12 +194,12 @@ class GroupMemebership(BaseModel): # ######################################## -def is_member_of(group_id: str, member_id: str) -> bool: - body = {"groupIds": [group_id]} +def is_member_of(group_ids: List[UUID], member_id: UUID) -> bool: + body = {"groupIds": group_ids} response = query_microsoft_graph_list( method="POST", resource=f"users/{member_id}/checkMemberGroups", body=body ) - return group_id in response + return group_ids in response @cached From 945affee2fe71610580ebf5cd1141d59c8227d5b Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Fri, 29 Oct 2021 17:24:24 -0700 Subject: [PATCH 069/103] adding GroupMembershipChecker abstraction --- .../__app__/onefuzzlib/azure/creds.py | 66 ++++++++++++------- src/api-service/__app__/onefuzzlib/request.py | 30 ++++----- 2 files changed, 55 insertions(+), 41 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/azure/creds.py b/src/api-service/__app__/onefuzzlib/azure/creds.py index 5eaf6b237b..15104da530 100644 --- a/src/api-service/__app__/onefuzzlib/azure/creds.py +++ b/src/api-service/__app__/onefuzzlib/azure/creds.py @@ -7,7 +7,7 @@ import logging import os import urllib.parse -from typing import Any, Callable, Dict, List, Optional, TypeVar, cast +from typing import Any, Callable, Dict, List, Optional, Protocol, TypeVar, cast from uuid import UUID import requests @@ -168,38 +168,54 @@ def query_microsoft_graph_list( raise GraphQueryError("Expected data containing a list of values", None) -# FOR TESTING PURPOSE ONLY ############ -def is_member_of_test(group_ids: List[UUID], member_id: UUID) -> bool: - from pydantic import BaseModel - from pydantic.tools import parse_raw_as +class GroupMembershipChecker(Protocol): + def is_member(self, group_ids: List[UUID], member_id: UUID) -> bool: + """Check if member is part of at least one of the groups""" - class GroupMemebership(BaseModel): - principal_id: UUID - groups: List[UUID] - memberships = os.environ.get("TEST_MSGRAPH_AAD") +def create_group_membership_checker() -> GroupMembershipChecker: + memberships = os.environ.get("_STATIC_GROUP_MEMBERSHIP") if memberships is None: - return True - - data = parse_raw_as(List[GroupMemebership], memberships) - for membership in data: - if membership.principal_id == member_id: - for group_id in group_ids: - if group_id not in membership.groups: - return False + return AzureADGroupMembership() + else: + return StaticGroupMembership(memberships) + + +class AzureADGroupMembership: + def is_member(self, group_ids: List[UUID], member_id: UUID) -> bool: + if member_id in group_ids: return True - return False + body = {"groupIds": group_ids} + response = query_microsoft_graph_list( + method="POST", resource=f"users/{member_id}/checkMemberGroups", body=body + ) + return group_ids in response -# ######################################## +class StaticGroupMembership: + def __init__(self, memberships: str): + from pydantic import BaseModel + from pydantic.tools import parse_raw_as -def is_member_of(group_ids: List[UUID], member_id: UUID) -> bool: - body = {"groupIds": group_ids} - response = query_microsoft_graph_list( - method="POST", resource=f"users/{member_id}/checkMemberGroups", body=body - ) - return group_ids in response + class GroupMemebership(BaseModel): + principal_id: UUID + groups: List[UUID] + + data = parse_raw_as(List[GroupMemebership], memberships) + self.memberships = data + + def is_member(self, group_ids: List[UUID], member_id: UUID) -> bool: + if member_id in group_ids: + return True + + for membership in self.memberships: + if membership.principal_id == member_id: + for group_id in group_ids: + if group_id not in membership.groups: + return False + return True + return False @cached diff --git a/src/api-service/__app__/onefuzzlib/request.py b/src/api-service/__app__/onefuzzlib/request.py index e01b787d97..86936ab867 100644 --- a/src/api-service/__app__/onefuzzlib/request.py +++ b/src/api-service/__app__/onefuzzlib/request.py @@ -18,7 +18,7 @@ from pydantic import BaseModel # noqa: F401 from pydantic import ValidationError -from .azure.creds import is_member_of, is_member_of_test +from .azure.creds import create_group_membership_checker from .orm import ModelMixin from .request_access import RequestAccess @@ -39,7 +39,7 @@ def get_rules() -> Optional[RequestAccess]: return RequestAccess.parse_rules(rules_data) -Testing = True +membership_checker = create_group_membership_checker() def check_access(req: HttpRequest) -> Optional[Error]: @@ -54,25 +54,23 @@ def check_access(req: HttpRequest) -> Optional[Error]: member_id = UUID(req.headers["x-ms-client-principal-id"]) try: - if Testing: - result = is_member_of_test(rule.allowed_groups_ids, member_id) - else: - result = is_member_of(rule.allowed_groups_ids, member_id) + result = membership_checker.is_member(rule.allowed_groups_ids, member_id) + if not result: + logging.error( + "unauthorized access: %s is not authorized to access in %s", + member_id, + req.url, + ) + return Error( + code=ErrorCode.UNAUTHORIZED, + errors=["not approved to use this instance of onefuzz"], + ) except Exception as e: return Error( code=ErrorCode.UNAUTHORIZED, errors=["unable to interact with graph", str(e)], ) - if not result: - logging.error( - "unauthorized access: %s is not authorized to access in %s", - member_id, - req.url, - ) - return Error( - code=ErrorCode.UNAUTHORIZED, - errors=["not approved to use this instance of onefuzz"], - ) + return None From a7e39406b4db90fb213e1afd5d80797dc0f273c6 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Fri, 29 Oct 2021 17:45:02 -0700 Subject: [PATCH 070/103] moving apiaccessrules to instance config --- src/api-service/__app__/onefuzzlib/request.py | 10 ++++---- .../__app__/onefuzzlib/request_access.py | 11 +++------ src/api-service/tests/test_request_access.py | 24 +++++++++---------- src/pytypes/onefuzztypes/models.py | 6 +++++ 4 files changed, 26 insertions(+), 25 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/request.py b/src/api-service/__app__/onefuzzlib/request.py index 86936ab867..bf564f6ada 100644 --- a/src/api-service/__app__/onefuzzlib/request.py +++ b/src/api-service/__app__/onefuzzlib/request.py @@ -19,6 +19,7 @@ from pydantic import ValidationError from .azure.creds import create_group_membership_checker +from .config import InstanceConfig from .orm import ModelMixin from .request_access import RequestAccess @@ -31,13 +32,12 @@ @cached def get_rules() -> Optional[RequestAccess]: - # todo: move to instancewide configuration - rules_data = os.environ.get("ONEFUZZ_AAD_GROUP_RULES") - if rules_data is None: + config = InstanceConfig.fetch() + if config.api_access_rules: + return RequestAccess.build(config.api_access_rules) + else: return None - return RequestAccess.parse_rules(rules_data) - membership_checker = create_group_membership_checker() diff --git a/src/api-service/__app__/onefuzzlib/request_access.py b/src/api-service/__app__/onefuzzlib/request_access.py index d73ac46e07..3fc1f57d3f 100644 --- a/src/api-service/__app__/onefuzzlib/request_access.py +++ b/src/api-service/__app__/onefuzzlib/request_access.py @@ -1,15 +1,10 @@ from typing import Dict, List from uuid import UUID +from onefuzztypes.models import ApiAccessRule from pydantic import BaseModel, parse_raw_as -class RuleDefinition(BaseModel): - methods: List[str] - endpoint: str - allowed_groups: List[UUID] - - class RequestAccess: """ Stores the rules associated with a the request paths @@ -98,11 +93,11 @@ def get_matching_rules(self, method: str, path: str) -> Rules: @classmethod def parse_rules(cls, rules_data: str) -> "RequestAccess": - rules = parse_raw_as(List[RuleDefinition], rules_data) + rules = parse_raw_as(List[ApiAccessRule], rules_data) return cls.build(rules) @classmethod - def build(cls, rules: List[RuleDefinition]) -> "RequestAccess": + def build(cls, rules: List[ApiAccessRule]) -> "RequestAccess": request_access = RequestAccess() for rule in rules: request_access.__add_url__( diff --git a/src/api-service/tests/test_request_access.py b/src/api-service/tests/test_request_access.py index 73ef9211a5..2efaabf82c 100644 --- a/src/api-service/tests/test_request_access.py +++ b/src/api-service/tests/test_request_access.py @@ -1,7 +1,7 @@ import unittest import uuid -from __app__.onefuzzlib.request_access import RequestAccess, RuleDefinition +from __app__.onefuzzlib.request_access import RequestAccess, ApiAccessRule class TestRequestAccess(unittest.TestCase): @@ -11,7 +11,7 @@ def test_exact_match(self) -> None: request_access = RequestAccess.build( [ - RuleDefinition( + ApiAccessRule( methods=["get"], endpoint="a/b/c", allowed_groups=[guid1], @@ -32,7 +32,7 @@ def test_wildcard(self) -> None: request_access = RequestAccess.build( [ - RuleDefinition( + ApiAccessRule( methods=["get"], endpoint="b/*/c", allowed_groups=[guid1], @@ -51,12 +51,12 @@ def test_adding_rule_on_same_path(self) -> None: try: RequestAccess.build( [ - RuleDefinition( + ApiAccessRule( methods=["get"], endpoint="a/b/c", allowed_groups=[guid1], ), - RuleDefinition( + ApiAccessRule( methods=["get"], endpoint="a/b/c", allowed_groups=[], @@ -75,12 +75,12 @@ def test_priority(self) -> None: request_access = RequestAccess.build( [ - RuleDefinition( + ApiAccessRule( methods=["get"], endpoint="a/*/c", allowed_groups=[guid1], ), - RuleDefinition( + ApiAccessRule( methods=["get"], endpoint="a/b/c", allowed_groups=[guid2], @@ -106,17 +106,17 @@ def test_inherit_rule(self) -> None: request_access = RequestAccess.build( [ - RuleDefinition( + ApiAccessRule( methods=["get"], endpoint="a/b/c", allowed_groups=[guid1], ), - RuleDefinition( + ApiAccessRule( methods=["get"], endpoint="f/*/c", allowed_groups=[guid2], ), - RuleDefinition( + ApiAccessRule( methods=["post"], endpoint="a/b", allowed_groups=[guid3], @@ -146,12 +146,12 @@ def test_override_rule(self) -> None: request_access = RequestAccess.build( [ - RuleDefinition( + ApiAccessRule( methods=["get"], endpoint="a/b/c", allowed_groups=[guid1], ), - RuleDefinition( + ApiAccessRule( methods=["get"], endpoint="a/b/c/d", allowed_groups=[guid2], diff --git a/src/pytypes/onefuzztypes/models.py b/src/pytypes/onefuzztypes/models.py index 0e72090b85..e69adc21ba 100644 --- a/src/pytypes/onefuzztypes/models.py +++ b/src/pytypes/onefuzztypes/models.py @@ -831,6 +831,11 @@ class AzureVmExtensionConfig(BaseModel): geneva: Optional[GenevaExtensionConfig] +class ApiAccessRule(BaseModel): + methods: List[str] + endpoint: str + allowed_groups: List[UUID] + class InstanceConfig(BaseModel): # initial set of admins can only be set during deployment. # if admins are set, only admins can update instance configs. @@ -843,6 +848,7 @@ class InstanceConfig(BaseModel): network_config: NetworkConfig = Field(default_factory=NetworkConfig) extensions: Optional[AzureVmExtensionConfig] proxy_vm_sku: str = Field(default="Standard_B2s") + api_access_rules: Optional[List[ApiAccessRule]] = None def update(self, config: "InstanceConfig") -> None: for field in config.__fields__: From 0337a0e9f95a7f8dd30c24ed70eb6a9cbf91fa16 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Fri, 29 Oct 2021 17:48:43 -0700 Subject: [PATCH 071/103] moving to github 3.8 to use protocols --- .github/workflows/ci.yml | 22 +++++++++---------- .../github-pipeline.yml | 2 +- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d92b9b0d81..44a581eddc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -23,7 +23,7 @@ jobs: key: rust-${{ runner.os }}-${{ env.ACTIONS_CACHE_KEY_DATE }} - name: Install Rust Prereqs if: steps.cache-rust-prereqs.outputs.cache-hit != 'true' - shell: bash + shell: bash run: src/ci/rust-prereqs.sh - name: Rust Compile Cache uses: actions/cache@v2 @@ -64,7 +64,7 @@ jobs: - uses: actions/checkout@v2 - uses: actions/setup-python@v2 with: - python-version: 3.7 + python-version: 3.8 - name: lint shell: bash run: src/ci/check-check-pr.sh @@ -78,7 +78,7 @@ jobs: shell: bash - uses: actions/setup-python@v2 with: - python-version: 3.7 + python-version: 3.8 - uses: actions/download-artifact@v2.0.8 with: name: build-artifacts @@ -162,7 +162,7 @@ jobs: - run: src/ci/set-versions.sh - uses: actions/setup-python@v2 with: - python-version: 3.7 + python-version: 3.8 - run: src/ci/onefuzztypes.sh - uses: actions/upload-artifact@v2.2.2 with: @@ -183,7 +183,7 @@ jobs: key: rust-${{ runner.os }}-${{ env.ACTIONS_CACHE_KEY_DATE }} - name: Install Rust Prereqs if: steps.cache-rust-prereqs.outputs.cache-hit != 'true' - shell: bash + shell: bash run: src/ci/rust-prereqs.sh - name: Rust Compile Cache uses: actions/cache@v2 @@ -206,7 +206,7 @@ jobs: - run: src/ci/set-versions.sh - uses: actions/setup-python@v2 with: - python-version: 3.7 + python-version: 3.8 - uses: actions/download-artifact@v2.0.8 with: name: build-artifacts @@ -241,7 +241,7 @@ jobs: mypy __app__ ./tests # set a minimum confidence to ignore known false positives - vulture --min-confidence 61 __app__ + vulture --min-confidence 61 __app__ ../ci/disable-py-cache.sh mypy __app__ ./tests @@ -302,7 +302,7 @@ jobs: path: artifacts - uses: actions/setup-python@v2 with: - python-version: 3.7 + python-version: 3.8 - name: Lint shell: bash run: | @@ -415,7 +415,7 @@ jobs: steps: - uses: actions/checkout@v2 - run: | - Set-ExecutionPolicy Bypass -Scope Process -Force + Set-ExecutionPolicy Bypass -Scope Process -Force $ProgressPreference = 'SilentlyContinue' Invoke-Expression (Invoke-RestMethod 'https://chocolatey.org/install.ps1') choco install llvm @@ -450,7 +450,7 @@ jobs: $env:LIB = $env:LIBRARY_PATH cd src/integration-tests - + mkdir artifacts/windows-libfuzzer cd libfuzzer make @@ -468,7 +468,7 @@ jobs: make -f Makefile.windows cp fuzz.exe,fuzz.pdb,bad1.dll,bad1.pdb,bad2.dll,bad2.pdb,seeds ../artifacts/windows-libfuzzer-linked-library -Recurse cd ../ - + mkdir artifacts/windows-libfuzzer-load-library cd libfuzzer-load-library make diff --git a/contrib/onefuzz-job-github-actions/github-pipeline.yml b/contrib/onefuzz-job-github-actions/github-pipeline.yml index 44e08e4ebb..dc602de517 100644 --- a/contrib/onefuzz-job-github-actions/github-pipeline.yml +++ b/contrib/onefuzz-job-github-actions/github-pipeline.yml @@ -25,7 +25,7 @@ jobs: - name: Setup Python uses: actions/setup-python@v2 with: - python-version: 3.7 + python-version: 3.8 - name: submit onefuzz job env: ONEFUZZ_ENDPOINT: ${{ secrets.onefuzz_endpoint }} From 5585d8cf0f4bd424a7c3f6283e407bf211a8ae67 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 1 Nov 2021 10:59:29 -0700 Subject: [PATCH 072/103] moving group_membership into its own file --- .../__app__/onefuzzlib/azure/creds.py | 52 +--------------- .../onefuzzlib/azure/group_membership.py | 61 +++++++++++++++++++ src/api-service/__app__/onefuzzlib/request.py | 2 +- 3 files changed, 63 insertions(+), 52 deletions(-) create mode 100644 src/api-service/__app__/onefuzzlib/azure/group_membership.py diff --git a/src/api-service/__app__/onefuzzlib/azure/creds.py b/src/api-service/__app__/onefuzzlib/azure/creds.py index 15104da530..38eff823a0 100644 --- a/src/api-service/__app__/onefuzzlib/azure/creds.py +++ b/src/api-service/__app__/onefuzzlib/azure/creds.py @@ -7,7 +7,7 @@ import logging import os import urllib.parse -from typing import Any, Callable, Dict, List, Optional, Protocol, TypeVar, cast +from typing import Any, Callable, Dict, List, Optional, TypeVar, cast from uuid import UUID import requests @@ -168,56 +168,6 @@ def query_microsoft_graph_list( raise GraphQueryError("Expected data containing a list of values", None) -class GroupMembershipChecker(Protocol): - def is_member(self, group_ids: List[UUID], member_id: UUID) -> bool: - """Check if member is part of at least one of the groups""" - - -def create_group_membership_checker() -> GroupMembershipChecker: - memberships = os.environ.get("_STATIC_GROUP_MEMBERSHIP") - if memberships is None: - return AzureADGroupMembership() - else: - return StaticGroupMembership(memberships) - - -class AzureADGroupMembership: - def is_member(self, group_ids: List[UUID], member_id: UUID) -> bool: - if member_id in group_ids: - return True - - body = {"groupIds": group_ids} - response = query_microsoft_graph_list( - method="POST", resource=f"users/{member_id}/checkMemberGroups", body=body - ) - return group_ids in response - - -class StaticGroupMembership: - def __init__(self, memberships: str): - from pydantic import BaseModel - from pydantic.tools import parse_raw_as - - class GroupMemebership(BaseModel): - principal_id: UUID - groups: List[UUID] - - data = parse_raw_as(List[GroupMemebership], memberships) - self.memberships = data - - def is_member(self, group_ids: List[UUID], member_id: UUID) -> bool: - if member_id in group_ids: - return True - - for membership in self.memberships: - if membership.principal_id == member_id: - for group_id in group_ids: - if group_id not in membership.groups: - return False - return True - return False - - @cached def get_scaleset_identity_resource_path() -> str: scaleset_id_name = "%s-scalesetid" % get_instance_name() diff --git a/src/api-service/__app__/onefuzzlib/azure/group_membership.py b/src/api-service/__app__/onefuzzlib/azure/group_membership.py new file mode 100644 index 0000000000..29888a1ccf --- /dev/null +++ b/src/api-service/__app__/onefuzzlib/azure/group_membership.py @@ -0,0 +1,61 @@ +#!/usr/bin/env python +# +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +import os +from typing import List, Protocol +from uuid import UUID + +from .creds import query_microsoft_graph_list + + +class GroupMembershipChecker(Protocol): + def is_member(self, group_ids: List[UUID], member_id: UUID) -> bool: + """Check if member is part of at least one of the groups""" + + +def create_group_membership_checker() -> GroupMembershipChecker: + + memberships = os.environ.get("_STATIC_GROUP_MEMBERSHIP") + if memberships is None: + return AzureADGroupMembership() + else: + return StaticGroupMembership(memberships) + + +class AzureADGroupMembership: + def is_member(self, group_ids: List[UUID], member_id: UUID) -> bool: + if member_id in group_ids: + return True + + body = {"groupIds": group_ids} + response = query_microsoft_graph_list( + method="POST", resource=f"users/{member_id}/checkMemberGroups", body=body + ) + return group_ids in response + + +class StaticGroupMembership: + def __init__(self, memberships: str): + from pydantic import BaseModel + from pydantic.tools import parse_raw_as + + class GroupMemebership(BaseModel): + principal_id: UUID + groups: List[UUID] + + data = parse_raw_as(List[GroupMemebership], memberships) + self.memberships = data + + def is_member(self, group_ids: List[UUID], member_id: UUID) -> bool: + if member_id in group_ids: + return True + + for membership in self.memberships: + if membership.principal_id == member_id: + for group_id in group_ids: + if group_id not in membership.groups: + return False + return True + return False diff --git a/src/api-service/__app__/onefuzzlib/request.py b/src/api-service/__app__/onefuzzlib/request.py index bf564f6ada..614a8b24b0 100644 --- a/src/api-service/__app__/onefuzzlib/request.py +++ b/src/api-service/__app__/onefuzzlib/request.py @@ -18,7 +18,7 @@ from pydantic import BaseModel # noqa: F401 from pydantic import ValidationError -from .azure.creds import create_group_membership_checker +from .azure.group_membership import create_group_membership_checker from .config import InstanceConfig from .orm import ModelMixin from .request_access import RequestAccess From dd455b807864e9f141c44879b935b1a175dfdb5a Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 1 Nov 2021 11:14:09 -0700 Subject: [PATCH 073/103] refactoring --- .../__app__/onefuzzlib/request_access.py | 8 ++++- src/api-service/tests/test_request_access.py | 31 ++++++++++--------- 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/request_access.py b/src/api-service/__app__/onefuzzlib/request_access.py index 3fc1f57d3f..d19cbf4963 100644 --- a/src/api-service/__app__/onefuzzlib/request_access.py +++ b/src/api-service/__app__/onefuzzlib/request_access.py @@ -5,6 +5,12 @@ from pydantic import BaseModel, parse_raw_as +class RuleConflictError(Exception): + def __init__(self, message: str) -> None: + super(RuleConflictError, self).__init__(message) + self.message = message + + class RequestAccess: """ Stores the rules associated with a the request paths @@ -54,7 +60,7 @@ def __add_url__(self, methods: List[str], path: str, rules: Rules) -> None: if current_segment_index == len(segments): for method in methods: if method in current_node.rules: - raise Exception(f"Conflicting rules on {method} {path}") + raise RuleConflictError(f"Conflicting rules on {method} {path}") while current_segment_index < len(segments): current_segment = segments[current_segment_index] diff --git a/src/api-service/tests/test_request_access.py b/src/api-service/tests/test_request_access.py index 2efaabf82c..5096dfaa55 100644 --- a/src/api-service/tests/test_request_access.py +++ b/src/api-service/tests/test_request_access.py @@ -1,7 +1,11 @@ import unittest import uuid -from __app__.onefuzzlib.request_access import RequestAccess, ApiAccessRule +from __app__.onefuzzlib.request_access import ( + ApiAccessRule, + RequestAccess, + RuleConflictError, +) class TestRequestAccess(unittest.TestCase): @@ -65,7 +69,7 @@ def test_adding_rule_on_same_path(self) -> None: ) self.fail("this is expected to fail") - except Exception: + except RuleConflictError: pass # The most specific rules takes priority over the ones containing a wildcard @@ -97,8 +101,7 @@ def test_priority(self) -> None: ) # if a path has no specific rule. it inherits from the parents - # /a/b/c inherint from a/b - # todo test the wildcard behavior + # /a/b/c inherit from a/b def test_inherit_rule(self) -> None: guid1 = uuid.uuid4() guid2 = uuid.uuid4() @@ -124,19 +127,19 @@ def test_inherit_rule(self) -> None: ] ) - rules = request_access.get_matching_rules("get", "a/b/c/d") + rules1 = request_access.get_matching_rules("get", "a/b/c/d") self.assertEqual( - rules.allowed_groups_ids[0], guid1, "expected to inherit rule of a/b/c" + rules1.allowed_groups_ids[0], guid1, "expected to inherit rule of a/b/c" ) - rules = request_access.get_matching_rules("get", "f/b/c/d") + rules2 = request_access.get_matching_rules("get", "f/b/c/d") self.assertEqual( - rules.allowed_groups_ids[0], guid2, "expected to inherit rule of f/*/c" + rules2.allowed_groups_ids[0], guid2, "expected to inherit rule of f/*/c" ) - rules = request_access.get_matching_rules("post", "a/b/c/d") + rules3 = request_access.get_matching_rules("post", "a/b/c/d") self.assertEqual( - rules.allowed_groups_ids[0], guid3, "expected to inherit rule of post a/b" + rules3.allowed_groups_ids[0], guid3, "expected to inherit rule of post a/b" ) # the lowest level rule override the parent rules @@ -159,12 +162,12 @@ def test_override_rule(self) -> None: ] ) - rules = request_access.get_matching_rules("get", "a/b/c") + rules1 = request_access.get_matching_rules("get", "a/b/c") self.assertEqual( - rules.allowed_groups_ids[0], guid1, "expected to inherit rule of a/b/c" + rules1.allowed_groups_ids[0], guid1, "expected to inherit rule of a/b/c" ) - rules = request_access.get_matching_rules("get", "a/b/c/d") + rules2 = request_access.get_matching_rules("get", "a/b/c/d") self.assertEqual( - rules.allowed_groups_ids[0], guid2, "expected to inherit rule of a/b/c/d" + rules2.allowed_groups_ids[0], guid2, "expected to inherit rule of a/b/c/d" ) From 45d99f7174c275106eaa1dfbd09f8bc3fff3f829 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 1 Nov 2021 19:17:46 -0700 Subject: [PATCH 074/103] generate docs --- docs/webhook_events.md | 74 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/docs/webhook_events.md b/docs/webhook_events.md index c4e065806e..760f7bf519 100644 --- a/docs/webhook_events.md +++ b/docs/webhook_events.md @@ -659,6 +659,36 @@ Each event will be submitted via HTTP POST to the user provided URL. ```json { "definitions": { + "ApiAccessRule": { + "properties": { + "allowed_groups": { + "items": { + "format": "uuid", + "type": "string" + }, + "title": "Allowed Groups", + "type": "array" + }, + "endpoint": { + "title": "Endpoint", + "type": "string" + }, + "methods": { + "items": { + "type": "string" + }, + "title": "Methods", + "type": "array" + } + }, + "required": [ + "methods", + "endpoint", + "allowed_groups" + ], + "title": "ApiAccessRule", + "type": "object" + }, "AzureMonitorExtensionConfig": { "properties": { "config_version": { @@ -753,6 +783,13 @@ Each event will be submitted via HTTP POST to the user provided URL. "title": "Allowed Aad Tenants", "type": "array" }, + "api_access_rules": { + "items": { + "$ref": "#/definitions/ApiAccessRule" + }, + "title": "Api Access Rules", + "type": "array" + }, "extensions": { "$ref": "#/definitions/AzureVmExtensionConfig" }, @@ -4906,6 +4943,36 @@ Each event will be submitted via HTTP POST to the user provided URL. ```json { "definitions": { + "ApiAccessRule": { + "properties": { + "allowed_groups": { + "items": { + "format": "uuid", + "type": "string" + }, + "title": "Allowed Groups", + "type": "array" + }, + "endpoint": { + "title": "Endpoint", + "type": "string" + }, + "methods": { + "items": { + "type": "string" + }, + "title": "Methods", + "type": "array" + } + }, + "required": [ + "methods", + "endpoint", + "allowed_groups" + ], + "title": "ApiAccessRule", + "type": "object" + }, "Architecture": { "description": "An enumeration.", "enum": [ @@ -5829,6 +5896,13 @@ Each event will be submitted via HTTP POST to the user provided URL. "title": "Allowed Aad Tenants", "type": "array" }, + "api_access_rules": { + "items": { + "$ref": "#/definitions/ApiAccessRule" + }, + "title": "Api Access Rules", + "type": "array" + }, "extensions": { "$ref": "#/definitions/AzureVmExtensionConfig" }, From 14cbb67024e2e402288853f27bc38bb9f893c9ea Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 2 Nov 2021 13:11:39 -0700 Subject: [PATCH 075/103] Revert "moving to github 3.8 to use protocols" This reverts commit 0337a0e9f95a7f8dd30c24ed70eb6a9cbf91fa16. --- .github/workflows/ci.yml | 22 +++++++++---------- .../github-pipeline.yml | 2 +- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 44a581eddc..d92b9b0d81 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -23,7 +23,7 @@ jobs: key: rust-${{ runner.os }}-${{ env.ACTIONS_CACHE_KEY_DATE }} - name: Install Rust Prereqs if: steps.cache-rust-prereqs.outputs.cache-hit != 'true' - shell: bash + shell: bash run: src/ci/rust-prereqs.sh - name: Rust Compile Cache uses: actions/cache@v2 @@ -64,7 +64,7 @@ jobs: - uses: actions/checkout@v2 - uses: actions/setup-python@v2 with: - python-version: 3.8 + python-version: 3.7 - name: lint shell: bash run: src/ci/check-check-pr.sh @@ -78,7 +78,7 @@ jobs: shell: bash - uses: actions/setup-python@v2 with: - python-version: 3.8 + python-version: 3.7 - uses: actions/download-artifact@v2.0.8 with: name: build-artifacts @@ -162,7 +162,7 @@ jobs: - run: src/ci/set-versions.sh - uses: actions/setup-python@v2 with: - python-version: 3.8 + python-version: 3.7 - run: src/ci/onefuzztypes.sh - uses: actions/upload-artifact@v2.2.2 with: @@ -183,7 +183,7 @@ jobs: key: rust-${{ runner.os }}-${{ env.ACTIONS_CACHE_KEY_DATE }} - name: Install Rust Prereqs if: steps.cache-rust-prereqs.outputs.cache-hit != 'true' - shell: bash + shell: bash run: src/ci/rust-prereqs.sh - name: Rust Compile Cache uses: actions/cache@v2 @@ -206,7 +206,7 @@ jobs: - run: src/ci/set-versions.sh - uses: actions/setup-python@v2 with: - python-version: 3.8 + python-version: 3.7 - uses: actions/download-artifact@v2.0.8 with: name: build-artifacts @@ -241,7 +241,7 @@ jobs: mypy __app__ ./tests # set a minimum confidence to ignore known false positives - vulture --min-confidence 61 __app__ + vulture --min-confidence 61 __app__ ../ci/disable-py-cache.sh mypy __app__ ./tests @@ -302,7 +302,7 @@ jobs: path: artifacts - uses: actions/setup-python@v2 with: - python-version: 3.8 + python-version: 3.7 - name: Lint shell: bash run: | @@ -415,7 +415,7 @@ jobs: steps: - uses: actions/checkout@v2 - run: | - Set-ExecutionPolicy Bypass -Scope Process -Force + Set-ExecutionPolicy Bypass -Scope Process -Force $ProgressPreference = 'SilentlyContinue' Invoke-Expression (Invoke-RestMethod 'https://chocolatey.org/install.ps1') choco install llvm @@ -450,7 +450,7 @@ jobs: $env:LIB = $env:LIBRARY_PATH cd src/integration-tests - + mkdir artifacts/windows-libfuzzer cd libfuzzer make @@ -468,7 +468,7 @@ jobs: make -f Makefile.windows cp fuzz.exe,fuzz.pdb,bad1.dll,bad1.pdb,bad2.dll,bad2.pdb,seeds ../artifacts/windows-libfuzzer-linked-library -Recurse cd ../ - + mkdir artifacts/windows-libfuzzer-load-library cd libfuzzer-load-library make diff --git a/contrib/onefuzz-job-github-actions/github-pipeline.yml b/contrib/onefuzz-job-github-actions/github-pipeline.yml index dc602de517..44e08e4ebb 100644 --- a/contrib/onefuzz-job-github-actions/github-pipeline.yml +++ b/contrib/onefuzz-job-github-actions/github-pipeline.yml @@ -25,7 +25,7 @@ jobs: - name: Setup Python uses: actions/setup-python@v2 with: - python-version: 3.8 + python-version: 3.7 - name: submit onefuzz job env: ONEFUZZ_ENDPOINT: ${{ secrets.onefuzz_endpoint }} From e42d06adcfbc2b7a841bcc8b189002a1c27450d5 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 2 Nov 2021 13:13:33 -0700 Subject: [PATCH 076/103] move service to 3.8 --- .github/workflows/ci.yml | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d92b9b0d81..4b6b4c401e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -23,7 +23,7 @@ jobs: key: rust-${{ runner.os }}-${{ env.ACTIONS_CACHE_KEY_DATE }} - name: Install Rust Prereqs if: steps.cache-rust-prereqs.outputs.cache-hit != 'true' - shell: bash + shell: bash run: src/ci/rust-prereqs.sh - name: Rust Compile Cache uses: actions/cache@v2 @@ -183,7 +183,7 @@ jobs: key: rust-${{ runner.os }}-${{ env.ACTIONS_CACHE_KEY_DATE }} - name: Install Rust Prereqs if: steps.cache-rust-prereqs.outputs.cache-hit != 'true' - shell: bash + shell: bash run: src/ci/rust-prereqs.sh - name: Rust Compile Cache uses: actions/cache@v2 @@ -206,7 +206,7 @@ jobs: - run: src/ci/set-versions.sh - uses: actions/setup-python@v2 with: - python-version: 3.7 + python-version: 3.8 - uses: actions/download-artifact@v2.0.8 with: name: build-artifacts @@ -241,7 +241,7 @@ jobs: mypy __app__ ./tests # set a minimum confidence to ignore known false positives - vulture --min-confidence 61 __app__ + vulture --min-confidence 61 __app__ ../ci/disable-py-cache.sh mypy __app__ ./tests @@ -415,7 +415,7 @@ jobs: steps: - uses: actions/checkout@v2 - run: | - Set-ExecutionPolicy Bypass -Scope Process -Force + Set-ExecutionPolicy Bypass -Scope Process -Force $ProgressPreference = 'SilentlyContinue' Invoke-Expression (Invoke-RestMethod 'https://chocolatey.org/install.ps1') choco install llvm @@ -450,7 +450,7 @@ jobs: $env:LIB = $env:LIBRARY_PATH cd src/integration-tests - + mkdir artifacts/windows-libfuzzer cd libfuzzer make @@ -468,7 +468,7 @@ jobs: make -f Makefile.windows cp fuzz.exe,fuzz.pdb,bad1.dll,bad1.pdb,bad2.dll,bad2.pdb,seeds ../artifacts/windows-libfuzzer-linked-library -Recurse cd ../ - + mkdir artifacts/windows-libfuzzer-load-library cd libfuzzer-load-library make From eeb2d13f508f21ed28556e3abb25f10834a18e88 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 8 Nov 2021 11:06:09 -0800 Subject: [PATCH 077/103] adding functional tests --- .../onefuzzlib/azure/group_membership.py | 60 +++++----- .../functional_tests/api_resttriction_test.py | 108 ++++++++++++++++++ .../functional_tests/requirements.txt | 2 + src/pytypes/onefuzztypes/models.py | 8 ++ 4 files changed, 145 insertions(+), 33 deletions(-) create mode 100644 src/api-service/functional_tests/api_resttriction_test.py create mode 100644 src/api-service/functional_tests/requirements.txt diff --git a/src/api-service/__app__/onefuzzlib/azure/group_membership.py b/src/api-service/__app__/onefuzzlib/azure/group_membership.py index 29888a1ccf..3b941a91ab 100644 --- a/src/api-service/__app__/onefuzzlib/azure/group_membership.py +++ b/src/api-service/__app__/onefuzzlib/azure/group_membership.py @@ -7,55 +7,49 @@ from typing import List, Protocol from uuid import UUID +from pytypes.onefuzztypes.models import GroupMemebership, InstanceConfig + from .creds import query_microsoft_graph_list class GroupMembershipChecker(Protocol): def is_member(self, group_ids: List[UUID], member_id: UUID) -> bool: """Check if member is part of at least one of the groups""" + if member_id in group_ids: + return True -def create_group_membership_checker() -> GroupMembershipChecker: + groups = self.get_groups(member_id) + return group_ids in groups - memberships = os.environ.get("_STATIC_GROUP_MEMBERSHIP") - if memberships is None: - return AzureADGroupMembership() - else: - return StaticGroupMembership(memberships) + def get_groups(self, member_id: UUID) -> List[UUID]: + """Gets all the groups of the provided member""" -class AzureADGroupMembership: - def is_member(self, group_ids: List[UUID], member_id: UUID) -> bool: - if member_id in group_ids: - return True +def create_group_membership_checker() -> GroupMembershipChecker: + config = InstanceConfig.fetch() + if config.group_membership: + return StaticGroupMembership(config.group_membership) + else: + return AzureADGroupMembership() + +class AzureADGroupMembership(GroupMembershipChecker): - body = {"groupIds": group_ids} + def get_groups(self, member_id: UUID) -> List[UUID]: response = query_microsoft_graph_list( - method="POST", resource=f"users/{member_id}/checkMemberGroups", body=body + method="GET", resource=f"users/{member_id}/memberOf" ) - return group_ids in response - - -class StaticGroupMembership: - def __init__(self, memberships: str): - from pydantic import BaseModel - from pydantic.tools import parse_raw_as + return response - class GroupMemebership(BaseModel): - principal_id: UUID - groups: List[UUID] - data = parse_raw_as(List[GroupMemebership], memberships) - self.memberships = data - - def is_member(self, group_ids: List[UUID], member_id: UUID) -> bool: - if member_id in group_ids: - return True +class StaticGroupMembership(GroupMembershipChecker): + def __init__(self, memberships: List[GroupMemebership]): + self.memberships = memberships + def get_groups(self, member_id: UUID) -> List[UUID]: + groups = set() for membership in self.memberships: if membership.principal_id == member_id: - for group_id in group_ids: - if group_id not in membership.groups: - return False - return True - return False + for g in membership.groups: + groups.add(g) + return list(groups) diff --git a/src/api-service/functional_tests/api_resttriction_test.py b/src/api-service/functional_tests/api_resttriction_test.py new file mode 100644 index 0000000000..21aa909db4 --- /dev/null +++ b/src/api-service/functional_tests/api_resttriction_test.py @@ -0,0 +1,108 @@ +#!/usr/bin/env python +# +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +import argparse +import subprocess # nosec +import sys +import time +import uuid +from azure.cli.core import get_default_cli + +from onefuzz.api import Onefuzz +from uuid import UUID +import os +from onefuzztypes.models import ApiAccessRule + +def az_cli(args): + cli = get_default_cli() + cli.logging_cls + cli.invoke(args, out_file=open(os.devnull, "w")) + if cli.result.result: + return cli.result.result + elif cli.result.error: + raise cli.result.error + + +class APIRestrictionTests: + def __init__(self, onefuzz_config_path=None): + self.onefuzz = Onefuzz(config_path=onefuzz_config_path) + self.intial_config = self.onefuzz.instance_config.get() + + def restore_config(self) -> None: + self.onefuzz.instance_config.update(self.intial_config) + + def assign(self, group_id: UUID, member_id: UUID) -> None: + instance_config = self.onefuzz.instance_config.get() + if member_id in instance_config.group_membership: + if group_id not in instance_config.group_membership[member_id]: + instance_config.group_membership[member_id].append(group_id) + + self.onefuzz.instance_config.update(instance_config) + + + def assign_current_user(self, group_id: UUID) -> None: + onefuzz_service_appId = az_cli( + [ + "ad", + "signed-in-user", + "show", + ]) + member_id = UUID(onefuzz_service_appId["objectid"]) + self.assign(group_id, member_id) + + + def test_restriction_on_current_user(self) -> None: + + print("Checking that the current user can get info") + info = self.onefuzz.info.get() + + + print("Creating test group") + group_id = uuid.uuid4() + + print("Adding restriction to the info endpoint") + instance_config = self.onefuzz.instance_config + if instance_config.api_access_rules is None: + instance_config.api_access_rules = [] + + instance_config.api_access_rules.append( + ApiAccessRule( + path="/info", + group_ids=[group_id], + method="GET", + ) + ) + self.onefuzz.instance_config.update(instance_config) + + print("Checking that the current user cannot get info") + try: + info = self.onefuzz.info.get() + raise Exception("Expected exception not thrown") + except Exception as e: + pass + + print("Assigning current user to test group") + self.assign_current_user(group_id) + + print("Checking that the current user can get info") + info = self.onefuzz.info.get() + + +def main() -> None: + parser = argparse.ArgumentParser() + parser.add_argument("--config_path", default=None) + args = parser.parse_args() + tester = APIRestrictionTests(args.config_path) + + try: + print("test current user restriction") + tester.test_restriction_on_current_user() + finally: + tester.restore_config() + + + +if __name__ == "__main__": + main() diff --git a/src/api-service/functional_tests/requirements.txt b/src/api-service/functional_tests/requirements.txt new file mode 100644 index 0000000000..19d50efb77 --- /dev/null +++ b/src/api-service/functional_tests/requirements.txt @@ -0,0 +1,2 @@ +onefuzz==0.0.0 +azure-cli-core==2.27.2 \ No newline at end of file diff --git a/src/pytypes/onefuzztypes/models.py b/src/pytypes/onefuzztypes/models.py index d8430062d4..2d82e7dcb7 100644 --- a/src/pytypes/onefuzztypes/models.py +++ b/src/pytypes/onefuzztypes/models.py @@ -841,6 +841,13 @@ class ApiAccessRule(BaseModel): endpoint: str allowed_groups: List[UUID] +PrincipalID = UUID +GroupId = UUID + +class GroupMemebership(BaseModel): + principal_id: UUID + groups: List[UUID] + class InstanceConfig(BaseModel): # initial set of admins can only be set during deployment. @@ -858,6 +865,7 @@ class InstanceConfig(BaseModel): extensions: Optional[AzureVmExtensionConfig] proxy_vm_sku: str = Field(default="Standard_B2s") api_access_rules: Optional[List[ApiAccessRule]] = None + group_membership: Optional[Dict[PrincipalID, List[GroupId]]] = None def update(self, config: "InstanceConfig") -> None: for field in config.__fields__: From 45aea087b4c6935717248b3c665131a4b16c2a50 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 8 Nov 2021 11:10:43 -0800 Subject: [PATCH 078/103] format --- src/pytypes/onefuzztypes/models.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/pytypes/onefuzztypes/models.py b/src/pytypes/onefuzztypes/models.py index 2d82e7dcb7..389bc80fe1 100644 --- a/src/pytypes/onefuzztypes/models.py +++ b/src/pytypes/onefuzztypes/models.py @@ -841,9 +841,11 @@ class ApiAccessRule(BaseModel): endpoint: str allowed_groups: List[UUID] + PrincipalID = UUID GroupId = UUID + class GroupMemebership(BaseModel): principal_id: UUID groups: List[UUID] From 438435c0580333f7e985913545a0e2cd40ab068b Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 8 Nov 2021 11:19:32 -0800 Subject: [PATCH 079/103] generate docs --- docs/webhook_events.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/docs/webhook_events.md b/docs/webhook_events.md index d0729db398..e0817d4aab 100644 --- a/docs/webhook_events.md +++ b/docs/webhook_events.md @@ -797,6 +797,17 @@ Each event will be submitted via HTTP POST to the user provided URL. "extensions": { "$ref": "#/definitions/AzureVmExtensionConfig" }, + "group_membership": { + "additionalProperties": { + "items": { + "format": "uuid", + "type": "string" + }, + "type": "array" + }, + "title": "Group Membership", + "type": "object" + }, "network_config": { "$ref": "#/definitions/NetworkConfig" }, @@ -5933,6 +5944,17 @@ Each event will be submitted via HTTP POST to the user provided URL. "extensions": { "$ref": "#/definitions/AzureVmExtensionConfig" }, + "group_membership": { + "additionalProperties": { + "items": { + "format": "uuid", + "type": "string" + }, + "type": "array" + }, + "title": "Group Membership", + "type": "object" + }, "network_config": { "$ref": "#/definitions/NetworkConfig" }, From dac2570288265eab90bb6702ab5fed53e7d615e8 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 8 Nov 2021 12:33:10 -0800 Subject: [PATCH 080/103] run tests only in the test folder --- .github/workflows/ci.yml | 2 +- src/api-service/functional_tests/api_resttriction_test.py | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4b6b4c401e..e68f1d3be1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -233,7 +233,7 @@ jobs: pip install ${GITHUB_WORKSPACE}/artifacts/sdk/onefuzztypes-*.whl pip install -r __app__/requirements.txt pip install -r requirements-dev.txt - pytest + pytest tests flake8 . bandit -r ./__app__/ black ./__app__/ ./tests --check diff --git a/src/api-service/functional_tests/api_resttriction_test.py b/src/api-service/functional_tests/api_resttriction_test.py index 21aa909db4..3c393039f3 100644 --- a/src/api-service/functional_tests/api_resttriction_test.py +++ b/src/api-service/functional_tests/api_resttriction_test.py @@ -4,9 +4,6 @@ # Licensed under the MIT License. import argparse -import subprocess # nosec -import sys -import time import uuid from azure.cli.core import get_default_cli From d948322a7686db91e34b76cbf7ab868a8bbf16f5 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 8 Nov 2021 15:41:01 -0800 Subject: [PATCH 081/103] build fix --- src/api-service/__app__/onefuzzlib/azure/group_membership.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/azure/group_membership.py b/src/api-service/__app__/onefuzzlib/azure/group_membership.py index 3b941a91ab..bb0bd75743 100644 --- a/src/api-service/__app__/onefuzzlib/azure/group_membership.py +++ b/src/api-service/__app__/onefuzzlib/azure/group_membership.py @@ -7,7 +7,7 @@ from typing import List, Protocol from uuid import UUID -from pytypes.onefuzztypes.models import GroupMemebership, InstanceConfig +from onefuzztypes.models import GroupMemebership, InstanceConfig from .creds import query_microsoft_graph_list @@ -18,7 +18,6 @@ def is_member(self, group_ids: List[UUID], member_id: UUID) -> bool: if member_id in group_ids: return True - groups = self.get_groups(member_id) return group_ids in groups @@ -33,8 +32,8 @@ def create_group_membership_checker() -> GroupMembershipChecker: else: return AzureADGroupMembership() -class AzureADGroupMembership(GroupMembershipChecker): +class AzureADGroupMembership(GroupMembershipChecker): def get_groups(self, member_id: UUID) -> List[UUID]: response = query_microsoft_graph_list( method="GET", resource=f"users/{member_id}/memberOf" From d75f3f5ccab4038cd4c316585aca1459e1106b61 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 8 Nov 2021 15:58:00 -0800 Subject: [PATCH 082/103] cleanup --- .../__app__/onefuzzlib/azure/group_membership.py | 14 ++++---------- src/pytypes/onefuzztypes/models.py | 5 ----- 2 files changed, 4 insertions(+), 15 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/azure/group_membership.py b/src/api-service/__app__/onefuzzlib/azure/group_membership.py index bb0bd75743..0de135e0f4 100644 --- a/src/api-service/__app__/onefuzzlib/azure/group_membership.py +++ b/src/api-service/__app__/onefuzzlib/azure/group_membership.py @@ -4,11 +4,10 @@ # Licensed under the MIT License. import os -from typing import List, Protocol +from typing import Dict, List, Protocol from uuid import UUID -from onefuzztypes.models import GroupMemebership, InstanceConfig - +from ..config import InstanceConfig from .creds import query_microsoft_graph_list @@ -42,13 +41,8 @@ def get_groups(self, member_id: UUID) -> List[UUID]: class StaticGroupMembership(GroupMembershipChecker): - def __init__(self, memberships: List[GroupMemebership]): + def __init__(self, memberships: Dict[UUID, List[UUID]]): self.memberships = memberships def get_groups(self, member_id: UUID) -> List[UUID]: - groups = set() - for membership in self.memberships: - if membership.principal_id == member_id: - for g in membership.groups: - groups.add(g) - return list(groups) + return self.memberships.get(member_id, []) diff --git a/src/pytypes/onefuzztypes/models.py b/src/pytypes/onefuzztypes/models.py index 389bc80fe1..27506150ef 100644 --- a/src/pytypes/onefuzztypes/models.py +++ b/src/pytypes/onefuzztypes/models.py @@ -846,11 +846,6 @@ class ApiAccessRule(BaseModel): GroupId = UUID -class GroupMemebership(BaseModel): - principal_id: UUID - groups: List[UUID] - - class InstanceConfig(BaseModel): # initial set of admins can only be set during deployment. # if admins are set, only admins can update instance configs. From b1de7c044c0aa62fda6fb04b5339d5cba00fc23f Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 8 Nov 2021 16:14:18 -0800 Subject: [PATCH 083/103] build fix --- src/api-service/__app__/onefuzzlib/request.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/request.py b/src/api-service/__app__/onefuzzlib/request.py index 614a8b24b0..7ade4e908b 100644 --- a/src/api-service/__app__/onefuzzlib/request.py +++ b/src/api-service/__app__/onefuzzlib/request.py @@ -39,9 +39,6 @@ def get_rules() -> Optional[RequestAccess]: return None -membership_checker = create_group_membership_checker() - - def check_access(req: HttpRequest) -> Optional[Error]: rules = get_rules() @@ -54,6 +51,7 @@ def check_access(req: HttpRequest) -> Optional[Error]: member_id = UUID(req.headers["x-ms-client-principal-id"]) try: + membership_checker = create_group_membership_checker() result = membership_checker.is_member(rule.allowed_groups_ids, member_id) if not result: logging.error( From e047646bc4501420f9096d188ca4dfb1f360b63f Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 8 Nov 2021 16:25:50 -0800 Subject: [PATCH 084/103] foramtting --- .../functional_tests/api_resttriction_test.py | 24 +++++++++---------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/src/api-service/functional_tests/api_resttriction_test.py b/src/api-service/functional_tests/api_resttriction_test.py index 3c393039f3..713c2c7a18 100644 --- a/src/api-service/functional_tests/api_resttriction_test.py +++ b/src/api-service/functional_tests/api_resttriction_test.py @@ -4,14 +4,15 @@ # Licensed under the MIT License. import argparse +import os import uuid -from azure.cli.core import get_default_cli +from uuid import UUID +from azure.cli.core import get_default_cli from onefuzz.api import Onefuzz -from uuid import UUID -import os from onefuzztypes.models import ApiAccessRule + def az_cli(args): cli = get_default_cli() cli.logging_cls @@ -38,31 +39,29 @@ def assign(self, group_id: UUID, member_id: UUID) -> None: self.onefuzz.instance_config.update(instance_config) - def assign_current_user(self, group_id: UUID) -> None: onefuzz_service_appId = az_cli( - [ - "ad", - "signed-in-user", - "show", - ]) + [ + "ad", + "signed-in-user", + "show", + ] + ) member_id = UUID(onefuzz_service_appId["objectid"]) self.assign(group_id, member_id) - def test_restriction_on_current_user(self) -> None: print("Checking that the current user can get info") info = self.onefuzz.info.get() - print("Creating test group") group_id = uuid.uuid4() print("Adding restriction to the info endpoint") instance_config = self.onefuzz.instance_config if instance_config.api_access_rules is None: - instance_config.api_access_rules = [] + instance_config.api_access_rules = [] instance_config.api_access_rules.append( ApiAccessRule( @@ -100,6 +99,5 @@ def main() -> None: tester.restore_config() - if __name__ == "__main__": main() From 745f40f31ee91bbe0853093387980c2cb9b69468 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 8 Nov 2021 16:58:18 -0800 Subject: [PATCH 085/103] fix functional tests --- .../functional_tests/api_resttriction_test.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/api-service/functional_tests/api_resttriction_test.py b/src/api-service/functional_tests/api_resttriction_test.py index 713c2c7a18..48eeee7046 100644 --- a/src/api-service/functional_tests/api_resttriction_test.py +++ b/src/api-service/functional_tests/api_resttriction_test.py @@ -6,6 +6,7 @@ import argparse import os import uuid +from typing import Any, List from uuid import UUID from azure.cli.core import get_default_cli @@ -13,7 +14,7 @@ from onefuzztypes.models import ApiAccessRule -def az_cli(args): +def az_cli(args: List[str]) -> Any: cli = get_default_cli() cli.logging_cls cli.invoke(args, out_file=open(os.devnull, "w")) @@ -24,7 +25,7 @@ def az_cli(args): class APIRestrictionTests: - def __init__(self, onefuzz_config_path=None): + def __init__(self, onefuzz_config_path: str = None) -> None: self.onefuzz = Onefuzz(config_path=onefuzz_config_path) self.intial_config = self.onefuzz.instance_config.get() @@ -33,6 +34,8 @@ def restore_config(self) -> None: def assign(self, group_id: UUID, member_id: UUID) -> None: instance_config = self.onefuzz.instance_config.get() + if instance_config.group_membership is None: + instance_config.group_membership = {} if member_id in instance_config.group_membership: if group_id not in instance_config.group_membership[member_id]: instance_config.group_membership[member_id].append(group_id) @@ -59,15 +62,15 @@ def test_restriction_on_current_user(self) -> None: group_id = uuid.uuid4() print("Adding restriction to the info endpoint") - instance_config = self.onefuzz.instance_config + instance_config = self.onefuzz.instance_config.get() if instance_config.api_access_rules is None: instance_config.api_access_rules = [] instance_config.api_access_rules.append( ApiAccessRule( - path="/info", - group_ids=[group_id], - method="GET", + endpoint="/info", + allowed_groups=[group_id], + methods=["GET"], ) ) self.onefuzz.instance_config.update(instance_config) From 804026fcbc2fadf1b49bc6a84d7f16822346f399 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 8 Nov 2021 17:11:04 -0800 Subject: [PATCH 086/103] flake fix --- src/api-service/functional_tests/api_resttriction_test.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/api-service/functional_tests/api_resttriction_test.py b/src/api-service/functional_tests/api_resttriction_test.py index 48eeee7046..2fe1834509 100644 --- a/src/api-service/functional_tests/api_resttriction_test.py +++ b/src/api-service/functional_tests/api_resttriction_test.py @@ -56,7 +56,7 @@ def assign_current_user(self, group_id: UUID) -> None: def test_restriction_on_current_user(self) -> None: print("Checking that the current user can get info") - info = self.onefuzz.info.get() + self.onefuzz.info.get() print("Creating test group") group_id = uuid.uuid4() @@ -77,16 +77,16 @@ def test_restriction_on_current_user(self) -> None: print("Checking that the current user cannot get info") try: - info = self.onefuzz.info.get() + self.onefuzz.info.get() raise Exception("Expected exception not thrown") - except Exception as e: + except Exception: pass print("Assigning current user to test group") self.assign_current_user(group_id) print("Checking that the current user can get info") - info = self.onefuzz.info.get() + self.onefuzz.info.get() def main() -> None: From 5e4434d45a09eb410cfff491de8d3b35ea697407 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 8 Nov 2021 17:16:23 -0800 Subject: [PATCH 087/103] lint --- src/api-service/__app__/onefuzzlib/azure/group_membership.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/api-service/__app__/onefuzzlib/azure/group_membership.py b/src/api-service/__app__/onefuzzlib/azure/group_membership.py index 0de135e0f4..1bc9978474 100644 --- a/src/api-service/__app__/onefuzzlib/azure/group_membership.py +++ b/src/api-service/__app__/onefuzzlib/azure/group_membership.py @@ -3,7 +3,6 @@ # Copyright (c) Microsoft Corporation. # Licensed under the MIT License. -import os from typing import Dict, List, Protocol from uuid import UUID From 1cba246c321084e04847c80b7b7f195543959cef Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 15 Nov 2021 13:56:14 -0800 Subject: [PATCH 088/103] - return None when not rule is found - add timeout to access rule cache - update python version in deployment - update unit tests --- src/api-service/__app__/onefuzzlib/request.py | 5 ++++- src/api-service/__app__/onefuzzlib/request_access.py | 6 +++--- src/api-service/tests/test_request_access.py | 6 +++--- src/deployment/azuredeploy.json | 2 +- 4 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/request.py b/src/api-service/__app__/onefuzzlib/request.py index 7ade4e908b..5aaedbc270 100644 --- a/src/api-service/__app__/onefuzzlib/request.py +++ b/src/api-service/__app__/onefuzzlib/request.py @@ -30,7 +30,7 @@ from onefuzztypes.requests import BaseRequest # noqa: F401 -@cached +@cached(ttl=60) def get_rules() -> Optional[RequestAccess]: config = InstanceConfig.fetch() if config.api_access_rules: @@ -48,6 +48,9 @@ def check_access(req: HttpRequest) -> Optional[Error]: path = urllib.parse.urlparse(req.url).path rule = rules.get_matching_rules(req.method, path) + if not rule: + return None + member_id = UUID(req.headers["x-ms-client-principal-id"]) try: diff --git a/src/api-service/__app__/onefuzzlib/request_access.py b/src/api-service/__app__/onefuzzlib/request_access.py index 6dc0c4a635..cbe15be6e8 100644 --- a/src/api-service/__app__/onefuzzlib/request_access.py +++ b/src/api-service/__app__/onefuzzlib/request_access.py @@ -1,4 +1,4 @@ -from typing import Dict, List +from typing import Dict, List, Optional from uuid import UUID from onefuzztypes.models import ApiAccessRule @@ -71,7 +71,7 @@ def __add_url__(self, methods: List[str], path: str, rules: Rules) -> None: for method in methods: current_node.rules[method] = rules - def get_matching_rules(self, method: str, path: str) -> Rules: + def get_matching_rules(self, method: str, path: str) -> Optional[Rules]: method = method.upper() segments = path.split("/") current_node = self.root @@ -79,7 +79,7 @@ def get_matching_rules(self, method: str, path: str) -> Rules: if method in current_node.rules: current_rule = current_node.rules[method] else: - current_rule = RequestAccess.Rules() + current_rule = None current_segment_index = 0 diff --git a/src/api-service/tests/test_request_access.py b/src/api-service/tests/test_request_access.py index c75ccc933c..98eb58f343 100644 --- a/src/api-service/tests/test_request_access.py +++ b/src/api-service/tests/test_request_access.py @@ -11,7 +11,7 @@ def test_empty(self) -> None: request_access1 = RequestAccess.build([]) rules1 = request_access1.get_matching_rules("get", "a/b/c") - self.assertEqual(len(rules1.allowed_groups_ids), 0, "expected nothing") + self.assertEqual(rules1, None, "expected nothing") guid2 = uuid.uuid4() request_access1 = RequestAccess.build( @@ -24,7 +24,7 @@ def test_empty(self) -> None: ] ) rules1 = request_access1.get_matching_rules("get", "") - self.assertEqual(len(rules1.allowed_groups_ids), 0, "expected nothing") + self.assertEqual(rules1, None, "expected nothing") def test_exact_match(self) -> None: @@ -46,7 +46,7 @@ def test_exact_match(self) -> None: self.assertNotEqual(len(rules1.allowed_groups_ids), 0, "empty allowed groups") self.assertEqual(rules1.allowed_groups_ids[0], guid1) - self.assertEqual(len(rules2.allowed_groups_ids), 0, "expected nothing") + self.assertEqual(rules2, None, "expected nothing") def test_wildcard(self) -> None: guid1 = uuid.uuid4() diff --git a/src/deployment/azuredeploy.json b/src/deployment/azuredeploy.json index f29d3f28dd..9930c7dd22 100644 --- a/src/deployment/azuredeploy.json +++ b/src/deployment/azuredeploy.json @@ -266,7 +266,7 @@ "value": "[parameters('owner')]" } ], - "linuxFxVersion": "Python|3.7", + "linuxFxVersion": "Python|3.8", "alwaysOn": true, "defaultDocuments": [], "httpLoggingEnabled": true, From 670dc9cfaec58ae14a724fa113908d508217b348 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 15 Nov 2021 16:02:46 -0800 Subject: [PATCH 089/103] fix type in tests fix type in static group memebership --- .../__app__/onefuzzlib/azure/group_membership.py | 4 ++-- src/api-service/__app__/onefuzzlib/request_access.py | 3 +-- src/api-service/tests/test_request_access.py | 9 +++++++++ src/pytypes/onefuzztypes/models.py | 3 ++- 4 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/azure/group_membership.py b/src/api-service/__app__/onefuzzlib/azure/group_membership.py index 1bc9978474..6346bd8743 100644 --- a/src/api-service/__app__/onefuzzlib/azure/group_membership.py +++ b/src/api-service/__app__/onefuzzlib/azure/group_membership.py @@ -40,8 +40,8 @@ def get_groups(self, member_id: UUID) -> List[UUID]: class StaticGroupMembership(GroupMembershipChecker): - def __init__(self, memberships: Dict[UUID, List[UUID]]): + def __init__(self, memberships: Dict[str, List[UUID]]): self.memberships = memberships def get_groups(self, member_id: UUID) -> List[UUID]: - return self.memberships.get(member_id, []) + return self.memberships.get(str(member_id), []) diff --git a/src/api-service/__app__/onefuzzlib/request_access.py b/src/api-service/__app__/onefuzzlib/request_access.py index cbe15be6e8..efbda15072 100644 --- a/src/api-service/__app__/onefuzzlib/request_access.py +++ b/src/api-service/__app__/onefuzzlib/request_access.py @@ -75,11 +75,10 @@ def get_matching_rules(self, method: str, path: str) -> Optional[Rules]: method = method.upper() segments = path.split("/") current_node = self.root + current_rule = None if method in current_node.rules: current_rule = current_node.rules[method] - else: - current_rule = None current_segment_index = 0 diff --git a/src/api-service/tests/test_request_access.py b/src/api-service/tests/test_request_access.py index 98eb58f343..9e1885c021 100644 --- a/src/api-service/tests/test_request_access.py +++ b/src/api-service/tests/test_request_access.py @@ -43,6 +43,8 @@ def test_exact_match(self) -> None: rules1 = request_access.get_matching_rules("get", "a/b/c") rules2 = request_access.get_matching_rules("get", "b/b/e") + assert rules1 is not None + assert rules2 is not None self.assertNotEqual(len(rules1.allowed_groups_ids), 0, "empty allowed groups") self.assertEqual(rules1.allowed_groups_ids[0], guid1) @@ -63,6 +65,7 @@ def test_wildcard(self) -> None: rules = request_access.get_matching_rules("get", "b/b/c") + assert rules is not None self.assertNotEqual(len(rules.allowed_groups_ids), 0, "empty allowed groups") self.assertEqual(rules.allowed_groups_ids[0], guid1) @@ -111,6 +114,7 @@ def test_priority(self) -> None: rules = request_access.get_matching_rules("get", "a/b/c") + assert rules is not None self.assertEqual( rules.allowed_groups_ids[0], guid2, @@ -145,16 +149,19 @@ def test_inherit_rule(self) -> None: ) rules1 = request_access.get_matching_rules("get", "a/b/c/d") + assert rules1 is not None self.assertEqual( rules1.allowed_groups_ids[0], guid1, "expected to inherit rule of a/b/c" ) rules2 = request_access.get_matching_rules("get", "f/b/c/d") + assert rules2 is not None self.assertEqual( rules2.allowed_groups_ids[0], guid2, "expected to inherit rule of f/*/c" ) rules3 = request_access.get_matching_rules("post", "a/b/c/d") + assert rules3 is not None self.assertEqual( rules3.allowed_groups_ids[0], guid3, "expected to inherit rule of post a/b" ) @@ -180,11 +187,13 @@ def test_override_rule(self) -> None: ) rules1 = request_access.get_matching_rules("get", "a/b/c") + assert rules1 is not None self.assertEqual( rules1.allowed_groups_ids[0], guid1, "expected to inherit rule of a/b/c" ) rules2 = request_access.get_matching_rules("get", "a/b/c/d") + assert rules2 is not None self.assertEqual( rules2.allowed_groups_ids[0], guid2, "expected to inherit rule of a/b/c/d" ) diff --git a/src/pytypes/onefuzztypes/models.py b/src/pytypes/onefuzztypes/models.py index 27506150ef..4d5d9514b3 100644 --- a/src/pytypes/onefuzztypes/models.py +++ b/src/pytypes/onefuzztypes/models.py @@ -842,7 +842,8 @@ class ApiAccessRule(BaseModel): allowed_groups: List[UUID] -PrincipalID = UUID +# json dumps doesn't support UUID as dictionary key +PrincipalID = str GroupId = UUID From c1023a2900fcc25f95b5a4669a3879590a0f43eb Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 15 Nov 2021 16:12:09 -0800 Subject: [PATCH 090/103] fix test --- src/api-service/tests/test_request_access.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/api-service/tests/test_request_access.py b/src/api-service/tests/test_request_access.py index 9e1885c021..57fdf2ecf8 100644 --- a/src/api-service/tests/test_request_access.py +++ b/src/api-service/tests/test_request_access.py @@ -44,7 +44,6 @@ def test_exact_match(self) -> None: rules2 = request_access.get_matching_rules("get", "b/b/e") assert rules1 is not None - assert rules2 is not None self.assertNotEqual(len(rules1.allowed_groups_ids), 0, "empty allowed groups") self.assertEqual(rules1.allowed_groups_ids[0], guid1) From b475c1fbfc88c5245a3771d33dc262830f9e115a Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 16 Nov 2021 10:18:03 -0800 Subject: [PATCH 091/103] better error message fix group membership check logic --- .../__app__/onefuzzlib/azure/group_membership.py | 7 ++++++- src/api-service/__app__/onefuzzlib/request.py | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/azure/group_membership.py b/src/api-service/__app__/onefuzzlib/azure/group_membership.py index 6346bd8743..0f1591d427 100644 --- a/src/api-service/__app__/onefuzzlib/azure/group_membership.py +++ b/src/api-service/__app__/onefuzzlib/azure/group_membership.py @@ -17,7 +17,12 @@ def is_member(self, group_ids: List[UUID], member_id: UUID) -> bool: return True groups = self.get_groups(member_id) - return group_ids in groups + for g in group_ids: + if g in groups: + return True + + return False + def get_groups(self, member_id: UUID) -> List[UUID]: """Gets all the groups of the provided member""" diff --git a/src/api-service/__app__/onefuzzlib/request.py b/src/api-service/__app__/onefuzzlib/request.py index 5aaedbc270..1edcfc9e02 100644 --- a/src/api-service/__app__/onefuzzlib/request.py +++ b/src/api-service/__app__/onefuzzlib/request.py @@ -64,7 +64,7 @@ def check_access(req: HttpRequest) -> Optional[Error]: ) return Error( code=ErrorCode.UNAUTHORIZED, - errors=["not approved to use this instance of onefuzz"], + errors=["not approved to use this endpoint"], ) except Exception as e: return Error( From 51e84cf4d16a55275be06d0b675c79134e269e25 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 16 Nov 2021 10:26:07 -0800 Subject: [PATCH 092/103] format --- src/api-service/__app__/onefuzzlib/azure/group_membership.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/api-service/__app__/onefuzzlib/azure/group_membership.py b/src/api-service/__app__/onefuzzlib/azure/group_membership.py index 0f1591d427..b8996a7c96 100644 --- a/src/api-service/__app__/onefuzzlib/azure/group_membership.py +++ b/src/api-service/__app__/onefuzzlib/azure/group_membership.py @@ -23,7 +23,6 @@ def is_member(self, group_ids: List[UUID], member_id: UUID) -> bool: return False - def get_groups(self, member_id: UUID) -> List[UUID]: """Gets all the groups of the provided member""" From 735e7b11544c9c2c00e77083fc2b3104fe9ae3d6 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 16 Nov 2021 10:48:22 -0800 Subject: [PATCH 093/103] remove old implementation of check_access make the group membership query transitive --- .../__app__/onefuzzlib/azure/group_membership.py | 2 +- src/api-service/__app__/onefuzzlib/request.py | 13 ------------- 2 files changed, 1 insertion(+), 14 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/azure/group_membership.py b/src/api-service/__app__/onefuzzlib/azure/group_membership.py index b8996a7c96..0b24914367 100644 --- a/src/api-service/__app__/onefuzzlib/azure/group_membership.py +++ b/src/api-service/__app__/onefuzzlib/azure/group_membership.py @@ -38,7 +38,7 @@ def create_group_membership_checker() -> GroupMembershipChecker: class AzureADGroupMembership(GroupMembershipChecker): def get_groups(self, member_id: UUID) -> List[UUID]: response = query_microsoft_graph_list( - method="GET", resource=f"users/{member_id}/memberOf" + method="GET", resource=f"users/{member_id}/transitiveMemberOf" ) return response diff --git a/src/api-service/__app__/onefuzzlib/request.py b/src/api-service/__app__/onefuzzlib/request.py index 1edcfc9e02..4c49263e7b 100644 --- a/src/api-service/__app__/onefuzzlib/request.py +++ b/src/api-service/__app__/onefuzzlib/request.py @@ -75,19 +75,6 @@ def check_access(req: HttpRequest) -> Optional[Error]: return None -def check_access_(req: HttpRequest) -> Optional[Error]: - - if "ONEFUZZ_AAD_GROUP_ID" in os.environ: - message = "ONEFUZZ_AAD_GROUP_ID configuration not supported" - logging.error(message) - return Error( - code=ErrorCode.INVALID_CONFIGURATION, - errors=[message], - ) - else: - return None - - def ok( data: Union[BaseResponse, Sequence[BaseResponse], ModelMixin, Sequence[ModelMixin]] ) -> HttpResponse: From 130891a26b72ff862340cbd02ca8dfff9e56832f Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 16 Nov 2021 18:48:59 -0800 Subject: [PATCH 094/103] fix typo is name api_access_rules is now a dictionary add environment variable to disable cache --- src/api-service/__app__/onefuzzlib/request.py | 9 +- .../__app__/onefuzzlib/request_access.py | 17 +- .../functional_tests/api_restriction_test.py | 183 ++++++++++++++++++ .../functional_tests/api_resttriction_test.py | 106 ---------- .../functional_tests/requirements.txt | 6 +- src/api-service/tests/test_request_access.py | 64 +++--- src/pytypes/onefuzztypes/models.py | 4 +- 7 files changed, 228 insertions(+), 161 deletions(-) create mode 100644 src/api-service/functional_tests/api_restriction_test.py delete mode 100644 src/api-service/functional_tests/api_resttriction_test.py diff --git a/src/api-service/__app__/onefuzzlib/request.py b/src/api-service/__app__/onefuzzlib/request.py index 4c49263e7b..4206a0434d 100644 --- a/src/api-service/__app__/onefuzzlib/request.py +++ b/src/api-service/__app__/onefuzzlib/request.py @@ -30,7 +30,6 @@ from onefuzztypes.requests import BaseRequest # noqa: F401 -@cached(ttl=60) def get_rules() -> Optional[RequestAccess]: config = InstanceConfig.fetch() if config.api_access_rules: @@ -38,9 +37,15 @@ def get_rules() -> Optional[RequestAccess]: else: return None +@cached(ttl=60) +def get_rules_cached() -> Optional[RequestAccess]: + return get_rules() def check_access(req: HttpRequest) -> Optional[Error]: - rules = get_rules() + if "NO_REQUEST_ACCESS_RULES_CACHE" in os.environ: + rules = get_rules() + else: + rules = get_rules_cached() if not rules: return None diff --git a/src/api-service/__app__/onefuzzlib/request_access.py b/src/api-service/__app__/onefuzzlib/request_access.py index efbda15072..67347eb65d 100644 --- a/src/api-service/__app__/onefuzzlib/request_access.py +++ b/src/api-service/__app__/onefuzzlib/request_access.py @@ -2,7 +2,6 @@ from uuid import UUID from onefuzztypes.models import ApiAccessRule -from pydantic import parse_raw_as class RuleConflictError(Exception): @@ -41,7 +40,7 @@ def __init__(self) -> None: def __add_url__(self, methods: List[str], path: str, rules: Rules) -> None: methods = list(map(lambda m: m.upper(), methods)) - segments = path.split("/") + segments = [s for s in path.split("/") if s != ""] if len(segments) == 0: return @@ -73,7 +72,7 @@ def __add_url__(self, methods: List[str], path: str, rules: Rules) -> None: def get_matching_rules(self, method: str, path: str) -> Optional[Rules]: method = method.upper() - segments = path.split("/") + segments = [s for s in path.split("/") if s != ""] current_node = self.root current_rule = None @@ -97,17 +96,13 @@ def get_matching_rules(self, method: str, path: str) -> Optional[Rules]: return current_rule @classmethod - def parse_rules(cls, rules_data: str) -> "RequestAccess": - rules = parse_raw_as(List[ApiAccessRule], rules_data) - return cls.build(rules) - - @classmethod - def build(cls, rules: List[ApiAccessRule]) -> "RequestAccess": + def build(cls, rules: Dict[str, ApiAccessRule]) -> "RequestAccess": request_access = RequestAccess() - for rule in rules: + for endpoint in rules: + rule = rules[endpoint] request_access.__add_url__( rule.methods, - rule.endpoint, + endpoint, RequestAccess.Rules(allowed_groups_ids=rule.allowed_groups), ) diff --git a/src/api-service/functional_tests/api_restriction_test.py b/src/api-service/functional_tests/api_restriction_test.py new file mode 100644 index 0000000000..afd9bdfca5 --- /dev/null +++ b/src/api-service/functional_tests/api_restriction_test.py @@ -0,0 +1,183 @@ +#!/usr/bin/env python +# +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +import argparse +import os +from urllib.parse import urlparse +import uuid +from typing import Any, List +from uuid import UUID + +from azure.cli.core import get_default_cli +from onefuzz.api import Onefuzz +from onefuzztypes.models import ApiAccessRule +import time + + +def az_cli(args: List[str]) -> Any: + cli = get_default_cli() + cli.logging_cls + cli.invoke(args, out_file=open(os.devnull, "w")) + if cli.result.result: + return cli.result.result + elif cli.result.error: + raise cli.result.error + + +class APIRestrictionTests: + def __init__(self, onefuzz_config_path: str = None) -> None: + self.onefuzz = Onefuzz(config_path=onefuzz_config_path) + self.intial_config = self.onefuzz.instance_config.get() + + def restore_config(self) -> None: + self.onefuzz.instance_config.update(self.intial_config) + + def assign(self, group_id: UUID, member_id: UUID) -> None: + instance_config = self.onefuzz.instance_config.get() + if instance_config.group_membership is None: + instance_config.group_membership = {} + if member_id in instance_config.group_membership: + if group_id not in instance_config.group_membership[member_id]: + instance_config.group_membership[member_id].append(group_id) + + self.onefuzz.instance_config.update(instance_config) + + def assign_current_user(self, group_id: UUID) -> None: + onefuzz_service_appId = az_cli( + [ + "ad", + "signed-in-user", + "show", + ] + ) + member_id = UUID(onefuzz_service_appId["objectId"]) + self.assign(group_id, member_id) + + def test_restriction_on_current_user(self) -> None: + + print("Checking that the current user can get jobs") + self.onefuzz.jobs.list() + + print("Creating test group") + group_id = uuid.uuid4() + + print("Adding restriction to the jobs endpoint") + instance_config = self.onefuzz.instance_config.get() + if instance_config.api_access_rules is None: + instance_config.api_access_rules = {} + + instance_config.api_access_rules["/api/jobs"] = ApiAccessRule( + allowed_groups=[group_id], + methods=["GET"], + ) + + self.onefuzz.instance_config.update(instance_config) + print("sleep 10 seconds") + # time.sleep(10) + print("Checking that the current user cannot get jobs") + try: + self.onefuzz.jobs.list() + failed = False + except Exception: + failed = True + pass + + if not failed: + raise Exception("Current user was able to get jobs") + + print("Assigning current user to test group") + self.assign_current_user(group_id) + + print("Checking that the current user can get jobs") + self.onefuzz.jobs.list() + + +def disable_api_access_rules_caching(instance_name: str, resource_group: str) -> None: + print("Disabling API access rules caching") + az_cli( + [ + "functionapp", + "config", + "appsettings", + "set", + "--name", + f"{instance_name}", + "--resource-group", + f"{resource_group}", + "--settings", + f"NO_REQUEST_ACCESS_RULES_CACHE=''", + ] + ) + az_cli( + [ + "functionapp", + "restart", + "--name", + f"{instance_name}", + "--resource-group", + f"{resource_group}", + ] + ) + + +def enable_api_access_rules_caching(instance_name: str, resource_group: str) -> None: + print("Enabling API access rules caching") + az_cli( + [ + "functionapp", + "config", + "appsettings", + "delete", + "--name", + f"{instance_name}", + "--resource-group", + f"{resource_group}", + "--setting-names", + f"NO_REQUEST_ACCESS_RULES_CACHE", + ] + ) + + az_cli( + [ + "functionapp", + "restart", + "--name", + f"{instance_name}", + "--resource-group", + f"{resource_group}", + ] + ) + + + # az functionapp restart + + +def main() -> None: + parser = argparse.ArgumentParser() + parser.add_argument("--config_path", default=None) + parser.add_argument("--resource_group", default=None) + args = parser.parse_args() + tester = APIRestrictionTests(args.config_path) + config = tester.onefuzz.config() + + instance_name = urlparse(config.endpoint).netloc.split(".")[0] + + if args.resource_group: + resource_group = args.resource_group + else: + resource_group = instance_name + + try: + disable_api_access_rules_caching(instance_name, resource_group) + print("test current user restriction") + tester.test_restriction_on_current_user() + finally: + pass + # enable_api_access_rules_caching(instance_name, resource_group) + # tester.restore_config() + + +if __name__ == "__main__": + main() diff --git a/src/api-service/functional_tests/api_resttriction_test.py b/src/api-service/functional_tests/api_resttriction_test.py deleted file mode 100644 index 2fe1834509..0000000000 --- a/src/api-service/functional_tests/api_resttriction_test.py +++ /dev/null @@ -1,106 +0,0 @@ -#!/usr/bin/env python -# -# Copyright (c) Microsoft Corporation. -# Licensed under the MIT License. - -import argparse -import os -import uuid -from typing import Any, List -from uuid import UUID - -from azure.cli.core import get_default_cli -from onefuzz.api import Onefuzz -from onefuzztypes.models import ApiAccessRule - - -def az_cli(args: List[str]) -> Any: - cli = get_default_cli() - cli.logging_cls - cli.invoke(args, out_file=open(os.devnull, "w")) - if cli.result.result: - return cli.result.result - elif cli.result.error: - raise cli.result.error - - -class APIRestrictionTests: - def __init__(self, onefuzz_config_path: str = None) -> None: - self.onefuzz = Onefuzz(config_path=onefuzz_config_path) - self.intial_config = self.onefuzz.instance_config.get() - - def restore_config(self) -> None: - self.onefuzz.instance_config.update(self.intial_config) - - def assign(self, group_id: UUID, member_id: UUID) -> None: - instance_config = self.onefuzz.instance_config.get() - if instance_config.group_membership is None: - instance_config.group_membership = {} - if member_id in instance_config.group_membership: - if group_id not in instance_config.group_membership[member_id]: - instance_config.group_membership[member_id].append(group_id) - - self.onefuzz.instance_config.update(instance_config) - - def assign_current_user(self, group_id: UUID) -> None: - onefuzz_service_appId = az_cli( - [ - "ad", - "signed-in-user", - "show", - ] - ) - member_id = UUID(onefuzz_service_appId["objectid"]) - self.assign(group_id, member_id) - - def test_restriction_on_current_user(self) -> None: - - print("Checking that the current user can get info") - self.onefuzz.info.get() - - print("Creating test group") - group_id = uuid.uuid4() - - print("Adding restriction to the info endpoint") - instance_config = self.onefuzz.instance_config.get() - if instance_config.api_access_rules is None: - instance_config.api_access_rules = [] - - instance_config.api_access_rules.append( - ApiAccessRule( - endpoint="/info", - allowed_groups=[group_id], - methods=["GET"], - ) - ) - self.onefuzz.instance_config.update(instance_config) - - print("Checking that the current user cannot get info") - try: - self.onefuzz.info.get() - raise Exception("Expected exception not thrown") - except Exception: - pass - - print("Assigning current user to test group") - self.assign_current_user(group_id) - - print("Checking that the current user can get info") - self.onefuzz.info.get() - - -def main() -> None: - parser = argparse.ArgumentParser() - parser.add_argument("--config_path", default=None) - args = parser.parse_args() - tester = APIRestrictionTests(args.config_path) - - try: - print("test current user restriction") - tester.test_restriction_on_current_user() - finally: - tester.restore_config() - - -if __name__ == "__main__": - main() diff --git a/src/api-service/functional_tests/requirements.txt b/src/api-service/functional_tests/requirements.txt index 19d50efb77..02f6e63732 100644 --- a/src/api-service/functional_tests/requirements.txt +++ b/src/api-service/functional_tests/requirements.txt @@ -1,2 +1,4 @@ -onefuzz==0.0.0 -azure-cli-core==2.27.2 \ No newline at end of file +../../cli +../../pytypes +azure-cli-core==2.27.2 +azure-cli==2.27.2 \ No newline at end of file diff --git a/src/api-service/tests/test_request_access.py b/src/api-service/tests/test_request_access.py index 57fdf2ecf8..e81d6e6a7f 100644 --- a/src/api-service/tests/test_request_access.py +++ b/src/api-service/tests/test_request_access.py @@ -15,13 +15,12 @@ def test_empty(self) -> None: guid2 = uuid.uuid4() request_access1 = RequestAccess.build( - [ - ApiAccessRule( + { + "a/b/c": ApiAccessRule( methods=["get"], - endpoint="a/b/c", allowed_groups=[guid2], ) - ] + } ) rules1 = request_access1.get_matching_rules("get", "") self.assertEqual(rules1, None, "expected nothing") @@ -31,13 +30,12 @@ def test_exact_match(self) -> None: guid1 = uuid.uuid4() request_access = RequestAccess.build( - [ - ApiAccessRule( + { + "a/b/c": ApiAccessRule( methods=["get"], - endpoint="a/b/c", allowed_groups=[guid1], ) - ] + } ) rules1 = request_access.get_matching_rules("get", "a/b/c") @@ -53,13 +51,12 @@ def test_wildcard(self) -> None: guid1 = uuid.uuid4() request_access = RequestAccess.build( - [ - ApiAccessRule( + { + "b/*/c": ApiAccessRule( methods=["get"], - endpoint="b/*/c", allowed_groups=[guid1], ) - ] + } ) rules = request_access.get_matching_rules("get", "b/b/c") @@ -73,18 +70,16 @@ def test_adding_rule_on_same_path(self) -> None: try: RequestAccess.build( - [ - ApiAccessRule( + { + "a/b/c": ApiAccessRule( methods=["get"], - endpoint="a/b/c", allowed_groups=[guid1], ), - ApiAccessRule( + "a/b/c/": ApiAccessRule( methods=["get"], - endpoint="a/b/c", allowed_groups=[], ), - ] + } ) self.fail("this is expected to fail") @@ -97,18 +92,16 @@ def test_priority(self) -> None: guid2 = uuid.uuid4() request_access = RequestAccess.build( - [ - ApiAccessRule( + { + "a/*/c": ApiAccessRule( methods=["get"], - endpoint="a/*/c", allowed_groups=[guid1], ), - ApiAccessRule( + "a/b/c": ApiAccessRule( methods=["get"], - endpoint="a/b/c", allowed_groups=[guid2], ), - ] + } ) rules = request_access.get_matching_rules("get", "a/b/c") @@ -128,23 +121,20 @@ def test_inherit_rule(self) -> None: guid3 = uuid.uuid4() request_access = RequestAccess.build( - [ - ApiAccessRule( + { + "a/b/c": ApiAccessRule( methods=["get"], - endpoint="a/b/c", allowed_groups=[guid1], ), - ApiAccessRule( + "f/*/c": ApiAccessRule( methods=["get"], - endpoint="f/*/c", allowed_groups=[guid2], ), - ApiAccessRule( + "a/b": ApiAccessRule( methods=["post"], - endpoint="a/b", allowed_groups=[guid3], ), - ] + } ) rules1 = request_access.get_matching_rules("get", "a/b/c/d") @@ -171,18 +161,16 @@ def test_override_rule(self) -> None: guid2 = uuid.uuid4() request_access = RequestAccess.build( - [ - ApiAccessRule( + { + "a/b/c": ApiAccessRule( methods=["get"], - endpoint="a/b/c", allowed_groups=[guid1], ), - ApiAccessRule( + "a/b/c/d": ApiAccessRule( methods=["get"], - endpoint="a/b/c/d", allowed_groups=[guid2], ), - ] + } ) rules1 = request_access.get_matching_rules("get", "a/b/c") diff --git a/src/pytypes/onefuzztypes/models.py b/src/pytypes/onefuzztypes/models.py index 4d5d9514b3..0d891a33e7 100644 --- a/src/pytypes/onefuzztypes/models.py +++ b/src/pytypes/onefuzztypes/models.py @@ -838,10 +838,10 @@ class AzureVmExtensionConfig(BaseModel): class ApiAccessRule(BaseModel): methods: List[str] - endpoint: str allowed_groups: List[UUID] +Endpoint = str # json dumps doesn't support UUID as dictionary key PrincipalID = str GroupId = UUID @@ -862,7 +862,7 @@ class InstanceConfig(BaseModel): ) extensions: Optional[AzureVmExtensionConfig] proxy_vm_sku: str = Field(default="Standard_B2s") - api_access_rules: Optional[List[ApiAccessRule]] = None + api_access_rules: Optional[Dict[Endpoint, ApiAccessRule]] = None group_membership: Optional[Dict[PrincipalID, List[GroupId]]] = None def update(self, config: "InstanceConfig") -> None: From 0ad5d0f636d75091556007d7f9aa6818142671a6 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 16 Nov 2021 20:09:34 -0800 Subject: [PATCH 095/103] generate docs --- docs/webhook_events.md | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/docs/webhook_events.md b/docs/webhook_events.md index e0817d4aab..b24aaf5078 100644 --- a/docs/webhook_events.md +++ b/docs/webhook_events.md @@ -673,10 +673,6 @@ Each event will be submitted via HTTP POST to the user provided URL. "title": "Allowed Groups", "type": "array" }, - "endpoint": { - "title": "Endpoint", - "type": "string" - }, "methods": { "items": { "type": "string" @@ -687,7 +683,6 @@ Each event will be submitted via HTTP POST to the user provided URL. }, "required": [ "methods", - "endpoint", "allowed_groups" ], "title": "ApiAccessRule", @@ -788,11 +783,11 @@ Each event will be submitted via HTTP POST to the user provided URL. "type": "array" }, "api_access_rules": { - "items": { + "additionalProperties": { "$ref": "#/definitions/ApiAccessRule" }, "title": "Api Access Rules", - "type": "array" + "type": "object" }, "extensions": { "$ref": "#/definitions/AzureVmExtensionConfig" @@ -4991,10 +4986,6 @@ Each event will be submitted via HTTP POST to the user provided URL. "title": "Allowed Groups", "type": "array" }, - "endpoint": { - "title": "Endpoint", - "type": "string" - }, "methods": { "items": { "type": "string" @@ -5005,7 +4996,6 @@ Each event will be submitted via HTTP POST to the user provided URL. }, "required": [ "methods", - "endpoint", "allowed_groups" ], "title": "ApiAccessRule", @@ -5935,11 +5925,11 @@ Each event will be submitted via HTTP POST to the user provided URL. "type": "array" }, "api_access_rules": { - "items": { + "additionalProperties": { "$ref": "#/definitions/ApiAccessRule" }, "title": "Api Access Rules", - "type": "array" + "type": "object" }, "extensions": { "$ref": "#/definitions/AzureVmExtensionConfig" From 00c4358319b8bd4e5ed1c0449000bce8c4ca01ae Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 16 Nov 2021 20:28:13 -0800 Subject: [PATCH 096/103] format --- src/api-service/__app__/onefuzzlib/request.py | 2 + .../__app__/onefuzzlib/request_access.py | 4 +- .../functional_tests/api_restriction_test.py | 68 +++++++++---------- 3 files changed, 36 insertions(+), 38 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/request.py b/src/api-service/__app__/onefuzzlib/request.py index 4206a0434d..4a22eaa898 100644 --- a/src/api-service/__app__/onefuzzlib/request.py +++ b/src/api-service/__app__/onefuzzlib/request.py @@ -37,10 +37,12 @@ def get_rules() -> Optional[RequestAccess]: else: return None + @cached(ttl=60) def get_rules_cached() -> Optional[RequestAccess]: return get_rules() + def check_access(req: HttpRequest) -> Optional[Error]: if "NO_REQUEST_ACCESS_RULES_CACHE" in os.environ: rules = get_rules() diff --git a/src/api-service/__app__/onefuzzlib/request_access.py b/src/api-service/__app__/onefuzzlib/request_access.py index 67347eb65d..1699deef43 100644 --- a/src/api-service/__app__/onefuzzlib/request_access.py +++ b/src/api-service/__app__/onefuzzlib/request_access.py @@ -40,7 +40,7 @@ def __init__(self) -> None: def __add_url__(self, methods: List[str], path: str, rules: Rules) -> None: methods = list(map(lambda m: m.upper(), methods)) - segments = [s for s in path.split("/") if s != ""] + segments = [s for s in path.split("/") if s != ""] if len(segments) == 0: return @@ -72,7 +72,7 @@ def __add_url__(self, methods: List[str], path: str, rules: Rules) -> None: def get_matching_rules(self, method: str, path: str) -> Optional[Rules]: method = method.upper() - segments = [s for s in path.split("/") if s != ""] + segments = [s for s in path.split("/") if s != ""] current_node = self.root current_rule = None diff --git a/src/api-service/functional_tests/api_restriction_test.py b/src/api-service/functional_tests/api_restriction_test.py index afd9bdfca5..bd048f195e 100644 --- a/src/api-service/functional_tests/api_restriction_test.py +++ b/src/api-service/functional_tests/api_restriction_test.py @@ -27,10 +27,20 @@ def az_cli(args: List[str]) -> Any: class APIRestrictionTests: - def __init__(self, onefuzz_config_path: str = None) -> None: + def __init__( + self, resource_group: str = None, onefuzz_config_path: str = None + ) -> None: self.onefuzz = Onefuzz(config_path=onefuzz_config_path) self.intial_config = self.onefuzz.instance_config.get() + self.instance_name = urlparse(self.onefuzz.config().endpoint).netloc.split(".")[ + 0 + ] + if resource_group: + self.resource_group = resource_group + else: + self.resource_group = self.instance_name + def restore_config(self) -> None: self.onefuzz.instance_config.update(self.intial_config) @@ -74,8 +84,7 @@ def test_restriction_on_current_user(self) -> None: ) self.onefuzz.instance_config.update(instance_config) - print("sleep 10 seconds") - # time.sleep(10) + restart_instance(self.instance_name, self.resource_group) print("Checking that the current user cannot get jobs") try: self.onefuzz.jobs.list() @@ -89,38 +98,45 @@ def test_restriction_on_current_user(self) -> None: print("Assigning current user to test group") self.assign_current_user(group_id) + restart_instance(self.instance_name, self.resource_group) print("Checking that the current user can get jobs") self.onefuzz.jobs.list() -def disable_api_access_rules_caching(instance_name: str, resource_group: str) -> None: - print("Disabling API access rules caching") +def restart_instance(instance_name: str, resource_group: str) -> None: + print("Restarting instance") az_cli( [ "functionapp", - "config", - "appsettings", - "set", + "restart", "--name", f"{instance_name}", "--resource-group", f"{resource_group}", - "--settings", - f"NO_REQUEST_ACCESS_RULES_CACHE=''", ] ) + + +def disable_api_access_rules_caching(instance_name: str, resource_group: str) -> None: + print("Disabling API access rules caching") az_cli( [ "functionapp", - "restart", + "config", + "appsettings", + "set", "--name", f"{instance_name}", "--resource-group", f"{resource_group}", + "--settings", + f"NO_REQUEST_ACCESS_RULES_CACHE=''", ] ) + restart_instance(instance_name, resource_group) + def enable_api_access_rules_caching(instance_name: str, resource_group: str) -> None: print("Enabling API access rules caching") @@ -139,19 +155,7 @@ def enable_api_access_rules_caching(instance_name: str, resource_group: str) -> ] ) - az_cli( - [ - "functionapp", - "restart", - "--name", - f"{instance_name}", - "--resource-group", - f"{resource_group}", - ] - ) - - - # az functionapp restart + restart_instance(instance_name, resource_group) def main() -> None: @@ -159,24 +163,16 @@ def main() -> None: parser.add_argument("--config_path", default=None) parser.add_argument("--resource_group", default=None) args = parser.parse_args() - tester = APIRestrictionTests(args.config_path) - config = tester.onefuzz.config() - - instance_name = urlparse(config.endpoint).netloc.split(".")[0] - - if args.resource_group: - resource_group = args.resource_group - else: - resource_group = instance_name + tester = APIRestrictionTests(args.resource_group, args.config_path) try: - disable_api_access_rules_caching(instance_name, resource_group) + disable_api_access_rules_caching(tester.instance_name, tester.resource_group) print("test current user restriction") tester.test_restriction_on_current_user() finally: pass - # enable_api_access_rules_caching(instance_name, resource_group) - # tester.restore_config() + enable_api_access_rules_caching(tester.instance_name, tester.resource_group) + tester.restore_config() if __name__ == "__main__": From 93e1236eaf5752504ea1c339c7c395d2c44f36e6 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Wed, 17 Nov 2021 08:45:38 -0800 Subject: [PATCH 097/103] format --- src/api-service/functional_tests/api_restriction_test.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/api-service/functional_tests/api_restriction_test.py b/src/api-service/functional_tests/api_restriction_test.py index bd048f195e..8a39ab717f 100644 --- a/src/api-service/functional_tests/api_restriction_test.py +++ b/src/api-service/functional_tests/api_restriction_test.py @@ -13,7 +13,6 @@ from azure.cli.core import get_default_cli from onefuzz.api import Onefuzz from onefuzztypes.models import ApiAccessRule -import time def az_cli(args: List[str]) -> Any: @@ -131,7 +130,7 @@ def disable_api_access_rules_caching(instance_name: str, resource_group: str) -> "--resource-group", f"{resource_group}", "--settings", - f"NO_REQUEST_ACCESS_RULES_CACHE=''", + "NO_REQUEST_ACCESS_RULES_CACHE=''", ] ) @@ -151,7 +150,7 @@ def enable_api_access_rules_caching(instance_name: str, resource_group: str) -> "--resource-group", f"{resource_group}", "--setting-names", - f"NO_REQUEST_ACCESS_RULES_CACHE", + "NO_REQUEST_ACCESS_RULES_CACHE", ] ) From 5cf3729a524c5adc574b4c3f75b103c4d72db22d Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Wed, 17 Nov 2021 08:59:58 -0800 Subject: [PATCH 098/103] mypy fix --- src/api-service/tests/test_request_access.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/api-service/tests/test_request_access.py b/src/api-service/tests/test_request_access.py index e81d6e6a7f..3021a9c4b5 100644 --- a/src/api-service/tests/test_request_access.py +++ b/src/api-service/tests/test_request_access.py @@ -8,7 +8,7 @@ class TestRequestAccess(unittest.TestCase): def test_empty(self) -> None: - request_access1 = RequestAccess.build([]) + request_access1 = RequestAccess.build({}) rules1 = request_access1.get_matching_rules("get", "a/b/c") self.assertEqual(rules1, None, "expected nothing") From 5f73e99f45029af0db780f01ddf50ba6903f727e Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Thu, 18 Nov 2021 14:47:19 -0800 Subject: [PATCH 099/103] add unit tests to StaticGroupMembership --- .../tests/test_group_membership.py | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 src/api-service/tests/test_group_membership.py diff --git a/src/api-service/tests/test_group_membership.py b/src/api-service/tests/test_group_membership.py new file mode 100644 index 0000000000..b38eff4bc7 --- /dev/null +++ b/src/api-service/tests/test_group_membership.py @@ -0,0 +1,36 @@ +import unittest +import uuid + +from onefuzztypes.models import ApiAccessRule + +from __app__.onefuzzlib.azure.group_membership import GroupMembershipChecker, StaticGroupMembership + + +class TestRequestAccess(unittest.TestCase): + def test_empty(self) -> None: + group_id = uuid.uuid4() + user_id = uuid.uuid4() + checker:GroupMembershipChecker = StaticGroupMembership({}) + + self.assertFalse(checker.is_member([group_id], user_id)) + self.assertTrue(checker.is_member([user_id], user_id)) + + + def test_matching_user_id(self) -> None: + group_id = uuid.uuid4() + user_id1 = uuid.uuid4() + user_id2 = uuid.uuid4() + + checker:GroupMembershipChecker = StaticGroupMembership({str(user_id1): [group_id] }) + self.assertTrue(checker.is_member([user_id1], user_id1)) + self.assertFalse(checker.is_member([user_id1], user_id2)) + + + + def test_user_in_group(self) -> None: + group_id1 = uuid.uuid4() + group_id2 = uuid.uuid4() + user_id = uuid.uuid4() + checker:GroupMembershipChecker = StaticGroupMembership({str(user_id): [group_id1] }) + self.assertTrue(checker.is_member([group_id1], user_id)) + self.assertFalse(checker.is_member([group_id2], user_id)) From 5c46c7d59edc96e535e836949c451717d0f05a5d Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Fri, 19 Nov 2021 12:38:05 -0800 Subject: [PATCH 100/103] adding comments --- src/api-service/__app__/onefuzzlib/request.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/request.py b/src/api-service/__app__/onefuzzlib/request.py index 4a22eaa898..39342f058a 100644 --- a/src/api-service/__app__/onefuzzlib/request.py +++ b/src/api-service/__app__/onefuzzlib/request.py @@ -49,12 +49,14 @@ def check_access(req: HttpRequest) -> Optional[Error]: else: rules = get_rules_cached() + # Noting to enforce if there are no rules. if not rules: return None path = urllib.parse.urlparse(req.url).path rule = rules.get_matching_rules(req.method, path) + # No restriction defined on this endpoint. if not rule: return None @@ -62,8 +64,8 @@ def check_access(req: HttpRequest) -> Optional[Error]: try: membership_checker = create_group_membership_checker() - result = membership_checker.is_member(rule.allowed_groups_ids, member_id) - if not result: + allowed = membership_checker.is_member(rule.allowed_groups_ids, member_id) + if not allowed: logging.error( "unauthorized access: %s is not authorized to access in %s", member_id, From abb2a90dc844a0624cd04440a6c74f4e881cee0f Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 22 Nov 2021 11:55:49 -0800 Subject: [PATCH 101/103] removing NO_REQUEST_ACCESS_RULES_CACHE fix functional test --- src/api-service/__app__/onefuzzlib/request.py | 11 +--- .../functional_tests/api_restriction_test.py | 58 ++++--------------- 2 files changed, 14 insertions(+), 55 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/request.py b/src/api-service/__app__/onefuzzlib/request.py index 39342f058a..1aeb0a3049 100644 --- a/src/api-service/__app__/onefuzzlib/request.py +++ b/src/api-service/__app__/onefuzzlib/request.py @@ -30,6 +30,7 @@ from onefuzztypes.requests import BaseRequest # noqa: F401 +@cached(ttl=60) def get_rules() -> Optional[RequestAccess]: config = InstanceConfig.fetch() if config.api_access_rules: @@ -38,16 +39,8 @@ def get_rules() -> Optional[RequestAccess]: return None -@cached(ttl=60) -def get_rules_cached() -> Optional[RequestAccess]: - return get_rules() - - def check_access(req: HttpRequest) -> Optional[Error]: - if "NO_REQUEST_ACCESS_RULES_CACHE" in os.environ: - rules = get_rules() - else: - rules = get_rules_cached() + rules = get_rules() # Noting to enforce if there are no rules. if not rules: diff --git a/src/api-service/functional_tests/api_restriction_test.py b/src/api-service/functional_tests/api_restriction_test.py index 8a39ab717f..9f446c50b0 100644 --- a/src/api-service/functional_tests/api_restriction_test.py +++ b/src/api-service/functional_tests/api_restriction_test.py @@ -13,6 +13,7 @@ from azure.cli.core import get_default_cli from onefuzz.api import Onefuzz from onefuzztypes.models import ApiAccessRule +import time def az_cli(args: List[str]) -> Any: @@ -47,9 +48,12 @@ def assign(self, group_id: UUID, member_id: UUID) -> None: instance_config = self.onefuzz.instance_config.get() if instance_config.group_membership is None: instance_config.group_membership = {} - if member_id in instance_config.group_membership: - if group_id not in instance_config.group_membership[member_id]: - instance_config.group_membership[member_id].append(group_id) + + if member_id not in instance_config.group_membership: + instance_config.group_membership[member_id] = [] + + if group_id not in instance_config.group_membership[member_id]: + instance_config.group_membership[member_id].append(group_id) self.onefuzz.instance_config.update(instance_config) @@ -62,6 +66,7 @@ def assign_current_user(self, group_id: UUID) -> None: ] ) member_id = UUID(onefuzz_service_appId["objectId"]) + print(f"adding user {member_id}") self.assign(group_id, member_id) def test_restriction_on_current_user(self) -> None: @@ -84,7 +89,9 @@ def test_restriction_on_current_user(self) -> None: self.onefuzz.instance_config.update(instance_config) restart_instance(self.instance_name, self.resource_group) + time.sleep(20) print("Checking that the current user cannot get jobs") + try: self.onefuzz.jobs.list() failed = False @@ -98,6 +105,7 @@ def test_restriction_on_current_user(self) -> None: print("Assigning current user to test group") self.assign_current_user(group_id) restart_instance(self.instance_name, self.resource_group) + time.sleep(20) print("Checking that the current user can get jobs") self.onefuzz.jobs.list() @@ -117,46 +125,6 @@ def restart_instance(instance_name: str, resource_group: str) -> None: ) -def disable_api_access_rules_caching(instance_name: str, resource_group: str) -> None: - print("Disabling API access rules caching") - az_cli( - [ - "functionapp", - "config", - "appsettings", - "set", - "--name", - f"{instance_name}", - "--resource-group", - f"{resource_group}", - "--settings", - "NO_REQUEST_ACCESS_RULES_CACHE=''", - ] - ) - - restart_instance(instance_name, resource_group) - - -def enable_api_access_rules_caching(instance_name: str, resource_group: str) -> None: - print("Enabling API access rules caching") - az_cli( - [ - "functionapp", - "config", - "appsettings", - "delete", - "--name", - f"{instance_name}", - "--resource-group", - f"{resource_group}", - "--setting-names", - "NO_REQUEST_ACCESS_RULES_CACHE", - ] - ) - - restart_instance(instance_name, resource_group) - - def main() -> None: parser = argparse.ArgumentParser() parser.add_argument("--config_path", default=None) @@ -165,13 +133,11 @@ def main() -> None: tester = APIRestrictionTests(args.resource_group, args.config_path) try: - disable_api_access_rules_caching(tester.instance_name, tester.resource_group) print("test current user restriction") tester.test_restriction_on_current_user() finally: - pass - enable_api_access_rules_caching(tester.instance_name, tester.resource_group) tester.restore_config() + pass if __name__ == "__main__": From 882ebd4689e70340a16bb274d345b22e2230cde1 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 22 Nov 2021 12:00:29 -0800 Subject: [PATCH 102/103] format --- src/api-service/__app__/onefuzzlib/request.py | 1 - src/api-service/tests/test_group_membership.py | 18 +++++++++++------- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/request.py b/src/api-service/__app__/onefuzzlib/request.py index 1aeb0a3049..45faa75d3c 100644 --- a/src/api-service/__app__/onefuzzlib/request.py +++ b/src/api-service/__app__/onefuzzlib/request.py @@ -5,7 +5,6 @@ import json import logging -import os import urllib from typing import TYPE_CHECKING, Optional, Sequence, Type, TypeVar, Union from uuid import UUID diff --git a/src/api-service/tests/test_group_membership.py b/src/api-service/tests/test_group_membership.py index b38eff4bc7..5cae33c788 100644 --- a/src/api-service/tests/test_group_membership.py +++ b/src/api-service/tests/test_group_membership.py @@ -3,34 +3,38 @@ from onefuzztypes.models import ApiAccessRule -from __app__.onefuzzlib.azure.group_membership import GroupMembershipChecker, StaticGroupMembership +from __app__.onefuzzlib.azure.group_membership import ( + GroupMembershipChecker, + StaticGroupMembership, +) class TestRequestAccess(unittest.TestCase): def test_empty(self) -> None: group_id = uuid.uuid4() user_id = uuid.uuid4() - checker:GroupMembershipChecker = StaticGroupMembership({}) + checker: GroupMembershipChecker = StaticGroupMembership({}) self.assertFalse(checker.is_member([group_id], user_id)) self.assertTrue(checker.is_member([user_id], user_id)) - def test_matching_user_id(self) -> None: group_id = uuid.uuid4() user_id1 = uuid.uuid4() user_id2 = uuid.uuid4() - checker:GroupMembershipChecker = StaticGroupMembership({str(user_id1): [group_id] }) + checker: GroupMembershipChecker = StaticGroupMembership( + {str(user_id1): [group_id]} + ) self.assertTrue(checker.is_member([user_id1], user_id1)) self.assertFalse(checker.is_member([user_id1], user_id2)) - - def test_user_in_group(self) -> None: group_id1 = uuid.uuid4() group_id2 = uuid.uuid4() user_id = uuid.uuid4() - checker:GroupMembershipChecker = StaticGroupMembership({str(user_id): [group_id1] }) + checker: GroupMembershipChecker = StaticGroupMembership( + {str(user_id): [group_id1]} + ) self.assertTrue(checker.is_member([group_id1], user_id)) self.assertFalse(checker.is_member([group_id2], user_id)) From 41d9c2491973bff775a1b62fa24c9aeb5bb8d141 Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 22 Nov 2021 12:11:58 -0800 Subject: [PATCH 103/103] more formatting --- src/api-service/functional_tests/api_restriction_test.py | 4 ++-- src/api-service/tests/test_group_membership.py | 2 -- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/api-service/functional_tests/api_restriction_test.py b/src/api-service/functional_tests/api_restriction_test.py index 9f446c50b0..3bb13ba772 100644 --- a/src/api-service/functional_tests/api_restriction_test.py +++ b/src/api-service/functional_tests/api_restriction_test.py @@ -5,15 +5,15 @@ import argparse import os -from urllib.parse import urlparse +import time import uuid from typing import Any, List +from urllib.parse import urlparse from uuid import UUID from azure.cli.core import get_default_cli from onefuzz.api import Onefuzz from onefuzztypes.models import ApiAccessRule -import time def az_cli(args: List[str]) -> Any: diff --git a/src/api-service/tests/test_group_membership.py b/src/api-service/tests/test_group_membership.py index 5cae33c788..bdf8274097 100644 --- a/src/api-service/tests/test_group_membership.py +++ b/src/api-service/tests/test_group_membership.py @@ -1,8 +1,6 @@ import unittest import uuid -from onefuzztypes.models import ApiAccessRule - from __app__.onefuzzlib.azure.group_membership import ( GroupMembershipChecker, StaticGroupMembership,