Encrypt in-app payment secrets (bug 742751)

apps/amo/models.py

@@ -365,3 +365,16 @@ def manual_order(qs, pks, pk_name='id'):
             select={'_manual': 'FIELD(%s, %s)'
                 % (pk_name, ','.join(map(str, pks)))},
             order_by=['_manual'])
+
+
+class VarbinaryField(models.Field):
+    """MySQL varbinary column.
+
+    This is for using AES_ENCYPT() to store values.
+    It could maybe turn into a fancy transparent encypt/decrypt field
+    like http://djangosnippets.org/snippets/2489/
+    """
+    description = "Varbinary"
+
+    def db_type(self, **kw):
+        return 'varbinary(%s)' % self.max_length

lib/crypto/__init__.py

@@ -0,0 +1 @@
+from .util import *

lib/crypto/util.py

@@ -0,0 +1,19 @@
+import base64
+import os
+
+
+__all__ = ['generate_key']
+
+
+def generate_key(byte_length):
+    """Return a true random ascii string that is byte_length long.
+
+    The resulting key is suitable for cryptogrpahy.
+    """
+    if byte_length < 32:  # at least 256 bit
+        raise ValueError('um, %s is probably not long enough for cryptography'
+                         % byte_length)
+    key = os.urandom(byte_length)
+    key = base64.b64encode(key).rstrip('=')  # strip off padding
+    key = key[0:byte_length]
+    return key

migrations/379-inapp-config-fix.sql

@@ -0,0 +1,6 @@
+-- We can't use these values anymore. The side effect here is that each app will
+-- have to reconfigure their in-app payments.
+DELETE FROM addon_inapp_log;
+DELETE FROM addon_inapp_payment;
+DELETE FROM addon_inapp;
+ALTER TABLE addon_inapp MODIFY COLUMN private_key VARBINARY(128) NULL;

mkt/developers/tests/test_views_payments.py

@@ -1,13 +1,17 @@
+import mock
 from nose.tools import eq_
 from pyquery import PyQuery as pq
 import waffle
 
+from django.conf import settings
+
 from addons.models import Addon
 import amo
 import amo.tests
 from mkt.inapp_pay.models import InappConfig
 
 
+@mock.patch.object(settings, 'DEBUG', True)
 class TestInappConfig(amo.tests.TestCase):
     fixtures = ['base/apps', 'base/users', 'webapps/337141-steamcube']
 
@@ -23,12 +27,13 @@ class TestInappConfig(amo.tests.TestCase):
     def config(self, public_key='pub-key', private_key='priv-key',
                status=amo.INAPP_STATUS_ACTIVE,
                postback_url='/postback', chargeback_url='/chargeback'):
-        return InappConfig.objects.create(public_key=public_key,
-                                          private_key=private_key,
-                                          addon=self.webapp,
-                                          status=status,
-                                          postback_url=postback_url,
-                                          chargeback_url=chargeback_url)
+        cfg = InappConfig.objects.create(public_key=public_key,
+                                         addon=self.webapp,
+                                         status=status,
+                                         postback_url=postback_url,
+                                         chargeback_url=chargeback_url)
+        cfg.set_private_key(private_key)
+        return cfg
 
     def post(self, data, expect_error=False):
         resp = self.client.post(self.url, data, follow=True)
@@ -44,7 +49,7 @@ class TestInappConfig(amo.tests.TestCase):
         eq_(inapp.postback_url, '/postback')
         eq_(inapp.status, amo.INAPP_STATUS_ACTIVE)
         assert inapp.public_key, 'public key was not generated'
-        assert inapp.private_key, 'private key was not generated'
+        assert inapp.has_private_key(), 'private key was not generated'
 
     def test_hide_inactive_keys(self):
         self.config(status=amo.INAPP_STATUS_INACTIVE)
@@ -61,11 +66,12 @@ class TestInappConfig(amo.tests.TestCase):
 
     def test_regenerate_keys_when_inactive(self):
         old_inapp = self.config(status=amo.INAPP_STATUS_INACTIVE)
+        old_secret = old_inapp.get_private_key()
         self.post(dict(chargeback_url='/chargeback', postback_url='/postback'))
         inapp = InappConfig.objects.get(addon=self.webapp,
                                         status=amo.INAPP_STATUS_ACTIVE)
