EZP-29817: Varnish - Purge requests with tokens

[damianz5]

Meta repo changes:

default_parameters.yml

    varnish_invalidate_token: '%env(HTTPCACHE_VARNISH_INVALIDATE_TOKEN)%'

    env(HTTPCACHE_VARNISH_INVALIDATE_TOKEN): ~

ezplatform.yml

ezpublish:
...
	system:
	...
		site_group: (same for admin_group)
            		http_cache:
			        purge_servers: ['%purge_server%']
+             			varnish_invalidate_token: '%varnish_invalidate_token%'

.env

# Enable varnish purging invalidate token by setting valid token
#HTTPCACHE_VARNISH_INVALIDATE_TOKEN=""

https://jira.ez.no/browse/EZP-29817

[andrerom]

Cool, didn't know it was planned 👍

As for VCL, not same approach, but started looking into it today and have this if you need some pointers on one possible approach: https://gist.github.com/andrerom/c1f123fe65b577ca915c57fa6ceaf835

[vidarl]

LGTM, but I would like to have vcl and docs updated too...

It's a poc, not for review yet afaik?

[andrerom]

Once we have updated VCL here, we can get @Plopix, or one of us to update https://github.com/ezsystems/ezplatform/pull/315/files as well to finish this code wise (doc will also be needed).

[damianz5]

@andrerom It was PoC at the beginning, but I will remove this label - review is required, I will continue working on this regarding the reviews

[andrerom]

Ok, I'd like to see changes to VCL (VCL in this repo, we can copy over changes to meta repo once this is approved and merged) so we can see the full picture as part of the review, feel free to use from example gist link above.

[damianz5]

Note: [platform.sh] check if ACL_INVALIDATE_TOKEN (or any env in general) will be injected to varnish node, and not only to app node.

[andrerom]

@damianz5 FYI tried to check with platform.sh folks if they prefered env or global var.
But no answer besides @kmadejski mentioning global var being slow.

Alternative is afaik to hardcode token in the VCL sub routine itself, which I was thinking we'd like to avoid.

[andrerom]

Besides nitpick, and assuming it works +1

[Plopix]

Regargind this: https://github.com/ezsystems/ezplatform/pull/315/files

I think they will or they have changed something else regarding the relationship.
I say that based on a conversation I had with @Crell but I am unsure of the status if they kept it or not.

Also with this token I don't think we need a relationship as the whole point is to remove it. (to avoid the circular reference)

I think we need Platform.sh in the loop to confirm everything.

Then I will be happy to update the PR!

But that's cool, I can't wait to have varnish ready

[andrerom]

You are bringing up a good point here @Plopix, we/Symfony rely on IP to denote which Proxy we trust. This implies still need for circular relationship unless there are suggestions on how we can avoid it.

One way is to add support for host names in app.php for trusted proxies (look them up). But if we can avoid that, it’d be great.

[Plopix]

hey @andrerom , again I am just guessing here, we need someone from Platform.sh.

To me, TrustedProxies are not relevant here https://symfony.com/doc/current/deployment/proxies.html

The normal chain on Platform.sh is Web -> Varnish -> App, the actual issue is that we need to purge Varnish and thus need a relation that does Web -> Varnish <-> App for App to purge Varnish kind of internally.

relationships:
    ...
    varnish: "varnish:http"

that my current understanding of the circular reference issue.

That why they will introduce a Token to allow to have only Web -> Varnish -> App and then App would purge through Web, the the Purge Request will not go directly from App -> Varnish but will go out and enter by Web (that is why we need a token)

Trusted Proxy is something else and it is probably not an issue.

[andrerom]

+1 We can cleanup VCL once we drop Varnish 4.1 support in the future.

[damianz5]

@andrerom @vidarl final review please

[andrerom]

Header could be a constant, but besides that I'm +1

[vidarl]

I have not tested, but this looks like a security hole

If server is configured to use whitelist (invalidators), and evil user sends x-purge-token header with empty value, this if might match if env variable HTTPCACHE_VARNISH_INVALIDATE_TOKEN is not set?

[damianz5]

I think if x-purge-token will be empty - the if condition if (req.http.x-purge-token) { will return false and second condition (req.http.x-purge-token != std.ge...) will not be called

[andrerom]

This is why it's not enabled by default. As enabling it means you should always inject env to avoid the

What @damianz5 says, the if won't enter unless header has value, so you can't match empty value. This is why there are two conditions here instead of one.

But, as p.sh does not support injecting it by env, which is where we need this. We can change example here to just have hardcoded token to avoid anyone stumbling into such an issue by mistake.

Avoids us having to document any differences between 4.1 and higher as well.

[andrerom]

Or, what was the outcome so far of discussions with p.sh?

[vidarl]

No reply from ph.sh yet. I have created a ticket in their jira, requesting us to be able to define env variables in the varnish container

[vidarl]

I have now tested sending token header with empty value, and indeed it seems to work as we intended.

So, since that is the case, why comment out the lines in the first place...?

[andrerom]

It was discussed on Slack, it's because std.getenv is not defined on Varnish 4.1 (LTS).
We can drop support for 4.x in a future major, but not in a minor where we currently need this.

[vidarl]

acknowledged

[andrerom]

Merging for followup on aligning VCL files. And progressing the work in meta repo once platform.sh adds more convenient ways to inject secrets to Varnish, ref: ezsystems/ezplatform#346