On Tue, 26 Mar 2024 06:00:33 GMT, Hai-May Chao <hc...@openjdk.org> wrote:
>> For the PKIX KeyManager and PKCS12 Keystore, when the TLS server sends the >> ServerHello message and ultimately calls the >> X509KeyManagerImpl.chooseEngineServerAlias() method, it retrieves the >> private key from the keystore, decrypts it, and caches both the key and its >> certificate. This caching currently occurs only during a single handshake. >> Since decryption can be time-consuming, a modification has been implemented >> to cache the keystore entries at initialization time. This way, it won't be >> necessary to retrieve and decrypt the keys for multiple handshakes, which >> could lead to performance drawbacks. >> >> A change was made to also update/refresh the cached entry as the >> certificates in the PKCS12 keystore may change, for scenarios like when the >> certificate expires and a new one is added under a different alias, and the >> certificate chain returned by the PKCS12 keystore is not the same as the one >> in the cache. While attempting to handle when to refresh a cached entry to >> accommodate keystore changes, we would like to know if you agree that this >> improvement is worth the risk. We would also like to know if you have a >> preference for other options: >> >> 1. Accept that PKIX+PKCS12 is slow. >> 2. Add a configuration option (system property, maybe) to decide the level >> of caching (1 - same as the existing one, 2 - same caching as in >> SunX509KeyManagerImpl, 3 - the new caching introduced in this change). >> >> Additionally, the benchmark test SSLHandshake.java is modified to include a >> @Param annotation, allowing it to pass different KeyManagerFactory values >> (SunX509 and PKIX) to the benchmark method. >> >> Running modified SSLHandshake.java test prior to the change that caches the >> PKCS12 keystore entries for PKIX: >> Benchmark (keyMgr) (resume) (tlsVersion) Mode Cnt >> Score Error Units >> SSLHandshake.doHandshake SunX509 true TLSv1.2 thrpt 15 >> 9346.292 ? 379.023 ops/s >> SSLHandshake.doHandshake SunX509 true TLS thrpt 15 >> 940.175 ? 21.215 ops/s >> SSLHandshake.doHandshake SunX509 false TLSv1.2 thrpt 15 >> 594.418 ? 23.374 ops/s >> SSLHandshake.doHandshake SunX509 false TLS thrpt 15 >> 534.030 ? 16.709 ops/s >> SSLHandshake.doHandshake PKIX true TLSv1.2 thrpt 15 >> 9359.086 ? 246.257 ops/s >> SSLHandshake.doHandshake PKIX true TLS thrpt 15 >> 933.835 ? 81.388 ops/s >> SSLHandshake.doHandshake PKIX false TLSv1.2 thrpt 15 ... > > Hai-May Chao has updated the pull request incrementally with one additional > commit since the last revision: > > Updated with John's comments Well this PR doesn't introduce new bugs, but it exacerbates a preexisting one. The key manager does not try to validate the private/public key pair in any way, and will use whatever the key store provides, even if not valid. With the current key manager implementation, as soon as a key pair is changed, the key manager will start using the new pair. With the proposed PR, the private key is cached, and the certificate chain is used as the cache key. If the key store entry is corrected by changing the private key only, the key manager cache will not be updated, and the bad private key will be served until the certificate chain changes. The default key store getEntry method implementation is not thread-safe, and during concurrent modification it can return a PrivateKeyEntry mixing the old private key and the new certificate chain. This was recently fixed for PKCS12 key stores (#18156), but other key store types might still be affected. This is why the PR is only caching PKCS12 key stores. This should really be explained in a code comment by the way. With PKCS12 key store, we can still cache an invalid entry if the invalid entry is actually present in the key store. This could happen if a user tried to update the key/cert pair, but used the wrong key in the process. If the user then tried to fix the mistake by updating the key without changing the certificate chain, the change would not be picked up by the key manager, which would keep serving the wrong key. The above scenario, as unlikely as it sounds, i why I'm not sure about this change. What alternatives do we have? - We could keep the existing code, and accept the fact that it's 400% slower than SunX509. However, given that we want to steer users away from SunX509 and change the default to PKIX ([JDK-8272875](https://bugs.openjdk.org/browse/JDK-8272875)), the performance penalty might be a bitter pill to swallow. - We could use a faster KDF for private key encryption in PKCS12; JKS is also encrypting private keys, but the encryption overhead is negligible (SunX509+JKS had similar performance to PKIX+JKS last time I measured) - We could implement a new caching key manager, that would cache key store entries during creation (like SunX509), and then behave identically to PKIX. - We could add a new system property to control caching in PKIX key manager, then switch to PKIX, then point the users who need every last bit of performance to that system property. ------------- PR Comment: https://git.openjdk.org/jdk/pull/17956#issuecomment-2022282831
