On Sat, 6 Nov 2021 08:54:33 GMT, Daniel Jeliński <d...@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
