launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/single-secure-create_token into lp:launchpad.

Commit message:
Use create_token everywhere for generating random strings, and switch it to use a CSPRNG.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/single-secure-create_token/+merge/232873

Use create_token everywhere for generating random strings, and switch it to use a CSPRNG.

TimeLimitedToken's and VoteSet's internal random string implementations are replaced with create_token, and all tokens are now at least 20 characters (112 bits) long. create_token now uses random.SystemRandom(), which consumes /dev/urandom rather than an insecure Mersenne Twister. create_unique_token_for_table is abolished, so we crash if we're getting into birthday paradox territory rather than silently carrying on.
-- 
https://code.launchpad.net/~wgrant/launchpad/single-secure-create_token/+merge/232873
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/single-secure-create_token into lp:launchpad.

=== modified file 'lib/lp/registry/model/poll.py'
--- lib/lp/registry/model/poll.py	2013-01-07 02:40:55 +0000
+++ lib/lp/registry/model/poll.py	2014-09-01 10:41:30 +0000
@@ -14,7 +14,6 @@
     ]
 
 from datetime import datetime
-import random
 
 import pytz
 from sqlobject import (
@@ -51,6 +50,7 @@
     SQLBase,
     sqlvalues,
     )
+from lp.services.tokens import create_token
 
 
 class Poll(SQLBase):
@@ -152,7 +152,7 @@
         secret one.
         """
         assert self.isOpen(when=when), "This poll is not open"
-        assert not self.personVoted(person), "Can't vote twice in the same poll"
+        assert not self.personVoted(person), "Can't vote twice in one poll"
         assert person.inTeam(self.team), (
             "Person %r is not a member of this poll's team." % person)
 
@@ -170,7 +170,7 @@
         assert self.type == PollAlgorithm.CONDORCET
         voteset = getUtility(IVoteSet)
 
-        token = voteset.newToken()
+        token = create_token(20)
         votes = []
         activeoptions = self.getActiveOptions()
         for option, preference in options.items():
@@ -200,7 +200,7 @@
             assert option.poll == self, (
                 "The option %r doesn't belong to this poll" % option)
             assert option.active, "Option %r is not active" % option
-        token = voteset.newToken()
+        token = create_token(20)
         # This is a simple-style poll, so you can vote only on a single option
         # and this option's preference must be 1
         preference = 1
@@ -285,7 +285,8 @@
                 datecloses=datecloses, secrecy=secrecy,
                 allowspoilt=allowspoilt, type=poll_type)
 
-    def selectByTeam(self, team, status=PollStatus.ALL, orderBy=None, when=None):
+    def selectByTeam(self, team, status=PollStatus.ALL, orderBy=None,
+                     when=None):
         """See IPollSet."""
         if when is None:
             when = datetime.now(pytz.timezone('UTC'))
@@ -293,7 +294,6 @@
         if orderBy is None:
             orderBy = Poll.sortingColumns
 
-
         status = set(status)
         status_clauses = []
         if PollStatus.OPEN in status:
@@ -411,15 +411,6 @@
 
     implements(IVoteSet)
 
-    def newToken(self):
-        """See IVoteSet."""
-        chars = '23456789bcdfghjkmnpqrstvwxzBCDFGHJKLMNPQRSTVWXZ'
-        length = 10
-        token = ''.join([random.choice(chars) for c in range(length)])
-        while self.getByToken(token):
-            token = ''.join([random.choice(chars) for c in range(length)])
-        return token
-
     def new(self, poll, option, preference, token, person):
         """See IVoteSet."""
         return Vote(poll=poll, option=option, preference=preference,
@@ -435,4 +426,3 @@
             raise OptionIsNotFromSimplePoll(
                 '%r is not an option of a simple-style poll.' % option)
         return Vote.selectBy(option=option).count()
-

=== modified file 'lib/lp/services/librarian/model.py'
--- lib/lp/services/librarian/model.py	2013-06-20 05:50:00 +0000
+++ lib/lp/services/librarian/model.py	2014-09-01 10:41:30 +0000
@@ -12,8 +12,6 @@
     ]
 
 from datetime import datetime
-from hashlib import md5
-import random
 from urlparse import urlparse
 
 from lazr.delegates import delegates
@@ -68,6 +66,7 @@
     IRestrictedLibrarianClient,
     LIBRARIAN_SERVER_DEFAULT_TIMEOUT,
     )
+from lp.services.tokens import create_token
 
 
 class LibraryFileContent(SQLBase):
@@ -319,18 +318,9 @@
         :return: A url fragment token ready to be attached to the url.
             e.g. 'a%20token'
         """
