diff --git a/monkey/monkey_island/cc/app.py b/monkey/monkey_island/cc/app.py index 60d6f9009..138e73625 100644 --- a/monkey/monkey_island/cc/app.py +++ b/monkey/monkey_island/cc/app.py @@ -173,6 +173,7 @@ def init_api_resources(api: FlaskDIWrapper): api.add_resource(Log) api.add_resource(IslandLog) + # API Spec: These two should be the same resource, GET for download and POST for upload api.add_resource(PBAFileDownload) api.add_resource(FileUpload) @@ -183,6 +184,9 @@ 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 + # 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) 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..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 @@ -3,6 +3,8 @@ from monkey_island.cc.services.infection_lifecycle import should_agent_die class StopAgentCheck(AbstractResource): + # 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): 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/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..af650655b 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 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_import.py b/monkey/monkey_island/cc/resources/configuration_import.py index ecf94e986..15ab3e41f 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: Should probably be merged with IslandConfiguration 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/island_configuration.py b/monkey/monkey_island/cc/resources/island_configuration.py index 21da243ed..1360d0c91 100644 --- a/monkey/monkey_island/cc/resources/island_configuration.py +++ b/monkey/monkey_island/cc/resources/island_configuration.py @@ -21,9 +21,16 @@ class IslandConfiguration(AbstractResource): @jwt_required def post(self): config_json = json.loads(request.data) + # 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: 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)? + # 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_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..6978f3b14 100644 --- a/monkey/monkey_island/cc/resources/island_mode.py +++ b/monkey/monkey_island/cc/resources/island_mode.py @@ -13,6 +13,7 @@ logger = logging.getLogger(__name__) class IslandMode(AbstractResource): + # API Spec: Instead of POST, this could just be PATCH urls = ["/api/island-mode"] @jwt_required @@ -33,6 +34,7 @@ class IslandMode(AbstractResource): 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..4e918e6c7 100644 --- a/monkey/monkey_island/cc/resources/local_run.py +++ b/monkey/monkey_island/cc/resources/local_run.py @@ -10,9 +10,11 @@ from monkey_island.cc.services.run_local_monkey import LocalMonkeyRunService class LocalRun(AbstractResource): - 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() @@ -23,12 +25,16 @@ 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) 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..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/pba_file_upload.py b/monkey/monkey_island/cc/resources/pba_file_upload.py index 9f741a039..a4ce2705a 100644 --- a/monkey/monkey_island/cc/resources/pba_file_upload.py +++ b/monkey/monkey_island/cc/resources/pba_file_upload.py @@ -18,6 +18,7 @@ WINDOWS_PBA_TYPE = "PBAwindows" class FileUpload(AbstractResource): + # API Spec: FileUpload -> PBAFileUpload. Change endpoint accordingly. """ File upload endpoint used to send/receive Custom PBA files """ @@ -80,6 +81,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 +103,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/remote_run.py b/monkey/monkey_island/cc/resources/remote_run.py index 820419487..c9b7a5d87 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 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/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/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): 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