launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/oauth-secret-sha256 into lp:launchpad with lp:~wgrant/launchpad/oauth-secret-concealment as a prerequisite.

Commit message:
Provide a feature flag to let OAuth secrets be stored as SHA-256 rather than plaintext. Either form is accepted while we migrate.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/oauth-secret-sha256/+merge/224778

Provide a feature flag to let OAuth secrets be stored as SHA-256 rather than plaintext. Either form is accepted while we migrate.

We don't need a computationally expensive KDF here, as the tokens already have log(50, 2) * 80 ~~ 300 bits of entropy, so they're not feasible to attack with bruteforce, rainbow tables, or similar approaches.
-- 
https://code.launchpad.net/~wgrant/launchpad/oauth-secret-sha256/+merge/224778
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/oauth-secret-sha256 into lp:launchpad.

=== modified file 'lib/lp/services/oauth/model.py'
--- lib/lp/services/oauth/model.py	2014-06-27 09:38:50 +0000
+++ lib/lp/services/oauth/model.py	2014-06-27 09:38:50 +0000
@@ -16,6 +16,7 @@
     datetime,
     timedelta,
     )
+import hashlib
 import re
 
 import pytz
@@ -43,6 +44,7 @@
 from lp.services.database.interfaces import IMasterStore
 from lp.services.database.sqlbase import SQLBase
 from lp.services.database.stormbase import StormBase
+from lp.services.features import getFeatureFlag
 from lp.services.librarian.model import LibraryFileAlias
 from lp.services.oauth.interfaces import (
     ClockSkew,
@@ -114,6 +116,11 @@
     key = StringCol(notNull=True)
     _secret = StringCol(dbName="secret", notNull=False, default='')
 
+    def __init__(self, key, secret):
+        if getFeatureFlag('auth.hash_oauth_secrets'):
+            secret = hashlib.sha256(secret).hexdigest()
+        super(OAuthConsumer, self).__init__(key=key, _secret=secret)
+
     # This regular expression singles out a consumer key that
     # represents any and all apps running on a specific computer. The
     # regular expression identifies the system type (eg. the OS) and
@@ -173,13 +180,13 @@
 
     def isSecretValid(self, secret):
         """See `IOAuthConsumer`."""
-        return secret == self._secret
+        return self._secret in (secret, hashlib.sha256(secret).hexdigest())
 
     def newRequestToken(self):
         """See `IOAuthConsumer`."""
         key, secret = create_token_key_and_secret(table=OAuthRequestToken)
         return (
-            OAuthRequestToken(consumer=self, key=key, _secret=secret), secret)
+            OAuthRequestToken(consumer=self, key=key, secret=secret), secret)
 
     def getAccessToken(self, key):
         """See `IOAuthConsumer`."""
@@ -198,7 +205,7 @@
         """See `IOAuthConsumerSet`."""
         assert self.getByKey(key) is None, (
             "The key '%s' is already in use by another consumer." % key)
-        return OAuthConsumer(key=key, _secret=secret)
+        return OAuthConsumer(key=key, secret=secret)
 
     def getByKey(self, key):
         """See `IOAuthConsumerSet`."""
@@ -232,6 +239,17 @@
         dbName='distribution', foreignKey='Distribution',
         notNull=False, default=None)
 
+    def __init__(self, consumer, permission, key, secret='', person=None,
+                 date_expires=None, product=None, project=None,
+                 distribution=None, sourcepackagename=None):
+        if getFeatureFlag('auth.hash_oauth_secrets'):
+            secret = hashlib.sha256(secret).hexdigest()
+        super(OAuthAccessToken, self).__init__(
+            consumer=consumer, permission=permission, key=key,
+            _secret=secret, person=person, date_expires=date_expires,
+            product=product, project=project, distribution=distribution,
+            sourcepackagename=sourcepackagename)
+
     @property
     def context(self):
         """See `IOAuthToken`."""
@@ -255,7 +273,7 @@
 
     def isSecretValid(self, secret):
         """See `IOAuthConsumer`."""
-        return secret == self._secret
+        return self._secret in (secret, hashlib.sha256(secret).hexdigest())
 
     def checkNonceAndTimestamp(self, nonce, timestamp):
         """See `IOAuthAccessToken`."""
@@ -320,6 +338,17 @@
         dbName='distribution', foreignKey='Distribution',
         notNull=False, default=None)
 
