Fixes #37761 - Allow rewrites needed for cockpit integration

[evgeni]

Mhh, UnsafeAllow3F is disabling the fix for https://www.cve.org/CVERecord?id=CVE-2024-38474 which raises the following two concerns:

• will this fail on systems that have no fix for that CVE yet? I guess so
• disabling a fix seems like a suboptimal solution, can we avoid having question marks in the redirections?

[adamruzicka]

will this fail on systems that have no fix for that CVE yet? I guess so

It will.

disabling a fix seems like a suboptimal solution, can we avoid having question marks in the redirections?

I'm afraid we cannot. I'll try looking into it a little bit more next week, but I'd be surprised if we could avoid it.

If I'm reading things right then the flow looks like this: 1) User goes to /webcon/=${hostname} 2) This hits foreman-cockpit 3) foreman-cockpit redirects to ${foreman_fqdn}/cockpit/redirect 4) ${foreman_fqdn}/cockpit/redirect redirects to ${foreman_fqdn}/webcon/=${hostname}?access_token=${token} 5) This hits foreman-cockpit again, now that the token is available the rest of the flow can continue

@mvollmer Hi, since you originally wrote most of this, would you have any ideas how we could resolve this situation?

[adamruzicka]

Ha! Scratch all that, apparently the token can be passed in as part of the query string or as an uri fragment, going with an uri fragment seems to do the trick

[adamruzicka]

https://github.com/theforeman/foreman_remote_execution/pull/918

[mvollmer]

Ha! Scratch all that, apparently the token can be passed in as part of the query string or as an uri fragment, going with an uri fragment seems to do the trick

Nice that you have figured this out!

I don't think I understand how this all works together in detail, but: is the rewrite rule only used during authentication, or also when Cockpit is already open and for loading the actual Cockpit URLs like .../cockpit/@localhost/system/index.html?

If so, this change to the rewrite rules might break downloading of SOS reports, and downloading of files in general in the new cockpit-files component. Downloading is done with "external channels" that use URLs like .../cockpit/channel/<token>?<base64-encoded-channel-options>.

If that is indeed the case, I think we can change Cockpit to not use the query part of URLs for this mechanism.

[adamruzicka]

is the rewrite rule only used during authentication, or also when Cockpit is already open and for loading the actual Cockpit URLs like .../cockpit/@localhost/system/index.html?

As far as I can tell it is used for every single request, no matter what "stage" we're at.

If so, this change to the rewrite rules might break downloading of SOS reports

Sigh, looks like you're right, trying to download a sosreport does fail with a 403 and the unsafe rewrite error in httpd error log.

If that is indeed the case, I think we can change Cockpit to not use the query part of URLs for this mechanism.

That would be nice

Thinking out loud: right now, we rewrite anything coming to /webcon/... to 127.0.0.1:19090/webcon/.... Assuming we don't modify the path, could we proxypass the request instead?

[adamruzicka]

Configuring httpd like this seems to work, any concerns about it?

<Location /webcon>
  ProxyPreserveHost On

  RewriteEngine On
  RewriteCond %{HTTP:Upgrade} =websocket [NC]
  RewriteRule /webcon/(.*)           ws://127.0.0.1:19090/webcon/$1 [P]

  ProxyPass http://127.0.0.1:19090/webcon
</Location>

[evgeni]

I thought [P] and ProxyPass are equivalent, but the are not! TIL

https://httpd.apache.org/docs/current/rewrite/flags.html#flag_p https://httpd.apache.org/docs/current/mod/mod_proxy.html#proxypass

But are we now proxying HTTP or WS? The old code does it conditionally on HTTP:Upgrade, but now not anymore?

[adamruzicka]

But are we now proxying HTTP or WS?

I would say we are still proxying both? http with proxypass and ws by rewriting the request and then proxying it?

The old code does it conditionally on HTTP:Upgrade, but now not anymore?

I'm not fluent in apache config, but isn't it more or less still there? Before we did if-elseif, now (in #1178) we do if-else, but it should be equivalent?

[mvollmer]

If that is indeed the case, I think we can change Cockpit to not use the query part of URLs for this mechanism.

That would be nice

Tell me if you need it! :-) I am not sure I can follow the discussion about the rewrite rules... The current assumption seems to be that ProxyPass works and can handle query parts just fine, and no changes to Cockpit are necessary, right?

[adamruzicka]

The current assumption seems to be that ProxyPass works and can handle query parts just fine, and no changes to Cockpit are necessary, right?

Yup, so far we're good. I'll let you know if anything changes, thank you