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