#48 Add per-SP option to verify SAML signatures
Closed 5 years ago by puiterwijk. Opened 5 years ago by rcritten.
rcritten/ipsilon signatures  into  master

@@ -227,7 +227,8 @@ 

                                   'Allowed NameIDs', 'Attribute Mapping',

                                   'Allowed Attributes', 'Description',

                                   'Service Provider link',

-                                  'Visible in Portal', 'Image File']:

+                                  'Visible in Portal', 'Image File',

+                                  'Verify Signatures']:

                          if not self.user.is_admin:

                              raise UnauthorizedUser(

                                  "Unauthorized to set %s" % key
@@ -240,7 +241,8 @@ 

                      value = new_db_values.get(name, False)

                      # A value of None means remove from the data store

                      if ((value is False or value == []) and

-                             name != 'Visible in Portal'):

+                             name not in ['Visible in Portal',

+                                          'Verify Signatures']):

                          continue

                      if name == 'Name':

                          if not self.sp.is_valid_name(value):
@@ -277,6 +279,8 @@ 

                              raise InvalidValueFormat(

                                  'Invalid Image file format'

                              )

+                     elif name == 'Verify Signatures':

+                         self.sp.verify_signatures = value

  

              except InvalidValueFormat, e:

                  message = str(e)

@@ -69,12 +69,28 @@ 

              self.saml2error(login, e.code, e.message)

          return self.reply(login)

  

-     def _parse_request(self, message):

+     def _parse_request(self, message, hint=None, final=False):

  

          login = self.cfg.idp.get_login_handler()

  

          try:

+             if hint:

+                 login.setSignatureVerifyHint(hint)

              login.processAuthnRequestMsg(message)

+         except lasso.DsInvalidSigalgError as e:

+             if login.remoteProviderId and not final:

+                 provider = ServiceProvider(self.cfg, login.remoteProviderId)

+                 if not provider.verify_signatures:

+                     self.error('Invalid or missing signature, setting hint.')

+                     return self._parse_request(

+                         message,

+                         hint=provider.get_signature_hint(),

+                         final=True

+                     )

+             msg = 'Invalid or missing signature algorithm %r [%r]' % (

+                 e, message

+             )

+             raise InvalidRequest(msg)

          except (lasso.ProfileInvalidMsgError,

                  lasso.ProfileMissingIssuerError), e:

  

@@ -36,6 +36,11 @@ 

  

          try:

              logout.processRequestMsg(message)

+         except lasso.DsInvalidSigalgError as e:

+             msg = 'Invalid or missing signature algorithm %r [%r]' % (

+                 e, message

+             )

+             raise InvalidRequest(msg)

          except (lasso.ServerProviderNotFoundError,

                  lasso.ProfileUnknownProviderError) as e:

              msg = 'Invalid SP [%s] (%r [%r])' % (logout.remoteProviderId,
@@ -113,6 +118,11 @@ 

  

          try:

              logout.processResponseMsg(message)

+         except lasso.ProfileCannotVerifySignatureError as e:

+             msg = 'Invalid or missing signature algorithm %r [%r]' % (

+                 e, message

+             )

+             raise InvalidRequest(msg)

          except getattr(lasso, 'ProfileRequestDeniedError',

                         lasso.LogoutRequestDeniedError):

              self.error('Logout request denied by %s' %
@@ -162,6 +172,11 @@ 

  

          try:

              logout.processRequestMsg(message)

+         except lasso.DsInvalidSigalgError as e:

+             msg = 'Invalid or missing signature algorithm %r [%r]' % (

+                 e, message

+             )

+             raise InvalidRequest(msg)

          except (lasso.ServerProviderNotFoundError,

                  lasso.ProfileUnknownProviderError) as e:

              msg = 'Invalid SP [%s] (%r [%r])' % (logout.remoteProviderId,

@@ -42,6 +42,8 @@ 

      def __init__(self, config, provider_id):

          super(ServiceProvider, self).__init__()

          self.cfg = config

+         if provider_id is None:

+             raise InvalidProviderId('No provider id supplied')

          data = self.cfg.get_data(name='id', value=provider_id)

          if len(data) != 1:

              raise InvalidProviderId('multiple matches')
@@ -109,6 +111,10 @@ 

                  'Defines a list of allowed attributes, applied after mapping.'

                  ' Setting this overrides the global values.',

                  self.allowed_attributes),

+             pconfig.Condition(

+                 'Verify Signatures',

+                 'Verify Signatures for this SP (recommended).',

+                 self.verify_signatures),

          )

  

      @property
@@ -233,6 +239,15 @@ 

              value = temp.export_value()

          self._staging['allowed_attributes'] = value

  

+     @property

+     def verify_signatures(self):

+         value = self._properties.get('verify_signatures', True)

+         return value in [u'1', 'True', True]

+ 

+     @verify_signatures.setter

+     def verify_signatures(self, value):

+         self._staging['verify_signatures'] = value

+ 

      def save_properties(self):

          data = self.cfg.get_data(name='id', value=self.provider_id)

          if len(data) != 1:
@@ -267,6 +282,24 @@ 

                      return nip.format

          raise NameIdNotAllowed(nip.format)

  

+     def get_signature_hint(self):

+         """

+         Call this before any lasso process message methods to get

+         the hint on whether to enforce signatures or not.

+         """

+         if self.verify_signatures:

+             hint = lasso.PROFILE_SIGNATURE_VERIFY_HINT_FORCE

+         else:

+             hint = lasso.PROFILE_SIGNATURE_VERIFY_HINT_IGNORE

+ 

+         self.error(

+             'Setting signature hint to "%s"' %

+             (hint == lasso.PROFILE_SIGNATURE_VERIFY_HINT_FORCE and

+              "force" or "ignore")

+         )

+ 

+         return hint

+ 

      def permanently_delete(self):

          data = self.cfg.get_data(name='id', value=self.provider_id)

          if len(data) != 1:

@@ -11,10 +11,14 @@ 

  {% for p in providers %}

      <div class="row">

          <div class="col-md-3 col-sm-3 col-xs-6">

-             <a href="{{ baseurl }}/sp/{{ p.name }}">{{ p.name }}</a>

+                 <a href="{{ baseurl }}/sp/{{ p.name }}">{{ p.name }}</a>

          </div>

          <div class="col-md-3 col-sm-3 col-xs-6">

-             {{ p.provider_id }}

+             {% if not p.verify_signatures %}

+                 <font color="#FF0000">{{ p.provider_id }}</font>

+             {% else %}

+                 {{ p.provider_id }}

+             {% endif %}

              <!-- TODO: add javascript popup to ask for confirmation ? -->

              <a class="btn btn-default" href="{{ baseurl }}/sp/{{ p.name }}/delete">Delete</a>

          </div>

no initial comment

I am missing a very clear UI indicator to the user/and or admin that signature checking is disabled.
Also, this does not log anything if signature checking was ignored.

What did you have in mind for the UI indicator? There isn't anything I can pass that would be guaranteed to show on an SP. It should be fairly obvious in the admin page of the SP though.

I can add logging.

I mean an indicator in the admin UI (mark the SP as red and have a mouse-over of "Signature checking disabled") and in the login screen.

Ok, I'll see what I can do.

I'm a little confused as to where you are trying to control the use of signatures. If it's for logout and the HTTP-Post or HTTP-Redirect binding is being used the SAML spec requires LogoutRequest messages to be signed, it's not optional.

I'm also a little concerned about setting a configuration parameter on the ServiceProvider concerning the signature hint. Are you aware both a SP and IdP can specify whether they want requests signed in their metadata? How will this config item play with the flag set in the provider metadata?

Also, if I recall correctly, we had agreed we would not make it an option in the admin UI, but that it would be an undocumented setting in the config, to prevent easy discovery of it.

An agreement to make this option invisible isn't recorded in the ticket. I don't see any problem with it as it helps with interop.

According to the SAML 2.0 spec logout request and response SHOULD be signed.

If verification is required for the SP then this option is really a no-op since this is the lasso default. It never gets to the hint. Unsigned requests will be rejected as invalid.

If verification is not required then signed requests are processed as-is and unsigned requests are not rejected.

This is what I found reading the spec:

The SAML Profiles spec, section 4.4.31, lines 1223-1224 says:

"The <LogoutRequest> message MUST be signed if the HTTP POST or Redirect binding is used."

Lines 1235-1236 adds:

"The requester MUST authenticate itself to the identity provider, either by signing
the <LogoutRequest> or using any other binding-supported mechanism."

Lines 1262-1264 adds:

"The responder MUST authenticate itself to the requesting identity provider, either by signing the <LogoutResponse> or using any other binding-supported mechanism."

Lines 1276-1277 adds:

"The <LogoutResponse> message MUST be signed if the HTTP POST or Redirect binding is used."

Lines 1297-1298 adds:

"The requester MUST authenticate itself to the responder and ensure message integrity, either by signing the message or using a binding-specific mechanism."

Lines 1309-1310 adds:

"The responder MUST authenticate itself to the requester and ensure message integrity, either by signing
the message or using a binding-specific mechanism."

I was looking in the SAML 2.0 core and it includes:

Section 3.7.1 line 2532 states that the LogoutRequest message SHOULD be signed.

Section 3.7.2 line 2572 states that the LogoutResponse message SHOULD be signed

Strange that these are inconsistent but I think the profile is quite a bit clearer so I'll remove the option from logout.

[Posted this on the ticket comment as well]

I have my doubts about adding a config option to disable signature verification. The SAML core spec is very clear, if a request or response is signed the signature MUST be verified, if verification fails the operation terminates with an error.

I know Rob said this is only for debugging purposes but it's still going to be a visible option that an admin can set. Do we really want to allow admins to turn off signature verification?

I checked Shibboleth and as far as I could tell Shibboleth does not support disabling signature verification. Shibboleth does allow you to specify per SP configuration, here is the link to the SAM2 SSO "profile", I don't see anything there which allows defeating signature verification.

https://wiki.shibboleth.net/confluence/display/SHIB2/IdPSAML2SSOProfileConfig

SimpleSAMLPHP and Keycloak both offer options to not enforce signing.

It looks like Shibboleth does this via security-policy.xml, the XMLSigning PolicyRule.

The Shibboleth security-policy.xml configuration file is for configuring a Service Provider not an IdP.

Because Shibboleth ships both an SP and an IdP (completely different implementations in different languages utilizing different libraries) you have to be careful when reading Shibboleth documentation to note whether the doc is labeled "NativeSP" or "IdP". Also Shibboleth released a major new version of it's IdP this year, v3. Although many sites are still using the v2 IdP the consortium is only supporting v3 now. The link I cited above is for v2 relaying party configuration, but the v3 relaying party configuration does not add the option to disable signing verification.

So in other words, if Shibboleth doesn't do it then Ipsilon can't either?

As for a hidden option, it would require a set of queries/updates or one very complex update. Not exactly the kind of thing easy to walk a user having problems through.

No, I'm not saying if Shibboleth doesn't do it then it's a non-starter. Rather what I was trying to address is a security concern on a very security sensitive authentication product. The specs say signatures if present must be verified and the preeminent implementation does not support disabling verification nor does Keycloak (see below).

Keycloak is disabling signature verification on responses received from an IdP, this is equivalent to disabling it in an SP which Shibboleth does as well. Maybe I missed it but I didn't see anything that disables signature verification in their IdP for messages received from a relaying party.

There is a significant difference between disabling signature verification in an SP vs. in a IdP. The IdP holds the keys to the kingdom.

I just worry about admins shooting themselves in the foot and then blaming us for giving them the gun that shot their toe off.

I won't object anymore.

This has been succeeded by #71, which has been merged.