From 59d1e618500bdb78bd7cc56b5177e9dfa99ed146 Mon Sep 17 00:00:00 2001 From: Daniel Sotirhos Date: Wed, 7 Aug 2024 09:12:34 -0700 Subject: [PATCH 1/6] Replace double quotes with single quotes --- lambdas/service/app.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lambdas/service/app.py b/lambdas/service/app.py index fea36acd8..aca377749 100644 --- a/lambdas/service/app.py +++ b/lambdas/service/app.py @@ -490,7 +490,7 @@ def static_resource(file): def oauth2_redirect(): oauth2_redirect_html = app.load_static_resource('swagger', 'oauth2-redirect.html') return Response(status_code=200, - headers={"Content-Type": "text/html"}, + headers={'Content-Type': 'text/html'}, body=oauth2_redirect_html) From 1c6521ae896771fc943917931ec218d69beabee7 Mon Sep 17 00:00:00 2001 From: Daniel Sotirhos Date: Tue, 20 Aug 2024 09:38:10 -0700 Subject: [PATCH 2/6] Move import statement above docstring --- scripts/convert_environment.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/scripts/convert_environment.py b/scripts/convert_environment.py index 1e330cd27..0fb886344 100644 --- a/scripts/convert_environment.py +++ b/scripts/convert_environment.py @@ -13,13 +13,6 @@ Optional, ) -""" -Convert an old-style `environment` Bash script to a new-style `environment.py`. - -This is by no means a complete parser for shell scripts. It recognizes only the -script statements typically used in `environment` and `environment.local` files. -""" - from azul.files import ( write_file_atomically, ) @@ -27,6 +20,13 @@ single_quote as sq, ) +""" +Convert an old-style `environment` Bash script to a new-style `environment.py`. + +This is by no means a complete parser for shell scripts. It recognizes only the +script statements typically used in `environment` and `environment.local` files. +""" + class Variable(NamedTuple): name: str From 983c3ab39d9baecd9b85ce7a9e73a7623117cb4e Mon Sep 17 00:00:00 2001 From: Daniel Sotirhos Date: Wed, 21 Aug 2024 11:21:15 -0700 Subject: [PATCH 3/6] Rename file parameter in swagger_resource() --- src/azul/chalice.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/azul/chalice.py b/src/azul/chalice.py index 0405133ff..933c6df6c 100644 --- a/src/azul/chalice.py +++ b/src/azul/chalice.py @@ -508,16 +508,16 @@ def swagger_ui(self) -> Response: headers={'Content-Type': 'text/html'}, body=swagger_ui_html) - def swagger_resource(self, file) -> Response: - if os.sep in file: - raise BadRequestError(file) + def swagger_resource(self, file_name: str) -> Response: + if os.sep in file_name: + raise BadRequestError(file_name) else: try: - body = self.load_static_resource('swagger', file) + body = self.load_static_resource('swagger', file_name) except FileNotFoundError: - raise NotFoundError(file) + raise NotFoundError(file_name) else: - path = pathlib.Path(file) + path = pathlib.Path(file_name) content_type = mimetypes.types_map[path.suffix] return Response(status_code=200, headers={'Content-Type': content_type}, From f7dbb4c2a217f6d1de75dd74590ba7460872c9c8 Mon Sep 17 00:00:00 2001 From: Daniel Sotirhos Date: Wed, 7 Aug 2024 09:13:24 -0700 Subject: [PATCH 4/6] Refactor assert of request and response headers in test case --- test/service/test_app_logging.py | 37 ++++++++++++++++++++------------ test/test_app_logging.py | 26 ++++++++++++++-------- 2 files changed, 40 insertions(+), 23 deletions(-) diff --git a/test/service/test_app_logging.py b/test/service/test_app_logging.py index 9585bde07..981e7f9c0 100644 --- a/test/service/test_app_logging.py +++ b/test/service/test_app_logging.py @@ -43,7 +43,7 @@ def test_request_logs(self): with self.assertLogs(logger=log, level=level) as logs: requests.get(str(url), headers=headers) logs = [(r.levelno, r.getMessage()) for r in logs.records] - headers = { + request_headers = { 'host': url.netloc, 'user-agent': 'python-requests/2.32.2', 'accept-encoding': 'gzip, deflate, br', @@ -51,11 +51,28 @@ def test_request_logs(self): 'connection': 'keep-alive', **headers, } + response_headers = { + 'Access-Control-Allow-Origin': '*', + 'Access-Control-Allow-Headers': 'Authorization,' + 'Content-Type,' + 'X-Amz-Date,' + 'X-Amz-Security-Token,' + 'X-Api-Key', + 'Content-Security-Policy': f"default-src {sq('self')}", + 'Referrer-Policy': 'strict-origin-when-cross-origin', + 'Strict-Transport-Security': 'max-age=63072000;' + ' includeSubDomains;' + ' preload', + 'X-Content-Type-Options': 'nosniff', + 'X-Frame-Options': 'DENY', + 'X-XSS-Protection': '1; mode=block', + 'Cache-Control': 'no-store' + } self.assertEqual(logs, [ ( INFO, - f"Received GET request for '/health/basic', " - f"with {json.dumps({'query': None, 'headers': headers})}."), + "Received GET request for '/health/basic', " + f"with {json.dumps(dict(query=None, headers=request_headers))}."), ( INFO, "Authenticated request as OAuth2(access_token='foo_token')" @@ -66,17 +83,9 @@ def test_request_logs(self): level, 'Returning 200 response. To log headers and body, set AZUL_DEBUG to 1.' if level == INFO else - 'Returning 200 response with headers {"Access-Control-Allow-Origin": ' - '"*", "Access-Control-Allow-Headers": ' - '"Authorization,Content-Type,X-Amz-Date,X-Amz-Security-Token,X-Api-Key", ' - f'"Content-Security-Policy": "default-src {sq("self")}", ' - '"Referrer-Policy": "strict-origin-when-cross-origin", ' - '"Strict-Transport-Security": "max-age=63072000; includeSubDomains; preload", ' - '"X-Content-Type-Options": "nosniff", ' - '"X-Frame-Options": "DENY", ' - '"X-XSS-Protection": "1; mode=block", ' - '"Cache-Control": "no-store"}. ' - 'See next line for the first 1024 characters of the body.\n' + 'Returning 200 response with headers ' + + json.dumps(response_headers) + '. ' + + 'See next line for the first 1024 characters of the body.\n' + '{"up": true}' ) ]) diff --git a/test/test_app_logging.py b/test/test_app_logging.py index 237270757..0453d0603 100644 --- a/test/test_app_logging.py +++ b/test/test_app_logging.py @@ -105,17 +105,25 @@ def fail(): self.assertTrue(response.startswith(traceback_header)) self.assertIn(magic_message, response) # … and the response is logged. + headers = { + 'Content-Type': 'text/plain', + 'Content-Security-Policy': f"default-src {sq('self')}", + 'Referrer-Policy': 'strict-origin-when-cross-origin', + 'Strict-Transport-Security': 'max-age=63072000;' + ' includeSubDomains;' + ' preload', + 'X-Content-Type-Options': 'nosniff', + 'X-Frame-Options': 'DENY', + 'X-XSS-Protection': '1; mode=block', + 'Cache-Control': 'no-store' + } self.assertEqual( azul_log.output[2], - 'DEBUG:azul.chalice:Returning 500 response with headers {"Content-Type": "text/plain", ' - f'"Content-Security-Policy": "default-src {sq("self")}", ' - '"Referrer-Policy": "strict-origin-when-cross-origin", ' - '"Strict-Transport-Security": "max-age=63072000; includeSubDomains; preload", ' - '"X-Content-Type-Options": "nosniff", ' - '"X-Frame-Options": "DENY", ' - '"X-XSS-Protection": "1; mode=block", ' - '"Cache-Control": "no-store"}. ' - 'See next line for the first 1024 characters of the body.\n' + response) + 'DEBUG:azul.chalice:Returning 500 response with headers ' + + json.dumps(headers) + '. ' + + 'See next line for the first 1024 characters of the body.\n' + + response + ) else: # Otherwise, a generic error response is returned … self.assertEqual(response.json(), { From 7f75bd357e0ac13be73bd0b3c68e75f351925d8f Mon Sep 17 00:00:00 2001 From: Daniel Sotirhos Date: Fri, 4 Oct 2024 17:01:18 -0700 Subject: [PATCH 5/6] Refactor security_headers property into a method --- src/azul/chalice.py | 36 +++++++++++++++++++----------------- src/azul/terraform.py | 2 +- test/integration_test.py | 4 ++-- 3 files changed, 22 insertions(+), 20 deletions(-) diff --git a/src/azul/chalice.py b/src/azul/chalice.py index 933c6df6c..24e13c564 100644 --- a/src/azul/chalice.py +++ b/src/azul/chalice.py @@ -193,29 +193,31 @@ def _api_gateway_context_middleware(self, event, get_response): finally: config.lambda_is_handling_api_gateway_request = False - hsts_max_age = 60 * 60 * 24 * 365 * 2 - - # Headers added to every response from the app, as well as canned 4XX and - # 5XX responses from API Gateway. Use of these headers addresses known - # security vulnerabilities. - # - security_headers = { - 'Content-Security-Policy': jw('default-src', sq('self')), - 'Referrer-Policy': 'strict-origin-when-cross-origin', - 'Strict-Transport-Security': jw(f'max-age={hsts_max_age};', - 'includeSubDomains;', - 'preload'), - 'X-Content-Type-Options': 'nosniff', - 'X-Frame-Options': 'DENY', - 'X-XSS-Protection': '1; mode=block' - } + @classmethod + def security_headers(cls) -> dict[str, str]: + """ + Headers added to every response from the app, as well as canned 4XX and + 5XX responses from API Gateway. Use of these headers addresses known + security vulnerabilities. + """ + hsts_max_age = 60 * 60 * 24 * 365 * 2 + return { + 'Content-Security-Policy': jw('default-src', sq('self')), + 'Referrer-Policy': 'strict-origin-when-cross-origin', + 'Strict-Transport-Security': jw(f'max-age={hsts_max_age};', + 'includeSubDomains;', + 'preload'), + 'X-Content-Type-Options': 'nosniff', + 'X-Frame-Options': 'DENY', + 'X-XSS-Protection': '1; mode=block' + } def _security_headers_middleware(self, event, get_response): """ Add headers to the response """ response = get_response(event) - response.headers.update(self.security_headers) + response.headers.update(self.security_headers()) # FIXME: Add a CSP header with a nonce value to text/html responses # https://github.com/DataBiosphere/azul-private/issues/6 if response.headers.get('Content-Type') == 'text/html': diff --git a/src/azul/terraform.py b/src/azul/terraform.py index f40382e80..15bad840f 100644 --- a/src/azul/terraform.py +++ b/src/azul/terraform.py @@ -829,7 +829,7 @@ def tf_config(self, app_name): # value, which that function would prohibit. # f'gatewayresponse.header.{k}': f"'{v}'" - for k, v in AzulChaliceApp.security_headers.items() + for k, v in AzulChaliceApp.security_headers().items() } } for response_type in ['4XX', '5XX'] } diff --git a/test/integration_test.py b/test/integration_test.py index ee66f595c..ed30db5c9 100644 --- a/test/integration_test.py +++ b/test/integration_test.py @@ -2053,7 +2053,7 @@ def test_response_security_headers(self): else: response = requests.get(str(endpoint / path)) response.raise_for_status() - expected = AzulChaliceApp.security_headers | expected_headers + expected = AzulChaliceApp.security_headers() | expected_headers # FIXME: Add a CSP header with a nonce value to text/html responses # https://github.com/DataBiosphere/azul-private/issues/6 if path in ['/', '/oauth2_redirect']: @@ -2065,5 +2065,5 @@ def test_default_4xx_response_headers(self): with self.subTest(endpoint=endpoint): response = requests.get(str(endpoint / 'does-not-exist')) self.assertEqual(403, response.status_code) - self.assertIsSubset(AzulChaliceApp.security_headers.items(), + self.assertIsSubset(AzulChaliceApp.security_headers().items(), response.headers.items()) From a45d3ad147486acd74fe9ce3662575255f788fb3 Mon Sep 17 00:00:00 2001 From: Daniel Sotirhos Date: Wed, 7 Aug 2024 09:15:45 -0700 Subject: [PATCH 6/6] Fix: Content Security Policy (CSP) Not Implemented (DataBiosphere/azul-private#6) --- lambdas/service/app.py | 16 +++++-- src/azul/chalice.py | 46 +++++++++++++------ ...=> oauth2-redirect.html.template.mustache} | 15 +++++- swagger/swagger-ui.html.template.mustache | 4 +- test/integration_test.py | 6 ++- test/service/test_app_logging.py | 8 +++- test/test_app_logging.py | 8 +++- 7 files changed, 79 insertions(+), 24 deletions(-) rename swagger/{oauth2-redirect.html => oauth2-redirect.html.template.mustache} (85%) diff --git a/lambdas/service/app.py b/lambdas/service/app.py index aca377749..599bbd651 100644 --- a/lambdas/service/app.py +++ b/lambdas/service/app.py @@ -12,6 +12,7 @@ ) import json import logging.config +import secrets from typing import ( Any, Callable, @@ -30,6 +31,7 @@ Response, UnauthorizedError, ) +import chevron from furl import ( furl, ) @@ -488,10 +490,18 @@ def static_resource(file): cache_control='no-store' ) def oauth2_redirect(): - oauth2_redirect_html = app.load_static_resource('swagger', 'oauth2-redirect.html') + file_name = 'oauth2-redirect.html.template.mustache' + template = app.load_static_resource('swagger', file_name) + nonce = secrets.token_urlsafe(32) + html = chevron.render(template, { + 'CSP_NONCE': json.dumps(nonce) + }) return Response(status_code=200, - headers={'Content-Type': 'text/html'}, - body=oauth2_redirect_html) + headers={ + 'Content-Type': 'text/html', + 'Content-Security-Policy': app.content_security_policy(nonce) + }, + body=html) common_specs = CommonEndpointSpecs(app_name='service') diff --git a/src/azul/chalice.py b/src/azul/chalice.py index 24e13c564..8fd5567e9 100644 --- a/src/azul/chalice.py +++ b/src/azul/chalice.py @@ -15,6 +15,7 @@ import mimetypes import os import pathlib +import secrets from typing import ( Any, Iterator, @@ -193,16 +194,30 @@ def _api_gateway_context_middleware(self, event, get_response): finally: config.lambda_is_handling_api_gateway_request = False + @classmethod + def content_security_policy(cls, nonce: str | None = None) -> str: + self_ = sq('self') + none = sq('none') + nonce = [] if nonce is None else [sq('nonce-' + nonce)] + + return ';'.join([ + jw('default-src', self_), + jw('img-src', self_, 'data:'), + jw('script-src', self_, *nonce), + jw('style-src', self_, *nonce), + jw('frame-ancestors', none), + ]) + @classmethod def security_headers(cls) -> dict[str, str]: """ - Headers added to every response from the app, as well as canned 4XX and - 5XX responses from API Gateway. Use of these headers addresses known - security vulnerabilities. + Default values for headers added to every response from the app, as well + as canned 4XX and 5XX responses from API Gateway. Use of these headers + addresses known security vulnerabilities. """ hsts_max_age = 60 * 60 * 24 * 365 * 2 return { - 'Content-Security-Policy': jw('default-src', sq('self')), + 'Content-Security-Policy': cls.content_security_policy(), 'Referrer-Policy': 'strict-origin-when-cross-origin', 'Strict-Transport-Security': jw(f'max-age={hsts_max_age};', 'includeSubDomains;', @@ -217,11 +232,10 @@ def _security_headers_middleware(self, event, get_response): Add headers to the response """ response = get_response(event) - response.headers.update(self.security_headers()) - # FIXME: Add a CSP header with a nonce value to text/html responses - # https://github.com/DataBiosphere/azul-private/issues/6 - if response.headers.get('Content-Type') == 'text/html': - del response.headers['Content-Security-Policy'] + # Add security headers to the response without overwriting any headers + # that might have been added already (e.g. Content-Security-Policy) + for key, val in self.security_headers().items(): + response.headers.setdefault(key, val) view_function = self.routes[event.path][event.method].view_function cache_control = getattr(view_function, 'cache_control') response.headers['Cache-Control'] = cache_control @@ -493,11 +507,14 @@ def _controller(self, controller_cls: Type[C], **kwargs) -> C: return controller_cls(app=self, **kwargs) def swagger_ui(self) -> Response: - swagger_ui_template = self.load_static_resource('swagger', 'swagger-ui.html.template.mustache') + file_name = 'swagger-ui.html.template.mustache' + template = self.load_static_resource('swagger', file_name) base_url = self.base_url redirect_url = furl(base_url).add(path='oauth2_redirect') deployment_url = furl(base_url).add(path='openapi') - swagger_ui_html = chevron.render(swagger_ui_template, { + nonce = secrets.token_urlsafe(32) + html = chevron.render(template, { + 'CSP_NONCE': json.dumps(nonce), 'DEPLOYMENT_PATH': json.dumps(str(deployment_url.path)), 'OAUTH2_CLIENT_ID': json.dumps(config.google_oauth2_client_id), 'OAUTH2_REDIRECT_URL': json.dumps(str(redirect_url)), @@ -507,8 +524,11 @@ def swagger_ui(self) -> Response: ]) }) return Response(status_code=200, - headers={'Content-Type': 'text/html'}, - body=swagger_ui_html) + headers={ + 'Content-Type': 'text/html', + 'Content-Security-Policy': self.content_security_policy(nonce) + }, + body=html) def swagger_resource(self, file_name: str) -> Response: if os.sep in file_name: diff --git a/swagger/oauth2-redirect.html b/swagger/oauth2-redirect.html.template.mustache similarity index 85% rename from swagger/oauth2-redirect.html rename to swagger/oauth2-redirect.html.template.mustache index 0c0b5da50..e6266a181 100644 --- a/swagger/oauth2-redirect.html +++ b/swagger/oauth2-redirect.html.template.mustache @@ -2,10 +2,10 @@ Swagger UI: OAuth2 Redirect - + - diff --git a/swagger/swagger-ui.html.template.mustache b/swagger/swagger-ui.html.template.mustache index 33889fca8..ffa004c54 100644 --- a/swagger/swagger-ui.html.template.mustache +++ b/swagger/swagger-ui.html.template.mustache @@ -5,7 +5,7 @@ Swagger UI -