launchpad-reviewers team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

Robert Collins has proposed merging lp:~lifeless/launchpad/private-librarian into lp:launchpad.

Requested reviews:
  Stuart Bishop (stub)
  Launchpad code reviewers (launchpad-reviewers)


I'm proposing this to get feedback on the approach - I put it together on the plane so it has had zero discussion so far.

The basic idea is to have an https librarian that uses an access token for a time limited period, rather than proxying on the appservers which is terrible in several ways that aren't all that relevant except to say its hard to improve and incompatible with our peformance goals.

So in this model, we hand out a token when someone (including wget) accesses a private attachment on launchpad, and issue a temporary redirect (over ssl) to https://launchpadlibrarian.net/...file?token=xxxxx

The token goes in the session DB, the garbo cleans that up, and we all are happy happy happy.

Oh, and the librarian rejects requests without a token for private files.

We can't use OAuth because then the OAuth token would be attackable by content in the private librarian.
-- 
https://code.launchpad.net/~lifeless/launchpad/private-librarian/+merge/31020
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/private-librarian into lp:launchpad.

=== modified file 'database/schema/launchpad_session.sql'
--- database/schema/launchpad_session.sql	2009-06-24 21:17:33 +0000
+++ database/schema/launchpad_session.sql	2010-07-27 05:26:48 +0000
@@ -14,3 +14,7 @@
 GRANT EXECUTE ON FUNCTION
     set_session_pkg_data(text, text, text, bytea) TO session;
 
+-- Let the session user access file access tokens.
+GRANT SELECT, INSERT, UPDATE, DELETE ON TimeLimitedToken TO session;
+-- And the garbo needs to run on it too.
+GRANT SELECT, DELETE ON TimeLimitedToken TO session;

=== modified file 'database/schema/session.sql'
--- database/schema/session.sql	2007-10-17 07:51:37 +0000
+++ database/schema/session.sql	2010-07-27 05:26:48 +0000
@@ -33,6 +33,18 @@
     ) WITHOUT OIDS;
 COMMENT ON TABLE SessionPkgData IS 'Stores the actual session data as a Python pickle.';
 
+CREATE TABLE TimeLimitedToken (
+    url text NOT NULL,
+    token text NOT NULL,
+    created timestamp with time zone NOT NULL DEFAULT CURRENT_TIMESTAMP,
+    constraint timelimitedtoken_pky primary key (url, token)
+    ) WITHOUT OIDS;
+COMMENT ON TABLE TimeLimitedToken IS 'stores tokens for granting access to a single url for a short while. The garbo takes care of cleanups, and we should only have a few thousand at a time. Tokens are handed out just-in-time on the appserver, when a client attempts to dereference a private thing which we do not want to deliver in-line. OAuth tokens cannot be used for the launchpadlibrarian content because they would then be attackable. See lib.canonical.database.librarian for the python class.';
+-- Give the garbo an efficient selection to cleanup
+CREATE INDEX timelimitedtoken_created ON TimeLimitedToken(created);
+-- Give the librarian an efficient lookup
+CREATE INDEX timelimitedtoken_url_token ON TimeLimitedToken(url, token);
+
 CREATE OR REPLACE FUNCTION ensure_session_client_id(p_client_id text)
 RETURNS VOID AS $$
 BEGIN

=== modified file 'lib/canonical/database/sqlbase.py'
--- lib/canonical/database/sqlbase.py	2010-05-21 13:08:18 +0000
+++ lib/canonical/database/sqlbase.py	2010-07-27 05:26:48 +0000
@@ -59,10 +59,12 @@
     'RandomiseOrderDescriptor',
     'reset_store',
     'rollback',
+    'session_store',
     'SQLBase',
     'sqlvalues',
     'StupidCache',
-    'ZopelessTransactionManager',]
+    'ZopelessTransactionManager',
+    ]
 
 # Default we want for scripts, and the PostgreSQL default. Note psycopg1 will
 # use SERIALIZABLE unless we override, but psycopg2 will not.
@@ -559,7 +561,7 @@
 
     """
     if not isinstance(x, basestring):
-        raise TypeError, 'Not a string (%s)' % type(x)
+        raise TypeError('Not a string (%s)' % type(x))
     return quote(x).replace('%', r'\\%').replace('_', r'\\_')
 
 
@@ -674,6 +676,7 @@
         clause = clause.replace('?', '%s') % sqlvalues(*parameters)
     return clause
 
+
 def flush_database_updates():
     """Flushes all pending database updates.
 
