This is an archive of the discontinued Mercurial Phabricator instance.

Differential D2034

sshpeer: move handshake outside of sshpeer
ClosedPublic

Authored by indygreg on Feb 4 2018, 10:35 PM.

Details

Reviewers
yuja
Group Reviewers
hg-reviewers
Commits
rHG80a2b8ae42a1: sshpeer: move handshake outside of sshpeer
Summary

With the handshake now performed before a peer class is instantiated,
we can now instantiate a different peer class depending on the results
of the handshake.

Our test extension had to change to cope with the new API. Because
we now issue the command via raw I/O calls and don't call
_callstream(), we no longer have to register the fake command.
(_callstream() uses the command registration to see what args to
send).

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

indygreg created this revision.Feb 4 2018, 10:35 PM
lothiraldan added a subscriber: lothiraldan.Feb 5 2018, 8:18 AM
lothiraldan added inline comments.
mercurial/sshpeer.py
175

Nit that could be outside of the try: Except:

422

Nit: Could the except block could be replaced by a finally block?

indygreg marked an inline comment as done.Feb 5 2018, 12:04 PM
indygreg added inline comments.
mercurial/sshpeer.py
422

No, because the pipes are used for the remainder of the connection.

I would like to switch peers to use context managers so we have stronger lifetimes for these pipes. But that will require a bit of changes throughout the code base. If someone were to write those patches, I'd review them. This is definitely the cycle to do it, as we're breaking all the APIs around the wire protocol.

indygreg marked an inline comment as done.Feb 5 2018, 12:15 PM
indygreg updated this revision to Diff 5229.
indygreg updated this revision to Diff 5235.Feb 5 2018, 5:21 PM
yuja accepted this revision.Feb 6 2018, 8:07 AM
This revision is now accepted and ready to land.Feb 6 2018, 8:07 AM
Closed by commit rHG80a2b8ae42a1: sshpeer: move handshake outside of sshpeer (authored by indygreg). · Explain WhyFeb 6 2018, 8:24 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 5229

mercurial/sshpeer.py

pipeo, pipei, pipee, proc = util.popen4(cmd, bufsize=0, env=sshenv) pipeo, pipei, pipee, proc = util.popen4(cmd, bufsize=0, env=sshenv)
pipei = util.bufferedinputpipe(pipei) pipei = util.bufferedinputpipe(pipei)
pipei = doublepipe(ui, pipei, pipee) pipei = doublepipe(ui, pipei, pipee)
pipeo = doublepipe(ui, pipeo, pipee) pipeo = doublepipe(ui, pipeo, pipee)
return proc, pipei, pipeo, pipee return proc, pipei, pipeo, pipee
def _performhandshake(ui, pipei, pipeo, pipee):
def badresponse():
msg = _('no suitable response from remote hg')
hint = ui.config('ui', 'ssherrorhint')
raise error.RepoError(msg, hint=hint)
requestlog = ui.configbool('devel', 'debug.peer-request')
try:
pairsarg = '%s-%s' % ('0' * 40, '0' * 40)
handshake = [
'hello\n',
'between\n',
'pairs %d\n' % len(pairsarg),
pairsarg,
]
lothiraldanUnsubmitted
Not Done

Nit that could be outside of the try: Except:

