Merge lp:~twom/canonical-identity-provider/unexpiring-discharge-macaroon into lp:canonical-identity-provider

[Tom Wardill]

Remove the expiry caveat from the discharge macaroon.

Expiry is now handled by the root macaroons of snapauth and SCA, so remove it from SSO. This allows the other services to control their own expiry.

[Colin Watson (cjwatson)]

I think you should also replace the 'expires' tests in BuildMacaroonFromRootDischargeTestCase.test_proper_discharging and BuildMacaroonDischargeTestCase.test_proper_discharging with tests that the 'expires' caveat is absent (perhaps just "return False" in the checker function if you encounter one of those). When you've done that, it should also be possible to remove MACAROON_TTL from django_project/settings_base.py.

This can't be landed until the corresponding SCA and snapauth changes are on production, and I'd suggest waiting a week or two after that for safety.

review: Approve

[Colin Watson (cjwatson)]

Oh, and please make sure that MacaroonRefreshTestCase tests the cases of refreshing an otherwise-valid macaroon both with and without an 'expires' caveat.

[lp:~twom/canonical-identity-provider/unexpiring-discharge-macaroon]

1670. By Tom Wardill on 2018-11-29

Merge trunk

1671. By Tom Wardill on 2018-11-29

Add tests for having an optional expiry caveat

1672. By Tom Wardill on 2018-11-29

Remove old comment

1673. By Tom Wardill on 2019-01-14

Merge with latest trunk

=== modified file 'django_project/settings_base.py'
--- django_project/settings_base.py	2018-12-20 20:25:26 +0000
+++ django_project/settings_base.py	2019-01-14 10:41:34 +0000
@@ -372,7 +372,6 @@
 LP_API_CONSUMER_KEY = ''
 LP_API_TOKEN = ''
 LP_API_TOKEN_SECRET = ''
-MACAROON_TTL = 365 * 24 * 3600  # seconds
 MACAROON_SERVICE_LOCATION = HOSTNAME
 MANAGERS = []
 MAX_EMAILS_PER_ACCOUNT = 30

=== modified file 'src/identityprovider/auth.py'
--- src/identityprovider/auth.py	2018-02-21 13:46:00 +0000
+++ src/identityprovider/auth.py	2019-01-14 10:41:34 +0000
@@ -577,13 +577,11 @@
     now_string = now().strftime(MACAROON_TIMESTAMP_FORMAT)
     if last_auth is None:
         last_auth = now_string
-    if expires is None:
-        expires_ts = now() + datetime.timedelta(seconds=settings.MACAROON_TTL)
-        expires = expires_ts.strftime(MACAROON_TIMESTAMP_FORMAT)
 
     d.add_first_party_caveat(service_location + '|valid_since|' + now_string)
     d.add_first_party_caveat(service_location + '|last_auth|' + last_auth)
-    d.add_first_party_caveat(service_location + '|expires|' + expires)
+    if expires is not None:
+        d.add_first_party_caveat(service_location + '|expires|' + expires)
     return d
 
 
@@ -685,7 +683,8 @@
     # the original discharge macaroon
     discharge = _build_discharge(
         caveat_info['3rdparty'], discharge_macaroon.identifier, account,
-        last_auth=macaroon_info['last_auth'], expires=macaroon_info['expires'])
+        last_auth=macaroon_info['last_auth'],
+        expires=macaroon_info.get('expires'))
 
     # return the unbound discharge macaroon
     return discharge

