#11793 deprecated legacy ciphers for conch #11794
Conversation
This includes * Blowfish * CAST5 * IDEA * SEED
There was a problem hiding this comment.
Many thanks for the changes and your help!
I think that before removal, we should first deprecate the SSH ciphers in Twisted Conch.
Even though there is a warning in cryptography, this is not a Twisted warning, and you might be using an older cryptography version in which the warning is not raised.
This is why I think it is required to raise a dedicated warning in Twisted.
The Twisted deprecation and removal policy is here https://docs.twisted.org/en/stable/development/compatibility-policy.html
There is the option of removing code without a prior warning, but this will need an explicit policy exception request and approval from the team.
Triggering a valid warning is not easy.
I assume that many Conch user might have blowfish enabled, but this is done via the default configuration ann not via an explicit configuration.
And we don't have a standard explicit configuration for ciphers in conch... so is hard to tell if someone is using a cipher because it is needed or because it was enabled by default.
So I guess we can just go with a generic warning.
But we need to raise a warning for Twisted, and not rely only on the warning from cryptography.
The backward policy is an important part of Twisted and the Twisted dev team aim is to create a Twisted library that is stable and that will not crash at the next release due to unknown reasons.
I hope my feedback makes sense.
If you think that my judgment is not correct, please resubmit for review.
Thanks again for the PR!
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
llouislu
changed the title
|
@adiroiban I've refactored it to a deprecation. Please review again. |
There was a problem hiding this comment.
Many thanks for the changes and the test.
I think the changes are in the right direction.
Is important to use the NEXT placeholder to denote the version.
Also, it would help not to introduce new public API.
If the public API is required, the it needs documentation.
Thanks again!
Co-authored-by: Adi Roiban <adiroiban@gmail.com>
Co-authored-by: Adi Roiban <adiroiban@gmail.com>
llouislu
force-pushed
the
deprecate-legacy-ciphers-for-conch
branch
from
32e4f75
to
0488372
Compare
llouislu
force-pushed
the
deprecate-legacy-ciphers-for-conch
branch
from
0488372
to
945b76f
Compare
|
@adiroiban Please review. |
There was a problem hiding this comment.
I am only leaving a comment, since I don't know what to suggest here.
For example, I am using the twisted code with a custom list of ciphers
Basically I have something like this.
-supportedCiphers = _getSupportedCiphers()
+supportedCiphers = _getCustomCiphers()I am not using blowfish (or 3DES or CBC), but with this change, I will still receive the deprecation warning.
And I guess that might be people using supportedCiphers as a property... so for example if they receive a connection from an IP range dedicate to legacy deviced they return a set of ciphers, while for everything else, they go with secure ciphers.
It would be nice to have better support for custom ciphers list in Twisted ... but this should be done in a separate ticket/PR.
At the same time, instead of removing a cipher, I would prefer to just have that cipher disabled by default.
For blowfish and IDEA this is not a big issue, as many embedded devices support AES-128.
For example in a separate PR twisted removed a DH KEX Group... and I had to hack it back, since an old switch was only supporting that KEX...and using SSH is still better than telnet :)
so I am ok to disable the support for something by default, but it should not be removed.
so maybe the deprecation should only be raised in SSHTransportBase.sendKexInit if supportedCiphers by that time still includes the deprecated ciphers.
But this is not a complete rejection.
I don't really mind the extra deprecation warning.
Is just that maybe this is good opportunity to improve the design and the extensibility of Twisted conch ssh.
But maybe, I am the only one with strange requirements for SSH... so we should not bother and just merge this code.
| cipherMap = { | ||
| # 3DES, ARC4 are also legacy (and ARC4 has serious security issues), |
There was a problem hiding this comment.
Why should we care here about ARC4 ?
And these days many automated SSH scanner will complain about any CBC mode...
I am not sure that we should add comment about these things here.
My suggestion is to just remove this comment.
There was a problem hiding this comment.
Thanks for the changes.
I have re-read Jean Paul's comment
It would really be much better if the warnings were emitted when someone actually used something that's deprecated. This lets them get rid of the deprecation by removing that use.
I agree with him.
So I am requesting changes.
@llouislu I am happy to help with the code for a better designed conch code.
So for this PR, we can discuss your use case, try to understand what problem it tries to solve and see how we can improve Twisted conch.
Thanks!
Fixes #11793
This PR deprecates legacy ciphers (CAST5, Blowfish) used in conch and updates documentation.