You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jmeter.apache.org by GitBox <gi...@apache.org> on 2021/07/23 15:26:51 UTC
[GitHub] [jmeter] veselov opened a new pull request #669: WiP: Fixes 65461
veselov opened a new pull request #669:
URL: https://github.com/apache/jmeter/pull/669
## 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][style-guide] of this project.
- [ ] I have updated the documentation accordingly.
[style-guide]: https://wiki.apache.org/jmeter/CodeStyleGuidelines
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] pmouawad commented on a change in pull request #669: 65461: support multiple keys in the client keystore (select the matching one by keyType)
Posted by GitBox <gi...@apache.org>.
pmouawad commented on a change in pull request #669:
URL: https://github.com/apache/jmeter/pull/669#discussion_r775913241
##########
File path: src/core/src/main/java/org/apache/jmeter/util/keystore/JmeterKeyStore.java
##########
@@ -303,36 +323,60 @@ private boolean isIndexInConfiguredRange(int index) {
* @throws IllegalArgumentException if {@link #clientCertAliasVarName}
* is not empty and no key for this alias could be found
*/
- public String getAlias() {
+ public String getAlias(String [] keyTypes) {
Review comment:
As @veselov explained, it implements the ability to test client certificate authentication.
This feature is useful and needed, so maybe it requires refactoring but just removing it is not acceptable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi commented on a change in pull request #669: 65461: support multiple keys in the client keystore (select the matching one by keyType)
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #669:
URL: https://github.com/apache/jmeter/pull/669#discussion_r775909820
##########
File path: src/core/src/main/java/org/apache/jmeter/util/keystore/JmeterKeyStore.java
##########
@@ -303,36 +323,60 @@ private boolean isIndexInConfiguredRange(int index) {
* @throws IllegalArgumentException if {@link #clientCertAliasVarName}
* is not empty and no key for this alias could be found
*/
- public String getAlias() {
+ public String getAlias(String [] keyTypes) {
Review comment:
this class should be removed though. I don't think it adds value :-/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] FSchumacher commented on pull request #669: 65461: support multiple keys in the client keystore (select the matching one by keyType)
Posted by GitBox <gi...@apache.org>.
FSchumacher commented on pull request #669:
URL: https://github.com/apache/jmeter/pull/669#issuecomment-1065890168
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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi commented on pull request #669: 65461: support multiple keys in the client keystore (select the matching one by keyType)
Posted by GitBox <gi...@apache.org>.
vlsi commented on pull request #669:
URL: https://github.com/apache/jmeter/pull/669#issuecomment-1002081165
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi edited a comment on pull request #669: 65461: support multiple keys in the client keystore (select the matching one by keyType)
Posted by GitBox <gi...@apache.org>.
vlsi edited a comment on pull request #669:
URL: https://github.com/apache/jmeter/pull/669#issuecomment-1001953747
We could probably use WireMock for testing: http://wiremock.org/docs/https/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] veselov commented on pull request #669: 65461: support multiple keys in the client keystore (select the matching one by keyType)
Posted by GitBox <gi...@apache.org>.
veselov commented on pull request #669:
URL: https://github.com/apache/jmeter/pull/669#issuecomment-1073413177
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi commented on pull request #669: 65461: support multiple keys in the client keystore (select the matching one by keyType)
Posted by GitBox <gi...@apache.org>.
vlsi commented on pull request #669:
URL: https://github.com/apache/jmeter/pull/669#issuecomment-1001953747
We could probably use WireMock for testing: https://github.com/Quuxplusone/BugzillaToGithub
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] veselov commented on pull request #669: 65461: support multiple keys in the client keystore (select the matching one by keyType)
Posted by GitBox <gi...@apache.org>.
veselov commented on pull request #669:
URL: https://github.com/apache/jmeter/pull/669#issuecomment-1002052031
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).
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] pmouawad commented on a change in pull request #669: 65461: support multiple keys in the client keystore (select the matching one by keyType)
Posted by GitBox <gi...@apache.org>.
pmouawad commented on a change in pull request #669:
URL: https://github.com/apache/jmeter/pull/669#discussion_r775907910
##########
File path: src/core/src/main/java/org/apache/jmeter/util/keystore/JmeterKeyStore.java
##########
@@ -303,36 +323,60 @@ private boolean isIndexInConfiguredRange(int index) {
* @throws IllegalArgumentException if {@link #clientCertAliasVarName}
* is not empty and no key for this alias could be found
*/
- public String getAlias() {
+ public String getAlias(String [] keyTypes) {
Review comment:
I would suggest returning Optional<String> since we're touching this class
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi commented on pull request #669: 65461: support multiple keys in the client keystore (select the matching one by keyType)
Posted by GitBox <gi...@apache.org>.
vlsi commented on pull request #669:
URL: https://github.com/apache/jmeter/pull/669#issuecomment-1001949871
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] veselov edited a comment on pull request #669: 65461: support multiple keys in the client keystore (select the matching one by keyType)
Posted by GitBox <gi...@apache.org>.
veselov edited a comment on pull request #669:
URL: https://github.com/apache/jmeter/pull/669#issuecomment-1002052031
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.
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] pmouawad commented on a change in pull request #669: 65461: support multiple keys in the client keystore (select the matching one by keyType)
Posted by GitBox <gi...@apache.org>.
pmouawad commented on a change in pull request #669:
URL: https://github.com/apache/jmeter/pull/669#discussion_r775908487
##########
File path: src/core/src/main/java/org/apache/jmeter/util/keystore/JmeterKeyStore.java
##########
@@ -303,36 +323,60 @@ private boolean isIndexInConfiguredRange(int index) {
* @throws IllegalArgumentException if {@link #clientCertAliasVarName}
* is not empty and no key for this alias could be found
*/
- public String getAlias() {
+ public String getAlias(String [] keyTypes) {
+
+ for (String keyType : keyTypes) {
+
+ String alias = getAlias(keyType);
+ if (alias != null) { return alias; }
+
+ }
+
+ return null;
+
+ }
+
+ /**
+ * Get the next, only or null alias for the specified key type.
+ *
+ * @param keyType key type that the key under the alias must have
+ * @return the next, only, or null alias
+ */
+ public String getAlias(String keyType) {
Review comment:
Same here regarding use of Optional
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi edited a comment on pull request #669: 65461: support multiple keys in the client keystore (select the matching one by keyType)
Posted by GitBox <gi...@apache.org>.
vlsi edited a comment on pull request #669:
URL: https://github.com/apache/jmeter/pull/669#issuecomment-1001949871
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi commented on pull request #669: 65461: support multiple keys in the client keystore (select the matching one by keyType)
Posted by GitBox <gi...@apache.org>.
vlsi commented on pull request #669:
URL: https://github.com/apache/jmeter/pull/669#issuecomment-1001952838
@veselov , WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] veselov edited a comment on pull request #669: 65461: support multiple keys in the client keystore (select the matching one by keyType)
Posted by GitBox <gi...@apache.org>.
veselov edited a comment on pull request #669:
URL: https://github.com/apache/jmeter/pull/669#issuecomment-1002052031
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi commented on pull request #669: 65461: support multiple keys in the client keystore (select the matching one by keyType)
Posted by GitBox <gi...@apache.org>.
vlsi commented on pull request #669:
URL: https://github.com/apache/jmeter/pull/669#issuecomment-1002079864
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org