[Bug 58807] https.use.cached.ssl.context=false is broken

[bugzilla]

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

Philippe Mouawad  changed:

   What|Removed |Added

   Target Milestone|--- |JMETER_4.1
   Keywords||FixedInTrunk

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 58807] https.use.cached.ssl.context=false is broken

[bugzilla]

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

Philippe Mouawad <p.moua...@ubik-ingenierie.com> changed:

   What|Removed |Added

 Status|NEW |RESOLVED
 Resolution|--- |FIXED

--- Comment #19 from Philippe Mouawad <p.moua...@ubik-ingenierie.com> ---
Author: pmouawad
Date: Sun Feb 25 21:19:42 2018
New Revision: 1825328

URL: http://svn.apache.org/viewvc?rev=1825328=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

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 58807] https.use.cached.ssl.context=false is broken

[bugzilla]

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

--- Comment #18 from Philippe Mouawad  ---
Created attachment 35744
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=35744=edit
NodeJS Project for client certificate authentication

NodeJS project that can be run :

node server.js

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 58807] https.use.cached.ssl.context=false is broken

[bugzilla]

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

--- Comment #17 from Philippe Mouawad  ---
Created attachment 35743
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=35743=edit
Test plan with different cases

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 58807] https.use.cached.ssl.context=false is broken

[bugzilla]

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

Philippe Mouawad  changed:

   What|Removed |Added

  Attachment #33410|0   |1
is obsolete||

--- Comment #16 from Philippe Mouawad  ---
Created attachment 35742
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=35742=edit
Amended Patch

Following dev mailing list discussion "Bug 58807" , this is a patch
implementing what I suggested.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 58807] https.use.cached.ssl.context=false is broken

[bugzilla]

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

Philippe Mouawad  changed:

   What|Removed |Added

Version|Nightly (Please specify |2.11
   |date)   |
   Severity|normal  |major
 Status|NEEDINFO|NEW

--- Comment #15 from Philippe Mouawad  ---
Still present in 4.0

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 58807] https.use.cached.ssl.context=false is broken

[bugzilla]

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

--- Comment #14 from bamboocha...@yahoo.de ---
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?

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 58807] https.use.cached.ssl.context=false is broken

[bugzilla]

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

--- Comment #13 from Philippe Mouawad  ---
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

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 58807] https.use.cached.ssl.context=false is broken

[bugzilla]

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

Philippe Mouawad  changed:

   What|Removed |Added

   Keywords||PatchAvailable

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 58807] https.use.cached.ssl.context=false is broken

[bugzilla]

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

Philippe Mouawad  changed:

   What|Removed |Added

 Status|NEW |NEEDINFO

--- Comment #12 from Philippe Mouawad  ---
Hi,
Can the patch be commited ?
Thanks

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 58807] https.use.cached.ssl.context=false is broken

[bugzilla]

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

--- Comment #3 from Sebb  ---
(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.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 58807] https.use.cached.ssl.context=false is broken

[bugzilla]

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

--- Comment #5 from Sebb  ---
(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?

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 58807] https.use.cached.ssl.context=false is broken

[bugzilla]

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

--- Comment #6 from Rainer Jung  ---
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 58807] https.use.cached.ssl.context=false is broken

[bugzilla]

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

--- Comment #4 from Rainer Jung  ---
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 58807] https.use.cached.ssl.context=false is broken

[bugzilla]

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

--- Comment #10 from Sebb  ---
(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.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 58807] https.use.cached.ssl.context=false is broken

[bugzilla]

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

--- Comment #11 from Rainer Jung  ---
(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.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 58807] https.use.cached.ssl.context=false is broken

[bugzilla]

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

--- Comment #2 from Rainer Jung  ---
Created attachment 33410
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=33410=edit
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 58807] https.use.cached.ssl.context=false is broken

[bugzilla]

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

--- Comment #1 from Rainer Jung  ---
> 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

-- 
You are receiving this mail because:
You are the assignee for the bug.