[jdk17] RFR: 8267397: AlgorithmId's OID cache is never refreshed
Could someone help review this small fix? This clears the static aliasOidsTable field in AlgorithmId class when provider list is changed. Enhanced existing regression test to cover the provider removal scenario. Thanks, Valerie - Commit messages: - 8267397: AlgorithmId's OID cache is never refreshed Changes: https://git.openjdk.java.net/jdk17/pull/36/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=36=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8267397 Stats: 105 lines in 3 files changed: 76 ins; 20 del; 9 mod Patch: https://git.openjdk.java.net/jdk17/pull/36.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/36/head:pull/36 PR: https://git.openjdk.java.net/jdk17/pull/36
Re: [jdk17] RFR: 8268621: SunJCE provider may throw unexpected NPE for un-initialized AES KW/KWP Ciphers
On Fri, 11 Jun 2021 21:22:49 GMT, Valerie Peng wrote: > Could someone help review this straightforward fix? The current impl for AES > KW and KWP cipher should check for possible null iv value in its > CipherSpi.engineGetIV() and CipherSpi.engineGetParameters() impls. Updated > the regression test to cover this scenario as well as some other minor > updates. > > Thanks! > Valerie Marked as reviewed by xuelei (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/34
JEP 411: Updates to alternatives
Noticing the updates made to JEP 411 Alternatives, I think I might have a minimalist alternative, you may find interesting: Remove: 1. SecurityManager 2. Policy provider and implementation 3. Permission checks in JDK code addressed by improvements to encapsulation, eg RuntimePermission "access class in package" and ReflectPermission, these are no longer necessary, however I would recommend retaining checks at System::getProperty and setProperty, as these may contain security sensitive information (eg keystore and truststore). 4. doPrivileged calls within the JVM other than those which preserve context across threads, most permissions that "leak" are addressed at #3 above, and POLP tooling can capture other permissions. It would appear that doPrivileged is more appropriate for application code, rather than JDK code. 5. I'm not sure about removing doPrivileged calls intended to preserve context within OpenJDK. Changes (improvements): 1. Make Guard::check a default method, that delegates to a provider, with a single method (eg Authority::confirm(Guard)) that does nothing by default. Remove all implementing instances of Permission::check. (this could be backported easily). SecurityManager methods are just permission checks, existing use cases of SecurityManager can be supported with this one method. This could be back ported to Java 8, so libraries that currently support all supported Java versions can continue to do so. All calls to SecurityManager methods in JDK code can be replaced by the corresponding permission check. 2. Add permission checks to data parsers (eg deserialization), this allows implementations to grant these permissions only to users, if there is not an authenticated user, then the data received by the parser cannot be trusted. 3. "Modules that are mapped to the boot loader get a unique ProtectionDomain that includes a useful code source rather than using a "shared" PD." This allows permission to be granted to users, (not code) so certain privileged operations, such as data parsing cannot be performed without an authenticated user, eg deserialization. When data can only be trusted from authenticated users. Removal of AccessController and AccessControlContext have greater impact. AccessController's stack walk is high scaling (I haven't observed any contention, I assume it's non blocking and thread confined?), it's certainly very performant, it could be replaced internally by StackWalker to reduce OpenJDK's maintenance burden, although it isn't clear what the performance impact might be, but it will no doubt performance improvement is possible. With SecurityManager gone, no implementation and no policy provider, it simply provides the mechanics for an authorization layer without all the baggage, allowing both simple and complex implementations. It's not for sandboxing untrusted code. Improvements will allow it to be utilised by developers, to prevent consumption of untrusted code or data and to limit the privileges of trusted code and users to principles of least privilege. It should also simplifies many tests, as JDK code only need confirm Permission checks are made and functionality of the AccessController and AccessControlContext methods if these are retained (I would prefer to see that, at least for JAAS compatibility). -- Regards, Peter Firmstone Zeus Project Services Pty Ltd.
[jdk17] RFR: 8268621: SunJCE provider may throw unexpected NPE for un-initialized AES KW/KWP Ciphers
Could someone help review this straightforward fix? The current impl for AES KW and KWP cipher should check for possible null iv value in its CipherSpi.engineGetIV() and CipherSpi.engineGetParameters() impls. Updated the regression test to cover this scenario as well as some other minor updates. Thanks! Valerie - Commit messages: - 8268621: SunJCE provider may throw unexpected NPE for un-initialized AES KW/KWP Ciphers Changes: https://git.openjdk.java.net/jdk17/pull/34/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=34=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268621 Stats: 50 lines in 4 files changed: 34 ins; 3 del; 13 mod Patch: https://git.openjdk.java.net/jdk17/pull/34.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/34/head:pull/34 PR: https://git.openjdk.java.net/jdk17/pull/34
Re: [jdk17] RFR: 8268349: Provide more detail in JEP 411 warning messages [v2]
> More loudly and precise warning messages when a security manager is either > enabled at startup or installed at runtime. > > This is new PR for the `openjdk/jdk17` repo copied from > https://github.com/openjdk/jdk/pull/4400. A new commit is added. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: update warnings to match the new CSR - Changes: - all: https://git.openjdk.java.net/jdk17/pull/13/files - new: https://git.openjdk.java.net/jdk17/pull/13/files/a8fc259e..c62dff99 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk17=13=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=13=00-01 Stats: 4 lines in 1 file changed: 0 ins; 1 del; 3 mod Patch: https://git.openjdk.java.net/jdk17/pull/13.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/13/head:pull/13 PR: https://git.openjdk.java.net/jdk17/pull/13
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v2]
On Fri, 11 Jun 2021 17:54:02 GMT, Vladimir Kozlov wrote: >> Thanks for your comments Vladimir. The intrinsic is called for encrypt as >> well as decrypt operation. > > Only one intrinsic is declared in this change: `_galoisCounterMode_AESCrypt`. > Other AES intrinsics have 2 that is why they have to pass intrinsic_id(). See > lines before this. Note, _counterMode_AESCrypt is not example - it has the same issue. - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v2]
On Fri, 11 Jun 2021 17:19:37 GMT, Smita Kamath wrote: >> src/hotspot/share/opto/library_call.cpp line 547: >> >>> 545: >>> 546: case vmIntrinsics::_galoisCounterMode_AESCrypt: >>> 547: return inline_galoisCounterMode_AESCrypt(intrinsic_id()); >> >> You don't need to pass `intrinsic_id()` for this implementation unless you >> plan to add decrypt intrinsic later. > > Thanks for your comments Vladimir. The intrinsic is called for encrypt as > well as decrypt operation. Only one intrinsic is declared in this change: `_galoisCounterMode_AESCrypt`. Other AES intrinsics have 2 that is why they have to pass intrinsic_id(). See lines before this. - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v2]
On Fri, 11 Jun 2021 15:45:02 GMT, Vladimir Kozlov wrote:
>> Smita Kamath has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8267125:Updated intrinsic signature to remove copies of counter, state and
>> subkeyHtbl
>
> src/hotspot/share/opto/library_call.cpp line 547:
>
>> 545:
>> 546: case vmIntrinsics::_galoisCounterMode_AESCrypt:
>> 547: return inline_galoisCounterMode_AESCrypt(intrinsic_id());
>
> You don't need to pass `intrinsic_id()` for this implementation unless you
> plan to add decrypt intrinsic later.
Thanks for your comments Vladimir. The intrinsic is called for encrypt as well
as decrypt operation.
> src/hotspot/share/opto/library_call.cpp line 6564:
>
>> 6562: Node* subkeyHtbl = load_field_from_object(ghash_object,
>> "subkeyHtbl", "[J");
>> 6563: Node* state = load_field_from_object(ghash_object, "state", "[J");
>> 6564: if (embeddedCipherObj == NULL || counter == NULL || subkeyHtbl ==
>> NULL || state == NULL) return false;
>
> Follow coding style for such long condition:
>
> if () {
> return false;
> }
I will make the change. Thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v2]
On Fri, 4 Jun 2021 23:49:31 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:
>
> 8267125:Updated intrinsic signature to remove copies of counter, state and
> subkeyHtbl
Do you plan to implement `decrypt` intrinsic too?
src/hotspot/share/opto/library_call.cpp line 547:
> 545:
> 546: case vmIntrinsics::_galoisCounterMode_AESCrypt:
> 547: return inline_galoisCounterMode_AESCrypt(intrinsic_id());
You don't need to pass `intrinsic_id()` for this implementation unless you plan
to add decrypt intrinsic later.
src/hotspot/share/opto/library_call.cpp line 6545:
> 6543: top_out != NULL && top_out->klass() != NULL, "args are
> strange");
> 6544:
> 6545: // checks are the responsibility of the caller
Do you have all NULL for all objects and range checks in Java code for this
intrinsic?
src/hotspot/share/opto/library_call.cpp line 6564:
> 6562: Node* subkeyHtbl = load_field_from_object(ghash_object, "subkeyHtbl",
> "[J");
> 6563: Node* state = load_field_from_object(ghash_object, "state", "[J");
> 6564: if (embeddedCipherObj == NULL || counter == NULL || subkeyHtbl ==
> NULL || state == NULL) return false;
Follow coding style for such long condition:
if () {
return false;
}
-
PR: https://git.openjdk.java.net/jdk/pull/4019
[jdk17] Integrated: 8268093: Manual Testcase: "sun/security/krb5/config/native/TestDynamicStore.java" Fails with NPE
On Fri, 11 Jun 2021 01:46:46 GMT, Weijun Wang wrote: > The test must run with sudo but I forgot to mention it. New comment and a > better error message is added. This pull request has now been integrated. Changeset: e39346e7 Author:Weijun Wang URL: https://git.openjdk.java.net/jdk17/commit/e39346e708a06cdee2b9a096f08c1cfe2e21dfc2 Stats: 14 lines in 2 files changed: 12 ins; 0 del; 2 mod 8268093: Manual Testcase: "sun/security/krb5/config/native/TestDynamicStore.java" Fails with NPE Reviewed-by: mullan - PR: https://git.openjdk.java.net/jdk17/pull/12
Re: [jdk17] RFR: 8268093: Manual Testcase: "sun/security/krb5/config/native/TestDynamicStore.java" Fails with NPE
On Fri, 11 Jun 2021 01:46:46 GMT, Weijun Wang wrote: > The test must run with sudo but I forgot to mention it. New comment and a > better error message is added. Marked as reviewed by mullan (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/12
Re: RFR: JDK-8268464 : Remove dependancy of TestHttpsServer, HttpTransaction, HttpCallback from open/test/jdk/sun/net/www/protocol/https/ tests [v3]
On Fri, 11 Jun 2021 13:38:14 GMT, Mahendra Chhipa
wrote:
>> …HttpCallback from open/test/jdk/sun/net/www/protocol/https/ tests
>
> Mahendra Chhipa has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Remove extra space.
test/jdk/sun/net/www/protocol/https/HttpsURLConnection/B6216082.java line 89:
> 87: // created as it will use an ephemeral port.
> 88: System.setProperty("https.proxyPort",
> 89: Integer.toString(proxy.getLocalPort()));
A potentially better alternative to setting system properties in main could be
to set a ProxySelector (using ProxySelector.setDefault());
test/jdk/sun/net/www/protocol/https/HttpsURLConnection/B6216082.java line 184:
> 182: HttpURLConnection uc = (HttpURLConnection)url.openConnection();
> 183: System.out.println(uc.getResponseCode());
> 184: if(uc.getResponseCode() == 400) {
It could be better to use: `uc.getResponseCode() != 200` here - since anything
but 200 should be an error?
test/jdk/sun/net/www/protocol/https/HttpsURLConnection/B6216082.java line 186:
> 184: if(uc.getResponseCode() == 400) {
> 185: uc.disconnect();
> 186: throw new RuntimeException("Test failed : bad http request");
Maybe add the response code to the exception message.
-
PR: https://git.openjdk.java.net/jdk/pull/4432
Re: RFR: JDK-8268464 : Remove dependancy of TestHttpsServer, HttpTransaction, HttpCallback from open/test/jdk/sun/net/www/protocol/https/ tests [v3]
> …HttpCallback from open/test/jdk/sun/net/www/protocol/https/ tests Mahendra Chhipa has updated the pull request incrementally with one additional commit since the last revision: Remove extra space. - Changes: - all: https://git.openjdk.java.net/jdk/pull/4432/files - new: https://git.openjdk.java.net/jdk/pull/4432/files/49965732..db615030 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4432=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4432=01-02 Stats: 3 lines in 1 file changed: 0 ins; 3 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4432.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4432/head:pull/4432 PR: https://git.openjdk.java.net/jdk/pull/4432
Re: RFR: JDK-8268464 : Remove dependancy of TestHttpsServer, HttpTransaction, HttpCallback from open/test/jdk/sun/net/www/protocol/https/ tests [v2]
> …HttpCallback from open/test/jdk/sun/net/www/protocol/https/ tests Mahendra Chhipa has updated the pull request incrementally with one additional commit since the last revision: Implemented review comments. - Changes: - all: https://git.openjdk.java.net/jdk/pull/4432/files - new: https://git.openjdk.java.net/jdk/pull/4432/files/b490e1ff..49965732 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4432=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4432=00-01 Stats: 36 lines in 2 files changed: 5 ins; 17 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/4432.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4432/head:pull/4432 PR: https://git.openjdk.java.net/jdk/pull/4432
Re: RFR: 8268427: Improve AlgorithmConstraints:checkAlgorithm performance
On Fri, 11 Jun 2021 04:21:15 GMT, Xue-Lei Andrew Fan wrote:
>> The collection required when new Constraints() should retain the default
>> case of the elements, because some code will depend on this, for example, .
>> [entry.startsWith("keySize")](https://github.com/openjdk/jdk/blob/dd1cbadc82bcecf718b96c833a5845fde79db061/src/java.base/share/classes/sun/security/util/DisabledAlgorithmConstraints.java#L383).
>> But the set required by the permits should unify the case of the elements,
>> because algorithm may be uppercase or lowercase, but the Set:contains()
>> cannot handle this situation.
>> So we need to create a new Set that ignores the default case of elements.
>
> For the entry.startsWith("keySize") example, I don't think keySize is an
> algorithm that could be listed individually in the list. The "keySize" may
> be just a part one algorithm, for example "RSA keySize < 1024".
>
> It's a good point about the lowercase and upper case. Did you check how
> constraints like the "keySize" are expressed in the list or set?
Yes, you're right. The "keySize" is not an independent algorithm listed in
list, it exists in a form like "ec keysize <224".
In the case of "keySize", the object in the list stored in
`algorithmConstraints` is `KeySizeConstraint`, then keysize will be checked in
[algorithmConstraints.permits(algorithm,
parameters)](https://github.com/openjdk/jdk/blob/dd1cbadc82bcecf718b96c833a5845fde79db061/src/java.base/share/classes/sun/security/util/DisabledAlgorithmConstraints.java#L169)
by `KeySizeConstraint:permits`.
-
PR: https://git.openjdk.java.net/jdk/pull/4424
