[Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/git-code-import-security-deletion into lp:launchpad
Colin Watson has proposed merging
lp:~cjwatson/launchpad/git-code-import-security-deletion into lp:launchpad with
lp:~cjwatson/launchpad/codeimport-git-worker as a prerequisite.
Commit message:
Grant vcs-imports edit access to imported repositories, and handle deletion of
imported repositories.
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/git-code-import-security-deletion/+merge/308319
This doesn't grant vcs-imports push access: IMPORTED repositories are handled
specially in lp.code.xmlrpc.git. However, it's still useful for the
vcs-imports team to be able to edit import metadata.
--
Your team Launchpad code reviewers is requested to review the proposed merge of
lp:~cjwatson/launchpad/git-code-import-security-deletion into lp:launchpad.
=== modified file 'lib/lp/_schema_circular_imports.py'
--- lib/lp/_schema_circular_imports.py 2016-06-25 08:05:06 +
+++ lib/lp/_schema_circular_imports.py 2016-10-12 23:49:44 +
@@ -578,6 +578,7 @@
patch_collection_property(IGitRepository, 'subscriptions', IGitSubscription)
patch_entry_return_type(IGitRepository, 'subscribe', IGitSubscription)
patch_entry_return_type(IGitRepository, 'getSubscription', IGitSubscription)
+patch_reference_property(IGitRepository, 'code_import', ICodeImport)
patch_collection_property(
IGitRepository, 'landing_targets', IBranchMergeProposal)
patch_collection_property(
=== modified file 'lib/lp/code/interfaces/gitnamespace.py'
--- lib/lp/code/interfaces/gitnamespace.py 2016-10-06 15:54:47 +
+++ lib/lp/code/interfaces/gitnamespace.py 2016-10-12 23:49:44 +
@@ -119,10 +119,12 @@
:return: An `InformationType`.
"""
-def validateRegistrant(registrant):
+def validateRegistrant(registrant, repository=None):
"""Check that the registrant can create a repository in this namespace.
:param registrant: An `IPerson`.
+:param repository: An optional `IGitRepository` to also check when
+working with imported repositories.
:raises GitRepositoryCreatorNotMemberOfOwnerTeam: if the namespace
owner is a team and the registrant is not in that team.
:raises GitRepositoryCreatorNotOwner: if the namespace owner is an
=== modified file 'lib/lp/code/interfaces/gitrepository.py'
--- lib/lp/code/interfaces/gitrepository.py 2016-10-05 09:54:42 +
+++ lib/lp/code/interfaces/gitrepository.py 2016-10-12 23:49:44 +
@@ -260,6 +260,11 @@
title=_("Persons subscribed to this repository."),
readonly=True, value_type=Reference(IPerson)))
+code_import = exported(Reference(
+title=_("The associated CodeImport, if any."),
+# Really ICodeImport, patched in _schema_circular_imports.py.
+schema=Interface))
+
def getRefByPath(path):
"""Look up a single reference in this repository by path.
@@ -946,14 +951,24 @@
return identities
-def user_has_special_git_repository_access(user):
+def user_has_special_git_repository_access(user, repository=None):
"""Admins have special access.
:param user: An `IPerson` or None.
+:param repository: An `IGitRepository` or None when checking collection
+access.
"""
if user is None:
return False
roles = IPersonRoles(user)
if roles.in_admin:
return True
-return False
+if repository is None:
+return False
+code_import = repository.code_import
+if code_import is None:
+return False
+return (
+roles.in_vcs_imports
+or (IPersonRoles(repository.owner).in_vcs_imports
+and user.inTerm(code_import.registrant)))
=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py 2016-06-20 20:32:36 +
+++ lib/lp/code/model/branch.py 2016-10-12 23:49:44 +
@@ -117,6 +117,7 @@
BRANCH_ID_ALIAS_PREFIX,
compose_public_url,
)
+from lp.code.interfaces.codeimport import ICodeImportSet
from lp.code.interfaces.seriessourcepackagebranch import (
IFindOfficialBranchLinks,
)
@@ -813,8 +814,7 @@
@cachedproperty
def code_import(self):
-from lp.code.model.codeimport import CodeImportSet
-return CodeImportSet().getByBranch(self)
+return getUtility(ICodeImportSet).getByBranch(self)
def _deletionRequirements(self):
"""Determine what operations must be performed to delete this branch.
@@ -1561,8 +1561,7 @@
self, code_import, _('This is the import data for this branch.'))
def __call__(self):
-from lp.code.model.codeimport import CodeImportSet
-CodeImportSet().delete(self.affected_object)
+getUtility(ICodeImportSet).delete(self.affected_object)
Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/codeimport-git-worker into lp:launchpad
Review: Approve code
Diff comments:
>
> === modified file 'lib/lp/codehosting/codeimport/worker.py'
> --- lib/lp/codehosting/codeimport/worker.py 2016-10-11 15:36:32 +
> +++ lib/lp/codehosting/codeimport/worker.py 2016-10-11 15:36:32 +
> @@ -918,3 +973,67 @@
> """See `PullingImportWorker.probers`."""
> from bzrlib.bzrdir import BzrProber, RemoteBzrProber
> return [BzrProber, RemoteBzrProber]
> +
> +
> +class GitToGitImportWorker(ImportWorker):
> +"""An import worker for imports from Git to Git."""
> +
> +def _runGit(self, *args, **kwargs):
> +"""Run git with arguments, sending output to the logger."""
> +cmd = ["git"] + list(args)
> +git_process = subprocess.Popen(
> +cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, **kwargs)
> +for line in git_process.stdout:
> +line = line.decode("UTF-8", "replace").rstrip("\n")
> +# Remove any user/password data from URLs.
> +line = re.sub(r"://([^:]*:[^@]*@)(\S+)", r"://\2", line)
> +self._logger.info(line)
> +retcode = git_process.wait()
> +if retcode:
> +raise subprocess.CalledProcessError(retcode, cmd)
> +
> +def _doImport(self):
> +self._logger.info("Starting job.")
> +self._logger.info(config.codeimport.git_repository_store)
> +try:
> +self._opener_policy.checkOneURL(self.source_details.url)
When we move these to prodstack we should probably set up some more sensible
outbound rules, but I guess it'll do for now. Hopefully git can't be confused
into emitting random HTTP things to stderr.
> +except BadUrl as e:
> +self._logger.info("Invalid URL: %s" % e)
> +return CodeImportWorkerExitCode.FAILURE_FORBIDDEN
> +unauth_target_url = urljoin(
> +config.codeimport.git_repository_store,
> +self.source_details.target_id)
> +split = urlsplit(unauth_target_url)
> +if split.hostname:
> +target_netloc = ":%s@%s" % (
> +self.source_details.macaroon.serialize(), split.hostname)
> +else:
> +target_netloc = ""
> +target_url = urlunsplit([
> +split.scheme, target_netloc, split.path, "", ""])
> +# XXX cjwatson 2016-10-11: Ideally we'd put credentials in a
> +# credentials store instead. However, git only accepts credentials
> +# that have both a non-empty username and a non-empty password.
> +self._logger.info("Getting existing repository from hosting
> service.")
> +try:
> +self._runGit("clone", "--bare", target_url, "repository")
> +except subprocess.CalledProcessError as e:
> +self._logger.info(
> +"Unable to get existing repository from hosting service: "
> +"%s" % e)
> +return CodeImportWorkerExitCode.FAILURE
> +self._logger.info("Fetching remote repository.")
> +try:
> +self._runGit(
> +"remote", "add", "-f", "--mirror=fetch",
> +"mirror", self.source_details.url, cwd="repository")
HEAD handling is always atrocious... but I hadn't realised git-remote fiddled
around so manually in ways that aren't exposed elsewhere.
> +except subprocess.CalledProcessError as e:
> +self._logger.info("Unable to fetch remote repository: %s" % e)
> +return CodeImportWorkerExitCode.FAILURE_INVALID
> +self._logger.info("Pushing repository to hosting service.")
> +try:
> +self._runGit("push", "--mirror", target_url, cwd="repository")
> +except subprocess.CalledProcessError as e:
> +self._logger.info("Unable to push to hosting service: %s" % e)
> +return CodeImportWorkerExitCode.FAILURE
> +return CodeImportWorkerExitCode.SUCCESS
>
> === modified file 'lib/lp/services/config/schema-lazr.conf'
> --- lib/lp/services/config/schema-lazr.conf 2016-10-11 15:36:32 +
> +++ lib/lp/services/config/schema-lazr.conf 2016-10-11 15:36:32 +
> @@ -417,7 +421,7 @@
> svn_revisions_import_limit: 500
>
> # Secret key for macaroons used to grant git push permission to workers.
> -macaroon_secret_key:
> +macaroon_secret_key: none
I have no words, but carry on.
>
> [codeimportdispatcher]
> # The directory where the code import worker should be directed to
--
https://code.launchpad.net/~cjwatson/launchpad/codeimport-git-worker/+merge/308142
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
___
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/codeimport-git-worker into lp:launchpad
I've sprinkled a bunch of extra comments around in various places.
Diff comments:
>
> === modified file 'lib/lp/codehosting/codeimport/worker.py'
> --- lib/lp/codehosting/codeimport/worker.py 2016-10-11 15:36:32 +
> +++ lib/lp/codehosting/codeimport/worker.py 2016-10-11 15:36:32 +
> @@ -918,3 +973,67 @@
> """See `PullingImportWorker.probers`."""
> from bzrlib.bzrdir import BzrProber, RemoteBzrProber
> return [BzrProber, RemoteBzrProber]
> +
> +
> +class GitToGitImportWorker(ImportWorker):
> +"""An import worker for imports from Git to Git."""
> +
> +def _runGit(self, *args, **kwargs):
> +"""Run git with arguments, sending output to the logger."""
> +cmd = ["git"] + list(args)
> +git_process = subprocess.Popen(
> +cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, **kwargs)
> +for line in git_process.stdout:
> +line = line.decode("UTF-8", "replace").rstrip("\n")
> +# Remove any user/password data from URLs.
> +line = re.sub(r"://([^:]*:[^@]*@)(\S+)", r"://\2", line)
> +self._logger.info(line)
> +retcode = git_process.wait()
> +if retcode:
> +raise subprocess.CalledProcessError(retcode, cmd)
> +
> +def _doImport(self):
> +self._logger.info("Starting job.")
> +self._logger.info(config.codeimport.git_repository_store)
> +try:
> +self._opener_policy.checkOneURL(self.source_details.url)
As far as I can see from http.c, git follows up to 20 redirects as long as
they're to http/https/ftp/ftps schemes, and there's no way to configure this.
The only things that this would let through that checkOneURL tries to forbid
would be redirects to git.launchpad.net and redirects to a blacklisted
hostname, which in practice is always localhost or 127.0.0.1. Perhaps we can
get away with this state for the time being? Redirects to git.launchpad.net
would basically be a slightly annoying waste of resources but nothing more,
while redirects to localhost should in practice be harmless given that code
import workers don't run web or FTP servers.
The only way I can think of to improve isolation here would be to direct all
git requests through a proxy, which would be a non-trivial chunk of work.
> +except BadUrl as e:
> +self._logger.info("Invalid URL: %s" % e)
> +return CodeImportWorkerExitCode.FAILURE_FORBIDDEN
> +unauth_target_url = urljoin(
> +config.codeimport.git_repository_store,
> +self.source_details.target_id)
> +split = urlsplit(unauth_target_url)
> +if split.hostname:
> +target_netloc = ":%s@%s" % (
> +self.source_details.macaroon.serialize(), split.hostname)
> +else:
> +target_netloc = ""
> +target_url = urlunsplit([
> +split.scheme, target_netloc, split.path, "", ""])
> +# XXX cjwatson 2016-10-11: Ideally we'd put credentials in a
> +# credentials store instead. However, git only accepts credentials
> +# that have both a non-empty username and a non-empty password.
> +self._logger.info("Getting existing repository from hosting
> service.")
> +try:
> +self._runGit("clone", "--bare", target_url, "repository")
> +except subprocess.CalledProcessError as e:
> +self._logger.info(
> +"Unable to get existing repository from hosting service: "
> +"%s" % e)
> +return CodeImportWorkerExitCode.FAILURE
> +self._logger.info("Fetching remote repository.")
> +try:
> +self._runGit(
> +"remote", "add", "-f", "--mirror=fetch",
> +"mirror", self.source_details.url, cwd="repository")
More or less, with the addition of --prune. Though good grief HEAD handling is
horrible, but I've just added a comment about that for now.
> +except subprocess.CalledProcessError as e:
> +self._logger.info("Unable to fetch remote repository: %s" % e)
> +return CodeImportWorkerExitCode.FAILURE_INVALID
> +self._logger.info("Pushing repository to hosting service.")
> +try:
> +self._runGit("push", "--mirror", target_url, cwd="repository")
> +except subprocess.CalledProcessError as e:
> +self._logger.info("Unable to push to hosting service: %s" % e)
> +return CodeImportWorkerExitCode.FAILURE
> +return CodeImportWorkerExitCode.SUCCESS
>
> === modified file 'lib/lp/services/config/schema-lazr.conf'
> --- lib/lp/services/config/schema-lazr.conf 2016-10-11 15:36:32 +
> +++ lib/lp/services/config/schema-lazr.conf 2016-10-11 15:36:32 +
> @@ -417,7 +421,7 @@
> svn_revisions_import_limit: 500
>
> # Secret key for macaroons used to grant git push permission to workers.
> -macaroon_secret_key:
>
[Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/codeimport-create-hosting into lp:launchpad
The proposal to merge lp:~cjwatson/launchpad/codeimport-create-hosting into lp:launchpad has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~cjwatson/launchpad/codeimport-create-hosting/+merge/308132 -- Your team Launchpad code reviewers is subscribed to branch lp:launchpad. ___ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
[Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/codeimport-worker-refactor into lp:launchpad
The proposal to merge lp:~cjwatson/launchpad/codeimport-worker-refactor into lp:launchpad has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~cjwatson/launchpad/codeimport-worker-refactor/+merge/308122 -- Your team Launchpad code reviewers is subscribed to branch lp:launchpad. ___ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
[Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/codeimport-source-details-refactor into lp:launchpad
The proposal to merge lp:~cjwatson/launchpad/codeimport-source-details-refactor into lp:launchpad has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~cjwatson/launchpad/codeimport-source-details-refactor/+merge/308117 -- Your team Launchpad code reviewers is subscribed to branch lp:launchpad. ___ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
[Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/codeimport-git-auth into lp:launchpad
The proposal to merge lp:~cjwatson/launchpad/codeimport-git-auth into lp:launchpad has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~cjwatson/launchpad/codeimport-git-auth/+merge/307968 -- Your team Launchpad code reviewers is subscribed to branch lp:launchpad. ___ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/codeimport-git-auth into lp:launchpad
Diff comments:
>
> === modified file 'lib/lp/code/model/codeimportjob.py'
> --- lib/lp/code/model/codeimportjob.py2016-10-03 17:00:56 +
> +++ lib/lp/code/model/codeimportjob.py2016-10-07 15:56:06 +
> @@ -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
I agree it's odd, but on balance I prefer the opposite resolution from the one
I think you're suggesting: I've moved the GitRepository -> CodeImportJob lookup
out to the caller in GitAPI instead. That way, CodeImportJobMacaroonIssuer
operates on CodeImportJob objects and GitAPI handles the specifics of
GitRepository objects, which makes a lot more sense to me.
> +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
--
https://code.launchpad.net/~cjwatson/launchpad/codeimport-git-auth/+merge/307968
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
___
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/codeimport-worker-refactor into lp:launchpad
Review: Approve code
Diff comments:
> === modified file 'lib/lp/code/xmlrpc/codeimportscheduler.py'
> --- lib/lp/code/xmlrpc/codeimportscheduler.py 2016-10-11 13:46:57 +
> +++ lib/lp/code/xmlrpc/codeimportscheduler.py 2016-10-11 13:46:57 +
> @@ -69,10 +69,10 @@
> job = self._getJob(job_id)
> arguments = CodeImportSourceDetails.fromCodeImportJob(
> job).asArguments()
> -branch = job.code_import.branch
> -branch_url = canonical_url(branch)
> -log_file_name = '%s.log' % branch.unique_name[1:].replace('/', '-')
> -return (arguments, branch_url, log_file_name)
> +target = job.code_import.target
> +target_url = canonical_url(target)
> +log_file_name = '%s.log' % target.unique_name[1:].replace('/', '-')
> +return (arguments, target_url, log_file_name)
One of these days I think every concept will be named "target".
>
> @return_fault
> def _updateHeartbeat(self, job_id, log_tail):
--
https://code.launchpad.net/~cjwatson/launchpad/codeimport-worker-refactor/+merge/308122
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
___
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/codeimport-create-hosting into lp:launchpad
Review: Approve code
Diff comments:
>
> === modified file 'lib/lp/code/model/tests/test_codeimport.py'
> --- lib/lp/code/model/tests/test_codeimport.py2016-10-03 17:00:56
> +
> +++ lib/lp/code/model/tests/test_codeimport.py2016-10-11 14:42:21
> +
> @@ -66,15 +67,22 @@
> "supports_source_cvs": True,
> "supports_source_svn": True,
> "supports_source_bzr": True,
> +"needs_hosting_fixture": False,
needs_git_hosting_fixture surely?
> }),
> ("GitRepository", {
> "target_rcs_type": TargetRevisionControlSystems.GIT,
> "supports_source_cvs": False,
> "supports_source_svn": False,
> "supports_source_bzr": False,
> +"needs_hosting_fixture": True,
> }),
> ]
>
> +def setUp(self, *args, **kwargs):
> +super(TestCodeImportBase, self).setUp(*args, **kwargs)
> +if self.needs_hosting_fixture:
> +self.hosting_fixture = self.useFixture(GitHostingFixture())
> +
>
> class TestCodeImportCreation(TestCodeImportBase):
> """Test the creation of CodeImports."""
--
https://code.launchpad.net/~cjwatson/launchpad/codeimport-create-hosting/+merge/308132
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
___
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/codeimport-source-details-refactor into lp:launchpad
Review: Approve code -- https://code.launchpad.net/~cjwatson/launchpad/codeimport-source-details-refactor/+merge/308117 Your team Launchpad code reviewers is subscribed to branch lp:launchpad. ___ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/codeimport-git-worker into lp:launchpad
Review: Approve code
Diff comments:
> === modified file 'configs/development/launchpad-lazr.conf'
> --- configs/development/launchpad-lazr.conf 2016-06-30 16:05:11 +
> +++ configs/development/launchpad-lazr.conf 2016-10-11 15:36:32 +
> @@ -55,6 +55,7 @@
>
> [codeimport]
> bazaar_branch_store: file:///tmp/bazaar-branches
> +git_repository_store: https://git.launchpad.dev/
It's entirely confusing that git_repository_store and bazaar_branch_store are
pretty radically different things. bazaar_branch_store is the intermediate tree
on maple (formerly escudero), whereas git_repository_store is the actual
hosting tree. The lack of puller for git imports is certainly a relief, but the
chance of this not causing someone to faceplant is tiny.
A comment might be sufficient, but could we also get away with using the
existing git_browse_root (GitRepository.git_https_url would be ideal, but
returns None for private repos which are technically a possibility for imports)?
> foreign_tree_store: file:///tmp/foreign-branches
>
> [codeimportdispatcher]
>
> === modified file 'lib/lp/codehosting/codeimport/tests/test_worker.py'
> --- lib/lp/codehosting/codeimport/tests/test_worker.py2016-10-11
> 15:36:32 +
> +++ lib/lp/codehosting/codeimport/tests/test_worker.py2016-10-11
> 15:36:32 +
> @@ -1416,6 +1453,19 @@
> str(code_import.branch.id), 'git',
> 'git://git.example.com/project.git']))
>
> +def test_git_to_git_arguments(self):
> +self.pushConfig('codeimport', macaroon_secret_key='some-secret')
> +self.useFixture(GitHostingFixture())
> +code_import = self.factory.makeCodeImport(
> +git_repo_url="git://git.example.com/project.git",
> +target_rcs_type=TargetRevisionControlSystems.GIT)
> +self.assertArgumentsMatch(
> +code_import, MatchesListwise([
> +Equals(code_import.git_repository.unique_name),
> +Equals('git:git'),
> Equals('git://git.example.com/project.git'),
> +CodeImportJobMacaroonVerifies(code_import.git_repository)]),
> +start_job=True)
A long time in the future in an editor far, far away: "Why would I want to
start the job? Silly test author, I'll drop that pointlessness while I'm
refactoring. Why is the macaroon verification silently failing now?"
> +
> def test_cvs_arguments(self):
> code_import = self.factory.makeCodeImport(
> cvs_root=':pserver:f...@example.com/bar', cvs_module='bar')
>
> === modified file 'lib/lp/codehosting/codeimport/worker.py'
> --- lib/lp/codehosting/codeimport/worker.py 2016-10-11 15:36:32 +
> +++ lib/lp/codehosting/codeimport/worker.py 2016-10-11 15:36:32 +
> @@ -269,36 +282,50 @@
> of the information suitable for passing around on executables' command
> lines.
>
> -:ivar target_id: The id of the Bazaar branch associated with this code
> -import, used for locating the existing import and the foreign tree.
> +:ivar target_id: The id of the Bazaar branch or the path of the Git
> +repository associated with this code import, used for locating the
> +existing import and the foreign tree.
> :ivar rcstype: 'cvs', 'git', 'bzr-svn', 'bzr' as appropriate.
> +:ivar target_rcstype: 'bzr' or 'git' as appropriate.
> :ivar url: The branch URL if rcstype in ['bzr-svn', 'git', 'bzr'], None
> otherwise.
> :ivar cvs_root: The $CVSROOT if rcstype == 'cvs', None otherwise.
> :ivar cvs_module: The CVS module if rcstype == 'cvs', None otherwise.
> :ivar stacked_on_url: The URL of the branch that the associated branch
> is stacked on, if any.
> +:ivar macaroon: A macaroon granting authority to push to the target
> +repository if target_rcstype == 'git', None otherwise.
> """
>
> -def __init__(self, target_id, rcstype, url=None, cvs_root=None,
> - cvs_module=None, stacked_on_url=None):
> +def __init__(self, target_id, rcstype, target_rcstype, url=None,
> + cvs_root=None, cvs_module=None, stacked_on_url=None,
> + macaroon=None):
> self.target_id = target_id
> self.rcstype = rcstype
> +self.target_rcstype = target_rcstype
> self.url = url
> self.cvs_root = cvs_root
> self.cvs_module = cvs_module
> self.stacked_on_url = stacked_on_url
> +self.macaroon = macaroon
>
> @classmethod
> def fromArguments(cls, arguments):
> """Convert command line-style arguments to an instance."""
> -target_id = int(arguments.pop(0))
> +target_id = arguments.pop(0)
> rcstype = arguments.pop(0)
> +if ':' in rcstype:
> +rcstype, target_rcstype = rcstype.split(':', 1)
> +else:
> +target_rcstype = 'bzr'
XXX this for cleanup post-deployment?
> if
Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/codeimport-git-auth into lp:launchpad
Review: Approve code
Diff comments:
>
> === modified file 'lib/lp/code/model/codeimportjob.py'
> --- lib/lp/code/model/codeimportjob.py2016-10-03 17:00:56 +
> +++ lib/lp/code/model/codeimportjob.py2016-10-07 15:56:06 +
> @@ -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)
We should probably also check that the location matches.
It's also slightly confusing that verifyMacaroon doesn't call
checkMacaroonIssuer -- seems like a recipe for mistakes later.
> +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
This case is only used in tests. Would it be better to refactor this slightly
so we didn't have the weird unused alternate case and tests that don't test the
normal path?
> +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
Cunning. Undebuggable and fragile, but it'll work for now and hopefully nobody
will touch it in inappropriate ways before we have a more generic mechanism.
>
> === modified file 'lib/lp/code/xmlrpc/git.py'
> --- lib/lp/code/xmlrpc/git.py 2016-10-06 20:37:39 +
> +++ lib/lp/code/xmlrpc/git.py 2016-10-07 15:56:06 +
> @@ -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):
The new "requester" parameter seems unused.
> 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
I wouldn't half mind a quick assertion here that repository_type == IMPORTED.
Makes the
