diff --git a/keepercommander/commands/pam/gateway_helper.py b/keepercommander/commands/pam/gateway_helper.py index aafed1c6e..d2d206ad9 100644 --- a/keepercommander/commands/pam/gateway_helper.py +++ b/keepercommander/commands/pam/gateway_helper.py @@ -93,6 +93,8 @@ def create_gateway(params, gateway_name, ksm_app, config_init, ott_expire_in_min one_time_token = config_str_and_config_dict.get('config_str') + # New controller may not appear in a warm get_all_gateways() cache. + invalidate_gateway_cache() return one_time_token diff --git a/keepercommander/service/README.md b/keepercommander/service/README.md index 670213241..9d89a2396 100644 --- a/keepercommander/service/README.md +++ b/keepercommander/service/README.md @@ -216,6 +216,7 @@ result_retention: 3600 # Result retention (1 hour) - **Default limits**: 60/minute, 600/hour, 6000/day - **Per-endpoint tracking**: Each API endpoint has independent rate limit counters - **Example**: Setting `"20/minute"` provides 20 requests per minute per endpoint per IP address +- Service rate limits are local (per client IP/endpoint). They are not the same as Keeper’s per-user API throttle; one service HTTP call may trigger many Keeper API calls. #### Error Responses @@ -224,7 +225,10 @@ result_retention: 3600 # Result retention (1 hour) - **401 Unauthorized**: Missing, invalid, or expired API key; no active session - **403 Forbidden**: IP not allowed, access denied, or command not in allowed list - **404 Not Found**: Request ID not found -- **429 Too Many Requests**: Rate limit exceeded +- **429 Too Many Requests**: JSON `{"status":"error","error":"","result_code":"..."}` + - `rate_limited` — service-local Flask limiter + - `throttled` — upstream Keeper API throttle (same as Commander) + - `429` — upstream edge/gateway “Too Many Requests” **Server Errors (5xx):** - **500 Internal Server Error**: Command execution failed or unexpected server error diff --git a/keepercommander/service/app.py b/keepercommander/service/app.py index 774760b6a..ea2b741a0 100644 --- a/keepercommander/service/app.py +++ b/keepercommander/service/app.py @@ -9,13 +9,15 @@ # Contact: ops@keepersecurity.com # -from flask import Flask +from flask import Flask, jsonify import logging from werkzeug.middleware.proxy_fix import ProxyFix +from flask_limiter.errors import RateLimitExceeded from .decorators.security import limiter, is_behind_proxy from .decorators.api_logging import SSLHandshakeFilter from .api.routes import init_routes from .decorators.logging import logger +from .util.throttle import rate_limited_response def create_app(): @@ -35,6 +37,12 @@ def create_app(): logger.debug("Configuring rate limiter") limiter.init_app(app) + @app.errorhandler(RateLimitExceeded) + def handle_rate_limit_exceeded(e): + detail = getattr(e, 'description', None) or str(e) + body, status = rate_limited_response(detail) + return jsonify(body), status + logger.debug("Initializing API routes") init_routes(app) diff --git a/keepercommander/service/util/command_util.py b/keepercommander/service/util/command_util.py index 35434b697..d49839bb7 100644 --- a/keepercommander/service/util/command_util.py +++ b/keepercommander/service/util/command_util.py @@ -17,10 +17,18 @@ from .config_reader import ConfigReader from .exceptions import CommandExecutionError from .parse_keeper_response import parse_keeper_response, ensure_record_add_json_format +from .throttle import ( + RESULT_EDGE_429, + RESULT_THROTTLED, + is_throttle_error, + throttle_error_response, +) from ..core.globals import get_current_params from ..decorators.logging import logger, debug_decorator, sanitize_debug_data from ... import cli, utils from ...crypto import encrypt_aes_v2 +from ...error import KeeperApiError + class CommandExecutor: @staticmethod @@ -114,6 +122,28 @@ def encrypt_response(response: Any) -> bytes: raise return response + @staticmethod + def _status_code_from_response(response: dict) -> int: + if 'status_code' in response: + return response.pop('status_code') + if response.get('status') in ('error', 'warning'): + return 400 + return 200 + + @classmethod + def _finalize_parsed_response(cls, response: Any) -> Tuple[Any, int]: + if not isinstance(response, dict): + return response, 200 + + status_code = cls._status_code_from_response(response) + result_code = response.get('result_code') + if status_code == 429 or result_code in (RESULT_THROTTLED, RESULT_EDGE_429): + body, status_code = throttle_error_response(response.get('error'), result_code) + if 'command' in response: + body['command'] = response['command'] + return body, status_code + return response, status_code + @classmethod def execute(cls, command: str) -> Tuple[Any, int]: logger.debug(f"Executing command: {command}") @@ -142,19 +172,7 @@ def execute(cls, command: str) -> Tuple[Any, int]: # Always let the parser handle the response (including empty responses and logs) response = parse_keeper_response(command, response, log_output) - - if isinstance(response, dict): - # Extract status_code and remove it from response body - if 'status_code' in response: - status_code = response.pop('status_code') - elif response.get("status") == "error": - status_code = 400 - elif response.get("status") == "warning": - status_code = 400 - else: - status_code = 200 - else: - status_code = 200 + response, status_code = cls._finalize_parsed_response(response) response = CommandExecutor.encrypt_response(response) logger.debug(f"Command executed successfully") @@ -162,16 +180,17 @@ def execute(cls, command: str) -> Tuple[Any, int]: except CommandExecutionError as e: # Return the actual command error instead of generic "server busy" logger.error(f"Command execution error: {e}") - error_response = { - "status": "error", - "error": str(e) - } - return error_response, 400 + if is_throttle_error(e): + return throttle_error_response(str(e)) + return {"status": "error", "error": str(e)}, 400 + except KeeperApiError as e: + if is_throttle_error(e): + return throttle_error_response(e.message or str(e), e.result_code) + logger.error(f"Unexpected error during command execution: {e}") + return {"status": "error", "error": f"Unexpected error: {str(e)}"}, 500 except Exception as e: + if is_throttle_error(e): + return throttle_error_response(str(e)) # Log unexpected errors and return a proper error response logger.error(f"Unexpected error during command execution: {e}") - error_response = { - "status": "error", - "error": f"Unexpected error: {str(e)}" - } - return error_response, 500 \ No newline at end of file + return {"status": "error", "error": f"Unexpected error: {str(e)}"}, 500 diff --git a/keepercommander/service/util/parse_keeper_response.py b/keepercommander/service/util/parse_keeper_response.py index bacd347c0..48cbe237f 100644 --- a/keepercommander/service/util/parse_keeper_response.py +++ b/keepercommander/service/util/parse_keeper_response.py @@ -12,6 +12,8 @@ from typing import Any, Dict, List, Optional, Tuple import re, json +from .throttle import RESULT_THROTTLED, clean_throttle_message, is_throttle_text + class KeeperResponseParser: @staticmethod def _clean_ansi_codes(text: str) -> str: @@ -1054,7 +1056,16 @@ def _parse_logging_based_command(command: str, response_str: str) -> Dict[str, A ] has_success_indicator = any(indicator in response_lower for indicator in success_indicators) - + + if is_throttle_text(response_str): + return { + "status": "error", + "status_code": 429, + "command": command.split()[0] if command.split() else command, + "error": clean_throttle_message(response_str), + "result_code": RESULT_THROTTLED, + } + forbidden_patterns = [ "not an msp administrator", "permission denied", diff --git a/keepercommander/service/util/throttle.py b/keepercommander/service/util/throttle.py new file mode 100644 index 000000000..ccb6b7ebb --- /dev/null +++ b/keepercommander/service/util/throttle.py @@ -0,0 +1,96 @@ +# _ __ +# | |/ /___ ___ _ __ ___ _ _ ® +# | ' bool: + if not text: + return False + lowered = text.lower() + return any(marker in lowered for marker in _THROTTLE_TEXT_MARKERS) + + +def is_throttle_error(exc_or_text: Union[BaseException, str]) -> bool: + if isinstance(exc_or_text, KeeperApiError): + code = exc_or_text.result_code + if code == RESULT_THROTTLED or code == 429 or str(code) == RESULT_EDGE_429: + return True + return is_throttle_text(exc_or_text.message or str(exc_or_text)) + return is_throttle_text(str(exc_or_text or '')) + + +def normalize_throttle_result_code(result_code: Any = None) -> str: + if result_code in (None, ''): + return RESULT_THROTTLED + if result_code == 429 or str(result_code) == RESULT_EDGE_429: + return RESULT_EDGE_429 + return str(result_code) + + +def clean_throttle_message(message: Optional[str]) -> str: + """Prefer Keeper's API message; drop duplicated client retry log noise.""" + if not message: + return 'Request throttled' + + text = message.replace('\\n', '\n') + for line in text.splitlines(): + line = line.strip() + if not line: + continue + lowered = line.lower() + # Skip Commander client retry warnings captured from logging + if 'retrying in' in lowered or ('throttled (' in lowered and 'attempt' in lowered): + continue + if 'too many requests' in lowered: + return 'Too Many Requests' + if 'throttled' in lowered or 'due to repeated attempts' in lowered: + if lowered.startswith('throttled:'): + return line.split(':', 1)[1].strip() or line + return line + + if 'retrying in' in text.lower(): + return 'Request throttled by Keeper API' + + first_line = text.strip().splitlines()[0] + return first_line[:500] + + +def throttle_error_response( + message: Optional[str] = None, + result_code: Any = RESULT_THROTTLED, +) -> Tuple[dict, int]: + return { + 'status': 'error', + 'error': clean_throttle_message(message), + 'result_code': normalize_throttle_result_code(result_code), + }, 429 + + +def rate_limited_response(detail: Optional[str] = None) -> Tuple[dict, int]: + error = 'Service rate limit exceeded' + if detail: + error = f'{error}: {detail}' + return { + 'status': 'error', + 'error': error, + 'result_code': RESULT_RATE_LIMITED, + }, 429 diff --git a/unit-tests/service/test_throttle_response.py b/unit-tests/service/test_throttle_response.py new file mode 100644 index 000000000..361ea5173 --- /dev/null +++ b/unit-tests/service/test_throttle_response.py @@ -0,0 +1,149 @@ +"""Service-mode throttle / rate-limit JSON 429 handling.""" + +import unittest +from unittest import mock + +from flask_limiter.errors import RateLimitExceeded + +from keepercommander.error import KeeperApiError +from keepercommander.service.app import create_app +from keepercommander.service.util.command_util import CommandExecutor +from keepercommander.service.util.parse_keeper_response import KeeperResponseParser +from keepercommander.service.util.throttle import ( + RESULT_EDGE_429, + RESULT_RATE_LIMITED, + RESULT_THROTTLED, + clean_throttle_message, + is_throttle_error, + is_throttle_text, + rate_limited_response, + throttle_error_response, +) + + +class TestThrottleHelpers(unittest.TestCase): + def test_clean_throttle_message_strips_retry_log_noise(self): + noisy = ( + "Throttled (attempt 1/3), retrying in 60 seconds\n" + "Throttled (attempt 1/3), retrying in 60 seconds\n" + "Due to repeated attempts, your request has been throttled. Try again in 1 minute." + ) + self.assertEqual( + clean_throttle_message(noisy), + 'Due to repeated attempts, your request has been throttled. Try again in 1 minute.', + ) + self.assertEqual( + clean_throttle_message('Throttled (attempt 1/3), retrying in 60 seconds\n' * 5), + 'Request throttled by Keeper API', + ) + + def test_clean_throttle_message_strips_result_code_prefix(self): + self.assertEqual( + clean_throttle_message('throttled: Try again in 1 minute.'), + 'Try again in 1 minute.', + ) + + def test_is_throttle_detection(self): + self.assertTrue(is_throttle_error(KeeperApiError('throttled', 'slow down'))) + self.assertTrue(is_throttle_error(KeeperApiError(429, 'Too Many Requests'))) + self.assertTrue(is_throttle_text('too many requests')) + self.assertFalse(is_throttle_text('invalid rate limit config value')) + self.assertFalse(is_throttle_error(KeeperApiError('access_denied', 'nope'))) + + def test_throttle_and_rate_limited_response_shapes(self): + body, status = throttle_error_response('Try again in 1 minute.', RESULT_THROTTLED) + self.assertEqual(status, 429) + self.assertEqual(body['result_code'], RESULT_THROTTLED) + self.assertEqual(body['error'], 'Try again in 1 minute.') + + body, status = throttle_error_response('Too Many Requests', 429) + self.assertEqual(body['result_code'], RESULT_EDGE_429) + + body, status = rate_limited_response('3 per 1 minute') + self.assertEqual(status, 429) + self.assertEqual(body['result_code'], RESULT_RATE_LIMITED) + self.assertIn('3 per 1 minute', body['error']) + + +class TestThrottleResponse(unittest.TestCase): + def test_parser_maps_throttled_text_to_429(self): + result = KeeperResponseParser._parse_logging_based_command( + 'keep-alive', + 'throttled: Due to repeated attempts, your request has been throttled.', + ) + self.assertEqual(result['status_code'], 429) + self.assertEqual(result['result_code'], RESULT_THROTTLED) + self.assertEqual( + result['error'], + 'Due to repeated attempts, your request has been throttled.', + ) + + def test_parser_does_not_treat_rate_limit_config_text_as_throttle(self): + result = KeeperResponseParser._parse_logging_based_command( + 'help', + 'Invalid rate limit config value', + ) + self.assertNotEqual(result.get('status_code'), 429) + self.assertNotEqual(result.get('result_code'), RESULT_THROTTLED) + + def test_execute_maps_keeper_throttle_to_429(self): + err = KeeperApiError('throttled', 'Try again in 1 minute.') + params = mock.Mock(service_mode=False) + params.rest_context = mock.Mock() + with mock.patch( + 'keepercommander.service.core.globals.ensure_params_loaded', + return_value=params, + ), mock.patch.object(CommandExecutor, 'capture_output_and_logs', side_effect=err): + body, status = CommandExecutor.execute('keep-alive') + self.assertEqual(status, 429) + self.assertEqual(body['result_code'], RESULT_THROTTLED) + self.assertEqual(body['error'], 'Try again in 1 minute.') + + def test_execute_maps_edge_429(self): + err = KeeperApiError(429, 'Too Many Requests') + params = mock.Mock(service_mode=False) + params.rest_context = mock.Mock() + with mock.patch( + 'keepercommander.service.core.globals.ensure_params_loaded', + return_value=params, + ), mock.patch.object(CommandExecutor, 'capture_output_and_logs', side_effect=err): + body, status = CommandExecutor.execute('keep-alive') + self.assertEqual(status, 429) + self.assertEqual(body['result_code'], RESULT_EDGE_429) + self.assertEqual(body['error'], 'Too Many Requests') + + def test_execute_preserves_non_throttle_keeper_error_as_500(self): + err = KeeperApiError('access_denied', 'nope') + params = mock.Mock(service_mode=False) + params.rest_context = mock.Mock() + with mock.patch( + 'keepercommander.service.core.globals.ensure_params_loaded', + return_value=params, + ), mock.patch.object(CommandExecutor, 'capture_output_and_logs', side_effect=err): + body, status = CommandExecutor.execute('keep-alive') + self.assertEqual(status, 500) + self.assertIn('Unexpected error', body['error']) + + def test_flask_limiter_returns_json_429(self): + with mock.patch('keepercommander.service.app.init_routes'), \ + mock.patch('keepercommander.service.app.is_behind_proxy', return_value=False), \ + mock.patch('keepercommander.service.app.limiter.init_app'): + app = create_app() + + limit = mock.MagicMock() + limit.error_message = '3 per 1 minute' + + @app.route('/_test_limited') + def _limited(): + raise RateLimitExceeded(limit) + + resp = app.test_client().get('/_test_limited') + self.assertEqual(resp.status_code, 429) + data = resp.get_json() + self.assertEqual(data['status'], 'error') + self.assertEqual(data['result_code'], RESULT_RATE_LIMITED) + self.assertIn('rate limit', data['error'].lower()) + + +if __name__ == '__main__': + unittest.main()