launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #23621
[Merge] lp:~cjwatson/launchpad/refactor-git-code-import-authz into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/refactor-git-code-import-authz into lp:launchpad.
Commit message:
Refactor code import authorisation to be clearer and safer.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/refactor-git-code-import-authz/+merge/367068
We now use the special "+launchpad-services" requester ID for code import job pushes, which makes somewhat more sense than treating them as a special kind of anonymous request.
If macaroon authentication fails, we now explicitly return an Unauthorized fault rather than falling through to end up with either PermissionDenied for a public repository or GitRepositoryNotFound for a public one. This is easier to understand and also more consistent with RFC 2616's description of "401 Unauthorized".
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/refactor-git-code-import-authz into lp:launchpad.
=== modified file 'lib/lp/code/xmlrpc/git.py'
--- lib/lp/code/xmlrpc/git.py 2019-04-26 13:13:37 +0000
+++ lib/lp/code/xmlrpc/git.py 2019-05-07 17:07:40 +0000
@@ -39,7 +39,10 @@
GitTargetError,
InvalidNamespace,
)
-from lp.code.interfaces.codehosting import LAUNCHPAD_ANONYMOUS
+from lp.code.interfaces.codehosting import (
+ LAUNCHPAD_ANONYMOUS,
+ LAUNCHPAD_SERVICES,
+ )
from lp.code.interfaces.gitapi import IGitAPI
from lp.code.interfaces.githosting import IGitHostingClient
from lp.code.interfaces.gitjob import IGitRefScanJobSource
@@ -97,19 +100,51 @@
repository, extra_path = getUtility(IGitLookup).getByPath(path)
if repository is None:
return None
+
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.
- # For the time being, this only works for code imports.
- assert (
- naked_repository.repository_type == GitRepositoryType.IMPORTED)
- hosting_path = naked_repository.getInternalPath()
- writable = True
- private = naked_repository.private
- else:
+ writable = None
+
+ if macaroon_raw is not None:
+ verified = self._verifyMacaroon(macaroon_raw, naked_repository)
+ if not verified:
+ # Macaroon authentication failed. Don't fall back to the
+ # requester's permissions, since macaroons typically have
+ # additional constraints. Instead, just return
+ # "authorisation required", thus preventing probing for the
+ # existence of repositories without presenting valid
+ # credentials.
+ raise faults.Unauthorized()
+
+ # Internal services use macaroons to authenticate. In this
+ # case, we know that the authentication parameters specifically
+ # grant access to this repository because we were able to verify
+ # the macaroon using the repository as its context, so we can
+ # bypass other checks. This is only permitted for selected
+ # macaroon issuers, currently only code import jobs.
+ # XXX cjwatson 2019-05-07: Remove None once
+ # authenticateWithPassword returns LAUNCHPAD_SERVICES for code
+ # import jobs on production.
+ if requester in (None, LAUNCHPAD_SERVICES):
+ repository_type = naked_repository.repository_type
+ if (verified.issuer_name == "code-import-job" and
+ repository_type == GitRepositoryType.IMPORTED):
+ hosting_path = naked_repository.getInternalPath()
+ writable = True
+ private = naked_repository.private
+ else:
+ raise faults.Unauthorized()
+
+ # In any other case, the macaroon constrains the permissions of
+ # the principal, so fall through to doing normal user
+ # authorisation.
+ elif requester == LAUNCHPAD_SERVICES:
+ # Internal services must authenticate using a macaroon.
+ raise faults.Unauthorized()
+
+ if writable is None:
+ # This isn't an authorised internal service, so perform normal
+ # user authorisation.
try:
hosting_path = repository.getInternalPath()
except Unauthorized:
@@ -321,11 +356,15 @@
"""See `IGitAPI`."""
# 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()
+ if not username:
+ verified = self._verifyMacaroon(password)
+ if verified:
+ auth_params = {"macaroon": password}
+ if verified.issuer_name == "code-import-job":
+ auth_params["uid"] = LAUNCHPAD_SERVICES
+ return auth_params
+ # Only macaroons are supported for password authentication.
+ return faults.Unauthorized()
def _renderPermissions(self, set_of_permissions):
"""Render a set of permission strings for XML-RPC output."""
=== modified file 'lib/lp/code/xmlrpc/tests/test_git.py'
--- lib/lp/code/xmlrpc/tests/test_git.py 2019-04-23 12:30:16 +0000
+++ lib/lp/code/xmlrpc/tests/test_git.py 2019-05-07 17:07:40 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015-2018 Canonical Ltd. This software is licensed under the
+# Copyright 2015-2019 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Tests for the internal Git API."""
@@ -26,6 +26,7 @@
TargetRevisionControlSystems,
)
from lp.code.errors import GitRepositoryCreationFault
+from lp.code.interfaces.codehosting import LAUNCHPAD_SERVICES
from lp.code.interfaces.codeimportjob import ICodeImportJobWorkflow
from lp.code.interfaces.gitcollection import IAllGitRepositories
from lp.code.interfaces.gitjob import IGitRefScanJobSource
@@ -66,7 +67,7 @@
def assertGitRepositoryNotFound(self, requester, path, permission="read",
can_authenticate=False, macaroon_raw=None):
"""Assert that the given path cannot be translated."""
- if requester is not None:
+ if requester is not None and requester != LAUNCHPAD_SERVICES:
requester = requester.id
auth_params = {"uid": requester, "can-authenticate": can_authenticate}
if macaroon_raw is not None:
@@ -80,7 +81,7 @@
permission="read", can_authenticate=False,
macaroon_raw=None):
"""Assert that looking at the given path returns PermissionDenied."""
- if requester is not None:
+ if requester is not None and requester != LAUNCHPAD_SERVICES:
requester = requester.id
auth_params = {"uid": requester, "can-authenticate": can_authenticate}
if macaroon_raw is not None:
@@ -90,19 +91,21 @@
def assertUnauthorized(self, requester, path,
message="Authorisation required.",
- permission="read", can_authenticate=False):
+ permission="read", can_authenticate=False,
+ macaroon_raw=None):
"""Assert that looking at the given path returns Unauthorized."""
- if requester is not None:
+ if requester is not None and requester != LAUNCHPAD_SERVICES:
requester = requester.id
- fault = self.git_api.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.git_api.translatePath(path, permission, auth_params)
self.assertEqual(faults.Unauthorized(message), fault)
def assertNotFound(self, requester, path, message, permission="read",
can_authenticate=False):
"""Assert that looking at the given path returns NotFound."""
- if requester is not None:
+ if requester is not None and requester != LAUNCHPAD_SERVICES:
requester = requester.id
fault = self.git_api.translatePath(
path, permission,
@@ -114,7 +117,7 @@
can_authenticate=False):
"""Assert that looking at the given path returns
InvalidSourcePackageName."""
- if requester is not None:
+ if requester is not None and requester != LAUNCHPAD_SERVICES:
requester = requester.id
fault = self.git_api.translatePath(
path, permission,
@@ -124,7 +127,7 @@
def assertInvalidBranchName(self, requester, path, message,
permission="read", can_authenticate=False):
"""Assert that looking at the given path returns InvalidBranchName."""
- if requester is not None:
+ if requester is not None and requester != LAUNCHPAD_SERVICES:
requester = requester.id
fault = self.git_api.translatePath(
path, permission,
@@ -134,7 +137,7 @@
def assertOopsOccurred(self, requester, path,
permission="read", can_authenticate=False):
"""Assert that looking at the given path OOPSes."""
- if requester is not None:
+ if requester is not None and requester != LAUNCHPAD_SERVICES:
requester = requester.id
fault = self.git_api.translatePath(
path, permission,
@@ -149,7 +152,7 @@
def assertTranslates(self, requester, path, repository, writable,
permission="read", can_authenticate=False,
macaroon_raw=None, trailing="", private=False):
- if requester is not None:
+ if requester is not None and requester != LAUNCHPAD_SERVICES:
requester = requester.id
auth_params = {"uid": requester, "can-authenticate": can_authenticate}
if macaroon_raw is not None:
@@ -194,7 +197,7 @@
def assertHasRefPermissions(self, requester, repository, ref_paths,
permissions, macaroon_raw=None):
- if requester is not None:
+ if requester is not None and requester != LAUNCHPAD_SERVICES:
requester = requester.id
auth_params = {"uid": requester}
if macaroon_raw is not None:
@@ -977,24 +980,30 @@
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",
+ self.assertUnauthorized(
+ LAUNCHPAD_SERVICES, path, permission="write",
macaroon_raw=macaroons[0].serialize())
with celebrity_logged_in("vcs_imports"):
getUtility(ICodeImportJobWorkflow).startJob(jobs[0], machine)
self.assertTranslates(
+ LAUNCHPAD_SERVICES, path, code_imports[0].git_repository, True,
+ permission="write", macaroon_raw=macaroons[0].serialize())
+ # XXX cjwatson 2019-05-07: Remove this once we remove the
+ # corresponding compatibility code.
+ self.assertTranslates(
None, path, code_imports[0].git_repository, True,
permission="write", macaroon_raw=macaroons[0].serialize())
- self.assertPermissionDenied(
- None, path, permission="write",
+ self.assertUnauthorized(
+ LAUNCHPAD_SERVICES, path, permission="write",
macaroon_raw=macaroons[1].serialize())
- self.assertPermissionDenied(
- None, path, permission="write",
+ self.assertUnauthorized(
+ LAUNCHPAD_SERVICES, path, permission="write",
macaroon_raw=Macaroon(
location=config.vhost.mainsite.hostname, identifier="another",
key="another-secret").serialize())
- self.assertPermissionDenied(
- None, path, permission="write", macaroon_raw="nonsense")
+ self.assertUnauthorized(
+ LAUNCHPAD_SERVICES, path, permission="write",
+ macaroon_raw="nonsense")
def test_translatePath_private_code_import(self):
# A code import worker with a suitable macaroon can write to a
@@ -1017,27 +1026,32 @@
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",
+ self.assertUnauthorized(
+ LAUNCHPAD_SERVICES, path, permission="write",
macaroon_raw=macaroons[0].serialize())
with celebrity_logged_in("vcs_imports"):
getUtility(ICodeImportJobWorkflow).startJob(jobs[0], machine)
self.assertTranslates(
+ LAUNCHPAD_SERVICES, path, code_imports[0].git_repository, True,
+ permission="write", macaroon_raw=macaroons[0].serialize(),
+ private=True)
+ # XXX cjwatson 2019-05-07: Remove this once we remove the
+ # corresponding compatibility code.
+ self.assertTranslates(
None, path, code_imports[0].git_repository, True,
permission="write", macaroon_raw=macaroons[0].serialize(),
private=True)
- # The expected faults are slightly different from the public case,
- # because we deny the existence of private repositories.
- self.assertGitRepositoryNotFound(
- None, path, permission="write",
+ self.assertUnauthorized(
+ LAUNCHPAD_SERVICES, path, permission="write",
macaroon_raw=macaroons[1].serialize())
- self.assertGitRepositoryNotFound(
- None, path, permission="write",
+ self.assertUnauthorized(
+ LAUNCHPAD_SERVICES, path, permission="write",
macaroon_raw=Macaroon(
location=config.vhost.mainsite.hostname, identifier="another",
key="another-secret").serialize())
- self.assertGitRepositoryNotFound(
- None, path, permission="write", macaroon_raw="nonsense")
+ self.assertUnauthorized(
+ LAUNCHPAD_SERVICES, path, permission="write",
+ macaroon_raw="nonsense")
def test_notify(self):
# The notify call creates a GitRefScanJob.
@@ -1081,7 +1095,7 @@
issuer = getUtility(IMacaroonIssuer, "code-import-job")
macaroon = removeSecurityProxy(issuer).issueMacaroon(job)
self.assertEqual(
- {"macaroon": macaroon.serialize()},
+ {"macaroon": macaroon.serialize(), "uid": "+launchpad-services"},
self.git_api.authenticateWithPassword("", macaroon.serialize()))
other_macaroon = Macaroon(identifier="another", key="another-secret")
self.assertIsInstance(
=== modified file 'lib/lp/services/configure.zcml'
--- lib/lp/services/configure.zcml 2019-04-16 14:30:40 +0000
+++ lib/lp/services/configure.zcml 2019-05-07 17:07:40 +0000
@@ -16,6 +16,7 @@
<include package=".inlinehelp" file="meta.zcml" />
<include package=".job" />
<include package=".librarian" />
+ <include package=".macaroons" />
<include package=".mail" />
<include package=".memcache" />
<include package=".messages" />
=== added file 'lib/lp/services/macaroons/configure.zcml'
--- lib/lp/services/macaroons/configure.zcml 1970-01-01 00:00:00 +0000
+++ lib/lp/services/macaroons/configure.zcml 2019-05-07 17:07:40 +0000
@@ -0,0 +1,14 @@
+<!-- Copyright 2019 Canonical Ltd. This software is licensed under the
+ GNU Affero General Public License version 3 (see the file LICENSE).
+-->
+
+<configure
+ xmlns="http://namespaces.zope.org/zope";
+ i18n_domain="launchpad">
+
+ <!-- MacaroonVerificationResult -->
+ <class class="lp.services.macaroons.model.MacaroonVerificationResult">
+ <allow interface="lp.services.macaroons.interfaces.IMacaroonVerificationResult" />
+ </class>
+
+</configure>
=== modified file 'lib/lp/services/macaroons/interfaces.py'
--- lib/lp/services/macaroons/interfaces.py 2019-05-01 16:43:54 +0000
+++ lib/lp/services/macaroons/interfaces.py 2019-05-07 17:07:40 +0000
@@ -9,9 +9,13 @@
__all__ = [
'BadMacaroonContext',
'IMacaroonIssuer',
+ 'IMacaroonVerificationResult',
]
-from zope.interface import Interface
+from zope.interface import (
+ Attribute,
+ Interface,
+ )
from zope.schema import Bool
@@ -25,9 +29,17 @@
self.context = context
+class IMacaroonVerificationResult(Interface):
+ """Information about a verified macaroon."""
+
+ issuer_name = Attribute("The name of the macaroon's issuer.")
+
+
class IMacaroonIssuerPublic(Interface):
"""Public interface to a policy for verifying macaroons."""
+ identifier = Attribute("An identifying name for this issuer.")
+
issuable_via_authserver = Bool(
"Does this issuer allow issuing macaroons via the authserver?")
@@ -46,7 +58,8 @@
appended to this list.
:param kwargs: Additional arguments that issuers may require to
verify a macaroon.
- :return: True if `macaroon` is valid for `context`, otherwise False.
+ :return: An `IMacaroonVerificationResult` if `macaroon` is valid for
+ `context`, otherwise None.
"""
=== modified file 'lib/lp/services/macaroons/model.py'
--- lib/lp/services/macaroons/model.py 2019-05-01 16:43:54 +0000
+++ lib/lp/services/macaroons/model.py 2019-05-07 17:07:40 +0000
@@ -15,9 +15,24 @@
Verifier,
)
from pymacaroons.exceptions import MacaroonVerificationFailedException
+from zope.interface import implementer
from lp.services.config import config
-from lp.services.macaroons.interfaces import BadMacaroonContext
+from lp.services.macaroons.interfaces import (
+ BadMacaroonContext,
+ IMacaroonVerificationResult,
+ )
+
+
+@implementer(IMacaroonVerificationResult)
+class MacaroonVerificationResult:
+
+ def __init__(self, identifier):
+ self._issuer_name = identifier
+
+ @property
+ def issuer_name(self):
+ return self._issuer_name
class MacaroonIssuerBase:
@@ -52,7 +67,7 @@
@property
def identifier(self):
- """An identifying name for this issuer."""
+ """See `IMacaroonIssuer`."""
raise NotImplementedError
@property
@@ -129,20 +144,21 @@
if errors is not None:
errors.append(
"Macaroon has unknown location '%s'." % macaroon.location)
- return False
+ return None
if require_context and context is None:
if errors is not None:
errors.append(
"Expected macaroon verification context but got None.")
- return False
+ return None
if context is not None:
try:
context = self.checkVerificationContext(context)
except BadMacaroonContext as e:
if errors is not None:
errors.append(str(e))
- return False
+ return None
seen = set()
+ verified = MacaroonVerificationResult(self.identifier)
def verify(caveat):
try:
@@ -175,7 +191,10 @@
try:
verifier = Verifier()
verifier.satisfy_general(verify)
- return verifier.verify(macaroon, self._root_secret)
+ if verifier.verify(macaroon, self._root_secret):
+ return verified
+ else:
+ return None
# XXX cjwatson 2019-04-24: This can currently raise a number of
# other exceptions in the presence of non-well-formed input data,
# but most of them are too broad to reasonably catch so we let them
@@ -184,4 +203,4 @@
except MacaroonVerificationFailedException as e:
if errors is not None and not errors:
errors.append(str(e))
- return False
+ return None
=== modified file 'lib/lp/services/macaroons/testing.py'
--- lib/lp/services/macaroons/testing.py 2019-04-25 22:10:36 +0000
+++ lib/lp/services/macaroons/testing.py 2019-05-07 17:07:40 +0000
@@ -12,6 +12,7 @@
]
from testtools.content import text_content
+from testtools.matchers import MatchesStructure
def find_caveats_by_name(macaroon, caveat_name):
@@ -25,8 +26,11 @@
def assertMacaroonVerifies(self, issuer, macaroon, context, **kwargs):
errors = []
try:
- self.assertTrue(issuer.verifyMacaroon(
- macaroon, context, errors=errors, **kwargs))
+ verified = issuer.verifyMacaroon(
+ macaroon, context, errors=errors, **kwargs)
+ self.assertIsNotNone(verified)
+ self.assertThat(verified, MatchesStructure.byEquality(
+ issuer_name=issuer.identifier))
except Exception:
if errors:
self.addDetail("errors", text_content("\n".join(errors)))
@@ -35,6 +39,6 @@
def assertMacaroonDoesNotVerify(self, expected_errors, issuer, macaroon,
context, **kwargs):
errors = []
- self.assertFalse(issuer.verifyMacaroon(
+ self.assertIsNone(issuer.verifyMacaroon(
macaroon, context, errors=errors, **kwargs))
self.assertEqual(expected_errors, errors)
Follow ups
