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

Conversation

dasm
Copy link
Contributor

@dasm dasm commented Apr 22, 2020

OpenSSL 7.6 removed support for arcfour. rsync which uses ssh is trying
to establish connection with use of arcfour. For newer installations it
is ending with an error to establish connection.

Removed hardcoded arcfour cipher from code.


This change is Reviewable

@dasm
Copy link
Contributor Author

dasm commented Apr 22, 2020

Identified the issue with outdated arcfour cipher. #11

@coveralls
Copy link

coveralls commented Apr 22, 2020

Coverage Status

Coverage decreased (-0.07%) to 58.197% when pulling 079f581 on dasm:remove_arcfour into 7870310 on pytest-dev:master.

@@ -86,7 +86,7 @@ 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?

Copy link
Member

@bubenkoff bubenkoff left a comment

Choose a reason for hiding this comment

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

please add the docs about new option, also please add a changelog entry

dasm added 4 commits April 23, 2020 09:26
OpenSSL 7.6 removed support for arcfour. rsync which uses ssh is trying
to establish connection with use of arcfour. For newer installations it
is ending with an error to establish connection.

Removed hardcoded arcfour cipher from code.
OpenSSH 7.6 is dropping support for arcfour. Many versions have already
disabled this cipher as unsafe.

Replaced arcfour with [email protected] to allow it to work.
Default option chosen for ssh cipher is aes128. However, user can
overwrite this by providing --cloud-rsync-cipher argument
@@ -85,6 +85,8 @@ 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.

@bubenkoff
Copy link
Member

bubenkoff commented Apr 23, 2020

@dasm what is your pypi username, I will add you as a maintainer so that you can make releases yourself

Copy link
Member

@bubenkoff bubenkoff left a comment

Choose a reason for hiding this comment

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

@dasm sorry, one more thing, please also add yourself to contributors

@dasm
Copy link
Contributor Author

dasm commented Apr 23, 2020

@bubenkoff my pypi is the same (just created :P). Added myself to AUTHORS too.

@bubenkoff
Copy link
Member

@dasm please don't forget to make and push git tags when making the releases

@bubenkoff
Copy link
Member

and of course, the version has to be changed

@dasm dasm merged commit 8453474 into pytest-dev:master Apr 23, 2020
@dasm dasm deleted the remove_arcfour branch April 23, 2020 17:05
@dasm
Copy link
Contributor Author

dasm commented Apr 23, 2020

OK. Will change version and push tags. Thanks @bubenkoff

@dasm
Copy link
Contributor Author

dasm commented Apr 24, 2020

@bubenkoff I tried to publish new tag on repository, but it's giving me: failed to push some refs
I believe I don't have permissions to do so. For PyPI, Manage button is greyed out too.

Am I missing something?

@bubenkoff
Copy link
Member

what's the git command you're running?
as for pypi, you don't have to manage it via the web interface, just do:

 python setup.py sdist upload

from the project folder

@dasm
Copy link
Contributor Author

dasm commented Apr 24, 2020

I successfully uploaded new version to pypi with CLI. I also pushed new tag. Forgot to create tag with git tag 4.0.0 master at first place. Doh.

Thank you @bubenkoff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants