RFR: 8274308: Improve efficiency for HandshakeContext initialization. [v2]

[Clive Verghese]

On Sat, 6 Nov 2021 08:54:33 GMT, Daniel Jeliński <duke at openjdk.java.net> wrote:

>>> Hi @XueleiFan,
>>> 
>>> Thank you for the feedback, though it’s not recommended to set the protocols and cipher suites, various frameworks often do set these values, [Some references below]. Would it still make sense to only optimize the default case?
>> 
>> I know, there are applications setting protocols and cipher suites for themselves.  But it should be common that the TLS connections created from the same SSLContext uses the same cipher suites and protocols.  There are still exceptions, but may be not that common.  Using an instance cache in SSLContext rather than static cache could be a further improvement.
>> 
>> I would not use static cache unless there is no other option.  The troublesome of static cache is more than the performance impact.  A series of problems like memory leaking and security issues could be followed even we design the cache very carefully.
>> 
>> About the benchmark, did you have a chance to check the result for full handshakes?  The code in the benchmark checks the initialization of the handshake, the handshake may not be full/completed.  A benchmark for full handshake may be able to provide more evidence about the overall impact of the improvement.
>
> Can we extend the public API of `SSLContext` with methods for managing `activeProtocols`, `activeCipherSuites` and possibly `algorithmConstraints`? Without this API change we would need to check every time if the active protocols, ciphers and constraints match the cached ones.
> 
> Also, looking at the flame graphs provided, it appears that about 50% of the handshake time is spent in `SSLAlgorithmDecomposer#decompose`; given that there are only 3 instances of that class (and could be further reduced to 2), would it make sense to optimize the method by caching the results of algorithm decomposition? We could use a size-limited soft cache to avoid memory leaks, and I fail to see how the cached information could be exploitable.

>I don't think it is the direction to change the immunity of SSLContext. There are two proposals right now. One is to change the SSLContext public APIs, @djelinski, by adding cipher suites and protocols configuration (immutable). This proposal does not impact the current code, where enabled protocols and cipher suites are configured in each socket. But it could be useful for future code to configure these parameters in the context level. It may be easier to maintain and could have better performance.
>
> I did not have time to think over the potentials yet, but I did have the proposal to move from static cache to instance cache. SSLContext/impl is ideal because normally only one SSLContext instance is required, surely there are exceptions, for an application. We may also missed another issue that the underlying crypto provider could change, for example unplugging a token from the slot. Once the crypto provider is changed, the cached binding between protocol and cipher suites may not work any longer. So we may need to take care of this kind of issue for static cache. For an instance of SSLContext, the crypto provider changing issues are still there, but get mitigated as the context has already bound to specific trust manager and key manager. Yes, it does change the SSLContext implementation from immutability to mutability. But it is in an acceptable level, just as what we did for session cache in the context instance.
>
> Just for your consideration. This is a tough problem, and it may take time to find a suitable solution. Let's keep exchanging our thoughts about it.

Thank you @XueleiFan for the feedback, I'll take a look at it from that perspective as well and post my findings here. 

> Yes, please. Your found is impressive to me.

I'll create a new JBS issue and PR along with benchmarks for this. Thank you.

-------------

PR: https://git.openjdk.java.net/jdk/pull/5793