+    def __init__(self, consumer, key, secret='', permission=None, person=None,
+                 date_expires=None, product=None, project=None,
+                 distribution=None, sourcepackagename=None):
+        if getFeatureFlag('auth.hash_oauth_secrets'):
+            secret = hashlib.sha256(secret).hexdigest()
+        super(OAuthRequestToken, self).__init__(
+            consumer=consumer, permission=permission, key=key,
+            _secret=secret, person=person, date_expires=date_expires,
+            product=product, project=project, distribution=distribution,
+            sourcepackagename=sourcepackagename)
+
     @property
     def context(self):
         """See `IOAuthToken`."""
@@ -344,7 +373,7 @@
 
     def isSecretValid(self, secret):
         """See `IOAuthConsumer`."""
-        return secret == self._secret
+        return self._secret in (secret, hashlib.sha256(secret).hexdigest())
 
     def review(self, user, permission, context=None, date_expires=None):
         """See `IOAuthRequestToken`."""
@@ -389,7 +418,7 @@
         access_level = AccessLevel.items[self.permission.name]
         access_token = OAuthAccessToken(
             consumer=self.consumer, person=self.person, key=key,
-            _secret=secret, permission=access_level,
+            secret=secret, permission=access_level,
             date_expires=self.date_expires, product=self.product,
             project=self.project, distribution=self.distribution,
             sourcepackagename=self.sourcepackagename)

=== modified file 'lib/lp/services/oauth/tests/test_tokens.py'
--- lib/lp/services/oauth/tests/test_tokens.py	2014-06-27 09:38:50 +0000
+++ lib/lp/services/oauth/tests/test_tokens.py	2014-06-27 09:38:50 +0000
@@ -11,6 +11,7 @@
     datetime,
     timedelta,
     )
+import hashlib
 
 import pytz
 import transaction
@@ -18,6 +19,7 @@
 from zope.security.interfaces import Unauthorized
 from zope.security.proxy import removeSecurityProxy
 
+from lp.services.features.testing import FeatureFixture
 from lp.services.mail import stub
 from lp.services.oauth.interfaces import (
     IOAuthAccessToken,
@@ -105,6 +107,8 @@
 class TestRequestTokens(TestOAuth):
     """Tests for OAuth request token objects."""
 
+    secret_len = 80
+
     def test_newRequestToken(self):
         request_token, secret = self.consumer.newRequestToken()
         verifyObject(IOAuthRequestToken, request_token)
@@ -114,7 +118,8 @@
     def test_key_and_secret_automatically_generated(self):
         request_token, secret = self.consumer.newRequestToken()
         self.assertEqual(len(request_token.key), 20)
-        self.assertEqual(len(removeSecurityProxy(request_token)._secret), 80)
+        self.assertEqual(
+            len(removeSecurityProxy(request_token)._secret), self.secret_len)
 
     def test_date_created(self):
         request_token, _ = self.consumer.newRequestToken()
@@ -263,6 +268,23 @@
         self.assertEquals(person.oauth_request_tokens.count(), 0)
 
 
+class TestRequestTokensHashed(TestRequestTokens):
+
+    secret_len = 64
+
+    def setUp(self):
+        super(TestRequestTokensHashed, self).setUp()
+        self.useFixture(FeatureFixture({'auth.hash_oauth_secrets': 'on'}))
+
+    def test_newRequestToken(self):
+        request_token, secret = self.consumer.newRequestToken()
+        verifyObject(IOAuthRequestToken, request_token)
+        self.assertIsInstance(secret, unicode)
+        self.assertEqual(
+            removeSecurityProxy(request_token)._secret,
+            hashlib.sha256(secret).hexdigest())
+
+
 class TestAccessTokens(TestOAuth):
     """Tests for OAuth access tokens."""
 
@@ -389,6 +411,24 @@
         self.assertEquals(person.oauth_access_tokens.count(), 0)
 
 
+class TestAccessTokensHashed(TestAccessTokens):
+
+    def setUp(self):
+        super(TestAccessTokensHashed, self).setUp()
+        self.useFixture(FeatureFixture({'auth.hash_oauth_secrets': 'on'}))
+
+    def test_exchange_request_token_for_access_token(self):
+        # Make sure the basic exchange of request token for access
+        # token works.
+        request_token, access_token, access_secret = (
+            self._exchange_request_token_for_access_token())
+        verifyObject(IOAuthAccessToken, access_token)
+        self.assertIsInstance(access_secret, unicode)
+        self.assertEqual(
+            removeSecurityProxy(access_token)._secret,
+            hashlib.sha256(access_secret).hexdigest())
+
+
 class TestHelperFunctions(TestOAuth):
 
     def setUp(self):