Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#8080 enhancement closed fixed (fixed)

Deprecate twisted.conch.ssh.keys.objectType and keep a local import

Reported by: Adi Roiban Owned by: Adi Roiban
Priority: normal Milestone:
Component: conch Keywords:
Cc: z3p Branch: branches/conch-keys-objectType-deprecation-8080
branch-diff, diff-cov, branch-cov, buildbot
Author: adiroiban

Description

We plan to remove pyCryto as a hard dependency

we can still keep it as a soft dependency during the deprecation phase

twisted.conch.ssh.keys.objectType should be deprecated as it accepts pyCryto objects and does not have any real use.

Change History (9)

comment:1 Changed 4 years ago by DefaultCC Plugin

Cc: z3p added

comment:2 Changed 4 years ago by Adi Roiban

Author: adiroiban
Branch: branches/conch-keys-objectType-deprecation-8080

(In [46034]) Branching to conch-keys-objectType-deprecation-8080.

comment:3 Changed 4 years ago by Adi Roiban

Scope

This deprecates twisted.conch.ssh.keys.objectType as we plan to remove the PyCryto dependecy.

We have Key.sshType() which should do the same thing.

Changes

I have deprecated objectType and update its tests.

I have replaced the objectType usage with just Key.sshKey() While its implementation is different, I hope it will get the same results..

The changed code will now raise a RuntimeError instead of the BadKeyError. Not sure if this is a blocker or not.

OpenSSHFactory.getPrivateKeys will fail in the same way (unchanged) if RuntimeError is raised.

ckeygen.generateRSAkey and ckeygen.generateDSAkey will now raise RuntimeError ... but since the call print() I assume they are only used from command line and this should not be a big problem.. as the script will still fail, even if with a different error message. There are no tests for generateRSAkey and generateDSAkey but there is a test for the private _saveKey() method... and I have extend it to also make use of the new keyTypeName argument.

I have no idea how to write a nice test for _saveKey with the defaulPath as it will need a lot of patching... I have provided a way to do manual tests.

While checking the objectType usage coverage I have updated the docstring of existing tests.

While using twistedchecker on my local system I tried to fix the low hanging errors reported by twistedchecker.

How to check

I did a first run of the full test with objectType deprecated (but not removed from Twisted) to see if the test will fail... they passed :( so deprecation warnings don't have a good coverage.

Check that changes make sense and that the exception changes are ok.

To check the changes from ckeygen script use

python ./twisted/conch/scripts/ckeygen.py -t rsa

Thanks!

Last edited 4 years ago by Adi Roiban (previous) (diff)

comment:4 Changed 4 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

Please see previous comment.

Should be ready to review.

Thanks!

comment:5 Changed 4 years ago by Glyph

Keywords: review removed
Owner: set to Adi Roiban

Thanks for working on this problem, adi. So nice to finally be paying down some tech debt in these modules.

There are unfortunately some issues with the implementation here.

  1. Several new warnings are emitted by these tests. It's seriously unfortunate that we don't have a buildbot that fails on these, but you can try adding a shell function like this:
    warnme () 
    { 
        export PYTHONWARNINGS='all::DeprecationWarning'
    }
    
    to turn on warnings so you can get trial to tell you about this stuff. When I do that, I see (for example):
    twisted.conch.test.test_keys
      ObjectTypeTests
        test_objectType_rsa ...                                                [OK]
    /Users/glyph/Projects/Twisted/twisted/conch/test/test_keys.py:144: DeprecationWarning: twisted.conch.ssh.keys.objectType was deprecated in Twisted 15.5.0
    
    Every test that does anything with a deprecated API needs to be careful to use a deprecation guard so that we don't leak more deprecation warnings into tests.

In addition to that necessary feedback, I have a few aesthetic observations as well:

  1. This adds a bunch of code, including some unfortunate code that uses PyCrypto directly, creating even more to clean up later. Could we instead perhaps construct a conch Key object and use its keyObject attribute rather than touching PyCrypto directly in more tests?
  2. Let's try to advertise deprecated-ness a bit more visibly; being redundant about it can't hurt. Start the docstring with a big, capital DEPRECATED. Hopefully pydoctor will catch up soon and make this unnecessary, but when it does, the docstring just won't show up at all, so the extra warning will be harmless.

Thanks again.

comment:6 Changed 4 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted
$ python -Werror::DeprecationWarning ./bin/trial twisted.conch.test.test_keys

did the trick and I also get a failure.

Deprecations warnings should now be handled.

I was not aware of this policy... I have create a ticket + requested a review for having a better documentation for the deprecation policy.


For the aesthetic observations.

  1. I was hoping that we can deprecated the public keys.Key.keyObject. The PyCrypto is only touched in these tests, which will be deprecated. I was hoping that keys.Key will switch to cryptography and then we might not get a PyCrypto object out of it.

They current keyObject tests do not depend on the Key.keyObject so we can deprecated/remove them as independent code.

  1. Done.

Please check latest changes. Thanks!

comment:7 in reply to:  6 Changed 4 years ago by Glyph

Keywords: review removed
Owner: set to Adi Roiban

Replying to adiroiban:

$ python -Werror::DeprecationWarning ./bin/trial twisted.conch.test.test_keys

did the trick and I also get a failure.

Deprecations warnings should now be handled.

I was not aware of this policy... I have create a ticket + requested a review for having a better documentation for the deprecation policy.

Thanks. I'm sorry it was poorly communicated. We need to just get to the point where we are at 0 warnings so it does not need to be communicated and we can just fail the tests if warnings are emitted :).

For the aesthetic observations.

  1. I was hoping that we can deprecated the public keys.Key.keyObject. The PyCrypto is only touched in these tests, which will be deprecated. I was hoping that keys.Key will switch to cryptography and then we might not get a PyCrypto object out of it.

They current keyObject tests do not depend on the Key.keyObject so we can deprecated/remove them as independent code.

Oh, absolutely. I was just thinking that we would only be using it to implement deprecated functionality anyway, but this is better, I guess; only one deprecation warning, it's not ambiguous.

  1. Done.

Thanks.

Please check latest changes. Thanks!

Looks good, merge away!

comment:8 Changed 4 years ago by Adi Roiban

Resolution: fixed
Status: newclosed

(In [46099]) Merge conch-keys-objectType-deprecation-8080: Deprecate twisted.conch.ssh.keys.objectType.

Author: adiroiban Reviewer: glyph Fixes: #8080

comment:9 Changed 4 years ago by Adi Roiban

You don't have to feel sorry for the deprecation warnings communication... just help with the review of the new docs :p

Once I get rid of pyflakes and sphinx errors, I will look at cleaning up the code for unhandled depreaction

Note: See TracTickets for help on using tickets.