-        assert inapp.private_key != old_inapp.private_key, (
-                    '%s != %s' % (inapp.private_key, old_inapp.private_key))
+        new_secret = inapp.get_private_key()
+        assert new_secret != old_secret, '%s != %s' % (new_secret, old_secret)
 
     def test_bad_urls(self):
         resp = self.post(dict(chargeback_url='chargeback',
@@ -86,12 +92,12 @@ class TestInappConfig(amo.tests.TestCase):
         eq_(inapp.postback_url, '/new/postback')
         eq_(inapp.status, amo.INAPP_STATUS_ACTIVE)
         eq_(inapp.public_key, 'exisiting-pub-key')
-        eq_(inapp.private_key, 'exisiting-priv-key')
+        eq_(inapp.get_private_key(), 'exisiting-priv-key')
 
     def test_show_secret(self):
-        inapp = self.config()
+        self.config(private_key='123456')
         resp = self.client.get(self.webapp.get_dev_url('in_app_secret'))
-        eq_(resp.content, inapp.private_key)
+        eq_(resp.content, '123456')
 
     def test_deny_secret_to_no_auth(self):
         self.config()

mkt/developers/views.py

@@ -439,9 +439,9 @@ def in_app_config(request, addon_id, addon, webapp=True):
         new_inapp.status = amo.INAPP_STATUS_ACTIVE
         if not new_inapp.public_key:
             new_inapp.public_key = InappConfig.generate_public_key()
-        if not new_inapp.private_key:
-            new_inapp.private_key = InappConfig.generate_private_key()
         new_inapp.save()
+        if not new_inapp.has_private_key():
+            new_inapp.set_private_key(InappConfig.generate_private_key())
 
         messages.success(request, _('Changes successfully saved.'))
         return redirect(addon.get_dev_url('in_app_config'))
@@ -456,7 +456,7 @@ def in_app_config(request, addon_id, addon, webapp=True):
 def in_app_secret(request, addon_id, addon, webapp=True):
     inapp_config = get_object_or_404(InappConfig, addon=addon,
                                      status=amo.INAPP_STATUS_ACTIVE)
-    return http.HttpResponse(inapp_config.private_key)
+    return http.HttpResponse(inapp_config.get_private_key())
 
 
 def _premium(request, addon_id, addon, webapp=False):

mkt/inapp_pay/models.py

@@ -1,13 +1,13 @@
-import base64
-import os
 import random
 
 from django.conf import settings
-from django.db import models
+from django.db import connection, models
 
 from tower import ugettext_lazy as _lazy
 
 import amo
+from amo.models import VarbinaryField
+from lib.crypto import generate_key
 
 
 class TooManyKeyGenAttempts(Exception):
@@ -25,7 +25,8 @@ class InappConfig(amo.models.ModelBase):
         help_text=_lazy(u'Relative URL in your app that the marketplace will '
                         u'post a confirmed transaction to. For example: '
                         u'/payments/postback'))
-    private_key = models.CharField(max_length=255, unique=True)
+    _encrypted_private_key = VarbinaryField(max_length=128, blank=True,
+                                            null=True, db_column='private_key')
     public_key = models.CharField(max_length=255, unique=True, db_index=True)
     # Allow https to be configurable only if it's declared in settings.
     # This is intended for development.
@@ -45,6 +46,29 @@ class InappConfig(amo.models.ModelBase):
     def __unicode__(self):
         return u'%s: %s' % (self.addon, self.status)
 
+    def get_private_key(self):
+        """Get the real private key from the database."""
+        cursor = connection.cursor()
+        cursor.execute('select AES_DECRYPT(private_key, %s) '
+                       'from addon_inapp where id=%s', [_get_key(), self.id])
+        secret = cursor.fetchone()[0]
+        if not secret:
+            raise ValueError('Secret was empty! It either was not set or '
+                             'the decryption key is wrong')
+        return str(secret)  # make sure it is in bytes
+
+    def has_private_key(self):
+        return bool(self._encrypted_private_key)
+
+    def set_private_key(self, raw_value):
+        """Store the private key in the database."""
+        if isinstance(raw_value, unicode):
+            raw_value = raw_value.encode('ascii')
+        cursor = connection.cursor()
+        cursor.execute('update addon_inapp set '
+                       'private_key = AES_ENCRYPT(%s, %s)',
+                       [raw_value, _get_key()])
+
     @classmethod
     def any_active(cls, addon, exclude_config=None):
         """
@@ -79,14 +103,14 @@ class InappConfig(amo.models.ModelBase):
 
     @classmethod
     def generate_private_key(cls, max_tries=40):
+        """Generate a random 43 character secret key."""
 
-        def gen_key():
-            """Generate a random 43 character secret key."""
-            key = os.urandom(32)  # 256 bit
-            return base64.b64encode(key).rstrip('=')  # strip off padding
-
-        for key in limited_keygen(gen_key, max_tries):
-            if cls.objects.filter(private_key=key).count() == 0:
+        for key in limited_keygen(lambda: generate_key(43), max_tries):
+            cursor = connection.cursor()
+            cursor.execute('select count(*) from addon_inapp where '
+                           'private_key = AES_ENCRYPT(%s, %s) ',
+                           [key, _get_key()])
+            if cursor.fetchone()[0] == 0:
                 return key
 
     def app_protocol(self):
@@ -104,6 +128,16 @@ def limited_keygen(gen_key, max_tries):
                                 % max_tries)
 
 
+def _get_key():
+    """Get the key used to encrypt data in the db."""
+    if (not settings.DEBUG and
+        settings.INAPP_KEY_PATH.endswith('inapp-sample-pay.key')):
+        raise EnvironmentError('encryption key looks like the one we '
+                               'committed to the repo!')
+    with open(settings.INAPP_KEY_PATH, 'rb') as fp:
+        return fp.read()
+
+
 class InappPayLog(amo.models.ModelBase):
     action = models.IntegerField()
     session_key = models.CharField(max_length=64)

mkt/inapp_pay/tasks.py

@@ -74,7 +74,7 @@ def _notify(payment_id, notice_type, extra_response=None):
                                             'description': payment.description,
                                             'productdata': payment.app_data},
                                 'response': response},
-                               config.private_key,
+                               config.get_private_key(),
                                algorithm='HS256')
     try:
         res = urlopen(url, signed_notice, timeout=5)

mkt/inapp_pay/tests/resources/inapp-sample-pay-alt.key

@@ -0,0 +1 @@
+<pretend this is a key different than the sample>

mkt/inapp_pay/tests/resources/inapp-sample-pay.key

@@ -0,0 +1 @@
+<pretend this is randomly generated text>

mkt/inapp_pay/tests/test_ipn.py

@@ -3,18 +3,23 @@ from mock import patch
 from nose.tools import eq_
 import waffle
 
+from django.conf import settings
+
 from addons.models import Addon
 import amo
 from mkt.inapp_pay.models import InappConfig
 from mkt.inapp_pay.tests.test_views import InappPaymentUtil
-from paypal.tests.test_views import PaypalTest, sample_reversal, sample_chained_refund
+from paypal.tests.test_views import (PaypalTest, sample_reversal,
+                                     sample_chained_refund)
 from users.models import UserProfile
 
 
 @patch('paypal.views.urllib2.urlopen')
+@patch.object(settings, 'DEBUG', True)
 class TestInappIPN(InappPaymentUtil, PaypalTest):
     fixtures = ['webapps/337141-steamcube', 'base/users']
 
+    @patch.object(settings, 'DEBUG', True)
     def setUp(self):
         super(TestInappIPN, self).setUp()
         self.app = self.get_app()
@@ -51,7 +56,8 @@ class TestInappIPN(InappPaymentUtil, PaypalTest):
     @fudge.patch('mkt.inapp_pay.tasks.chargeback_notify')
     def test_refund(self, urlopen, chargeback_notify):
         urlopen.return_value = self.urlopener('VERIFIED')
-        con = self.make_contrib(transaction_id=sample_chained_refund['tracking_id'])
+        tx = sample_chained_refund['tracking_id']
+        con = self.make_contrib(transaction_id=tx)
         pay = self.make_payment(contrib=con)
 
         chargeback_notify.expects('delay').with_args(pay.pk, 'refund')

mkt/inapp_pay/tests/test_models.py

@@ -1,11 +1,16 @@
 import inspect
+import os
+
+from django.conf import settings
 
 import fudge
+import mock
 from nose.tools import eq_, raises
 
 import amo
 import amo.tests
-from mkt.inapp_pay.models import InappConfig, TooManyKeyGenAttempts, InappPayLog
+from mkt.inapp_pay.models import (InappConfig, TooManyKeyGenAttempts,
+                                  InappPayLog)
 from mkt.inapp_pay import verify
 from mkt.inapp_pay.verify import InappPaymentError
 from mkt.webapps.models import Webapp
@@ -16,7 +21,6 @@ class TestInapp(amo.tests.TestCase):
     def setUp(self):
         self.app = Webapp.objects.create(manifest_url='http://foo.ca')
         self.inapp = InappConfig.objects.create(addon=self.app,
-                                                private_key='asd',
                                                 public_key='asd')
 
     def test_active(self):
@@ -27,7 +31,7 @@ class TestInapp(amo.tests.TestCase):
     def test_any_active_excludes_config_under_edit(self):
         c = InappConfig.objects.create(addon=self.app,
                                        status=amo.INAPP_STATUS_ACTIVE,
-                                       private_key='asd-1', public_key='asd-1')
+                                       public_key='asd-1')
         assert not InappConfig.any_active(self.app, exclude_config=c.pk)
         c.save()  # no exception
 
@@ -35,24 +39,27 @@ class TestInapp(amo.tests.TestCase):
         assert not InappConfig.any_active(self.app)
         InappConfig.objects.create(addon=self.app,
                                    status=amo.INAPP_STATUS_ACTIVE,
-                                   private_key='asd-1', public_key='asd-1')
+                                   public_key='asd-1')
         assert InappConfig.any_active(self.app)
         self.assertRaises(ValueError, InappConfig.objects.create,
                           addon=self.app, status=amo.INAPP_STATUS_ACTIVE,
-                          private_key='asd-2', public_key='asd-2')
+                          public_key='asd-2')
 
     def test_generate_public_key(self):
         key = InappConfig.generate_public_key()
         assert key
 
+    @mock.patch.object(settings, 'DEBUG', True)
     def test_generate_private_key(self):
         key = InappConfig.generate_private_key()
         assert key
 
     @raises(TooManyKeyGenAttempts)
-    @fudge.patch('mkt.inapp_pay.models.InappConfig.objects.filter')
-    def test_exhaust_private_keygen_attempts(self, fake_filter):
-        fake_filter.expects_call().returns_fake().expects('count').returns(1)
+    @mock.patch.object(settings, 'DEBUG', True)
+    @fudge.patch('mkt.inapp_pay.models.connection')
+    def test_exhaust_private_keygen_attempts(self, fake_conn):
+        (fake_conn.expects('cursor').returns_fake()
+                  .expects('execute').expects('fetchone').returns([1]))
         InappConfig.generate_private_key(max_tries=5)
 
     @raises(TooManyKeyGenAttempts)
@@ -61,20 +68,50 @@ class TestInapp(amo.tests.TestCase):
         fake_filter.expects_call().returns_fake().expects('count').returns(1)
         InappConfig.generate_public_key(max_tries=5)
 
-    @fudge.patch('mkt.inapp_pay.models.InappConfig.objects.filter')
-    def test_retry_private_keygen_until_unique(self, fake_filter):
-        (fake_filter.expects_call().returns_fake().expects('count').returns(1)
-                                                  .next_call().returns(1)
-                                                  .next_call().returns(0))
+    @mock.patch.object(settings, 'DEBUG', True)
+    @fudge.patch('mkt.inapp_pay.models.connection')
+    def test_retry_private_keygen_until_unique(self, fake_conn):
+        (fake_conn.expects('cursor').returns_fake()
+                  .expects('execute')
+                  .expects('fetchone').returns([1])
+                  .next_call().returns([1])
+                  .next_call().returns([0]))
         assert InappConfig.generate_private_key(max_tries=5)
 
     @fudge.patch('mkt.inapp_pay.models.InappConfig.objects.filter')
     def test_retry_public_keygen_until_unique(self, fake_filter):
-        (fake_filter.expects_call().returns_fake().expects('count').returns(1)
-                                                  .next_call().returns(1)
-                                                  .next_call().returns(0))
+        (fake_filter.expects_call().returns_fake()
+                                   .expects('count').returns(1)
+                                   .next_call().returns(1)
+                                   .next_call().returns(0))
         assert InappConfig.generate_public_key(max_tries=5)
 
+    @raises(EnvironmentError)
+    @mock.patch.object(settings, 'DEBUG', False)
+    def test_key_path_cannot_match_sample(self):
+        self.inapp.set_private_key('sekret')
+
+    @mock.patch.object(settings, 'DEBUG', True)
+    def test_encrypted_key_storage(self):
+        sk = 'this is the secret'
+        self.inapp.set_private_key(sk)
+        eq_(self.inapp.get_private_key(), sk)
+        cfg = InappConfig.objects.get(pk=self.inapp.pk)
+        assert cfg._encrypted_private_key != sk, (
+                        'secret was not encrypted: %s'
+                        % cfg._encrypted_private_key)
+
+    @raises(ValueError)
+    @mock.patch.object(settings, 'DEBUG', True)
+    def test_wrong_key(self):
+        sk = 'your coat is hidden under the stairs'
+        self.inapp.set_private_key(sk)
+        with mock.patch.object(settings, 'INAPP_KEY_PATH',
+                               os.path.join(os.path.dirname(__file__),
+                                            'resources',
+                                            'inapp-sample-pay-alt.key')):
+            self.inapp.get_private_key()
+
 
 def test_exception_mapping():
     at_least_one = False

mkt/inapp_pay/tests/test_tasks.py

@@ -20,6 +20,7 @@ from mkt.inapp_pay.models import InappPayNotice
 from mkt.inapp_pay.tests.test_views import PaymentTest
 
 
+@mock.patch.object(settings, 'DEBUG', True)
 class TestNotifyApp(PaymentTest):
 
     def setUp(self):
@@ -52,7 +53,7 @@ class TestNotifyApp(PaymentTest):
             dd = jwt.decode(req, verify=False)
             eq_(dd['request'], payload['request'])
             eq_(dd['typ'], payload['typ'])
-            jwt.decode(req, self.inapp_config.private_key, verify=True)
+            jwt.decode(req, self.inapp_config.get_private_key(), verify=True)
             return True
 
         (urlopen.expects_call().with_args(url, arg.passes_test(req_ok),
@@ -79,7 +80,7 @@ class TestNotifyApp(PaymentTest):
             eq_(dd['typ'], payload['typ'])
             eq_(dd['response']['transactionID'], self.contrib.pk)
             eq_(dd['response']['reason'], 'refund')
-            jwt.decode(req, self.inapp_config.private_key, verify=True)
+            jwt.decode(req, self.inapp_config.get_private_key(), verify=True)
             return True
 
         (urlopen.expects_call().with_args(url, arg.passes_test(req_ok),
@@ -98,7 +99,6 @@ class TestNotifyApp(PaymentTest):
     @fudge.patch('mkt.inapp_pay.tasks.urlopen')
     def test_notify_reversal_chargeback(self, urlopen):
         url = self.url(self.chargeback)
-        payload = self.payload(typ='mozilla/payments/pay/chargeback/v1')
 
         def req_ok(req):
             dd = jwt.decode(req, verify=False)
@@ -219,7 +219,7 @@ class TestNotifyApp(PaymentTest):
         # Ensure that the JWT sent to the app for payment notification
         # includes the same payment data that the app originally sent.
         def is_valid(payload):
-            data = jwt.decode(payload, self.inapp_config.private_key,
+            data = jwt.decode(payload, self.inapp_config.get_private_key(),
                               verify=True)
             eq_(data['iss'], settings.INAPP_MARKET_ID)
             eq_(data['aud'], self.inapp_config.public_key)

mkt/inapp_pay/tests/test_verify.py

@@ -18,6 +18,7 @@ from mkt.inapp_pay.verify import (verify_request, UnknownAppError,
                                   InvalidRequest)
 
 
+@mock.patch.object(settings, 'DEBUG', True)
 class TestVerify(PaymentTest):
 
     def verify(self, request=None, update=None, update_request=None):

mkt/inapp_pay/tests/test_views.py

@@ -73,16 +73,19 @@ class InappPaymentUtil:
         }
 
 
+@mock.patch.object(settings, 'DEBUG', True)
 class PaymentTest(InappPaymentUtil, amo.tests.TestCase):
     fixtures = ['webapps/337141-steamcube', 'base/users']
 
+    @mock.patch.object(settings, 'DEBUG', True)
     def setUp(self):
         self.app = self.get_app()
         cfg = self.inapp_config = InappConfig(addon=self.app,
                                               status=amo.INAPP_STATUS_ACTIVE)
         cfg.public_key = self.app_id = InappConfig.generate_public_key()
-        cfg.private_key = self.app_secret = InappConfig.generate_private_key()
+        self.app_secret = InappConfig.generate_private_key()
         cfg.save()
+        cfg.set_private_key(self.app_secret)
         self.app.paypal_id = 'app-dev-paypal@theapp.com'
         self.app.save()
 
@@ -109,6 +112,7 @@ class PaymentViewTest(PaymentTest):
                                  password='password')
 
 
+@mock.patch.object(settings, 'DEBUG', True)
 class TestPayStart(PaymentViewTest):
 
     def test_missing_pay_request_on_start(self):
@@ -150,6 +154,7 @@ class TestPayStart(PaymentViewTest):
         self.assertContains(rp, 'RequestVerificationError')
 
 
+@mock.patch.object(settings, 'DEBUG', True)
 class TestPay(PaymentViewTest):
 
     def setUp(self):

mkt/inapp_pay/verify.py

@@ -103,7 +103,7 @@ def verify_request(signed_request):
     # TODO(Kumar) see bug 736573 for HSM integration
     try:
         with statsd.timer('inapp_pay.verify'):
-            jwt.decode(signed_request, cfg.private_key, verify=True)
+            jwt.decode(signed_request, cfg.get_private_key(), verify=True)
     except jwt.DecodeError, exc:
         _re_raise_as(RequestVerificationError,
                      'Payment verification failed: %s' % exc,

mkt/settings.py

@@ -166,4 +166,8 @@ INAPP_VERBOSE_ERRORS = False
 # This is useful for development and testing.
 INAPP_REQUIRE_HTTPS = True
 
+# Path to key for local AES encrypt/decrypt.
+INAPP_KEY_PATH = os.path.join(ROOT, 'mkt', 'inapp_pay', 'tests', 'resources',
+                              'inapp-sample-pay.key')
+
 #CACHE_EMPTY_QUERYSETS = True

mkt/zadmin/management/commands/genkey.py

@@ -0,0 +1,28 @@
+import os
+from optparse import make_option
+
+from django.core.management.base import BaseCommand, CommandError
+
+from lib.crypto import generate_key
+
+
+class Command(BaseCommand):
+    help = 'Generate a randomized encryption encryption key'
+    option_list = BaseCommand.option_list + (
+        make_option('--dest', action='store',
+                    help='Location for key file. Default: %default',
+                    default='./encryption.key'),
+        make_option('--length', action='store', type=int,
+                    help='Key length in bytes. Default: %default',
+                    default=128),
+    )
+
+    def handle(self, *args, **options):
+        if os.path.exists(options['dest']):
+            raise CommandError('Key file already exists at %s; remove it '
+                               'first or specify a new path with --dest'
+                               % options['dest'])
+        with open(options['dest'], 'wb') as fp:
+            fp.write(generate_key(options['length']))
+        os.chmod(options['dest'], 0600)
+        print 'Wrote in-app payment key: %s' % options['dest']

mkt/zadmin/tests/test_commands.py

@@ -0,0 +1,27 @@
+import os
+import shutil
+import tempfile
+
+from django.conf import settings
+from django.core.management.base import CommandError
+
+from nose.tools import eq_, raises
+
+import amo.tests
+
+from mkt.zadmin.management.commands.genkey import Command
+
+
+class TestCommand(amo.tests.TestCase):
+
+    def test_gen_key(self):
+        tmp = tempfile.mkdtemp()
+        self.addCleanup(lambda: shutil.rmtree(tmp))
+        tmp_key = os.path.join(tmp, 'inapp.key')
+        Command().handle(dest=tmp_key, length=256)
+        with open(tmp_key, 'r') as fp:
+            eq_(len(fp.read()), 256)
+
+    @raises(CommandError)
+    def test_gen_key_existing(self):
+        Command().handle(dest=settings.INAPP_KEY_PATH)