Bug 52033 - Allowing multiple certificates (JKS)
Allowing multiple certificates (JKS)
Status: RESOLVED FIXED
Product: JMeter
Classification: Unclassified
Component: Main
2.5.1
All Linux
: P2 enhancement (vote)
: ---
Assigned To: JMeter issues mailing list
:
: 22510 (view as bug list)
Depends on: 52871
Blocks:
  Show dependency tree
 
Reported: 2011-10-14 19:34 UTC by Julian Cesar
Modified: 2012-03-09 12:06 UTC (History)
2 users (show)



Attachments
Suggested changes (11.47 KB, patch)
2011-10-14 19:34 UTC, Julian Cesar
Details | Diff
New patch with spaces. (12.51 KB, patch)
2011-10-14 20:14 UTC, Julian Cesar
Details | Diff
Correction (598 bytes, patch)
2012-02-24 18:34 UTC, Julian Cesar
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Cesar 2011-10-14 19:34:39 UTC
Created attachment 27777 [details]
Suggested changes

We had a need to use multiple certificates for mutual authentication in web servers, from some code suggestions found on the internet we create a model that would like to suggest for the next release of JMeter.

The expected behavior is that when you configure a key store containing more than one private key, it must request a certificate for each virtual user (thread), and if the number of virtual users is greater than the certificate, he should restart the list of certificates.

Because the delay in loading the JKS and the noises caused for him in the test, we changed the code so that the JKS was initializated on JMeter startup.

We use the tag v2_5_RC3 for development. The suggestion to update the code of 4 classes is attached as a patch to SVN.
We did some tests and the load time of 7000 certificates / keys lasted average of 3 seconds.

This implementation can be used in the next release of JMeter?
Comment 1 Philippe Mouawad 2011-10-14 20:06:14 UTC
Hello,
Thanks for the patch but can you recreate it by removing all tabs that have been added ?
We generally use 4 spaces indentation (never any tabs).

Thanks
Comment 2 Julian Cesar 2011-10-14 20:14:32 UTC
Created attachment 27778 [details]
New patch with spaces.

Sorry.
Comment 3 Sebb 2011-10-15 15:42:46 UTC
Thanks for the patch; unfortunately it does not apply cleanly to the code in trunk.

Also, there are several instances of using @SuppressWarnings.
As far as possible we don't allow these.
The instance ArrayList fields in DefaultKeyStore should be local variables.
The mutable static caches in JsseSLManager are not thread-safe.

But we can fix these.

I'm more concerned about the use of the system property names javax.net.ssl.keyStoreStartIndex and javax.net.ssl.keyStoreEndIndex.

Are these standard property names?
I could not find any reference to them.
If these are invented names, then they should not use the javax namespace.
Probably should be JMeter properties.

The proposal needs a bit more consideration.
Comment 4 Julian Cesar 2011-10-17 11:19:30 UTC
I was waiting for us to reach this point, this properties is used to load a range of certificates/keys of jks, we use this when using distributed tests, we can help to write a documentation about this and we consider that this feature is very important.
About the names of properties, really dont exists in documentation, we invented that! however we can change it.

We appreciate your patience, we intend to help more with less mistakes in the future, thank you!
Comment 5 Sebb 2011-10-17 11:39:39 UTC
OK

In which case the only problem is that a system property in the javax namespace is not appropriate.

I'll see about incorporating the patch in due course.
Comment 6 Vanderson 2011-10-21 19:01:07 UTC
Sebba, we tested the proposal that appears to be stable. We are very interested in including this feature to prevent us to use an unofficial version of JMeter. There are planning on which version will the use of multiple certificates as proposed? How long could we wait?
Comment 7 Sebb 2011-10-23 02:06:37 UTC
Found a few other issues with the patch:
- the behaviour for start = end = 0 was changed
- unnecessary class fields
- unsafe caching of fields

Applied changes to SVN:

URL: http://svn.apache.org/viewvc?rev=1187840&view=rev
Log:
Bug 52033 - Allowing multiple certificates (JKS)

Modified:
   jakarta/jmeter/trunk/bin/jmeter.properties
   jakarta/jmeter/trunk/src/core/org/apache/jmeter/util/JsseSSLManager.java
   jakarta/jmeter/trunk/src/core/org/apache/jmeter/util/SSLManager.java
   jakarta/jmeter/trunk/src/core/org/apache/jmeter/util/keystore/DefaultKeyStore.java
   jakarta/jmeter/trunk/src/core/org/apache/jmeter/util/keystore/JmeterKeyStore.java
   jakarta/jmeter/trunk/xdocs/changes.xml

Please check if the updated code works for you.
Note that I changed the property names, and the meaning of the last index.
If start = end, then that is the index of the single alias to be used.

Also, at present the index range has to be chosen before starting a test.
Is there a use case for allowing the range of aliases to vary between samples?
Or more likely, using a different fixed alias for different samples?
For example, when connecting to different hosts, perhaps these will need different aliases?
Comment 8 Sebb 2011-10-25 10:50:04 UTC
Made another change to improve thread-safety in r1187876.

Please try with any build from that fix and report if the issue is solved.
Comment 9 Philippe Mouawad 2011-11-01 12:57:48 UTC
Message from Julian Cesar:
Hello Philippe,

