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

Ecdsa sha2++ #336

Merged
merged 13 commits into from
Jul 4, 2017
Merged

Ecdsa sha2++ #336

merged 13 commits into from
Jul 4, 2017

Conversation

Igerly
Copy link

@Igerly Igerly commented Jun 25, 2017

Hey there!

I've added support for ecdsa-sha2-nistp384 and ecdsa-sha2-nistp521 in this PR. The *521 one also uncovered a problem in the way you've been asn1-encoding stuff, so I've switched that from "manual" to a more "official" way.

Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
Repository owner deleted a comment Jun 25, 2017
@hierynomus
Copy link
Owner

Hi, Thanks for the PR! :) Great work. Could you add some tests with the different keytypes, especially for the bug found in the ASN.1 encoding? THen it is complete and ready to be merged.

@Igerly
Copy link
Author

Igerly commented Jun 26, 2017

Sure, just a couple of questions :)

  • With the keytypes, do you mean the ones alike to the yesterday's failed one - computing the fingerprints?
  • I haven't been able to find any tests which would actually connect to a kind of ssh server - are there any (I was testing these against a local Bitvise server, but, perhaps, openssh or something could also be used)?

@hierynomus
Copy link
Owner

Yes, please add at least tests for the failures you found, that would prevent regressions.

There are some tests that connect to an Apache Mina SSH server, which is booted up in memory. But I would also welcome some (Dockerized) OpenSSH tests, that would be awesome.

@Igerly
Copy link
Author

Igerly commented Jul 3, 2017

I've added the unit-tests for the cases.
I've also started to look into the dockerized integration version, but will add these in another PR, if they turn out working :)

Repository owner deleted a comment Jul 3, 2017
Repository owner deleted a comment Jul 3, 2017
@hierynomus
Copy link
Owner

Looks great, thanks! I'll do some tests tomorrow and merge it afterwards.

Looking forward to your next pull request.

Sent from my Samsung SM-G950F using FastHub

Repository owner deleted a comment Jul 4, 2017
@hierynomus hierynomus merged commit 5e3a08a into hierynomus:master Jul 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants