IBX-11181: Trusted Proxies is not set on Ibexa Cloud - #699
Conversation
38c87b6 to
17e4096
Compare
| @@ -15,16 +15,6 @@ | |||
|
|
|||
| final class TrustedHeaderClientIpEventSubscriber implements EventSubscriberInterface | |||
There was a problem hiding this comment.
| final class TrustedHeaderClientIpEventSubscriber implements EventSubscriberInterface | |
| final class IbexaCloudTrustedProxiesEventSubscriber implements EventSubscriberInterface |
Or this is considered a BC ? Class is final though, so maybe not a problem?
There was a problem hiding this comment.
isnt IbexaCloud too generic tho? this whole class is about Upsun & Fastly, right?
17e4096 to
b1a199d
Compare
|
ec3cba0 to
d53633c
Compare
| $trustedHeaderName = $this->trustedHeaderName; | ||
| if (null === $trustedHeaderName && $this->isPlatformShProxy($request)) { | ||
| $trustedHeaderName = self::PLATFORM_SH_TRUSTED_HEADER_CLIENT_IP; | ||
| if ($this->isPlatformShProxy($request) && $request->headers->get('Client-Cdn') === 'fastly') { |
There was a problem hiding this comment.
As discussed with Vidar Client-Cdn header is stripped by Upsun when request is not coming from trusted CDN (in case we care about security and we do)
There was a problem hiding this comment.
Confirmed by Upsun (thanks Vidar for reaching out to them with this question): client can't spoof Client-Cdn header - it should arrive into their docs soon so we know for sure we can trust this header.
|
konradoboza
left a comment
konradoboza
left a comment
There was a problem hiding this comment.
PR can be rebased as #794 got merged, it contains PHPStan issues fix.
A few questions:
PR for filtering X-Forward-For in varnish and fastly VCLs are comming
Do we have this PR already? It should most likely incorporate X-Forward-For and X-Forward-Host in our VCL, right?
Shouldn't this indeed belong to ibexa/fastly since it's covering this very case? It will also make more sense concerning merging up the changes.
In regards to:
As the Upsun router anyway sets the X-Forward-For header, and devs still needed to set trusted_proxies in config ( which IMO is impossible), the old implementation in TrustedHeaderClientIpEventSubscriber is void.
But it certainly may be that I am missing something important....?
https://github.com/ibexa/post-install/blob/4.6/resources/platformsh/ibexa-commerce/4.6/.platform.app.yaml#L65 I think this is where it can be omitted once projects are created from templates. This seems like potential security flaw where XFF remains untouched. The subscriber in question overwrites XFF (attacker-influenced?) with X-Client-IP (router-controlled) so that the processing reads a clean value.
It seems we need to do the following to summarize:
- sunset
ibexa/fastly-forwarded-host(archive), - make sure VCL is properly updated,
- the combo mentioned below is verified not to fallback to the Symfony defaults.
| if (null === $trustedHeaderName && $this->isPlatformShProxy($request)) { | ||
| $trustedHeaderName = self::PLATFORM_SH_TRUSTED_HEADER_CLIENT_IP; | ||
| if ($this->isPlatformShProxy($request) && $request->headers->get('Client-Cdn') === 'fastly') { | ||
| Request::setTrustedProxies(['REMOTE_ADDR'], Request::getTrustedHeaderSet()); |
There was a problem hiding this comment.
Wouldn't that fallback to Symfony's default if framework.trusted_proxies was never set? Shouldn't we be more specific here by passing: HEADER_X_FORWARDED_FOR | HEADER_X_FORWARDED_HOST | HEADER_X_FORWARDED_PROTO | HEADER_X_FORWARDED_PORT combo?
There was a problem hiding this comment.
Via Symfony's code:
$trustedHeaderSet = $trustedHeaders ?? (Request::HEADER_X_FORWARDED_FOR | Request::HEADER_X_FORWARDED_PORT | Request::HEADER_X_FORWARDED_PROTO);Host and Prefix are omitted in this case but the question is - do we really need them? It's not like previous Subscriber added Host so imo this change is proper. Subscriber didn't pass Host and we aren't passing Host now.
Host has the following note in Symfony docs:
Enabling the Request::HEADER_X_FORWARDED_HOST option exposes the application to [HTTP Host header attacks](https://www.skeletonscribe.net/2013/05/practical-http-host-header-attacks.html). Make sure the proxy really sends an x-forwarded-host header.
There was a problem hiding this comment.
On the other hand, HOST was set manually in https://github.com/ibexa/fastly-forwarded-host/blob/main/src/bundle/EventSubscriber/KernelRequestSubscriber.php and if we are deprecating this bundle I would say we also need to pass X_FORWARDED_HOST explicitly? I believe we can pass it safely because proxy sends it safely and cannot be spoofed due to Client-Cdn existance that guarantess request is actually coming from proxy?
There was a problem hiding this comment.
Additionally, if I understand correctly fastly-forwarded-host bundle was created some time ago because of the need to handle a lot of Upsun domains and overcome some routing (??) limitations. Perhaps the FORWARDED-HOST should be enabled explicitly in this subscriber via a dedicated config option if such a limitation occurs?
7 participants
Related PRs:
Description:
Trusted proxies are not set on Ibexa Cloud when using Fastly. We document (via Upsun,) that user must disable
TRUSTED_PROXIES: "REMOTE_ADDR"in.platform.app.yaml(which is correct), but we don’t say or specify how to configureTRUSTED_PROXIEScorrectly.And frankly, it is not easy/possible to do via static config given how the Upsun router behaves:
Upsun router behavior:
X-FORWARD-FORsent by malicious clientX-FORWARD-FOR, appending header if already set ( "1.1.1.1" => "1.1.1.1, 2.2.2.2")X-Client-IPheader ( value is IP of client). Value is also end-user's IP if request is received via Fastly ( doc )fastlySo there is no static IP or IP range you can set as trusted proxies when using Fastly on Upsun. For instance, setting trusted hosts to Fasty's public IP list will not do any good. Thus, Ibexa DXP should automatically detect on Ibexa Cloud if request is received via Fastly
Next, I really doesn't understand the original purpose of
TrustedHeaderClientIpEventSubscriber. It basically does this:X_FORWARDED_FORwill be set (value will be copied from theX-Client-IPrequest header) andX_FORWARDED_FORwill be set astrusted_headerstrusted_proxiescorrectly.X-Client-IPas sourceMy next doubt is why there was some need for this in relation to IBX-4046
FYI : Original PR : https://github.com/ibexa/core/pull/165/files
As the Upsun router anyway sets the X-Forward-For header, and devs still needed to set
trusted_proxiesin config ( which IMO is impossible), the old implementation inTrustedHeaderClientIpEventSubscriberis void.But it certainly may be that I am missing something important....?
What the new implementation of
TrustedHeaderClientIpEventSubscriberdoes:For QA:
Maybe do sanity checks with "create Applications for Customer portal" feature, as original implementation of
TrustedHeaderClientIpEventSubscriberwas implemented in relation to that.Documentation: