Skip to content
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

Drop support for RC4. #667

Merged
merged 3 commits into from Jun 6, 2017
Merged

Drop support for RC4. #667

merged 3 commits into from Jun 6, 2017

Conversation

@alex
Copy link
Member

@alex alex commented Jan 28, 2016

It's cryptoanalytically completely 100% broken, and practical attacks have been demonstrated against it's usage in TLS.

As far as I'm aware, there's no use case for RC4 based on compatibility.

It's cryptoanalytically completely 100% broken, and practical attacks have been demonstrated against it's usage in TLS.

As far as I'm aware, there's no use case for RC4 based on compatibility.
@alex
Copy link
Member Author

@alex alex commented Jan 28, 2016

Test failure unrelated, known flakey.

@coveralls
Copy link

@coveralls coveralls commented Apr 23, 2016

Coverage Status

Coverage decreased (-0.001%) to 72.58% when pulling b4474bb on alex:remove-rc4 into a14d266 on paramiko:master.

@alex
Copy link
Member Author

@alex alex commented Apr 23, 2016

@bitprophet This is green now, nice work fixing that flakey bug!

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Apr 24, 2016

Thinking 2.0 for this too (meaning I'll merge it same time as #394) unless you can think of a great reason to put it in 1.17. Not like someone's gonna roll up to 1.17 all "oh hey this still supports ARC4, gonna use that for this new code here!".

@bitprophet bitprophet added this to the 2.0 milestone Apr 24, 2016
@bitprophet
Copy link
Member

@bitprophet bitprophet commented Apr 24, 2016

Also oh god is it a feature, bug or support? it could be all 3!! #maintainerproblems

P.S., thanks for this :) Love deleting stuff.

@alex
Copy link
Member Author

@alex alex commented Apr 24, 2016

I've abandoned the existential task of categorizing security improvements
as bugs or features :-)

On Sun, Apr 24, 2016 at 6:39 PM, Jeff Forcier notifications@github.com
wrote:

Also oh god is it a feature, bug or support? it could be all 3!!
#maintainerproblems

P.S., thanks for this :) Love deleting stuff.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#667 (comment)

"I disapprove of what you say, but I will defend to the death your right to
say it." -- Evelyn Beatrice Hall (summarizing Voltaire)
"The people's good is the highest law." -- Cicero
GPG Key fingerprint: D1B3 ADC0 E023 8CA6

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Apr 25, 2016

Honestly, that strikes me as an argument for expanding the categorizations so there's just one for 'security'. Sadly bugs are already colored red in my setups...maybe I'll bring back <blink>...(and no I'm not seriously going to block this for yak shaving my changelog library, don't worry)

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Apr 25, 2016

Feels silly to make this 2.0 and the md5 stuff 3.0, let's call this 3.0 instead.

@bitprophet bitprophet modified the milestones: 3.0, 2.0 Apr 25, 2016
@alex
Copy link
Member Author

@alex alex commented May 27, 2017

The report in #923 is correct, the RC4 code looks broken. To me that's an argument for removing it now, since it's both broken (which no one seems to mind) and insecure.

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 6, 2017

For reference, the original code adding arcfour/RC4 appears to have been this commit from 2009. Seems largely untouched since then.

Seems moot tho re: the previous note about RC4 being broken at least as of our 2.0 release. If it did work back in the 1.x line, then this just falls under the increasingly large category of "if you can't get with modern crypto, just use the hey-it-still-works-fine 1.x releases". If it didn't, then...100% meh!

@alex
Copy link
Member Author

@alex alex commented Jun 6, 2017

@bitprophet it was broken when I migrated it to cryptography: 56a4739#diff-15fec8ffdb11118d0c724574145b99b5R1562 should be .update()

@bitprophet bitprophet modified the milestones: 2.2, 3.0 Jun 6, 2017
@bitprophet bitprophet changed the base branch from master to 2.0 Jun 6, 2017
@bitprophet bitprophet merged commit 79fcbda into paramiko:2.0 Jun 6, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
bitprophet added a commit that referenced this pull request Jun 6, 2017
@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 6, 2017

Moved to 2.0+ cuz it's been broken since 2.0 and removing the feature outright is clearly the pro-est bugfix. Thanks again Alex!

@alex alex deleted the alex:remove-rc4 branch Jun 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants