From 0ff4545babd8879d10f3e7fb90d7e6c416eecf9d Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Sun, 12 Jul 2020 16:17:53 +0200 Subject: [PATCH] providers/saml: fix AuthnRequest Signature validation, add unittests --- .../saml/processors/request_parser.py | 66 +++++++++++++++---- .../saml/tests/test_auth_n_request.py | 55 ++++++++++++++++ passbook/providers/saml/views.py | 6 +- 3 files changed, 113 insertions(+), 14 deletions(-) create mode 100644 passbook/providers/saml/tests/test_auth_n_request.py diff --git a/passbook/providers/saml/processors/request_parser.py b/passbook/providers/saml/processors/request_parser.py index 099f78f15..b37d0ffdb 100644 --- a/passbook/providers/saml/processors/request_parser.py +++ b/passbook/providers/saml/processors/request_parser.py @@ -1,8 +1,12 @@ """SAML AuthNRequest Parser and dataclass""" +from base64 import b64decode from dataclasses import dataclass from typing import Optional +from urllib.parse import quote_plus from cryptography.exceptions import InvalidSignature +from cryptography.hazmat.primitives import hashes +from cryptography.hazmat.primitives.asymmetric import padding from defusedxml import ElementTree from signxml import XMLVerifier from structlog import get_logger @@ -21,7 +25,7 @@ class AuthNRequest: # pylint: disable=invalid-name id: Optional[str] = None - relay_state: str = "" + relay_state: Optional[str] = None class AuthNRequestParser: @@ -32,18 +36,7 @@ class AuthNRequestParser: def __init__(self, provider: SAMLProvider): self.provider = provider - def parse(self, saml_request: str, relay_state: str) -> AuthNRequest: - """Parses various parameters from _request_xml into _request_params.""" - - decoded_xml = decode_base64_and_inflate(saml_request) - - if self.provider.require_signing and self.provider.signing_kp: - try: - XMLVerifier().verify( - decoded_xml, x509_cert=self.provider.signing_kp.certificate_data - ) - except InvalidSignature as exc: - raise CannotHandleAssertion("Failed to verify signature") from exc + def _parse_xml(self, decoded_xml: str, relay_state: Optional[str]) -> AuthNRequest: root = ElementTree.fromstring(decoded_xml) @@ -60,6 +53,53 @@ class AuthNRequestParser: auth_n_request = AuthNRequest(id=root.attrib["ID"], relay_state=relay_state) return auth_n_request + def parse(self, saml_request: str, relay_state: Optional[str]) -> AuthNRequest: + """Validate and parse raw request with enveloped signautre.""" + decoded_xml = decode_base64_and_inflate(saml_request) + + if self.provider.signing_kp: + try: + XMLVerifier().verify( + decoded_xml, x509_cert=self.provider.signing_kp.certificate_data + ) + except InvalidSignature as exc: + raise CannotHandleAssertion("Failed to verify signature") from exc + + return self._parse_xml(decoded_xml, relay_state) + + def parse_detached( + self, + saml_request: str, + relay_state: Optional[str], + signature: Optional[str] = None, + sig_alg: Optional[str] = None, + ) -> AuthNRequest: + """Validate and parse raw request with detached signature""" + decoded_xml = decode_base64_and_inflate(saml_request) + + if signature and sig_alg: + # if sig_alg == "http://www.w3.org/2000/09/xmldsig#rsa-sha1": + sig_hash = hashes.SHA1() # nosec + + querystring = f"SAMLRequest={quote_plus(saml_request)}&" + if relay_state is not None: + querystring += f"RelayState={quote_plus(relay_state)}&" + querystring += f"SigAlg={sig_alg}" + + public_key = self.provider.signing_kp.private_key.public_key() + try: + public_key.verify( + b64decode(signature), + querystring.encode(), + padding.PSS( + mgf=padding.MGF1(sig_hash), salt_length=padding.PSS.MAX_LENGTH + ), + sig_hash, + ) + except InvalidSignature as exc: + raise CannotHandleAssertion("Failed to verify signature") from exc + return self._parse_xml(decoded_xml, relay_state) + def idp_initiated(self) -> AuthNRequest: """Create IdP Initiated AuthNRequest""" return AuthNRequest() diff --git a/passbook/providers/saml/tests/test_auth_n_request.py b/passbook/providers/saml/tests/test_auth_n_request.py new file mode 100644 index 000000000..eeec1e85b --- /dev/null +++ b/passbook/providers/saml/tests/test_auth_n_request.py @@ -0,0 +1,55 @@ +"""Test AuthN Request generator and parser""" +from django.test import RequestFactory, TestCase + +from passbook.crypto.models import CertificateKeyPair +from passbook.flows.models import Flow +from passbook.providers.saml.models import SAMLProvider +from passbook.providers.saml.processors.request_parser import AuthNRequestParser +from passbook.providers.saml.utils.encoding import deflate_and_base64_encode +from passbook.sources.saml.models import SAMLSource +from passbook.sources.saml.processors.request import RequestProcessor + + +class TestAuthNRequest(TestCase): + """Test AuthN Request generator and parser""" + + def setUp(self): + self.provider = SAMLProvider.objects.create( + authorization_flow=Flow.objects.get( + slug="default-provider-authorization-implicit-consent" + ), + acs_url="http://testserver/source/saml/provider/acs/", + signing_kp=CertificateKeyPair.objects.first(), + ) + self.source = SAMLSource.objects.create( + slug="provider", + issuer="passbook", + signing_kp=CertificateKeyPair.objects.first(), + ) + self.factory = RequestFactory() + + def test_signed_valid(self): + """Test generated AuthNRequest with valid signature""" + http_request = self.factory.get("/") + # First create an AuthNRequest + request_proc = RequestProcessor(self.source, http_request, "test_state") + request = request_proc.build_auth_n() + # Now we check the ID and signature + parsed_request = AuthNRequestParser(self.provider).parse( + deflate_and_base64_encode(request), "test_state" + ) + self.assertEqual(parsed_request.id, request_proc.request_id) + self.assertEqual(parsed_request.relay_state, "test_state") + + def test_signed_valid_detached(self): + """Test generated AuthNRequest with valid signature (detached)""" + http_request = self.factory.get("/") + # First create an AuthNRequest + request_proc = RequestProcessor(self.source, http_request, "test_state") + params = request_proc.build_auth_n_detached() + # Now we check the ID and signature + parsed_request = AuthNRequestParser(self.provider).parse_detached( + params["SAMLRequest"], "test_state", params["Signature"], params["SigAlg"] + ) + self.assertEqual(parsed_request.id, request_proc.request_id) + self.assertEqual(parsed_request.relay_state, "test_state") diff --git a/passbook/providers/saml/views.py b/passbook/providers/saml/views.py index 2ec095148..f8745a8d1 100644 --- a/passbook/providers/saml/views.py +++ b/passbook/providers/saml/views.py @@ -39,6 +39,8 @@ from passbook.stages.consent.stage import PLAN_CONTEXT_CONSENT_TEMPLATE LOGGER = get_logger() URL_VALIDATOR = URLValidator(schemes=("http", "https")) SESSION_KEY_SAML_REQUEST = "SAMLRequest" +SESSION_KEY_SAML_SIGNATURE = "Signature" +SESSION_KEY_SAML_SIG_ALG = "SigAlg" SESSION_KEY_SAML_RESPONSE = "SAMLResponse" SESSION_KEY_RELAY_STATE = "RelayState" SESSION_KEY_AUTH_N_REQUEST = "authn_request" @@ -102,9 +104,11 @@ class SAMLSSOBindingRedirectView(SAMLSSOView): ) try: - auth_n_request = AuthNRequestParser(self.provider).parse( + auth_n_request = AuthNRequestParser(self.provider).parse_detached( request.GET[SESSION_KEY_SAML_REQUEST], request.GET.get(SESSION_KEY_RELAY_STATE, ""), + request.GET.get(SESSION_KEY_SAML_SIGNATURE), + request.GET.get(SESSION_KEY_SAML_SIG_ALG), ) self.request.session[SESSION_KEY_AUTH_N_REQUEST] = auth_n_request except CannotHandleAssertion as exc: