Re: RFR: 8261433: Better pkcs11 performance for libpkcs11:C_EncryptInit/libpkcs11:C_DecryptInit
On Tue, 26 Mar 2024 22:57:59 GMT, Valerie Peng wrote: > > Now that we are going with the normative version first, maybe we should > > make additional changes to clean up the flow further?Say, > > > > 1. update `jGCMParamsToCKGCMParamPtr(JNIEnv *env, jobject jParam, CK_ULONG > > *pLength)` to allocate the normative structure instead. > > 2. enhance `updateGCMParams(JNIEnv *env, CK_MECHANISM_PTR mechPtr)` to > > return a copy of mech pointer containing the non-normative structure. > > If PKCS11 spec version is less than 2.40, we are retaining the same logic as > it exists today. If we make the above changes then there will be a crash in > NSS version 3.51 and below where PKCS11 2.20 spec is used. > > However, I can optimze the code by introducing new boolean variable in > C_DecryptInit and C_EncryptInit to indicate if we need to send normative > struct. If so, I'll add a goto statement to updateGCM params. I did not do > this initially because in 11u and above I have seen goto being removed. I prototyped this a bit and we can add GCM specific encrypt/decrypt init methods which takes additional boolean argument which indicate which version to use first for the PKCS11 init call. This way, we can remove the mechanism check in C_EncryptInit/C_DecryptInit since GCM init would go through the new GCM specific encrypt/decrypt init methods. I ended up also making changes to p11_convert.c and p11_util.c as well to use the normative version by default. It's easier to explain though the webrev/patch which I will share with you through slack. I don't have the older NSS library to verify the prototype changes, but it works fine w/ the current NSS library in the artifactory. Maybe you can take a look and let me know what do you think. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/18425#issuecomment-2023985885
RFR: 8329213: Better validation for com.sun.security.ocsp.useget option
[JDK-8328638](https://bugs.openjdk.org/browse/JDK-8328638) introduced a new boolean option, `com.sun.security.ocsp.useget`. We use the usual `Boolean.parseBoolean` to convert it from String to boolean value, which works correctly for `false` and `true` as boolean values. However, any string that is not `true` would be treated as `false`. Which means that if users mistype the value, they would get a `false`, which is a non-default value, which is against the spirit of the JDK-8328638. It would be preferable to validate the option range a bit better, and default to the correct value on any error. Additional testing: - [x] Eyeballing `GetAndPostTests` debugging, checking that GET/POST are properly enabled/disabled for `false`, `true`, `foobar` passed as option values - [x] `jdk_security`, out of the box - [x] `jdk_security` with `-Dcom.sun.security.ocsp.useget=false` passes - [x] `jdk_security` with `-Dcom.sun.security.ocsp.useget=foobar` passes - Commit messages: - Fix Changes: https://git.openjdk.org/jdk/pull/18525/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18525&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8329213 Stats: 37 lines in 2 files changed: 33 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/18525.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18525/head:pull/18525 PR: https://git.openjdk.org/jdk/pull/18525
Re: RFR: 8322767: TLS full handshake is slow with PKCS12KeyStore and X509KeyManagerImpl [v3]
On Wed, 27 Mar 2024 08:15:08 GMT, Hai-May Chao wrote: > I ran the benchmark to measure the time needed to build a TLS context using > PKIX KeyManager (with protocols "TLSv1.2" and "TLS”) before and after the > changes to X509KeyManagerImpl.java. Here are the results: > > Without changes: Benchmark (tlsVersion) Mode Cnt Score Error Units > SSLHandshake.doHandshake TLSv1.2 thrpt 15 1073890.853 ? 18249.629 ops/s > SSLHandshake.doHandshake TLS thrpt 15 1139049.624 ? 5507.867 ops/s > > With changes: Benchmark (tlsVersion) Mode Cnt Score Error Units > SSLHandshake.doHandshake TLSv1.2 thrpt 15 130.039 ? 4.372 ops/s > SSLHandshake.doHandshake TLS thrpt 15 134.157 ? 1.903 ops/s > > For the case without changes, it has higher Error values and looks like there > is more variability in the measurements. I ran it again, and got similar > results with no background processes running. I can’t explain if there may be > environmental variables affecting the Error values. The changes that arise > time to set up cached map at initialization time will be paid off for > multiple handshaking. The Error values looks fine to me. The differences between them are caused by the differences between op/s numbers. If I read the numbers correctly, the TLS context buildup performance impact is about 100 times. The performance downgrading is not small. If I remembered correctly, a few years ago we tried hard to speed up the TLS booting up as it is very slow to initialize a TLS context and customers run into problems in their environment. Thinking about the industry effort to deploy the service in cloud environment, where need to speed up the booting time as soon as possible. In cloud or micro-service loading environment, it is more important to boot-up the context as soon as possible, comparing to speed up each connections. I don't think we have to complete the cache during the context building period. It is also possible to build the cache when the entry is required. - PR Comment: https://git.openjdk.org/jdk/pull/17956#issuecomment-2023101804
Re: RFR: 8328638: Fallback option for POST-only OCSP requests [v5]
On Mon, 25 Mar 2024 19:24:39 GMT, Aleksey Shipilev wrote: >> See the rationale/discussion in the bug. This patch introduces the option >> that allows to restore >> pre-[JDK-8179503](https://bugs.openjdk.org/browse/JDK-8179503) behavior. The >> default behavior does not change. Better suggestions for flag name are >> welcome. >> >> Additional testing: >> - [x] `jdk_security` passes out of the box (includes new test config) >> - [x] `jdk_security` passes with flag override >> - [x] Eyeballing `GetPostTests` amended debugging output, `GET`-s are used >> by default for small requests, `POST`-s are used for everything with flag >> override > > Aleksey Shipilev has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains five commits: > > - Merge branch 'master' into JDK-8328638-ocsp-post > - Merge branch 'master' into JDK-8328638-ocsp-post > - Merge branch 'master' into JDK-8328638-ocsp-post > - Amend CAInterop test > - Fix All right, thanks! Here it goes. - PR Comment: https://git.openjdk.org/jdk/pull/18408#issuecomment-2022952755
Integrated: 8328638: Fallback option for POST-only OCSP requests
On Wed, 20 Mar 2024 19:48:52 GMT, Aleksey Shipilev wrote: > See the rationale/discussion in the bug. This patch introduces the option > that allows to restore > pre-[JDK-8179503](https://bugs.openjdk.org/browse/JDK-8179503) behavior. The > default behavior does not change. Better suggestions for flag name are > welcome. > > Additional testing: > - [x] `jdk_security` passes out of the box (includes new test config) > - [x] `jdk_security` passes with flag override > - [x] Eyeballing `GetPostTests` amended debugging output, `GET`-s are used > by default for small requests, `POST`-s are used for everything with flag > override This pull request has now been integrated. Changeset: 614db2ea Author:Aleksey Shipilev URL: https://git.openjdk.org/jdk/commit/614db2ea9e10346475eef34629eab54878aa482d Stats: 83 lines in 3 files changed: 79 ins; 0 del; 4 mod 8328638: Fallback option for POST-only OCSP requests Reviewed-by: mullan, rhalade - PR: https://git.openjdk.org/jdk/pull/18408
Re: RFR: 8328638: Fallback option for POST-only OCSP requests [v5]
On Wed, 27 Mar 2024 09:30:34 GMT, Aleksey Shipilev wrote: > CSR is done. I assume we are good here, and we can integrate? Yes. - PR Comment: https://git.openjdk.org/jdk/pull/18408#issuecomment-2022687436
Re: RFR: 8328638: Fallback option for POST-only OCSP requests [v5]
On Mon, 25 Mar 2024 19:24:39 GMT, Aleksey Shipilev wrote: >> See the rationale/discussion in the bug. This patch introduces the option >> that allows to restore >> pre-[JDK-8179503](https://bugs.openjdk.org/browse/JDK-8179503) behavior. The >> default behavior does not change. Better suggestions for flag name are >> welcome. >> >> Additional testing: >> - [x] `jdk_security` passes out of the box (includes new test config) >> - [x] `jdk_security` passes with flag override >> - [x] Eyeballing `GetPostTests` amended debugging output, `GET`-s are used >> by default for small requests, `POST`-s are used for everything with flag >> override > > Aleksey Shipilev has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains five commits: > > - Merge branch 'master' into JDK-8328638-ocsp-post > - Merge branch 'master' into JDK-8328638-ocsp-post > - Merge branch 'master' into JDK-8328638-ocsp-post > - Amend CAInterop test > - Fix CSR is done. I assume we are good here, and we can integrate? - PR Comment: https://git.openjdk.org/jdk/pull/18408#issuecomment-2022305331
Re: RFR: 8322767: TLS full handshake is slow with PKCS12KeyStore and X509KeyManagerImpl [v3]
On Tue, 26 Mar 2024 06:00:33 GMT, Hai-May Chao 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 implemen
Re: RFR: 8322767: TLS full handshake is slow with PKCS12KeyStore and X509KeyManagerImpl [v3]
On Tue, 26 Mar 2024 06:00:33 GMT, Hai-May Chao 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 Yes, it’d be nice to make sure no bugs were introduced. It is not desirable to introduce bugs when working on this issue to improve performance. As each run may have slightly different scores, I ran the benchmark again. This time, it shows no regression as below. It seems the small score degradation may not be flagged as regression; hence, I should run multiple times to try to get the average results. Benchmark (keyMgr) (resume) (tlsVersion) Mode CntScore ErrorUnits SSLHandshake.doHandshakeSunX509 true TLSv1.2 thrpt 15 9381.086 ? 171.396 ops/s SSLHandshake.doHandshakeSunX509 true TLS thrpt 15953.039 ? 19.431 ops/s SSLHandshake.doHandshakeSunX509falseTLSv1.2thrpt 15 616.918 ? 7.652 ops/s SSLHandshake.doHandshakeSunX509false TLSthrpt 15530.865 ? 7.109 ops/s SSLHandshake.doHandshake PKIX trueTLSv1.2 thrpt 15 9653.252 ? 92.254 ops/s SSLHandshake.doHandshake PKIX true TLS thrpt 15 951.771 ? 28.665 ops/s SSLHandshake.doHandshake PKIX falseTLSv1.2 thrpt 15 617.288 ? 7.197 ops/s SSLHandshake.doHandshake PKIX false TLS thrpt 15521.930 ?19.292 ops/s I ran the benchmark to measure the time needed to build a TLS context using PKIX KeyManager (with protocols "TLSv1.2" and "TLS”) before and after the changes to X509KeyManagerImpl.java. Here are the results: Without changes: Benchmark(tlsVersion)ModeCnt Score Error Units SSLHandshake.doHandshakeTLSv1.2 thrpt 15 1073890.853 ? 18249.629 ops/s SSLHandshake.doHandshake TLS thrpt 15 1139049.624 ? 5507.867 ops/s With changes: Benchmark(tlsVersion)