-        # We use random.random to get a string which varies reasonably, and we
-        # hash it to distribute it widely and get a easily copy and pastable
-        # single string (nice for debugging). The randomness is not a key
-        # factor here: as long as tokens are not guessable, they are hidden by
-        # https, not exposed directly in the API (tokens will be allocated by
-        # the appropriate objects), not by direct access to the
-        # TimeLimitedToken class.
-        baseline = str(random.random())
-        hashed = md5(baseline).hexdigest()
-        token = hashed
         store = session_store()
         path = TimeLimitedToken.url_to_token_path(url)
+        token = create_token(32).encode('ascii')
         store.add(TimeLimitedToken(path, token))
         # The session isn't part of the main transaction model, and in fact it
         # has autocommit on. The commit here is belts and bracers: after

=== modified file 'lib/lp/services/oauth/model.py'
--- lib/lp/services/oauth/model.py	2014-09-01 03:47:11 +0000
+++ lib/lp/services/oauth/model.py	2014-09-01 10:41:30 +0000
@@ -46,10 +46,7 @@
     IOAuthRequestToken,
     IOAuthRequestTokenSet,
     )
-from lp.services.tokens import (
-    create_token,
-    create_unique_token_for_table,
-    )
+from lp.services.tokens import create_token
 from lp.services.webapp.interfaces import (
     AccessLevel,
     OAuthPermission,
@@ -392,11 +389,13 @@
     :table: The table in which the key/secret are going to be used. Must be
         one of OAuthAccessToken or OAuthRequestToken.
 
-    The key will have a length of 20 and we'll make sure it's not yet in the
-    given table.  The secret will have a length of 80.
+    The key will have a length of 20. The secret will have a length of 80.
     """
+    # Even a length of 20 has 112 bits of entropy, so uniqueness is a
+    # good assumption. If we generate a duplicate then the DB insertion
+    # will crash, which is desirable because it indicates an RNG issue.
     key_length = 20
-    key = create_unique_token_for_table(key_length, getattr(table, "key"))
+    key = create_token(key_length)
     secret_length = 80
     secret = create_token(secret_length)
     return key, secret

=== modified file 'lib/lp/services/tokens.py'
--- lib/lp/services/tokens.py	2013-06-20 05:50:00 +0000
+++ lib/lp/services/tokens.py	2014-09-01 10:41:30 +0000
@@ -7,13 +7,10 @@
 
 __all__ = [
     'create_token',
-    'create_unique_token_for_table',
     ]
 
 import random
 
-from lp.services.database.interfaces import IMasterStore
-
 
 def create_token(token_length):
     """Create a random token string.
@@ -24,24 +21,6 @@
     # below to prevent them from having curse/offensive words.
     characters = '0123456789bcdfghjklmnpqrstvwxzBCDFGHJKLMNPQRSTVWXZ'
     token = ''.join(
-        random.choice(characters) for count in range(token_length))
+        random.SystemRandom().choice(characters)
+        for count in range(token_length))
     return unicode(token)
-
-
-def create_unique_token_for_table(token_length, column):
-    """Create a new unique token in a table.
-
-    Generates a token and makes sure it does not already exist in
-    the table and column specified.
-
-    :param token_length: The length for the token string
-    :param column: Database column where the token will be stored.
-
-    :return: A new token string
-    """
-    # Use the master Store to ensure no race conditions. 
-    store = IMasterStore(column.cls)
-    token = create_token(token_length)
-    while store.find(column.cls, column==token).one() is not None:
-        token = create_token(token_length)
-    return token

=== modified file 'lib/lp/services/verification/model/logintoken.py'
--- lib/lp/services/verification/model/logintoken.py	2013-06-20 05:50:00 +0000
+++ lib/lp/services/verification/model/logintoken.py	2014-09-01 10:41:30 +0000
@@ -36,7 +36,7 @@
     format_address,
     simple_sendmail,
     )
