Ensure signature checking for SAML Responses is enabled by default

[jkakavas]

Make sure that all Service Provider deployments based on pysaml2 are protected from SAML signature exclusion attacks by default.
Fixes #424

[jkakavas]

Yes sorry for that, working on it now. I'll update the PR

…

On Aug 1, 2017 13:55, "Johan Lundberg" ***@***.***> wrote: There are several tests failing on SignatureError: Signature missing for response. I guess that is expected but the tests needs to be updated. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#439 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJzhKIlbghqNoYawHEpkWrhTmQgzTm-Dks5sTwQ0gaJpZM4OpfZA> .

[johanlundberg]

There are several tests failing on SignatureError: Signature missing for response. I guess that is expected but the tests needs to be updated.

[c00kiemon5ter]

IMO this shouts for a refactor. What we want to achieve is use the configuration-provided values and if they are not set, use the default values. However, this piece of code does the exact opposite - it sets the defaults, then looks up the configuration-provided values. It also never mentions default or configuration which makes it harder to reason about. Lets fix this.

diff --git a/src/saml2/client_base.py b/src/saml2/client_base.py
index 531ddea..e3b7134 100644
--- a/src/saml2/client_base.py
+++ b/src/saml2/client_base.py
@@ -113,17 +113,25 @@ class Base(Entity):
         else:
             self.state = state_cache
 
-        self.logout_requests_signed = False
-        self.allow_unsolicited = False
-        self.authn_requests_signed = False
-        self.want_assertions_signed = False
-        self.want_response_signed = False
-        for foo in ["allow_unsolicited", "authn_requests_signed",
-                    "logout_requests_signed", "want_assertions_signed",
-                    "want_response_signed"]:
-            v = self.config.getattr(foo, "sp")
-            if v is True or v == 'true':
-                setattr(self, foo, True)
+        attribute_defaults = {
+            "logout_requests_signed": False,
+            "allow_unsolicited": False,
+            "authn_requests_signed": False,
+            "want_assertions_signed": False,
+            "want_response_signed": True,
+        }
+
+        for attr, val_default in attribute_defaults:
+            val_config = self.config.getattr(attr, "sp")
+            if val_config is None:
+                val = val_default
+            else:
+                val = val_config
+
+            if val == 'true':
+                val = True
+
+            setattr(self, attr, val)
 
         self.artifact2response = {}

[c00kiemon5ter]

Could we restore this whitespace? Formatting is already ugly in the source files, lets try to fix it by each new commit.

[jkakavas]

aye

[rohe]

I fixed a conflict and then one test fails. Could you fix @jkakavas ?

[jkakavas]

@rohe at your service!