lothiraldan: Nit that could be outside of the try: Except:
if requestlog:
ui.debug('devel-peer-request: hello\n')
ui.debug('sending hello command\n')
if requestlog:
ui.debug('devel-peer-request: between\n')
ui.debug('devel-peer-request: pairs: %d bytes\n' %len(pairsarg))
ui.debug('sending between command\n')
pipeo.write(''.join(handshake))
pipeo.flush()
except IOError:
badresponse()
lines = ['', 'dummy']
max_noise = 500
while lines[-1] and max_noise:
try:
l = pipei.readline()
_forwardoutput(ui, pipee)
if lines[-1] == '1\n' and l == '\n':
break
if l:
ui.debug('remote: ', l)
lines.append(l)
max_noise -= 1
except IOError:
badresponse()
else:
badresponse()
caps = set()
for l in reversed(lines):
if l.startswith('capabilities:'):
caps.update(l[:-1].split(':')[1].split())
break
return caps
class sshpeer(wireproto.wirepeer): class sshpeer(wireproto.wirepeer):
def __init__(self, ui, url, proc, pipei, pipeo, pipee): def __init__(self, ui, url, proc, pipei, pipeo, pipee, caps):
"""Create a peer from an existing SSH connection. """Create a peer from an existing SSH connection.
``proc`` is a handle on the underlying SSH process. ``proc`` is a handle on the underlying SSH process.
``pipei``, ``pipeo``, and ``pipee`` are handles on the stdin, ``pipei``, ``pipeo``, and ``pipee`` are handles on the stdin,
stdout, and stderr file descriptors for that process. stdout, and stderr file descriptors for that process.
``caps`` is a set of capabilities supported by the remote.
""" """
self._url = url self._url = url
self._ui = ui self._ui = ui
# self._subprocess is unused. Keeping a handle on the process # self._subprocess is unused. Keeping a handle on the process
# holds a reference and prevents it from being garbage collected. # holds a reference and prevents it from being garbage collected.
self._subprocess = proc self._subprocess = proc
self._pipei = pipei self._pipei = pipei
self._pipeo = pipeo self._pipeo = pipeo
self._pipee = pipee self._pipee = pipee
self._caps = caps
self._validaterepo()
# Begin of _basepeer interface. # Begin of _basepeer interface.
@util.propertycache @util.propertycache
def ui(self): def ui(self):
return self._ui return self._ui
def url(self): def url(self):
# Begin of _basewirecommands interface. # Begin of _basewirecommands interface.
def capabilities(self): def capabilities(self):
return self._caps return self._caps
# End of _basewirecommands interface. # End of _basewirecommands interface.
def _validaterepo(self):
def badresponse():
msg = _("no suitable response from remote hg")
hint = self.ui.config("ui", "ssherrorhint")
self._abort(error.RepoError(msg, hint=hint))
try:
pairsarg = '%s-%s' % ('0' * 40, '0' * 40)
handshake = [
'hello\n',
'between\n',
'pairs %d\n' % len(pairsarg),
pairsarg,
]
requestlog = self.ui.configbool('devel', 'debug.peer-request')
if requestlog:
self.ui.debug('devel-peer-request: hello\n')
self.ui.debug('sending hello command\n')
if requestlog:
self.ui.debug('devel-peer-request: between\n')
self.ui.debug('devel-peer-request: pairs: %d bytes\n' %
len(pairsarg))
self.ui.debug('sending between command\n')
self._pipeo.write(''.join(handshake))
self._pipeo.flush()
except IOError:
badresponse()
lines = ["", "dummy"]
max_noise = 500
while lines[-1] and max_noise:
try:
l = self._pipei.readline()
_forwardoutput(self.ui, self._pipee)
if lines[-1] == "1\n" and l == "\n":
break
if l:
self.ui.debug("remote: ", l)
lines.append(l)
max_noise -= 1
except IOError:
badresponse()
else:
badresponse()
self._caps = set()
for l in reversed(lines):
if l.startswith("capabilities:"):
self._caps.update(l[:-1].split(":")[1].split())
break
def _readerr(self): def _readerr(self):
_forwardoutput(self.ui, self._pipee) _forwardoutput(self.ui, self._pipee)
def _abort(self, exception): def _abort(self, exception):
self._cleanup() self._cleanup()
raise exception raise exception
def _cleanup(self): def _cleanup(self):
ui.debug('running %s\n' % cmd) ui.debug('running %s\n' % cmd)
res = ui.system(cmd, blockedtag='sshpeer', environ=sshenv) res = ui.system(cmd, blockedtag='sshpeer', environ=sshenv)
if res != 0: if res != 0:
raise error.RepoError(_('could not create remote repo')) raise error.RepoError(_('could not create remote repo'))
proc, pipei, pipeo, pipee = _makeconnection(ui, sshcmd, args, remotecmd, proc, pipei, pipeo, pipee = _makeconnection(ui, sshcmd, args, remotecmd,
remotepath, sshenv) remotepath, sshenv)
return sshpeer(ui, path, proc, pipei, pipeo, pipee) try:
caps = _performhandshake(ui, pipei, pipeo, pipee)
except Exception:
_cleanuppipes(ui, pipei, pipeo, pipee)
raise
lothiraldanUnsubmitted
Done

Nit: Could the except block could be replaced by a finally block?

lothiraldan: Nit: Could the except block could be replaced by a finally block?
indygregAuthorUnsubmitted
Not Done

No, because the pipes are used for the remainder of the connection.

I would like to switch peers to use context managers so we have stronger lifetimes for these pipes. But that will require a bit of changes throughout the code base. If someone were to write those patches, I'd review them. This is definitely the cycle to do it, as we're breaking all the APIs around the wire protocol.

indygreg: No, because the pipes are used for the remainder of the connection. I would like to switch…
return sshpeer(ui, path, proc, pipei, pipeo, pipee, caps)

tests/sshprotoext.py

# sshprotoext.py - Extension to test behavior of SSH protocol # sshprotoext.py - Extension to test behavior of SSH protocol
# #
# Copyright 2018 Gregory Szorc <gregory.szorc@gmail.com> # Copyright 2018 Gregory Szorc <gregory.szorc@gmail.com>
# #
# This software may be used and distributed according to the terms of the # This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version. # GNU General Public License version 2 or any later version.
# This extension replaces the SSH server started via `hg serve --stdio`. # This extension replaces the SSH server started via `hg serve --stdio`.
# The server behaves differently depending on environment variables. # The server behaves differently depending on environment variables.
from __future__ import absolute_import from __future__ import absolute_import
from mercurial import ( from mercurial import (
error, error,
extensions,
registrar, registrar,
sshpeer, sshpeer,
wireproto, wireproto,
wireprotoserver, wireprotoserver,
) )
configtable = {} configtable = {}
configitem = registrar.configitem(configtable) configitem = registrar.configitem(configtable)
self._sendresponse(b'') self._sendresponse(b'')
l = self._fin.readline() l = self._fin.readline()
assert l == b'between\n' assert l == b'between\n'
rsp = wireproto.dispatch(self._repo, self, b'between') rsp = wireproto.dispatch(self._repo, self, b'between')
self._handlers[rsp.__class__](self, rsp) self._handlers[rsp.__class__](self, rsp)
super(prehelloserver, self).serve_forever() super(prehelloserver, self).serve_forever()
class extrahandshakecommandspeer(sshpeer.sshpeer): def performhandshake(orig, ui, pipei, pipeo, pipee):
"""An ssh peer that sends extra commands as part of initial handshake.""" """Wrapped version of sshpeer._performhandshake to send extra commands."""
def _validaterepo(self): mode = ui.config(b'sshpeer', b'handshake-mode')
mode = self._ui.config(b'sshpeer', b'handshake-mode')
if mode == b'pre-no-args': if mode == b'pre-no-args':
self._callstream(b'no-args') ui.debug(b'sending no-args command\n')
return super(extrahandshakecommandspeer, self)._validaterepo() pipeo.write(b'no-args\n')
pipeo.flush()
return orig(ui, pipei, pipeo, pipee)
elif mode == b'pre-multiple-no-args': elif mode == b'pre-multiple-no-args':
self._callstream(b'unknown1') ui.debug(b'sending unknown1 command\n')
self._callstream(b'unknown2') pipeo.write(b'unknown1\n')
self._callstream(b'unknown3') ui.debug(b'sending unknown2 command\n')
return super(extrahandshakecommandspeer, self)._validaterepo() pipeo.write(b'unknown2\n')
ui.debug(b'sending unknown3 command\n')
pipeo.write(b'unknown3\n')
pipeo.flush()
return orig(ui, pipei, pipeo, pipee)
else: else:
raise error.ProgrammingError(b'unknown HANDSHAKECOMMANDMODE: %s' % raise error.ProgrammingError(b'unknown HANDSHAKECOMMANDMODE: %s' %
mode) mode)
def registercommands():
def dummycommand(repo, proto):
raise error.ProgrammingError('this should never be called')
wireproto.wireprotocommand(b'no-args', b'')(dummycommand)
wireproto.wireprotocommand(b'unknown1', b'')(dummycommand)
wireproto.wireprotocommand(b'unknown2', b'')(dummycommand)
wireproto.wireprotocommand(b'unknown3', b'')(dummycommand)
def extsetup(ui): def extsetup(ui):
# It's easier for tests to define the server behavior via environment # It's easier for tests to define the server behavior via environment
# variables than config options. This is because `hg serve --stdio` # variables than config options. This is because `hg serve --stdio`
# has to be invoked with a certain form for security reasons and # has to be invoked with a certain form for security reasons and
# `dummyssh` can't just add `--config` flags to the command line. # `dummyssh` can't just add `--config` flags to the command line.
servermode = ui.environ.get(b'SSHSERVERMODE') servermode = ui.environ.get(b'SSHSERVERMODE')
if servermode == b'banner': if servermode == b'banner':
wireprotoserver.sshserver = bannerserver wireprotoserver.sshserver = bannerserver
elif servermode == b'no-hello': elif servermode == b'no-hello':
wireprotoserver.sshserver = prehelloserver wireprotoserver.sshserver = prehelloserver
elif servermode: elif servermode:
raise error.ProgrammingError(b'unknown server mode: %s' % servermode) raise error.ProgrammingError(b'unknown server mode: %s' % servermode)
peermode = ui.config(b'sshpeer', b'mode') peermode = ui.config(b'sshpeer', b'mode')
if peermode == b'extra-handshake-commands': if peermode == b'extra-handshake-commands':
sshpeer.sshpeer = extrahandshakecommandspeer extensions.wrapfunction(sshpeer, '_performhandshake', performhandshake)
registercommands()
elif peermode: elif peermode:
raise error.ProgrammingError(b'unknown peer mode: %s' % peermode) raise error.ProgrammingError(b'unknown peer mode: %s' % peermode)

tests/test-check-interfaces.py

