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

Consider switching to the JSch fork at mwiede/jsch #323

Closed
e6c31d opened this issue Apr 6, 2022 · 10 comments
Closed

Consider switching to the JSch fork at mwiede/jsch #323

e6c31d opened this issue Apr 6, 2022 · 10 comments

Comments

@e6c31d
Copy link

e6c31d commented Apr 6, 2022

JSch hasn't had a release since 2018, and it's missing many features (such as ed25519 keys).

There's a fork at https://github.com/mwiede/jsch and the changelog is quite impressive. Have the TurboVNC project considered it?

@dcommander
Copy link
Member

Yes, I noticed that. I will consider switching in the next major release.

@dcommander
Copy link
Member

I will also look into merging some of the less disruptive fixes into TurboVNC 3.0.x. For instance, it would be nice to have support for the OpenSSH private key format. Otherwise, generating a new SSH private key and then attempting to use it directly with the TurboVNC Viewer (with the -sshkeyfile command-line argument or the IdentityFile OpenSSH config parameter, as opposed to through an SSH agent) causes JSch to throw an "invalid privatekey" error unless you convert the private key to PEM format (e.g. by invoking ssh-keygen -p -N "" -m pem -f {private-key-file}.) Support for the newer crypto algorithms isn't as important at the moment, but it will eventually become more important.

@dcommander
Copy link
Member

dcommander commented May 19, 2022

The patch that allows JSch to support OpenSSH v1 private keys has been merged into the main branch (which will eventually become TurboVNC 3.0.1.)

@dcommander
Copy link
Member

Support for rsa-sha2-256 and rsa-sha2-512 has been merged as well. Those were the two most pressing issues for the TurboVNC 3.0.x release series, since they made our JSch implementation incompatible with recent OpenSSH releases.

@samh
Copy link

samh commented Feb 13, 2023

It seems like perhaps you used a different version of JSch in the v3.0 release? I recently updated to 3.0.2 and was surprised when my ed25519 key no longer worked:

JSch: ssh-ed25519 cannot be used as public key type for identity <name>

I see you said in #331 that "TurboVNC Viewer doesn’t support ED25519 keys" so I was surprised when I retested 3.0 and it worked 😄 (I reinstalled v3.0 to verify that it does indeed work, v3.0.1 and v3.0.2 and the latest prerelease show that error, given the -loglevel 110 option). Anyway, it's no big inconvenience, I generated a new RSA key for now, just wanted to mention it in case somebody else comes across it, as I was confused for a while as to why I had to start typing my password.

Thanks for your work on the SSH support, it's been very nice to use (the authentication and the session manager)!

@dcommander
Copy link
Member

@samh See #360

@dcommander
Copy link
Member