-from lp.services.tokens import create_unique_token_for_table
+from lp.services.tokens import create_token
 from lp.services.verification.interfaces.authtoken import LoginTokenType
 from lp.services.verification.interfaces.logintoken import (
     ILoginToken,
@@ -339,7 +339,7 @@
             # Aha! According to our policy, we shouldn't raise ValueError.
             raise ValueError(
                 "tokentype is not an item of LoginTokenType: %s" % tokentype)
-        token = create_unique_token_for_table(20, LoginToken.token)
+        token = create_token(20)
         return LoginToken(requester=requester, requesteremail=requesteremail,
                           email=email, token=token, tokentype=tokentype,
                           created=UTC_NOW, fingerprint=fingerprint,

=== modified file 'lib/lp/services/verification/tests/test_token_creation.py'
--- lib/lp/services/verification/tests/test_token_creation.py	2011-12-30 06:14:56 +0000
+++ lib/lp/services/verification/tests/test_token_creation.py	2014-09-01 10:41:30 +0000
@@ -3,18 +3,9 @@
 
 __metaclass__ = type
 
-import random
-
 import testtools
 
-from lp.services.database.constants import UTC_NOW
-from lp.services.tokens import (
-    create_token,
-    create_unique_token_for_table,
-    )
-from lp.services.verification.interfaces.authtoken import LoginTokenType
-from lp.services.verification.model.logintoken import LoginToken
-from lp.testing.layers import DatabaseFunctionalLayer
+from lp.services.tokens import create_token
 
 
 class Test_create_token(testtools.TestCase):
@@ -22,28 +13,3 @@
     def test_length(self):
         token = create_token(99)
         self.assertEquals(len(token), 99)
-
-
-class Test_create_unique_token_for_table(testtools.TestCase):
-    layer = DatabaseFunctionalLayer
-
-    def test_token_uniqueness(self):
-        orig_state = random.getstate()
-        self.addCleanup(lambda: random.setstate(orig_state))
-        # Calling create_unique_token_for_table() twice with the same
-        # random.seed() will generate two identical tokens, as the token was
-        # never inserted in the table.
-        random.seed(0)
-        token1 = create_unique_token_for_table(99, LoginToken.token)
-        random.seed(0)
-        token2 = create_unique_token_for_table(99, LoginToken.token)
-        self.assertEquals(token1, token2)
-
-        # Now insert the token in the table so that the next time we call
-        # create_unique_token_for_table() we get a different token.
-        LoginToken(
-            requester=None, token=token2, email='email@xxxxxxxxxxx',
-            tokentype=LoginTokenType.ACCOUNTMERGE, created=UTC_NOW)
-        random.seed(0)
-        token3 = create_unique_token_for_table(99, LoginToken.token)
-        self.assertNotEquals(token1, token3)

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2014-09-01 07:51:32 +0000
+++ lib/lp/soyuz/model/archive.py	2014-09-01 10:41:30 +0000
@@ -110,10 +110,7 @@
     cachedproperty,
     get_property_cache,
     )
-from lp.services.tokens import (
-    create_token,
-    create_unique_token_for_table,
-    )
+from lp.services.tokens import create_token
 from lp.services.webapp.authorization import check_permission
 from lp.services.webapp.interfaces import ILaunchBag
 from lp.services.webapp.url import urlappend
@@ -1905,7 +1902,7 @@
 
         # Now onto the actual token creation:
         if token is None:
-            token = create_unique_token_for_table(20, ArchiveAuthToken.token)
+            token = create_token(20)
         archive_auth_token = ArchiveAuthToken()
         archive_auth_token.archive = self
         archive_auth_token.person = person
@@ -2409,8 +2406,7 @@
 
         # Private teams cannot have public PPAs.
         if owner.visibility == PersonVisibility.PRIVATE:
-            new_archive.buildd_secret = create_unique_token_for_table(
-                20, Archive.buildd_secret)
+            new_archive.buildd_secret = create_token(20)
             new_archive.private = True
         else:
             new_archive.private = private