launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #23491
[Merge] lp:~cjwatson/launchpad/refactor-macaroon-testing into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/refactor-macaroon-testing into lp:launchpad with lp:~cjwatson/launchpad/authserver-issue-macaroon as a prerequisite.
Commit message:
Add logging to macaroon verification, and refactor tests to check it.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/refactor-macaroon-testing/+merge/365859
It was all too easy for macaroon verification to fail for some reason other than the one that a test expected, and tests had no way to tell; indeed, I ran into and fixed exactly such a case while working on this branch. We now log details of verification failures, and I added a couple of test helpers to make it easy for tests to check these consistently.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/refactor-macaroon-testing into lp:launchpad.
=== modified file 'lib/lp/code/model/codeimportjob.py'
--- lib/lp/code/model/codeimportjob.py 2019-04-11 13:31:23 +0000
+++ lib/lp/code/model/codeimportjob.py 2019-04-11 13:31:24 +0000
@@ -430,9 +430,10 @@
def checkVerificationContext(self, context):
"""See `MacaroonIssuerBase`."""
- if (not ICodeImportJob.providedBy(context) or
- context.state != CodeImportJobState.RUNNING):
- raise ValueError
+ if not ICodeImportJob.providedBy(context):
+ raise ValueError("Cannot handle context %r." % context)
+ if context.state != CodeImportJobState.RUNNING:
+ raise ValueError("%r is not in the RUNNING state." % context)
return context
def verifyPrimaryCaveat(self, caveat_value, context):
=== modified file 'lib/lp/code/model/tests/test_codeimportjob.py'
--- lib/lp/code/model/tests/test_codeimportjob.py 2019-04-11 13:31:23 +0000
+++ lib/lp/code/model/tests/test_codeimportjob.py 2019-04-11 13:31:24 +0000
@@ -62,6 +62,7 @@
from lp.services.librarian.interfaces import ILibraryFileAliasSet
from lp.services.librarian.interfaces.client import ILibrarianClient
from lp.services.macaroons.interfaces import IMacaroonIssuer
+from lp.services.macaroons.testing import MacaroonTestMixin
from lp.services.webapp import canonical_url
from lp.testing import (
ANONYMOUS,
@@ -1268,7 +1269,7 @@
get_feedback_messages(user_browser.contents))
-class TestCodeImportJobMacaroonIssuer(TestCaseWithFactory):
+class TestCodeImportJobMacaroonIssuer(MacaroonTestMixin, TestCaseWithFactory):
"""Test CodeImportJob macaroon issuing and verification."""
layer = DatabaseFunctionalLayer
@@ -1321,7 +1322,7 @@
issuer = getUtility(IMacaroonIssuer, "code-import-job")
getUtility(ICodeImportJobWorkflow).startJob(job, machine)
macaroon = removeSecurityProxy(issuer).issueMacaroon(job)
- self.assertTrue(issuer.verifyMacaroon(macaroon, job))
+ self.assertMacaroonVerifies(issuer, macaroon, job)
def test_verifyMacaroon_good_no_context(self):
machine = self.factory.makeCodeImportMachine(set_online=True)
@@ -1329,8 +1330,8 @@
issuer = getUtility(IMacaroonIssuer, "code-import-job")
getUtility(ICodeImportJobWorkflow).startJob(job, machine)
macaroon = removeSecurityProxy(issuer).issueMacaroon(job)
- self.assertTrue(
- issuer.verifyMacaroon(macaroon, None, require_context=False))
+ self.assertMacaroonVerifies(
+ issuer, macaroon, job, require_context=False)
def test_verifyMacaroon_no_context_but_require_context(self):
machine = self.factory.makeCodeImportMachine(set_online=True)
@@ -1338,7 +1339,9 @@
issuer = getUtility(IMacaroonIssuer, "code-import-job")
getUtility(ICodeImportJobWorkflow).startJob(job, machine)
macaroon = removeSecurityProxy(issuer).issueMacaroon(job)
- self.assertFalse(issuer.verifyMacaroon(macaroon, None))
+ self.assertMacaroonDoesNotVerify(
+ ["Expected macaroon verification context but got None."],
+ issuer, macaroon, None)
def test_verifyMacaroon_wrong_location(self):
machine = self.factory.makeCodeImportMachine(set_online=True)
@@ -1348,9 +1351,12 @@
macaroon = Macaroon(
location="another-location",
key=removeSecurityProxy(issuer)._root_secret)
- self.assertFalse(issuer.verifyMacaroon(macaroon, job))
- self.assertFalse(
- issuer.verifyMacaroon(macaroon, None, require_context=False))
+ self.assertMacaroonDoesNotVerify(
+ ["Macaroon has unknown location 'another-location'."],
+ issuer, macaroon, job)
+ self.assertMacaroonDoesNotVerify(
+ ["Macaroon has unknown location 'another-location'."],
+ issuer, macaroon, job, require_context=False)
def test_verifyMacaroon_wrong_key(self):
machine = self.factory.makeCodeImportMachine(set_online=True)
@@ -1359,19 +1365,28 @@
getUtility(ICodeImportJobWorkflow).startJob(job, machine)
macaroon = Macaroon(
location=config.vhost.mainsite.hostname, key="another-secret")
- self.assertFalse(issuer.verifyMacaroon(macaroon, job))
- self.assertFalse(
- issuer.verifyMacaroon(macaroon, None, require_context=False))
+ self.assertMacaroonDoesNotVerify(
+ ["Signatures do not match."], issuer, macaroon, job)
+ self.assertMacaroonDoesNotVerify(
+ ["Signatures do not match."],
+ issuer, macaroon, job, require_context=False)
def test_verifyMacaroon_not_running(self):
job = self.makeJob()
issuer = getUtility(IMacaroonIssuer, "code-import-job")
macaroon = removeSecurityProxy(issuer).issueMacaroon(job)
- self.assertFalse(issuer.verifyMacaroon(macaroon, job))
+ self.assertMacaroonDoesNotVerify(
+ ["%r is not in the RUNNING state." % job],
+ issuer, macaroon, job)
def test_verifyMacaroon_wrong_job(self):
+ machine = self.factory.makeCodeImportMachine(set_online=True)
job = self.makeJob()
- other_job = self.factory.makeCodeImportJob(code_import=job.code_import)
+ other_job = self.makeJob()
issuer = getUtility(IMacaroonIssuer, "code-import-job")
+ getUtility(ICodeImportJobWorkflow).startJob(job, machine)
macaroon = removeSecurityProxy(issuer).issueMacaroon(other_job)
- self.assertFalse(issuer.verifyMacaroon(macaroon, job))
+ self.assertMacaroonDoesNotVerify(
+ ["Caveat check for 'lp.code-import-job %s' failed." %
+ other_job.id],
+ issuer, macaroon, job)
=== modified file 'lib/lp/services/macaroons/model.py'
--- lib/lp/services/macaroons/model.py 2019-04-11 13:31:23 +0000
+++ lib/lp/services/macaroons/model.py 2019-04-11 13:31:24 +0000
@@ -14,8 +14,10 @@
Macaroon,
Verifier,
)
+from pymacaroons.exceptions import MacaroonVerificationFailedException
from lp.services.config import config
+from lp.services.scripts import log
class MacaroonIssuerBase:
@@ -109,32 +111,54 @@
def verifyMacaroon(self, macaroon, context, require_context=True):
"""See `IMacaroonIssuer`."""
if macaroon.location != config.vhost.mainsite.hostname:
+ log.info("Macaroon has unknown location '%s'." % macaroon.location)
return False
if require_context and context is None:
+ log.info("Expected macaroon verification context but got None.")
return False
if context is not None:
try:
context = self.checkVerificationContext(context)
- except ValueError:
+ except ValueError as e:
+ log.info(str(e))
return False
+ # XXX cjwatson 2019-04-11: Once we're on Python 3, we should use
+ # "nonlocal" instead of this hack.
+ class VerificationState:
+ logged_caveat_error = False
+
+ state = VerificationState()
+
def verify(caveat):
try:
caveat_name, caveat_value = caveat.split(" ", 1)
except ValueError:
+ log.info("Cannot parse caveat '%s'." % caveat)
+ state.logged_caveat_error = True
return False
if caveat_name == self.primary_caveat_name:
checker = self.verifyPrimaryCaveat
else:
- # XXX cjwatson 2019-04-09: For now we just fail closed if
- # there are any other caveats, which is good enough for
- # internal use.
+ checker = self.checkers.get(caveat_name)
+ if checker is None:
+ log.info("Unhandled caveat name '%s'." % caveat_name)
+ state.logged_caveat_error = True
+ return False
+ if not checker(caveat_value, context):
+ log.info("Caveat check for '%s' failed." % caveat)
+ state.logged_caveat_error = True
return False
- return checker(caveat_value, context)
+ return True
try:
verifier = Verifier()
verifier.satisfy_general(verify)
return verifier.verify(macaroon, self._root_secret)
+ except MacaroonVerificationFailedException as e:
+ if not state.logged_caveat_error:
+ log.info(str(e))
+ return False
except Exception:
+ log.exception("Unhandled exception while verifying macaroon.")
return False
=== added file 'lib/lp/services/macaroons/testing.py'
--- lib/lp/services/macaroons/testing.py 1970-01-01 00:00:00 +0000
+++ lib/lp/services/macaroons/testing.py 2019-04-11 13:31:24 +0000
@@ -0,0 +1,40 @@
+# Copyright 2019 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Macaroon testing helpers."""
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+__all__ = [
+ 'find_caveats_by_name',
+ 'MacaroonTestMixin',
+ ]
+
+from fixtures import FakeLogger
+from testtools.content import text_content
+
+
+def find_caveats_by_name(macaroon, caveat_name):
+ return [
+ caveat for caveat in macaroon.caveats
+ if caveat.caveat_id.startswith(caveat_name + " ")]
+
+
+class MacaroonTestMixin:
+
+ def assertMacaroonVerifies(self, issuer, macaroon, context, **kwargs):
+ with FakeLogger() as logger:
+ try:
+ self.assertTrue(issuer.verifyMacaroon(
+ macaroon, context, **kwargs))
+ except Exception:
+ self.addDetail("log", text_content(logger.output))
+ raise
+
+ def assertMacaroonDoesNotVerify(self, expected_log_lines, issuer, macaroon,
+ context, **kwargs):
+ with FakeLogger() as logger:
+ self.assertFalse(issuer.verifyMacaroon(
+ macaroon, context, **kwargs))
+ self.assertEqual(expected_log_lines, logger.output.splitlines())
=== modified file 'lib/lp/snappy/tests/test_snapbuild.py'
--- lib/lp/snappy/tests/test_snapbuild.py 2019-04-11 13:31:23 +0000
+++ lib/lp/snappy/tests/test_snapbuild.py 2019-04-11 13:31:24 +0000
@@ -43,6 +43,7 @@
from lp.services.job.interfaces.job import JobStatus
from lp.services.librarian.browser import ProxiedLibraryFileAlias
from lp.services.macaroons.interfaces import IMacaroonIssuer
+from lp.services.macaroons.testing import MacaroonTestMixin
from lp.services.propertycache import clear_property_cache
from lp.services.webapp.interfaces import OAuthPermission
from lp.services.webapp.publisher import canonical_url
@@ -782,7 +783,7 @@
self.assertCanOpenRedirectedUrl(browser, file_url)
-class TestSnapBuildMacaroonIssuer(TestCaseWithFactory):
+class TestSnapBuildMacaroonIssuer(MacaroonTestMixin, TestCaseWithFactory):
"""Test SnapBuild macaroon issuing and verification."""
layer = LaunchpadZopelessLayer
@@ -836,7 +837,7 @@
issuer = removeSecurityProxy(
getUtility(IMacaroonIssuer, "snap-build"))
macaroon = issuer.issueMacaroon(build)
- self.assertTrue(issuer.verifyMacaroon(macaroon, ref.repository))
+ self.assertMacaroonVerifies(issuer, macaroon, ref.repository)
def test_verifyMacaroon_wrong_location(self):
[ref] = self.factory.makeGitRefs(
@@ -848,7 +849,9 @@
getUtility(IMacaroonIssuer, "snap-build"))
macaroon = Macaroon(
location="another-location", key=issuer._root_secret)
- self.assertFalse(issuer.verifyMacaroon(macaroon, ref.repository))
+ self.assertMacaroonDoesNotVerify(
+ ["Macaroon has unknown location 'another-location'."],
+ issuer, macaroon, ref.repository)
def test_verifyMacaroon_wrong_key(self):
[ref] = self.factory.makeGitRefs(
@@ -860,7 +863,8 @@
getUtility(IMacaroonIssuer, "snap-build"))
macaroon = Macaroon(
location=config.vhost.mainsite.hostname, key="another-secret")
- self.assertFalse(issuer.verifyMacaroon(macaroon, ref.repository))
+ self.assertMacaroonDoesNotVerify(
+ ["Signatures do not match."], issuer, macaroon, ref.repository)
def test_verifyMacaroon_refuses_branch(self):
branch = self.factory.makeAnyBranch(
@@ -871,7 +875,8 @@
issuer = removeSecurityProxy(
getUtility(IMacaroonIssuer, "snap-build"))
macaroon = issuer.issueMacaroon(build)
- self.assertFalse(issuer.verifyMacaroon(macaroon, branch))
+ self.assertMacaroonDoesNotVerify(
+ ["Cannot handle context %r." % branch], issuer, macaroon, branch)
def test_verifyMacaroon_not_building(self):
[ref] = self.factory.makeGitRefs(
@@ -881,7 +886,9 @@
issuer = removeSecurityProxy(
getUtility(IMacaroonIssuer, "snap-build"))
macaroon = issuer.issueMacaroon(build)
- self.assertFalse(issuer.verifyMacaroon(macaroon, ref.repository))
+ self.assertMacaroonDoesNotVerify(
+ ["Caveat check for 'lp.snap-build %s' failed." % build.id],
+ issuer, macaroon, ref.repository)
def test_verifyMacaroon_wrong_build(self):
[ref] = self.factory.makeGitRefs(
@@ -895,7 +902,9 @@
issuer = removeSecurityProxy(
getUtility(IMacaroonIssuer, "snap-build"))
macaroon = issuer.issueMacaroon(other_build)
- self.assertFalse(issuer.verifyMacaroon(macaroon, ref.repository))
+ self.assertMacaroonDoesNotVerify(
+ ["Caveat check for 'lp.snap-build %s' failed." % other_build.id],
+ issuer, macaroon, ref.repository)
def test_verifyMacaroon_wrong_repository(self):
[ref] = self.factory.makeGitRefs(
@@ -907,4 +916,6 @@
issuer = removeSecurityProxy(
getUtility(IMacaroonIssuer, "snap-build"))
macaroon = issuer.issueMacaroon(build)
- self.assertFalse(issuer.verifyMacaroon(macaroon, other_repository))
+ self.assertMacaroonDoesNotVerify(
+ ["Caveat check for 'lp.snap-build %s' failed." % build.id],
+ issuer, macaroon, other_repository)
=== modified file 'lib/lp/soyuz/tests/test_binarypackagebuild.py'
--- lib/lp/soyuz/tests/test_binarypackagebuild.py 2019-04-11 13:31:23 +0000
+++ lib/lp/soyuz/tests/test_binarypackagebuild.py 2019-04-11 13:31:24 +0000
@@ -32,6 +32,7 @@
from lp.services.config import config
from lp.services.log.logger import DevNullLogger
from lp.services.macaroons.interfaces import IMacaroonIssuer
+from lp.services.macaroons.testing import MacaroonTestMixin
from lp.services.webapp.interaction import ANONYMOUS
from lp.services.webapp.interfaces import OAuthPermission
from lp.soyuz.enums import (
@@ -910,7 +911,8 @@
archive, getUtility(ILaunchpadCelebrities).ppa_admin)
-class TestBinaryPackageBuildMacaroonIssuer(TestCaseWithFactory):
+class TestBinaryPackageBuildMacaroonIssuer(
+ MacaroonTestMixin, TestCaseWithFactory):
"""Test BinaryPackageBuild macaroon issuing and verification."""
layer = LaunchpadZopelessLayer
@@ -957,7 +959,7 @@
issuer = removeSecurityProxy(
getUtility(IMacaroonIssuer, "binary-package-build"))
macaroon = issuer.issueMacaroon(build)
- self.assertTrue(issuer.verifyMacaroon(macaroon, lfa_id))
+ self.assertMacaroonVerifies(issuer, macaroon, lfa_id)
def test_verifyMacaroon_wrong_location(self):
build = self.factory.makeBinaryPackageBuild(
@@ -970,7 +972,9 @@
getUtility(IMacaroonIssuer, "binary-package-build"))
macaroon = Macaroon(
location="another-location", key=issuer._root_secret)
- self.assertFalse(issuer.verifyMacaroon(macaroon, lfa_id))
+ self.assertMacaroonDoesNotVerify(
+ ["Macaroon has unknown location 'another-location'."],
+ issuer, macaroon, lfa_id)
def test_verifyMacaroon_wrong_key(self):
build = self.factory.makeBinaryPackageBuild(
@@ -983,7 +987,8 @@
getUtility(IMacaroonIssuer, "binary-package-build"))
macaroon = Macaroon(
location=config.vhost.mainsite.hostname, key="another-secret")
- self.assertFalse(issuer.verifyMacaroon(macaroon, lfa_id))
+ self.assertMacaroonDoesNotVerify(
+ ["Signatures do not match."], issuer, macaroon, lfa_id)
def test_verifyMacaroon_not_building(self):
build = self.factory.makeBinaryPackageBuild(
@@ -994,7 +999,10 @@
issuer = removeSecurityProxy(
getUtility(IMacaroonIssuer, "binary-package-build"))
macaroon = issuer.issueMacaroon(build)
- self.assertFalse(issuer.verifyMacaroon(macaroon, lfa_id))
+ self.assertMacaroonDoesNotVerify(
+ ["Caveat check for 'lp.binary-package-build %s' failed." %
+ build.id],
+ issuer, macaroon, lfa_id)
def test_verifyMacaroon_wrong_build(self):
build = self.factory.makeBinaryPackageBuild(
@@ -1009,7 +1017,10 @@
issuer = removeSecurityProxy(
getUtility(IMacaroonIssuer, "binary-package-build"))
macaroon = issuer.issueMacaroon(other_build)
- self.assertFalse(issuer.verifyMacaroon(macaroon, lfa_id))
+ self.assertMacaroonDoesNotVerify(
+ ["Caveat check for 'lp.binary-package-build %s' failed." %
+ other_build.id],
+ issuer, macaroon, lfa_id)
def test_verifyMacaroon_wrong_file(self):
build = self.factory.makeBinaryPackageBuild(
@@ -1022,4 +1033,7 @@
issuer = removeSecurityProxy(
getUtility(IMacaroonIssuer, "binary-package-build"))
macaroon = issuer.issueMacaroon(build)
- self.assertFalse(issuer.verifyMacaroon(macaroon, lfa_id))
+ self.assertMacaroonDoesNotVerify(
+ ["Caveat check for 'lp.binary-package-build %s' failed." %
+ build.id],
+ issuer, macaroon, lfa_id)
Follow ups
