Rewrite to support reverse proxy #677
Conversation
ekohl
force-pushed
the
apache-reverse-proxy
branch
2 times, most recently
from
988b182 to
b252850
Compare
| # $passenger:: Configure foreman via apache and passenger | ||
| # $apache:: Configure Foreman via Apache. By default via passenger but otherwise as a reverse proxy. | ||
| # | ||
| # $passenger:: Whether to configure Apache with passenger or as a reverse proxy. |
There was a problem hiding this comment.
Should these two conflict? or is the idea you need both now to get passenger?
There was a problem hiding this comment.
This changes it so that the parameter is ignored if apache is false
There was a problem hiding this comment.
A future-proof approach, if we consider multiple kind of setups, could be an Enum['apache+passenger', 'apache+puma', 'puma', 'nginx+passenger', 'nginx+puma'], what do you think about?
There was a problem hiding this comment.
While I like the idea of supporting nginx, I think it would be premature. Longer term I'd like to deprecate passenger. It'd be better to decide then how to best support it.
There was a problem hiding this comment.
I agree about nginx, I noted it as example. The idea I want to point is to have a single class variable to select the targeted mode. Internally, we could keep $passenger, $apache and $use_foreman_service.
|
Just to show you how easy "enabling" puma is (And confirm my theory works about installing the RPM): |
|
Not finding a traceback, but I did get an error trying to switch from passenger to service: |
|
I got that too in the acceptance tests and wondered if it was something about the docker container. |
manifests/config/apache.pp
Outdated
| use_optional_includes => true, | ||
| custom_fragment => $custom_fragment, | ||
| * => $passenger_options + $proxy_https_options, | ||
| passenger_pre_start => $passenger_https_prestart, |
There was a problem hiding this comment.
Is there a way to pass these parts optionally? This currently configures the vhost with the Passenger directives if you set passenger false which feels misleading.
There was a problem hiding this comment.
It shouldn't. If passenger is false it should be undef. The reason I don't set it in passenger_options is that the pre-start differs for HTTP and HTTPS and didn't want to duplicate more code.
|
Running the enable on the system: Googling around seems to have something to do with if the symlink already exists. Perhaps something else is creating it before sytsemctl? |
Perhaps we do it wrong in our systemd unit setup in packaging? Edit: I noticed we also do some setup with |
|
I noticed this error with |
|
Yes, this is a breaking change and those modules need to be modified. We could keep the defined type with a deprecation to make it easier. |
|
Just keeping track of the issues I run into while testing. Trying to switch from passenger to non-passenger, the Apache proxy seems to be failing: |
|
What needs to be done is to unset the request headers so they can't be spoofed. Then the application needs to be configured to use HTTP_SSL_* instead of SSL_. |
|
Will need a rebase |
ekohl
force-pushed
the
apache-reverse-proxy
branch
from
3392911 to
f468073
Compare
|
Did a trivial rebase. Not sure when I'll get back to it, but setting the right headers and configuring Foreman shouldn't be hard to add and then it'll work mostly of out the box. |
|
Adding some additional information here based on conversation. Katello takes in certificates from Apache and processes them here: https://github.com/Katello/katello/blob/master/app/services/cert/rhsm_client.rb That code is called from the following and derived from this: |
|
Two additional thoughts from testing: I believe this would break one expectation Katello has which is a certificate passing through a reverse proxy on a smart proxy and then to the main server. See the comment from here where we do part of this today -- https://github.com/theforeman/puppet-katello/blob/76496ef444c1b6051b1e41a2596428fc32103f1f/files/katello-apache-ssl.conf#L2-L3 The second part is around how these are controlled. The code seems to make use of a Foreman configuration setting to determine the header which seems odd to me that an admin in the UI can control the infrastructure configuration of the setup, i.e. I'll do some digging to see if I can understand why this would ever need to be set this way vs. hard-coded or in |
ekohl
force-pushed
the
apache-reverse-proxy
branch
from
f468073 to
89084d8
Compare
|
This is a rather thorough rebase where I've tried to split things up into smaller pieces. Still not entirely content but I want to start testing this now and see if it works. |
ekohl
force-pushed
the
apache-reverse-proxy
branch
2 times, most recently
from
cfaf5df to
1e38143
Compare
| :ssl_client_dn_env: HTTP_SSL_CLIENT_S_DN | ||
| :ssl_client_verify_env: HTTP_SSL_CLIENT_VERIFY | ||
| :ssl_client_cert_env: HTTP_SSL_CLIENT_CERT | ||
| <% end -%> |
There was a problem hiding this comment.
Based on our discussions, I thought you were gonna keep this still being set in the Settings (database) vs moving to SETTINGS (app restart). Don't get me wrong I like this change given what these represent, but I'd do this stand alone so we can make the code change for moving them alongside it easier.
There was a problem hiding this comment.
At this point I first want to get this working, then I'll see whether we use the config file or database. First I want to get the Apache config right.
There was a problem hiding this comment.
Right... my point is the application won't respect these because they are SETTINGS and it currently is built to read Settings (database).
I was wondering if for now we'd be best doing:
foreman_config_entry { 'ssl_client_cert_env':
value => 'HTTP_SSL_CLIENT_CERt',
}
There was a problem hiding this comment.
Settings that are in settings.yaml are visible, but can't be overridden:
Note it's visible, but there's no edit button. The's also a tooltip:
This setting is defined in the configuration file 'settings.yaml' and is read-only.
ekohl
force-pushed
the
apache-reverse-proxy
branch
2 times, most recently
from
b065bd3 to
921c217
Compare
|
In its basic form I think this works now. |
ekohl
force-pushed
the
apache-reverse-proxy
branch
2 times, most recently
from
7c05062 to
2ce9a8f
Compare
| @@ -52,7 +52,7 @@ previous stable release. | |||
| ### Foreman version compatibility notes | |||
|
|
|||
| This module targets Foreman 1.21+. Running without passenger is only supported | |||
| on Foreman 1.22+. | |||
| on Foreman 1.23+. | |||
There was a problem hiding this comment.
We didn't cherry pick theforeman/foreman-packaging@ab3a75c into 1.22. So for RPMs it's 1.22 but debs 1.23. Rather than cherry picking it for a less used function I'm choosing to just mark it as 1.23.
| String $priority = $::foreman::vhost_priority, | ||
| Stdlib::Fqdn $servername = $::foreman::servername, | ||
| Array[Stdlib::Fqdn] $serveraliases = $::foreman::serveraliases, | ||
| Stdlib::Port $server_port = $::foreman::server_port, | ||
| Stdlib::Port $server_ssl_port = $::foreman::server_ssl_port, | ||
| Stdlib::Httpurl $proxy_backend = "http://${::foreman::foreman_service_bind}:${::foreman::foreman_service_port}/", | ||
| Hash $proxy_params = {'retry' => '0'}, | ||
| Array[String] $proxy_no_proxy_uris = ['/pulp', '/streamer', '/pub'], |
There was a problem hiding this comment.
These are the ones Katello has. I'm wondering if this is needed or if there's a cleaner solution.
There was a problem hiding this comment.
/pub is probably required, /pulp is a wsgi mount, isn't it?
There was a problem hiding this comment.
Honestly, I don't know exactly. Some locations are mapped to files. It's a very complicated deployment. This was my "rather safe than sorry" solution.
|
I want to get this merged soon now that we're fresh in the 1.24 development cycle. @ehelms @timogoebel @evgeni mind having a look? |
ekohl
force-pushed
the
apache-reverse-proxy
branch
from
2ce9a8f to
36ead27
Compare
| String $priority = $::foreman::vhost_priority, | ||
| Stdlib::Fqdn $servername = $::foreman::servername, | ||
| Array[Stdlib::Fqdn] $serveraliases = $::foreman::serveraliases, | ||
| Stdlib::Port $server_port = $::foreman::server_port, | ||
| Stdlib::Port $server_ssl_port = $::foreman::server_ssl_port, | ||
| Stdlib::Httpurl $proxy_backend = "http://${::foreman::foreman_service_bind}:${::foreman::foreman_service_port}/", | ||
| Hash $proxy_params = {'retry' => '0'}, | ||
| Array[String] $proxy_no_proxy_uris = ['/pulp', '/streamer', '/pub'], |
There was a problem hiding this comment.
/pub is probably required, /pulp is a wsgi mount, isn't it?
| path => '/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin', | ||
| if $apache { | ||
| if $passenger { | ||
| exec {'restart_foreman': |
There was a problem hiding this comment.
It's used implicitly. It's a refreshonly exec, but when Class['foreman::service'] is refreshed, this exec is refreshed. Note it's been there for ages.
|
Once tests are passing this is a 👍 for me, I'd love to get this in and running for a few weeks in nightly. |
|
One thing to consider is that if you can make HTTP requests to the Puma backend, you can imitate anyone. This is a security concern because most things can connect to http://localhost:$port. It could be mitigated by running Puma on a unix socket and limiting access. https://github.com/puma/puma/blob/master/docs/systemd.md#socket-activation describes how we could use this. Is this something we want to do? |
ekohl
force-pushed
the
apache-reverse-proxy
branch
from
36ead27 to
0f66faf
Compare
I would make it the default but allow to change it to some localhost port. The app (e.g. foreman) would then need to verify if the client is actually a trusted host (e.g. that's why we have https://github.com/theforeman/foreman/blob/d74dc4251f8397e31fcb9214d83f929c8992c9c3/app/controllers/concerns/foreman/controller/ip_from_request_env.rb#L12) |
ekohl
force-pushed
the
apache-reverse-proxy
branch
from
0f66faf to
9f15f13
Compare
|
I'm looking into the socket and while I think it should be possible, I'm going to prefer to harden the setup later and initially launch it as an experimental feature. I've adapted @neomilium's suggestion and added websockets support now. |
|
Current issues:
|
I think theforeman/foreman-packaging#4044 should fix this. |
ekohl
force-pushed
the
apache-reverse-proxy
branch
from
9f15f13 to
3dc3cda
Compare
|
Still the issue: Not sure if my change actually made it into nightlies. |
|
The issue with statsd is that the Apache config without passenger cleans |
This unifies the Apache stopping code and also removes mod_passenger on RH since puppetlabs-apache doesn't deal well with it being installed but not used. This will be relevant when using the reverse proxy setup.
ekohl
force-pushed
the
apache-reverse-proxy
branch
from
e0cbabf to
70ae2d6
Compare
|
I've cancelled the Debian acceptance tests because they need https://ci.theforeman.org/job/foreman-nightly-deb-pipeline/143/ to complete before they can pass. Other than that I think this PR should turn green. |
ekohl
force-pushed
the
apache-reverse-proxy
branch
from
70ae2d6 to
7870a1c
Compare
|
💚 now |
|
merged, dank je wel @ekohl! |
This doesn't implement the actual reverse proxy part, but it does make passenger optional and prepares the code for it.