Add support for openssh host key certificates

[FliegenKLATSCH]

This is part of SSHD-660. Would be happy to get some feedback.

[lgoldstein]

Seems like solid work in general, but it lacks unit tests and most important - a real live server that we can test this code...

[lgoldstein]

Still a few minor issues remain + need to figure out the lazy-load solution

[lgoldstein]

Just a reminder - please rebase your branch on the latest master version (I pushed some changes in the last few days) so it will be easier to merge this change.

I would still love to see some jUnit test code for the following scenarios:

• "Mixed" certificate and regular keys are available - make sure the certificate availability does not interfere with the "legacy" behavior
• Only certificate keys are available
• Bad certificate code behavior - make sure that if the certificate is invalid then code indeed disconnects as required

[lgoldstein]

Seems in order - I am willing to merge it in as-is (provided you have re-based it) and wait for a separate PR with unit tests - let me know how you would like to proceed.

[FliegenKLATSCH]

• Bad certificate code behavior - make sure that if the certificate is invalid then code indeed disconnects as required

OpenSSH actually does a fallback to the plain host key, maybe we should do the same instead of aborting the connection if the certificate is invalid. Makes especially sense if the certificate is expired, you still want to be able to connect..

Certificate invalid: name is not a listed principal
debug1: No matching CA found. Retry with plain key

I am currently on the unit tests, having some issues with RSA key mismatch exception... 512 vs 256 .. need to investigate further...

[lgoldstein]

OpenSSH actually does a fallback to the plain host key, maybe we should do the same instead of aborting the connection if the certificate is invalid. Makes especially sense if the certificate is expired, you still want to be able to connect..

I can live with that - just a suggestion - you can make the behavior configurable via a property that you can retrieve from the session (default can be whatever you decide).

I am currently on the unit tests, having some issues with RSA key mismatch exception... 512 vs 256 .. need to investigate further...

Great - will wait for you to let me know when you feel the code is ready for more review and merging.

[FliegenKLATSCH]

Please note commit a8cdbde This relates to SSHD-895, if client and server negotiated e.g. rsa-sha512 the signature verification would fail with ssh-rsa retrieved from KeyUtils.getKeyType.
If all is fine now for you, should be ready to merge from my side. Many thanks for your fast reviews! I really appreciate this.

[lgoldstein]

Merged with acknowledgement and many thanks (see 47f779f06cb345c7cb706cb81f1214c37dab1fda). Please note that I added a commit to fix some minor styling issues as well as update the relevant documentation.

Please close this PR

[FliegenKLATSCH]

Thanks a lot, but it's not only server side support but also client side (I started with client side actually and had two different commits..).
And according to your latest comment in SSHD-895 we should revert the changes in ClientBuilder?
(Although I am still convinced that we should even add sha256/512 to have the broadest support and best security per default..)

[lgoldstein]

Thanks a lot, but it's not only server side support but also client side (I started with client side actually and had two different commits..).

Sorry - too late now - the important thing is that the code is in

And according to your latest comment in SSHD-895 we should revert the changes in ClientBuilder?

I have reviewed the code before merging and have reached the conclusion that I can live with the change you suggested - i.e., include the certified keys by default. I have decided though not to include the ssh-rsa-sah256 yet since it is still a rather recent feature.

[FliegenKLATSCH]

Ok, thanks. Changelog might just be misleading.

[alex-sherwin]

Do you know what is missing from this to make client certificates work?

I've been hacking away for a few nights to try and figure it out, and it feels like it nearly works if you simply remove the hard-coded check that only allows decoding the host cert variant here 47f779f#diff-4067485581e7df0fec6ea6f408772edbbcd5eb7a1d9833e5ed9130a1de9015d1R74

Then things nearly "just work", but currently I'm trying to debug the publickey auth process in https://tools.ietf.org/html/rfc4252#section-7

