Opened 3 years ago

Closed 2 years ago

#8798 enhancement closed fixed (fixed)

Adding EC support to t.conch.ssh.keys

Reported by: Abhishek Choudhary Owned by: Amber Brown <hawkowl@…>
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch:
Author:

Description

This ticket is a prerequisite to adding ECDH, ECDSA support to ssh/conch.

Change History (15)

comment:1 Changed 3 years ago by Abhishek Choudhary

Keywords: review added

comment:2 Changed 3 years ago by Glyph

I'm not closing this as a duplicate of #8370 because it seems like it's more restricted in scope (just the ECDSA additions to twisted.conch.ssh.keys). But it should probably reference that ticket in some way :).

comment:3 Changed 3 years ago by Glyph

Keywords: review removed
Owner: set to Abhishek Choudhary

Thank you for taking over since the0id has been somewhat busy, and for splitting this up into a manageably-sized change. It's a big help and this is an important feature for Conch!

I've left a bunch of review feedback on the github issue; if you could address those and resubmit it would be great.

My one other concern is that this is mostly the0id's work but your commits don't merge from theirs or credit them. Could you add some kind of citation so it's clear you're cleaning up and splitting out someone else's code? My preference would be to just make this be a fork of their branch so that it will merge more cleanly but given that you've already diverged fairly significantly, it might be best just to put their name in the final commit message.

Thanks again; I'll try to address the next view as soon as I can.

comment:4 Changed 3 years ago by Glyph

Next review. It's not a real code review if there isn't a typo caught at the last second :).

comment:5 Changed 3 years ago by Abhishek Choudhary

Keywords: review added
Owner: Abhishek Choudhary deleted

Thank you for the review. I addressed your feedback from PR.

I decided to get rid of ecKeyName attribute altogether as it was only required for getting the sshType of the key which I handled using the key size. Only limitation is it supports the curves currently mentioned in the curve table which I think is okay because, https://tools.ietf.org/html/rfc5656#section-10. I hope this is acceptable. Thanks again

comment:6 Changed 3 years ago by Abhishek Choudhary

I am keeping this in the review queue even though some tests failed, hoping that it wasn't my fault as same tests failed on updating the branch in PR539.

comment:7 Changed 3 years ago by Abhishek Choudhary

I added support for other EC curves also, without using ecKeyName attribute. I hope this is good.

comment:8 Changed 3 years ago by Cory Benfield

Keywords: review removed
Owner: set to acabhishek

This is a good start! I have a few issues and concerns about the state of the patch, but they're all entirely workable: they relate mostly to the fallbacks for unexpected and unknown key types and wanting a little extra commenting and justification for choices that are not immediately transparent to me.

Note that, while I can eventually give a positive review, I cannot really justify merging this myself because I fundamentally don't understand SSH or conch that well. That means that I'm a reviewer of limited use: I can see some things that are wrong, but just because I don't see anything wrong doesn't mean that there isn't something that needs addressing. I'm happy to do some reviews, but eventually you'll need someone who understands conch better than me to do a final review.

comment:9 Changed 3 years ago by Abhishek Choudhary

Keywords: review added
Owner: acabhishek deleted

I have addressed lukasa's feedback and adding this back to the review queue. I haven't added citation indicating this is mostly theOid's work as I think it should be the final commit, so I have to wait till this patch is approved by maintainers, there is also the elif problem which is compromising coverage for code clarity, I don't know if that is acceptable. Thank You for the feedback.

Last edited 3 years ago by Abhishek Choudhary (previous) (diff)

comment:10 Changed 3 years ago by Glyph

Keywords: review removed
Owner: set to Abhishek Choudhary

Thanks again, acabhishek942.

I did another review here; please address the coverage issues and resubmit. Other than the public key test it's all nitpicks though, and if you don't have the energy to address those you can ignore them.

comment:11 Changed 3 years ago by Abhishek Choudhary

Keywords: review added
Owner: Abhishek Choudhary deleted

Thanks for the review.

I addressed your comments and also added additional test in signandverify tests for using private RSA and DSA key, even though it is not related to this ticket but helps with the project coverage. I hope this is acceptable.

comment:12 Changed 3 years ago by hawkowl

Keywords: review removed
Owner: set to acabhishek

LGTM. Added a few notes on the GitHub review for you to fix as you see fit, cc me on the PR/comment here when it's ready for merge.

comment:13 Changed 3 years ago by Abhishek Choudhary

Keywords: review added
Owner: acabhishek deleted

Thanks for the feedback.

I addressed hawkowl's comments from the PR, but decided to keep the 'pragma' statement as that code is supposed to be refactored into multiple methods as described in #8854 and https://github.com/twisted/twisted/pull/533#discussion_r82115620, without the pragma statement coverage is compromised.

comment:14 Changed 2 years ago by hawkowl

Keywords: review removed

comment:15 Changed 2 years ago by Amber Brown <hawkowl@…>

Owner: set to Amber Brown <hawkowl@…>
Resolution: fixed
Status: newclosed

In 8164d89:

Merge 8798-acabhishek942-eckeys: Add ECDSA support to t.conch.ssh.keys

Authors: the0id, acabhishek942
Reviewers: glyph, lukasa, hawkowl
Fixes: #8798

Note: See TracTickets for help on using tickets.