Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add scheme hook to detect X-Forwarded-Proto request header #33

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

smartalock
Copy link

Patch to allow mod_auth_mellon to detect X-Forwarded-Proto in request headers and if set to 'https' then use that. This helps fix issues where the protected resource is behind a reverse proxy which does the SSL handling.

@thijskh
Copy link

thijskh commented Aug 26, 2020

Thanks for the PR. I wonder if this needs to be mellon specific. Is it not better to use more standard Apache facilities to set the scheme than inside this specific module? E.g., to set the scheme via the ServerName directive (https://httpd.apache.org/docs/2.4/mod/core.html#servername) so it is set for all modules doing something with the scheme.

@smartalock
Copy link
Author

I agree it would be nice to have a more generic solution but this was the only way I could see to get it working in my setup.

My setup has an SSL reverse proxy sitting in front of the backend server with runs http only. mod_auth_mellon is running on the backend server. The backend server can be accessed via the reserve proxy, or directly from our LAN - when accessing from the LAN we disable mod_auth_mellon and fallback to Basic auth.

If I use ServerName with the optional scheme portion (e.g. ServerName https://www.example.com) then this internally sets the server_scheme variable in the Apache core. Any time Apache now needs to create a full URL from a relative one - e.g. when doing a redirect it will ignore the scheme in the request and instead use the one in the ServerName directive - even if UseCanonicalName is set to Off. It could be argued that this is a bug in Apache core ap_construct_url() function in core.c that should respect the UseCanonicalName for the scheme portion. A simple redirect like RewriteRule ^/$ /app/ [L,R=302] will create a Location header with https even if requested over http

I can control the Hostname that mod_auth_mellon sees by setting a ServerName that doesn't contain a scheme portion and then optionally using UseCanonicalName On when the request is coming from my reverse proxy

I have seen this technique used in mod_auth_openidc to detect the server's name/scheme when sitting behind a reverse proxy. They go one step further and also support X-Forwarded-Host and X-Forwarded-Port request headers. This may be a cleaner solution which would require rewriting the am_reconstruct_url function in auth_mellon_util.c

@thijskh
Copy link

thijskh commented Aug 26, 2020

The problem with these headers is that by default they are untrustworthy (when you are not behind a reverse proxy or the reverse proxy does not filter these request headers). So with only allowing the scheme to be 'upgraded' from http to https I can see that the risk is limited. But accepting more parameters like hostname it seems risky and open up possibilities for attack.

@simo5
Copy link
Member

simo5 commented Aug 26, 2020

@thijskh I learned over time that policing admins is a defeating proposition.
As long as the defaults are sane there is really no problem in giving admin foot guns, document them, let the best ones do what they need following guidance.
Making a tool work where needed trumps fears of slippery slopes IMO.
That said I do not advocate for any specific scheme, I advocate just for not letting the perfect be the enemy of good.

@thijskh
Copy link

thijskh commented Aug 26, 2020

I agree somewhat, but if we accept these header values by default any remote unauthenticated user can set their values. So it's not something an admin would opt in to, but something that would be available in any default install. Unless we add extra guards.

@smartalock
Copy link
Author

Agreed that the X-Forwarded-xxxxx headers are trivial for someone to spoof so could have unintended side-effects if they were all automatically accepted.

X-Forwarded-Proto is probably low risk and we only allow switching to https - I'm not sure there are any SAML installations that wouldn't be running https?

Another option would be to add a specific configuration directive - e.g. MellonForceScheme https and only do the switch if it is set

@simo5
Copy link
Member

simo5 commented Aug 26, 2020

We can't accept unsafe options by default of course, needs a gating switch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants