-
Notifications
You must be signed in to change notification settings - Fork 111
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
additional kex algorithms #60
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR looks good to me from a code standpoint. I haven't manually tested this, however.
rfc8332 enabled ssh-dss used for Mina in KexManager.verifySignature() rsa-sha2-256 used for libssh in KexManager.verifySignature()
Public key
rfc8332 disabled ssh-dss used for Mina im KexManager.verifySignature() ssh-rsa used for libssh in in KexManager.verifySignature() then works ok but tests are failing. @kuisathaverat I saw you had reverted this before. Did you experience something similar? Solved and how? |
Ideally, we'd like to both support RSA SHA-2 as well as deprecating or removing RSA SHA-1 as that's what OpenSSH has been recommending for several months (possibly a couple years by now) as they'll be removing support in a future release (it's currently disabled by default at least). I believe the code added here about extension negotiation may be necessary to ensure the SSH server supports RSA SHA-2 as my previous attempt at supporting this would simply send the SHA-2 signature first and then retry with the SHA-1 signature if it failed, though some SSH servers may be overly paranoid and reject the second attempt (it is allowed by the RFC after all, though it's a shitty response). |
yep, every time we add a new algorithm, those tests have a list of algorithms in order, it should be the same that is returned by the method |
@jvz I can try to add extension negotiation if we cannot live with this commit as is. @kuisathaverat how shall we proceed? I need 3 algorithms for testing of libssh server. |
@jvz I checked and it seems to be quite some changes in to get sha 2 working but I will give it a try. |
@jvz I checked code and I cannot see that ext info is used for providing info for SHA2. I can see this for HostKey verification.
So it seems SHA1 is used. |
Oops, sorry, I was referring to the SHA-2 support I had introduced in an earlier commit that we had to revert. I had tested that out with OpenSSH, though we still had regression reports (possible that OpenSSH fixed something itself in newer versions to accept that). In that code, the RSA public key is still the same regardless if it's using SHA-1 or SHA-2; the hash is only used for host key verification (while signatures are usually used for authentication). So that code snippet is correct at least, @mpet. |
@jvz so my PR is ok if I fix the test? |
From my point of view, yes. Ivan is the maintainer, though, so he'll confirm and merge this when he can. I'm on the Jenkins Security team, so I tend to review or audit cryptography and other security changes in Jenkins things, even when we're not the maintainers of the code (it is used in Jenkins after all!). |
@jvz ok. @kuisathaverat what is your opinion? |
@kuisathaverat could you make a delivery? |
I will test it this weekend, if you need a binary you can use the incremental https://repo.jenkins-ci.org/incrementals/org/jenkins-ci/trilead-ssh2/build-217-jenkins-27-rc201.14ce3b77aeaa/ |
@kuisathaverat thanks! Greatly appreciated. |
I've tested that does not break anything, but I did not test if the support for the new kex works, I have to prepare some agents with those kex only enabled. Along this week I will try to finish my tests. |
It was quicker than expected, the
This is my test environment https://github.com/kuisathaverat/jenkins-issues/tree/master/kex-algorithms |
@kuisathaverat I fixed the list issue. Could you have a look again? Also which code formatter do you use for project? |
@kuisathaverat could you check last commit. Or tell me how to test it? |
@kuisathaverat could you look into it now? |
during the week I do not have too much time, Friday I have time for it. If you want to test it you only have to checkout my test environment and make https://github.com/kuisathaverat/jenkins-issues/tree/master/kex-algorithms |
@kuisathaverat where do I set to use my PR in the testenv? |
you have to replace the trilead-api plugin (https://github.com/kuisathaverat/jenkins-issues/tree/master/kex-algorithms/jenkins/jenkins_home/plugins) with the incremental generated by jenkinsci/trilead-api-plugin#20 this file is loaded on you Jenkins instance when you create the Docker container (make clean start) If you make changes on this PR, a new incremental would be generated, you have to follow the incrementals link in the GitHub checks and take the incremental number then you have to modify the incremental version and compile again the trilead-api plugin https://github.com/jenkinsci/trilead-api-plugin/pull/20/files |
@kuisathaverat I failed to do it. I create a PR of jenkinsci/trilead-api-plugin#21 but it fails. |
The latest changes fix the issue with the
|
why would they work? (have they ever worked?) This pullrequest is to add kex algorithms and does not state that it adds any host key algorithms. |
@jvz I have tried rsa-sha2-512 for sshlib 0.9.5 as peer. This is what happens: In verifySignature() we can see that:
The PublicKey is: We try to decode the signature from peer.
RSAKeyAlgorithm class we do decodeSignature:
sig_format is 'ssh-rsa' but should be 'rsa-sha2-512' So this means the signature for peer is not rsa-sha-512 So the signature in response from peer:
contains a signature using 'ssh-rsa' How is it intended to work? |
@kuisathaverat could you make yet another check? i got it working with our product under test. |
Ha, I love this RFC because you, just like me, have been confused by what exactly the damn RFC is modifying. The key format is still called "ssh-rsa" because it's just the RSA key; the only change here is for signatures which follow the sha2 format. This same problem has popped up in other SSH libraries during development of the same feature. An ssh-rsa signature is still sha1, but an ssh-rsa host key or authentication key will remain the same for sha2. |
Relevant RFC: https://tools.ietf.org/html/rfc8332 Note that there might be a bug (or missing feature?) with some SSH implementations related to trying to use the SHA-2 variant first before falling back to trying again with SHA-1. If there's some extension negotiation info that can help reduce the unnecessary encoding attempts, that might be what you're seeing. It's been a while since I was deep in that code (the latest SSH-related changes I've worked on elsewhere (apache/mina-sshd#177) were related to curve25519-sha256 and curve448-sha512 support similar to this PR). A potentially related commit in Mina that might help you figure this out: |
@kuisathaverat any time to check again? would be nice with a README on how to test :-) |
@kuisathaverat this is a friendly bump about running tests to verify ;-) |
Thanks for handling this, Ivan! |
In my company we use trilead with protobuf-java excluded and it been working fine, (we use all of the KEX algorithms jenkinsci#60 which is the reason why tink was added and indirectly protobuf-java )
In my company we use trilead with protobuf-java excluded and it been working fine, (we use all of the KEX algorithms #60 which is the reason why tink was added and indirectly protobuf-java )
Hi
This again add support for kex algorithms:
ecdh-sha2-nistp256
ecdh-sha2-nistp384
ecdh-sha2-nistp521
curve25519-sha256
I have tested all but curve25519-sha256 vs Mina.
I tried to keep it as small as possible. Changes taken from connectbot/sshlib.
I also added some more tests from the same project.
@kuisathaverat I would appreciate if you could do a new check of code.
//mike