Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EZP-32017: fix trusted proxy setup for platform.sh #136

Open
wants to merge 1 commit into
base: 1.0
Choose a base branch
from

Conversation

ITernovtsii
Copy link
Contributor

@ITernovtsii ITernovtsii commented Oct 9, 2020

Question Answer
JIRA issue EZP-32017
Type Bug
Target version 1.0
BC breaks no
Doc needed no

platform.sh use "ngx_http_realip_module" nginx module, and put client IP into REMOTE_ADDR.
This makes it impossible to configure an application to be accessible from both with and without varnish.

If you'll put REMOTE_ADDR in TRUSTED_PROXIES, admin panel will be open to IP address spoof using "x-forwarded-for" header.
And, the user will be able to request your varnish invalidation token.

If you'll put other IP addresses, or don't specify TRUSTED_PROXIES at all, you'll receive "Unauthorized" from varnish (because of "/_ez_http_invalidatetoken" route checking "$request->isFromTrustedProxy()", which will return false).

This PR makes it possible to have varnish without defined TRUSTED_PROXIES

Related to ezsystems/ezplatform#609 but completely different)

TODO:

  • Implement feature / fix a bug.
  • Implement tests + specs and passing ($ composer test)
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@ITernovtsii ITernovtsii changed the base branch from master to 0.8 October 9, 2020 20:35
@ITernovtsii ITernovtsii changed the base branch from 0.8 to 1.0 October 9, 2020 20:35
@ITernovtsii ITernovtsii changed the base branch from 1.0 to 2.2 December 16, 2020 06:36
@ITernovtsii ITernovtsii changed the base branch from 2.2 to 1.0 December 16, 2020 06:37
@ITernovtsii
Copy link
Contributor Author

let me know if it's not something you would like to release for 2.5 and I'll rebase to 3.2

@vidarl vidarl self-requested a review December 16, 2020 07:29
Copy link
Member

@vidarl vidarl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the purpose of tokenAction() is no longer to actually return the invalidate-token, but to validate that a given token is correct.

To make that clear to everybody, I think we should state that in PHP doc for this method.

To better reflect this change, the controller should maybe no longer then return the token? Instead, the .vcl could check http status in the response. That will cause a BC though, (Things will breake if someone would update bundle, but not .vcl).
So, maybe just leave the logic in .vcl as is. I'll let others comment on that

@vidarl vidarl requested a review from a team December 16, 2020 08:55
@ITernovtsii
Copy link
Contributor Author

So, the purpose of tokenAction() is no longer to actually return the invalidate-token, but to validate that a given token is correct.
To make that clear to everybody, I think we should state that in PHP doc for this method.

I would like to keep the token returned to varnish. Varnish will cache it and don't bother PHP for PURGE requests with invalid token. But this will work only if the cache doesn't Vary by X-Invalidate-Token.
I think it doesn't, but I'll recheck to be sure.

@ITernovtsii
Copy link
Contributor Author

@vidarl I rechecked and see token validated properly (on varnish) for requests with invalid token, without sending /_ez_http_invalidatetoken to backend. So, I would vote to keep PR as is, without BC changes.

@ITernovtsii ITernovtsii requested review from vidarl and removed request for a team December 16, 2020 10:39
Copy link
Member

@vidarl vidarl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Fine by me. I'll leave it up to @ezsystems/php-dev-team to reflect a bit on my comment: #136 (review)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants