Fixes #30535 - Use HTTP headers in puma

[hsahmed]

This along with the pull request to foreman-puppet theforeman/puppet-foreman#872 fixes the external authentication issue #30535

[theforeman-bot]

Can one of the admins verify this patch?

[theforeman-bot]

Can one of the admins verify this patch?

[theforeman-bot]

Issues: #30535

[ekohl]

This will break on Apache + Passenger deployments which at least for now are still supported. I'm not sure what the best way to deal with this is. We can either make a setting that is a prefix or a setting for each header that's used.

@tbrisker @timogoebel I'd like to get your opinion on the settings part.

[tbrisker]

Perhaps it is possible to modify the code so that it works for both header sets? i.e. if the headers in passenger format it will look at those headers using the existing code, and if they are in puma format it will use the new code?

[ekohl]

That can easily create a security issue. If the webserver doesn't set/clear the header, it can be spoofed by an attacker to pretend they're another user. Unlike in the Apache module (which passes secure content as env vars), in a reverse proxy situation you have no way to know whether a header is trusted. You're mixing secure content into an insecure channel. A common way to deal with that is explicit configuration to opt in, which should function as a confirmation that you set up the other side correctly.

[tbrisker]

That can easily create a security issue. If the webserver doesn't set/clear the header, it can be spoofed by an attacker to pretend they're another user. Unlike in the Apache module (which passes secure content as env vars), in a reverse proxy situation you have no way to know whether a header is trusted. You're mixing secure content into an insecure channel. A common way to deal with that is explicit configuration to opt in, which should function as a confirmation that you set up the other side correctly.

