Improved validation constraints for API signatures (#485)

* improved validation of API signatures

* corrected Sacct schema instance_id validator max allowed size

* adjust __validate_request func to allow not raising exception for missing request.json body to use with PC proxy

* split PCProxy schema in two (args and body) and added request size validation for PC proxy APIs

* adoption of changed and new schemas

* added test for `raise_on_missing_body`

* added test for custom validator

Co-authored-by: Marco Basile <[email protected]>

Author: BarcoMasile

api/PclusterApiHandler.py

@@ -27,7 +27,7 @@
 from api.security.csrf.csrf import csrf_needed
 from api.utils import disable_auth
 from api.validation import validated
-from api.validation.schemas import PCProxy
+from api.validation.schemas import PCProxyArgs, PCProxyBody
 
 USER_POOL_ID = os.getenv("USER_POOL_ID")
 AUTH_PATH = os.getenv("AUTH_PATH")
@@ -700,15 +700,15 @@ def _get_params(_request):
 
 @pc.get('/', strict_slashes=False)
 @authenticated({'admin'})
-@validated(params=PCProxy)
+@validated(params=PCProxyArgs)
 def pc_proxy_get():
     response = sigv4_request(request.method, API_BASE_URL, request.args.get("path"), _get_params(request))
     return response.json(), response.status_code
 
 @pc.route('/', methods=['POST','PUT','PATCH','DELETE'], strict_slashes=False)
 @authenticated({'admin'})
 @csrf_needed
-@validated(params=PCProxy)
+@validated(params=PCProxyArgs, body=PCProxyBody, raise_on_missing_body=False)
 def pc_proxy():
     body = None
     try:

api/tests/validation/test_api_custom_validators.py

@@ -0,0 +1,18 @@
+import pytest
+from marshmallow import ValidationError
+
+from api.validation.validators import size_not_exceeding
+
+
+def test_size_not_exceeding():
+    max_size = 300
+    test_str_not_exceeding = 'a' * (max_size - 2) # save 2 chars for double quotes
+
+    size_not_exceeding(test_str_not_exceeding, max_size)
+
+def test_size_not_exceeding_failing():
+    max_size = 300
+    test_str_not_exceeding = 'a' * max_size # will produce "aaa...", max_size + 2
+
+    with pytest.raises(ValidationError):
+        size_not_exceeding(test_str_not_exceeding, max_size)

api/tests/validation/test_api_validation.py

@@ -1,3 +1,5 @@
+from unittest.mock import Mock, PropertyMock
+
 import pytest
 from marshmallow import Schema, fields, validate, ValidationError
 
@@ -30,6 +32,33 @@ def test_valid_request_successful(mock_csrf_needed, mock_enable_auth):
 
     assert errors == {}
 
+def test_invalid_request_successful_with_raise_on_missing_body_enabled(mock_csrf_needed, mock_enable_auth):
+    request = MockRequest()
+    mock_json_property = PropertyMock(side_effect=Exception)
+    original_json_property = MockRequest.json
+    MockRequest.json = mock_json_property
+
+    with pytest.raises(ValueError):
+        __validate_request(request, body_schema=MockRequestJsonSchema(), params_schema=MockRequestArgsSchema(),
+                                    cookies_schema=MockRequestCookiesSchema())
+
+    mock_json_property.assert_called_once()
+    MockRequest.json = original_json_property
+
+def test_invalid_request_successful_with_raise_on_missing_body_disabled(mock_csrf_needed, mock_enable_auth):
+    request = MockRequest()
+    mock_json_property = PropertyMock(side_effect=Exception)
+    original_json_property = MockRequest.json
+    MockRequest.json = mock_json_property
+
+    errors = __validate_request(request, body_schema=MockRequestJsonSchema(), params_schema=MockRequestArgsSchema(),
+                                cookies_schema=MockRequestCookiesSchema(), raise_on_missing_body=False)
+
+    mock_json_property.assert_called_once()
+    assert errors == {}
+
+    MockRequest.json = original_json_property
+
 
 def test_valid_request_failure(mock_csrf_needed, mock_enable_auth):
     request = MockRequest()

api/validation/__init__.py

@@ -6,13 +6,14 @@
 from api.validation.schemas import EC2Action
 
 
-def __validate_request(_request: Request, *, body_schema: Schema = None, params_schema: Schema = None, cookies_schema: Schema = None):
+def __validate_request(_request: Request, *, body_schema: Schema = None, params_schema: Schema = None, cookies_schema: Schema = None, raise_on_missing_body = True):
     errors = {}
     if body_schema:
         try:
             errors.update(body_schema.validate(_request.json))
         except:
-            raise ValueError('Expected json body')
+            if raise_on_missing_body:
+                raise ValueError('Expected json body')
 
     if params_schema:
         errors.update(params_schema.validate(_request.args))
@@ -23,11 +24,11 @@ def __validate_request(_request: Request, *, body_schema: Schema = None, params_
     return errors
 
 
-def validated(*, body: Schema = None, params: Schema = None, cookies: Schema = None):
+def validated(*, body: Schema = None, params: Schema = None, cookies: Schema = None, raise_on_missing_body = True):
     def wrapper(func):
         @wraps(func)
         def decorated(*pargs, **kwargs):
-            errors = __validate_request(request, body_schema=body, params_schema=params, cookies_schema=cookies)
+            errors = __validate_request(request, body_schema=body, params_schema=params, cookies_schema=cookies, raise_on_missing_body=raise_on_missing_body)
             if errors:
                 raise ValidationError(f'Input validation failed for {request.path}', data=errors)
             return func(*pargs, **kwargs)

api/validation/schemas.py

@@ -1,21 +1,21 @@
-from marshmallow import Schema, fields, validate, INCLUDE
+from marshmallow import Schema, fields, validate, INCLUDE, validates_schema, ValidationError
 
-from api.validation.validators import comma_splittable, aws_region_validator, is_alphanumeric_with_hyphen, valid_api_log_levels_predicate
-from api.logging import VALID_LOG_LEVELS
+from api.validation.validators import comma_splittable, aws_region_validator, is_alphanumeric_with_hyphen, \
+    valid_api_log_levels_predicate, size_not_exceeding
 
 
 class EC2ActionSchema(Schema):
     action = fields.String(required=True, validate=validate.OneOf(['stop_instances', 'start_instances']))
-    instance_ids = fields.String(required=True, validate=comma_splittable)
+    instance_ids = fields.String(required=True, validate=validate.And(comma_splittable, validate.Length(max=2048)))
     region = fields.String(validate=aws_region_validator)
 
 
 EC2Action = EC2ActionSchema(unknown=INCLUDE)
 
 
 class CreateUserSchema(Schema):
-    Username = fields.Email(required=True)
-    Phonenumber = fields.String()
+    Username = fields.Email(required=True, validate=validate.Length(max=320)) # Email RFC allows max 320 chars
+    Phonenumber = fields.String(validate=validate.Length(max=15)) # ITU-T E.164 allows phone numbers no more than 15 digits
 
 
 CreateUser = CreateUserSchema(unknown=INCLUDE)
@@ -29,13 +29,13 @@ class DeleteUserSchema(Schema):
 
 class GetClusterConfigSchema(Schema):
     region = fields.String(validate=aws_region_validator)
-    cluster_name = fields.String(required=True, validate=is_alphanumeric_with_hyphen)
+    cluster_name = fields.String(required=True, validate=validate.And(is_alphanumeric_with_hyphen, validate.Length(max=60))) # PC allow cluster name of max 60 chars
 
 GetClusterConfig = GetClusterConfigSchema(unknown=INCLUDE)
 
 
 class GetCustomImageConfigSchema(Schema):
-    image_id = fields.String(required=True, validate=is_alphanumeric_with_hyphen)
+    image_id = fields.String(required=True, validate=validate.And(is_alphanumeric_with_hyphen, validate.Length(min=1, max=1024))) # AMI id min 1, max 1024 chars
 
 GetCustomImageConfig = GetCustomImageConfigSchema(unknown=INCLUDE)
 
@@ -53,72 +53,82 @@ class GetInstanceTypesSchema(Schema):
 
 
 class GetDcvSessionSchema(Schema):
-    user = fields.String()
-    instance_id = fields.String(required=True)
+    user = fields.String(validate=validate.Length(max=64))
+    instance_id = fields.String(required=True, validate=validate.Length(max=60))
     region = fields.String(validate=aws_region_validator)
 
 GetDcvSession = GetDcvSessionSchema(unknown=INCLUDE)
 
 
 class QueueStatusSchema(Schema):
-    user = fields.String()
-    instance_id = fields.String(required=True)
+    user = fields.String(validate=validate.Length(max=64))
+    instance_id = fields.String(required=True, validate=validate.Length(max=60))
     region = fields.String(required=True, validate=aws_region_validator)
 
 QueueStatus = QueueStatusSchema(unknown=INCLUDE)
 
 
 class ScontrolJobSchema(Schema):
-    user = fields.String()
-    instance_id = fields.String(required=True)
-    job_id = fields.String(required=True)
+    user = fields.String(validate=validate.Length(max=64))
+    instance_id = fields.String(required=True, validate=validate.Length(max=60))
+    job_id = fields.String(required=True, validate=validate.Length(max=256))
     region = fields.String(required=True, validate=aws_region_validator)
 
 ScontrolJob = ScontrolJobSchema(unknown=INCLUDE)
 
 
 class CancelJobSchema(Schema):
-    user = fields.String()
-    instance_id = fields.String(required=True)
-    job_id = fields.String(required=True)
+    user = fields.String(validate=validate.Length(max=64))
+    instance_id = fields.String(required=True, validate=validate.Length(max=60))
+    job_id = fields.String(required=True, validate=validate.Length(max=256))
     region = fields.String(required=True, validate=aws_region_validator)
 
 CancelJob = CancelJobSchema(unknown=INCLUDE)
 
 
 class SacctSchema(Schema):
-    user = fields.String()
-    instance_id = fields.String(required=True)
-    cluster_name = fields.String(required=True, validate=is_alphanumeric_with_hyphen)
+    user = fields.String(validate=validate.Length(max=64))
+    instance_id = fields.String(required=True, validate=validate.Length(max=60))
+    cluster_name = fields.String(required=True, validate=validate.And(is_alphanumeric_with_hyphen, validate.Length(max=60)))
     region = fields.String(required=True, validate=aws_region_validator)
 
 Sacct = SacctSchema(unknown=INCLUDE)
 
 
 class LoginSchema(Schema):
-    code = fields.String(required=True)
+    code = fields.String(required=True, validate=validate.Length(max=128))
 
 Login = LoginSchema(unknown=INCLUDE)
 
 
 class PushLogSchema(Schema):
     class PushLogEntrySchema(Schema):
         level = fields.String(required=True, validate=valid_api_log_levels_predicate)
-        message = fields.String(required=True)
+        message = fields.String(required=True, validate=validate.Length(max=246000)) # CW limit is 256k, leaving 1k to extra and level
         extra = fields.Dict()
 
     logs = fields.List(fields.Nested(PushLogEntrySchema), required=True)
 
 PushLog = PushLogSchema(unknown=INCLUDE)
 
 class PriceEstimateSchema(Schema):
-    cluster_name = fields.String(required=True)
-    queue_name = fields.String(required=True)
+    cluster_name = fields.String(required=True, validate=validate.Length(max=60))
+    queue_name = fields.String(required=True, validate=validate.Length(max=60))
     region = fields.String(validate=aws_region_validator, required=True)
 
 PriceEstimate = PriceEstimateSchema(unknown=INCLUDE)
 
-class PCProxySchema(Schema):
-    path = fields.String(required=True)
+class PCProxyArgsSchema(Schema):
+    path = fields.String(required=True, validate=validate.Length(max=512))
 
-PCProxy = PCProxySchema(unknown=INCLUDE)
+PCProxyArgs = PCProxyArgsSchema(unknown=INCLUDE)
+
+class PCProxyBodySchema(Schema):
+    def __init__(self, max_size, **kwargs):
+        super().__init__(**kwargs)
+        self.max_size = max_size
+    @validates_schema(pass_original=False)
+    def request_body_not_exceeding(self, data, **kwargs):
+        size_not_exceeding(data, self.max_size)
+
+PCProxyBody = PCProxyBodySchema(max_size=8192,unknown=INCLUDE)

api/validation/validators.py

@@ -1,4 +1,6 @@
-from marshmallow import validate
+import json
+
+from marshmallow import validate, ValidationError
 import re
 
 from api.logging import VALID_LOG_LEVELS
@@ -33,3 +35,9 @@ def is_alphanumeric_with_hyphen(arg: str):
 
 def valid_api_log_levels_predicate(loglevel):
     return loglevel.lower() in VALID_LOG_LEVELS
+
+def size_not_exceeding(data, size):
+    bytes_ = bytes(json.dumps(data), 'utf-8')
+    byte_size = len(bytes_)
+    if byte_size > size:
+        raise ValidationError(f'Request body exceeded max size of {size}')