=== modified file 'src/identityprovider/tests/test_auth.py'
--- src/identityprovider/tests/test_auth.py	2018-02-21 14:12:14 +0000
+++ src/identityprovider/tests/test_auth.py	2019-01-14 10:41:34 +0000
@@ -26,9 +26,11 @@
 from pymacaroons import Verifier
 
 from identityprovider.auth import (
+    MACAROON_TIMESTAMP_FORMAT,
     LaunchpadBackend,
     SSOOAuthAuthentication,
     SSORequestValidator,
+    _build_discharge,
     _decrypt_caveat,
     basic_authenticate,
     build_discharge_macaroon,
@@ -1041,6 +1043,10 @@
                 self.assertEqual(expected, actual)
                 return True
 
+            # Expires caveat no longer exists by default
+            if key == 'expires':
+                return False
+
             if key == 'expires':
                 expires = datetime.strptime(value, '%Y-%m-%dT%H:%M:%S.%f')
                 before_plus_ttl = before + timedelta(
@@ -1206,9 +1212,49 @@
 
         # get a discharge for the test macaroon
         self.account = self.factory.make_account()
+        self.caveat = caveat
         self.discharge_macaroon = build_discharge_macaroon(
             self.account, caveat.caveat_id)
 
+    def checker(self, caveat):
+        """Assure all caveats inside the discharged macaroon are ok."""
+        source, key, value = caveat.split("|", 2)
+
+        if key == 'valid_since':
+            valid_since = datetime.strptime(value, '%Y-%m-%dT%H:%M:%S.%f')
+            self.assertGreater(valid_since, self.before)
+            self.assertGreater(self.after, valid_since)
+            return True
+
+        if key == 'last_auth':
+            self.assertEqual(value, self.old_last_auth)
+            return True
+
+        if key == 'account':
+            expected = {
+                'openid': self.account.openid_identifier,
+                'email': self.mail.email,
+                'displayname': "New test display name",
+                'is_verified': self.account.is_verified,
+                'username': self.account.person_name
+            }
+            actual = json.loads(base64.b64decode(value).decode("utf8"))
+            self.assertEqual(expected, actual)
+            return True
+
+        if key == 'expires':
+            self.assertEqual(value, self.old_expires)
+            return True
+
+        # we're not validating an SSO from the discharged macaroon, fail!
+        return False
+
+    def get_value(self, discharge, service_location, search_key):
+        for caveat in discharge.first_party_caveats():
+            source, key, value = caveat.caveat_id.split("|", 2)
+            if source == service_location and key == search_key:
+                return value
+
     def test_discharge_macaroon_corrupt(self):
         self.assertRaises(
             ValidationError, refresh_discharge, "Seriously corrupted macaroon")
@@ -1239,61 +1285,57 @@
         old_discharge = self.discharge_macaroon  # just rename for readability
         service_location = settings.MACAROON_SERVICE_LOCATION
 
-        def get_value(search_key):
-            for caveat in old_discharge.first_party_caveats():
-                source, key, value = caveat.caveat_id.split("|", 2)
-                if source == service_location and key == search_key:
-                    return value
-
         # get old values from the macaroon and also change the account to see
         # that reflected
-        old_last_auth = get_value('last_auth')
-        old_expires = get_value('expires')
-        new_mail = self.factory.make_email_for_account(
+        self.old_last_auth = self.get_value(
+            old_discharge, service_location, 'last_auth')
+        # expires isn't set by default, but we can test that it has no value
+        self.old_expires = 'NOTSET'
+        self.mail = self.factory.make_email_for_account(
             self.account, status=EmailStatus.PREFERRED)
         self.account.displayname = "New test display name"
         self.account.save()
 
         # call!
-        before = now()
+        self.before = now()
         new_discharge = refresh_discharge(old_discharge.serialize())
-        after = now()
-
-        # test
-        def checker(caveat):
-            """Assure all caveats inside the discharged macaroon are ok."""
-            source, key, value = caveat.split("|", 2)
-
-            if key == 'valid_since':
-                valid_since = datetime.strptime(value, '%Y-%m-%dT%H:%M:%S.%f')
-                self.assertGreater(valid_since, before)
-                self.assertGreater(after, valid_since)
-                return True
-
-            if key == 'last_auth':
-                self.assertEqual(value, old_last_auth)
-                return True
-
-            if key == 'account':
-                expected = {
-                    'openid': self.account.openid_identifier,
-                    'email': new_mail.email,
-                    'displayname': "New test display name",
-                    'is_verified': self.account.is_verified,
-                    'username': self.account.person_name
-                }
-                actual = json.loads(base64.b64decode(value).decode("utf8"))
-                self.assertEqual(expected, actual)
-                return True
-
-            if key == 'expires':
-                self.assertEqual(value, old_expires)
-                return True
-
-            # we're not validating an SSO from the discharged macaroon, fail!
-            return False
-
-        # verify using the NEW discharge macaroon
-        v = Verifier()
-        v.satisfy_general(checker)
+        self.after = now()
+
+        # verify using the NEW discharge macaroon
+        v = Verifier()
+        v.satisfy_general(self.checker)
+        v.verify(new_discharge, self.random_key, [])
+
+    def test_refreshing_with_expiry(self):
+        expires = datetime.now() + timedelta(seconds=1000)
+        caveat_info = _decrypt_caveat(self.caveat.caveat_id)
+        # use _build_discharge as it can take an expiry
+        self.discharge_macaroon = _build_discharge(
+            caveat_info['3rdparty'],
+            self.caveat.caveat_id,
+            self.account,
+            expires=expires.strftime(MACAROON_TIMESTAMP_FORMAT))
+
+        service_location = settings.MACAROON_SERVICE_LOCATION
+
+        # get old values from the macaroon and also change the account to see
+        # that reflected
+        self.old_last_auth = self.get_value(
+            self.discharge_macaroon, service_location, 'last_auth')
+        # grab our old expiry value
+        self.old_expires = self.get_value(
+            self.discharge_macaroon, service_location, 'expires')
+        self.mail = self.factory.make_email_for_account(
+            self.account, status=EmailStatus.PREFERRED)
+        self.account.displayname = "New test display name"
+        self.account.save()
+
+        # call!
+        self.before = now()
+        new_discharge = refresh_discharge(self.discharge_macaroon.serialize())
+        self.after = now()
+
+        # verify using the NEW discharge macaroon
+        v = Verifier()
+        v.satisfy_general(self.checker)
         v.verify(new_discharge, self.random_key, [])