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

Proposed by Tom Wardill on 2018-11-23
Status: Merged
Approved by: Tom Wardill on 2019-01-14
Approved revision: 1673
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: lp:~twom/canonical-identity-provider/unexpiring-discharge-macaroon
Merge into: lp:canonical-identity-provider
Diff against target: 225 lines (+95/-55)
3 files modified
django_project/settings_base.py (+0/-1)
src/identityprovider/auth.py (+4/-5)
src/identityprovider/tests/test_auth.py (+91/-49)
To merge this branch: bzr merge lp:~twom/canonical-identity-provider/unexpiring-discharge-macaroon
Reviewer Review Type Date Requested Status
Colin Watson 2018-11-23 Approve on 2018-11-28
Review via email: mp+359240@code.launchpad.net

Commit message

Remove the expiry caveat from the discharge macaroon.

Description of the change

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.

To post a comment you must log in.
Colin Watson (cjwatson) wrote :

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) wrote :

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

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'django_project/settings_base.py'
2--- django_project/settings_base.py 2018-12-20 20:25:26 +0000
3+++ django_project/settings_base.py 2019-01-14 10:41:34 +0000
4@@ -372,7 +372,6 @@
5 LP_API_CONSUMER_KEY = ''
6 LP_API_TOKEN = ''
7 LP_API_TOKEN_SECRET = ''
8-MACAROON_TTL = 365 * 24 * 3600 # seconds
9 MACAROON_SERVICE_LOCATION = HOSTNAME
10 MANAGERS = []
11 MAX_EMAILS_PER_ACCOUNT = 30
12
13=== modified file 'src/identityprovider/auth.py'
14--- src/identityprovider/auth.py 2018-02-21 13:46:00 +0000
15+++ src/identityprovider/auth.py 2019-01-14 10:41:34 +0000
16@@ -577,13 +577,11 @@
17 now_string = now().strftime(MACAROON_TIMESTAMP_FORMAT)
18 if last_auth is None:
19 last_auth = now_string
20- if expires is None:
21- expires_ts = now() + datetime.timedelta(seconds=settings.MACAROON_TTL)
22- expires = expires_ts.strftime(MACAROON_TIMESTAMP_FORMAT)
23
24 d.add_first_party_caveat(service_location + '|valid_since|' + now_string)
25 d.add_first_party_caveat(service_location + '|last_auth|' + last_auth)
26- d.add_first_party_caveat(service_location + '|expires|' + expires)
27+ if expires is not None:
28+ d.add_first_party_caveat(service_location + '|expires|' + expires)
29 return d
30
31
32@@ -685,7 +683,8 @@
33 # the original discharge macaroon
34 discharge = _build_discharge(
35 caveat_info['3rdparty'], discharge_macaroon.identifier, account,
36- last_auth=macaroon_info['last_auth'], expires=macaroon_info['expires'])
37+ last_auth=macaroon_info['last_auth'],
38+ expires=macaroon_info.get('expires'))
39
40 # return the unbound discharge macaroon
41 return discharge
42
43=== modified file 'src/identityprovider/tests/test_auth.py'
44--- src/identityprovider/tests/test_auth.py 2018-02-21 14:12:14 +0000
45+++ src/identityprovider/tests/test_auth.py 2019-01-14 10:41:34 +0000
46@@ -26,9 +26,11 @@
47 from pymacaroons import Verifier
48
49 from identityprovider.auth import (
50+ MACAROON_TIMESTAMP_FORMAT,
51 LaunchpadBackend,
52 SSOOAuthAuthentication,
53 SSORequestValidator,
54+ _build_discharge,
55 _decrypt_caveat,
56 basic_authenticate,
57 build_discharge_macaroon,
58@@ -1041,6 +1043,10 @@
59 self.assertEqual(expected, actual)
60 return True
61
62+ # Expires caveat no longer exists by default
63+ if key == 'expires':
64+ return False
65+
66 if key == 'expires':
67 expires = datetime.strptime(value, '%Y-%m-%dT%H:%M:%S.%f')
68 before_plus_ttl = before + timedelta(
69@@ -1206,9 +1212,49 @@
70
71 # get a discharge for the test macaroon
72 self.account = self.factory.make_account()
73+ self.caveat = caveat
74 self.discharge_macaroon = build_discharge_macaroon(
75 self.account, caveat.caveat_id)
76
77+ def checker(self, caveat):
78+ """Assure all caveats inside the discharged macaroon are ok."""
79+ source, key, value = caveat.split("|", 2)
80+
81+ if key == 'valid_since':
82+ valid_since = datetime.strptime(value, '%Y-%m-%dT%H:%M:%S.%f')
83+ self.assertGreater(valid_since, self.before)
84+ self.assertGreater(self.after, valid_since)
85+ return True
86+
87+ if key == 'last_auth':
88+ self.assertEqual(value, self.old_last_auth)
89+ return True
90+
91+ if key == 'account':
92+ expected = {
93+ 'openid': self.account.openid_identifier,
94+ 'email': self.mail.email,
95+ 'displayname': "New test display name",
96+ 'is_verified': self.account.is_verified,
97+ 'username': self.account.person_name
98+ }
99+ actual = json.loads(base64.b64decode(value).decode("utf8"))
100+ self.assertEqual(expected, actual)
101+ return True
102+
103+ if key == 'expires':
104+ self.assertEqual(value, self.old_expires)
105+ return True
106+
107+ # we're not validating an SSO from the discharged macaroon, fail!
108+ return False
109+
110+ def get_value(self, discharge, service_location, search_key):
111+ for caveat in discharge.first_party_caveats():
112+ source, key, value = caveat.caveat_id.split("|", 2)
113+ if source == service_location and key == search_key:
114+ return value
115+
116 def test_discharge_macaroon_corrupt(self):
117 self.assertRaises(
118 ValidationError, refresh_discharge, "Seriously corrupted macaroon")
119@@ -1239,61 +1285,57 @@
120 old_discharge = self.discharge_macaroon # just rename for readability
121 service_location = settings.MACAROON_SERVICE_LOCATION
122
123- def get_value(search_key):
124- for caveat in old_discharge.first_party_caveats():
125- source, key, value = caveat.caveat_id.split("|", 2)
126- if source == service_location and key == search_key:
127- return value
128-
129 # get old values from the macaroon and also change the account to see
130 # that reflected
131- old_last_auth = get_value('last_auth')
132- old_expires = get_value('expires')
133- new_mail = self.factory.make_email_for_account(
134+ self.old_last_auth = self.get_value(
135+ old_discharge, service_location, 'last_auth')
136+ # expires isn't set by default, but we can test that it has no value
137+ self.old_expires = 'NOTSET'
138+ self.mail = self.factory.make_email_for_account(
139 self.account, status=EmailStatus.PREFERRED)
140 self.account.displayname = "New test display name"
141 self.account.save()
142
143 # call!
144- before = now()
145+ self.before = now()
146 new_discharge = refresh_discharge(old_discharge.serialize())
147- after = now()
148-
149- # test
150- def checker(caveat):
151- """Assure all caveats inside the discharged macaroon are ok."""
152- source, key, value = caveat.split("|", 2)
153-
154- if key == 'valid_since':
155- valid_since = datetime.strptime(value, '%Y-%m-%dT%H:%M:%S.%f')
156- self.assertGreater(valid_since, before)
157- self.assertGreater(after, valid_since)
158- return True
159-
160- if key == 'last_auth':
161- self.assertEqual(value, old_last_auth)
162- return True
163-
164- if key == 'account':
165- expected = {
166- 'openid': self.account.openid_identifier,
167- 'email': new_mail.email,
168- 'displayname': "New test display name",
169- 'is_verified': self.account.is_verified,
170- 'username': self.account.person_name
171- }
172- actual = json.loads(base64.b64decode(value).decode("utf8"))
173- self.assertEqual(expected, actual)
174- return True
175-
176- if key == 'expires':
177- self.assertEqual(value, old_expires)
178- return True
179-
180- # we're not validating an SSO from the discharged macaroon, fail!
181- return False
182-
183- # verify using the NEW discharge macaroon
184- v = Verifier()
185- v.satisfy_general(checker)
186+ self.after = now()
187+
188+ # verify using the NEW discharge macaroon
189+ v = Verifier()
190+ v.satisfy_general(self.checker)
191+ v.verify(new_discharge, self.random_key, [])
192+
193+ def test_refreshing_with_expiry(self):
194+ expires = datetime.now() + timedelta(seconds=1000)
195+ caveat_info = _decrypt_caveat(self.caveat.caveat_id)
196+ # use _build_discharge as it can take an expiry
197+ self.discharge_macaroon = _build_discharge(
198+ caveat_info['3rdparty'],
199+ self.caveat.caveat_id,
200+ self.account,
201+ expires=expires.strftime(MACAROON_TIMESTAMP_FORMAT))
202+
203+ service_location = settings.MACAROON_SERVICE_LOCATION
204+
205+ # get old values from the macaroon and also change the account to see
206+ # that reflected
207+ self.old_last_auth = self.get_value(
208+ self.discharge_macaroon, service_location, 'last_auth')
209+ # grab our old expiry value
210+ self.old_expires = self.get_value(
211+ self.discharge_macaroon, service_location, 'expires')
212+ self.mail = self.factory.make_email_for_account(
213+ self.account, status=EmailStatus.PREFERRED)
214+ self.account.displayname = "New test display name"
215+ self.account.save()
216+
217+ # call!
218+ self.before = now()
219+ new_discharge = refresh_discharge(self.discharge_macaroon.serialize())
220+ self.after = now()
221+
222+ # verify using the NEW discharge macaroon
223+ v = Verifier()
224+ v.satisfy_general(self.checker)
225 v.verify(new_discharge, self.random_key, [])