From ebda84bcafcfb8f7824d0a71ad27c84be5e6ae52 Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Wed, 26 Dec 2018 21:56:08 +0100 Subject: [PATCH] saml_idp: cleanup, fix XML signing --- passbook/saml_idp/base.py | 7 ++- passbook/saml_idp/models.py | 8 ++- .../templates/saml/xml/assertions/generic.xml | 6 +-- .../templates/saml/xml/attributes.xml | 4 +- .../saml_idp/templates/saml/xml/metadata.xml | 2 +- .../saml_idp/templates/saml/xml/response.xml | 1 + .../saml_idp/templates/saml/xml/subject.xml | 2 +- passbook/saml_idp/urls.py | 4 +- passbook/saml_idp/views.py | 51 ++++--------------- passbook/saml_idp/xml_render.py | 2 +- passbook/saml_idp/xml_signing.py | 12 ++--- 11 files changed, 36 insertions(+), 63 deletions(-) diff --git a/passbook/saml_idp/base.py b/passbook/saml_idp/base.py index 177c3a1b1..a7d9f24b7 100644 --- a/passbook/saml_idp/base.py +++ b/passbook/saml_idp/base.py @@ -51,7 +51,7 @@ class Processor: _saml_response = None _session_index = None _subject = None - _subject_format = 'urn:oasis:names:tc:SAML:2.0:nameid-format:email' + _subject_format = 'urn:oasis:names:tc:SAML:2.0:nameid-format:persistent' _system_params = { 'ISSUER': CONFIG.y('saml_idp.issuer'), } @@ -84,8 +84,7 @@ class Processor: 'AUTH_INSTANT': get_time_string(), 'ISSUE_INSTANT': get_time_string(), 'NOT_BEFORE': get_time_string(-1 * HOURS), # TODO: Make these settings. - 'NOT_ON_OR_AFTER': get_time_string(int(CONFIG.y('saml_idp.assertion_valid_for')) - * MINUTES), + 'NOT_ON_OR_AFTER': get_time_string(86400 * MINUTES), 'SESSION_INDEX': self._session_index, 'SESSION_NOT_ON_OR_AFTER': get_time_string(8 * HOURS), 'SP_NAME_QUALIFIER': self._audience, @@ -226,7 +225,7 @@ class Processor: self._saml_response = sp_config self._session_index = sp_config self._subject = sp_config - self._subject_format = 'urn:oasis:names:tc:SAML:2.0:nameid-format:email' + self._subject_format = 'urn:oasis:names:tc:SAML:2.0:nameid-format:persistent' self._system_params = { 'ISSUER': CONFIG.y('saml_idp.issuer'), } diff --git a/passbook/saml_idp/models.py b/passbook/saml_idp/models.py index 318d1fc55..18ba9ccf3 100644 --- a/passbook/saml_idp/models.py +++ b/passbook/saml_idp/models.py @@ -40,8 +40,12 @@ class SAMLProvider(Provider): def link_download_metadata(self): """Get link to download XML metadata for admin interface""" - return reverse('passbook_saml_idp:metadata_xml', - kwargs={'provider_id': self.pk}) + # pylint: disable=no-member + if self.application: + # pylint: disable=no-member + return reverse('passbook_saml_idp:metadata_xml', + kwargs={'application': self.application.slug}) + return None class Meta: diff --git a/passbook/saml_idp/templates/saml/xml/assertions/generic.xml b/passbook/saml_idp/templates/saml/xml/assertions/generic.xml index 0d1e44643..1976b936c 100644 --- a/passbook/saml_idp/templates/saml/xml/assertions/generic.xml +++ b/passbook/saml_idp/templates/saml/xml/assertions/generic.xml @@ -5,14 +5,14 @@ {{ ISSUER }} {% include 'saml/xml/signature.xml' %} {{ SUBJECT_STATEMENT }} - + {{ AUDIENCE }} - + - urn:oasis:names:tc:SAML:2.0:ac:classes:Password + urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport {{ ATTRIBUTE_STATEMENT }} diff --git a/passbook/saml_idp/templates/saml/xml/attributes.xml b/passbook/saml_idp/templates/saml/xml/attributes.xml index da51589e8..054b96ca4 100644 --- a/passbook/saml_idp/templates/saml/xml/attributes.xml +++ b/passbook/saml_idp/templates/saml/xml/attributes.xml @@ -1,7 +1,7 @@ {% for attr in attributes %} - - {{ attr.Value }} + + {{ attr.Value }} {% endfor %} diff --git a/passbook/saml_idp/templates/saml/xml/metadata.xml b/passbook/saml_idp/templates/saml/xml/metadata.xml index 52e0a2092..2eff8fe23 100644 --- a/passbook/saml_idp/templates/saml/xml/metadata.xml +++ b/passbook/saml_idp/templates/saml/xml/metadata.xml @@ -16,7 +16,7 @@ - urn:oasis:names:tc:SAML:2.0:nameid-format:email + urn:oasis:names:tc:SAML:2.0:nameid-format:persistent {% comment %} diff --git a/passbook/saml_idp/templates/saml/xml/response.xml b/passbook/saml_idp/templates/saml/xml/response.xml index cf663f690..88d620d96 100644 --- a/passbook/saml_idp/templates/saml/xml/response.xml +++ b/passbook/saml_idp/templates/saml/xml/response.xml @@ -1,4 +1,5 @@ - + {{ SUBJECT }} diff --git a/passbook/saml_idp/urls.py b/passbook/saml_idp/urls.py index 7b4a394e1..81f99ca3a 100644 --- a/passbook/saml_idp/urls.py +++ b/passbook/saml_idp/urls.py @@ -6,8 +6,8 @@ from passbook.saml_idp import views urlpatterns = [ path('login//', views.LoginBeginView.as_view(), name="saml_login_begin"), - path('login//idp_init/', - views.LoginInitView.as_view(), name="saml_login_init"), + path('login//initiate/', + views.InitiateLoginView.as_view(), name="saml_login_init"), path('login//process/', views.LoginProcessView.as_view(), name='saml_login_process'), path('logout/', views.LogoutView.as_view(), name="saml_logout"), diff --git a/passbook/saml_idp/views.py b/passbook/saml_idp/views.py index a9ad3f7ce..20cf133a1 100644 --- a/passbook/saml_idp/views.py +++ b/passbook/saml_idp/views.py @@ -1,7 +1,6 @@ """passbook SAML IDP Views""" from logging import getLogger -from django.conf import settings from django.contrib.auth import logout from django.contrib.auth.mixins import LoginRequiredMixin from django.core.exceptions import ValidationError @@ -10,14 +9,6 @@ from django.http import HttpResponse, HttpResponseBadRequest from django.shortcuts import get_object_or_404, redirect, render, reverse from django.utils.datastructures import MultiValueDictKeyError from django.views import View -from saml2 import BINDING_HTTP_POST -from saml2.authn_context import PASSWORD, AuthnBroker, authn_context_class_ref -from saml2.config import IdPConfig -from saml2.ident import NameID -from saml2.metadata import entity_descriptor -from saml2.s_utils import UnknownPrincipal, UnsupportedBinding -from saml2.saml import NAMEID_FORMAT_EMAILADDRESS, NAMEID_FORMAT_UNSPECIFIED -from saml2.server import Server from signxml.util import strip_pem_header from passbook.core.models import Application @@ -50,11 +41,13 @@ def render_xml(request, template, ctx): class ProviderMixin: + """Mixin class for Views using a provider instance""" _provider = None @property def provider(self): + """Get provider instance""" if not self._provider: application = get_object_or_404(Application, slug=self.kwargs['application']) self._provider = get_object_or_404(SAMLProvider, pk=application.provider_id) @@ -123,7 +116,7 @@ class LoginProcessView(ProviderMixin, View): saml_response=request.POST.get('SAMLResponse'), relay_state=request.POST.get('RelayState')) try: - full_res = _generate_response(request, provider) + full_res = _generate_response(request, self.provider) return full_res except exceptions.CannotHandleAssertion as exc: LOGGER.debug(exc) @@ -166,33 +159,17 @@ class SLOLogout(CSRFExemptMixin, LoginRequiredMixin, View): logout(request) return render(request, 'saml/idp/logged_out.html') -class IdPMixin(ProviderMixin): - - provider = None - - def dispatch(self, request, application): - - def get_identity(self, provider, user): - """ Create Identity dict (using SP-specific mapping) - """ - sp_mapping = {'username': 'username'} - # return provider.processor.create_identity(user, sp_mapping) - return { - out_attr: getattr(user, user_attr) - for user_attr, out_attr in sp_mapping.items() - if hasattr(user, user_attr) - } - class DescriptorDownloadView(ProviderMixin, View): """Replies with the XML Metadata IDSSODescriptor.""" def get(self, request, application): """Replies with the XML Metadata IDSSODescriptor.""" - super().dispatch(request, application) entity_id = CONFIG.y('saml_idp.issuer') slo_url = request.build_absolute_uri(reverse('passbook_saml_idp:saml_logout')) - sso_url = request.build_absolute_uri(reverse('passbook_saml_idp:saml_login_begin')) + sso_url = request.build_absolute_uri(reverse('passbook_saml_idp:saml_login_begin', kwargs={ + 'application': application + })) pubkey = strip_pem_header(self.provider.signing_cert.replace('\r', '')).replace('\n', '') ctx = { 'entity_id': entity_id, @@ -207,19 +184,11 @@ class DescriptorDownloadView(ProviderMixin, View): return response -class LoginInitView(IdPMixin, LoginRequiredMixin, View): +class InitiateLoginView(ProviderMixin, LoginRequiredMixin, View): + """IdP-initiated Login""" def dispatch(self, request, application): """Initiates an IdP-initiated link to a simple SP resource/target URL.""" super().dispatch(request, application) - - # # linkdict = dict(metadata.get_links(sp_config)) - # # pattern = linkdict[resource] - # # is_simple_link = ('/' not in resource) - # # if is_simple_link: - # # simple_target = kwargs['target'] - # # url = pattern % simple_target - # # else: - # # url = pattern % kwargs - # provider.processor.init_deep_link(request, 'deep url') - # return _generate_response(request, provider) + self.provider.processor.init_deep_link(request, '') + return _generate_response(request, self.provider) diff --git a/passbook/saml_idp/xml_render.py b/passbook/saml_idp/xml_render.py index 92f1ac4f3..a8cb517c3 100644 --- a/passbook/saml_idp/xml_render.py +++ b/passbook/saml_idp/xml_render.py @@ -83,5 +83,5 @@ def get_response_xml(parameters, saml_provider: 'SAMLProvider', assertion_id='') signed = sign_with_signxml( saml_provider.signing_key, raw_response, saml_provider.signing_cert, - reference_uri=assertion_id).decode("utf-8") + reference_uri=assertion_id) return signed diff --git a/passbook/saml_idp/xml_signing.py b/passbook/saml_idp/xml_signing.py index 78f2d48ff..334cf3ac9 100644 --- a/passbook/saml_idp/xml_signing.py +++ b/passbook/saml_idp/xml_signing.py @@ -3,9 +3,8 @@ from logging import getLogger from cryptography.hazmat.backends import default_backend from cryptography.hazmat.primitives import serialization -from defusedxml import ElementTree from lxml import etree # nosec -from signxml import XMLSigner +from signxml import XMLSigner, XMLVerifier from passbook.lib.utils.template import render_to_string @@ -17,12 +16,13 @@ def sign_with_signxml(private_key, data, cert, reference_uri=None): key = serialization.load_pem_private_key( str.encode('\n'.join([x.strip() for x in private_key.split('\n')])), password=None, backend=default_backend()) - # LXML is used here because defusedxml causes issues with serialization - # data is trusted so no issues + # defused XML is not used here because it messes up XML namespaces + # Data is trusted, so lxml is ok root = etree.fromstring(data) # nosec signer = XMLSigner(c14n_algorithm='http://www.w3.org/2001/10/xml-exc-c14n#') - signed = signer.sign(root, key=key, cert=cert, reference_uri=reference_uri) - return ElementTree.tostring(signed) + signed = signer.sign(root, key=key, cert=[cert], reference_uri=reference_uri) + XMLVerifier().verify(signed, x509_cert=cert) + return etree.tostring(signed).decode('utf-8') # nosec def get_signature_xml():