From 07e773feed6f823f6a5fa31f3e98d18cd1f4af77 Mon Sep 17 00:00:00 2001 From: Evgeni Golov Date: Wed, 18 Sep 2024 09:00:40 +0200 Subject: [PATCH] Fixes #37761 - use ProxyPass and upgrade=websocket where possible MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RewriteRules need special handling of some characters (esp "?"), which differs based on Apache version. Instead of going down that way, we can switch to using ProxyPass as proxying is the only thing we really need here, at least for HTTP. For WebSockets, we need to allow protocol upgrades, which modern (2.4.47+) Apache can do itself via "ProxyPass … upgrade=websocket". For older Apache (esp on EL8 and Ubuntu 20.04), we keep the RewriteRules in place. To be removed when we drop support for those targets. Co-Authored-By: Adam Ruzicka --- manifests/config/apache.pp | 32 ++++++---- manifests/plugin/remote_execution/cockpit.pp | 11 +++- spec/classes/foreman_config_apache_spec.rb | 62 +++++++++++++------ .../plugin/remote_execution_cockpit_spec.rb | 15 ++++- templates/cockpit-apache-ssl-rewrite.conf.erb | 11 ++++ templates/cockpit-apache-ssl.conf.erb | 7 +-- 6 files changed, 96 insertions(+), 42 deletions(-) create mode 100644 templates/cockpit-apache-ssl-rewrite.conf.erb diff --git a/manifests/config/apache.pp b/manifests/config/apache.pp index 8b6a861db..43ca86c7d 100644 --- a/manifests/config/apache.pp +++ b/manifests/config/apache.pp @@ -167,8 +167,9 @@ if $suburi { $custom_fragment = undef } else { - # mod_env and mod_expires are required by configuration in _assets.conf.erb + # mod_env, mod_expires and mod_rewrite are required by configuration in _assets.conf.erb include apache::mod::env + include apache::mod::rewrite # apache::mod::expires pulls in a config file we don't want, like apache::default_mods # It uses ensure_resource to be compatible with both $apache::default_mods set to true and false include apache @@ -182,8 +183,23 @@ order => '03', } - include apache::mod::proxy_wstunnel - $websockets_backend = regsubst($_proxy_backend, 'http://', 'ws://') + # mod_proxy supports "ProxyPass ... upgrade=websocket" since 2.4.47 + # EL8: 2.4.37 / EL9: 2.4.62 / Debian11: 2.4.62 / Ubuntu20.04: 2.4.41 / Ubuntu22.04: 2.4.52 + $proxy_upgrade_websocket = !($facts['os']['family'] == 'RedHat' and $facts['os']['release']['major'] == '8') and !($facts['os']['name'] == 'Ubuntu' and $facts['os']['release']['major'] == '20.04') + if $proxy_upgrade_websocket { + $vhost_rewrites = [] + $_proxy_params = $proxy_params + { 'upgrade' => 'websocket' } + } else { + include apache::mod::proxy_wstunnel + $websockets_backend = regsubst($_proxy_backend, 'http://', 'ws://') + $websockets_rewrite = { + 'comment' => 'Upgrade Websocket connections', + 'rewrite_cond' => '%{HTTP:Upgrade} =websocket [NC]', + 'rewrite_rule' => "/(.*) ${websockets_backend}\$1 [P,L]", + } + $vhost_rewrites = [$websockets_rewrite] + $_proxy_params = $proxy_params + } $vhost_http_request_headers = [ 'set X_FORWARDED_PROTO "http"', @@ -209,15 +225,9 @@ 'no_proxy_uris' => $_proxy_no_proxy_uris, 'path' => pick($suburi, '/'), 'url' => $_proxy_backend, - 'params' => $proxy_params, + 'params' => $_proxy_params, }, - 'rewrites' => [ - { - 'comment' => 'Upgrade Websocket connections', - 'rewrite_cond' => '%{HTTP:Upgrade} =websocket [NC]', - 'rewrite_rule' => "/(.*) ${websockets_backend}\$1 [P,L]", - }, - ], + 'rewrites' => $vhost_rewrites, } $vhost_https_request_headers = [ diff --git a/manifests/plugin/remote_execution/cockpit.pp b/manifests/plugin/remote_execution/cockpit.pp index 86bfcc584..d5ba34e21 100644 --- a/manifests/plugin/remote_execution/cockpit.pp +++ b/manifests/plugin/remote_execution/cockpit.pp @@ -72,11 +72,16 @@ require => Class['foreman::database'], } } else { - include apache::mod::rewrite - include apache::mod::proxy_wstunnel include apache::mod::proxy_http + if $foreman::config::apache::proxy_upgrade_websocket { + $_apache_template = 'cockpit-apache-ssl.conf.erb' + } else { + include apache::mod::rewrite + include apache::mod::proxy_wstunnel + $_apache_template = 'cockpit-apache-ssl-rewrite.conf.erb' + } foreman::config::apache::fragment { 'cockpit': - ssl_content => template('foreman/cockpit-apache-ssl.conf.erb'), + ssl_content => template("foreman/${_apache_template}"), } foreman_config_entry { 'remote_execution_cockpit_url': diff --git a/spec/classes/foreman_config_apache_spec.rb b/spec/classes/foreman_config_apache_spec.rb index de5bd7fa2..4f9bb8144 100644 --- a/spec/classes/foreman_config_apache_spec.rb +++ b/spec/classes/foreman_config_apache_spec.rb @@ -15,6 +15,16 @@ end end + let(:proxy_pass_params) do + if (facts[:os]['family'] == 'RedHat' && facts[:os]['release']['major'] == '8') || + (facts[:os]['name'] == 'Ubuntu' && facts[:os]['release']['major'] == '20.04') + { 'retry' => '0' } + else + { 'retry' => '0', 'upgrade' => 'websocket' } + end + end + + it { should compile.with_all_deps } it 'should include apache with modules' do @@ -22,8 +32,11 @@ should contain_apache__mod('expires') should contain_class('apache::mod::proxy') should contain_class('apache::mod::proxy_http') - should contain_class('apache::mod::proxy_wstunnel') should contain_class('apache::mod::rewrite') + if (facts[:os]['family'] == 'RedHat' && facts[:os]['release']['major'] == '8') || + (facts[:os]['name'] == 'Ubuntu' && facts[:os]['release']['major'] == '20.04') + should contain_class('apache::mod::proxy_wstunnel') + end end it 'does not manage the docroot' do @@ -79,15 +92,19 @@ "no_proxy_uris" => ['/pulp', '/pub', '/icons', '/images', '/server-status', '/webpack', '/assets'], "path" => '/', "url" => 'unix:///run/foreman.sock|http://foreman/', - "params" => { "retry" => '0' }, + "params" => proxy_pass_params, ) - .with_rewrites([ - { - 'comment' => 'Upgrade Websocket connections', - 'rewrite_cond' => '%{HTTP:Upgrade} =websocket [NC]', - 'rewrite_rule' => '/(.*) unix:///run/foreman.sock|ws://foreman/$1 [P,L]', - }, - ]) + if (facts[:os]['family'] == 'RedHat' && facts[:os]['release']['major'] == '8') || + (facts[:os]['name'] == 'Ubuntu' && facts[:os]['release']['major'] == '20.04') + should contain_apache__vhost('foreman') + .with_rewrites([ + { + 'comment' => 'Upgrade Websocket connections', + 'rewrite_cond' => '%{HTTP:Upgrade} =websocket [NC]', + 'rewrite_rule' => '/(.*) unix:///run/foreman.sock|ws://foreman/$1 [P,L]', + }, + ]) + end end it 'does not configure the HTTPS vhost' do @@ -109,8 +126,11 @@ class { 'apache': should contain_apache__mod('expires') should contain_class('apache::mod::proxy') should contain_class('apache::mod::proxy_http') - should contain_class('apache::mod::proxy_wstunnel') should contain_class('apache::mod::rewrite') + if (facts[:os]['family'] == 'RedHat' && facts[:os]['release']['major'] == '8') || + (facts[:os]['name'] == 'Ubuntu' && facts[:os]['release']['major'] == '20.04') + should contain_class('apache::mod::proxy_wstunnel') + end end end @@ -152,7 +172,7 @@ class { 'apache': "no_proxy_uris" => ['/pulp', '/pub', '/icons', '/images', '/server-status'], "path" => '/', "url" => 'unix:///run/foreman.sock|http://foreman/', - "params" => { "retry" => '0' }, + "params" => proxy_pass_params, ) } end @@ -229,15 +249,19 @@ class { 'apache': "no_proxy_uris" => ['/pulp', '/pub', '/icons', '/images', '/server-status', '/webpack', '/assets'], "path" => '/', "url" => 'unix:///run/foreman.sock|http://foreman/', - "params" => { "retry" => '0' }, + "params" => proxy_pass_params, ) - .with_rewrites([ - { - 'comment' => 'Upgrade Websocket connections', - 'rewrite_cond' => '%{HTTP:Upgrade} =websocket [NC]', - 'rewrite_rule' => '/(.*) unix:///run/foreman.sock|ws://foreman/$1 [P,L]', - }, - ]) + if (facts[:os]['family'] == 'RedHat' && facts[:os]['release']['major'] == '8') || + (facts[:os]['name'] == 'Ubuntu' && facts[:os]['release']['major'] == '20.04') + should contain_apache__vhost('foreman-ssl') + .with_rewrites([ + { + 'comment' => 'Upgrade Websocket connections', + 'rewrite_cond' => '%{HTTP:Upgrade} =websocket [NC]', + 'rewrite_rule' => '/(.*) unix:///run/foreman.sock|ws://foreman/$1 [P,L]', + }, + ]) + end end describe 'with vhost and ssl, no CRL explicitly' do diff --git a/spec/classes/plugin/remote_execution_cockpit_spec.rb b/spec/classes/plugin/remote_execution_cockpit_spec.rb index 4913e6d01..b26895eb7 100644 --- a/spec/classes/plugin/remote_execution_cockpit_spec.rb +++ b/spec/classes/plugin/remote_execution_cockpit_spec.rb @@ -65,13 +65,22 @@ class {'foreman': end it 'configures apache' do - is_expected.to contain_class('apache::mod::proxy_wstunnel') is_expected.to contain_class('apache::mod::proxy_http') is_expected.to contain_foreman__config__apache__fragment('cockpit') .without_content .with_ssl_content(%r{^$}) - .with_ssl_content(%r{^ RewriteRule /webcon/\(\.\*\) ws://127\.0\.0\.1:19090/webcon/\$1 \[P\]$}) - .with_ssl_content(%r{^ RewriteRule /webcon/\(\.\*\) http://127\.0\.0\.1:19090/webcon/\$1 \[P\]$}) + if (facts[:os]['family'] == 'RedHat' && facts[:os]['release']['major'] == '8') || + (facts[:os]['name'] == 'Ubuntu' && facts[:os]['release']['major'] == '20.04') + is_expected.to contain_class('apache::mod::rewrite') + is_expected.to contain_class('apache::mod::proxy_wstunnel') + is_expected.to contain_foreman__config__apache__fragment('cockpit') + .with_ssl_content(%r{^ RewriteRule /webcon/\(\.\*\) ws://127\.0\.0\.1:19090/webcon/\$1 \[P\]$}) + .with_ssl_content(%r{^ ProxyPass http://127\.0\.0\.1:19090/webcon$}) + else + is_expected.to contain_foreman__config__apache__fragment('cockpit') + .without_ssl_content(%r{RewriteRule}) + .with_ssl_content(%r{^ ProxyPass http://127\.0\.0\.1:19090/webcon upgrade=websocket$}) + end end end diff --git a/templates/cockpit-apache-ssl-rewrite.conf.erb b/templates/cockpit-apache-ssl-rewrite.conf.erb new file mode 100644 index 000000000..54b8c149d --- /dev/null +++ b/templates/cockpit-apache-ssl-rewrite.conf.erb @@ -0,0 +1,11 @@ +### File managed with puppet ### + +> + ProxyPreserveHost On + + RewriteEngine On + RewriteCond %{HTTP:Upgrade} =websocket [NC] + RewriteRule <%= @cockpit_path %>/(.*) ws://<%= @cockpit_host %>:<%= @cockpit_port %><%= @cockpit_path %>/$1 [P] + + ProxyPass http://<%= @cockpit_host %>:<%= @cockpit_port %><%= @cockpit_path %> + diff --git a/templates/cockpit-apache-ssl.conf.erb b/templates/cockpit-apache-ssl.conf.erb index 91fe9cadc..17b8b2648 100644 --- a/templates/cockpit-apache-ssl.conf.erb +++ b/templates/cockpit-apache-ssl.conf.erb @@ -2,10 +2,5 @@ > ProxyPreserveHost On - - RewriteEngine On - RewriteCond %{HTTP:Upgrade} =websocket [NC] - RewriteRule <%= @cockpit_path %>/(.*) ws://<%= @cockpit_host %>:<%= @cockpit_port %><%= @cockpit_path %>/$1 [P] - RewriteCond %{HTTP:Upgrade} !=websocket [NC] - RewriteRule <%= @cockpit_path %>/(.*) http://<%= @cockpit_host %>:<%= @cockpit_port %><%= @cockpit_path %>/$1 [P] + ProxyPass http://<%= @cockpit_host %>:<%= @cockpit_port %><%= @cockpit_path %> upgrade=websocket