RFR: 8272120: Avoid looking for standard encodings in "java." modules
This is the continuation of JDK-8233884 and JDK-8271456. This change affects
fewer cases so I fix all "java." modules at once.
In many places standard charsets are looked up via their names, for example:
absolutePath.getBytes("UTF-8");
This could be done more efficiently(up to x20 time faster) with use of
java.nio.charset.StandardCharsets:
absolutePath.getBytes(StandardCharsets.UTF_8);
The later variant also makes the code cleaner, as it is known not to throw
UnsupportedEncodingException in contrary to the former variant.
tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.
-
Commit messages:
- Initial fix of JDK-8272120
Changes: https://git.openjdk.java.net/jdk/pull/5063/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5063=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8272120
Stats: 127 lines in 15 files changed: 24 ins; 53 del; 50 mod
Patch: https://git.openjdk.java.net/jdk/pull/5063.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/5063/head:pull/5063
PR: https://git.openjdk.java.net/jdk/pull/5063
Re: RFR: 8270137: Kerberos Credential Retrieval from Cache not Working in Cross-Realm Setup
On Fri, 6 Aug 2021 19:27:30 GMT, Martin Balao wrote:
> I'd like to propose a fix for JDK-8270137 [1].
>
> This bug is triggered when using a previously stored referral ticket (in the
> Referrals Cache) at the moment of following a S4U2Proxy cross-realm referral.
> The mistakenly-used referral ticket matched the client and service names but
> it was obtained as a result of a non-S4U2Proxy request. In fact, it was the
> middle service that got it while trying to determine the backend service
> realm in a previous S4U2Proxy communication. The mistakenly-used referral
> ticket was not bind to the impersonated user (in other words, it was not
> obtained attaching the user's TGS as part of a S4U2Proxy request) and, thus,
> must not be used.
>
> Even when one possible approach to fix this issue could be to be more
> selective at the moment of getting referral tickets from the Cache (that is:
> do not get anything from the Cache if it's for a S4U2Proxy request), I
> decided to go one step further and enhance the Referrals Cache. With this
> enhancement, we add more information to the stored referral tickets such as a
> footprint of the TGS (in the case of S4U2Proxy requests) or the user
> principal (in the case of S4U2Self requests). We now allow to store S4U2Proxy
> and S4U2Self referrals tickets but those will be re-used only if there is a
> perfect match of the TGS or user principal. As an example, if a middle
> service tries to replicate the exact S4U2Self communication for exactly the
> same user, cached referral tickets should be okay. With this enhancement, we
> increase the use of the Cache and the performance (time, network resources,
> etc.).
>
> The ReferralsTest is enhanced to reflect these new scenarios and now uses
> cached S4U2Proxy/S4U2Self referral tickets.
>
> No regressions observed in jdk/sun/security/krb5.
>
> --
> [1] - https://bugs.openjdk.java.net/browse/JDK-8270137
Looks fine. Some small comments.
src/java.security.jgss/share/classes/sun/security/krb5/internal/CredentialsUtil.java
line 90:
> 88: Credentials creds = serviceCreds(
> 89: KDCOptions.with(KDCOptions.FORWARDABLE),
> 90: ccreds, ccreds.getClient(), sname, client,
How about we rename `client` to `user` here?
src/java.security.jgss/share/classes/sun/security/krb5/internal/CredentialsUtil.java
line 496:
> 494: */
> 495: private static void handleS4U2SelfReferral(PAData[] pas,
> 496: PrincipalName user, Credentials oldCeds, Credentials
> newCreds)
`oldCreds` is useless now.
src/java.security.jgss/share/classes/sun/security/krb5/internal/ReferralsCache.java
line 59:
> 57: private byte[] clientSvcTicketEnc; // S4U2Proxy only
> 58: ReferralCacheKey (PrincipalName cname, PrincipalName sname,
> 59: PrincipalName user, Ticket clientSvcTicket) {
It's probably not necessary, but I somehow feel it will be clearer to add
S4U2Type into the key. In fact, with all these info it almost look like the key
contains everything in a TGS-REQ (except for the timestamp maybe).
-
Marked as reviewed by weijun (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5036
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature
On Fri, 6 Aug 2021 20:51:23 GMT, Martin Balao wrote: > > > Yes, I see what you mean. Contrary to P11PrivateKey::getFormat and > P11PrivateKey::getEncodedInternal where a 'null' returned value is documented > in java.security.Key, we don't have that documentation for the other > interfaces such as java.security.interfaces.DSAPrivateKey. That can lead to > NPE if the client casts the P11Key to the interface, invokes the method and > depends on a non-null value. I will give this some thought and try to come up > with something, because the information is already there and (in reality) we > need it internally only. It's clear that all these interfaces were not > designed with unextractable P11 keys in mind, because it makes sense to me > (conceptually) to have a private key from which we can retrieve some values > (public, such as the parameters) and not other ones. Right, PKCS11 and unextractable keys come a few releases later after the JCA API. So, there may be some difficulties trying to work them into existing APIs. In order to maintain backward compatibility, API changes are also limited. - PR: https://git.openjdk.java.net/jdk/pull/4961
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v7]
On Fri, 6 Aug 2021 19:35:23 GMT, Anthony Scarpino wrote:
>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java
>> line 1779:
>>
>>> 1777: int len = 0;
>>> 1778: if (inLen >= PARALLEL_LEN) {
>>> 1779: implGCMCrypt(in, inOfs, inLen, in, inOfs, out,
>>> outOfs, gctr,
>>
>> Should save the return value into 'len'? For consistency sake, choose
>> between GaloisCounterMode.implGCMCrypt(...) and implGCMCrypt and not both?
>
> I do not understand this comment
Doesn't implGCMCrypt(...) return an int telling how much bytes it has
processed? Then we adjust the index and remain input length with this value.
But here we didn't save the return value which looks wrong. Did I miss
something?
Never mind my second comment, I mis-read the code.
-
PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v8]
On Mon, 9 Aug 2021 15:49:07 GMT, Smita Kamath wrote: >> I would like to submit AES-GCM optimization for x86_64 architectures >> supporting AVX3+VAES (Evex encoded AES). This optimization interleaves AES >> and GHASH operations. >> Performance gain of ~1.5x - 2x for message sizes 8k and above. > > Smita Kamath has updated the pull request incrementally with one additional > commit since the last revision: > > rewiew update src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 1652: > 1650: outOfs += resultLen; > 1651: len += resultLen; > 1652: } Seems redundant since DecryptOp.doFinal(..) already has this check? - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v7]
On Fri, 6 Aug 2021 20:37:22 GMT, Anthony Scarpino wrote: >> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java >> line 1120: >> >>> 1118: inOfs += r; >>> 1119: inLen -= r; >>> 1120: } >> >> Have you considered move the "if (inLen >= PARALLEL_LEN) block" into >> EncryptOp.update() impl (just like the Encrypt.doFinal() impl) ? Even though >> not all op.update() calls process large data, but it'd reduce code >> duplication and ensures that all large data processed by EncryptOp.update() >> calls would call the intrinsified method. > > There are cases where inLen is known to be smaller than PARALLEL_LEN and is a > waste of a check, such as merging with the ibuffer to create one block. Also > moving it into EncryptOp would always mean an additional check and maybe an > unnecessary jump to another method. > > I did that for doFinal, because gctr/ghash.doFinal() needs to was no extra > checks. Alright, as long as you did consider that. - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v7]
On Fri, 6 Aug 2021 19:53:28 GMT, Anthony Scarpino wrote: >> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java >> line 87: >> >>> 85: private static final int MAX_BUF_SIZE = Integer.MAX_VALUE; >>> 86: // data size when buffer is divided up to aid in intrinsics >>> 87: private static final int TRIGGERLEN = 65536; // 64k >> >> With this interleaved impl, is this TRIGGERLEN still needed? The >> implGCMCrypt(byte[] in, int inOfs, int inLen, >> byte[] ct, int ctOfs, byte[] out, int outOfs, GCTR gctr, GHASH >> ghash) method is intrinsified, would there be a difference in increasing the >> number of gctr/ghash calls inside an already intrinsified method? > > Yes, they are two different intrinsics. The new implGCMCrypt intrinsic is > supported by newer processors so there is no guarantee that implGCMCrypt will > run the intrinsic. Hmm, ok. Thanks for the explanation. - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v8]
> I would like to submit AES-GCM optimization for x86_64 architectures > supporting AVX3+VAES (Evex encoded AES). This optimization interleaves AES > and GHASH operations. > Performance gain of ~1.5x - 2x for message sizes 8k and above. Smita Kamath has updated the pull request incrementally with one additional commit since the last revision: rewiew update - Changes: - all: https://git.openjdk.java.net/jdk/pull/4019/files - new: https://git.openjdk.java.net/jdk/pull/4019/files/69145008..ecf8e6d7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4019=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4019=06-07 Stats: 29 lines in 1 file changed: 12 ins; 5 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/4019.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4019/head:pull/4019 PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8270137: Kerberos Credential Retrieval from Cache not Working in Cross-Realm Setup
On Fri, 6 Aug 2021 19:27:30 GMT, Martin Balao wrote: > I'd like to propose a fix for JDK-8270137 [1]. > > This bug is triggered when using a previously stored referral ticket (in the > Referrals Cache) at the moment of following a S4U2Proxy cross-realm referral. > The mistakenly-used referral ticket matched the client and service names but > it was obtained as a result of a non-S4U2Proxy request. In fact, it was the > middle service that got it while trying to determine the backend service > realm in a previous S4U2Proxy communication. The mistakenly-used referral > ticket was not bind to the impersonated user (in other words, it was not > obtained attaching the user's TGS as part of a S4U2Proxy request) and, thus, > must not be used. > > Even when one possible approach to fix this issue could be to be more > selective at the moment of getting referral tickets from the Cache (that is: > do not get anything from the Cache if it's for a S4U2Proxy request), I > decided to go one step further and enhance the Referrals Cache. With this > enhancement, we add more information to the stored referral tickets such as a > footprint of the TGS (in the case of S4U2Proxy requests) or the user > principal (in the case of S4U2Self requests). We now allow to store S4U2Proxy > and S4U2Self referrals tickets but those will be re-used only if there is a > perfect match of the TGS or user principal. As an example, if a middle > service tries to replicate the exact S4U2Self communication for exactly the > same user, cached referral tickets should be okay. With this enhancement, we > increase the use of the Cache and the performance (time, network resources, > etc.). > > The ReferralsTest is enhanced to reflect these new scenarios and now uses > cached S4U2Proxy/S4U2Self referral tickets. > > No regressions observed in jdk/sun/security/krb5. > > -- > [1] - https://bugs.openjdk.java.net/browse/JDK-8270137 Thanks for your effort. We tested this patch on our internal environment and it works fine. It definetly makes sense to store Proxy tickets in the cache to reduce network load instead of our proposed "do not use cache for Proxy tickets" approach. - PR: https://git.openjdk.java.net/jdk/pull/5036
Re: RFR: 8266182: Automate manual steps listed in the test jdk/sun/security/pkcs12/ParamsTest.java [v4]
On Mon, 9 Aug 2021 11:06:25 GMT, Abdul Kolarkunnu
wrote:
>> ParamsTest is an interop test between keytool <-> openssl. There are some
>> manual steps listed in jdk/sun/security/pkcs12/params/README to perform
>> after the execution of jtreg execution. So this test is to perform that
>> manual steps.
>
> Abdul Kolarkunnu has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8266182: Automate manual steps listed in the test
> jdk/sun/security/pkcs12/ParamsTest.java
Thanks for the fix. Code looks fine. I'm only concerned on the failure when
openssl is not found.
test/jdk/sun/security/pkcs12/KeytoolOpensslInteropTest.java line 48:
> 46: import java.nio.file.Files;
> 47: import java.nio.file.Path;
> 48: import java.nio.file.Paths;
IntelliJ IDEA shows quite some imports are useless.
test/jdk/sun/security/pkcs12/KeytoolOpensslInteropTest.java line 88:
> 86: testWithJavaCommands();
> 87: testWithOpensslCommands(opensslPath);
> 88: } else {
I still think it's better to succeed with a warning when the openssl binary
cannot be found. IMHO it's a little unfriendly to force people setting up a
system property to let the test pass. It's also dependent on runner machines
and the user might have to adjust their scripts or launcher all the time. I
would rather keep the old test/data if I want to ensure the test gets run
everywhere.
Also, it might also help if the test can search for openssl, maybe simply from
`/usr/bin/openssl` or `/usr/local/bin/openssl`, but this means you might need
to check for the version.
Third, is it OK to let the system property pointing to the binary itself
directly? When I was trying out this test I was using the openssl I built and
it's not in a `bin` sub-directory.
test/jdk/sun/security/pkcs12/KeytoolOpensslInteropTest.java line 100:
> 98: OutputAnalyzer output = ProcessTools.executeCommand(
> 99: opensslPath, "version")
> 100: .shouldHaveExitValue(0);
This looks like a good time to ensure the version is 1.1.* or at least 1.* (I
haven't tested with 1.0.? versions). Also, when trying out with 3.0.0, there
are only 2 failures (line 119 generating weak file without -legacy. line 479
succeeds with a warning).
test/jdk/sun/security/pkcs12/KeytoolOpensslInteropTest.java line 580:
> 578: return SecurityTools.keytool(s);
> 579: }
> 580:
I feel the lines below should go to a test library.
-
PR: https://git.openjdk.java.net/jdk/pull/4413
Re: RFR: 8266182: Automate manual steps listed in the test jdk/sun/security/pkcs12/ParamsTest.java [v4]
> ParamsTest is an interop test between keytool <-> openssl. There are some > manual steps listed in jdk/sun/security/pkcs12/params/README to perform after > the execution of jtreg execution. So this test is to perform that manual > steps. Abdul Kolarkunnu has updated the pull request incrementally with one additional commit since the last revision: 8266182: Automate manual steps listed in the test jdk/sun/security/pkcs12/ParamsTest.java - Changes: - all: https://git.openjdk.java.net/jdk/pull/4413/files - new: https://git.openjdk.java.net/jdk/pull/4413/files/20d6bb5f..fe17653f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4413=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4413=02-03 Stats: 1463 lines in 10 files changed: 633 ins; 830 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4413.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4413/head:pull/4413 PR: https://git.openjdk.java.net/jdk/pull/4413
Re: RFR: 8269130: Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying
On Mon, 26 Jul 2021 19:15:55 GMT, Brett Okken wrote: >> I found few places, where code initially perform `Object[] >> Colleciton.toArray()` call and then manually copy array into another array >> with required type. >> This PR cleanups such places to more shorter call `T[] >> Collection.toArray(T[])`. > > src/java.base/share/classes/java/security/Security.java line 656: > >> 654: return null; >> 655: >> 656: return candidates.toArray(new Provider[0]); > > Is this called often enough to warrant creating a constant of `new > Provider[0]` (benefits of this covered in the _Wisdom of the Ancients_ blog > linked)? May be yes, may be not. I like `0`-sized array approach more. 1. It looks clearer and easier to read 2. It should be faster than original code anyway - PR: https://git.openjdk.java.net/jdk/pull/4487
Re: RFR: 8269130: Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying
On Mon, 26 Jul 2021 19:55:09 GMT, Brett Okken wrote: >> I found few places, where code initially perform `Object[] >> Colleciton.toArray()` call and then manually copy array into another array >> with required type. >> This PR cleanups such places to more shorter call `T[] >> Collection.toArray(T[])`. > > src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/SystemDictionaryHelper.java > line 92: > >> 90: } >> 91: >> 92: InstanceKlass[] searchResult = tmp.toArray(new InstanceKlass[0]); > > Is it too far outside the scope of these changes to make `tmp` an `ArrayList` > rather than a `Vector`? I'll create separate PRs to migrate Vector usages to ArrayList. - PR: https://git.openjdk.java.net/jdk/pull/4487
