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>

4 files changed, 60 insertions, 14 deletions

diff --git a/src/saml2/client.py b/src/saml2/client.py
index e8990a46..65491a24 100644
--- a/src/saml2/client.py
+++ b/src/saml2/client.py
@@ -49,6 +49,7 @@ class Saml2Client(Base):
         scoping=None,
         consent=None, extensions=None,
         sign=None,
+        sigalg=None,
         response_binding=saml2.BINDING_HTTP_POST,
         **kwargs,
     ):
@@ -79,6 +80,7 @@ class Saml2Client(Base):
             consent=consent,
             extensions=extensions,
             sign=sign,
+            sigalg=sigalg,
             response_binding=response_binding,
             **kwargs,
         )
@@ -104,6 +106,7 @@ class Saml2Client(Base):
         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 @@ class Saml2Client(Base):
             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 @@ class Saml2Client(Base):
                 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 @@ class Saml2Client(Base):
                 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 @@ class Saml2Client(Base):
                     destination,
                     relay_state,
                     sign=sign,
-                    sigalg=sigalg,
+                    sigalg=sign_alg,
                 )
 
                 if binding == BINDING_SOAP:
diff --git a/src/saml2/client_base.py b/src/saml2/client_base.py
index d33bbbf0..c82b978f 100644
--- a/src/saml2/client_base.py
+++ b/src/saml2/client_base.py
@@ -54,6 +54,8 @@ from saml2 import BINDING_HTTP_REDIRECT
 from saml2 import BINDING_HTTP_POST
 from saml2 import BINDING_PAOS
 
+import saml2.xmldsig as ds
+
 
 logger = logging.getLogger(__name__)
 
@@ -444,7 +446,13 @@ class Base(Entity):
 
         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:
diff --git a/src/saml2/entity.py b/src/saml2/entity.py
index af2d8ba4..57d2a55a 100644
--- a/src/saml2/entity.py
+++ b/src/saml2/entity.py
@@ -72,6 +72,9 @@ from saml2.sigver import pre_encrypt_assertion
 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 @@ class Entity(HTTPBase):
         relay_state="",
         response=False,
         sign=None,
+        sigalg=None,
         **kwargs,
     ):
         """
@@ -212,6 +216,22 @@ class Entity(HTTPBase):
         :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 @@ class Entity(HTTPBase):
             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 @@ class Entity(HTTPBase):
                 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:
diff --git a/src/saml2/pack.py b/src/saml2/pack.py
index 7b693f28..e2cb2a8d 100644
--- a/src/saml2/pack.py
+++ b/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(