[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)

[GitBox]

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-unsubscr...@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
us...@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)

[GitBox]

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-unsubscr...@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
us...@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)

[GitBox]

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-unsubscr...@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
us...@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)

[GitBox]

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 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-unsubscr...@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: DISCUSS: Bugzilla vs JIRA vs GitHub issues

[Vladimir Sitnikov]

>That's a annoying if my understanding is correct, how do we distinguish
>reporters comments from dev/committers ones ?

Comments on GitHub would start with "${username} commented: ${comment}"

>Does it means bugzilla would still be up ? What is the point then ?

It will be read-only for historical/archival reasons.

>Do the downsides vs benefit worth the migration ?

The migration is worth doing anyway.
It is worth doing even if we skip issue migration, and just make Bugzilla
read-only,
and ask everybody to open new issues in GitHub.

Vladimir

Re: DISCUSS: Bugzilla vs JIRA vs GitHub issues

[Philippe Mouawad]

Hello,
Find my answers inline below.

Regards

On Tue, Dec 28, 2021 at 7:34 AM Vladimir Sitnikov <
sitnikov.vladi...@gmail.com> wrote:

> Unfortunately, infra says GitHub can't help us with the migration:
> https://issues.apache.org/jira/browse/INFRA-22618
>
> So what we can do is to follow
> https://github.com/Quuxplusone/BugzillaToGithub
>
> The downsides would be:
> a) All comments will be authored by the "asf-git" (or a similar bot-like
> account)
>
That's a annoying if my understanding is correct, how do we distinguish
reporters comments from dev/committers ones ?

b) Attachments won't be copied (we could hyperlink to the Bugzilla instance
> though)
>
Does it means bugzilla would still be up ? What is the point then ?

Do the downsides vs benefit worth the migration ?

>
> Vladimir
>


-- 
Cordialement
Philippe M.
Ubik-Ingenierie

[GitHub] [jmeter] vlsi commented on pull request #669: 65461: support multiple keys in the client keystore (select the matching one by keyType)

[GitBox]

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-unsubscr...@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
us...@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)

[GitBox]

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-unsubscr...@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
us...@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)

[GitBox]

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-unsubscr...@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
us...@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)

[GitBox]

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-unsubscr...@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
us...@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)

[GitBox]

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-unsubscr...@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [jmeter] vlsi closed pull request #662: Setters in ConstantThroughputTimer need to set Properties to be picked up by test during runtime

[GitBox]

vlsi closed pull request #662:
URL: https://github.com/apache/jmeter/pull/662


   


-- 
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-unsubscr...@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Jenkins build is back to normal : JMeter » JMeter-trunk #364

[Apache Jenkins Server]

See 

[GitHub] [jmeter] vlsi closed pull request #659: Clean task fix (bug 63914)

[GitBox]

vlsi closed pull request #659:
URL: https://github.com/apache/jmeter/pull/659


   


-- 
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-unsubscr...@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [jmeter] vlsi closed pull request #647: 60935: Add "unsaved changes" indicator

[GitBox]

vlsi closed pull request #647:
URL: https://github.com/apache/jmeter/pull/647


   


-- 
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-unsubscr...@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [jmeter] vlsi commented on pull request #647: 60935: Add "unsaved changes" indicator

[GitBox]

vlsi commented on pull request #647:
URL: https://github.com/apache/jmeter/pull/647#issuecomment-1001959959


   @paivanjerry , would you please create a branch that contains only the 
change relevant for "unsaved changes" indicator and create a new PR?
   
   Pull requests from `master` branches are really confusing.


-- 
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-unsubscr...@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
us...@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)

[GitBox]

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-unsubscr...@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
us...@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)

[GitBox]

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-unsubscr...@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
us...@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)

[GitBox]

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-unsubscr...@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
us...@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)

[GitBox]

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-unsubscr...@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
us...@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)

[GitBox]

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-unsubscr...@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org