Re: svn commit: r1070179 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_cache.xml modules/cache/cache_storage.c modules/cache/cache_storage.h modules/cache/mod_cache.c modules/cache/mod_cache.h
On 23 Nov 2011, at 1:15 PM, Graham Leggett wrote:
In order to make invalidation possible, we would need to add an invalidate()
function to the mod_cache provider, and keep AP_CACHE_INVALIDATE inside
ap_cache_status_e. Invalidation involves marking the entry as invalidated and
to be freshened on next hit, as opposed to deleted.
What I could do is back out the change, and make just the above API changes,
allowing us to backport this to v2.4 when we are ready.
This releases v2.4 to get on with it, and keeps the option open for us to fix
this properly at a future date in the v2.4 cycle when we're ready.
The change has been reverted on v2.4, and the following patch gives the API
changes that allow us to fix this in the v2.4 series:
Index: modules/cache/mod_cache.h
===
--- modules/cache/mod_cache.h (revision 1208384)
+++ modules/cache/mod_cache.h (working copy)
@@ -109,17 +109,20 @@
const char *urlkey);
int (*remove_url) (cache_handle_t *h, request_rec *r);
apr_status_t (*commit_entity)(cache_handle_t *h, request_rec *r);
+apr_status_t (*invalidate_entity)(cache_handle_t *h, request_rec *r);
} cache_provider;
typedef enum {
AP_CACHE_HIT,
AP_CACHE_REVALIDATE,
-AP_CACHE_MISS
+AP_CACHE_MISS,
+AP_CACHE_INVALIDATE
} ap_cache_status_e;
#define AP_CACHE_HIT_ENV cache-hit
#define AP_CACHE_REVALIDATE_ENV cache-revalidate
#define AP_CACHE_MISS_ENV cache-miss
+#define AP_CACHE_INVALIDATE_ENV cache-invalidate
#define AP_CACHE_STATUS_ENV cache-status
Index: modules/cache/mod_cache_disk.c
===
--- modules/cache/mod_cache_disk.c (revision 1208379)
+++ modules/cache/mod_cache_disk.c (working copy)
@@ -1325,6 +1325,11 @@
return APR_SUCCESS;
}
+static apr_status_t invalidate_entity(cache_handle_t *h, request_rec *r)
+{
+return APR_ENOTIMPL;
+}
+
static void *create_dir_config(apr_pool_t *p, char *dummy)
{
disk_cache_dir_conf *dconf = apr_pcalloc(p, sizeof(disk_cache_dir_conf));
@@ -1502,7 +1507,8 @@
create_entity,
open_entity,
remove_url,
-commit_entity
+commit_entity,
+invalidate_entity
};
static void disk_cache_register_hook(apr_pool_t *p)
Regards,
Graham
--
Re: svn commit: r1070179 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_cache.xml modules/cache/cache_storage.c modules/cache/cache_storage.h modules/cache/mod_cache.c modules/cache/mod_cache.h
On 30 Nov 2011, at 1:30 PM, Graham Leggett wrote: The change has been reverted on v2.4, and the following patch gives the API changes that allow us to fix this in the v2.4 series: Applied in r 1208822 and backported in r1208824. Regards, Graham --
Re: svn commit: r1070179 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_cache.xml modules/cache/cache_storage.c modules/cache/cache_storage.h modules/cache/mod_cache.c modules/cache/mod_cache.h
On 22 Nov 2011, at 11:58 PM, Stefan Fritsch wrote: From the thread, it is not clear to me that there was consensous on how to fix it. Therefore I would vote for reverting r1070179 from 2.4, fixing it in trunk, and then backporting the whole thing to 2.4 again (maybe after 2.4.0). In order to make invalidation possible, we would need to add an invalidate() function to the mod_cache provider, and keep AP_CACHE_INVALIDATE inside ap_cache_status_e. Invalidation involves marking the entry as invalidated and to be freshened on next hit, as opposed to deleted. What I could do is back out the change, and make just the above API changes, allowing us to backport this to v2.4 when we are ready. This releases v2.4 to get on with it, and keeps the option open for us to fix this properly at a future date in the v2.4 cycle when we're ready. Thoughts? Regards, Graham --
Re: svn commit: r1070179 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_cache.xml modules/cache/cache_storage.c modules/cache/cache_storage.h modules/cache/mod_cache.c modules/cache/mod_cache.h
On 13 Feb 2011, at 9:59 AM, Roy T. Fielding wrote: On Feb 12, 2011, at 6:03 PM, minf...@apache.org wrote: Author: minfrin Date: Sun Feb 13 02:03:29 2011 New Revision: 1070179 URL: http://svn.apache.org/viewvc?rev=1070179view=rev Log: mod_cache: When a request other than GET or HEAD arrives, we must invalidate existing cache entities as per RFC2616 13.10. PR 15868. Cache entries should not be invalidated unless the response comes back as a success (2xx). Likewise, it only applies to methods with known write effects (not including TRACE, OPTIONS, PROPFIND, etc.). This has already been updated in httpbis p6. Reviving this unresolved issue. I need to update r1070179 to fix this, do I a) revert r1070179 on v2.4.x and continue to fix on trunk, or b) fix this on trunk and v2.4.x at the risk of potential further delay? Regards, Graham --
Re: svn commit: r1070179 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_cache.xml modules/cache/cache_storage.c modules/cache/cache_storage.h modules/cache/mod_cache.c modules/cache/mod_cache.h
On Tuesday 22 November 2011, Graham Leggett wrote: On 13 Feb 2011, at 9:59 AM, Roy T. Fielding wrote: On Feb 12, 2011, at 6:03 PM, minf...@apache.org wrote: Author: minfrin Date: Sun Feb 13 02:03:29 2011 New Revision: 1070179 URL: http://svn.apache.org/viewvc?rev=1070179view=rev Log: mod_cache: When a request other than GET or HEAD arrives, we must invalidate existing cache entities as per RFC2616 13.10. PR 15868. Cache entries should not be invalidated unless the response comes back as a success (2xx). Likewise, it only applies to methods with known write effects (not including TRACE, OPTIONS, PROPFIND, etc.). This has already been updated in httpbis p6. Reviving this unresolved issue. I need to update r1070179 to fix this, do I a) revert r1070179 on v2.4.x and continue to fix on trunk, or b) fix this on trunk and v2.4.x at the risk of potential further delay? From the thread, it is not clear to me that there was consensous on how to fix it. Therefore I would vote for reverting r1070179 from 2.4, fixing it in trunk, and then backporting the whole thing to 2.4 again (maybe after 2.4.0).
Re: svn commit: r1070179 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_cache.xml modules/cache/cache_storage.c modules/cache/cache_storage.h modules/cache/mod_cache.c modules/cache/mod_cache.h
On Feb 13, 2011, at 5:03 AM, Graham Leggett wrote: On 13 Feb 2011, at 9:59 AM, Roy T. Fielding wrote: URL: http://svn.apache.org/viewvc?rev=1070179view=rev Log: mod_cache: When a request other than GET or HEAD arrives, we must invalidate existing cache entities as per RFC2616 13.10. PR 15868. Cache entries should not be invalidated unless the response comes back as a success (2xx). Likewise, it only applies to methods with known write effects (not including TRACE, OPTIONS, PROPFIND, etc.). This has already been updated in httpbis p6. Hmmm... So is it right to say that POST, PUT and DELETE should invalidate, while all others would be left alone? Or do we need some kind of mechanism for modules that respond to new methods to declare those methods as needing to be invalidated or not? Well, I thought I could answer that with a simple link to the drafts, but they just got changed in the wrong direction. It should be implemented as invalidating when the response is 2xx and the method is *not* one of the safe ones (GET, HEAD, OPTIONS, TRACE, PROPFIND, and maybe REPORT). That way, a new extension method would invalidate on success. Should the cache try and cache any of the other methods, or is cacheability limited to GET only? It can cache other methods if it knows how to. PROPFIND is probably the only other method worth caching, but its key depends on a bunch of webdav stuff outside the URI. Roy
Re: svn commit: r1070179 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_cache.xml modules/cache/cache_storage.c modules/cache/cache_storage.h modules/cache/mod_cache.c modules/cache/mod_cache.h
On Feb 12, 2011, at 6:03 PM, minf...@apache.org wrote: Author: minfrin Date: Sun Feb 13 02:03:29 2011 New Revision: 1070179 URL: http://svn.apache.org/viewvc?rev=1070179view=rev Log: mod_cache: When a request other than GET or HEAD arrives, we must invalidate existing cache entities as per RFC2616 13.10. PR 15868. Cache entries should not be invalidated unless the response comes back as a success (2xx). Likewise, it only applies to methods with known write effects (not including TRACE, OPTIONS, PROPFIND, etc.). This has already been updated in httpbis p6. Roy
Re: svn commit: r1070179 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_cache.xml modules/cache/cache_storage.c modules/cache/cache_storage.h modules/cache/mod_cache.c modules/cache/mod_cache.h
On 13 Feb 2011, at 9:59 AM, Roy T. Fielding wrote: URL: http://svn.apache.org/viewvc?rev=1070179view=rev Log: mod_cache: When a request other than GET or HEAD arrives, we must invalidate existing cache entities as per RFC2616 13.10. PR 15868. Cache entries should not be invalidated unless the response comes back as a success (2xx). Likewise, it only applies to methods with known write effects (not including TRACE, OPTIONS, PROPFIND, etc.). This has already been updated in httpbis p6. Hmmm... So is it right to say that POST, PUT and DELETE should invalidate, while all others would be left alone? Or do we need some kind of mechanism for modules that respond to new methods to declare those methods as needing to be invalidated or not? Should the cache try and cache any of the other methods, or is cacheability limited to GET only? Regards, Graham --
Re: svn commit: r1070179 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_cache.xml modules/cache/cache_storage.c modules/cache/cache_storage.h modules/cache/mod_cache.c modules/cache/mod_cache.h
On 02/13/2011 03:03 AM, minf...@apache.org wrote: Author: minfrin Date: Sun Feb 13 02:03:29 2011 New Revision: 1070179 URL: http://svn.apache.org/viewvc?rev=1070179view=rev Log: mod_cache: When a request other than GET or HEAD arrives, we must invalidate existing cache entities as per RFC2616 13.10. PR 15868. Modified: httpd/httpd/trunk/CHANGES httpd/httpd/trunk/docs/manual/mod/mod_cache.xml httpd/httpd/trunk/modules/cache/cache_storage.c httpd/httpd/trunk/modules/cache/cache_storage.h httpd/httpd/trunk/modules/cache/mod_cache.c httpd/httpd/trunk/modules/cache/mod_cache.h Modified: httpd/httpd/trunk/modules/cache/cache_storage.c URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cache/cache_storage.c?rev=1070179r1=1070178r2=1070179view=diff == --- httpd/httpd/trunk/modules/cache/cache_storage.c (original) +++ httpd/httpd/trunk/modules/cache/cache_storage.c Sun Feb 13 02:03:29 2011 @@ -389,6 +389,53 @@ int cache_select(cache_request_rec *cach return DECLINED; } +/* + * invalidate a specific URL entity in all caches + * + * All cached entities for this URL are removed, usually in + * response to a POST/PUT or DELETE. + * + * This function returns OK if at least one entity was found and + * removed, and DECLINED if no cached entities were removed. + */ +int cache_invalidate(cache_request_rec *cache, request_rec *r) Why not adjusting cache_remove_url accordingly? It does nearly the same and has the same prototype. Regards Rüdiger
Re: svn commit: r1070179 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_cache.xml modules/cache/cache_storage.c modules/cache/cache_storage.h modules/cache/mod_cache.c modules/cache/mod_cache.h
On 13 Feb 2011, at 5:08 PM, Ruediger Pluem wrote: +/* + * invalidate a specific URL entity in all caches + * + * All cached entities for this URL are removed, usually in + * response to a POST/PUT or DELETE. + * + * This function returns OK if at least one entity was found and + * removed, and DECLINED if no cached entities were removed. + */ +int cache_invalidate(cache_request_rec *cache, request_rec *r) Why not adjusting cache_remove_url accordingly? It does nearly the same and has the same prototype. Nearly the same, but not enough. cache_remove_url() only takes effect if a previous cached entry was found, while cache_invalidate() is unconditional. As it turns out, invalidating the entry unconditionally is wrong, as we need to do the invalidation only on 2xx. This means this code needs to be changed, waiting for Roy to confirm the exact behaviour before changing it. Regards, Graham --
Re: svn commit: r1070179 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_cache.xml modules/cache/cache_storage.c modules/cache/cache_storage.h modules/cache/mod_cache.c modules/cache/mod_cache.h
On Sun, Feb 13, 2011 at 5:03 AM, Graham Leggett minf...@sharp.fm wrote: On 13 Feb 2011, at 9:59 AM, Roy T. Fielding wrote: URL: http://svn.apache.org/viewvc?rev=1070179view=rev Log: mod_cache: When a request other than GET or HEAD arrives, we must invalidate existing cache entities as per RFC2616 13.10. PR 15868. Cache entries should not be invalidated unless the response comes back as a success (2xx). Likewise, it only applies to methods with known write effects (not including TRACE, OPTIONS, PROPFIND, etc.). This has already been updated in httpbis p6. Hmmm... So is it right to say that POST, PUT and DELETE should invalidate, while all others would be left alone? Or do we need some kind of mechanism for modules that respond to new methods to declare those methods as needing to be invalidated or not? Should the cache try and cache any of the other methods, or is cacheability limited to GET only? Additionally, this should be a configurable behavior. Lets say you run a popular website that depends on mod_cache to protect backend systems from complete overload. All you need to do now as an attacker is POST / DELETE to / or another important URL every 200ms, and the cache becomes invalidated, causing a flood of requests to backends that might not be able to support it. Thoughts? Thanks, Paul
Re: svn commit: r1070179 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_cache.xml modules/cache/cache_storage.c modules/cache/cache_storage.h modules/cache/mod_cache.c modules/cache/mod_cache.h
On 14 Feb 2011, at 1:56 AM, Paul Querna wrote: Additionally, this should be a configurable behavior. Lets say you run a popular website that depends on mod_cache to protect backend systems from complete overload. All you need to do now as an attacker is POST / DELETE to / or another important URL every 200ms, and the cache becomes invalidated, causing a flood of requests to backends that might not be able to support it. Thoughts? How is this different from Cache-Control: no-cache in the request? Regards, Graham --
Re: svn commit: r1070179 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_cache.xml modules/cache/cache_storage.c modules/cache/cache_storage.h modules/cache/mod_cache.c modules/cache/mod_cache.h
On Sun, Feb 13, 2011 at 4:00 PM, Graham Leggett minf...@sharp.fm wrote: On 14 Feb 2011, at 1:56 AM, Paul Querna wrote: Additionally, this should be a configurable behavior. Lets say you run a popular website that depends on mod_cache to protect backend systems from complete overload. All you need to do now as an attacker is POST / DELETE to / or another important URL every 200ms, and the cache becomes invalidated, causing a flood of requests to backends that might not be able to support it. Thoughts? How is this different from Cache-Control: no-cache in the request? It does a single request to the backend, but doesn't _invalidate_ the existing cache, which would cause a flood of other, non-attacker clients to come in.
Re: svn commit: r1070179 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_cache.xml modules/cache/cache_storage.c modules/cache/cache_storage.h modules/cache/mod_cache.c modules/cache/mod_cache.h
On 14 Feb 2011, at 2:15 AM, Paul Querna wrote: It does a single request to the backend, but doesn't _invalidate_ the existing cache, which would cause a flood of other, non-attacker clients to come in. I think that would be the origin of Roy saying that we should only invalidate if the result is 2xx. Someone trying methods in the hope they would do something would get a 405 Method Not Supported. Regards, Graham --
Re: svn commit: r1070179 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_cache.xml modules/cache/cache_storage.c modules/cache/cache_storage.h modules/cache/mod_cache.c modules/cache/mod_cache.h
On 02/14/2011 01:23 AM, Graham Leggett wrote: On 14 Feb 2011, at 2:15 AM, Paul Querna wrote: It does a single request to the backend, but doesn't _invalidate_ the existing cache, which would cause a flood of other, non-attacker clients to come in. I think that would be the origin of Roy saying that we should only invalidate if the result is 2xx. Someone trying methods in the hope they would do something would get a 405 Method Not Supported. Not sure about Roy, but I guess Pauls fear was more about POST which could be handled by the backend in the same way as GET and hence would provide an easy remote way to remove cache entries. Of course one could argue that the backend application should be fixed then, but I guess we have many examples where the flexibility of httpd makes it possible to work around bugs in backends where they are not fixable for whatever reason :-). Regards Rüdiger
Re: svn commit: r1070179 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_cache.xml modules/cache/cache_storage.c modules/cache/cache_storage.h modules/cache/mod_cache.c modules/cache/mod_cache.h
On 02/13/2011 11:29 PM, Graham Leggett wrote:
On 13 Feb 2011, at 5:08 PM, Ruediger Pluem wrote:
+/*
+ * invalidate a specific URL entity in all caches
+ *
+ * All cached entities for this URL are removed, usually in
+ * response to a POST/PUT or DELETE.
+ *
+ * This function returns OK if at least one entity was found and
+ * removed, and DECLINED if no cached entities were removed.
+ */
+int cache_invalidate(cache_request_rec *cache, request_rec *r)
Why not adjusting cache_remove_url accordingly? It does nearly the
same and
has the same prototype.
Nearly the same, but not enough. cache_remove_url() only takes effect if
a previous cached entry was found, while cache_invalidate() is
unconditional.
I guess current cache_remove_url has a bug that you correctly pointed to with
cache_invalidate:
while(list) {
list-provider-remove_url(h, r);
list = list-next;
}
looks wrong because we must pass a provider specific h to each remove_url call.
So I think the
rv = list-provider-open_entity(h, r, cache-key);
step would be needed in remove_url as well, which of course makes
if (!cache-key) {
rv = cache_generate_key(r, r-pool, cache-key);
if (rv != APR_SUCCESS) {
return DECLINED;
}
}
needed there as well.
Or cache_remove_url should be changed that it only flushes the cache entry in
the
backend original selected (cache-provider).
Regards
Rüdiger
