TurboVNC / turbovnc

Main TurboVNC repository
https://TurboVNC.org
GNU General Public License v2.0
773 stars 139 forks source link

Consider adding Certificate Support to Jsch. #357

Closed Parakleta closed 1 year ago

Parakleta commented 1 year ago

Certificate versions of the public keys are being adopted through zeroauth services like smallstep, and I am now required to use this as my users no longer have secure private storage on which to store their private keys.

I have been able to enable support for this in TurboVNC by adding the following line to a custom ssh_config file:

PubkeyAcceptedAlgorithms rsa-sha2-256,rsa-sha2-256-cert-v01@openssh.com,rsa-sha2-512,rsa-sha2-512-cert-v01@openssh.com,ssh-rsa,ssh-rsa-cert-v01@openssh.com,ssh-dss,ssh-dss-cert-v01@openssh.com,ecdsa-sha2-nistp256,ecdsa-sha2-nistp256-cert-v01@openssh.com,ecdsa-sha2-nistp384,ecdsa-sha2-nistp384-cert-v01@openssh.com,ecdsa-sha2-nistp521,ecdsa-sha2-nistp521-cert-v01@openssh.com

It would be nice if these could be added explicitly to https://github.com/TurboVNC/turbovnc/blob/6bc215bccb51e7d2091c123850e9afd7bf849a5e/java/com/jcraft/jsch/JSch.java#L136 or by modifying https://github.com/TurboVNC/turbovnc/blob/6bc215bccb51e7d2091c123850e9afd7bf849a5e/java/com/jcraft/jsch/UserAuthPublicKey.java#L88 to strip off the suffix -cert-v01@openssh.com before comparing the keys (since every key is also valid as a certificate, at least in my install with OpenSSH_8.8).

dcommander commented 1 year ago

Will look into this for the next release.

dcommander commented 1 year ago

Should be implemented now, and pre-release builds with the fix will be available shortly.

dcommander commented 1 year ago

Please test and make sure I didn't screw something up.

Parakleta commented 1 year ago

Thanks so much, I’ll take it for a spin next week

Parakleta commented 1 year ago

Tested, works well. Thanks. :)

dcommander commented 1 year ago

Further information:

This appears to be the same issue as #360. Specifically, JSch doesn't support SSH certificates, but they used to work in TurboVNC 3.0 as long as the certificate was provided by an SSH agent. Certificates stopped working because of d6ae34d6f4bd29308acc9835c8eb80f5e4b2143c in TurboVNC 3.0.1, which introduced the PubkeyAcceptedAlgorithms keyword and caused JSch to reject any algorithm that wasn't specified in PubkeyAcceptedAlgorithms, including for agent-provided keys. JSch can technically use any key provided by an SSH agent, regardless of whether it explicitly supports the key's algorithm, but filtering such keys based on the value of PubkeyAcceptedAlgorithms is consistent with the behavior of OpenSSH. I consider that to be a feature, because it allows users to explicitly disable the use of certain algorithms both for explicitly specified keys and agent-provided keys.

Can you confirm that you are using an SSH agent? If not, then how are you using the certificate with the TurboVNC Viewer?

Parakleta commented 1 year ago

Yes, I am using an agent. This is the method of operation used with the OAuth based ephemeral certificates.

My request here was just based on bringing the default set in line with OpenSSH, however I didn’t realise there was a different acceptance method whether a key is through an agent or loaded directly (this might explain why another program I use which supposedly doesn’t support certificates is also working).

dcommander commented 1 year ago

Yeah, when a key is provided by an SSH agent, the agent is responsible for decrypting it, so the SSH client doesn't actually need to support the key's algorithm.

It is slightly problematic for the default value of PubkeyAcceptedAlgorithms to contain algorithms that aren't technically supported by the SSH client. However, I also understand that adding an OpenSSH config entry for every host is too much to ask. What if the TurboVNC Viewer implemented the global PubkeyAcceptedAlgorithms keyword and the +/- syntax for that keyword, so you could simply add PubkeyAcceptedAlgorithms +<whatever_certificate_type> to the top of ~/.ssh/config rather than having to specify it on a per-host basis? If that would create an undue administrative burden (such as if you would need to do that for 500 users in your organization), then I am OK with keeping things as-is. (The current solution is only "slightly" problematic, because we don't advertise any certificate algorithms other than the ones that correspond to the algorithms we already support.)

Parakleta commented 1 year ago

I'm happy to go with whichever form is the most technically correct, and I think you're right that the certs should be removed from the default since JSch cannot decrypt them itself.

Having the +key type would definitely be good since it would make the config files must simpler and future proof (which was part of my concern with brute forcing the accepted keys, potentially having to reconfigure to access higher strength keys later) and it would also be nice if you had a way to match the wildcard behaviour, although I accept that that might take a fair bit more work.

I also encountered a problem with the Windows installation of OpenSSH being too old to support the new PubkeyAcceptedAlgorithms keyword so I was using the IgnoreUnknown keyword to get around this problem, although I have since discovered the PubkeyAcceptedKeyTypes is the legacy option which should work in both.

I can manage the updating of ssh_config files with the installation of a new version without a problem so that wont be an issue.

dcommander commented 1 year ago

OK, cool. Since there is already code in the JSch form for implementing the +/- syntax, I think that's the way to go.

dcommander commented 1 year ago

fbaadc2a2eb74711a1799d5cdfef309d7a18256d has been reverted, and +/^/- syntax for the PubkeyAcceptedAlgorithms/PubkeyAcceptedKeyTypes keyword has been added, so you should now be able to add

PubkeyAcceptedAlgorithms +rsa-sha2-256-cert-v01@openssh.com,rsa-sha2-512-cert-v01@openssh.com,ssh-rsa-cert-v01@openssh.com,ssh-dss-cert-v01@openssh.com,ecdsa-sha2-nistp256-cert-v01@openssh.com,ecdsa-sha2-nistp384-cert-v01@openssh.com,ecdsa-sha2-nistp521-cert-v01@openssh.com

to the top of ~/.ssh/config to achieve the same effect that fbaadc2a2eb74711a1799d5cdfef309d7a18256d achieved (no wildcards, though.) GitHub Actions is derping for some reason, so only Windows and macOS pre-release builds are available with the changes at the moment.