Repository navigation
Conversation
434e83e to
15d27f9
Compare
|
There was a problem hiding this comment.
The VCL and reasoning looks good enough, I like the extended testing setup. Summoning @alongosz for his opinion on the bash/CI changes and @ibexa/quality-assurance in regards to docker/Behat. 😉
| - name: Run test suite | ||
| run: composer run-script --timeout=600 test | ||
|
|
||
| varnish-vcl-tests: |
There was a problem hiding this comment.
@alongosz is this setup aligned with the current, org-wise CI adjustments?
There was a problem hiding this comment.
@konradoboza @alongosz If you refere to IBX-11778 / #90, I have aligned that now:
- the varnish-vcl-tests job runs on ubuntu-26.04 (fixup 7d80b5a, as you suggested)
- uses actions/checkout@v7 like the rest of the workflow.
| AUTOMATION_CLIENT_INSTALLATION: ${{ secrets.AUTOMATION_CLIENT_INSTALLATION }} | ||
| AUTOMATION_CLIENT_SECRET: ${{ secrets.AUTOMATION_CLIENT_SECRET }} | ||
| SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK_URL }} | ||
| varnish7-trusted-proxy: |
There was a problem hiding this comment.
@konradoboza @alongosz : Aligned this based on existings jobs:
1cf910d to
131cf41
Compare
19690e1 to
fc5e116
Compare
9d4791b to
03221b4
Compare
|
4 participants
Caution
DO-NOT-MERGE : PR contains TMP commit with dependencies.json for making CI green
Note
SonarCloud error is false positive. behat tests refers to github actions by ibexa repo/branchname instead of SHA commit hash
Related PRs:
ibexa/docker#65 must be merged before this one ( once it is merged, the [TMP] commit in this PR can be removed )
Description:
Follow-up to ibexa/core#699, which makes Ibexa DXP declare the peer a trusted proxy when a request
arrives via Fastly on Ibexa Cloud. Once trusted proxies are in play, every
X-Forwarded-*header aclient sends is believed by Symfony, so Varnish has to stop them from reaching the application.
Until now the VCLs took
X-Forwarded-Protostraight from the client and derivedX-Forwarded-Portfrom it, and never touched
Forwarded,X-Forwarded-HostorX-Forwarded-Prefix. A client couldtherefore spoof the scheme and host the application sees. Since
X-Forwarded-*is not part of theVarnish cache key, a spoofed value also poisons the cached object for everyone else.
vcl_recvnow strips those headers for every client that is not listed in a newtrusted_proxiesACL in
parameters.vcl, and overwritesX-Forwarded-Forwith the real client IP.X-Client-IPandClient-Cdnare stripped too, so a client cannot fake the Ibexa Cloud detection added inibexa/core#699. A proxy that is in the ACL keeps passing everything through, which is what a TLS
terminator, load balancer or CDN in front of Varnish relies on.
Applies to
varnish5.vcl,varnish6.vclandvarnish7.vcl.varnish9.vclis deliberately leftout while its own PRs are open; it gets the same treatment once they land.
SonarCloud complains:
Use full commit SHA hash for this dependency.. However, I refer to main branch in a ibexa repository. This is considered safe and that warning can be ignored.BC breaks:
varnish5/6/7.vclnow reference thetrusted_proxiesACL, which lives inparameters.vcl. Varnishresolves ACLs at VCL load time, so
parameters.vcland the VCL must be updated together - anexisting
parameters.vclwithout the ACL makes Varnish fail to load withSymbol not found: 'trusted_proxies'. There is no way to make an ACL reference optional in VCL.The ACL ships with localhost only. Anyone running a TLS terminator in front of Varnish has to add
its IP, otherwise the
X-Forwarded-Protoit sets is discarded and URLs are generated ashttp://.Documented in
docs/varnish/README.md.For QA:
Check that https URL generation still works behind a TLS terminator once its IP is added to the
trusted_proxiesACL, and that cache invalidation and theX-Cachedebug headers are unaffected.Easiest done using a custom controller that outputs those headers if they exists, maybe