launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #29622
[Merge] ~cjwatson/launchpad:git-ignore-auth-on-notify into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:git-ignore-auth-on-notify into launchpad:master.
Commit message:
Ignore authentication for git push notifications
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/436545
Back when Ioana was implementing sending pack statistics for git push notifications (which are emitted by turnip at the end of each push), one of my review comments was that, since this was directly updating properties of the repository, the call ought to be authenticated. I remember being slightly ambivalent about that even at the time, but we went along with it since it seemed a relatively harmless piece of caution.
However, recently I've been experimenting with getting snapcraft to use personal access tokens, and one thing I noticed was that authenticated push notifications make it difficult to use appropriate expiry times. `snapcraft remote-build` creates a repository, generates an access token for it, and then pushes to it; the token relieves the user of the need to set up an SSH key before this system can work. The time between generating the token and starting the push is very short, and the token is only used once, so it seems as though a short expiry time (perhaps a minute) should be more than enough. However, because `notify` is called at the end of the push and also checks authorization, we have to choose an expiry time long enough to push an entire potentially-large repository over a connection of unknown speed.
We already have to trust turnip to handle part of the git authorization process, so an authorization check that effectively just ensures that turnip hasn't processed a push without appropriate authorization doesn't add any interesting security. Being able to update pack statistics on a repository would be a very low-value attack in any case. On the other hand, being able to use short-expiry tokens effectively feels like a much clearer security advantage.
I'm therefore reversing my previous view: it seems acceptably safe to skip authentication for `notify`, given the other safeguards around that and the benefits for use of personal access tokens.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:git-ignore-auth-on-notify into launchpad:master.
diff --git a/lib/lp/code/xmlrpc/git.py b/lib/lp/code/xmlrpc/git.py
index 9793646..3b97a85 100644
--- a/lib/lp/code/xmlrpc/git.py
+++ b/lib/lp/code/xmlrpc/git.py
@@ -589,11 +589,11 @@ class GitAPI(LaunchpadXMLRPCView):
del result
@return_fault
- def _notify(self, requester, translated_path, statistics, auth_params):
+ def _notify(self, translated_path, statistics, auth_params):
logger = self._getLogger()
- if requester == LAUNCHPAD_ANONYMOUS:
- requester = None
- repository = getUtility(IGitLookup).getByHostingPath(translated_path)
+ repository = removeSecurityProxy(
+ getUtility(IGitLookup).getByHostingPath(translated_path)
+ )
if repository is None:
fault = faults.NotFound(
"No repository found for '%s'." % translated_path
@@ -602,22 +602,30 @@ class GitAPI(LaunchpadXMLRPCView):
return fault
if repository is None:
raise faults.GitRepositoryNotFound(translated_path)
- if auth_params is not None:
- verified = self._verifyAuthParams(
- requester, repository, auth_params
+ if statistics:
+ removeSecurityProxy(repository).setRepackData(
+ statistics.get("loose_object_count"),
+ statistics.get("pack_count"),
)
- writable = self._isWritable(requester, repository, verified)
- if writable and statistics:
- removeSecurityProxy(repository).setRepackData(
- statistics.get("loose_object_count"),
- statistics.get("pack_count"),
- )
getUtility(IGitRefScanJobSource).create(
removeSecurityProxy(repository)
)
def notify(self, translated_path, statistics, auth_params):
"""See `IGitAPI`."""
+ # This receives authentication parameters for historical reasons,
+ # but ignores them. We already checked authorization at the start
+ # of the operation whose completion we're now being notified about,
+ # so we don't do so again here, as it can have weird effects: for
+ # example, it should be possible to have a short-duration personal
+ # access token that expires between the start and the end of a long
+ # push operation. We have to trust turnip anyway, and the worst
+ # thing that any of this can do is spuriously update statistics.
+ #
+ # If we feel the need to authorize notify calls in future, then it
+ # should be done by checking whether a previous operation was
+ # authorized, e.g. by generating a single-use token earlier. At the
+ # moment this seems like overkill, though.
logger = self._getLogger()
logger.info(
"Request received: notify('%s', '%d', '%d')",
@@ -626,14 +634,7 @@ class GitAPI(LaunchpadXMLRPCView):
statistics.get("pack_count"),
)
- requester_id = _get_requester_id(auth_params)
- result = run_with_login(
- requester_id,
- self._notify,
- translated_path,
- statistics,
- auth_params,
- )
+ result = self._notify(translated_path, statistics, auth_params)
try:
if isinstance(result, xmlrpc.client.Fault):
logger.error("notify failed: %r", result)
diff --git a/lib/lp/code/xmlrpc/tests/test_git.py b/lib/lp/code/xmlrpc/tests/test_git.py
index 6cd603c..01b10a5 100644
--- a/lib/lp/code/xmlrpc/tests/test_git.py
+++ b/lib/lp/code/xmlrpc/tests/test_git.py
@@ -3187,210 +3187,25 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
[job] = list(job_source.iterReady())
self.assertEqual(repository, job.repository)
- def assertSetsRepackData(self, repo, auth_params):
+ def test_notify_set_repack_data(self):
+ # The notify call sets the repack indicators (loose_object_count,
+ # pack_count, date_last_scanned) when received from turnip.
+ requester_owner = self.factory.makePerson()
+ repository = self.factory.makeGitRepository(owner=requester_owner)
start_time = datetime.now(pytz.UTC)
self.assertIsNone(
self.assertDoesNotFault(
None,
"notify",
- repo.getInternalPath(),
+ repository.getInternalPath(),
{"loose_object_count": 5, "pack_count": 2},
- auth_params,
+ {"uid": requester_owner.id},
)
)
end_time = datetime.now(pytz.UTC)
- naked_repo = removeSecurityProxy(repo)
- self.assertEqual(5, naked_repo.loose_object_count)
- self.assertEqual(2, naked_repo.pack_count)
- self.assertBetween(start_time, naked_repo.date_last_scanned, end_time)
-
- def test_notify_set_repack_data(self):
- # The notify call sets the repack
- # indicators (loose_object_count, pack_count, date_last_scanned)
- # when received from Turnip for a user
- # that has their ordinary privileges on the corresponding repository
- requester_owner = self.factory.makePerson()
- repository = self.factory.makeGitRepository(owner=requester_owner)
-
- self.assertSetsRepackData(repository, {"uid": requester_owner.id})
-
- def test_notify_set_repack_data_user_macaroon(self):
- self.pushConfig("codehosting", git_macaroon_secret_key="some-secret")
- requester = self.factory.makePerson()
- repository = self.factory.makeGitRepository(owner=requester)
- issuer = getUtility(IMacaroonIssuer, "git-repository")
- with person_logged_in(requester):
- macaroon = removeSecurityProxy(issuer).issueMacaroon(
- repository, user=requester
- )
- auth_params = _make_auth_params(
- requester, macaroon_raw=macaroon.serialize()
- )
- self.assertSetsRepackData(repository, auth_params)
-
- def test_notify_set_repack_data_user_mismatch(self):
- # notify refuses macaroons in the case where the
- # user doesn't match what the issuer claims was verified. (We use a
- # dummy issuer for this, since this is a stopgap check to defend
- # against issuer bugs)
-
- issuer = DummyMacaroonIssuer()
- # Claim to be the code-import-job issuer. This is a bit weird, but
- # it gets us past the difficulty that only certain named issuers are
- # allowed to confer write permissions.
- issuer.identifier = "code-import-job"
- self.useFixture(
- ZopeUtilityFixture(issuer, IMacaroonIssuer, name="code-import-job")
- )
- requesters = [self.factory.makePerson() for _ in range(2)]
- owner = self.factory.makeTeam(members=requesters)
- repository = self.factory.makeGitRepository(owner=owner)
- macaroon = issuer.issueMacaroon(repository)
-
- for verified_user, authorized, unauthorized in (
- (
- requesters[0],
- [requesters[0]],
- [LAUNCHPAD_SERVICES, requesters[1], None],
- ),
- ([None, NO_USER], [], [LAUNCHPAD_SERVICES] + requesters + [None]),
- ):
- issuer._verified_user = verified_user
- for requester in authorized:
- login(ANONYMOUS)
- auth_params = _make_auth_params(
- requester, macaroon_raw=macaroon.serialize()
- )
- self.assertSetsRepackData(repository, auth_params)
-
- for requester in unauthorized:
- login(ANONYMOUS)
- auth_params = _make_auth_params(
- requester, macaroon_raw=macaroon.serialize()
- )
- self.assertFault(
- faults.Unauthorized,
- None,
- "notify",
- repository.getInternalPath(),
- {"loose_object_count": 5, "pack_count": 2},
- auth_params,
- )
-
- def test_notify_set_repack_data_user_access_token(self):
- requester = self.factory.makePerson()
- repository = self.factory.makeGitRepository(owner=requester)
- _, token = self.factory.makeAccessToken(
- owner=requester,
- target=repository,
- scopes=[AccessTokenScope.REPOSITORY_PUSH],
- )
- auth_params = _make_auth_params(
- requester, access_token_id=removeSecurityProxy(token).id
- )
- self.assertSetsRepackData(repository, auth_params)
-
- def test_notify_set_repack_data_user_access_token_nonexistent(self):
- requester = self.factory.makePerson()
- repository = self.factory.makeGitRepository(owner=requester)
- auth_params = _make_auth_params(requester, access_token_id=0)
- self.assertFault(
- faults.Unauthorized,
- None,
- "notify",
- repository.getInternalPath(),
- {"loose_object_count": 5, "pack_count": 2},
- auth_params,
- )
-
- def test_notify_set_repack_data_code_import(self):
- # We set the repack data on a repository from a code import worker
- # with a suitable macaroon.
- self.pushConfig(
- "launchpad", internal_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
- ]
-
- with celebrity_logged_in("vcs_imports"):
- getUtility(ICodeImportJobWorkflow).startJob(jobs[0], machine)
-
- auth_params = _make_auth_params(
- LAUNCHPAD_SERVICES, macaroon_raw=macaroons[0].serialize()
- )
- self.assertSetsRepackData(code_imports[0].git_repository, auth_params)
-
- auth_params = _make_auth_params(
- LAUNCHPAD_SERVICES, macaroon_raw=macaroons[1].serialize()
- )
- self.assertFault(
- faults.Unauthorized,
- None,
- "notify",
- code_imports[0].git_repository.getInternalPath(),
- {"loose_object_count": 5, "pack_count": 2},
- auth_params,
- )
-
- def test_notify_set_repack_data_private_code_import(self):
- self.pushConfig(
- "launchpad", internal_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)
- ]
- private_repository = code_imports[0].git_repository
- path = private_repository.getInternalPath()
- removeSecurityProxy(private_repository).transitionToInformationType(
- InformationType.PRIVATESECURITY, private_repository.owner
- )
- 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
- ]
-
- with celebrity_logged_in("vcs_imports"):
- getUtility(ICodeImportJobWorkflow).startJob(jobs[0], machine)
-
- auth_params = _make_auth_params(
- LAUNCHPAD_SERVICES, macaroon_raw=macaroons[0].serialize()
- )
- self.assertSetsRepackData(code_imports[0].git_repository, auth_params)
-
- auth_params = _make_auth_params(
- LAUNCHPAD_SERVICES, macaroon_raw=macaroons[1].serialize()
- )
- self.assertFault(
- faults.Unauthorized,
- None,
- "notify",
- path,
- {"loose_object_count": 5, "pack_count": 2},
- auth_params,
- )
+ self.assertEqual(5, repository.loose_object_count)
+ self.assertEqual(2, repository.pack_count)
+ self.assertBetween(start_time, repository.date_last_scanned, end_time)
def test_authenticateWithPassword(self):
self.assertFault(