The mina client will send the RSA-CERT in the first SSH_MSG_USERAUTH_REQUEST, and the openssh server accepts it, but the followup SSH_MSG_USERAUTH_REQUEST that includes a signature fails on the openssh server side with

debug3: mm_answer_keyverify: publickey 0x7f2e7882b0a0 signature unverified: key type does not match
Failed publickey for user01 from 172.17.0.1 port 64500 ssh2: RSA-CERT SHA256:gBrWshYSkWIDTUkeNmUWQaCsyO+NIGVsQljoeL64+bU ID user01 (serial 0) CA RSA SHA256:6vk74tKCkS3WqaeL68mM2TMvnCyaFsqt1ac26fktSqQ

It to me looks like the mina client code it doing all the right things (I think), the instructions on https://tools.ietf.org/html/rfc4252#section-7 say that "The 'public key blob' may contain certificates" and the changes in this PR for Buffer encode the client cert into the Buffer when UserAuthPublicKey invokes buffer.putPublicKey(pubKey) where pubKey is a OpenSshCertificateImpl instance

Quick recap:

1. If you disable the hard-coded check that only decodes host certs
2. Quickly hack KeyUtils.compareKeys(PublicKey, PublicKey) so that if two OpenSshCertificate instances are provided that they are compared
3. Setup a client whose KeyPair is an instance of the clients private key and certificate-based public key (the cert-based client pub key is parsed with PublicKeyEntry.parsePublicKeyEntry(str).resolvePublicKey(null, null, null)
4. Connect to an OpenSSH server

It seems to do all the right things (I think), but something appears to be wrong with the SSH_MSG_USERAUTH_REQUEST that contains a signature, and I'm not sure where the right place is to really figure out where this mismatch is.

It's worth nothing that if I compare the verbose OpenSSH server-side output when compared with a a OpenSSH client making a successful client-based RSA-CERT connection and mina client, the output is identical right up until the error from above:

debug3: mm_answer_keyverify: publickey 0x7f2e7882b0a0 signature unverified: key type does not match
Failed publickey for user01 from 172.17.0.1 port 64500 ssh2: RSA-CERT SHA256:gBrWshYSkWIDTUkeNmUWQaCsyO+NIGVsQljoeL64+bU ID user01 (serial 0) CA RSA SHA256:6vk74tKCkS3WqaeL68mM2TMvnCyaFsqt1ac26fktSqQ

All the displayed public keys and signatures and algorithms displayed in the verbose OpenSSH server-side output are identical up to here

If you could provide any guidance on what docs to read or any insights on what the problem may be, that would be great

Thanks,

[tomaswolf]

What about verbose client-side output? Check what ssh -vvv tells you about the signature algorithm being used on the client side, and compare against the debug logging from the Apache sshd client.

I think the error "key type does not match" comes from ssh-rsa.c. sigtype should be one of ssh-rsa, rsa-sha2-256, or rsa-sha2-512 at that point. My suspicion is that the client by mistake sends "ssh-rsa-cert-v01@openssh.com" instead. If the client's debug log shows something like "send SSH_MSG_USERAUTH_REQUEST request ... type=ssh-rsa-cert-v01@openssh.com - fingerprint ..." then this is the problem.

[tomaswolf]

Looking deeper into the Apache MINA sshd code, I think the problem is in UserAuthPublicKey.appendSignature():

        bs.putByte(SshConstants.SSH_MSG_USERAUTH_REQUEST);
        bs.putString(username);
        bs.putString(service);
        bs.putString(name);
        bs.putBoolean(true);
        bs.putString(algo); // <-- This should be the key type
        bs.putPublicKey(key);

        byte[] contents = bs.getCompactData();
        byte[] sig;
        try {
            Map.Entry<String, byte[]> result = current.sign(session, algo, contents);
            String factoryName = result.getKey();
            // An RSA -cert... signature generates a ssh-rsa, rsa-sha2-256, or rsa-sha2-256 signature, so this check may not
            // make sense?
            ValidateUtils.checkState(algo.equalsIgnoreCase(factoryName),
                    "Mismatched signature type generated: requested=%s, used=%s", algo, factoryName);
            sig = result.getValue();
        } catch (Error e) {
            warn("appendSignature({})[{}][{}] failed ({}) to sign contents using {}: {}",
                    session, service, name, e.getClass().getSimpleName(), algo, e.getMessage(), e);
            throw new RuntimeSshException(e);
        }

        // Trace logging omitted here

        bs.clear();
        bs.putString(algo); // <-- But here we should have the signature algorithm name, shouldn't we?
        bs.putBytes(sig);

[alex-sherwin]

Hi @tomaswolf

Yes this is the problem, I had debugged the native OpenSSH client code and checked the raw buffer differences to come down to that same conclusion

I haven't put together a formal PR for this yet but I plan to soon, but I'm still not entirely sure what the right process is for determining the right value to put here for the algo value. I can make it work with known, contrived values (rsa-sha2-512 for rsa client certs and ssh-ed25519 for ed25519 client certs), but I haven't spent the time yet to find out the "right" way to determine this value dynamically yet

The followup, once that's determined, will be how to properly unit test it since mina sshd server-side doesn't support client cert based auth. I haven't investigated yet if it's plausible to unit test the client behavior in isolation or if the project will want complete end-to-end testing, which may require me to implement the server-side portion of this as well (I hope not, as I don't need that functionality)

[alex-sherwin]

I've researched this some more and I don't think there's any place in the current mina-sshd project that maintains a map of these signature algorithms that are utilized, at least, not aliased to the values required here.

The SignatureRSA class has an internal sshAlgorithmName that holds the in-use algo (rsa-sha2-512 / rsa-sha2-256), but the SignatureEd25519 (which is third party anyways) has no similar internal field to even carry the ssh-ed25519 value), for example

Best I can tell, it may be necessary to create a specific map of the signature values from the original type to the value required for this part of the SSH_MSG_USERAUTH_REQUEST response buf

[alex-sherwin]

I've cleaned up an implementation that works well for all currently supported OpenSSH certificate formats on my fork (diff preview: master...alex-sherwin:master)

I'm currently just using a hacky integration test that tests all valid OpenSSH client cert types against a local OpenSSH sshd daemon (which are all working from MINA ssh client now):

• rsa-sha2-256-cert-v01@openssh.com
• rsa-sha2-512-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
• ssh-ed25519-cert-v01@openssh.com

I'm not testing ssh-dss-cert-v01@openssh.com since it's not enabled in OpenSSH by default anymore.

I added function KeyUtils.getCertificateSignatureAlgorithm which is able to determine the correct value to use for the signature in the signed SSH_MSG_USERAUTH_REQUEST buffer for the supported certificate types

It also simply removes the host key type check in the OpenSSH certificate decoding, which didn't really need to be there (other then that currently only host certs were supported for any functionality). Perhaps this explicit check should be moved into the code path for the host key usage (but it appears there's already some filtering going on to discover host keys by type, so this check seems superficial)