We test and we found a problem.
In line 89 (v_certChains.add((X509Certificate[]) chain)) of DefaultKeyStore we have a class cast problem:

2011/11/01 10:19:10 ERROR - jmeter.util.SSLManager: Problem loading keystore: [Ljava.security.cert.Certificate; cannot be cast to [Ljava.security.cert.X509Certificate;

We are using the revision 1195404.
Comment 10 Julian Cesar 2011-11-01 13:02:06 UTC
Maybe we need to iterate one by one of certificates to be cast to x509. In the code originally sent we did this way.
Comment 11 Philippe Mouawad 2011-11-01 13:13:42 UTC
Would it be possible for you to test again with upcoming revision, I just
commited the fix to this:

Date: Tue Nov  1 13:11:56 2011
New Revision: 1195978

URL: http://svn.apache.org/viewvc?rev=1195978&view=rev
Log:
Bug 52033 - Allowing multiple certificates (JKS):
Fixed classcastexception

Modified:
  
jakarta/jmeter/trunk/src/core/org/apache/jmeter/util/keystore/DefaultKeyStore.java
Comment 12 Philippe Mouawad 2011-11-01 13:58:14 UTC
Message from Julian Cesar:
The problem with the class cast is solved, but 2 problems still exists.

1. Always the same certificate is used, if you use the keystore was sent you can see this;
2. Is interesting that the certificates are loaded at startup jmeter or as soon as a key store is selected, because this load takes around 3 seconds, and this can cause noise in the test and also the incomplete certificates loading can cause a malfunction of user requests.
Comment 13 Philippe Mouawad 2011-11-01 14:18:19 UTC
Hello,
You can achieve the first by setting:
- HTTPClient 3.1 or 4 as implementation
- setting https.use.cached.ssl.context=false


For the second , we will make it an option.
Regards
Philippe
Comment 14 Julian Cesar 2011-11-01 15:49:28 UTC
if you create a option to load key store on start jmeter would be perfect.
We would like a nightly builds for the final tests, when we have it?
Comment 15 Philippe Mouawad 2011-11-01 21:52:57 UTC
Date: Tue Nov  1 21:51:46 2011
New Revision: 1196306

URL: http://svn.apache.org/viewvc?rev=1196306&view=rev
Log:
Bug 52033 - Allowing multiple certificates (JKS)
Added Keystore Config component to enable configuring:
- Preload
- Start alias index
- End alias index

Added:
   jmeter/trunk/src/components/org/apache/jmeter/config/KeystoreConfig.java   (with props)
   jmeter/trunk/src/components/org/apache/jmeter/config/KeystoreConfigBeanInfo.java   (with props)
   jmeter/trunk/src/components/org/apache/jmeter/config/KeystoreConfigResources.properties   (with props)
   jmeter/trunk/xdocs/images/screenshots/keystore_config.png   (with props)
Modified:
   jmeter/trunk/src/core/org/apache/jmeter/util/SSLManager.java
   jmeter/trunk/src/core/org/apache/jmeter/util/keystore/DefaultKeyStore.java
   jmeter/trunk/src/core/org/apache/jmeter/util/keystore/JmeterKeyStore.java
   jmeter/trunk/xdocs/usermanual/component_reference.xml
Comment 16 Philippe Mouawad 2011-11-01 22:07:54 UTC
Hello Julian,
Preload was implemented through new "KKeystore Config" new config element.
AS soon as Nightly build is available you can test it.
You will need to change your test plan to add "Keystore Config" component.

Regards
Philippe
Comment 17 Julian Cesar 2011-11-03 15:56:56 UTC
I tested with 7000 certs and everything is ok!

It would be interesting if not put values ​​for the start and index end on Keystore Configuration, we try to get the https.keyStoreStartIndex and https.keyStoreEndIndex contained in the file jmeter.properties, because if it is distributed test is a necessary thing. This makes sense for you?
Comment 18 Philippe Mouawad 2011-11-03 16:33:52 UTC
Implemented as required.
I will close issue as solved, please reopen if you find any issue and in all cases, comment to say tests are OK on your side.

Thanks for your contribution on this.

Date: Thu Nov  3 16:29:32 2011
New Revision: 1197207

URL: http://svn.apache.org/viewvc?rev=1197207&view=rev
Log:
BUG_52033 :
Default to jmeter.properties if start and end not set
Handle errors in inputed start and endIndex

Modified:
   jmeter/trunk/src/components/org/apache/jmeter/config/KeystoreConfig.java
Comment 19 Philippe Mouawad 2011-11-06 23:05:39 UTC
*** Bug 22510 has been marked as a duplicate of this bug. ***
Comment 20 Julian Cesar 2012-02-24 18:34:18 UTC
Created attachment 28381 [details]
Correction
Comment 21 Julian Cesar 2012-02-24 18:34:50 UTC
Hi, I made a dumb mistake in this feature! 
We did not have detected this problem until we need to get a specific certificate.
The correction is atached in ticket.
Comment 22 Philippe Mouawad 2012-02-24 18:37:19 UTC
Can you explain the issue ?

Maybe you should create a new Bugzilla for this.

Thank you
Regards
Philippe
Comment 23 Julian Cesar 2012-02-24 19:14:35 UTC
Done! The bug 52762 was created.

Thanks