@@ -814,6 +817,7 @@
     DEPRECATED - use of this class is deprecated in favour of using
     Store.execute().
     """
+
     def __init__(self):
         self._connection = _get_sqlobject_store()._connection
         self._result = None
@@ -842,3 +846,8 @@
         if self._result is not None:
             self._result.close()
             self._result = None
+
+
+def session_store():
+    """Return a store connected to the session DB."""
+    return getUtility(IZStorm).get('session', 'launchpad-session:')

=== modified file 'lib/canonical/launchpad/database/librarian.py'
--- lib/canonical/launchpad/database/librarian.py	2009-11-24 15:36:44 +0000
+++ lib/canonical/launchpad/database/librarian.py	2010-07-27 05:26:48 +0000
@@ -18,6 +18,7 @@
 from zope.interface import implements
 
 from sqlobject import StringCol, ForeignKey, IntCol, SQLRelatedJoin, BoolCol
+import storm.base
 from storm.locals import Date, Desc, Int, Reference, Store
 
 from canonical.config import config
@@ -239,3 +240,20 @@
     count = Int(allow_none=False)
     country_id = Int(name='country', allow_none=True)
     country = Reference(country_id, 'Country.id')
+
+
+class TimeLimitedToken(storm.base.Storm):
+    """A time limited access token for accessing a private file."""
+
+    __storm_table__ = 'TimeLimitedToken'
+
+    created = UtcDateTimeCol(notNull=True, default=UTC_NOW)
+    url = StringCol(notNull=True)
+    token = StringCol(notNull=True)
+    __storm_primary__ = ("url", "token")
+
+    def __init__(self, url, token, created=None):
+        """Create a TimeLimitedToken."""
+        self.created = created
+        self.url = url
+        self.token = token

=== modified file 'lib/canonical/launchpad/interfaces/lpstorm.py'
--- lib/canonical/launchpad/interfaces/lpstorm.py	2009-06-25 05:30:52 +0000
+++ lib/canonical/launchpad/interfaces/lpstorm.py	2010-07-27 05:26:48 +0000
@@ -32,4 +32,3 @@
 
 class IMasterObject(IDBObject):
     """A Storm database object associated with its master Store."""
-

=== modified file 'lib/canonical/launchpad/webapp/session.py'
--- lib/canonical/launchpad/webapp/session.py	2010-02-17 13:39:59 +0000
+++ lib/canonical/launchpad/webapp/session.py	2010-07-27 05:26:48 +0000
@@ -14,6 +14,7 @@
 from lazr.uri import URI
 
 from canonical.config import config
+from canonical.database.sqlbase import session_store
 
 
 SECONDS = 1
@@ -78,7 +79,7 @@
         # Secret is looked up here rather than in __init__, because
         # we can't be sure the database connections are setup at that point.
         if self._secret is None:
-            store = getUtility(IZStorm).get('session', 'launchpad-session:')
+            store = session_store()
             result = store.execute("SELECT secret FROM secret")
             self._secret = result.get_one()[0]
         return self._secret

=== modified file 'lib/canonical/testing/layers.py'
--- lib/canonical/testing/layers.py	2010-05-06 15:51:34 +0000
+++ lib/canonical/testing/layers.py	2010-07-27 05:26:48 +0000
@@ -96,7 +96,8 @@
 from canonical.config import CanonicalConfig, config, dbconfig
 from canonical.database.revision import (
     confirm_dbrevision, confirm_dbrevision_on_startup)
-from canonical.database.sqlbase import cursor, ZopelessTransactionManager
+from canonical.database.sqlbase import (cursor, session_store,
+    ZopelessTransactionManager)
 from canonical.launchpad.interfaces import IMailBox, IOpenLaunchBag
 from lp.testing import ANONYMOUS, login, logout, is_logged_in
 import lp.services.mail.stub
@@ -193,9 +194,7 @@
     # as soon as SQLBase._connection is accessed
     r = main_store.execute('SELECT count(*) FROM LaunchpadDatabaseRevision')
     assert r.get_one()[0] > 0, 'Storm is not talking to the database'
-
-    session_store = getUtility(IZStorm).get('session', 'launchpad-session:')
-    assert session_store is not None, 'Failed to reconnect'
+    assert session_store() is not None, 'Failed to reconnect'
 
 
 def wait_children(seconds=120):