HttpCache looks in need of a rewrite

[Brian Eaton <be...@google.com>]

Hey folks -

I don't have time to look at in detail right now, but in debugging a
problem with OAuth I stumbled across the HttpCache interface.  I think
it may be broken in at least two ways.

1) It doesn't take into account request method when checking the
cache.  URI alone is sufficient.  The requests HEAD /foo, POST /foo
and GET /foo have totally different cache semantics.  Are these being
handled somewhere?

2) It doesn't take into account authentication.  A signed request for
a GET /foo shares the same cache namespace as an unsigned GET /foo.
It looks like the SigningFetcher class is very careful to strip out
any unpredictable portions of the URI to improve the cache hit rate,
with the end result that you can retrieve data intended for another
user simply by knowing their opensocial id: just send a makeRequest
with authn=NONE, and craft the URL to match what SigningFetcher
creates for the cache!

Again, excuse me if I'm missing something here; I've only read the
code briefly, I haven't really tested either of these, and I may have
missed the code that addresses these issues.

Cheers,
Brian

Re: HttpCache looks in need of a rewrite

[Kevin Brown <et...@google.com>]

On Tue, May 27, 2008 at 4:14 PM, Brian Eaton <be...@google.com> wrote:

> On Tue, May 27, 2008 at 9:55 AM, Louis Ryan <lr...@google.com> wrote:
> > Another step we can take in the meantime is to forbid the use of
> > opensocial_viewer_id and opensocial_owner_id from non-signed requests and
> > even the existence of those values from non-signed requests to deter
> folks
> > from using them in untrustable ways.
>
> This sounds like (another) permanent scar on the opensocial APIs.
>
> How hard will it be for us to change the cache interface so it knows
> about authentication types and which users data is keyed off of?
>

It's not too hard at all -- if nobody else does it, I can patch this in
pretty trivially.

Re: HttpCache looks in need of a rewrite

[Brian Eaton <be...@google.com>]

On Tue, May 27, 2008 at 9:55 AM, Louis Ryan <lr...@google.com> wrote:
> Another step we can take in the meantime is to forbid the use of
> opensocial_viewer_id and opensocial_owner_id from non-signed requests and
> even the existence of those values from non-signed requests to deter folks
> from using them in untrustable ways.

This sounds like (another) permanent scar on the opensocial APIs.

How hard will it be for us to change the cache interface so it knows
about authentication types and which users data is keyed off of?

Re: HttpCache looks in need of a rewrite

[Louis Ryan <lr...@google.com>]

Another step we can take in the meantime is to forbid the use of
opensocial_viewer_id and opensocial_owner_id from non-signed requests and
even the existence of those values from non-signed requests to deter folks
from using them in untrustable ways.

On Sun, May 25, 2008 at 1:18 PM, Kevin Brown <et...@google.com> wrote:

> On Sun, May 25, 2008 at 9:22 AM, Brian Eaton <be...@google.com> wrote:
>
> > Hey folks -
> >
> > I don't have time to look at in detail right now, but in debugging a
> > problem with OAuth I stumbled across the HttpCache interface.  I think
> > it may be broken in at least two ways.
> >
> > 1) It doesn't take into account request method when checking the
> > cache.  URI alone is sufficient.  The requests HEAD /foo, POST /foo
> > and GET /foo have totally different cache semantics.  Are these being
> > handled somewhere?
>
>
> Only GET is cached (HEAD isn't actually supported, though it could be
> cached, too).
>
>
> > 2) It doesn't take into account authentication.  A signed request for
> > a GET /foo shares the same cache namespace as an unsigned GET /foo.
> > It looks like the SigningFetcher class is very careful to strip out
> > any unpredictable portions of the URI to improve the cache hit rate,
> > with the end result that you can retrieve data intended for another
> > user simply by knowing their opensocial id: just send a makeRequest
> > with authn=NONE, and craft the URL to match what SigningFetcher
> > creates for the cache!
>
>
> Exactly -- I pointed this problem out to Louis the other day. I already
> have
> a patch that cleans up these issues with HttpCache (related to my post a
> few
> weeks ago titled 'ContentFetchers are a mess'. This is another step in
> fixing that. The cache key should be a string, not a URI, and it shouldn't
> be spoofable like this.
>
>
> >
> > Again, excuse me if I'm missing something here; I've only read the
> > code briefly, I haven't really tested either of these, and I may have
> > missed the code that addresses these issues.
> >
> > Cheers,
> > Brian
> >
>

Re: HttpCache looks in need of a rewrite

[Kevin Brown <et...@google.com>]

On Sun, May 25, 2008 at 9:22 AM, Brian Eaton <be...@google.com> wrote:

> Hey folks -
>
> I don't have time to look at in detail right now, but in debugging a
> problem with OAuth I stumbled across the HttpCache interface.  I think
> it may be broken in at least two ways.
>
> 1) It doesn't take into account request method when checking the
> cache.  URI alone is sufficient.  The requests HEAD /foo, POST /foo
> and GET /foo have totally different cache semantics.  Are these being
> handled somewhere?


Only GET is cached (HEAD isn't actually supported, though it could be
cached, too).


> 2) It doesn't take into account authentication.  A signed request for
> a GET /foo shares the same cache namespace as an unsigned GET /foo.
> It looks like the SigningFetcher class is very careful to strip out
> any unpredictable portions of the URI to improve the cache hit rate,
> with the end result that you can retrieve data intended for another
> user simply by knowing their opensocial id: just send a makeRequest
> with authn=NONE, and craft the URL to match what SigningFetcher
> creates for the cache!


Exactly -- I pointed this problem out to Louis the other day. I already have
a patch that cleans up these issues with HttpCache (related to my post a few
weeks ago titled 'ContentFetchers are a mess'. This is another step in
fixing that. The cache key should be a string, not a URI, and it shouldn't
be spoofable like this.


>
> Again, excuse me if I'm missing something here; I've only read the
> code briefly, I haven't really tested either of these, and I may have
> missed the code that addresses these issues.
>
> Cheers,
> Brian
>