remove pyasn1 from conch and everywhere else

[reaperhulk]

This PR removes pyasn1 from the dependencies of conch. However, there is one consequence: broken OpenSSH RSA keys from #3008 are no longer supported.

[adiroiban]

Many thanks, Paul for the PR.

I think this is a good start.

I don't think this can be merged as it is.

There is a bit of red tape, in that I don't see an associated ticket for this PR, but I think that is not a big problem.

---

The biggest problem is the backward compatibility support.

A deprecation warning should be first raised for anyone still using the RSA keys that are no longer supported.

This procedure is documented here https://docs.twisted.org/en/stable/development/compatibility-policy.html#procedure-for-exceptions-to-this-policy

Since these RSA keys are most probably a bug I think that this can also qualify for Gross Violation of Specifications

https://docs.twisted.org/en/stable/development/compatibility-policy.html#bug-fixes-and-gross-violation-of-specifications

---

TODO: why not just support serialization here

I think that this to-do needs a follow-up GitHub issue, not just a comment.

---

I am +1 for using the cryptography API for the serialization and deserialization of SSH keys.

I think that twisted.conch.ssh.keys should only handle load-blod/sign/verify operations.
All the OpenSSH/PuTTY/Tectia key format handling should be done outside of Twisted (it already does sign and verify).
In this way, the code can be shared with paramiko and other Python SSH implementations.

---

I think that we can also go with Gross Violation of Specifications but we should also update the release notes to mention that some RSA keys will no longer work.

I can look into updating the existing twisted.conch.ssh.Keys code to raise a deprecation warning when a "broken" SSH key is loaded. I can trigger a release with this change.

And after that, we can just merge this.

Thanks again for the PR.

I am happy to see code removed from Twisted and moved into libraries that can be shared by other projects.

[adiroiban]

At the same time, I see that the current Twisted trunk fails to load this OpenSSH key.

To implement the deprecation warning, I have moved the key into a separate test, and I can see that the tests fails if executed on top of 525da4a (this should be the current trunk)

diff --git a/src/twisted/conch/test/test_keys.py b/src/twisted/conch/test/test_keys.py
index 22bd1ac691..d1c4c5a4ca 100644
--- a/src/twisted/conch/test/test_keys.py
+++ b/src/twisted/conch/test/test_keys.py
@@ -281,10 +281,7 @@ class KeyTests(unittest.TestCase):
             ),
             keys.Key.fromString(keydata.privateRSA_openssh),
         )
-        self.assertEqual(
-            keys.Key.fromString(keydata.privateRSA_openssh_alternate),
-            keys.Key.fromString(keydata.privateRSA_openssh),
-        )
+
         self._testPublicPrivateFromString(
             keydata.publicDSA_openssh,
             keydata.privateDSA_openssh,
@@ -300,6 +297,18 @@ class KeyTests(unittest.TestCase):
                 keydata.Ed25519Data,
             )
 
+    def test_fromOpenSSH_alternate_deprecate(self):
+        """
+        Some versions of OpenSSH generate these (slightly different keys)
+        It is not any standard key format and was probably a bug in OpenSSH at
+        some point.
+
+        When loading such a key a deprecation warning is raised.
+        """
+        referenceKey = keys.Key.fromString(keydata.privateRSA_openssh)
+        alternateKey = keys.Key.fromString(keydata.privateRSA_openssh_alternate)
+        self.assertEqual(referenceKey, alternateKey)
+

===============================================================================
[ERROR]
Traceback (most recent call last):
  File "/home/adi/chevah/twisted/src/twisted/conch/test/test_keys.py", line 307, in test_fromOpenSSH_alternate
    alternateKey = keys.Key.fromString(keydata.privateRSA_openssh_alternate)
  File "/home/adi/chevah/twisted/src/twisted/conch/ssh/keys.py", line 236, in fromString
    return method(data, passphrase)
  File "/home/adi/chevah/twisted/src/twisted/conch/ssh/keys.py", line 633, in _fromString_PRIVATE_OPENSSH
    return cls._fromPrivateOpenSSH_PEM(data, passphrase)
  File "/home/adi/chevah/twisted/src/twisted/conch/ssh/keys.py", line 566, in _fromPrivateOpenSSH_PEM
    decodedKey = berDecoder.decode(keyData)[0]
  File "/home/adi/chevah/twisted/venv/lib/python3.10/site-packages/pyasn1/codec/ber/decoder.py", line 2003, in __call__
    for asn1Object in streamingDecoder:
  File "/home/adi/chevah/twisted/venv/lib/python3.10/site-packages/pyasn1/codec/ber/decoder.py", line 1918, in __iter__
    for asn1Object in self._singleItemDecoder(
  File "/home/adi/chevah/twisted/venv/lib/python3.10/site-packages/pyasn1/codec/ber/decoder.py", line 1778, in __call__
    for value in concreteDecoder.valueDecoder(
  File "/home/adi/chevah/twisted/venv/lib/python3.10/site-packages/pyasn1/codec/ber/decoder.py", line 660, in valueDecoder
    for asn1Object in self._decodeComponentsSchemaless(
  File "/home/adi/chevah/twisted/venv/lib/python3.10/site-packages/pyasn1/codec/ber/decoder.py", line 604, in _decodeComponentsSchemaless
    componentTypes.add(component.tagSet)
builtins.AttributeError: 'NoneType' object has no attribute 'tagSet'

twisted.conch.test.test_keys.KeyTests.test_fromOpenSSH_alternate
-------------------------------------------------------------------------------
Ran 170 tests in 8.888s

[adiroiban]

So... the test is green with pyasn1==0.4.8 but fails with pyasn1==0.5.0

I can see this was raised by someone upstream pyasn1/pyasn1#29

[alex]

FWIW, the twisted tests breaking on this weeks pyasn1 release was the proximate motivation to look to remove pyasn1 entirely from twisted.

…

On Sun, Apr 23, 2023 at 1:09 PM Adi Roiban ***@***.***> wrote: So... the test is green with pyasn1==0.4.8 but fails with pyasn1==0.5.0 — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

-- All that is necessary for evil to succeed is for good people to do nothing.

[adiroiban]

Thanks, Alex for the info.

I guess that a quick fix is to update Twisted deps to depend on an older pyans1.

I am trying this solution here #11845

---

I think that for Twisted Conch, it's ok to remove pyasn1 and use only cryptography

At the same time, I am happy to see that the devs from pyasn1 are considering the support for streaming.

[adiroiban]

The PR for raising a deprecation warning is here #11847

I think that it's best to make a release of Twisted that will raise a warning if these OpenSSH keys are used.

And then we can remove support for them in the following Twisted release.

[glyph]

A deprecation warning should be first raised for anyone still using the RSA keys that are no longer supported.

I disagree. Dealing with old, broken, rarely-used data formats whose compatibility is not explicitly documented is a not covered by any of the requirements here. It hews pretty closely to "gross violation of specifications", particularly when it comes to cryptographic formats, which we have to be considerably more aggressive about deprecating for security reasons.

[glyph]

I absolutely do not want to pin to older pyasn1 because that is preventing security updates to a security-critical library.

[adiroiban]

Thanks for the PR.

I left a few comments inline.

I think they are not controversial/hard to fix.

Let me know if you need help with this branch.

Happy to help get this merge.

[glyph]

Thanks for the re-review @adiroiban !

[glyph]

LGTM, I think this works as-is and the remainder can be addressed in a follow-up.

addressed the NEWSFILE change, other stuff seems too minor to block on