add option to disable Signature on AuthnRequest v1 PROPOSED

Peter Brand: 1
 add option to disable Signature on AuthnRequest

 4 files changed, 29 insertions(+), 9 deletions(-)

[PATCH] add option to disable Signature on AuthnRequest Export this patch

Signing AuthnRequests is only useful if there's information included in the
AuthnRequest that must not be tampered with, e.g. to prevend someone from
sending an alternative AuthnRequest that's missing a required
AuthnContextClassRef or has disabled ForceAuthn when that's required.

François Kooman <fkooman@tuxed.net>

4 months ago

You are correct. However, providing a toggle to admins increases the 
risk they'll shoot themselves in the foot in a possible future where ACR 
or "ForceAuthn" is enabled and they forget to (re)enable signatures.

Also having signatures always enabled makes it less likely the IdP will 
stop working when signatures are (all of a sudden) enabled.

Next

(Note that the IDP can still ensure that the ACS URL from the AuthnRequest "is
in fact associated with the requester", commonly via SAML 2.0 Metadata. SAML
Core 3.4.1 actually says the IDP MUST ensure this.)

While it not always being useful may still be good enough to always keep signing,
this also exposes the SP to trivial unauthenticaetd DoS attacks: Every
(computationally cheap) HTTP GET request to a protected resource (or to a
SessionInitiator or other local URL that initiates a login) results in a
(computationally comparably expensive) cryptographic signing operation.

François Kooman <fkooman@tuxed.net>

4 months ago

If this indeed increases the ease with with DoS attacks can be 
performed, i.e. would be lower hanging fruit compared to other readily 
available DoS approaches, e.g. saturating TLS connections or something, 
it might be worth considering this, or working on mitigating mechanisms, 
e.g. rate limiting or indeed disabling AuthnRequest signatures.

Next

So an option was added to allow a deployment decision to be made whether
protecing the content of generated AuthnRequests is worth the added risk of
signing them. That option of course defaults to the current behaviour of
always signing AuthnRequests.

François Kooman <fkooman@tuxed.net>

4 months ago

We prefer keeping always signing for now, unless there is a real need to 
be able to allow disabling it.

Thanks for your patch!

Regards,
François

TODO:
* Dynamically unset SPSSODescriptor/@AuthnRequestsSigned in
  src/tpl/Metadata.php when the new option `signAuthnRequest` is set to false.
* Adapt tests/SPTest.php
---
 README.md                 |  2 +-
 config/config.php.example |  4 ++++
 src/SP.php                | 20 ++++++++++++--------
 src/Web/Config.php        | 12 ++++++++++++
 4 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/README.md b/README.md
index 7bc706c..b69094c 100644
--- a/README.md
+++ b/README.md
@@ -39,7 +39,7 @@ which is secure. So you won't find SHA1 support or insecure encryption.
 - Only HTTP-Redirect for sending `AuthnRequest`, `LogoutRequest` to IdP
 - Only HTTP-Redirect binding for receiving `LogoutResponse` from IdP
 - Only HTTP-POST binding for receiving `Response` from IdP
-- Always signs `AuthnRequest`
+- Signs `AuthnRequest` by default
 - Always signs `LogoutRequest`
 - Supports signed `samlp:Response` and/or signed 
   `samlp:Response/saml:Assertion`
diff --git a/config/config.php.example b/config/config.php.example
index 4906796..b8e77dc 100644
--- a/config/config.php.example
+++ b/config/config.php.example
@@ -13,6 +13,10 @@ return [
     // DEFAULT: false
     //'requireEncryption' => false,
 
+    // sign AuthnRequest using HTTP-Redirect Protocol Binding
+    // DEFAULT: true
+    //'signAuthnRequest' => true,
+
     // the default language of the UI
     // DEFAULT: 'en-US'
     //'defaultLanguage' => 'en-US',
diff --git a/src/SP.php b/src/SP.php
index ed60903..faa4816 100644
--- a/src/SP.php
+++ b/src/SP.php
@@ -26,6 +26,7 @@ namespace fkooman\SAML\SP;
 
 use DateTime;
 use DateTimeZone;
+use fkooman\SAML\SP\Web\Config;
 use fkooman\SAML\SP\Exception\SpException;
 
 /**
@@ -337,16 +338,19 @@ class SP
         $httpQueryParameters = [
             'SAMLRequest' => Base64::encode($deflatedXml),
             'RelayState' => $relayState,
-            'SigAlg' => 'http://www.w3.org/2001/04/xmldsig-more#rsa-sha256',
         ];
 
-        // add the Signature key/value to the HTTP query
-        $httpQueryParameters['Signature'] = Base64::encode(
-            Crypto::sign(
-                \http_build_query($httpQueryParameters),
-                $privateKey
-            )
-        );
+        $config = Config::fromFile('/usr/share/php-saml-sp/config/config.php');
+        if (true === $config->getSignAuthnRequest()) {
+            // add the Signature key/value to the HTTP query
+            $httpQueryParameters['SigAlg'] = 'http://www.w3.org/2001/04/xmldsig-more#rsa-sha256';
+            $httpQueryParameters['Signature'] = Base64::encode(
+                Crypto::sign(
+                    \http_build_query($httpQueryParameters),
+                    $privateKey
+                )
+            );
+        }
 
         return \sprintf(
             '%s%s%s',
diff --git a/src/Web/Config.php b/src/Web/Config.php
index 5de2a01..56a50c5 100644
--- a/src/Web/Config.php
+++ b/src/Web/Config.php
@@ -87,6 +87,18 @@ class Config
         return $requireEncryption;
     }
 
+    /**
+     * @return bool
+     */
+    public function getSignAuthnRequest()
+    {
+        if (null === $signAuthnRequest = $this->get('signAuthnRequest')) {
+            return true;
+        }
+
+        return $signAuthnRequest;
+    }
+
     /**
      * @param string $configFile
      *
-- 
2.30.2