-
Notifications
You must be signed in to change notification settings - Fork 84
Implemented Symfony reverse proxy support for user context hash. #152
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,188 @@ | ||
<?php | ||
|
||
/* | ||
* This file is part of the FOSHttpCacheBundle package. | ||
* | ||
* (c) FriendsOfSymfony <http://friendsofsymfony.github.com/> | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
|
||
namespace FOS\HttpCacheBundle; | ||
|
||
use Symfony\Bundle\FrameworkBundle\HttpCache\HttpCache as BaseHttpCache; | ||
use Symfony\Component\HttpFoundation\Request; | ||
use Symfony\Component\HttpFoundation\Response; | ||
use Symfony\Component\HttpKernel\HttpKernelInterface; | ||
|
||
/** | ||
* Base class for enhanced Symfony reverse proxy. | ||
* | ||
* @author Jérôme Vieilledent <[email protected]> (courtesy of eZ Systems AS) | ||
* | ||
* {@inheritdoc} | ||
*/ | ||
abstract class HttpCache extends BaseHttpCache | ||
{ | ||
/** | ||
* Hash for anonymous user. | ||
*/ | ||
const ANONYMOUS_HASH = '38015b703d82206ebc01d17a39c727e5'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could this hash conflict with a hash for an authenticated user ? How is it built ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No it couldn't. This one is computed with md5 while user context hashes are generated with sha256. |
||
|
||
/** | ||
* Accept header value to be used to request the user hash to the backend application. | ||
* It must match the one defined in FOSHttpCacheBundle's configuration. | ||
*/ | ||
const USER_HASH_ACCEPT_HEADER = 'application/vnd.fos.user-context-hash'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what about making the constants private properties instead that are injected through the constructor? then a user could overwrite them much easier when extending the class. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, this would complexify things IMHO. Constants are very easy to customize in that case thanks to late static binding. And in the case of several apps in a project, you would duplicate this class as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh, was not aware of this. i thought constant would mean its There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, I'll do that :-) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Symfony already defines a concept of options in the HttpCache, and the end-user can overwrite a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could avoid changing constructor signature using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After thinking about it and trying a few things, I couldn't find a clean approach to get a dedicated abstract class HttpCache extends BaseHttpCache
{
public function __contruct(HttpKernelInterface $kernel, $cacheDir = null)
{
$this->fosOptions = $this->getFosOptions() + array(
'user_has_header' => 'X-User-Context-Hash',
'user_hash_uri' => '/_fos_user_context_hash',
// ...
);
parent::__construct($kernel, $cacheDir);
}
/**
* One might override this method
*/
protected function getFosOptions()
{
return array();
}
} But I don't find this pretty 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hm, the signature of the HttpCache constructor has $options. why not have this and then keep a copy of $options for us?
i see the thing about missing getOption on the HttpCache, otherwise we would not need a $fosOptions property. we can then follow the same logic and have a private $fosOptions method. and, if you want, a getFosOption() method but i think it should be ok to set all options in constructor and not later. they should not be dynamic i guess. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Because it's ugly ? 😄
This is precisely why I use constants 😉. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. okay, lets leave as is. |
||
|
||
/** | ||
* Name of the header the user context hash will be stored into. | ||
* It must match the one defined in FOSHttpCacheBundle's configuration. | ||
*/ | ||
const USER_HASH_HEADER = 'X-User-Context-Hash'; | ||
|
||
/** | ||
* URI used with the forwarded request for user context hash generation. | ||
*/ | ||
const USER_HASH_URI = '/_fos_user_context_hash'; | ||
|
||
/** | ||
* HTTP Method used with the forwarded request for user context hash generation. | ||
*/ | ||
const USER_HASH_METHOD = 'GET'; | ||
|
||
/** | ||
* Prefix for session names. | ||
* Must match your session configuration. | ||
*/ | ||
const SESSION_NAME_PREFIX = 'PHPSESSID'; | ||
|
||
/** | ||
* Generated user hash. | ||
* | ||
* @var string | ||
*/ | ||
private $userHash; | ||
|
||
public function handle(Request $request, $type = HttpKernelInterface::MASTER_REQUEST, $catch = true) | ||
{ | ||
if (!$this->isInternalRequest($request)) { | ||
// Prevent tampering attacks on the hash mechanism | ||
if ($request->headers->get('accept') === static::USER_HASH_ACCEPT_HEADER | ||
|| $request->headers->get(static::USER_HASH_HEADER) !== null) { | ||
return new Response('', 400); | ||
} | ||
|
||
if ($request->isMethodSafe()) { | ||
$request->headers->set(static::USER_HASH_HEADER, $this->getUserHash($request)); | ||
} | ||
} | ||
|
||
return parent::handle($request, $type, $catch); | ||
} | ||
|
||
/** | ||
* Checks if passed request object is to be considered internal (e.g. for user hash lookup). | ||
* | ||
* @param Request $request | ||
* | ||
* @return bool | ||
*/ | ||
private function isInternalRequest(Request $request) | ||
{ | ||
return $request->attributes->get('internalRequest', false) === true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there nothing more explicit for this? if there isn't, this solution seems ok to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not that I could think about, there is no |
||
} | ||
|
||
/** | ||
* Returns the user context hash for $request. | ||
* | ||
* @param Request $request | ||
* | ||
* @return string | ||
*/ | ||
private function getUserHash(Request $request) | ||
{ | ||
if (isset($this->userHash)) { | ||
return $this->userHash; | ||
} | ||
|
||
if ($this->isAnonymous($request)) { | ||
return $this->userHash = static::ANONYMOUS_HASH; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a performance shortcut with a hardcoded hash for anonymous, right? as the hash lookup is cached, the price seems not that high to me, but still on many cms sites 99% will be anonymous, so it might still be worth it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes it's cached, but having it hardcoded is much much faster for complete anonymous users. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ddboer we might want to propose the same idea in the varnish config for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i created FriendsOfSymfony/FOSHttpCache#132 for this. @lolautruche did you try how relevant this optimization is when using varnish? for the HttpCache php class i totally see its relevant, but do you have any numbers how many milliseconds / load this saves on varnish once the anonymous hash request is cached? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No I never benchmarked this. It's saving a whole request restart anyway so it definitely worths it IMHO, even if it's saving a few milliseconds. |
||
|
||
// Forward the request to generate the user hash | ||
$forwardReq = $this->generateForwardRequest($request); | ||
$resp = $this->handle($forwardReq); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this request will be cached by the HttpCache kernel class, right? otherwise it would be very expensive. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes it will. Tested 1.000 times 😃 |
||
// Store the user hash in memory for sub-requests (processed in the same thread). | ||
$this->userHash = $resp->headers->get(static::USER_HASH_HEADER); | ||
|
||
return $this->userHash; | ||
} | ||
|
||
/** | ||
* Checks if current request is considered anonymous. | ||
* | ||
* @param Request $request | ||
* | ||
* @return bool | ||
*/ | ||
private function isAnonymous(Request $request) | ||
{ | ||
foreach ($request->cookies as $name => $value) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. makes me wonder: what about other authentication methods? basic authentication with the "Authorization" header? or custom authentication methods. i think this method needs to be protected too (sorry @ddeboer, i agree with private as much as we can, but seems here we can't). then for basic auth, you would instead check if that header is not present, and also overwrite the cleanup method to unconditionally remove all cookies. |
||
if ($this->isSessionName($name)) { | ||
return false; | ||
} | ||
} | ||
|
||
return true; | ||
} | ||
|
||
/** | ||
* Checks if passed string can be considered as a session name, such as would be used in cookies. | ||
* | ||
* @param string $name | ||
* | ||
* @return bool | ||
*/ | ||
private function isSessionName($name) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hm, i think we should keep this protected. i could have a different logic or support a legacy cookie or something like that. |
||
{ | ||
return strpos($name, static::SESSION_NAME_PREFIX) === 0; | ||
} | ||
|
||
/** | ||
* Generates the request object that will be forwarded to get the user context hash. | ||
* | ||
* @param Request $request | ||
* | ||
* @return Request | ||
*/ | ||
private function generateForwardRequest(Request $request) | ||
{ | ||
$forwardReq = Request::create(static::USER_HASH_URI, static::USER_HASH_METHOD, array(), array(), array(), $request->server->all()); | ||
$forwardReq->attributes->set('internalRequest', true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this also done by symfony in other cases of internal requests? or is this your way of transporting the information that this is a valid request for the hash through the call to handle()? if its the later, can't we do this simpler, e.g. have a dedicated method that calls parent::handle at the end and have our handle() unconditionally refuse hash requests? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not used anywhere in Symfony, this is just a convenient flag used in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove this line, but in getUserHash do $this->handleInternalRequest which just calls parent::handleInternalRequest. okay, i see the problem, if the user extends handle() it could be surprising. might not be the best idea then. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, can I leave it as is ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah i think its better than my proposal. (just trying to challenge your solutions to see if it could be simpler) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
$forwardReq->headers->set('Accept', static::USER_HASH_ACCEPT_HEADER); | ||
$this->cleanupForwardRequest($forwardReq, $request); | ||
|
||
return $forwardReq; | ||
} | ||
|
||
/** | ||
* Cleans up request to forward for user hash generation. | ||
* Cleans cookie header to only get proper sessionIds in it. This is to make the hash request cacheable. | ||
* | ||
* @param Request $forwardReq | ||
* @param Request $originalRequest | ||
*/ | ||
protected function cleanupForwardRequest(Request $forwardReq, Request $originalRequest) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, that was the whole point why we moved that into its own method, so that it can be tweaked. |
||
{ | ||
$sessionIds = array(); | ||
foreach ($originalRequest->cookies as $name => $value) { | ||
if ( $this->isSessionName($name)) { | ||
$sessionIds[$name] = $value; | ||
$forwardReq->cookies->set($name, $value); | ||
} | ||
} | ||
$forwardReq->headers->set('Cookie', http_build_query($sessionIds, '', '; ')); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks, i think this is more robust now. |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
Symfony HttpCache | ||
================= | ||
|
||
Symfony comes with a built-in reverse proxy written in PHP, known as | ||
``HttpCache``. It can be useful when one hosts a Symfony application on shared | ||
hosting for instance | ||
(see [HttpCache documentation](http://symfony.com/doc/current/book/http_cache.html#symfony-reverse-proxy). | ||
|
||
If you use Symfony ``HttpCache``, you'll need to make your ``AppCache`` class | ||
extend ``FOS\HttpCacheBundle\HttpCache`` instead of | ||
``Symfony\Bundle\FrameworkBundle\HttpCache\HttpCache``. | ||
|
||
.. warning:: | ||
|
||
Symfony HttpCache support is currently limited to following features: | ||
|
||
* User context | ||
|
||
Class constants | ||
--------------- | ||
|
||
``FOS\HttpCacheBundle\HttpCache`` defines constants that can easily be overriden | ||
in your ``AppCache`` class: | ||
|
||
.. code-block:: php | ||
|
||
use FOS\HttpCacheBundle\HttpCache; | ||
|
||
class AppCache extends HttpCache | ||
{ | ||
/** | ||
* Overriding default value for SESSION_NAME_PREFIX | ||
* to use eZSESSID instead. | ||
*/ | ||
const SESSION_NAME_PREFIX = 'eZSESSID'; | ||
} | ||
|
||
User context | ||
~~~~~~~~~~~~ | ||
|
||
.. note:: | ||
|
||
For detailed information on user context, please read the | ||
`user context documentation page </features/user-context>` | ||
|
||
* ``SESSION_NAME_PREFIX``: Prefix for session names. Must match your session | ||
configuration. | ||
Needed for caching correctly generated user context hash for each user session. | ||
|
||
**default**: ``PHPSESSID`` | ||
|
||
.. warning:: | ||
|
||
If you have a customized session name, it is **very important** that this | ||
constant matches it. | ||
Session IDs are indeed used as keys to cache the generated use context hash. | ||
|
||
Wrong session name will lead to unexpected results such as having the same | ||
user context hash for every users, | ||
or not having it cached at all (painful for performance. | ||
|
||
* ``USER_HASH_ACCEPT_HEADER``: Accept header value to be used to request the | ||
user hash to the backend application. | ||
It must match the one defined in FOSHttpCacheBundle's configuration (see below). | ||
|
||
**default**: ``application/vnd.fos.user-context-hash`` | ||
|
||
* ``USER_HASH_HEADER``: Name of the header the user context hash will be stored | ||
into. | ||
It must match the one defined in FOSHttpCacheBundle's configuration (see below). | ||
|
||
**default**: ``X-User-Context-Hash`` | ||
|
||
* ``USER_HASH_URI``: URI used with the forwarded request for user context hash | ||
generation. | ||
|
||
**default**: ``/_fos_user_context_hash`` | ||
|
||
* ``USER_HASH_METHOD``: HTTP Method used with the forwarded request for user | ||
context hash generation. | ||
|
||
**default**: ``GET`` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,9 +11,17 @@ Configuration | |
Caching Proxy Configuration | ||
~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
||
First you need to set up your caching proxy as explained in the | ||
Varnish | ||
""""""" | ||
|
||
Set up Varnish caching proxy as explained in the | ||
:ref:`user context documentation <foshttpcache:user-context>`. | ||
|
||
Symfony reverse proxy | ||
""""""""""""""""""""" | ||
|
||
Set up Symfony reverse proxy as explained in the :doc:`Symfony HttpCache dedicated documentation page </features/symfony-http-cache>`. | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. one blank line too much. |
||
Context Hash Route | ||
~~~~~~~~~~~~~~~~~~ | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this to have the context lookup response being cached?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is. Without it, HttpCache won't cache it... Varnish doesn't care by default, though we added a specific rule in VCL for eZ.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, then this is a very important fix indeed. symfony docs explicitly recommend respecting private and varnish 4 respects it by default...
seems we should have a test that the hash lookup is cached (not in this PR), we should see a failure on varnish 4 for that test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean it's not present and it should be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dbu So I guess https://github.com/ezsystems/ezpublish-community/pull/198/files#diff-ac09e211769f6ae9a0e69c1e08f81c84R107 is not needed then (Varnish 4)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, that is not needed, see https://www.varnish-cache.org/trac/browser/bin/varnishd/builtin.vcl?rev=5c5634dbd9211c4b784268166709a22177d643de#L159 - i would remove it to avoid confusion.
about my comment regarding a missing test for the context being cached by the caches, i created #154