I am in the process of implementing a number of fixes to our SSH client based on an extensive regression test that I developed for it. In the context of fixing those issues, I have had an opportunity to examine the JSch fork closely, and my impression at the moment is that it still has a lot of quirks-- differences in behavior relative to OpenSSH that I have had to address, with some difficulty, in our own implementation. The JSch fork also relies on JNA for its SSH agent connectors, which is a non-starter for TurboVNC. Of the two features that I have ported from the JSch fork so far (OpenSSH v1 private key support and rsa-sha2-256/rsa-sha2-512 signature support), the latter of those features caused TurboVNC to regress. (See #361.) In order to adopt their code base, I would have to spend a great deal of time submitting upstream pull requests that fix and test the various quirks, as well as figuring out how to integrate as much of their code as possible without integrating the parts of it that would break TurboVNC. Honestly, it would be easier to just port specific features from their code base into ours based on user demand. That would also give me an opportunity to closely examine and regression test each new feature that I adopt.

Thus, please list the features that you would most like to see TurboVNC adopt from the JSch fork.

@dcommander
Copy link
Member

3d28dd1 adds explicit support for Ed25519 keys. The 3.2 Evolving builds (https://turbovnc.org/DeveloperInfo/PreReleases) now include that feature.

@dcommander
Copy link
Member

I can't do a wholesale merge from Matthias' JSch fork, because our fork contains some TurboVNC-specific fixes (mainly to address behavior differences vs. OpenSSH) and because Matthias' JSch fork is managed in a way that would make it difficult or impossible to integrate with our project management and quality control processes. IOW, I could merge a snapshot of it and spend many unpaid hours bringing that code base in line with the TurboVNC code base, but since I can't afford to do that more than once (I actually can't even afford to do it once), our JSch fork would quickly diverge from Matthias' unless I spent many more unpaid hours continuously participating in the development of his fork. This is a fundamental limitation of maintaining an enterprise-quality project such as TurboVNC with only one developer and an extremely limited R&D budget. If I can't rely on downstream projects to maintain enterprise levels of quality, then I must stabilize a snapshot of the downstream code base and avoid modifying it unless absolutely necessary. Thus, the end result would not really be switching to his fork. It would simply be merging a snapshot of his fork. In the long term, I would prefer to move away from JSch altogether and use libssh2, perhaps in concert with #419 or even simply invoked from JNI. At this point, however, I have integrated all of the features from Matthias' JSch fork that are necessary to address deficiencies that users have highlighted in our JSch fork. Thus, I am closing this issue. If there are any other specific features that people feel I should integrate from Matthias' JSch fork, then please file separate issues for those.

dcommander added a commit that referenced this issue Feb 4, 2025
... with the following exceptions:

- Do not include Bouncy Castle algorithm implementations. (Require Java
  16 instead.)  This means that our JSch fork cannot currently support
  the [email protected] cipher, since the non-Bouncy-Castle
  implementation of that algorithm from JSch v0.1.71 and prior is not
  compatible with the current OpenSSH implementation.

- Restore formatted comments nuked by the Maven formatter.

- Do not include unused/unneeded code.

- Restore public access to *.jzlib.JZlib and *.jzlib.ZStream.
  (NOTE: We should maybe revisit eliminating JZlib and using
  java.util.zip for everything, since we don't actually use SSH
  compression.)

- Adapt the server-sig-algs implementation in mwiede/jsch@c17147c8 to
  better emulate the behavior of OpenSSH.  (More specifically, OpenSSH
  only applies server-sig-algs to RSA keys:
  https://github.com/openssh/openssh-portable/blob/826483d51a9fee60703298bbf839d9ce37943474/sshconnect2.c#L1163-L1169.)

  Take the example of an SSH agent that offers 7 keys.  The 7th key is
  the correct key to authenticate with a server, but that key uses a
  non-RSA algorithm (e.g. ssh-ed25519) that the server does not
  advertise in server-sig-algs.  With MaxAuthTries=6, OpenSSH will fail
  to authenticate with that configuration by default, but it will
  succeed if the correct key is explicitly specified with the ssh -i
  option or the IdentityFile configuration keyword (because explicitly
  specifying the key promotes it to the head of the list.)

  JSch v0.1.66+ performs an initial authentication pass with only the
  algorithms advertised in server-sig-algs, then it performs a second
  pass with the other client-supported algorithms.  With the
  aforementioned configuration, that behavior causes MaxAuthTries to be
  exceeded on the first pass (a fatal error), and the second pass never
  happens.  Our implementation instead populates the existing
  Session.supportedRSAMethods list from the server-sig-algs message, if
  the server sent that message and the jsch.enable_server_sig_algs
  system property is enabled.  This ensures that the client will not try
  any RSA algorithms except for those advertised in server-sig-algs.
  Note, however, that some OpenSSH server implementations still
  advertise ssh-rsa in server-sig-algs even if the algorithm is disabled
  in sshd_config.

Functional and logging code from the following TurboVNC commits has been
retained or adapted:

095c380
fb36f3b
053e754
dda0283
0a4aeb6
b632a9c
6838846
4a40896
273bfde
fd34df2
ed50650
58986b7
dc2a88f
d654a91
674e98c

Completes #323
@dcommander
Copy link
Member

dcommander commented Feb 4, 2025

d2da5fe fully integrates v0.2.23 of the JSch fork. I know I said I didn't want to do that, but the lack of support for modern cipher, KEX, and MAC algorithms was causing our implementation to fall behind security best practices. A wholesale upgrade was a major effort, but it would have been more difficult to continue cherry picking the new commits onto an old code base. I had to go through line-by-line and figure out which of our functional changes were still necessary (almost all of them, as it turns out.) Fortunately I have two regression test suites for the built-in SSH client that cover all of our functional changes to JSch, so moving forward, it should be a lot easier to keep pace with the upstream code base.

dcommander added a commit that referenced this issue Feb 4, 2025
... with the following exceptions:

- Do not include Bouncy Castle algorithm implementations. (Require Java
  16 instead.)  This means that our JSch fork cannot currently support
  the [email protected] cipher, since the non-Bouncy-Castle
  implementation of that algorithm from JSch v0.1.71 and prior is not
  compatible with the current OpenSSH implementation.

- Restore formatted comments nuked by the Maven formatter.

- Do not include unused/unneeded code.

- Restore public access to *.jzlib.JZlib and *.jzlib.ZStream.
  (NOTE: We should maybe revisit eliminating JZlib and using
  java.util.zip for everything, since we don't actually use SSH
  compression.)

- Adapt the server-sig-algs implementation in mwiede/jsch@c17147c8 to
  better emulate the behavior of OpenSSH.  (More specifically, OpenSSH
  only applies server-sig-algs to RSA keys:
  https://github.com/openssh/openssh-portable/blob/826483d51a9fee60703298bbf839d9ce37943474/sshconnect2.c#L1163-L1169.)

  Take the example of an SSH agent that offers 7 keys.  The 7th key is
  the correct key to authenticate with a server, but that key uses a
  non-RSA algorithm (e.g. ssh-ed25519) that the server does not
  advertise in server-sig-algs.  With MaxAuthTries=6, OpenSSH will fail
  to authenticate with that configuration by default, but it will
  succeed if the correct key is explicitly specified with the ssh -i
  option or the IdentityFile configuration keyword (because explicitly
  specifying the key promotes it to the head of the list.)

  JSch v0.1.66+ performs an initial authentication pass with only the
  algorithms advertised in server-sig-algs, then it performs a second
  pass with the other client-supported algorithms.  With the
  aforementioned configuration, that behavior causes MaxAuthTries to be
  exceeded on the first pass (a fatal error), and the second pass never
  happens.  Our implementation instead populates the existing
  Session.supportedRSAMethods list from the server-sig-algs message, if
  the server sent that message and the jsch.enable_server_sig_algs
  system property is enabled.  This ensures that the client will not try
  any RSA algorithms except for those advertised in server-sig-algs.
  Note, however, that some OpenSSH server implementations still
  advertise ssh-rsa in server-sig-algs even if the algorithm is disabled
  in sshd_config.

Functional and logging code from the following TurboVNC commits has been
retained or adapted:

095c380
fb36f3b
053e754
dda0283
0a4aeb6
b632a9c
6838846
4a40896
273bfde
fd34df2
ed50650
58986b7
dc2a88f
d654a91
674e98c

Completes #323
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants