Modify am_handler setup to run before mod_proxy

[jhrozek]

The way the ECP flow works is that when a client initiates the flow, the
SP's response is HTTP 200, but not the requested content, but a signed XML
document that contains the "samlp:AuthnRequest" element. The idea is that
the ECP client would then determine the IDP and send the document to the
IDP, get a samlp:Response and convey that to the SP to get access to the
protected resource.

Internally, the auth check which is normally done with am_check_uid() set to
apache's ap_hook_check_user_id() hook, just responds with OK, so it pretends
to authenticate the user. Then in the usual flow, the request reaches the
ap_hook_handler which handles the request. There in the pipeline, mellon
registers functions am_handler() which should run first (APR_HOOK_FIRST),
determine that this request is an ECP one and return the ECP AuthnRequest
document. But in case the proxy module is also in the picture, the proxy
module "races" for who gets to be the first to handle the request in the
pipeline and wins. Therefore, the request reaches the protected resource
via mod_proxy and returns it.

This fix modifies the ap_hook_handler() call to explicitly run before
handlers from mod_proxy.c

To reproduce the bug:
0) Have a SP with mellon connected to a Keycloak IDP (or any other IDP I
guess). In the example below, my SAML SP is saml.federation.test

1. Set a Location protected by mellon that proxies requests to another
   URL. For example:

   ProxyPass /sp-proxy http://app.federation.test/example_app/
   <Location /sp-proxy>
   AuthType Mellon
   MellonEnable auth
   Require valid-user
   

2. call:
   curl -L -H "Accept: application/vnd.paos+xml"
   -H 'PAOS: ver="urn:liberty:paos:2003-08";"urn:oasis:names:tc:SAML:2.0:profiles:SSO:ecp"'
   http://saml.federation.test/sp-proxy

Before the patch, you would see whatever is served from the proxied
page. With the patch, you should get back a XML document with a
samlp:AuthnRequest.

[jhrozek]

cc @jdennis

[jhrozek]

By the way there is another approach that I was considering and honestly I am not sure which is better. it would use APR_HOOK_REALLY_FIRST instead of hardcoding mod_proxy. The reason I'm not sure which approach is better is that on one hand my programmer's training tells me it's better to be generic and hardcoding mod_proxy.c seems like the opposite :-) on the other hand we only ever saw the problem with mod_proxy, so hardcoding it might actually be safer. You can view the other approach here: master...jhrozek:am_handler_really_first

[jdennis]

The Apache hook doc (https://httpd.apache.org/docs/2.4/developer/hooks.html) is clear, you're supposed to use the list of module names passed in the 2nd and 3rd parameters of the function used to register the hook. Furthermore it explicitly states "Note that there are two more values, APR_HOOK_REALLY_FIRST and APR_HOOK_REALLY_LAST. These should only be used by the hook exporter." If you think about it APR_HOOK_REALLY_FIRST is no better than APR_HOOK_FIRST if more than one module uses that value. I'll grant you that explicitly enumerating the names of modules seems problematic in the face of unknown loaded modules and their behavior this is at the moment the most robust way to assure the hook ordering, it's the documented method and it's used elsewhere in Apache.

[jhrozek]

Thanks @jdennis that indeed makes sense (and therefore the patch in the PR stands).

@olavmrk any comments about the patch?

[olavmrk]

Hi,

sorry for the late response here -- I have been on vacation for a week.

I am positive to this change, but I would like to see a short comment before the line initializing the run_before variable, saying why it needs to run before mod_proxy. E.g.:

/* Our handler needs to run before mod_proxy so that it can properly
 * return ECP AuthnRequest messages when running as a reverse proxy.
 * See: https://github.com/Uninett/mod_auth_mellon/pull/196
 */
static const char * const run_before[]={ "mod_proxy.c", NULL };

I'd also like to see the variable renamed to something slightly more specific, e.g. run_handler_before.

[jhrozek]

Done, thank you for the comments.

[olavmrk]

Thanks!