Check ACS location in AuthN request in all cases

[baszoetekouw]

Currently, EB's validation of ACS locations in the AuthnRequest seems to work like this:

• If the ACSLocation in the AuthnRequest is also defined in the metadata, use it.
• Otherwise (unknown ACS location):
  • if an SP certificate is defined in the metadata and the message is signed: check signature, if valid use ACS location from AuthnRequest, otherwise fail with user-facing error
  • if an SP certificate is defined in the metadata and is message is unsigned: fail with user-facing error
  • if no SP certificate is defined in the metadata: always fall back to default ACS location from metadata

The latter two options are confusing for the SP administrator. It would be more consistent to always fail if the ACSLocation from the AuthnRequest doesn't match the AuthnRequest from the metadata. So:

• If the ACSLocation in the AuthnRequest is also defined in the metadata, use it.
• Otherwise (unknown ACS location):
  • if an SP certificate is defined in the metadata and the message is signed and the signature is correct: use ACS location from AuthnRequest
  • Otherwise: fail with user-facing error.

[relaxnow]

if an SP certificate is defined in the metadata and is message is unsigned: fail with user-facing error

That seems wrong to me don't we only do this if "AuthnRequestsSigned" was set in the metadata or manually by an admin?

In any case this behavior is unspecified by the specs I think (though I would have to check) but I think it's a mistake to show an error to a poor user here. EB was designed to try it's best to forward the user and log that something was wrong with this request. If the SP messes up it's ACS locations then the user will still arrive back at the SP, just at the wrong location. But the user will correctly identify the SP as the party in error.

I understand that this makes debugging harder, but that is why that bit of EB is very verbose with NOTICE logging. Ideally we'd be able to show that logging to the SP administrator too, but that would require a self-service environment.

[baszoetekouw]

That seems wrong to me don't we only do this if "AuthnRequestsSigned" was set in the metadata or manually by an admin?

Ah, indeed. Henny and I were just debugging; it seems I overlooked this setting. So whereever I wrote "certificate is defined" you should read "redirect.sign is enabled".

In any case this behavior is unspecified by the specs I think (though I would have to check) but I think it's a mistake to show an error to a poor user here. EB was designed to try it's best to forward the user and log that something was wrong with this request. If the SP messes up it's ACS locations then the user will still arrive back at the SP, just at the wrong location. But the user will correctly identify the SP as the party in error.

That's a valid point of view, I guess. However, I would like to argue that's it better to show an error when something obviously wrong is detected, rather than to guess what was meant. In the first case, the issue will most probably be fixed; in the second case, it will probably lead to much more work in the future, as it is not clear what is going on.

Regardless, it should be possible to show a clear error message that tells the user this is an SP problem, and that the SP tech contact should fix this (including this contact info).

[relaxnow]

I would like to argue that's it better to show an error when something obviously wrong is detected,

But show an error to the appropriate party, which is not the user UNLESS the user is the SP administrator, but we have no way of discerning that currently.

I would like to argue that's it better to show an error when something obviously wrong is detected, rather than to guess what was meant

We're not guessing what is meant, we're simply rejecting invalid input and proceeding on valid input. Just like we don't care if extra URL parameters are added to the SSO URL. Or if the browser of the user sent a HEAD request before doing the GET. Or about a million other things that could be different about that request.

In the first case, the issue will most probably be fixed; in the second case, it will probably lead to much more work in the future, as it is not clear what is going on.

In the first case a lot of users will see errors and a tiny percentage of them will complain "something is wrong with OpenConext". Hopefully they will complain to their SP, though more likely to their IdP or the EB operator. In any case the EB operator should be able to quickly discern what went wrong by trying to repeat the results and monitoring the logging for his experiment. Maybe something could be improved here?

Regardless, it should be possible to show a clear error message that tells the user this is an SP problem, and that the SP tech contact should fix this (including this contact info).

The message for:

if an SP certificate is defined in the metadata and is message is unsigned: fail with user-facing error

should definitely mention that the Service Provider is in error and we absolutely can't continue for security reasons. Please contact the SP support contact.

[thijskh]

We've not seen any problems with this in past years, the current implementation is robust and it's not clear what added value removing the fallback to the default endpoint would bring, other than likely breaking many currently functioning connections.