65461: support multiple keys in the client keystore (select the matching one by keyType)

[veselov]

WiP STATUS

• there don't seem to be any tests around the key manager at present, please see comment below
• I'm not sure if this needs any documentation change, I don't think so, as it's just a bug fix.

Description

Fixes support for cases when the key type request to the key manager does not match the key type of the available keys. Without this fix, a key of incorrect type may be returned to the SSL engine, resulting in key misuse and connection failure.

Motivation and Context

https://bz.apache.org/bugzilla/show_bug.cgi?id=65461

How Has This Been Tested?

The tests pass, and I tested this for my case specifically, but I don't see any unit test that exercise this code. Any pointers or suggestions on how to test this during build? The simplest would probably be to have a keystore and simulate calls to the key manager.

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)

Checklist:

• [X] My code follows the code style of this project.
• [ ] I have updated the documentation accordingly.

[vlsi]

I did try to review this, and my conclusion is as follows:

1. I would like to decline the patch, as adding more and more stuff to JMeter's SSL makes the stuff harder and harder to follow
2. There are no tests to cover the change. I agree SSL-related tests was not there, however, the code is already hard to follow
3. I'm inclined that JmeterKeyStore should be removed, and we should use the default KeyStore implementation
4. SSLManager.getInstance() is singleton, so having different keystores for different threads would be problematic
5. JMeter's "first key index", and "last key index" parameters are really obscure. I would like to just drop them.

[vlsi]

@veselov , WDYT?

[vlsi]

We could probably use WireMock for testing: http://wiremock.org/docs/https/

[veselov]

1. Without this fix, we can't use client-side client authentication testing, i.e., there is no workaround for bug 65461 (that I could find). The API contract that ServerAliasKeyManager must honor as part of being a X509KeyManager is broken without this. Note that this actually is not about supporting multiple key types in the same store, a key store with ED25519 keys won't work either; because the key manager is "probed" for keys, so if it returns an EC key for a RSA key type request (which is what happens without the fix), the entire SSL tanks.
2. I can try adding wiremock tests. The examples at the link you gave look promising and fitting.
3. JmeterKeyStore is not a key store. It can be removed, as a class, but all the logic of handling alias selection is still needed, and will have to be moved to JsseSSLManager. I'm not sure this will be any better (but see 4)
4. Ah, are you suggesting to, instead of using a singleton SSL manager, have an SSL manager (and therefore individual key stores) for each thread? Thus eliminating the need for JmeterKeyStore and all the complexities dealing with a shared key store? That sounds fine to me. IDK how the multiple key stores are to be specified, .ZIP file?
5. I certainly don't mind if they were gone. A workaround to providing indices into an existing key store is always to copy the key store, leaving only the items that somebody cares about. I imagine this feature was done anticipating running multi-node tests, where each node gets a "range" of keys from the same keystore. It's more feasible to have different keystores for that scenario anyway.

[vlsi]

It can be removed, as a class, but all the logic of handling alias selection is still needed, and will have to be moved to JsseSSLManager

I guess the only logic we need is to "try using jmeterproperty-based key first", which we could place into KeyManager. On the other hand, I guess, the default implementation of KeyManager should be capable enough to select the key that supports the needed algorithm.

[vlsi]

Ah, are you suggesting to, instead of using a singleton SSL manager

A bit of a story is that currently JMeter relies on Java's default keystore properties, and Java provides no way to configure multiple keystores to be used at the same time.

Frankly speaking, I do not have a need for multiple different keystores to be used for different thread groups, and I do not suggest implementing it.

[FSchumacher]

To get this going again.

• To merge this, tests would be needed.
• Vladimir proposes to use Java's Keystore instead. Would this be feasible without breaking backwards compatibility? Is there a good way to introduce this in small steps (with a feature flag, for example)?
• Can the API changes be made without breaking old clients? (E.g. without the removal of public methods)

[veselov]

It's most likely possible to implement this using keystores directly. This won't help with the testing, as there are still no tests around it. It can be implemented such that the new implementation is completely separate, so no APIs are touched, and then there is a switch somewhere that selects old or new implementation.