launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #21076
[Merge] lp:~cjwatson/launchpad/codeimport-git-auth into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/codeimport-git-auth into lp:launchpad.
Commit message:
Allow pushing to Git repositories associated with running code import jobs using macaroon credentials.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1469459 in Launchpad itself: "import external code into a LP git repo (natively)"
https://bugs.launchpad.net/launchpad/+bug/1469459
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/codeimport-git-auth/+merge/307968
Allow pushing to Git repositories associated with running code import jobs using macaroon credentials.
We don't actually issue suitable macaroons as yet outside the test suite; that will come in a later branch.
I've tried to design the IMacaroonIssuer interface so that it can be useful for some other similar applications (e.g. granting access tokens to builders), but it's provisional and I entirely expect it to need to change when it's actually battle-tested for that kind of thing.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/codeimport-git-auth into lp:launchpad.
=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml 2016-10-03 17:00:56 +0000
+++ lib/lp/code/configure.zcml 2016-10-07 15:56:06 +0000
@@ -729,6 +729,15 @@
reclaimJob"/>
</securedutility>
+ <!-- CodeImportJobMacaroonIssuer -->
+
+ <securedutility
+ class="lp.code.model.codeimportjob.CodeImportJobMacaroonIssuer"
+ provides="lp.services.macaroons.interfaces.IMacaroonIssuer"
+ name="code-import-job">
+ <allow interface="lp.services.macaroons.interfaces.IMacaroonIssuerPublic"/>
+ </securedutility>
+
<!-- CodeImportEvent -->
<class class="lp.code.model.codeimportevent.CodeImportEvent">
=== modified file 'lib/lp/code/model/codeimportjob.py'
--- lib/lp/code/model/codeimportjob.py 2016-10-03 17:00:56 +0000
+++ lib/lp/code/model/codeimportjob.py 2016-10-07 15:56:06 +0000
@@ -12,6 +12,10 @@
import datetime
+from pymacaroons import (
+ Macaroon,
+ Verifier,
+ )
from sqlobject import (
ForeignKey,
IntCol,
@@ -27,7 +31,9 @@
CodeImportMachineState,
CodeImportResultStatus,
CodeImportReviewStatus,
+ GitRepositoryType,
)
+from lp.code.interfaces.codeimport import ICodeImportSet
from lp.code.interfaces.codeimportevent import ICodeImportEventSet
from lp.code.interfaces.codeimportjob import (
ICodeImportJob,
@@ -37,6 +43,7 @@
)
from lp.code.interfaces.codeimportmachine import ICodeImportMachineSet
from lp.code.interfaces.codeimportresult import ICodeImportResultSet
+from lp.code.interfaces.gitrepository import IGitRepository
from lp.code.model.codeimportresult import CodeImportResult
from lp.registry.interfaces.person import validate_public_person
from lp.services.config import config
@@ -48,6 +55,7 @@
SQLBase,
sqlvalues,
)
+from lp.services.macaroons.interfaces import IMacaroonIssuer
@implementer(ICodeImportJob)
@@ -340,3 +348,57 @@
# 4)
getUtility(ICodeImportEventSet).newReclaim(
code_import, machine, job_id)
+
+
+@implementer(IMacaroonIssuer)
+class CodeImportJobMacaroonIssuer:
+
+ @property
+ def _root_secret(self):
+ secret = config.codeimport.macaroon_secret_key
+ if not secret:
+ raise RuntimeError(
+ "codeimport.macaroon_secret_key not configured.")
+ return secret
+
+ def issueMacaroon(self, context):
+ """See `IMacaroonIssuer`."""
+ assert context.code_import.git_repository is not None
+ macaroon = Macaroon(
+ location=config.vhost.mainsite.hostname,
+ identifier="code-import-job", key=self._root_secret)
+ macaroon.add_first_party_caveat("code-import-job %s" % context.id)
+ return macaroon
+
+ def checkMacaroonIssuer(self, macaroon):
+ """See `IMacaroonIssuer`."""
+ try:
+ verifier = Verifier()
+ verifier.satisfy_general(
+ lambda caveat: caveat.startswith("code-import-job "))
+ return verifier.verify(macaroon, self._root_secret)
+ except Exception:
+ return False
+
+ def verifyMacaroon(self, macaroon, context):
+ """See `IMacaroonIssuer`."""
+ if IGitRepository.providedBy(context):
+ if context.repository_type != GitRepositoryType.IMPORTED:
+ return False
+ code_import = getUtility(ICodeImportSet).getByGitRepository(
+ context)
+ if code_import is None:
+ return False
+ job = code_import.import_job
+ if job is None:
+ return False
+ else:
+ job = context
+ try:
+ verifier = Verifier()
+ verifier.satisfy_exact("code-import-job %s" % job.id)
+ return (
+ verifier.verify(macaroon, self._root_secret) and
+ job.state == CodeImportJobState.RUNNING)
+ except Exception:
+ return False
=== modified file 'lib/lp/code/model/tests/test_codeimportjob.py'
--- lib/lp/code/model/tests/test_codeimportjob.py 2015-10-19 10:56:16 +0000
+++ lib/lp/code/model/tests/test_codeimportjob.py 2016-10-07 15:56:06 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Unit tests for CodeImportJob and CodeImportJobWorkflow."""
@@ -13,7 +13,12 @@
import StringIO
import unittest
+from pymacaroons import Macaroon
from pytz import UTC
+from testtools.matchers import (
+ MatchesListwise,
+ MatchesStructure,
+ )
import transaction
from zope.component import getUtility
from zope.security.proxy import removeSecurityProxy
@@ -23,6 +28,7 @@
CodeImportJobState,
CodeImportResultStatus,
CodeImportReviewStatus,
+ TargetRevisionControlSystems,
)
from lp.code.interfaces.codeimport import ICodeImportSet
from lp.code.interfaces.codeimportevent import ICodeImportEventSet
@@ -41,6 +47,7 @@
from lp.services.database.constants import UTC_NOW
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.webapp import canonical_url
from lp.testing import (
ANONYMOUS,
@@ -1144,5 +1151,70 @@
get_feedback_messages(user_browser.contents))
+class TestCodeImportJobMacaroonIssuer(TestCaseWithFactory):
+ """Test CodeImportJob macaroon issuing and verification."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestCodeImportJobMacaroonIssuer, self).setUp()
+ login_for_code_imports()
+ self.pushConfig("codeimport", macaroon_secret_key="some-secret")
+
+ def makeJob(self, target_rcs_type=TargetRevisionControlSystems.GIT):
+ code_import = self.factory.makeCodeImport(
+ target_rcs_type=target_rcs_type)
+ return self.factory.makeCodeImportJob(code_import=code_import)
+
+ def test_issueMacaroon_refuses_branch(self):
+ job = self.makeJob(target_rcs_type=TargetRevisionControlSystems.BZR)
+ issuer = getUtility(IMacaroonIssuer, "code-import-job")
+ self.assertRaises(
+ AssertionError, removeSecurityProxy(issuer).issueMacaroon, job)
+
+ def test_issueMacaroon_good(self):
+ job = self.makeJob()
+ issuer = getUtility(IMacaroonIssuer, "code-import-job")
+ macaroon = removeSecurityProxy(issuer).issueMacaroon(job)
+ self.assertEqual("launchpad.dev", macaroon.location)
+ self.assertEqual("code-import-job", macaroon.identifier)
+ self.assertThat(macaroon.caveats, MatchesListwise([
+ MatchesStructure.byEquality(
+ caveat_id="code-import-job %s" % job.id),
+ ]))
+
+ def test_checkMacaroonIssuer_good(self):
+ job = self.makeJob()
+ issuer = getUtility(IMacaroonIssuer, "code-import-job")
+ macaroon = removeSecurityProxy(issuer).issueMacaroon(job)
+ self.assertTrue(issuer.checkMacaroonIssuer(macaroon))
+
+ def test_checkMacaroonIssuer_wrong_key(self):
+ issuer = getUtility(IMacaroonIssuer, "code-import-job")
+ macaroon = Macaroon(key="another-secret")
+ self.assertFalse(issuer.checkMacaroonIssuer(macaroon))
+
+ def test_verifyMacaroon_good(self):
+ machine = self.factory.makeCodeImportMachine(set_online=True)
+ job = self.makeJob()
+ issuer = getUtility(IMacaroonIssuer, "code-import-job")
+ getUtility(ICodeImportJobWorkflow).startJob(job, machine)
+ macaroon = removeSecurityProxy(issuer).issueMacaroon(job)
+ self.assertTrue(issuer.verifyMacaroon(macaroon, job))
+
+ def test_not_running(self):
+ job = self.makeJob()
+ issuer = getUtility(IMacaroonIssuer, "code-import-job")
+ macaroon = removeSecurityProxy(issuer).issueMacaroon(job)
+ self.assertFalse(issuer.verifyMacaroon(macaroon, job))
+
+ def test_wrong_job(self):
+ job = self.makeJob()
+ other_job = self.factory.makeCodeImportJob(code_import=job.code_import)
+ issuer = getUtility(IMacaroonIssuer, "code-import-job")
+ macaroon = removeSecurityProxy(issuer).issueMacaroon(other_job)
+ self.assertFalse(issuer.verifyMacaroon(macaroon, job))
+
+
def test_suite():
return unittest.TestLoader().loadTestsFromName(__name__)
=== modified file 'lib/lp/code/xmlrpc/git.py'
--- lib/lp/code/xmlrpc/git.py 2016-10-06 20:37:39 +0000
+++ lib/lp/code/xmlrpc/git.py 2016-10-07 15:56:06 +0000
@@ -10,9 +10,13 @@
import sys
+from pymacaroons import Macaroon
from storm.store import Store
import transaction
-from zope.component import getUtility
+from zope.component import (
+ ComponentLookupError,
+ getUtility,
+ )
from zope.error.interfaces import IErrorReportingUtility
from zope.interface import implementer
from zope.security.interfaces import Unauthorized
@@ -53,6 +57,7 @@
NoSuchProduct,
)
from lp.registry.interfaces.sourcepackagename import ISourcePackageNameSet
+from lp.services.macaroons.interfaces import IMacaroonIssuer
from lp.services.webapp import LaunchpadXMLRPCView
from lp.services.webapp.authorization import check_permission
from lp.services.webapp.errorlog import ScriptRequest
@@ -68,22 +73,47 @@
super(GitAPI, self).__init__(*args, **kwargs)
self.repository_set = getUtility(IGitRepositorySet)
- def _performLookup(self, path):
+ def _verifyMacaroon(self, macaroon_raw, repository=None):
+ try:
+ macaroon = Macaroon.deserialize(macaroon_raw)
+ except Exception:
+ return False
+ try:
+ issuer = getUtility(IMacaroonIssuer, macaroon.identifier)
+ except ComponentLookupError:
+ return False
+ if repository is not None:
+ return issuer.verifyMacaroon(macaroon, repository)
+ else:
+ return issuer.checkMacaroonIssuer(macaroon)
+
+ def _performLookup(self, requester, path, auth_params):
repository, extra_path = getUtility(IGitLookup).getByPath(path)
if repository is None:
return None
- try:
- hosting_path = repository.getInternalPath()
- except Unauthorized:
- return None
- writable = (
- repository.repository_type == GitRepositoryType.HOSTED and
- check_permission("launchpad.Edit", repository))
+ macaroon_raw = auth_params.get("macaroon")
+ naked_repository = removeSecurityProxy(repository)
+ if (macaroon_raw is not None and
+ self._verifyMacaroon(macaroon_raw, naked_repository)):
+ # The authentication parameters specifically grant access to
+ # this repository, so we can bypass other checks.
+ hosting_path = naked_repository.getInternalPath()
+ writable = True
+ private = naked_repository.private
+ else:
+ try:
+ hosting_path = repository.getInternalPath()
+ except Unauthorized:
+ return None
+ writable = (
+ repository.repository_type == GitRepositoryType.HOSTED and
+ check_permission("launchpad.Edit", repository))
+ private = repository.private
return {
"path": hosting_path,
"writable": writable,
"trailing": extra_path,
- "private": repository.private,
+ "private": private,
}
def _getGitNamespaceExtras(self, path, requester):
@@ -233,11 +263,11 @@
if requester == LAUNCHPAD_ANONYMOUS:
requester = None
try:
- result = self._performLookup(path)
+ result = self._performLookup(requester, path, auth_params)
if (result is None and requester is not None and
permission == "write"):
self._createRepository(requester, path)
- result = self._performLookup(path)
+ result = self._performLookup(requester, path, auth_params)
if result is None:
raise faults.GitRepositoryNotFound(path)
if permission != "read" and not result["writable"]:
@@ -286,5 +316,10 @@
def authenticateWithPassword(self, username, password):
"""See `IGitAPI`."""
- # Password authentication isn't supported yet.
- return faults.Unauthorized()
+ # XXX cjwatson 2016-10-06: We only support free-floating macaroons
+ # at the moment, not ones bound to a user.
+ if not username and self._verifyMacaroon(password):
+ return {"macaroon": password}
+ else:
+ # Only macaroons are supported for password authentication.
+ return faults.Unauthorized()
=== modified file 'lib/lp/code/xmlrpc/tests/test_git.py'
--- lib/lp/code/xmlrpc/tests/test_git.py 2016-10-06 11:17:05 +0000
+++ lib/lp/code/xmlrpc/tests/test_git.py 2016-10-07 15:56:06 +0000
@@ -5,6 +5,7 @@
__metaclass__ = type
+from pymacaroons import Macaroon
from testscenarios import (
load_tests_apply_scenarios,
WithScenarios,
@@ -13,8 +14,12 @@
from zope.security.proxy import removeSecurityProxy
from lp.app.enums import InformationType
-from lp.code.enums import GitRepositoryType
+from lp.code.enums import (
+ GitRepositoryType,
+ TargetRevisionControlSystems,
+ )
from lp.code.errors import GitRepositoryCreationFault
+from lp.code.interfaces.codeimportjob import ICodeImportJobWorkflow
from lp.code.interfaces.gitcollection import IAllGitRepositories
from lp.code.interfaces.gitjob import IGitRefScanJobSource
from lp.code.interfaces.gitrepository import (
@@ -24,10 +29,12 @@
from lp.code.tests.helpers import GitHostingFixture
from lp.code.xmlrpc.git import GitAPI
from lp.registry.enums import TeamMembershipPolicy
+from lp.services.macaroons.interfaces import IMacaroonIssuer
from lp.services.webapp.escaping import html_escape
from lp.testing import (
admin_logged_in,
ANONYMOUS,
+ celebrity_logged_in,
login,
person_logged_in,
TestCaseWithFactory,
@@ -74,13 +81,15 @@
def assertPermissionDenied(self, requester, path,
message="Permission denied.",
- permission="read", can_authenticate=False):
+ permission="read", can_authenticate=False,
+ macaroon_raw=None):
"""Assert that looking at the given path returns PermissionDenied."""
if requester is not None:
requester = requester.id
- fault = self._translatePath(
- path, permission,
- {"uid": requester, "can-authenticate": can_authenticate})
+ auth_params = {"uid": requester, "can-authenticate": can_authenticate}
+ if macaroon_raw is not None:
+ auth_params["macaroon"] = macaroon_raw
+ fault = self._translatePath(path, permission, auth_params)
self.assertEqual(faults.PermissionDenied(message), fault)
def assertUnauthorized(self, requester, path,
@@ -143,12 +152,13 @@
def assertTranslates(self, requester, path, repository, writable,
permission="read", can_authenticate=False,
- trailing="", private=False):
+ macaroon_raw=None, trailing="", private=False):
if requester is not None:
requester = requester.id
- translation = self._translatePath(
- path, permission,
- {"uid": requester, "can-authenticate": can_authenticate})
+ auth_params = {"uid": requester, "can-authenticate": can_authenticate}
+ if macaroon_raw is not None:
+ auth_params["macaroon"] = macaroon_raw
+ translation = self._translatePath(path, permission, auth_params)
login(ANONYMOUS)
self.assertEqual(
{"path": repository.getInternalPath(), "writable": writable,
@@ -634,6 +644,46 @@
"GitRepositoryCreationFault: nothing here",
self.oopses[0]["tb_text"])
+ def test_translatePath_code_import(self):
+ # A code import worker with a suitable macaroon can write to a
+ # repository associated with a running code import job.
+ self.pushConfig("codeimport", macaroon_secret_key="some-secret")
+ machine = self.factory.makeCodeImportMachine(set_online=True)
+ code_imports = [
+ self.factory.makeCodeImport(
+ target_rcs_type=TargetRevisionControlSystems.GIT)
+ for _ in range(2)]
+ with celebrity_logged_in("vcs_imports"):
+ jobs = [
+ self.factory.makeCodeImportJob(code_import=code_import)
+ for code_import in code_imports]
+ issuer = getUtility(IMacaroonIssuer, "code-import-job")
+ macaroons = [
+ removeSecurityProxy(issuer).issueMacaroon(job) for job in jobs]
+ path = u"/%s" % code_imports[0].git_repository.unique_name
+ self.assertPermissionDenied(
+ None, path, permission="write", macaroon_raw=macaroons[0])
+ with celebrity_logged_in("vcs_imports"):
+ getUtility(ICodeImportJobWorkflow).startJob(jobs[0], machine)
+ # This only works with new-style passing of authentication parameters.
+ if self.auth_params_dict:
+ self.assertTranslates(
+ None, path, code_imports[0].git_repository, True,
+ permission="write", macaroon_raw=macaroons[0].serialize())
+ else:
+ self.assertPermissionDenied(
+ None, path, permission="write",
+ macaroon_raw=macaroons[0].serialize())
+ self.assertPermissionDenied(
+ None, path, permission="write",
+ macaroon_raw=macaroons[1].serialize())
+ self.assertPermissionDenied(
+ None, path, permission="write",
+ macaroon_raw=Macaroon(
+ identifier="another", key="another-secret").serialize())
+ self.assertPermissionDenied(
+ None, path, permission="write", macaroon_raw="nonsense")
+
def test_notify(self):
# The notify call creates a GitRefScanJob.
repository = self.factory.makeGitRepository()
@@ -666,6 +716,26 @@
self.git_api.authenticateWithPassword('foo', 'bar'),
faults.Unauthorized)
+ def test_authenticateWithPassword_code_import(self):
+ self.pushConfig("codeimport", macaroon_secret_key="some-secret")
+ code_import = self.factory.makeCodeImport(
+ target_rcs_type=TargetRevisionControlSystems.GIT)
+ with celebrity_logged_in("vcs_imports"):
+ job = self.factory.makeCodeImportJob(code_import=code_import)
+ issuer = getUtility(IMacaroonIssuer, "code-import-job")
+ macaroon = removeSecurityProxy(issuer).issueMacaroon(job)
+ self.assertEqual(
+ {"macaroon": macaroon.serialize()},
+ self.git_api.authenticateWithPassword("", macaroon.serialize()))
+ other_macaroon = Macaroon(identifier="another", key="another-secret")
+ self.assertIsInstance(
+ self.git_api.authenticateWithPassword(
+ "", other_macaroon.serialize()),
+ faults.Unauthorized)
+ self.assertIsInstance(
+ self.git_api.authenticateWithPassword("", "nonsense"),
+ faults.Unauthorized)
+
class TestGitAPISecurity(TestGitAPIMixin, TestCaseWithFactory):
"""Slow tests for `IGitAPI`.
=== modified file 'lib/lp/services/config/schema-lazr.conf'
--- lib/lp/services/config/schema-lazr.conf 2016-10-04 11:19:07 +0000
+++ lib/lp/services/config/schema-lazr.conf 2016-10-07 15:56:06 +0000
@@ -416,6 +416,9 @@
# Import only this many revisions from svn (via bzr-svn) at once.
svn_revisions_import_limit: 500
+# Secret key for macaroons used to grant git push permission to workers.
+macaroon_secret_key:
+
[codeimportdispatcher]
# The directory where the code import worker should be directed to
# store its logs.
=== added directory 'lib/lp/services/macaroons'
=== added file 'lib/lp/services/macaroons/__init__.py'
=== added file 'lib/lp/services/macaroons/interfaces.py'
--- lib/lp/services/macaroons/interfaces.py 1970-01-01 00:00:00 +0000
+++ lib/lp/services/macaroons/interfaces.py 2016-10-07 15:56:06 +0000
@@ -0,0 +1,45 @@
+# Copyright 2016 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Interface to a policy for issuing and verifying macaroons."""
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+__all__ = [
+ 'IMacaroonIssuer',
+ ]
+
+from zope.interface import Interface
+
+
+class IMacaroonIssuerPublic(Interface):
+ """Public interface to a policy for verifying macaroons."""
+
+ def checkMacaroonIssuer(macaroon):
+ """Check that `macaroon` was issued by this issuer.
+
+ This does not verify that the macaroon is valid for a given context,
+ only that it could be valid for some context. Use this in the
+ authentication part of an authentication/authorisation API.
+ """
+
+ def verifyMacaroon(macaroon, context):
+ """Verify that `macaroon` is valid for `context`.
+
+ :param macaroon: A `Macaroon`.
+ :param context: The context to check.
+ :return: True if `macaroon` is valid for `context`, otherwise False.
+ """
+
+
+class IMacaroonIssuer(IMacaroonIssuerPublic):
+ """Interface to a policy for issuing and verifying macaroons."""
+
+ def issueMacaroon(context):
+ """Issue a macaroon for `context`.
+
+ :param context: The context that the returned macaroon should relate
+ to.
+ :return: A macaroon.
+ """
Follow ups
