From eef1470ee632b5d65da9b745a73762b0e259ddeb Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Thu, 28 Nov 2024 14:51:10 +0100 Subject: [PATCH 1/6] Detect correct fingerprint when using PGP subkeys --- irrd/utils/pgp.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/irrd/utils/pgp.py b/irrd/utils/pgp.py index ec2b89aa..a4cbb1fb 100644 --- a/irrd/utils/pgp.py +++ b/irrd/utils/pgp.py @@ -79,5 +79,5 @@ def validate_pgp_signature( logger.info(f"checked PGP signature, response: {log_message}") if result.valid and result.key_status is None: logger.info(f"Found valid PGP signature, fingerprint {result.fingerprint}") - return new_message, result.fingerprint + return new_message, result.pubkey_fingerprint return None, None From a21d6cb4d5df26ba4f514c65443517323b0fd90c Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Thu, 28 Nov 2024 14:51:42 +0100 Subject: [PATCH 2/6] Fix #949 - Add support for inline PGP signatures in RPSL update form --- irrd/webui/endpoints.py | 11 +++++++---- irrd/webui/templates/rpsl_form.html | 4 ++-- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/irrd/webui/endpoints.py b/irrd/webui/endpoints.py index c6e603bf..c2759a3b 100644 --- a/irrd/webui/endpoints.py +++ b/irrd/webui/endpoints.py @@ -3,7 +3,7 @@ from asgiref.sync import sync_to_async from starlette.requests import Request from starlette.responses import Response -from starlette_wtf import csrf_protect, csrf_token +from starlette_wtf import csrf_token from irrd import META_KEY_HTTP_CLIENT_IP from irrd.conf import get_setting @@ -18,6 +18,7 @@ from irrd.storage.orm_provider import ORMSessionProvider, session_provider_manager from irrd.storage.queries import RPSLDatabaseQuery from irrd.updates.handler import ChangeSubmissionHandler +from irrd.utils.pgp import validate_pgp_signature from irrd.webui.auth.decorators import authentication_required, mark_user_mfa_incomplete from irrd.webui.helpers import filter_auth_hash_non_mntner from irrd.webui.rendering import template_context_render @@ -101,7 +102,6 @@ async def rpsl_detail(request: Request, user_mfa_incomplete: bool, session_provi ) -@csrf_protect @mark_user_mfa_incomplete @session_provider_manager async def rpsl_update( @@ -147,6 +147,7 @@ async def rpsl_update( elif request.method == "POST": form_data = await request.form() + submission = form_data.get("data", form_data.get("DATA")) request_meta = { META_KEY_HTTP_CLIENT_IP: request.client.host if request.client else "", "HTTP-User-Agent": request.headers.get("User-Agent"), @@ -160,8 +161,10 @@ async def rpsl_update( # and therefore needs wrapping in a thread @sync_to_async def save(): + signed_submission, pgp_fingerprint = validate_pgp_signature(submission) return ChangeSubmissionHandler().load_text_blob( - object_texts_blob=form_data["data"], + object_texts_blob=signed_submission if signed_submission else submission, + pgp_fingerprint=pgp_fingerprint, origin=AuthoritativeChangeOrigin.webui, request_meta=request_meta, internal_authenticated_user=active_user, @@ -172,7 +175,7 @@ def save(): "rpsl_form.html", request, { - "existing_data": form_data["data"], + "existing_data": submission, "status": handler.status(), "report": handler.submitter_report_human(), "mntner_perms": mntner_perms, diff --git a/irrd/webui/templates/rpsl_form.html b/irrd/webui/templates/rpsl_form.html index 8f8d85f0..6cf05d37 100644 --- a/irrd/webui/templates/rpsl_form.html +++ b/irrd/webui/templates/rpsl_form.html @@ -28,8 +28,8 @@

Change/create/delete object(s){% if status %}: {{ status }}{% endif %}

This form is identical to email submissions, which means you can use the pseudo-attributes delete for deletions or password for password authentication. - PGP is not supported. - See the IRRD documentation for more details. + PGP inline signatures are supported. + See the IRRD documentation for more details.

{% if user and user.override %}

From 583bd6abc9ab2c7ae259776893892c31aae450bd Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Thu, 28 Nov 2024 15:34:29 +0100 Subject: [PATCH 3/6] Fix CSRF protection --- irrd/webui/endpoints.py | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/irrd/webui/endpoints.py b/irrd/webui/endpoints.py index c2759a3b..e56baafb 100644 --- a/irrd/webui/endpoints.py +++ b/irrd/webui/endpoints.py @@ -1,9 +1,10 @@ +import functools from collections import defaultdict from asgiref.sync import sync_to_async from starlette.requests import Request from starlette.responses import Response -from starlette_wtf import csrf_token +from starlette_wtf import csrf_protect, csrf_token from irrd import META_KEY_HTTP_CLIENT_IP from irrd.conf import get_setting @@ -102,10 +103,24 @@ async def rpsl_detail(request: Request, user_mfa_incomplete: bool, session_provi ) +def optional_csrf_protect(func): + @functools.wraps(func) + async def wrapper(*args, **kwargs): + try: + decorated_func = csrf_protect(func) + return await decorated_func(*args, csrf_protected=True, **kwargs) + except Exception as e: + print(f"Exception captured: {e}") + return await func(*args, csrf_protected=False, **kwargs) + + return wrapper + + +@optional_csrf_protect @mark_user_mfa_incomplete @session_provider_manager async def rpsl_update( - request: Request, user_mfa_incomplete: bool, session_provider: ORMSessionProvider + request: Request, user_mfa_incomplete: bool, session_provider: ORMSessionProvider, csrf_protected: bool ) -> Response: """ Web form for submitting RPSL updates. @@ -113,7 +128,11 @@ async def rpsl_update( but with pre-authentication through the logged in user or override. Can also be used anonymously. """ - active_user = request.auth.user if request.auth.is_authenticated and not user_mfa_incomplete else None + active_user = ( + request.auth.user + if request.auth.is_authenticated and not user_mfa_incomplete and csrf_protected + else None + ) mntner_perms = defaultdict(list) if active_user: for mntner in request.auth.user.mntners_user_management: From 52bea4067dfdc5cb99694c9b8f2e61efa4717492 Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Thu, 28 Nov 2024 16:27:09 +0100 Subject: [PATCH 4/6] Add testing for CSRF fail --- irrd/server/http/app.py | 7 ++- irrd/webui/endpoints.py | 13 ++++-- irrd/webui/tests/test_endpoints.py | 68 ++++++++++++++++++++++++++---- 3 files changed, 75 insertions(+), 13 deletions(-) diff --git a/irrd/server/http/app.py b/irrd/server/http/app.py index 68929a69..98d479d4 100644 --- a/irrd/server/http/app.py +++ b/irrd/server/http/app.py @@ -145,7 +145,8 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: def set_middleware(app): testing = os.environ.get("TESTING", False) - if testing: + csrf_disabled = testing and not getattr(app, "force_csrf_in_testing", False) + if csrf_disabled: logger.info("Running in testing mode, disabling CSRF.") app.user_middleware = [ # Use asgi-log to work around https://github.com/encode/uvicorn/issues/1384 @@ -157,7 +158,9 @@ def set_middleware(app): Middleware(MemoryTrimMiddleware), Middleware(SessionMiddleware, secret_key=secret_key_derive("web.session_middleware")), Middleware( - CSRFProtectMiddleware, csrf_secret=secret_key_derive("web.csrf_middleware"), enabled=not testing + CSRFProtectMiddleware, + csrf_secret=secret_key_derive("web.csrf_middleware"), + enabled=not csrf_disabled, ), auth_middleware, ] diff --git a/irrd/webui/endpoints.py b/irrd/webui/endpoints.py index e56baafb..44ab24e5 100644 --- a/irrd/webui/endpoints.py +++ b/irrd/webui/endpoints.py @@ -4,7 +4,7 @@ from asgiref.sync import sync_to_async from starlette.requests import Request from starlette.responses import Response -from starlette_wtf import csrf_protect, csrf_token +from starlette_wtf import CSRFError, csrf_protect, csrf_token from irrd import META_KEY_HTTP_CLIENT_IP from irrd.conf import get_setting @@ -104,13 +104,20 @@ async def rpsl_detail(request: Request, user_mfa_incomplete: bool, session_provi def optional_csrf_protect(func): + """ + The RPSL update endpoint is special re CSRF: it may be called from + a browser, with typically a valid CSRF token, or from an API call, + without CSRF. Therefore, this decorator tries to validate CSRF, + and if not, tells the endpoint, which will then ignore user session + info and only look at the post data. + """ + @functools.wraps(func) async def wrapper(*args, **kwargs): try: decorated_func = csrf_protect(func) return await decorated_func(*args, csrf_protected=True, **kwargs) - except Exception as e: - print(f"Exception captured: {e}") + except CSRFError: return await func(*args, csrf_protected=False, **kwargs) return wrapper diff --git a/irrd/webui/tests/test_endpoints.py b/irrd/webui/tests/test_endpoints.py index 88480e9d..206f9588 100644 --- a/irrd/webui/tests/test_endpoints.py +++ b/irrd/webui/tests/test_endpoints.py @@ -3,12 +3,14 @@ from unittest.mock import create_autospec import pytest +from starlette.testclient import TestClient from irrd.updates.handler import ChangeSubmissionHandler from irrd.utils.rpsl_samples import SAMPLE_MNTNER from irrd.webui import datetime_format from ...rpsl.rpsl_objects import rpsl_object_from_text +from ...server.http.app import app from ...storage.database_handler import DatabaseHandler from ...storage.models import JournalEntryOrigin from ...updates.parser_state import UpdateRequestType @@ -141,7 +143,11 @@ class TestRpslUpdateNoInitial(WebRequestTest): requires_mfa = False def test_valid_mntner_logged_in_mfa_complete_no_user_management( - self, irrd_db_session_with_user, test_client, mock_change_submission_handler + self, + irrd_db_session_with_user, + test_client, + mock_change_submission_handler, + tmp_gpg_dir, ): session_provider, user = irrd_db_session_with_user self._login(test_client, user) @@ -154,7 +160,11 @@ def test_valid_mntner_logged_in_mfa_complete_no_user_management( assert "(you can not update this mntner itself)" in response.text def test_valid_mntner_logged_in_mfa_complete_user_management( - self, irrd_db_session_with_user, test_client, mock_change_submission_handler + self, + irrd_db_session_with_user, + test_client, + mock_change_submission_handler, + tmp_gpg_dir, ): session_provider, user = irrd_db_session_with_user self._login(test_client, user) @@ -173,8 +183,37 @@ def test_valid_mntner_logged_in_mfa_complete_user_management( assert mock_handler_kwargs["object_texts_blob"] == SAMPLE_MNTNER assert mock_handler_kwargs["internal_authenticated_user"].pk == user.pk + def test_valid_mntner_logged_in_mfa_complete_user_management_no_csrf( + self, + irrd_db_session_with_user, + test_client, + mock_change_submission_handler, + tmp_gpg_dir, + ): + # print(test_client.app.user_middleware) + # raise Exception() + session_provider, user = irrd_db_session_with_user + self._login(test_client, user) + self._verify_mfa(test_client) + create_permission(session_provider, user) + + app.force_csrf_in_testing = True + with TestClient(app, cookies=test_client.cookies) as client_csrf: + app.force_csrf_in_testing = False + client_csrf.cookies = test_client.cookies + response = client_csrf.post(self.url, data={"data": SAMPLE_MNTNER}) + assert response.status_code == 200 + assert mock_change_submission_handler.mock_calls[1][0] == "().load_text_blob" + mock_handler_kwargs = mock_change_submission_handler.mock_calls[1][2] + assert mock_handler_kwargs["object_texts_blob"] == SAMPLE_MNTNER + assert mock_handler_kwargs["internal_authenticated_user"] is None + def test_valid_mntner_logged_in_mfa_incomplete_user_management( - self, irrd_db_session_with_user, test_client, mock_change_submission_handler + self, + irrd_db_session_with_user, + test_client, + mock_change_submission_handler, + tmp_gpg_dir, ): session_provider, user = irrd_db_session_with_user self._login(test_client, user) @@ -192,7 +231,11 @@ def test_valid_mntner_logged_in_mfa_incomplete_user_management( assert mock_handler_kwargs["internal_authenticated_user"] is None def test_valid_mntner_not_logged_in( - self, irrd_db_session_with_user, test_client, mock_change_submission_handler + self, + irrd_db_session_with_user, + test_client, + mock_change_submission_handler, + tmp_gpg_dir, ): session_provider, user = irrd_db_session_with_user response = test_client.get(self.url) @@ -213,7 +256,10 @@ class TestRpslUpdateWithInitial(WebRequestTest): requires_mfa = False def test_valid_mntner_logged_in_mfa_complete_no_user_management( - self, irrd_db_session_with_user, test_client + self, + irrd_db_session_with_user, + test_client, + tmp_gpg_dir, ): session_provider, user = irrd_db_session_with_user self._login(test_client, user) @@ -227,7 +273,10 @@ def test_valid_mntner_logged_in_mfa_complete_no_user_management( assert "DUMMYVALUE" in response.text.upper() def test_valid_mntner_logged_in_mfa_complete_user_management( - self, irrd_db_session_with_user, test_client + self, + irrd_db_session_with_user, + test_client, + tmp_gpg_dir, ): session_provider, user = irrd_db_session_with_user self._login(test_client, user) @@ -241,7 +290,10 @@ def test_valid_mntner_logged_in_mfa_complete_user_management( assert "DUMMYVALUE" not in response.text.upper() def test_valid_mntner_logged_in_mfa_incomplete_user_management( - self, irrd_db_session_with_user, test_client + self, + irrd_db_session_with_user, + test_client, + tmp_gpg_dir, ): session_provider, user = irrd_db_session_with_user self._login(test_client, user) @@ -252,7 +304,7 @@ def test_valid_mntner_logged_in_mfa_incomplete_user_management( assert "TEST-MNT" in response.text assert "DUMMYVALUE" in response.text.upper() - def test_valid_mntner_not_logged_in(self, irrd_db_session_with_user, test_client): + def test_valid_mntner_not_logged_in(self, irrd_db_session_with_user, test_client, tmp_gpg_dir): session_provider, user = irrd_db_session_with_user response = test_client.get(self.url) assert response.status_code == 200 From 98230b44af8faefb1491999a88b3e8278accc433 Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Mon, 2 Dec 2024 12:41:38 +0100 Subject: [PATCH 5/6] finetune --- irrd/utils/pgp.py | 2 +- irrd/webui/endpoints.py | 11 ++++++----- irrd/webui/tests/test_endpoints.py | 2 -- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/irrd/utils/pgp.py b/irrd/utils/pgp.py index a4cbb1fb..deff6d09 100644 --- a/irrd/utils/pgp.py +++ b/irrd/utils/pgp.py @@ -78,6 +78,6 @@ def validate_pgp_signature( log_message = result.stderr.replace("\n", " -- ").replace("gpg: ", "") logger.info(f"checked PGP signature, response: {log_message}") if result.valid and result.key_status is None: - logger.info(f"Found valid PGP signature, fingerprint {result.fingerprint}") + logger.info(f"Found valid PGP signature, pubkey fingerprint {result.pubkey_fingerprint}, fingerprint {result.fingerprint}") return new_message, result.pubkey_fingerprint return None, None diff --git a/irrd/webui/endpoints.py b/irrd/webui/endpoints.py index 44ab24e5..0f038b6d 100644 --- a/irrd/webui/endpoints.py +++ b/irrd/webui/endpoints.py @@ -105,9 +105,10 @@ async def rpsl_detail(request: Request, user_mfa_incomplete: bool, session_provi def optional_csrf_protect(func): """ - The RPSL update endpoint is special re CSRF: it may be called from - a browser, with typically a valid CSRF token, or from an API call, - without CSRF. Therefore, this decorator tries to validate CSRF, + The RPSL update endpoint is special re CSRF: while not entirely supported, + users sometimes call this through curl, and miss the CSRF token. + It does need CSRF protection, because a logged in user gets additional + mntner access. Therefore, this decorator tries to validate CSRF, and if not, tells the endpoint, which will then ignore user session info and only look at the post data. """ @@ -115,8 +116,8 @@ def optional_csrf_protect(func): @functools.wraps(func) async def wrapper(*args, **kwargs): try: - decorated_func = csrf_protect(func) - return await decorated_func(*args, csrf_protected=True, **kwargs) + csrf_protected_func = csrf_protect(func) + return await csrf_protected_func(*args, csrf_protected=True, **kwargs) except CSRFError: return await func(*args, csrf_protected=False, **kwargs) diff --git a/irrd/webui/tests/test_endpoints.py b/irrd/webui/tests/test_endpoints.py index 206f9588..fecde237 100644 --- a/irrd/webui/tests/test_endpoints.py +++ b/irrd/webui/tests/test_endpoints.py @@ -190,8 +190,6 @@ def test_valid_mntner_logged_in_mfa_complete_user_management_no_csrf( mock_change_submission_handler, tmp_gpg_dir, ): - # print(test_client.app.user_middleware) - # raise Exception() session_provider, user = irrd_db_session_with_user self._login(test_client, user) self._verify_mfa(test_client) From fba715e10e96bd0c347870afb4c0d314efce5068 Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Mon, 2 Dec 2024 12:42:33 +0100 Subject: [PATCH 6/6] rn --- docs/releases/4.5.0.rst | 2 ++ irrd/utils/pgp.py | 5 ++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/docs/releases/4.5.0.rst b/docs/releases/4.5.0.rst index 2560d5a6..7bbb1af6 100644 --- a/docs/releases/4.5.0.rst +++ b/docs/releases/4.5.0.rst @@ -57,6 +57,8 @@ as of 7 October 2024 and therefore no longer supported. Other changes ------------- +* The RPSL update submission page, on `/rpsl/update/`, now also accepts + inline PGP signed messages for mntner authentication. * The ``sources.{name}.object_class_filter`` setting can now also be used to restrict permitted changes to authoritative objects. * The setting ``sources.{name}.authoritative_retain_last_modified`` was diff --git a/irrd/utils/pgp.py b/irrd/utils/pgp.py index deff6d09..6613452d 100644 --- a/irrd/utils/pgp.py +++ b/irrd/utils/pgp.py @@ -78,6 +78,9 @@ def validate_pgp_signature( log_message = result.stderr.replace("\n", " -- ").replace("gpg: ", "") logger.info(f"checked PGP signature, response: {log_message}") if result.valid and result.key_status is None: - logger.info(f"Found valid PGP signature, pubkey fingerprint {result.pubkey_fingerprint}, fingerprint {result.fingerprint}") + logger.info( + f"Found valid PGP signature, pubkey fingerprint {result.pubkey_fingerprint}, fingerprint" + f" {result.fingerprint}" + ) return new_message, result.pubkey_fingerprint return None, None