Details

Reviewers

epriestley
Pawka

Group Reviewers

Blessed Reviewers

Commits

rP4c09e88c9582: Add parsing for ssh options (-o) which are passed when using GIT v2 wire…

Summary

Makes ssh-connect compatible with Git v2 wire protocol over SSH

More details about git V2 wire: https://opensource.googleblog.com/2018/05/introducing-git-protocol-version-2.html

git command (2.18+) passes extra options (-o "SendEnv GIT_PROTOCOL") to underlying ssh command to enable v2 wire protocol (environment variable enabling new protocol).

Phabricator ssh-connect command doesn't understand -o options and interprets it as host parts hence when you enable git v2 all clones/ls-remotes crash with:

#0 ExecFuture::resolvex() called at [<phabricator>/src/applications/repository/storage/PhabricatorRepository.php:525]
#1 PhabricatorRepository::execxRemoteCommand(string, PhutilOpaqueEnvelope) called at [<phabricator>/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php:400]
#2 PhabricatorRepositoryPullEngine::loadGitRemoteRefs(PhabricatorRepository) called at [<phabricator>/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php:343]
#3 PhabricatorRepositoryPullEngine::executeGitUpdate() called at [<phabricator>/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php:126]
#4 PhabricatorRepositoryPullEngine::pullRepositoryWithLock() called at [<phabricator>/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php:40]
#5 PhabricatorRepositoryPullEngine::pullRepository() called at [<phabricator>/src/applications/repository/management/PhabricatorRepositoryManagementUpdateWorkflow.php:59]
#6 PhabricatorRepositoryManagementUpdateWorkflow::execute(PhutilArgumentParser) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:441]
#7 PhutilArgumentParser::parseWorkflowsFull(array) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:333]
#8 PhutilArgumentParser::parseWorkflows(array) called at [<phabricator>/scripts/repository/manage_repositories.php:22]
COMMAND
git ls-remote '********'
STDOUT
(empty)
STDERR
ssh: Could not resolve hostname -o: Name or service not known
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
at [<phutil>/src/future/exec/ExecFuture.php:369]

Test Plan

How to reproduce:

add repository to Phabricator which is accessed via ssh
Use git 2.18+
Enable wire protocol in /etc/gitconfig:

[protocol]
    version = 2

Try refreshing repository: phabricator/bin/repository update somecallsing
Repository update fails with ssh: Could not resolve hostname -o: Name or service not known

after this changes - updates will succeed

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

artms created this revision.Jul 27 2018, 7:50 AM

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptJul 27 2018, 7:50 AM

artms requested review of this revision.Jul 27 2018, 7:50 AM

Herald added a subscriber: Korvin. · View Herald TranscriptJul 27 2018, 7:50 AM

Harbormaster completed remote builds in B20519: Diff 46722.Jul 27 2018, 7:50 AM

Old arcanist

Harbormaster completed remote builds in B20520: Diff 46723.Jul 27 2018, 7:55 AM

I think we should whitelist the permitted options and allow only SendEnv GIT_PROTOCOL exactly. Some options (including ProxyCommand) are dangerous. See T12961 for context.

Although we control invocations of ssh-connect and it "should" be safe to allow dangerous options, T12961 "should" also never have happened in the first place, but no VCS interacted with the SSH subprocess safely.

The downside is that we need to bring a similar patch upstream every few years, but our turnaround time on this stuff is generally pretty good and I'd like to balance things in favor of security paranoia over future-version-compatibility here.

This revision now requires changes to proceed.Jul 27 2018, 4:03 PM

artms planned changes to this revision.Jul 30 2018, 8:02 AM

Allow only SendEnv=GIT_PROTOCOL ssh option

Harbormaster completed remote builds in B20529: Diff 46731.Jul 30 2018, 9:31 AM

artms added inline comments.Jul 30 2018, 9:33 AM

scripts/ssh/ssh-connect.php
129	This will essentially dump "unrecognized" options, please advise if such behavior is acceptable? We can also "crash" if unrecognized options is passed to have less surprises in future when some unrecognized option is passed to ssh command

Yeah, just throw an exception -- something like this, probably:

throw new Exeception(
  pht(
    'Disallowed option "%s" given with "-o". Allowed options are: %s.',
    $option,
    implode(', ', $allowed_ssh_options)));

Looks good otherwise. Thanks!

This revision now requires changes to proceed.Jul 30 2018, 3:01 PM

Throw error if unknown ssh option is passed

Harbormaster completed remote builds in B20550: Diff 46751.Aug 1 2018, 11:45 AM

artms marked an inline comment as done.Aug 1 2018, 11:49 AM

Thanks!

I added you to Blessed Committers, so you should be able to land this yourself. See that project description for more instructions, or let me know if you run into trouble.

(I also added you to Community, granting you sweeping janitorial powers on this install.)

This revision is now accepted and ready to land.Aug 1 2018, 1:08 PM

Closed by commit rP4c09e88c9582: Add parsing for ssh options (-o) which are passed when using GIT v2 wire… (authored by artms). · Explain WhyAug 2 2018, 1:43 PM

This revision was automatically updated to reflect the committed changes.

Add parsing for ssh options (-o) which are passed when using GIT v2 wire protocol by git command (SSH transport)
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 46759

scripts/ssh/ssh-connect.php

Add parsing for ssh options (-o) which are passed when using GIT v2 wire protocol by git command (SSH transport)ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 46759

scripts/ssh/ssh-connect.php

Add parsing for ssh options (-o) which are passed when using GIT v2 wire protocol by git command (SSH transport)
ClosedPublic
Actions

Revision Contents
Changeset List