Correctly sign an AuthnRequest with Redirect binding

When an AuthnRequest is created with HTTP-Redirect binding, the XML document is not
signed, but instead, a signature is calculated and becomes part of the query params of
the Redirect-URL, through the Signature and SignAlg params.

Previously, when the Redirect binding was requested and signing was enabled but no
SignAlg params were defined, the Signature and SignAlg query params would be missing.

Now, if no SignAlg is defined, the default is used and the request is correctly created
with the proper query params.

Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>

Author: c00kiemon5ter

src/saml2/client.py

@@ -49,6 +49,7 @@ def prepare_for_authenticate(
         scoping=None,
         consent=None, extensions=None,
         sign=None,
+        sigalg=None,
         response_binding=saml2.BINDING_HTTP_POST,
         **kwargs,
     ):
@@ -79,6 +80,7 @@ def prepare_for_authenticate(
             consent=consent,
             extensions=extensions,
             sign=sign,
+            sigalg=sigalg,
             response_binding=response_binding,
             **kwargs,
         )
@@ -104,6 +106,7 @@ def prepare_for_negotiated_authenticate(
         extensions=None,
         sign=None,
         response_binding=saml2.BINDING_HTTP_POST,
+        sigalg=None,
         **kwargs,
     ):
         """ Makes all necessary preparations for an authentication request
@@ -133,6 +136,13 @@ def prepare_for_negotiated_authenticate(
             destination = self._sso_location(entityid, binding)
             logger.info("destination to provider: %s", destination)
 
+            # XXX - sign_post will embed the signature to the xml doc
+            # XXX   ^through self.create_authn_request(...)
+            # XXX - sign_redirect will add the signature to the query params
+            # XXX   ^through self.apply_binding(...)
+            sign_post = (binding == BINDING_HTTP_POST and sign)
+            sign_redirect = (binding == BINDING_HTTP_REDIRECT and sign)
+
             reqid, request = self.create_authn_request(
                 destination,
                 vorg,
@@ -141,25 +151,21 @@ def prepare_for_negotiated_authenticate(
                 nameid_format,
                 consent=consent,
                 extensions=extensions,
-                sign=sign,
+                sign=sign_post,
+                sign_alg=sigalg,
                 **kwargs,
             )
 
             _req_str = str(request)
             logger.info("AuthNReq: %s", _req_str)
 
-            try:
-                args = {'sigalg': kwargs["sigalg"]}
-            except KeyError:
-                args = {}
-
             http_info = self.apply_binding(
                 binding,
                 _req_str,
                 destination,
                 relay_state,
-                sign=sign,
-                **args,
+                sign=sign_redirect,
+                sigalg=sigalg,
             )
 
             return reqid, binding, http_info
@@ -258,11 +264,16 @@ def do_logout(self, name_id, entity_ids, reason, expire, sign=None,
                 if sign is None:
                     sign = self.logout_requests_signed
 
-                sigalg = None
+                def_sig = ds.DefaultSignature()
+                sign_alg = def_sig.get_sign_alg() if sign_alg is None else sign_alg
+                digest_alg = (
+                    def_sig.get_digest_alg()
+                    if digest_alg is None
+                    else digest_alg
+                )
+
                 if sign:
                     if binding == BINDING_HTTP_REDIRECT:
-                        sigalg = kwargs.get("sigalg", ds.DefaultSignature().get_sign_alg())
-                        # key = kwargs.get("key", self.signkey)
                         srequest = str(request)
                     else:
                         srequest = self.sign(
@@ -279,7 +290,7 @@ def do_logout(self, name_id, entity_ids, reason, expire, sign=None,
                     destination,
                     relay_state,
                     sign=sign,
-                    sigalg=sigalg,
+                    sigalg=sign_alg,
                 )
 
                 if binding == BINDING_SOAP:

src/saml2/client_base.py

@@ -54,6 +54,8 @@
 from saml2 import BINDING_HTTP_POST
 from saml2 import BINDING_PAOS
 
+import saml2.xmldsig as ds
+
 
 logger = logging.getLogger(__name__)
 
@@ -444,7 +446,13 @@ def create_authn_request(
 
         client_crt = kwargs.get("client_crt")
         nsprefix = kwargs.get("nsprefix")
+
+        # XXX will be used to embed the signature to the xml doc - ie, POST binding
+        # XXX always called by the SP, no need to check the context
         sign = self.authn_requests_signed if sign is None else sign
+        def_sig = ds.DefaultSignature()
+        sign_alg = sign_alg or def_sig.get_sign_alg()
+        digest_alg = digest_alg or def_sig.get_digest_alg()
 
         if (sign and self.sec.cert_handler.generate_cert()) or client_crt is not None:
             with self.lock:

src/saml2/entity.py

@@ -72,6 +72,9 @@
 from saml2.sigver import signed_instance_factory
 from saml2.virtual_org import VirtualOrg
 
+import saml2.xmldsig as ds
+
+
 logger = logging.getLogger(__name__)
 
 __author__ = 'rolandh'
@@ -198,6 +201,7 @@ def apply_binding(
         relay_state="",
         response=False,
         sign=None,
+        sigalg=None,
         **kwargs,
     ):
         """
@@ -212,6 +216,22 @@ def apply_binding(
         :param kwargs: response type specific arguments
         :return: A dictionary
         """
+
+        # XXX authn_requests_signed (obj property) applies only to an SP
+        # XXX sign_response (config option) applies to idp/aa
+        # XXX Looking into sp/idp/aa properties should be done in the same way
+        # XXX ^this discrepancy should be fixed
+        sign_config = (
+            self.authn_requests_signed
+            if self.config.context == "sp"
+            else self.config.getattr("sign_response")
+            if self.config.context == "idp"
+            else None
+        )
+        sign = sign_config if sign is None else sign
+        def_sig = ds.DefaultSignature()
+        sigalg = sigalg or def_sig.get_sign_alg()
+
         # unless if BINDING_HTTP_ARTIFACT
         if response:
             typ = "SAMLResponse"
@@ -231,7 +251,6 @@ def apply_binding(
             info["method"] = "POST"
         elif binding == BINDING_HTTP_REDIRECT:
             logger.info("HTTP REDIRECT")
-            sigalg = kwargs.get("sigalg")
             signer = (
                 self.sec.sec_backend.get_signer(sigalg)
                 if sign and sigalg
@@ -243,12 +262,15 @@ def apply_binding(
                 relay_state,
                 typ,
                 signer=signer,
+                sigalg=sigalg,
                 **kwargs,
             )
             info["url"] = str(destination)
             info["method"] = "GET"
         elif binding == BINDING_SOAP or binding == BINDING_PAOS:
-            info = self.use_soap(msg_str, destination, sign=sign, **kwargs)
+            info = self.use_soap(
+                msg_str, destination, sign=sign, sigalg=sigalg, **kwargs
+            )
         elif binding == BINDING_URI:
             info = self.use_http_uri(msg_str, typ, destination)
         elif binding == BINDING_HTTP_ARTIFACT:

src/saml2/pack.py

@@ -178,7 +178,12 @@ def http_redirect_message(message, location, relay_state="", typ="SAMLRequest",
     if relay_state:
         args["RelayState"] = relay_state
 
+    # XXX !should not depend on signer, but on sign
+    # XXX if both signalg and signer are here they have to match
+    # XXX now we allow them to differ
+    # XXX signer should be created here; not passed in
     if signer:
+        # XXX check for allowed algo -- should do the same for POST binding
         # sigalgs, should be one defined in xmldsig
         if sigalg not in [long_name for short_name, long_name in SIG_ALLOWED_ALG]:
             raise Exception(