[SSHD-1161] OpenSSH client certificate publickey authentication

[alex-sherwin]

Fully implements https://issues.apache.org/jira/browse/SSHD-1161

This PR covers using OpenSSH client certificate publickey authentication from the MINA client code

It's unit tested against OpenSSH sshd using testcontainers

OpenSSH client certificate publickey authentication is an extension to https://datatracker.ietf.org/doc/html/rfc4252#section-7 as defined in https://cvsweb.openbsd.org/src/usr.bin/ssh/PROTOCOL.certkeys?annotate=HEAD

It slightly modifies the existing MINA certkeys code, which was originally added to only support host certificates. The most significant change to the existing code was renaming the OpenSshCertificate.getServerHostKey function to OpenSshCertificate.getCertPubKey, as getServerHostKey was a confusing name since a OpenSshCertificate instance can be either a server host cert or client cert.

The unit test covers the following client certificate types (which are all the types that current stable OpenSSH enables by default):

• ecdsa-sha2-nistp256-cert-v01@openssh.com
• ecdsa-sha2-nistp384-cert-v01@openssh.com
• ecdsa-sha2-nistp521-cert-v01@openssh.com
• ssh-ed25519-cert-v01@openssh.com
• rsa-sha2-256-cert-v01@openssh.com
• rsa-sha2-512-cert-v01@openssh.com

[tomaswolf]

Looks very promising; unfortunately I can't get the test to run locally. Also, there's a number of minor fixable problems that fail the build.

[tomaswolf]

Looks like the windows VMs used in the test builds don't have docker? We might have to make this test run conditionally on non-Windows only. Maybe with a profile that we'd enable only for the Linux builds? (If we used JUnit5, I think there'd be an @testcontainers(disabledWithoutDocker=true) annotation that could be used.)

[alex-sherwin]

Hi @tomaswolf, is there a particular way you would prefer this to be done? I'm sure I can figure something out with maven profiles and the OS conditional checks, but didn't know if you had something specific pattern in mind.

Thanks,

[tomaswolf]

No, not really. A profile was just the first thing that I thought of. An unsuspecting user who doesn't have docker on his machine should probably still be able to run a maven build locally and just have the tests that require testcontainers be skipped/not run. OTOH, we'd want to enable that profile in the build Github action for the Linux builds.

But maybe there are better ways to do this than via profiles. I also thought the Github Windows VMs did have docker:

• https://github.com/actions/virtual-environments/blob/main/images/win/Windows2016-Readme.md
• https://github.com/actions/virtual-environments/blob/main/images/win/Windows2019-Readme.md

Maybe it's a testcontainers problem.

[alex-sherwin]

The Windows VM's do have docker, but it's windows-native docker (does anyone really use that?): actions/runner-images#1143 (comment)

They don't install any variant of Docker on Windows that wraps a Linux VM, unfortunately

[tomaswolf]

This looks great. Thanks a lot! The PR now contains a lot of commits that leave intermediary stages in the repo and a lot of clean-up commits, and merging in master in the middle will make for a strange commit history when merged. I prefer to ultimately merge commits such that master builds at any stage, so I'd like to refactor this series of commits before merging:

• Rebase it onto master, then:
• One commit for the completely unrelated formatting clean-up in ReflectionUtils
• One commit by Alec for the renaming of OpenSshCertificate.getServerHostKey()
• One commit by you with Alec mentioned in the commit message for all the main code changes
• One commit by you for the test and the testcontainers setup

@alex-sherwin , would it be OK with you if I did such a refactoring of the commits?

[alex-sherwin]

@tomaswolf If possible could you make the commit w/ the main code changes by Alec and mention me instead?

He did stay up pretty late to sort out the final implementation :)

Other then that it sounds good to me

[tomaswolf]

If possible could you make the commit w/ the main code changes by Alec and mention me instead?

He did stay up pretty late to sort out the final implementation :)

Of course. Want me to force-push to your feature branch to update this PR so you can take a look before it goes in?

[alex-sherwin]

That's good with me

[tomaswolf]

Done. Take a look and if OK with you, I'd merge like that.

[FliegenKLATSCH]

Nice work, two comments from my side:

• If we have the SIGNATURE_ALGORITHM_MAP now, we should remove the mapping I introduced in the RSA classes, it's just duplicated..
• We should still somewhere validate the type of the certificate such that a host certificate cannot be used for user authentication and vice versa.

[alex-sherwin]

@tomaswolf The rebase looks good to me, I'm happy with the PR as-is

However I'm also happy to make the changes suggested by @FliegenKLATSCH, or, start a separate PR for that?

[tomaswolf]

Good points. I suggest we do both in follow-up changes.

• Validation of the certificate type would also have to happen in the server-side code (client certificate presented in pubkey auth, and don't consider and log host keys that are client certificates?) and in client-side code (host certificate in presented as host key, and in pubkey auth skip and log certificates that are not client certificates?), and need additional tests for these cases.
• The clean-up for determining the signature algorithm name could be done separately anyway. Needs a little thought, too. I like FliegenKLATSCH's approach with the method on Signature, but unfortunately the logic in UserAuthPublicKey is a bit different and we don't have direct access to the Signatureobject there.

However, I'll make one more change in ClientOpenSSHCertificatesTest: derive it from BaseTestSupport and then use setupTestClient() instead of SshClient.setUpDefaultClient(). That way, the test will not read the real ~/.ssh/config.

[tomaswolf]

The build failures are unrelated. There are unstable tests. Going to merge this as is.