Skip to content

Dropped outdated arcfour cipher #12

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Apr 23, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS.rst
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ Authors
These people have contributed to `pytest-cloud`, in alphabetical order:

* `Andreas Pelme <[email protected]>`_
* `Dariusz Smigiel <[email protected]>`_
* `Loic Dachary <[email protected]>`_
* `Michael Overmeyer <[email protected]>`_
* `Orthographic Pedant <[email protected]>`_
5 changes: 5 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
Changelog
=========

3.1.0
-----

- Add config option to select cipher for ssh connection (dasm)

3.0.1
-----

Expand Down
4 changes: 4 additions & 0 deletions README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,10 @@ Command-line options
Optional process count limit for `rsync` processes. By default there's no limit so rsyncing will be in parallel
for all test nodes.

* `--cloud-rsync-cipher`
Optional ssh cipher selection for `rsync` processes. [email protected] by default.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add a reason for choosing the non-default cipher

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added more verbose description.

Default cipher is chosen to have the least possible network overhead. Network overhead is system, compilation
and CPU architecture dependent, however chosen cipher is showing good results in majority of use cases.

Ini file options
----------------
Expand Down
8 changes: 7 additions & 1 deletion pytest_cloud/plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,10 @@ def pytest_addoption(parser):
"--cloud-rsync-bandwidth-limit",
help="maximum number of processes per test node", type='int', action="store",
dest='cloud_rsync_bandwidth_limit', metavar="NUMBER", default=10000)
parser.addoption(
"--cloud-rsync-cipher",
help="cipher for ssh connection used by rsync", type=str,
dest="cloud_rsync_cipher", metavar="STRING", default="[email protected]")
parser.addini(
'cloud_develop_eggs', 'list of python package paths to install in develop mode on the remote side',
type="pathlist")
Expand Down Expand Up @@ -216,7 +220,7 @@ def get_develop_eggs(root_dir, config):

def get_nodes_specs(
nodes, python=None, chdir=None, virtualenv_path=None, mem_per_process=None,
max_processes=None, rsync_max_processes=None, rsync_bandwidth_limit=None, config=None):
max_processes=None, rsync_max_processes=None, rsync_bandwidth_limit=None, rsync_cipher=None, config=None):
"""Get nodes specs.

Get list of node names, connect to each of them, get the system information, produce the list of node specs out of
Expand Down Expand Up @@ -262,6 +266,7 @@ def get_nodes_specs(
jobs=rsync_max_processes or len(nodes),
bwlimit=rsync_bandwidth_limit,
bandwidth_limit=rsync_bandwidth_limit,
ssh_cipher=rsync_cipher,
**n_m.rsyncoptions)
print('Detecting connectable test nodes...')
for node in nodes:
Expand Down Expand Up @@ -326,6 +331,7 @@ def check_options(config):
rsync_bandwidth_limit=config.option.cloud_rsync_bandwidth_limit,
max_processes=config.option.cloud_max_processes,
mem_per_process=mem_per_process,
rsync_cipher=config.option.cloud_rsync_cipher,
config=config)
if node_specs:
print('Scheduling with {0} parallel test sessions'.format(len(node_specs)))
Expand Down
6 changes: 4 additions & 2 deletions pytest_cloud/rsync.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class RSync(object):
# pylint: disable=R0913,W0613
def __init__(
self, sourcedir, targetdir, verbose=False, ignores=None, includes=None, jobs=None, debug=False,
bwlimit=None, **kwargs):
bwlimit=None, ssh_cipher=None, **kwargs):
"""Initialize new RSync instance."""
self.sourcedir = str(sourcedir)
self.targetdir = str(targetdir)
Expand All @@ -44,6 +44,7 @@ def __init__(
self.targets = set()
self.jobs = jobs
self.bwlimit = bwlimit
self.ssh_cipher = ssh_cipher

def get_ignores(self):
"""Get ignores."""
Expand Down Expand Up @@ -86,10 +87,11 @@ def send(self, raises=True):
'--inplace '
'--delete-excluded '
'--delete '
'-e \"ssh -T -c arcfour -o Compression=no -x\" '
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as I remember, this cipher was passed to have the least possible network overhead, so what is the next another cipher we can pass from the supported ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, that makes sense. Let me look around to see which cipher should replace arcfour.

Copy link
Contributor Author

@dasm dasm Apr 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on this old post: https://blog.famzah.net/2010/06/11/openssh-ciphers-performance-benchmark/ indeed the best was arcfour. But later, author updated his benchmark https://blog.famzah.net/2015/06/26/openssh-ciphers-performance-benchmark-update-2015/ and found that the fastest is [email protected].

According to analysis, everything depends on type of platform. AES instruction set, introduced by Intel in 2010 and AMD in mid-2013 is significantly improving speeds of operations.

Do you think we should force ssh to use this cipher? Would it make sense to leave decision about cipher up to ssh?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's use [email protected] then
the point of pytest-cloud that it's an opinionated approach, so the less to configure for the user is better

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced with [email protected]. Thanks

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't this option be just confuguration with sane default?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'll try making it config option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bubenkoff @spinus is it looking better now?

'-e \"ssh -T -c {ssh_cipher} -o Compression=no -x\" '
'. {{}}:{chdir}'.format(
verbose='v' if self.verbose else '',
bwlimit='--bwlimit={0} '.format(self.bwlimit) if self.bwlimit else '',
ssh_cipher=self.ssh_cipher,
chdir=self.targetdir,
ignores=ignores_path,
includes=includes_path,
Expand Down