Page MenuHomePhabricator
Differential D19542

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

Authored by artms on Jul 27 2018, 7:50 AM.
Tags
None
Referenced Files
F14076701: D19542.id46759.diff
Thu, Nov 21, 7:24 PM
Unknown Object (File)
Wed, Nov 20, 6:45 PM
Unknown Object (File)
Tue, Nov 19, 11:44 AM
Unknown Object (File)
Fri, Nov 15, 4:52 AM
Unknown Object (File)
Thu, Nov 14, 7:02 AM
Unknown Object (File)
Mon, Nov 11, 5:27 AM
Unknown Object (File)
Sun, Nov 10, 8:00 AM
Unknown Object (File)
Fri, Nov 8, 9:54 PM
View All Files
Subscribers
Korvin

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:

  1. add repository to Phabricator which is accessed via ssh
  2. Use git 2.18+
  3. Enable wire protocol in /etc/gitconfig:
[protocol]
    version = 2
  1. Try refreshing repository: phabricator/bin/repository update somecallsing
  2. 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
Branch
ssh_options
Lint
Lint Errors
SeverityLocationCodeMessage
Errorsrc/applications/differential/editor/DifferentialTransactionEditor.php:688PHL1Unknown Symbol
Unit
No Test Coverage
Build Status
Buildable 20519
Build 27866: arc lint + arc unit

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
artms updated this revision to Diff 46723.Jul 27 2018, 7:55 AM

Old arcanist

Harbormaster completed remote builds in B20520: Diff 46723.Jul 27 2018, 7:55 AM
epriestley requested changes to this revision.Jul 27 2018, 4:03 PM

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
artms updated this revision to Diff 46731.Jul 30 2018, 9:31 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

epriestley requested changes to this revision.Jul 30 2018, 3:01 PM

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
artms updated this revision to Diff 46751.Aug 1 2018, 11:45 AM
  • 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
epriestley accepted this revision.Aug 1 2018, 1:08 PM

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.

Revision Contents
Changeset List

PathSize
scripts/
ssh/
17 lines

Diff 46722

View Options

scripts/ssh/ssh-connect.php

Loading...
Phabricator · Built by Phacility