Bug 58807 - Reset SSL State on Thread Group iteration only (was https.use.cached.ssl.context=false is broken)
Summary: Reset SSL State on Thread Group iteration only (was https.use.cached.ssl.cont...
Status: RESOLVED FIXED
Alias: None
Product: JMeter
Classification: Unclassified
Component: HTTP (show other bugs)
Version: 2.11
Hardware: All All
: P2 major (vote)
Target Milestone: JMETER_5.0
Assignee: JMeter issues mailing list
URL:
Keywords: FixedInTrunk, PatchAvailable
: 62734 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-01-05 20:56 UTC by Rainer Jung
Modified: 2018-09-18 21:34 UTC (History)
2 users (show)



Attachments
Fixing SSLContext/HTTPClient/Connection reuse (8.34 KB, patch)
2016-01-06 02:36 UTC, Rainer Jung
Details | Diff
Amended Patch (11.00 KB, patch)
2018-02-24 21:46 UTC, Philippe Mouawad
Details | Diff
Test plan with different cases (52.57 KB, application/xml)
2018-02-24 21:54 UTC, Philippe Mouawad
Details
NodeJS Project for client certificate authentication (45.79 KB, application/octet-stream)
2018-02-24 21:56 UTC, Philippe Mouawad
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Rainer Jung 2016-01-05 20:56:24 UTC
All of the following assumes you have set https.use.cached.ssl.context=false and are doing HTTPS tests using HC3 or HC4. The property is supposed the close/reset SSLContext, HTTPS connections and similar per thread at the beginning of each test iteration.

Interface TestIterationListener has a method testIterationStart() that was used in r1172147 to implement this (feature request BZ 51380). Originally the implemention of the method in HTTPSamplerBase called sslMgr.resetContext() and then notifySSLContextWasReset() which in turn immediately closed all thread local HC3 and HC4 connections using closeThreadLocalConnections() in the respective implementations.

Unfortunately there was a negative side effect, namely closing of http connections as well (BZ 55023). Since all the info needed to decide which protocol was used etc. would only be available when a sampler actually executes a request, the implementation was switched to delay the close/reset of the objects in the samplers until the next sample of the respective sampler executes. The TestIterationListener would only set a flag "resetSSLContext" in the sampler and the sampler would reset its flag once it has closed/reset the objects during the next execution. The flag is per sampler instance.

Now here's the problem: multiple samplers can and often will share the HTTP connections and SSLContext etc. So in a test plan with e.g. two HC4 samplers s1 and s2 both requesting some URI from the same HTTPS host, after the iteration starts s1 would close/reset, then create new objects and run its sample, next s2 would close/reset again, again create new objects and run its sample.

More or less this means that https.use.cached.ssl.context=false also disables reusing SSLContext, connection Keep-Alive etc. between samples in the same iteration.

As far as I can see, resetting the SSLContext via JsseSSLManager should be done as one single call at the start of an iteration. It is only needed

The other part of the close/reset, namely cleaning up the httpClient and its connections etc. is mor etricky, because there is a whole map of HTTPClient instances. Currently each sampler cares about "his" HTTPClient instance, even if that one is shared with other samplers. I think it would be better to add a static method to HTTPHC4Impl (and HC3) which would do the reset/close for all HTTPClient instances in the (thread-local) Map HTTPCLIENTS_CACHE_PER_THREAD_AND_HTTPCLIENTKEY.

Both parts could then be controlled via static flags instead of instance flags. One flag would be in HTTPHCAbstractImpl and controls reset of the SSLContext, one flag would be in HTTPHC4Impl and HTTPHC3Impl and controls the HTTPClient resets.

I'll try that and see whether it makes sense. If it works, one should also be able to support config for SSLContext reset independent of HTTPClient and Connection reset (this feature then also in the HTTP case, not only for HTTPS).
Comment 1 Rainer Jung 2016-01-05 20:58:39 UTC
> Both parts could then be controlled via static flags instead of instance
> flags. One flag would be in HTTPHCAbstractImpl and controls reset of the
> SSLContext, one flag would be in HTTPHC4Impl and HTTPHC3Impl and controls
> the HTTPClient resets.

Correction: static -> static ThreadLocal
Comment 2 Rainer Jung 2016-01-06 02:36:48 UTC
Created attachment 33410 [details]
Fixing SSLContext/HTTPClient/Connection reuse

The patch fixes reuse/reset between test iterations (on the same thread) of SSLContext, HTTPClient and HTTP connections for HC4 (and likely HC3).

For experiments the reuse of the three objects can be controlled using three different system properties

- reuse.http.connections
- reuse.http.client
- https.use.cached.ssl.context

"true" means "reuse" in all three cases, "false" means "reset/close" (do not reuse).

Resetting the HTTPClient results in also resetting HTTP Connections, but not vice versa.

It is not yet clear to me if we need to support both cases, resetting connection plus HTTPClient and also resetting Connections but reusing HTTPClient, or if one of the twi would suffice.

Feedback welcome.
Comment 3 Sebb 2016-01-06 12:07:03 UTC
(In reply to Rainer Jung from comment #2)
> For experiments the reuse of the three objects can be controlled using three
> different system properties

They seem to be JMeter properties, which is better.
Comment 4 Rainer Jung 2016-01-06 12:34:33 UTC
Yes, sorry, JMeter properties.

I think we don't need/want all three of them (individual control over connection reuse, HTTPClient reuse and SSLContext reuse). After some experimenting IMHO the useful cases are:

- full reuse of SSLContext, HTTPClient and existing connections
- no reuse of SSLContext, HTTPClient and existing connections

The default should be full reuse like it is today.

The question is though, whether the name "https.use.cached.ssl.context" for the property is good. Once could use a uniform switch to determine reuse in the HTTP case as well, something like reset.http.objects.before.iteration. Default would be "false" (no reset = reuse) and to start each iteration with fresh objects you would switch to "true".

The old flag could be deprecated. If a user would use both, the old and the new, we could warn during startup and the new flag would win.
Comment 5 Sebb 2016-01-06 14:07:17 UTC
(In reply to Rainer Jung from comment #4)
> 
> The question is though, whether the name "https.use.cached.ssl.context" for
> the property is good. 

Seems OK to me; the only problem is it does not fully describe what is re-used.
But that could be fixed by better docn in the jmeter.properties file.

> Once could use a uniform switch to determine reuse in
> the HTTP case as well, something like reset.http.objects.before.iteration.

Why do we need the reuse flag for http?
Comment 6 Rainer Jung 2016-01-06 14:31:03 UTC
As far as I understand the situation reusing (the default) means e.g. connections are kept open and are reused via HTTP Keep-Alive during the next iteration. I do see this behavior using https, I expect it to happen for http too. That's of course efficient but might not be what you want. A new iteration typically is a new user who would start with new connections.

Of course modelling the connection behavior in an exact way is beyond what we support, but the decision to start a new iteration with new connections could be interesting for some (approximate modeling). It is somehow analogous to modeling the handshake rate by resetting the SSLContext, although a new connection is of cours less disruptive than a new ssl handshake. All in all not a critical feature.

In addition I think the optional behavior "reset instead of reuse" for http and https is a good safety net against problems that might creep in due to reuse of objects that might have more context than we expect (wrong basic auth, wrong cookie, wrong client cert, wrong SNI etc.). Of course most of these things do work now, but reusing the connections and the objects from the HC libs as well as the SSLContext might introduce future bugs of the sort "next iteration uses context data from the previous one" which could then be worked around quickly by disabling the reuse. In general since the risks are quite vague so I'd stay on the performance side here for the default value (reuse). But probably based on my incomplete understanding of the lifecycles of HC objects, I'd feel better if we had such a "reset between iterations" switch.
Comment 7 Sebb 2016-01-06 17:19:35 UTC
(In reply to Rainer Jung from comment #6)
> As far as I understand the situation reusing (the default) means e.g.
> connections are kept open and are reused via HTTP Keep-Alive during the next
> iteration. I do see this behavior using https, I expect it to happen for
> http too. That's of course efficient but might not be what you want. A new
> iteration typically is a new user who would start with new connections.
>
> Of course modelling the connection behavior in an exact way is beyond what
> we support, but the decision to start a new iteration with new connections
> could be interesting for some (approximate modeling). It is somehow
> analogous to modeling the handshake rate by resetting the SSLContext,
> although a new connection is of cours less disruptive than a new ssl
> handshake. All in all not a critical feature.

Surelt that's already catered for by the "Use Keepalive" checkbox?
If not checked, JMeter sends Connection: close.
 
> In addition I think the optional behavior "reset instead of reuse" for http
> and https is a good safety net against problems that might creep in due to
> reuse of objects that might have more context than we expect (wrong basic
> auth, wrong cookie, wrong client cert, wrong SNI etc.). Of course most of
> these things do work now, but reusing the connections and the objects from
> the HC libs as well as the SSLContext might introduce future bugs of the
> sort "next iteration uses context data from the previous one" which could
> then be worked around quickly by disabling the reuse. In general since the
> risks are quite vague so I'd stay on the performance side here for the
> default value (reuse). But probably based on my incomplete understanding of
> the lifecycles of HC objects, I'd feel better if we had such a "reset
> between iterations" switch.

I think we already do.
Comment 8 Rainer Jung 2016-01-06 19:10:37 UTC
(In reply to Sebb from comment #7)
> (In reply to Rainer Jung from comment #6)
> > As far as I understand the situation reusing (the default) means e.g.
> > connections are kept open and are reused via HTTP Keep-Alive during the next
> > iteration. I do see this behavior using https, I expect it to happen for
> > http too. That's of course efficient but might not be what you want. A new
> > iteration typically is a new user who would start with new connections.
> >
> > Of course modelling the connection behavior in an exact way is beyond what
> > we support, but the decision to start a new iteration with new connections
> > could be interesting for some (approximate modeling). It is somehow
> > analogous to modeling the handshake rate by resetting the SSLContext,
> > although a new connection is of cours less disruptive than a new ssl
> > handshake. All in all not a critical feature.
> 
> Surelt that's already catered for by the "Use Keepalive" checkbox?
> If not checked, JMeter sends Connection: close.

I didn't mean to completely disable Keep-Alive but to not alow Keep-Alive from one itertaion to the next. The "Use Keepalive" checkbox would disable Keep-Alive completely.

> > In addition I think the optional behavior "reset instead of reuse" for http
> > and https is a good safety net against problems that might creep in due to
> > reuse of objects that might have more context than we expect (wrong basic
> > auth, wrong cookie, wrong client cert, wrong SNI etc.). Of course most of
> > these things do work now, but reusing the connections and the objects from
> > the HC libs as well as the SSLContext might introduce future bugs of the
> > sort "next iteration uses context data from the previous one" which could
> > then be worked around quickly by disabling the reuse. In general since the
> > risks are quite vague so I'd stay on the performance side here for the
> > default value (reuse). But probably based on my incomplete understanding of
> > the lifecycles of HC objects, I'd feel better if we had such a "reset
> > between iterations" switch.
>
> I think we already do.

I don't understand you. The current impl of reset (https.use.cached.ssl.context) is a NOP in the http case.
Comment 9 Philippe Mouawad 2016-01-06 20:56:14 UTC
(In reply to Rainer Jung from comment #4)
> Yes, sorry, JMeter properties.
> 
> I think we don't need/want all three of them (individual control over
> connection reuse, HTTPClient reuse and SSLContext reuse). After some
> experimenting IMHO the useful cases are:
> 
> - full reuse of SSLContext, HTTPClient and existing connections
> - no reuse of SSLContext, HTTPClient and existing connections

Looks ok to me.
> 
> The default should be full reuse like it is today.
> 
> The question is though, whether the name "https.use.cached.ssl.context" for
> the property is good. Once could use a uniform switch to determine reuse in
> the HTTP case as well, something like reset.http.objects.before.iteration.
> Default would be "false" (no reset = reuse) and to start each iteration with
> fresh objects you would switch to "true".
> 
> The old flag could be deprecated. If a user would use both, the old and the
> new, we could warn during startup and the new flag would win.

Thanks for fix
Comment 10 Sebb 2016-01-07 03:01:42 UTC
(In reply to Rainer Jung from comment #8)
> (In reply to Sebb from comment #7)
> > (In reply to Rainer Jung from comment #6)
> > > As far as I understand the situation reusing (the default) means e.g.
> > > connections are kept open and are reused via HTTP Keep-Alive during the next
> > > iteration. I do see this behavior using https, I expect it to happen for
> > > http too. That's of course efficient but might not be what you want. A new
> > > iteration typically is a new user who would start with new connections.
> > >
> > > Of course modelling the connection behavior in an exact way is beyond what
> > > we support, but the decision to start a new iteration with new connections
> > > could be interesting for some (approximate modeling). It is somehow
> > > analogous to modeling the handshake rate by resetting the SSLContext,
> > > although a new connection is of cours less disruptive than a new ssl
> > > handshake. All in all not a critical feature.
> > 
> > Surelt that's already catered for by the "Use Keepalive" checkbox?
> > If not checked, JMeter sends Connection: close.
> 
> I didn't mean to completely disable Keep-Alive but to not alow Keep-Alive
> from one itertaion to the next. 

What do you mean by iteration?
If you mean the start of the next loop, one just needs to clear Use KeepAlive on the last sampler.

> The "Use Keepalive" checkbox would disable
> Keep-Alive completely.

Use Keepalive only applies to a single sample.
If deselected, it means the connection will be reset after it completes.

> 
> > > In addition I think the optional behavior "reset instead of reuse" for http
> > > and https is a good safety net against problems that might creep in due to
> > > reuse of objects that might have more context than we expect (wrong basic
> > > auth, wrong cookie, wrong client cert, wrong SNI etc.). Of course most of
> > > these things do work now, but reusing the connections and the objects from
> > > the HC libs as well as the SSLContext might introduce future bugs of the
> > > sort "next iteration uses context data from the previous one" which could
> > > then be worked around quickly by disabling the reuse. In general since the
> > > risks are quite vague so I'd stay on the performance side here for the
> > > default value (reuse). But probably based on my incomplete understanding of
> > > the lifecycles of HC objects, I'd feel better if we had such a "reset
> > > between iterations" switch.
> >
> > I think we already do.
> 
> I don't understand you. The current impl of reset
> (https.use.cached.ssl.context) is a NOP in the http case.

I meant that Use KeepAlive acts as the reset if deselected.

So I think we already have the means to do a reset each iteration.
Comment 11 Rainer Jung 2016-01-07 03:52:51 UTC
(In reply to Sebb from comment #10)
> (In reply to Rainer Jung from comment #8)
> > (In reply to Sebb from comment #7)
> > > (In reply to Rainer Jung from comment #6)
> > > > As far as I understand the situation reusing (the default) means e.g.
> > > > connections are kept open and are reused via HTTP Keep-Alive during the next
> > > > iteration. I do see this behavior using https, I expect it to happen for
> > > > http too. That's of course efficient but might not be what you want. A new
> > > > iteration typically is a new user who would start with new connections.
> > > >
> > > > Of course modelling the connection behavior in an exact way is beyond what
> > > > we support, but the decision to start a new iteration with new connections
> > > > could be interesting for some (approximate modeling). It is somehow
> > > > analogous to modeling the handshake rate by resetting the SSLContext,
> > > > although a new connection is of cours less disruptive than a new ssl
> > > > handshake. All in all not a critical feature.
> > > 
> > > Surelt that's already catered for by the "Use Keepalive" checkbox?
> > > If not checked, JMeter sends Connection: close.
> > 
> > I didn't mean to completely disable Keep-Alive but to not alow Keep-Alive
> > from one itertaion to the next. 
> 
> What do you mean by iteration?

One loop for the same thread.

> If you mean the start of the next loop, one just needs to clear Use
> KeepAlive on the last sampler.

Depending on the logic controllers in the plan, there is no easy way to decide upon the last sampler, more precisely the last execution of the last sampler in one loop. It could change during plan execution. Most often it will be a defined logout sample or similar, but not necessarily. You e.g. might want to loop the last sampler until a condition becomes true.

> > The "Use Keepalive" checkbox would disable
> > Keep-Alive completely.
> 
> Use Keepalive only applies to a single sample.
> If deselected, it means the connection will be reset after it completes.

OK, you were talking about a single HTTP sampler, I was talking about the HTTP sampler default config element. See above for problem of defining the "last sampler".

> > > > In addition I think the optional behavior "reset instead of reuse" for http
> > > > and https is a good safety net against problems that might creep in due to
> > > > reuse of objects that might have more context than we expect (wrong basic
> > > > auth, wrong cookie, wrong client cert, wrong SNI etc.). Of course most of
> > > > these things do work now, but reusing the connections and the objects from
> > > > the HC libs as well as the SSLContext might introduce future bugs of the
> > > > sort "next iteration uses context data from the previous one" which could
> > > > then be worked around quickly by disabling the reuse. In general since the
> > > > risks are quite vague so I'd stay on the performance side here for the
> > > > default value (reuse). But probably based on my incomplete understanding of
> > > > the lifecycles of HC objects, I'd feel better if we had such a "reset
> > > > between iterations" switch.
> > >
> > > I think we already do.
> > 
> > I don't understand you. The current impl of reset
> > (https.use.cached.ssl.context) is a NOP in the http case.
> 
> I meant that Use KeepAlive acts as the reset if deselected.
> 
> So I think we already have the means to do a reset each iteration.

As I wrote, connection keep alive is only one of the arguments. General object reuse between loop iterations (HTTPClient instance) is what makes me feel more uncomfortable. If someone with enough HC knowledge than me comments strongly that such a reset will most likely not be necessary, I'm OK to forget about that reset feature for http.
Comment 12 Philippe Mouawad 2016-01-12 21:04:43 UTC
Hi,
Can the patch be commited ?
Thanks
Comment 13 Philippe Mouawad 2016-03-11 22:49:10 UTC
Hello,

I suggest we delay this bugfix to 3.1 unless there is a volunteer to test it and confirm it is OK (including Parallel Download)

Regards
Philippe
Comment 14 bamboocha324 2016-08-31 18:31:20 UTC
Hi,

well in jmeter 2.13 i had the following (so far not clear to me) issue with https where the https WAS mutual ssl.
I set up the ssl debug on and watched what happend with my http samplers.

I had a java key store whith 2 client certs.
I had a key store configuration element which starts at 0 and ends at 1 (2 elements).

Now i had one thread group and 2 http samplers both pointing to the same https server. One sampler was targeting https://server/uri and the second ../uri2.

Now i had the following problem.
The first http sampler picked the first ssl cert from my keystore [0] made a mutual ssl connection and execute .../uri. -great.

The second http sampler DOES NOT REUSE the connection and DOES NOT executed the ../uri2 BUT the second http sampler made a NEW connection (ssl) and picked a NEW client cert from the keystore [1]

It was STILL the SAME Thread just the next http sampler.
It was a mess because the user with the client cert on [1] was not allowed to execute the ../uri2. 

Then...i changed the parameter "https.use.cached.ssl.context" and put value "true".
I have no idea why...but it WORKED?!?. 
Means the first http sampler did mutual ssl and execute ../uri
The second http sampler did NOT the handshake again but execute the ../uri2 as the same user as the first sampler. perfect.
But why?
Comment 15 Philippe Mouawad 2018-02-24 18:37:30 UTC
Still present in 4.0
Comment 16 Philippe Mouawad 2018-02-24 21:46:31 UTC
Created attachment 35742 [details]
Amended Patch

Following dev mailing list discussion "Bug 58807" , this is a patch implementing what I suggested.
Comment 17 Philippe Mouawad 2018-02-24 21:54:54 UTC
Created attachment 35743 [details]
Test plan with different cases
Comment 18 Philippe Mouawad 2018-02-24 21:56:05 UTC
Created attachment 35744 [details]
NodeJS Project for client certificate authentication

NodeJS project that can be run :

node server.js
Comment 19 Philippe Mouawad 2018-02-25 21:20:09 UTC
Author: pmouawad
Date: Sun Feb 25 21:19:42 2018
New Revision: 1825328

URL: http://svn.apache.org/viewvc?rev=1825328&view=rev
Log:
Bug 58807 - https.use.cached.ssl.context=false is broken
Based partly on Rainer Jung analysis and patch.
Bugzilla Id: 58807

Modified:
    jmeter/trunk/bin/jmeter.properties
    jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPHC4Impl.java
    jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPHCAbstractImpl.java
    jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerProxy.java
    jmeter/trunk/xdocs/changes.xml
    jmeter/trunk/xdocs/usermanual/properties_reference.xml
Comment 20 Philippe Mouawad 2018-08-28 21:14:32 UTC
Author: pmouawad
Date: Tue Aug 28 21:12:36 2018
New Revision: 1839506

URL: http://svn.apache.org/viewvc?rev=1839506&view=rev
Log:
Bug 58807 - Reset SSL State on Thread Group iteration only (was https.use.cached.ssl.context=false is broken)
Document
Bugzilla Id: 58807

Modified:
    jmeter/trunk/xdocs/changes.xml
    jmeter/trunk/xdocs/usermanual/component_reference.xml
    
pmouawad@apache.org
	
11:13 PM (0 minutes ago)
	
to commits
Author: pmouawad
Date: Tue Aug 28 21:13:48 2018
New Revision: 1839507

URL: http://svn.apache.org/viewvc?rev=1839507&view=rev
Log:
Bug 58807 - Reset SSL State on Thread Group iteration only (was https.use.cached.ssl.context=false is broken)
Document
Bugzilla Id: 58807

Modified:
    jmeter/trunk/xdocs/usermanual/properties_reference.xml
Comment 21 Philippe Mouawad 2018-09-18 21:34:47 UTC
*** Bug 62734 has been marked as a duplicate of this bug. ***