Could we instead check somehow else if we're on puma or passenger inside the code?
In any case this auth source only works if the user enabled external authentication (see #available?), so it should be a matter of selecting the correct headers for the current server here.

[hsahmed]

@ekohl I have updated the patch to installer (theforeman/puppet-foreman#872) which now sets the apache env variables to match the ruby variable names so they should now work with Passenger (I was unable to test this due to vanilla Foreman installation with the --foreman-passenger true failing, it could be another bug). Since this is only used if external auth is enabled, it will only be a security issue if it is mis-configured (for instance by enabling authentication_delegation in foreman without configuring httpd), which is a concern even without this patch. We can further secure it by setting headers+environment in the default http configuration (/etc/httpd/conf.d/05-foreman-ssl.conf) to always set the REMOTE_USER_* variables overriding any spoofed values in the request.

[timogoebel]

Perhaps it is possible to modify the code so that it works for both header sets?

@ekohl: I concur with @tbrisker. I believe the cleanest solution is implementing an interface to access env variables that detects if Foreman is running in puma or passenger and adds the prefix if necessary. I believe we'll have the same problem in other places, don't we?

[hsahmed]

@timogoebel the only other place REMOTE_USER is accessed is foreman/app/controllers/application_controller.rb which can be changed to use HTTP_REMOTE_USER too. AFAIK Puma sitting behind apache and being accessed using mod_proxy would have no way to access Apache env even with Apache API. The file apache.rb checks for authentication delegation setting and if enabled forwards to extlogin page which is the only location that authenticates and resets the variables preventing any spoofing.

[ekohl]

if Foreman is running in puma or passenger and adds the prefix if necessary. I believe we'll have the same problem in other places, don't we?

For the SSL certs we explicitly configure the headers. I suppose this also makes it easy to use in envs which have non-standard headers.

https://github.com/theforeman/puppet-foreman/blob/e3bf011200d2519895627e29f0cb458dc47338b7/templates/settings.yaml.erb#L102-L108

AFAIK Puma sitting behind apache and being accessed using mod_proxy would have no way to access Apache env even with Apache API

Correct. That means if you find REMOTE_USER in the env, you can assume it's safe to use. However, if you don't I don't know for sure how to safely detect a valid config. Given this auth is off by default, it may be safe enough to assume the user configured it correctly. That probably warrants an upgrade note.

[hsahmed]

We can update foreman-installer to have options to specify header name to be set to pass the REMOTE_USER from Apache to Puma for external authentication. I really don't see any other option to resolve this issue other than passing REMOTE_USER as a header, it is a standard practice for systems using Apache for external auth. This is how oVirt does it:

    RewriteCond %{LA-U:REMOTE_USER} ^(.*)$
    RewriteRule ^(.*)$ - [L,NS,P,E=REMOTE_USER:%1]
    RequestHeader set X-Remote-User %{REMOTE_USER}s

ManageIQ also takes similar approach ManageIQ

[ekohl]

Yes, passing the header isn't the problem. The problem is how we define exactly which header and how to deal with Passenger.

Having an explicit setting on the Foreman side gives the most flexibility. We do this for the client certs. The installer can then configure both sides to match. It also gives users who terminate using a different webserver setup flexibility. I also like the parity with client certs.

On the other hand, it does mean a bunch of additional settings.

Personally I like the explicitness of a bunch of settings. They're very unlikely to change during runtime so I think they can live in settings.yaml and have some sane defaults.

@tbrisker @timogoebel thoughts?

[tbrisker]

On the other hand, it does mean a bunch of additional settings.

I would prefer not adding a bunch of settings that the user shouldn't care about. We should decide on how we want the headers to be called by apache/passenger/puma and have foreman look for them.
Users should only care about if they want to allow external auth or not, not the wiring between the webserver and foreman.

[ekohl]

On the other hand, it does mean a bunch of additional settings.

I would prefer not adding a bunch of settings that the user shouldn't care about. We should decide on how we want the headers to be called by apache/passenger/puma and have foreman look for them.
Users should only care about if they want to allow external auth or not, not the wiring between the webserver and foreman.

I think these settings should only live in the YAML file, not as a database setting. Perhaps we should even remove the current DB settings and only allow them to be configured via YAML.

In #7960 I started with a setting that lets you configure whether there's a reverse proxy. That may be the only right way. For example, it's valid to run Puma without a reverse proxy but there's no way that I can think of to detect that.

[ares]

After a discussion with other maintainers: we want to fix this but the fix needs to work for both passenger and puma deployments. We don't want to introduce settings for these (not even in YAML). We should be setting headers name explicitly in the config, there's very little chance of the need to change them.

[ehelms]

What is the downside of the proposal here (theforeman/puppet-foreman#872) alongside this change? That enables both Puma and Passenger to work from my own testing.

[ekohl]

It requires a change in the deployment for existing Passenger users. If they forget that, their installation is suddenly insecure because any attacker can send HTTP headers to impersonate any user. This is also try for anyone who can send HTTP requests directly to Puma, though theforeman/puppet-foreman#883 should counter that.

A bit of an edge case, but right now they can enable the SSO setting, even if they don't use Apache at all. This is still secure because there's no way for an attacker to send the headers. After this, it becomes insecure.

[ehelms]

It requires a change in the deployment for existing Passenger users.

Does this only apply to users who don't use the installer or puppet-foreman directly?

A bit of an edge case, but right now they can enable the SSO setting, even if they don't use Apache at all. This is still secure because there's no way for an attacker to send the headers. After this, it becomes insecure.

I'm not as versed on these attack vectors. If a user only had this code change, and not the associated installer update, what is it about changing the name of the header used that enables an attacker to have a vector of a ttack with Puma and Passenger?

[ekohl]

A bit of an edge case, but right now they can enable the SSO setting, even if they don't use Apache at all. This is still secure because there's no way for an attacker to send the headers. After this, it becomes insecure.

I'm not as versed on these attack vectors. If a user only had this code change, and not the associated installer update, what is it about changing the name of the header used that enables an attacker to have a vector of a ttack with Puma and Passenger?

With Passenger Apache would set up some memory and pass environment variables as well as headers. In a reverse proxy setup the communication between Apache and Puma is done using regular HTTP, so rather than environment variables, regular HTTP headers are now used. The problem is Puma can't tell who set the header. It could have been an attacker or Apache.

That is why on the Apache side you set up configuration to always clear the header and then set it to some value if there is any. Basically you always reject the Remote-User header (and others). That's why it's so important that the application side and webserver side match and create a secure configuration.

However, if you can reach Puma directly then you can bypass Apache and pretend to be Apache. By default we now bind to http://127.0.0.1:3000 which means that if you can perform a HTTP request there, you are in.

More concretely: if anyone runs enables SSO, then pretending to be admin can be done by roughly sending curl -H 'Remote-User: admin' http://localhost:3000/. This does require that there is an admin user configured from SSO, but if there isn't and you know the name of a group with sufficient permissions, you can use REMOTE_USER_GROUPS to create a new user in one of these groups.

While it's uncommon to have shell accounts on the system of untrusted users, exploits are often used together. One allows you to combine it with another.

This is something we can mitigate in our installer (theforeman/puppet-foreman#883 does it by binding to a unix socket with strict permissions). However, I think that in the containers we ship, there is no Apache. Just straight up Puma. That means that any user of those who enables the SSO config boolean has opened their installation up.

This is why I think that explicit configuration of headers is a good thing. It tells the application that the admin has verified those headers are secure.

[ehelms]

Thanks for the detailed write up. From the sounds of it, this is an existing problem and this change to fix a problem with SSO does not enable the security concern, that concern exists no matter what. Unless I am missing something fundamental. Which would put this in two separate issues that we've identified:

1. Fixing this SSO bug through the change proposed here that would work for passenger and puma
2. Make the change to sockets proposed here Fixes #30803: Bind to socket for Puma and Apache puppet-foreman#883 to harden the deployment with Puma

This is why I think that explicit configuration of headers is a good thing. It tells the application that the admin has verified those headers are secure.

I am missing something about this case. This sounds like security by obscurity, that the app server and Apache are agreeing to a set of headers to use but if a user can guess or figure those out they can still send them if they have access to the box.

[ekohl]

From the sounds of it, this is an existing problem and this change to fix a problem with SSO does not enable the security concern, that concern exists no matter what.

Not for SSO. Prior to this patch, it used environment variables. Those are only available in Passenger and in Puma with a reverse proxy they're simply nil. Note how they didn't use to have the HTTP_ prefix which indicates they're not user provided headers.

This is why I think that explicit configuration of headers is a good thing. It tells the application that the admin has verified those headers are secure.

I am missing something about this case. This sounds like security by obscurity, that the app server and Apache are agreeing to a set of headers to use but if a user can guess or figure those out they can still send them if they have access to the box.

No, it's not through obscurity. If we look at how we configure ssl_client_dn_env and friends:

foreman/app/models/setting/auth.rb

Lines 15 to 17 in 40c5618

	set('ssl_client_dn_env', N_('Environment variable containing the subject DN from a client SSL certificate'), 'SSL_CLIENT_S_DN', N_('SSL client DN env')),
	set('ssl_client_verify_env', N_('Environment variable containing the verification status of a client SSL certificate'), 'SSL_CLIENT_VERIFY', N_('SSL client verify env')),
	set('ssl_client_cert_env', N_("Environment variable containing a client's SSL certificate"), 'SSL_CLIENT_CERT', N_('SSL client cert env')),

Here we see the env vars don't have HTTP_ prefixes. This means they don't accept HTTP headers out of the box. The installer configures these to look at headers in case of Puma. This is OK because their values are cleared.

In this PR right now the implications for the user are not clear. The setting that controls this, explicitly mentions environment variable, not HTTP header.

foreman/app/models/setting/auth.rb

Lines 24 to 25 in 40c5618

	set('authorize_login_delegation', N_("Authorize login delegation with REMOTE_USER environment variable"), false, N_('Authorize login delegation')),
	set('authorize_login_delegation_api', N_("Authorize login delegation with REMOTE_USER environment variable for API calls too"), false, N_('Authorize login delegation API')),

That makes it very easy for users outside of the installer to misconfigure their setup and end up with an gaping security hole. Remember that in the Foreman installation manual the installer is not mandatory.

[hsahmed]

In this PR right now the implications for the user are not clear. The setting that controls this, explicitly mentions environment variable, not HTTP header.

foreman/app/models/setting/auth.rb

Lines 24 to 25 in 40c5618

	set('authorize_login_delegation', N_("Authorize login delegation with REMOTE_USER environment variable"), false, N_('Authorize login delegation')),
	set('authorize_login_delegation_api', N_("Authorize login delegation with REMOTE_USER environment variable for API calls too"), false, N_('Authorize login delegation API')),

That makes it very easy for users outside of the installer to misconfigure their setup and end up with an gaping security hole. Remember that in the Foreman installation manual the installer is not mandatory.

We should have the variable names set in the settings, even if it's just a single prefix like "HTTP_X_FOREMAN_", the SSO service can check these if external authentication is requested and fail if these header names are not set. This way the only way this change can cause a security hole is if the user enables external authentication in settings, sets the headers in the settings file and then not clears/sets those headers in Apache configuration. Even in that case the access will be without any roles unless the admin either pre-creates the user and assigns it roles or maps any external groups that can then be spoofed.

[ehelms]

@hsahmed Can you take a shot at implementing what you described ?

[tbrisker]

Trying to summarize the discussion so far, please correct me if I got something wrong:

1. The only way we have of passing variables from Apache to Puma currently is by adding request headers (which appear in rack applications like Puma as HTTP_... keys in request.env). This is different than when we used mod_passenger which allowed setting additional environment variables for the request (e.g. REMOTE_USER in request.env). The difference is that HTTP_... headers could originate either from a user request or from apache setting them, and we have no means of differentiating between the two.
2. We can clear user-provided HTTP_... request headers in Apache before passing the request to Puma. However, this requires Apache to be properly set up to clear these headers (e.g. using the installer). This means that if a user doesn't use Apache in their set up, or sets it up manually without clearing these headers, an attacker could identify themselves as a remote user by adding the specified headers to their requests.
3. Even when set up properly, a user with local access could connect to puma directly over HTTP and pass these headers. This will however be mitigated by switching puma to only listen on a unix socket.
4. In another case, we require the user to explicitly set the SSL header names as settings, although the installer automatically sets up these settings with default HTTP_ names when puma is used (and clears any such headers passed by the user in Apache).
5. A possible way to resolve this is to add another setting that defined the header names to be used, which will configured by the installer to use HTTP_... when puma is used.
6. In the long run, the plan is to completely remove mod_passenger and use only Puma. The current implementation attempts to allow external authentication using both passenger and puma.
7. It is common practice in other projects to clear such headers and set them in apache with a hard coded name, such as X-Remote-User.
8. By default, external authentication is disabled. To enable it, a user must change a setting and set up apache according to https://theforeman.org/manuals/2.1/index.html#5.7ExternalAuthentication . We could improve the documentation around how external authentication works and clarify which headers are used and how.
9. Users could use puma without apache, but in that case external authentication would not work anyways right now afaict. They will need to figure out how to properly configure the headers themselves. Given that this isn't currently a supported deployment option anyways, I don't think that we should go out of our way to enable it.

In my opinion, adding a setting for the header names does not solve any issue. In the majority of cases, users will use the installer that will set the names anyway for them to the default one. Changing the header names used via the setting would only cause more potential issues - for example, forgetting to change the header names in apache or forgetting to clear the new header names. If we have hard coded header names (in combination with the unix socket change), we can have apache always clear them and make sure that in the supported setup they can not originate from a potential attacker.

[hsahmed]

@tbrisker I agree with you, since even for users not using installer, they still need to set authorize_login_delegation: true in the settings file and assure that Apache is configured properly for external authentication to work and the header details can be highlighted in the external authentication documentation. Out of the box with default installation this PR does not impact anything and enabling it with installer unsets existing headers in Apache before setting them from environment. I have further simplified the installer template in the puppet-foreman project. There is one thing however, I could not test this with passenger; trying --foreman-passenger=true with latest Foreman (2.1.2) fails with the error:

/opt/puppetlabs/puppet/bin/puppet:5:in `<main>'
 /Stage[main]/Foreman_proxy::Register/Foreman_smartproxy[fore212.example.com]: Failed to call refresh: Proxy fore212.example.com cannot be retrieved: unknown error (response 500)
 /Stage[main]/Foreman_proxy::Register/Foreman_smartproxy[fore212.example.com]: Proxy fore212.example.com cannot be retrieved: unknown error (response 500)

However since Passenger also reads HTTP headers from request.env, it should also work there.

[tbrisker]

[test foreman]

[ekohl]

Overall this looks sane to me. https://theforeman.org/manuals/nightly/index.html#3.5.2ConfigurationOptions and https://theforeman.org/manuals/2.1/index.html#5.7.3KerberosSingleSign-On will need to be updated to reflect this too.

I'll be out until next week, after that I can see if I can spin up an env to reproduce it if nobody beat me to it. It looks like the installer part is backwards compatible, so it may be good to merge that first.

[ehelms]

I tested this and the installer change, and both worked with Puma based application server:

[root@centos7-foreman-nightly vagrant]# curl -k -u : --negotiate https://`hostname`/users/extlogin/
<html><body>You are being <a href="https://centos7-foreman-nightly.war.example.com/hosts">redirected</a>.</body></html>

[tbrisker]

[test foreman]

[hsahmed]

[test foreman]

Is there anything I can do to prevent "Assert Single Commit (non-blocking) / build (pull_request)" test from failing?

[tbrisker]

@hsahmed i'm not sure why that's failing, considering there is just one commit in this PR. perhaps it needs rebasing on top of latest develop? In any case it isn't blocking, more of a notice for maintainer to make sure commits are squashed prior to merging.
I've moved the redmine issue to foreman project, so that check will also be fine.

[wiad]

I'm using mod_auth_cas to provide SSO via Apache and that module sets it's own headers (it sets the 'REMOTE_USER' header but has its own name for the other headers). I have been unsuccessful in trying to create new headers (for example REMOTE_USER_LASTNAME) off of the mod_auth_cas provided headers, I only end up with '(null)' as value, seems like the mod_auth_cas headers are not available when i'm trying to set my requestheaders.

If I modify apache.rb and replace the header names there with my mod_auth_cas headernames it all works. It is pretty common with applications that have support for external web authentication that you can configure the names of the HTTP headers that contain the authentication info - could that maybe be a good idea for Foreman as well?

[ekohl]

It is indeed true that those can contain the literal string (null). We see the same thing with certificates:

foreman/app/services/foreman/client_certificate.rb

Line 11 in b10e4c0

@raw_cert_available ||= request.ssl? && raw_data.present? && raw_data != '(null)'

It would be best to filter those out.

[ehelms]

This PR sounds like it is ready to go and @wiad is requesting a follow up PR to aide in the support of CAS by making the names of the headers configurable by the admin?

[wiad]

This PR sounds like it is ready to go and @wiad is requesting a follow up PR to aide in the support of CAS by making the names of the headers configurable by the admin?

Not sure if this was a question directed at me, but if it was - yes, I would love to have the header names configurable!

[ehelms]

@wiad Could you create an issue for your use case and request at https://projects.theforeman.org/projects/foreman/issues/new so we can look at a follow on to support it?

[wiad]

Created issue for configurable HTTP headers at https://projects.theforeman.org/issues/31079

[tbrisker]

Any other comments @ekohl or @ehelms ? If not I'd like to merge this so we can get it into 2.2 RC4, if there's any follow-up needed we can open additional PRs to address it.

[ekohl]

In itself it looks good, but this area might need some more work.

[ekohl]

I like this constant a lot more than duplicating it everywhere. Not going to block on it, but I'd like to see similar constants for HTTP_REMOTE_USER_GROUPS + email + firstname + lastname.

However, it is weird that the constant is only used here and not in ApplicationController.

[ekohl]

Where is @remote_user even used? I don't see it anywhere. It looks like this was used until 4e7ea9b but is now delegated to the abstract SSO provider. Feels like this part should also be delegated to the SSO provider.

[tbrisker]

Thanks @hsahmed for pushing this to the finish line and @ekohl @ehelms for in depth reviews! Let's follow up on the remaining comments in further PRs.

[tbrisker]

pulled into 2.2 as 4ecb372