#173 Add per-SP option to disable signature checking for authn requests
Closed: Fixed None Opened 3 years ago by simo.

We found an issue verifying SAML signatures in authn requests.
It may be useful for debugging purposes to be able to disable signature checking.
it is a single option in auth.py:AuthenticateRequest._parse_request()
right after login is instantiated conditionally call:
login.setSignatureVerifyHint(lasso.PROFILE_SIGNATURE_VERIFY_HINT_IGNORE)


Thinking a little bit more, we'll have to allow this globally because the hint is set before we know which SP we are bneing sent authn from.

however we might be able to set lasso.PROFILE_SIGNATURE_VERIFY_HINT_MAYBE) and then check with login.getSignatureStatus() and fail if signature verification failed and was required.

Fields changed

milestone: => 1.2

lasso.PROFILE_SIGNATURE_VERIFY_HINT_MAYBE uses a global variable to determine if signature checking is enabled or not, controlled via lasso.setFlag('[no-]verify-signature'). So I don't think this is what we want.

According to the doc message in the python bindings using setSignatureVerifyHint is a per-transaction hint so it should do what you are suggesting.

I think this should apply to logout as well but in testing so far setting the hint still raises the exception lasso.ProfileCannotVerifySignatureError(). Still investigating.

Ok, never mind, I was setting it in the wrong place in logout. The hint works as expected.

I propose adding a config option to the SP: Verify Signatures: TRUE/FALSE, defaulting to TRUE.

owner: => rcritten
status: new => accepted

I tested using a SimpleSAMLPHP SP and tweaked the values of

    'redirect.sign'        => TRUE,
    'sign.authnrequest'    => TRUE,
    'redirect.validate'    => TRUE,

Setting to TRUE for signing and FALSE for no signing. I then flipped the value of Verify Signatures in the SP in Ipsilon to enable/disable signature checking.

https://pagure.io/ipsilon/pull-request/48

patch_available: 0 => 1

John pointed out in the review that signed logout is a MUST in the profile spec (less clear in the core spec).

We also agreed that notification on the login screen was overkill.

So this will:

  • display the providerId in red in the admin UI in the SP selection screen
  • if an unsigned authn request is retrieved then the verify_signatures value will be queried, potentially allowing unsigned login requests to be allowed.

To test this set sign.authnrequest => FALSE and redirect.sign and redirect.validate to TRUE.

[I'm adding this to the review 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

We decided that setting this option will kick off a timer for 8 hours. During this time unsigned login responses will be accepted. After that window they will again be rejected.

As an implementation point, we want this to appear as a boolean in the UI to turn it on or off but be a time+offset internally. We probably also want to show somewhere how much time is left in the timeout, though that may have to come later.

No signature checking is needed for Google Apps support. What we decided is that if the SP metadata has no public key then we won't enforce signing.

owner: rcritten => puiterwijk
status: accepted => assigned

We also decided that we will be showing warnings in the admin UI if the SP doesn't have keys.

status: assigned => accepted

This PR has been merged as f93dd96

resolution: => fixed
status: accepted => closed

Metadata Update from @puiterwijk:
- Issue assigned to puiterwijk
- Issue set to the milestone: 1.2

2 years ago

Login to comment on this ticket.

Metadata