From d49f7c8e3da0fb8dd5ba2f27feddac498891a820 Mon Sep 17 00:00:00 2001 From: Shreya Malviya Date: Tue, 24 May 2022 13:43:18 +0530 Subject: [PATCH 1/7] Island: Add comments about the current API design --- monkey/monkey_island/cc/app.py | 1 + .../cc/resources/agent_controls/stop_agent_check.py | 3 ++- .../cc/resources/agent_controls/stop_all_agents.py | 2 ++ .../monkey_island/cc/resources/attack/attack_report.py | 1 + monkey/monkey_island/cc/resources/auth/auth.py | 1 + monkey/monkey_island/cc/resources/auth/registration.py | 1 + .../monkey_island/cc/resources/blackbox/clear_caches.py | 3 +++ .../cc/resources/blackbox/log_blackbox_endpoint.py | 1 + .../cc/resources/blackbox/monkey_blackbox_endpoint.py | 1 + .../cc/resources/blackbox/telemetry_blackbox_endpoint.py | 1 + .../monkey_island/cc/resources/configuration_export.py | 1 + .../monkey_island/cc/resources/configuration_import.py | 3 +++ .../cc/resources/exploitations/manual_exploitation.py | 5 +++++ .../cc/resources/exploitations/monkey_exploitation.py | 5 +++++ .../monkey_island/cc/resources/island_configuration.py | 5 +++++ monkey/monkey_island/cc/resources/island_logs.py | 1 + monkey/monkey_island/cc/resources/island_mode.py | 7 +++++++ monkey/monkey_island/cc/resources/local_run.py | 6 +++++- monkey/monkey_island/cc/resources/log.py | 1 + monkey/monkey_island/cc/resources/monkey.py | 3 +++ monkey/monkey_island/cc/resources/monkey_download.py | 1 + monkey/monkey_island/cc/resources/pba_file_download.py | 1 + monkey/monkey_island/cc/resources/pba_file_upload.py | 4 ++++ monkey/monkey_island/cc/resources/ransomware_report.py | 1 + monkey/monkey_island/cc/resources/remote_run.py | 9 +++++++++ monkey/monkey_island/cc/resources/security_report.py | 1 + monkey/monkey_island/cc/resources/telemetry.py | 2 ++ monkey/monkey_island/cc/resources/telemetry_feed.py | 1 + .../cc/resources/zero_trust/finding_event.py | 2 ++ .../cc/resources/zero_trust/zero_trust_report.py | 1 + 30 files changed, 73 insertions(+), 2 deletions(-) diff --git a/monkey/monkey_island/cc/app.py b/monkey/monkey_island/cc/app.py index 60d6f9009..da027bf62 100644 --- a/monkey/monkey_island/cc/app.py +++ b/monkey/monkey_island/cc/app.py @@ -183,6 +183,7 @@ def init_api_resources(api: FlaskDIWrapper): api.add_resource(StopAllAgents) # Resources used by black box tests + # API Spec: Fix all the following endpoints, see comments in the resource classes api.add_resource(MonkeyBlackboxEndpoint) api.add_resource(ClearCaches) api.add_resource(LogBlackboxEndpoint) diff --git a/monkey/monkey_island/cc/resources/agent_controls/stop_agent_check.py b/monkey/monkey_island/cc/resources/agent_controls/stop_agent_check.py index e26b2d172..b5585dce3 100644 --- a/monkey/monkey_island/cc/resources/agent_controls/stop_agent_check.py +++ b/monkey/monkey_island/cc/resources/agent_controls/stop_agent_check.py @@ -1,8 +1,9 @@ from monkey_island.cc.resources.AbstractResource import AbstractResource from monkey_island.cc.services.infection_lifecycle import should_agent_die - class StopAgentCheck(AbstractResource): + # API Spec: Not a resource, checking is an action; RPC-style endpoint? + # REST feels okay too but probably rename it to AgentStopStatus or something. urls = ["/api/monkey-control/needs-to-stop/"] def get(self, monkey_guid: int): diff --git a/monkey/monkey_island/cc/resources/agent_controls/stop_all_agents.py b/monkey/monkey_island/cc/resources/agent_controls/stop_all_agents.py index 82667ecf2..c3d719bd8 100644 --- a/monkey/monkey_island/cc/resources/agent_controls/stop_all_agents.py +++ b/monkey/monkey_island/cc/resources/agent_controls/stop_all_agents.py @@ -9,6 +9,7 @@ from monkey_island.cc.services.infection_lifecycle import set_stop_all, should_a class StopAllAgents(AbstractResource): + # API Spec: This is an action and there's no "resource"; RPC-style endpoint? urls = ["/api/monkey-control/stop-all-agents"] @jwt_required @@ -21,5 +22,6 @@ class StopAllAgents(AbstractResource): else: return make_response({}, 400) + # API Spec: This is the exact same thing as what's in StopAgentCheck def get(self, monkey_guid): return {"stop_agent": should_agent_die(monkey_guid)} diff --git a/monkey/monkey_island/cc/resources/attack/attack_report.py b/monkey/monkey_island/cc/resources/attack/attack_report.py index 2dffa929d..ef5fdfa5d 100644 --- a/monkey/monkey_island/cc/resources/attack/attack_report.py +++ b/monkey/monkey_island/cc/resources/attack/attack_report.py @@ -7,6 +7,7 @@ from monkey_island.cc.services.attack.attack_schema import SCHEMA class AttackReport(AbstractResource): + # API Spec: This is an action and there's no "resource"; RPC-style endpoint? urls = ["/api/report/attack"] @jwt_required diff --git a/monkey/monkey_island/cc/resources/auth/auth.py b/monkey/monkey_island/cc/resources/auth/auth.py index 61aee423e..811f4238b 100644 --- a/monkey/monkey_island/cc/resources/auth/auth.py +++ b/monkey/monkey_island/cc/resources/auth/auth.py @@ -46,4 +46,5 @@ class Authenticate(AbstractResource): except IncorrectCredentialsError: return make_response({"error": "Invalid credentials"}, 401) + # API Spec: Why are we sending "error" here? return make_response({"access_token": access_token, "error": ""}, 200) diff --git a/monkey/monkey_island/cc/resources/auth/registration.py b/monkey/monkey_island/cc/resources/auth/registration.py index 1625de62b..9ef640915 100644 --- a/monkey/monkey_island/cc/resources/auth/registration.py +++ b/monkey/monkey_island/cc/resources/auth/registration.py @@ -23,5 +23,6 @@ class Registration(AbstractResource): try: AuthenticationService.register_new_user(username, password) return make_response({"error": ""}, 200) + # API Spec: HTTP status code for AlreadyRegisteredError should be 409 (CONFLICT) except (InvalidRegistrationCredentialsError, AlreadyRegisteredError) as e: return make_response({"error": str(e)}, 400) diff --git a/monkey/monkey_island/cc/resources/blackbox/clear_caches.py b/monkey/monkey_island/cc/resources/blackbox/clear_caches.py index 116bc0f07..ee86b1275 100644 --- a/monkey/monkey_island/cc/resources/blackbox/clear_caches.py +++ b/monkey/monkey_island/cc/resources/blackbox/clear_caches.py @@ -13,6 +13,7 @@ logger = logging.getLogger(__name__) class ClearCaches(AbstractResource): + # API Spec: Action, not a resource; RPC-style endpoint? urls = ["/api/test/clear-caches"] """ Used for timing tests - we want to get actual execution time of functions in BlackBox without @@ -34,6 +35,8 @@ class ClearCaches(AbstractResource): if ReportService.is_report_generated() or AttackReportService.is_report_generated(): logger.error(NOT_ALL_REPORTS_DELETED) + # API Spec: Why is this 500? 500 should be returned in case an exception occurs on the + # server. 40x makes more sense. flask_restful.abort(500, error_info=NOT_ALL_REPORTS_DELETED) return {"success": "true"} diff --git a/monkey/monkey_island/cc/resources/blackbox/log_blackbox_endpoint.py b/monkey/monkey_island/cc/resources/blackbox/log_blackbox_endpoint.py index a2700cc37..643b7f592 100644 --- a/monkey/monkey_island/cc/resources/blackbox/log_blackbox_endpoint.py +++ b/monkey/monkey_island/cc/resources/blackbox/log_blackbox_endpoint.py @@ -7,6 +7,7 @@ from monkey_island.cc.resources.request_authentication import jwt_required class LogBlackboxEndpoint(AbstractResource): + # API Spec: Rename to noun, BlackboxTestsLogs or something urls = ["/api/test/log"] @jwt_required diff --git a/monkey/monkey_island/cc/resources/blackbox/monkey_blackbox_endpoint.py b/monkey/monkey_island/cc/resources/blackbox/monkey_blackbox_endpoint.py index b02cf224f..4a140f265 100644 --- a/monkey/monkey_island/cc/resources/blackbox/monkey_blackbox_endpoint.py +++ b/monkey/monkey_island/cc/resources/blackbox/monkey_blackbox_endpoint.py @@ -7,6 +7,7 @@ from monkey_island.cc.resources.request_authentication import jwt_required class MonkeyBlackboxEndpoint(AbstractResource): + # API Spec: Rename to noun, BlackboxTestsMonkeys or something urls = ["/api/test/monkey"] @jwt_required diff --git a/monkey/monkey_island/cc/resources/blackbox/telemetry_blackbox_endpoint.py b/monkey/monkey_island/cc/resources/blackbox/telemetry_blackbox_endpoint.py index e3b91cc46..e722ded0e 100644 --- a/monkey/monkey_island/cc/resources/blackbox/telemetry_blackbox_endpoint.py +++ b/monkey/monkey_island/cc/resources/blackbox/telemetry_blackbox_endpoint.py @@ -7,6 +7,7 @@ from monkey_island.cc.resources.request_authentication import jwt_required class TelemetryBlackboxEndpoint(AbstractResource): + # API Spec: Rename to noun, BlackboxTestsTelemetries or something urls = ["/api/test/telemetry"] @jwt_required diff --git a/monkey/monkey_island/cc/resources/configuration_export.py b/monkey/monkey_island/cc/resources/configuration_export.py index e10e5d1a3..972f833f4 100644 --- a/monkey/monkey_island/cc/resources/configuration_export.py +++ b/monkey/monkey_island/cc/resources/configuration_export.py @@ -9,6 +9,7 @@ from monkey_island.cc.services.config import ConfigService class ConfigurationExport(AbstractResource): + # API Spec: This is an action and there's no "resource"; RPC-style endpoint? urls = ["/api/configuration/export"] @jwt_required diff --git a/monkey/monkey_island/cc/resources/configuration_import.py b/monkey/monkey_island/cc/resources/configuration_import.py index ecf94e986..94aa50def 100644 --- a/monkey/monkey_island/cc/resources/configuration_import.py +++ b/monkey/monkey_island/cc/resources/configuration_import.py @@ -39,6 +39,7 @@ class ResponseContents: class ConfigurationImport(AbstractResource): + # API Spec: This is an action and there's no "resource"; RPC-style endpoint? urls = ["/api/configuration/import"] SUCCESS = False @@ -56,11 +57,13 @@ class ConfigurationImport(AbstractResource): config_schema=ConfigService.get_config_schema(), import_status=ImportStatuses.UNSAFE_OPTION_VERIFICATION_REQUIRED, ).form_response() + # API Spec: HTTP status code should be 401 here except InvalidCredentialsError: return ResponseContents( import_status=ImportStatuses.INVALID_CREDENTIALS, message="Invalid credentials provided", ).form_response() + # API Spec: HTTP status code should be 400 (or something else) here except InvalidConfigurationError: return ResponseContents( import_status=ImportStatuses.INVALID_CONFIGURATION, diff --git a/monkey/monkey_island/cc/resources/exploitations/manual_exploitation.py b/monkey/monkey_island/cc/resources/exploitations/manual_exploitation.py index 8006e31af..bbb345483 100644 --- a/monkey/monkey_island/cc/resources/exploitations/manual_exploitation.py +++ b/monkey/monkey_island/cc/resources/exploitations/manual_exploitation.py @@ -6,6 +6,11 @@ from monkey_island.cc.services.reporting.exploitations.manual_exploitation impor class ManualExploitation(AbstractResource): + # API Spec: RPC-style endpoint? Getting manual exploitations is an action and there's no + # "resource", however one could argue that at some point in time, we may want to access + # specific manual exploitation data based on, say, the monkey guid, at a later stage. Leave + # as is in that case, and consider modifying the logic to make this behave more like a + # "resource" and change the name. urls = ["/api/exploitations/manual"] @jwt_required diff --git a/monkey/monkey_island/cc/resources/exploitations/monkey_exploitation.py b/monkey/monkey_island/cc/resources/exploitations/monkey_exploitation.py index 5cbf42a35..be5669c22 100644 --- a/monkey/monkey_island/cc/resources/exploitations/monkey_exploitation.py +++ b/monkey/monkey_island/cc/resources/exploitations/monkey_exploitation.py @@ -6,6 +6,11 @@ from monkey_island.cc.services.reporting.exploitations.monkey_exploitation impor class MonkeyExploitation(AbstractResource): + # API Spec: RPC-style endpoint? Getting manual exploitations is an action and there's no + # "resource", however one could argue that at some point in time, we may want to access + # specific monkey exploitation data based on, say, the monkey guid, at a later stage. Leave + # as is in that case, and consider modifying the logic to make this behave more like a + # "resource" and change the name. urls = ["/api/exploitations/monkey"] @jwt_required diff --git a/monkey/monkey_island/cc/resources/island_configuration.py b/monkey/monkey_island/cc/resources/island_configuration.py index 21da243ed..e75d44e6f 100644 --- a/monkey/monkey_island/cc/resources/island_configuration.py +++ b/monkey/monkey_island/cc/resources/island_configuration.py @@ -21,9 +21,14 @@ class IslandConfiguration(AbstractResource): @jwt_required def post(self): config_json = json.loads(request.data) + # API Spec: Resetting config is an action; separate RPC-style endpoint for this? if "reset" in config_json: ConfigService.reset_config() else: if not ConfigService.update_config(config_json, should_encrypt=True): abort(400) + # API Spec: We're updating the config and then returning the config back? + # RESTfulness of a POST request is to return an identifier of the updated/newly created + # resource. Since there's only one thing we're updating (and not multiple "resources"), + # should this also be an RPC-style endpoint (/api/resetConfig and /api/updateConfig)? return self.get() diff --git a/monkey/monkey_island/cc/resources/island_logs.py b/monkey/monkey_island/cc/resources/island_logs.py index ec36b7a88..24162ba3e 100644 --- a/monkey/monkey_island/cc/resources/island_logs.py +++ b/monkey/monkey_island/cc/resources/island_logs.py @@ -8,6 +8,7 @@ logger = logging.getLogger(__name__) class IslandLog(AbstractResource): + # API Spec: Why the inconsistency in endpoints of IslandLog and Log? urls = ["/api/log/island/download"] @jwt_required diff --git a/monkey/monkey_island/cc/resources/island_mode.py b/monkey/monkey_island/cc/resources/island_mode.py index fed0d0aff..a77829972 100644 --- a/monkey/monkey_island/cc/resources/island_mode.py +++ b/monkey/monkey_island/cc/resources/island_mode.py @@ -13,6 +13,9 @@ logger = logging.getLogger(__name__) class IslandMode(AbstractResource): + # API Spec: Should these be RPC-style endpoints? + # POST is not updating/creating a "resource" but setting a property of the Island. + # Perhaps /api/setMode and /api/getMode would be more appropriate. urls = ["/api/island-mode"] @jwt_required @@ -30,9 +33,13 @@ class IslandMode(AbstractResource): "Using default advanced configuration." ) + # API Spec: RESTful way is to return an identifier of the updated/newly created + # resource but it doesn't make sense here which brings me back to the comment on + # lines 16-18. return make_response({}, 200) except (AttributeError, json.decoder.JSONDecodeError): return make_response({}, 400) + # API Spec: Check if HTTP codes make sense except ValueError: return make_response({}, 422) diff --git a/monkey/monkey_island/cc/resources/local_run.py b/monkey/monkey_island/cc/resources/local_run.py index 7de0b844a..54b0ac0b5 100644 --- a/monkey/monkey_island/cc/resources/local_run.py +++ b/monkey/monkey_island/cc/resources/local_run.py @@ -10,7 +10,8 @@ from monkey_island.cc.services.run_local_monkey import LocalMonkeyRunService class LocalRun(AbstractResource): - + # API Spec: This should be an RPC-style API i.e. two endpoints - + # "/api/getLocalMonkeyRunningStatus" and "/api/startLocalMonkey" urls = ["/api/local-monkey"] @jwt_required @@ -28,7 +29,10 @@ class LocalRun(AbstractResource): body = json.loads(request.data) if body.get("action") == "run": local_run = LocalMonkeyRunService.run_local_monkey() + # API Spec: Feels weird to return "error_text" even when "is_running" is True return jsonify(is_running=local_run[0], error_text=local_run[1]) # default action + # API Spec: Why is this 500? 500 should be returned in case an exception occurs on the + # server. 40x makes more sense. return make_response({"error": "Invalid action"}, 500) diff --git a/monkey/monkey_island/cc/resources/log.py b/monkey/monkey_island/cc/resources/log.py index 541ffb1af..432b27c95 100644 --- a/monkey/monkey_island/cc/resources/log.py +++ b/monkey/monkey_island/cc/resources/log.py @@ -12,6 +12,7 @@ from monkey_island.cc.services.node import NodeService class Log(AbstractResource): + # API Spec: What log? Agent log? urls = ["/api/log"] @jwt_required diff --git a/monkey/monkey_island/cc/resources/monkey.py b/monkey/monkey_island/cc/resources/monkey.py index 21ffbed36..811ccd0ee 100644 --- a/monkey/monkey_island/cc/resources/monkey.py +++ b/monkey/monkey_island/cc/resources/monkey.py @@ -17,9 +17,11 @@ from monkey_island.cc.services.node import NodeService class Monkey(AbstractResource): + # API Spec: Resource name should be plural urls = [ "/api/agent", "/api/agent/", + # API Spec: Resource names should alternate with IDs (/api/agents/123/config-format/xyz) "/api/agent//", ] @@ -64,6 +66,7 @@ class Monkey(AbstractResource): ttl = create_monkey_ttl_document(DEFAULT_MONKEY_TTL_EXPIRY_DURATION_IN_SECONDS) update["$set"]["ttl_ref"] = ttl.id + # API Spec: What is this returning? Check that it follows rules. return mongo.db.monkey.update({"_id": monkey["_id"]}, update, upsert=False) # Used by monkey. can't secure. diff --git a/monkey/monkey_island/cc/resources/monkey_download.py b/monkey/monkey_island/cc/resources/monkey_download.py index 9c19b70dc..775252c22 100644 --- a/monkey/monkey_island/cc/resources/monkey_download.py +++ b/monkey/monkey_island/cc/resources/monkey_download.py @@ -20,6 +20,7 @@ class UnsupportedOSError(Exception): class MonkeyDownload(AbstractResource): + # API Spec: This is an action and there's no "resource"; RPC-style endpoint? urls = ["/api/agent/download/"] # Used by monkey. can't secure. diff --git a/monkey/monkey_island/cc/resources/pba_file_download.py b/monkey/monkey_island/cc/resources/pba_file_download.py index 73658d0ad..4415f90eb 100644 --- a/monkey/monkey_island/cc/resources/pba_file_download.py +++ b/monkey/monkey_island/cc/resources/pba_file_download.py @@ -9,6 +9,7 @@ logger = logging.getLogger(__file__) class PBAFileDownload(AbstractResource): + # API Spec: This is an action and there's no "resource"; RPC-style endpoint? urls = ["/api/pba/download/"] """ File download endpoint used by monkey to download user's PBA file diff --git a/monkey/monkey_island/cc/resources/pba_file_upload.py b/monkey/monkey_island/cc/resources/pba_file_upload.py index 9f741a039..45b72727e 100644 --- a/monkey/monkey_island/cc/resources/pba_file_upload.py +++ b/monkey/monkey_island/cc/resources/pba_file_upload.py @@ -18,6 +18,8 @@ WINDOWS_PBA_TYPE = "PBAwindows" class FileUpload(AbstractResource): + # API Spec: FileUpload -> CustomPBAFile or something to make it more RESTful. Change endpoint + # accordingly. """ File upload endpoint used to send/receive Custom PBA files """ @@ -80,6 +82,7 @@ class FileUpload(AbstractResource): safe_filename, ) + # API Spec: HTTP status code should be 201 response = Response(response=safe_filename, status=200, mimetype="text/plain") return response @@ -101,6 +104,7 @@ class FileUpload(AbstractResource): self._file_storage_service.delete_file(filename) ConfigService.set_config_value(filename_path, "") + # API Spec: HTTP status code should be 204 return make_response({}, 200) @staticmethod diff --git a/monkey/monkey_island/cc/resources/ransomware_report.py b/monkey/monkey_island/cc/resources/ransomware_report.py index d229361ea..e8661640c 100644 --- a/monkey/monkey_island/cc/resources/ransomware_report.py +++ b/monkey/monkey_island/cc/resources/ransomware_report.py @@ -6,6 +6,7 @@ from monkey_island.cc.services.ransomware import ransomware_report class RansomwareReport(AbstractResource): + # API Spec: This is an action and there's no "resource"; RPC-style endpoint? urls = ["/api/report/ransomware"] @jwt_required diff --git a/monkey/monkey_island/cc/resources/remote_run.py b/monkey/monkey_island/cc/resources/remote_run.py index 820419487..74a6f8abc 100644 --- a/monkey/monkey_island/cc/resources/remote_run.py +++ b/monkey/monkey_island/cc/resources/remote_run.py @@ -20,6 +20,9 @@ NO_CREDS_ERROR_FORMAT = ( class RemoteRun(AbstractResource): + # API Spec: POST request is an action, it's not updating/creating any resource. + # GET makes sense. The resource should be split up since these two use cases don't + # really go together. urls = ["/api/remote-monkey"] def __init__(self, aws_service: AWSService): @@ -35,9 +38,11 @@ class RemoteRun(AbstractResource): try: resp["instances"] = self._aws_service.get_managed_instances() except NoCredentialsError as e: + # API Spec: HTTP status code should be 401 resp["error"] = NO_CREDS_ERROR_FORMAT.format(e) return jsonify(resp) except ClientError as e: + # API Spec: HTTP status code should not be 200 resp["error"] = CLIENT_ERROR_FORMAT.format(e) return jsonify(resp) return jsonify(resp) @@ -49,9 +54,13 @@ class RemoteRun(AbstractResource): body = json.loads(request.data) if body.get("type") == "aws": results = self.run_aws_monkeys(body) + # API Spec: POST should return identifier or updated/newly created resource, not some + # kind of data. That's more of a GET thing. return RemoteRun._encode_results(results) # default action + # API Spec: Why is this 500? 500 should be returned in case an exception occurs on the + # server. 40x makes more sense. return make_response({"error": "Invalid action"}, 500) def run_aws_monkeys(self, request_body) -> Sequence[AWSCommandResults]: diff --git a/monkey/monkey_island/cc/resources/security_report.py b/monkey/monkey_island/cc/resources/security_report.py index 62b254931..7e1784dd2 100644 --- a/monkey/monkey_island/cc/resources/security_report.py +++ b/monkey/monkey_island/cc/resources/security_report.py @@ -4,6 +4,7 @@ from monkey_island.cc.services.reporting.report import ReportService class SecurityReport(AbstractResource): + # API Spec: This is an action and there's no "resource"; RPC-style endpoint? urls = ["/api/report/security"] @jwt_required diff --git a/monkey/monkey_island/cc/resources/telemetry.py b/monkey/monkey_island/cc/resources/telemetry.py index 11e4ba06e..1d16c3ad9 100644 --- a/monkey/monkey_island/cc/resources/telemetry.py +++ b/monkey/monkey_island/cc/resources/telemetry.py @@ -18,6 +18,7 @@ logger = logging.getLogger(__name__) class Telemetry(AbstractResource): + # API Spec: Resource name should be plural urls = ["/api/telemetry", "/api/telemetry/"] @jwt_required @@ -62,6 +63,7 @@ class Telemetry(AbstractResource): process_telemetry(telemetry_json) + # API Spec: RESTful way is to return an identifier of the updated/newly created resource return {}, 201 @staticmethod diff --git a/monkey/monkey_island/cc/resources/telemetry_feed.py b/monkey/monkey_island/cc/resources/telemetry_feed.py index fa97f3257..9087e2b97 100644 --- a/monkey/monkey_island/cc/resources/telemetry_feed.py +++ b/monkey/monkey_island/cc/resources/telemetry_feed.py @@ -39,6 +39,7 @@ class TelemetryFeed(AbstractResource): } except KeyError as err: logger.error("Failed parsing telemetries. Error: {0}.".format(err)) + # API Spec: Should return HTTP status code 404 (?) return {"telemetries": [], "timestamp": datetime.now().isoformat()} @staticmethod diff --git a/monkey/monkey_island/cc/resources/zero_trust/finding_event.py b/monkey/monkey_island/cc/resources/zero_trust/finding_event.py index d96e741f9..0b63935b6 100644 --- a/monkey/monkey_island/cc/resources/zero_trust/finding_event.py +++ b/monkey/monkey_island/cc/resources/zero_trust/finding_event.py @@ -8,6 +8,8 @@ from monkey_island.cc.services.zero_trust.monkey_findings.monkey_zt_finding_serv class ZeroTrustFindingEvent(AbstractResource): + # API Spec: Why is the endpoint separated? Why not just + # "/api/zero-trust-finding-event/"? urls = ["/api/zero-trust/finding-event/"] @jwt_required diff --git a/monkey/monkey_island/cc/resources/zero_trust/zero_trust_report.py b/monkey/monkey_island/cc/resources/zero_trust/zero_trust_report.py index 359328114..2f0305150 100644 --- a/monkey/monkey_island/cc/resources/zero_trust/zero_trust_report.py +++ b/monkey/monkey_island/cc/resources/zero_trust/zero_trust_report.py @@ -17,6 +17,7 @@ REPORT_DATA_PRINCIPLES_STATUS = "principles" class ZeroTrustReport(AbstractResource): + # API Spec: This is an action and there's no "resource"; RPC-style endpoint? urls = ["/api/report/zero-trust/"] @jwt_required From ad7d726b524c57ffb048ff93c6f8e6befcae8db9 Mon Sep 17 00:00:00 2001 From: vakarisz Date: Wed, 25 May 2022 12:35:53 +0300 Subject: [PATCH 2/7] Island: Add more comments about URL structure refactoring --- monkey/monkey_island/cc/app.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/monkey/monkey_island/cc/app.py b/monkey/monkey_island/cc/app.py index da027bf62..015c2e4a0 100644 --- a/monkey/monkey_island/cc/app.py +++ b/monkey/monkey_island/cc/app.py @@ -155,6 +155,8 @@ def init_api_resources(api: FlaskDIWrapper): api.add_resource(IslandConfiguration) api.add_resource(ConfigurationExport) api.add_resource(ConfigurationImport) + # Rename to /api/agent-binary, because information about agent runs + # and binary files are different resources api.add_resource(MonkeyDownload) api.add_resource(NetMap) api.add_resource(Edge) @@ -173,12 +175,16 @@ def init_api_resources(api: FlaskDIWrapper): api.add_resource(Log) api.add_resource(IslandLog) + # These two should have the same url syntax api.add_resource(PBAFileDownload) api.add_resource(FileUpload) api.add_resource(PropagationCredentials) + # API Spec: Should use RPC convention api.add_resource(RemoteRun) + # Rename to /version-info api.add_resource(VersionUpdate) + # API Spec: Fix endpoint (see comment in StopAgentCheck) api.add_resource(StopAgentCheck) api.add_resource(StopAllAgents) From dfe93feb0b84a4115f88879bea12298442b64b93 Mon Sep 17 00:00:00 2001 From: Shreya Malviya Date: Thu, 26 May 2022 14:02:42 +0530 Subject: [PATCH 3/7] Island: Update API spec comments after discussion --- monkey/monkey_island/cc/app.py | 6 +++--- .../cc/resources/agent_controls/stop_agent_check.py | 3 +-- monkey/monkey_island/cc/resources/attack/attack_report.py | 1 - monkey/monkey_island/cc/resources/configuration_export.py | 1 - monkey/monkey_island/cc/resources/configuration_import.py | 2 +- .../cc/resources/exploitations/manual_exploitation.py | 5 ----- .../cc/resources/exploitations/monkey_exploitation.py | 5 ----- monkey/monkey_island/cc/resources/island_configuration.py | 4 +++- monkey/monkey_island/cc/resources/island_mode.py | 7 +------ monkey/monkey_island/cc/resources/local_run.py | 6 ++++-- monkey/monkey_island/cc/resources/monkey_download.py | 1 - monkey/monkey_island/cc/resources/pba_file_download.py | 1 - monkey/monkey_island/cc/resources/pba_file_upload.py | 3 +-- monkey/monkey_island/cc/resources/ransomware_report.py | 1 - monkey/monkey_island/cc/resources/remote_run.py | 2 +- monkey/monkey_island/cc/resources/security_report.py | 1 - .../cc/resources/zero_trust/zero_trust_report.py | 1 - 17 files changed, 15 insertions(+), 35 deletions(-) diff --git a/monkey/monkey_island/cc/app.py b/monkey/monkey_island/cc/app.py index 015c2e4a0..576f25890 100644 --- a/monkey/monkey_island/cc/app.py +++ b/monkey/monkey_island/cc/app.py @@ -155,7 +155,7 @@ def init_api_resources(api: FlaskDIWrapper): api.add_resource(IslandConfiguration) api.add_resource(ConfigurationExport) api.add_resource(ConfigurationImport) - # Rename to /api/agent-binary, because information about agent runs + # API Spec: Rename to /api/agent-binary, because information about agent runs # and binary files are different resources api.add_resource(MonkeyDownload) api.add_resource(NetMap) @@ -175,14 +175,14 @@ def init_api_resources(api: FlaskDIWrapper): api.add_resource(Log) api.add_resource(IslandLog) - # These two should have the same url syntax + # API Spec: These two should have the same url syntax api.add_resource(PBAFileDownload) api.add_resource(FileUpload) api.add_resource(PropagationCredentials) # API Spec: Should use RPC convention api.add_resource(RemoteRun) - # Rename to /version-info + # API Spec: Rename to /version-info api.add_resource(VersionUpdate) # API Spec: Fix endpoint (see comment in StopAgentCheck) api.add_resource(StopAgentCheck) diff --git a/monkey/monkey_island/cc/resources/agent_controls/stop_agent_check.py b/monkey/monkey_island/cc/resources/agent_controls/stop_agent_check.py index b5585dce3..fd1fc213a 100644 --- a/monkey/monkey_island/cc/resources/agent_controls/stop_agent_check.py +++ b/monkey/monkey_island/cc/resources/agent_controls/stop_agent_check.py @@ -2,8 +2,7 @@ from monkey_island.cc.resources.AbstractResource import AbstractResource from monkey_island.cc.services.infection_lifecycle import should_agent_die class StopAgentCheck(AbstractResource): - # API Spec: Not a resource, checking is an action; RPC-style endpoint? - # REST feels okay too but probably rename it to AgentStopStatus or something. + # API Spec: Rename to AgentStopStatus or something urls = ["/api/monkey-control/needs-to-stop/"] def get(self, monkey_guid: int): diff --git a/monkey/monkey_island/cc/resources/attack/attack_report.py b/monkey/monkey_island/cc/resources/attack/attack_report.py index ef5fdfa5d..2dffa929d 100644 --- a/monkey/monkey_island/cc/resources/attack/attack_report.py +++ b/monkey/monkey_island/cc/resources/attack/attack_report.py @@ -7,7 +7,6 @@ from monkey_island.cc.services.attack.attack_schema import SCHEMA class AttackReport(AbstractResource): - # API Spec: This is an action and there's no "resource"; RPC-style endpoint? urls = ["/api/report/attack"] @jwt_required diff --git a/monkey/monkey_island/cc/resources/configuration_export.py b/monkey/monkey_island/cc/resources/configuration_export.py index 972f833f4..e10e5d1a3 100644 --- a/monkey/monkey_island/cc/resources/configuration_export.py +++ b/monkey/monkey_island/cc/resources/configuration_export.py @@ -9,7 +9,6 @@ from monkey_island.cc.services.config import ConfigService class ConfigurationExport(AbstractResource): - # API Spec: This is an action and there's no "resource"; RPC-style endpoint? urls = ["/api/configuration/export"] @jwt_required diff --git a/monkey/monkey_island/cc/resources/configuration_import.py b/monkey/monkey_island/cc/resources/configuration_import.py index 94aa50def..15ab3e41f 100644 --- a/monkey/monkey_island/cc/resources/configuration_import.py +++ b/monkey/monkey_island/cc/resources/configuration_import.py @@ -39,7 +39,7 @@ class ResponseContents: class ConfigurationImport(AbstractResource): - # API Spec: This is an action and there's no "resource"; RPC-style endpoint? + # API Spec: Should probably be merged with IslandConfiguration urls = ["/api/configuration/import"] SUCCESS = False diff --git a/monkey/monkey_island/cc/resources/exploitations/manual_exploitation.py b/monkey/monkey_island/cc/resources/exploitations/manual_exploitation.py index bbb345483..8006e31af 100644 --- a/monkey/monkey_island/cc/resources/exploitations/manual_exploitation.py +++ b/monkey/monkey_island/cc/resources/exploitations/manual_exploitation.py @@ -6,11 +6,6 @@ from monkey_island.cc.services.reporting.exploitations.manual_exploitation impor class ManualExploitation(AbstractResource): - # API Spec: RPC-style endpoint? Getting manual exploitations is an action and there's no - # "resource", however one could argue that at some point in time, we may want to access - # specific manual exploitation data based on, say, the monkey guid, at a later stage. Leave - # as is in that case, and consider modifying the logic to make this behave more like a - # "resource" and change the name. urls = ["/api/exploitations/manual"] @jwt_required diff --git a/monkey/monkey_island/cc/resources/exploitations/monkey_exploitation.py b/monkey/monkey_island/cc/resources/exploitations/monkey_exploitation.py index be5669c22..5cbf42a35 100644 --- a/monkey/monkey_island/cc/resources/exploitations/monkey_exploitation.py +++ b/monkey/monkey_island/cc/resources/exploitations/monkey_exploitation.py @@ -6,11 +6,6 @@ from monkey_island.cc.services.reporting.exploitations.monkey_exploitation impor class MonkeyExploitation(AbstractResource): - # API Spec: RPC-style endpoint? Getting manual exploitations is an action and there's no - # "resource", however one could argue that at some point in time, we may want to access - # specific monkey exploitation data based on, say, the monkey guid, at a later stage. Leave - # as is in that case, and consider modifying the logic to make this behave more like a - # "resource" and change the name. urls = ["/api/exploitations/monkey"] @jwt_required diff --git a/monkey/monkey_island/cc/resources/island_configuration.py b/monkey/monkey_island/cc/resources/island_configuration.py index e75d44e6f..1360d0c91 100644 --- a/monkey/monkey_island/cc/resources/island_configuration.py +++ b/monkey/monkey_island/cc/resources/island_configuration.py @@ -21,7 +21,8 @@ class IslandConfiguration(AbstractResource): @jwt_required def post(self): config_json = json.loads(request.data) - # API Spec: Resetting config is an action; separate RPC-style endpoint for this? + # API Spec: Makes more sense to have a PATCH request for this since the resource, + # i.e. the configuration, is being updated. if "reset" in config_json: ConfigService.reset_config() else: @@ -31,4 +32,5 @@ class IslandConfiguration(AbstractResource): # RESTfulness of a POST request is to return an identifier of the updated/newly created # resource. Since there's only one thing we're updating (and not multiple "resources"), # should this also be an RPC-style endpoint (/api/resetConfig and /api/updateConfig)? + # Simplest way it to just send a GET request after this to get the updated config. return self.get() diff --git a/monkey/monkey_island/cc/resources/island_mode.py b/monkey/monkey_island/cc/resources/island_mode.py index a77829972..6978f3b14 100644 --- a/monkey/monkey_island/cc/resources/island_mode.py +++ b/monkey/monkey_island/cc/resources/island_mode.py @@ -13,9 +13,7 @@ logger = logging.getLogger(__name__) class IslandMode(AbstractResource): - # API Spec: Should these be RPC-style endpoints? - # POST is not updating/creating a "resource" but setting a property of the Island. - # Perhaps /api/setMode and /api/getMode would be more appropriate. + # API Spec: Instead of POST, this could just be PATCH urls = ["/api/island-mode"] @jwt_required @@ -33,9 +31,6 @@ class IslandMode(AbstractResource): "Using default advanced configuration." ) - # API Spec: RESTful way is to return an identifier of the updated/newly created - # resource but it doesn't make sense here which brings me back to the comment on - # lines 16-18. return make_response({}, 200) except (AttributeError, json.decoder.JSONDecodeError): return make_response({}, 400) diff --git a/monkey/monkey_island/cc/resources/local_run.py b/monkey/monkey_island/cc/resources/local_run.py index 54b0ac0b5..4e918e6c7 100644 --- a/monkey/monkey_island/cc/resources/local_run.py +++ b/monkey/monkey_island/cc/resources/local_run.py @@ -10,10 +10,11 @@ from monkey_island.cc.services.run_local_monkey import LocalMonkeyRunService class LocalRun(AbstractResource): - # API Spec: This should be an RPC-style API i.e. two endpoints - - # "/api/getLocalMonkeyRunningStatus" and "/api/startLocalMonkey" urls = ["/api/local-monkey"] + # API Spec: Both of these methods should be separated to their own resources + + # API Spec: This should be a REST endpoint, /api/monkeys or something @jwt_required def get(self): island_monkey = NodeService.get_monkey_island_monkey() @@ -24,6 +25,7 @@ class LocalRun(AbstractResource): return jsonify(is_running=is_monkey_running) + # API Spec: This should be an RPC-style endpoint @jwt_required def post(self): body = json.loads(request.data) diff --git a/monkey/monkey_island/cc/resources/monkey_download.py b/monkey/monkey_island/cc/resources/monkey_download.py index 775252c22..9c19b70dc 100644 --- a/monkey/monkey_island/cc/resources/monkey_download.py +++ b/monkey/monkey_island/cc/resources/monkey_download.py @@ -20,7 +20,6 @@ class UnsupportedOSError(Exception): class MonkeyDownload(AbstractResource): - # API Spec: This is an action and there's no "resource"; RPC-style endpoint? urls = ["/api/agent/download/"] # Used by monkey. can't secure. diff --git a/monkey/monkey_island/cc/resources/pba_file_download.py b/monkey/monkey_island/cc/resources/pba_file_download.py index 4415f90eb..73658d0ad 100644 --- a/monkey/monkey_island/cc/resources/pba_file_download.py +++ b/monkey/monkey_island/cc/resources/pba_file_download.py @@ -9,7 +9,6 @@ logger = logging.getLogger(__file__) class PBAFileDownload(AbstractResource): - # API Spec: This is an action and there's no "resource"; RPC-style endpoint? urls = ["/api/pba/download/"] """ File download endpoint used by monkey to download user's PBA file diff --git a/monkey/monkey_island/cc/resources/pba_file_upload.py b/monkey/monkey_island/cc/resources/pba_file_upload.py index 45b72727e..a4ce2705a 100644 --- a/monkey/monkey_island/cc/resources/pba_file_upload.py +++ b/monkey/monkey_island/cc/resources/pba_file_upload.py @@ -18,8 +18,7 @@ WINDOWS_PBA_TYPE = "PBAwindows" class FileUpload(AbstractResource): - # API Spec: FileUpload -> CustomPBAFile or something to make it more RESTful. Change endpoint - # accordingly. + # API Spec: FileUpload -> PBAFileUpload. Change endpoint accordingly. """ File upload endpoint used to send/receive Custom PBA files """ diff --git a/monkey/monkey_island/cc/resources/ransomware_report.py b/monkey/monkey_island/cc/resources/ransomware_report.py index e8661640c..d229361ea 100644 --- a/monkey/monkey_island/cc/resources/ransomware_report.py +++ b/monkey/monkey_island/cc/resources/ransomware_report.py @@ -6,7 +6,6 @@ from monkey_island.cc.services.ransomware import ransomware_report class RansomwareReport(AbstractResource): - # API Spec: This is an action and there's no "resource"; RPC-style endpoint? urls = ["/api/report/ransomware"] @jwt_required diff --git a/monkey/monkey_island/cc/resources/remote_run.py b/monkey/monkey_island/cc/resources/remote_run.py index 74a6f8abc..c9b7a5d87 100644 --- a/monkey/monkey_island/cc/resources/remote_run.py +++ b/monkey/monkey_island/cc/resources/remote_run.py @@ -55,7 +55,7 @@ class RemoteRun(AbstractResource): if body.get("type") == "aws": results = self.run_aws_monkeys(body) # API Spec: POST should return identifier or updated/newly created resource, not some - # kind of data. That's more of a GET thing. + # kind of data. That's a GET thing. return RemoteRun._encode_results(results) # default action diff --git a/monkey/monkey_island/cc/resources/security_report.py b/monkey/monkey_island/cc/resources/security_report.py index 7e1784dd2..62b254931 100644 --- a/monkey/monkey_island/cc/resources/security_report.py +++ b/monkey/monkey_island/cc/resources/security_report.py @@ -4,7 +4,6 @@ from monkey_island.cc.services.reporting.report import ReportService class SecurityReport(AbstractResource): - # API Spec: This is an action and there's no "resource"; RPC-style endpoint? urls = ["/api/report/security"] @jwt_required diff --git a/monkey/monkey_island/cc/resources/zero_trust/zero_trust_report.py b/monkey/monkey_island/cc/resources/zero_trust/zero_trust_report.py index 2f0305150..359328114 100644 --- a/monkey/monkey_island/cc/resources/zero_trust/zero_trust_report.py +++ b/monkey/monkey_island/cc/resources/zero_trust/zero_trust_report.py @@ -17,7 +17,6 @@ REPORT_DATA_PRINCIPLES_STATUS = "principles" class ZeroTrustReport(AbstractResource): - # API Spec: This is an action and there's no "resource"; RPC-style endpoint? urls = ["/api/report/zero-trust/"] @jwt_required From e9f2c9b411cb246bf1e1769a30593c616d3493b1 Mon Sep 17 00:00:00 2001 From: Shreya Malviya Date: Fri, 27 May 2022 18:53:25 +0530 Subject: [PATCH 4/7] Island: Remove comment about 500 HTTP status code in clear_caches.py --- monkey/monkey_island/cc/resources/blackbox/clear_caches.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/monkey/monkey_island/cc/resources/blackbox/clear_caches.py b/monkey/monkey_island/cc/resources/blackbox/clear_caches.py index ee86b1275..af650655b 100644 --- a/monkey/monkey_island/cc/resources/blackbox/clear_caches.py +++ b/monkey/monkey_island/cc/resources/blackbox/clear_caches.py @@ -35,8 +35,6 @@ class ClearCaches(AbstractResource): if ReportService.is_report_generated() or AttackReportService.is_report_generated(): logger.error(NOT_ALL_REPORTS_DELETED) - # API Spec: Why is this 500? 500 should be returned in case an exception occurs on the - # server. 40x makes more sense. flask_restful.abort(500, error_info=NOT_ALL_REPORTS_DELETED) return {"success": "true"} From 8845588ec97944896b285492b3b0e8170a7d030f Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Fri, 27 May 2022 09:40:40 -0400 Subject: [PATCH 5/7] Island: Add a note about removing blackbox endpoints --- monkey/monkey_island/cc/app.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/monkey/monkey_island/cc/app.py b/monkey/monkey_island/cc/app.py index 576f25890..2df4b7b1c 100644 --- a/monkey/monkey_island/cc/app.py +++ b/monkey/monkey_island/cc/app.py @@ -190,6 +190,8 @@ def init_api_resources(api: FlaskDIWrapper): # Resources used by black box tests # API Spec: Fix all the following endpoints, see comments in the resource classes + # Note: Preferably, the API will provide a rich feature set and allow access to all of the + # necessary data. This would make these endpoints obsolete. api.add_resource(MonkeyBlackboxEndpoint) api.add_resource(ClearCaches) api.add_resource(LogBlackboxEndpoint) From c54b3da86d95d82f78a1c15920563c6569dfe1d3 Mon Sep 17 00:00:00 2001 From: Shreya Malviya Date: Fri, 27 May 2022 23:09:03 +0530 Subject: [PATCH 6/7] Island: Minor changes to API spec comments as per CR --- monkey/monkey_island/cc/app.py | 4 ++-- .../cc/resources/agent_controls/stop_agent_check.py | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/monkey/monkey_island/cc/app.py b/monkey/monkey_island/cc/app.py index 2df4b7b1c..a9305cfc9 100644 --- a/monkey/monkey_island/cc/app.py +++ b/monkey/monkey_island/cc/app.py @@ -155,7 +155,7 @@ def init_api_resources(api: FlaskDIWrapper): api.add_resource(IslandConfiguration) api.add_resource(ConfigurationExport) api.add_resource(ConfigurationImport) - # API Spec: Rename to /api/agent-binary, because information about agent runs + # API Spec: Rename to /api/agent-binaries, because information about agent runs # and binary files are different resources api.add_resource(MonkeyDownload) api.add_resource(NetMap) @@ -175,7 +175,7 @@ def init_api_resources(api: FlaskDIWrapper): api.add_resource(Log) api.add_resource(IslandLog) - # API Spec: These two should have the same url syntax + # API Spec: These two should be the same resource, GET for download and POST for upload api.add_resource(PBAFileDownload) api.add_resource(FileUpload) diff --git a/monkey/monkey_island/cc/resources/agent_controls/stop_agent_check.py b/monkey/monkey_island/cc/resources/agent_controls/stop_agent_check.py index fd1fc213a..18195e1ce 100644 --- a/monkey/monkey_island/cc/resources/agent_controls/stop_agent_check.py +++ b/monkey/monkey_island/cc/resources/agent_controls/stop_agent_check.py @@ -1,8 +1,10 @@ from monkey_island.cc.resources.AbstractResource import AbstractResource from monkey_island.cc.services.infection_lifecycle import should_agent_die + class StopAgentCheck(AbstractResource): - # API Spec: Rename to AgentStopStatus or something + # API Spec: Rename to AgentStopStatus or something, endpoint for this could be + # "/api/agents//stop-status" urls = ["/api/monkey-control/needs-to-stop/"] def get(self, monkey_guid: int): From 8b1a82814eaeefd807a0515c9f095a6464cea0d6 Mon Sep 17 00:00:00 2001 From: Shreya Malviya Date: Fri, 27 May 2022 23:59:20 +0530 Subject: [PATCH 7/7] Island: Update API spec comments after rebase --- monkey/monkey_island/cc/app.py | 5 ----- monkey/monkey_island/cc/resources/monkey_download.py | 2 ++ monkey/monkey_island/cc/resources/version_update.py | 1 + 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/monkey/monkey_island/cc/app.py b/monkey/monkey_island/cc/app.py index a9305cfc9..138e73625 100644 --- a/monkey/monkey_island/cc/app.py +++ b/monkey/monkey_island/cc/app.py @@ -155,8 +155,6 @@ def init_api_resources(api: FlaskDIWrapper): api.add_resource(IslandConfiguration) api.add_resource(ConfigurationExport) api.add_resource(ConfigurationImport) - # API Spec: Rename to /api/agent-binaries, because information about agent runs - # and binary files are different resources api.add_resource(MonkeyDownload) api.add_resource(NetMap) api.add_resource(Edge) @@ -180,11 +178,8 @@ def init_api_resources(api: FlaskDIWrapper): api.add_resource(FileUpload) api.add_resource(PropagationCredentials) - # API Spec: Should use RPC convention api.add_resource(RemoteRun) - # API Spec: Rename to /version-info api.add_resource(VersionUpdate) - # API Spec: Fix endpoint (see comment in StopAgentCheck) api.add_resource(StopAgentCheck) api.add_resource(StopAllAgents) diff --git a/monkey/monkey_island/cc/resources/monkey_download.py b/monkey/monkey_island/cc/resources/monkey_download.py index 9c19b70dc..3ecceaf5c 100644 --- a/monkey/monkey_island/cc/resources/monkey_download.py +++ b/monkey/monkey_island/cc/resources/monkey_download.py @@ -20,6 +20,8 @@ class UnsupportedOSError(Exception): class MonkeyDownload(AbstractResource): + # API Spec: Rename to /api/agent-binaries, because information about agent runs + # and binary files are different resources urls = ["/api/agent/download/"] # Used by monkey. can't secure. diff --git a/monkey/monkey_island/cc/resources/version_update.py b/monkey/monkey_island/cc/resources/version_update.py index 0544cea4c..a2cd6abf6 100644 --- a/monkey/monkey_island/cc/resources/version_update.py +++ b/monkey/monkey_island/cc/resources/version_update.py @@ -8,6 +8,7 @@ logger = logging.getLogger(__name__) class VersionUpdate(AbstractResource): + # API Spec: Rename to /version-info urls = ["/api/version-update"] def __init__(self):