I've love to clean up the rest of this and make a PR, but since all client unit test code I can see uses MINA-based SshServer instances to test against, I'm not sure how (in the current unit testing patterns) that this OpenSSH client certificate publickey auth could be covered in unit tests since the MINA sshd server doesn't support OpenSSH client certificates for publickey auth

Do you have any suggestion for that? Or will I need to implement the server-side portion of this in tandem for the PR?

Thanks,

[tomaswolf]

This is definitely going in the right direction, but will need more work before it could be incorporated into the main code.

As for testing: I think testing against a real OpenSSH server would be great. Testing only against an Apache MINA sshd server is better than nothing, but it's inbreeding and actually only shows that both our client and server side can talk to each other. It doesn't show that either implements the SSH protocol correctly; they might both implement their own completely different protocol. But I don't know if the containers used for the CI builds on Travis do actually have OpenSSH, nor do I know what the best way to configure, create and tear down an OpenSSH server during tests would be. We do run tests on Windows; I suspect there at least we might not have an OpenSSH server available. Perhaps @lgoldstein has an idea...

Some detail comments on your code: (can't comment directly on the preview diff)

• Comparing -cert keys should probably also compare the signatures.
• There really needs to be some mapping from the -cert signature algorithm names to the names of the underlying real signature. But I'd try to avoid simply stripping off the -cert-v01@openssh.com suffix. A explicit mapping table might be clearer.
• OpenSSH has some restrictions on which signature algorithms it accepts for which -cert key; see ssh-rsa.c, line 274. In general it must be the same as used for the certificate, except for ssh-rsa-cert-v01@openssh.com, where it may be any of ssh-rsa, rsa-sha2-256, or rsa-sha2-512. But for a rsa-sha2-256-cert-v01 key, for instance, it must be rsa-sha2-256. I suspect the determination of eligible algorithms in UserAuthPublicKey.sendAuthDataRquest() might also need to be changed. I suspect with the current "alias"-based way, we'd try for an rsa-sha2-256-cert-v01 key first rsa-sha2-512, get a failure, and only then re-try with rsa-sha2-256.

[gnodet]

This is definitely going in the right direction, but will need more work before it could be incorporated into the main code.

As for testing: I think testing against a real OpenSSH server would be great. Testing only against an Apache MINA sshd server is better than nothing, but it's inbreeding and actually only shows that both our client and server side can talk to each other. It doesn't show that either implements the SSH protocol correctly; they might both implement their own completely different protocol. But I don't know if the containers used for the CI builds on Travis do actually have OpenSSH, nor do I know what the best way to configure, create and tear down an OpenSSH server during tests would be. We do run tests on Windows; I suspect there at least we might not have an OpenSSH server available. Perhaps @lgoldstein has an idea...

Afaik, we don't have any client side test that can use an SSH server other than ours. We could work on that by using testcontainers. We're already using it for SFTP to test performance / latency, but it should be easy to setup an OpenSSH server and use it for testing.

[gnodet]

@alex-sherwin it could be a good idea to open a different issue as this PR is actually closed.

[alex-sherwin]

@gnodet @tomaswolf

Sure, I didn't mean to start a whole thread here, was originally just looking to solicit info on what may be missing from this PR's original changes

Since it sounds like you're receptive to this change in general I'm happy to put together an initial PR tonight.

I did see that testcontainers was already in use for the SFTP performance test, looked like a one-off thing to me so I wasn't sure if it was something the project wanted to adopt or not. But I totally agree that I think it makes sense to test against OpenSSH sshd so that you're testing mina client code against a different implementation.

I have a lot of experience using testcontainers so I'm more then happy to use it in some unit tests for this PR

Thanks for your input so far

[alex-sherwin]

@gnodet @tomaswolf I've opened a JIRA here https://issues.apache.org/jira/browse/SSHD-1161 for this work

A colleague & myself should hopefully have a PR ready in the next day or two

[noskojan]

Hello,
I am wondering is the support of using SSH client certificate authentication available in Mina client, please? Since the PR is already closed, but issue SSHD-1161 is still opened. Also, I am not able to find any documentation for this. Can you please help me to understand if Mina SSH client is capable of using this kind of authentication or not?
Thanks

[alex-sherwin]

@noskojan

I'm not a maintainer here, but, FYI, the features have all been merged and are covered by unit tests

However there has been no stable release since those PR's were merged, and best I can tell the current 2.9.0-SNAPSHOT version has not been published to maven central (looking at https://repository.apache.org/content/groups/snapshots/org/apache/sshd/sshd-core/)

So you'd have to build 2.9.0-SNAPSHOT yourself to use it currently

[lgoldstein]

I believe we merged these changes into the released 2.8.0 version