def __init__(self): def __init__(self):
self.ui = uimod.ui() self.ui = uimod.ui()
def filtered(self, name): def filtered(self, name):
pass pass
def _restrictcapabilities(self, caps): def _restrictcapabilities(self, caps):
pass pass
# Facilitates testing sshpeer without requiring an SSH server. # Facilitates testing sshpeer without requiring an SSH server.
class testingsshpeer(sshpeer.sshpeer):
def _validaterepo(self, *args, **kwargs):
pass
class badpeer(httppeer.httppeer): class badpeer(httppeer.httppeer):
def __init__(self): def __init__(self):
super(badpeer, self).__init__(uimod.ui(), 'http://localhost') super(badpeer, self).__init__(uimod.ui(), 'http://localhost')
self.badattribute = True self.badattribute = True
def badmethod(self): def badmethod(self):
pass pass
def main(): def main():
ui = uimod.ui() ui = uimod.ui()
checkobject(badpeer()) checkobject(badpeer())
checkobject(httppeer.httppeer(ui, 'http://localhost')) checkobject(httppeer.httppeer(ui, 'http://localhost'))
checkobject(localrepo.localpeer(dummyrepo())) checkobject(localrepo.localpeer(dummyrepo()))
checkobject(testingsshpeer(ui, 'ssh://localhost/foo', None, None, None, checkobject(sshpeer.sshpeer(ui, 'ssh://localhost/foo', None, None, None,
None)) None, None))
checkobject(bundlerepo.bundlepeer(dummyrepo())) checkobject(bundlerepo.bundlepeer(dummyrepo()))
checkobject(statichttprepo.statichttppeer(dummyrepo())) checkobject(statichttprepo.statichttppeer(dummyrepo()))
checkobject(unionrepo.unionpeer(dummyrepo())) checkobject(unionrepo.unionpeer(dummyrepo()))
main() main()

tests/test-ssh-proto.t

0 0
384 384
capabilities: lookup changegroupsubset branchmap pushkey known getbundle unbundlehash batch streamreqs=generaldelta,revlogv1 $USUAL_BUNDLE2_CAPS_SERVER$ unbundle=HG10GZ,HG10BZ,HG10UN capabilities: lookup changegroupsubset branchmap pushkey known getbundle unbundlehash batch streamreqs=generaldelta,revlogv1 $USUAL_BUNDLE2_CAPS_SERVER$ unbundle=HG10GZ,HG10BZ,HG10UN
1 1
$ hg --config sshpeer.mode=extra-handshake-commands --config sshpeer.handshake-mode=pre-no-args --debug debugpeer ssh://user@dummy/server $ hg --config sshpeer.mode=extra-handshake-commands --config sshpeer.handshake-mode=pre-no-args --debug debugpeer ssh://user@dummy/server
running * "*/tests/dummyssh" 'user@dummy' 'hg -R server serve --stdio' (glob) running * "*/tests/dummyssh" 'user@dummy' 'hg -R server serve --stdio' (glob)
devel-peer-request: no-args
sending no-args command sending no-args command
devel-peer-request: hello devel-peer-request: hello
sending hello command sending hello command
devel-peer-request: between devel-peer-request: between
devel-peer-request: pairs: 81 bytes devel-peer-request: pairs: 81 bytes
sending between command sending between command
remote: 0 remote: 0
remote: 384 remote: 384
0 0
384 384
capabilities: lookup changegroupsubset branchmap pushkey known getbundle unbundlehash batch streamreqs=generaldelta,revlogv1 $USUAL_BUNDLE2_CAPS_SERVER$ unbundle=HG10GZ,HG10BZ,HG10UN capabilities: lookup changegroupsubset branchmap pushkey known getbundle unbundlehash batch streamreqs=generaldelta,revlogv1 $USUAL_BUNDLE2_CAPS_SERVER$ unbundle=HG10GZ,HG10BZ,HG10UN
1 1
$ hg --config sshpeer.mode=extra-handshake-commands --config sshpeer.handshake-mode=pre-multiple-no-args --debug debugpeer ssh://user@dummy/server $ hg --config sshpeer.mode=extra-handshake-commands --config sshpeer.handshake-mode=pre-multiple-no-args --debug debugpeer ssh://user@dummy/server
running * "*/tests/dummyssh" 'user@dummy' 'hg -R server serve --stdio' (glob) running * "*/tests/dummyssh" 'user@dummy' 'hg -R server serve --stdio' (glob)
devel-peer-request: unknown1
sending unknown1 command sending unknown1 command
devel-peer-request: unknown2
sending unknown2 command sending unknown2 command
devel-peer-request: unknown3
sending unknown3 command sending unknown3 command
devel-peer-request: hello devel-peer-request: hello
sending hello command sending hello command
devel-peer-request: between devel-peer-request: between
devel-peer-request: pairs: 81 bytes devel-peer-request: pairs: 81 bytes
sending between command sending between command
remote: 0 remote: 0
remote: 0 remote: 0