Re: RFR: 8267860: Off-by-one bug when searching arrays in AlpnGreaseTest [v2]
> This PR resolves: https://bugs.openjdk.org/browse/JDK-8267860 Kevin Driver has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - Merge branch 'openjdk:master' into JDK-8267860 - Update AlpnGreaseTest.java - JDK-8267860: off-by-one error - Changes: - all: https://git.openjdk.org/jdk/pull/9131/files - new: https://git.openjdk.org/jdk/pull/9131/files/507abec6..11379da8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=9131=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=9131=00-01 Stats: 901 lines in 25 files changed: 616 ins; 53 del; 232 mod Patch: https://git.openjdk.org/jdk/pull/9131.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9131/head:pull/9131 PR: https://git.openjdk.org/jdk/pull/9131
Re: RFR: 8267860: Off-by-one bug when searching arrays in AlpnGreaseTest
On Fri, 10 Jun 2022 17:12:30 GMT, Kevin Driver wrote:
> This PR resolves: https://bugs.openjdk.org/browse/JDK-8267860
LGTM.
test/jdk/sun/security/ssl/ALPN/AlpnGreaseTest.java line 86:
> 84:
> 85: private static void findGreaseInClientHello(byte[] bytes) throws
> Exception {
> 86: for (int i = 0; i < bytes.length - greaseBytes.length + 1; i++) {
LGTM.
-
Marked as reviewed by wetmore (Reviewer).
PR: https://git.openjdk.org/jdk/pull/9131
RFR: 8267860: Off-by-one bug when searching arrays in AlpnGreaseTest
This PR resolves: https://bugs.openjdk.org/browse/JDK-8267860 - Commit messages: - Update AlpnGreaseTest.java - JDK-8267860: off-by-one error Changes: https://git.openjdk.org/jdk/pull/9131/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=9131=00 Issue: https://bugs.openjdk.org/browse/JDK-8267860 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/9131.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9131/head:pull/9131 PR: https://git.openjdk.org/jdk/pull/9131
Re: RFR: 8267860: Off-by-one bug when searching arrays in AlpnGreaseTest
On Fri, 10 Jun 2022 17:12:30 GMT, Kevin Driver wrote: > This PR resolves: https://bugs.openjdk.org/browse/JDK-8267860 Sorry for the noisy comment section... I was just trying to trigger jcheck. It looks to have been triggered. - PR: https://git.openjdk.org/jdk/pull/9131
Re: RFR: JDK-8285263 Minor cleanup could be done in java.security [v6]
On Fri, 10 Jun 2022 21:27:58 GMT, Mark Powers wrote: >> https://bugs.openjdk.java.net/browse/JDK-8285263 Minor cleanup could be done >> in java.security >> >> JDK-8273046 is the umbrella bug for this bug. The changes were too large for >> a single code review, so it was decided to split into smaller chunks. This >> is one such chunk: >> >> open/src/java.base/share/classes/java/security > > Mark Powers has updated the pull request incrementally with two additional > commits since the last revision: > > - bad grammar > - Max comments Most of the changes are trivial, like those no-more-public for interfaces. Reviewing 94 files is still easier than fixing 94 out of ...er... 185 files. - PR: https://git.openjdk.org/jdk/pull/8319
Integrated: 8288270: Tier1 build failures after JDK-8287178
On Fri, 10 Jun 2022 23:49:45 GMT, Hai-May Chao wrote: > Please review the small fix in comment. This pull request has now been integrated. Changeset: f7a4be75 Author:Hai-May Chao URL: https://git.openjdk.org/jdk/commit/f7a4be75fbe9e703dea94459285c72094d4d8646 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8288270: Tier1 build failures after JDK-8287178 Reviewed-by: weijun, jiefu - PR: https://git.openjdk.org/jdk/pull/9135
Re: Integrated: 8288270: Tier1 build failures after JDK-8287178
On Fri, 10 Jun 2022 23:53:37 GMT, Jie Fu wrote: >> Please review the small fix in comment. > > Looks good and trivial. > Thanks. @DamonFool @wangweij Thanks for the review! - PR: https://git.openjdk.org/jdk/pull/9135
Integrated: 8288270: Tier1 build failures after JDK-8287178
Please review the small fix in comment. - Commit messages: - 8288270: Tier1 build failures after JDK-8287178 Changes: https://git.openjdk.org/jdk/pull/9135/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=9135=00 Issue: https://bugs.openjdk.org/browse/JDK-8288270 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/9135.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9135/head:pull/9135 PR: https://git.openjdk.org/jdk/pull/9135
Re: Integrated: 8288270: Tier1 build failures after JDK-8287178
On Fri, 10 Jun 2022 23:49:45 GMT, Hai-May Chao wrote: > Please review the small fix in comment. Looks good and trivial. Thanks. - Marked as reviewed by jiefu (Reviewer). PR: https://git.openjdk.org/jdk/pull/9135
Re: Integrated: 8288270: Tier1 build failures after JDK-8287178
On Fri, 10 Jun 2022 23:49:45 GMT, Hai-May Chao wrote: > Please review the small fix in comment. LGTM. Thanks. - Marked as reviewed by weijun (Reviewer). PR: https://git.openjdk.org/jdk/pull/9135
Re: RFR: JDK-8285263 Minor cleanup could be done in java.security [v6]
On Fri, 10 Jun 2022 21:27:58 GMT, Mark Powers wrote: >> https://bugs.openjdk.java.net/browse/JDK-8285263 Minor cleanup could be done >> in java.security >> >> JDK-8273046 is the umbrella bug for this bug. The changes were too large for >> a single code review, so it was decided to split into smaller chunks. This >> is one such chunk: >> >> open/src/java.base/share/classes/java/security > > Mark Powers has updated the pull request incrementally with two additional > commits since the last revision: > > - bad grammar > - Max comments Thanks!!! You did a great job reviewing 94 files!!! Do you ever sleep? - PR: https://git.openjdk.org/jdk/pull/8319
Re: RFR: 8286779: javax.crypto.CryptoPolicyParser#isConsistent always returns 'true' [v4]
On Fri, 10 Jun 2022 21:41:50 GMT, Hai-May Chao wrote: >> Please review a small fix in CryptoPolicyParser class that it should not >> pass “processedPermissions” parameter by value. >> Ran MACH5 tier1 and tier2 without failures. > > Hai-May Chao has updated the pull request incrementally with one additional > commit since the last revision: > > Manual test instr using System.out Marked as reviewed by rhalade (Reviewer). - PR: https://git.openjdk.org/jdk/pull/8985
Re: RFR: JDK-8285263 Minor cleanup could be done in java.security [v5]
On Fri, 10 Jun 2022 21:44:16 GMT, Weijun Wang wrote: >> A privilegedGetProperty method? That would be the subject of a new bug I >> think. > > Yes. JDK-8288271 has been created - PR: https://git.openjdk.org/jdk/pull/8319
Integrated: 8287178: IntegerModuloP::multiplicativeInverse returns 0 for 0
On Thu, 9 Jun 2022 21:34:56 GMT, Weijun Wang wrote: > Add comment to the method. This pull request has now been integrated. Changeset: d4b473d8 Author:Weijun Wang URL: https://git.openjdk.org/jdk/commit/d4b473d89046874f25aa6f65f3ae96f7d8397d50 Stats: 8 lines in 1 file changed: 7 ins; 0 del; 1 mod 8287178: IntegerModuloP::multiplicativeInverse returns 0 for 0 Reviewed-by: jnimeh - PR: https://git.openjdk.org/jdk/pull/9115
Re: RFR: JDK-8285263 Minor cleanup could be done in java.security [v5]
On Fri, 10 Jun 2022 00:35:16 GMT, Mark Powers wrote: >> src/java.base/share/classes/java/security/SecureRandom.java line 905: >> >>> 903: private static final Pattern pattern = >>> 904: Pattern.compile( >>> 905: "\\s*([\\S&&[^:,]]*)(:([\\S&&[^,]]*))?\\s*(,(.*))?"); >> >> Don't quite understand this change. Can you explain? > > The author must have thought `:` and `,` were metacharacters that needed to > be escaped to turn them into ordinary characters. I verified with a small > java program that` \:` and` \,` are equivalent to `;` and `,`. > The IntelliJ complaint is "Redundant character escape `'\:'`". I see. Thanks for the explanation. >> src/java.base/share/classes/java/security/cert/CertPathValidator.java line >> 335: >> >>> 333: String cpvtype = >>> 334: AccessController.doPrivileged((PrivilegedAction) >>> () -> >>> 335: Security.getProperty(CPV_TYPE)); >> >> See too many, maybe we need a dedicated method in >> `sun.security.util.SecurityProperties`? > > A privilegedGetProperty method? That would be the subject of a new bug I > think. Yes. - PR: https://git.openjdk.org/jdk/pull/8319
Re: RFR: JDK-8285263 Minor cleanup could be done in java.security [v6]
On Fri, 10 Jun 2022 21:27:58 GMT, Mark Powers wrote: >> https://bugs.openjdk.java.net/browse/JDK-8285263 Minor cleanup could be done >> in java.security >> >> JDK-8273046 is the umbrella bug for this bug. The changes were too large for >> a single code review, so it was decided to split into smaller chunks. This >> is one such chunk: >> >> open/src/java.base/share/classes/java/security > > Mark Powers has updated the pull request incrementally with two additional > commits since the last revision: > > - bad grammar > - Max comments Looks good to me now. For `Provider$ServiceKey::matches`, maybe you can add a comment on why `==` is correct. For other changes suggested by IntelliJ, maybe just follow them to make the IDE silent. You decide it. - Marked as reviewed by weijun (Reviewer). PR: https://git.openjdk.org/jdk/pull/8319
Re: RFR: 8286779: javax.crypto.CryptoPolicyParser#isConsistent always returns 'true' [v3]
On Fri, 10 Jun 2022 17:31:56 GMT, Rajan Halade wrote: >> Hai-May Chao has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed copyright > > test/jdk/javax/crypto/CryptoPermissions/InconsistentEntries.java line 38: > >> 36: import java.security.Security; >> 37: >> 38: // This is a manual test to test a custom “default_local.policy" >> containing inconsistent entries > > Thanks for the instructions. For manual test, I would rather have these as a > System.out on test start than a comment here. This helps engineers who look > at jtr file for failure analysis as they generally won't refer test source > code to understand missing steps. Good point. Made the changes as suggested. Thanks for the review. - PR: https://git.openjdk.org/jdk/pull/8985
Re: RFR: 8286779: javax.crypto.CryptoPolicyParser#isConsistent always returns 'true' [v4]
> Please review a small fix in CryptoPolicyParser class that it should not pass > “processedPermissions” parameter by value. > Ran MACH5 tier1 and tier2 without failures. Hai-May Chao has updated the pull request incrementally with one additional commit since the last revision: Manual test instr using System.out - Changes: - all: https://git.openjdk.org/jdk/pull/8985/files - new: https://git.openjdk.org/jdk/pull/8985/files/2e8f5d95..9f94f620 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8985=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8985=02-03 Stats: 26 lines in 1 file changed: 14 ins; 12 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/8985.diff Fetch: git fetch https://git.openjdk.org/jdk pull/8985/head:pull/8985 PR: https://git.openjdk.org/jdk/pull/8985
Re: RFR: JDK-8285263 Minor cleanup could be done in java.security [v6]
> https://bugs.openjdk.java.net/browse/JDK-8285263 Minor cleanup could be done > in java.security > > JDK-8273046 is the umbrella bug for this bug. The changes were too large for > a single code review, so it was decided to split into smaller chunks. This is > one such chunk: > > open/src/java.base/share/classes/java/security Mark Powers has updated the pull request incrementally with two additional commits since the last revision: - bad grammar - Max comments - Changes: - all: https://git.openjdk.org/jdk/pull/8319/files - new: https://git.openjdk.org/jdk/pull/8319/files/44140a01..59b2383e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8319=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8319=04-05 Stats: 24 lines in 8 files changed: 3 ins; 9 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/8319.diff Fetch: git fetch https://git.openjdk.org/jdk pull/8319/head:pull/8319 PR: https://git.openjdk.org/jdk/pull/8319
Re: RFR: JDK-8285263 Minor cleanup could be done in java.security [v5]
On Wed, 8 Jun 2022 16:01:48 GMT, Weijun Wang wrote: >> Mark Powers has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 11 commits: >> >> - Merge >> - fourth iteration >> - Merge >> - Merge >> - third iteration >> - Merge >> - Merge >> - second iteration >> - Merge >> - Merge >> - ... and 1 more: https://git.openjdk.org/jdk/compare/4d6fb515...44140a01 > > src/java.base/share/classes/java/security/Security.java line 260: > >> 258: } >> 259: >> 260: return null; > > If `entry` is always null, then we don't need to declare it at all. Agreed. - PR: https://git.openjdk.org/jdk/pull/8319
Re: RFR: JDK-8285263 Minor cleanup could be done in java.security [v5]
On Wed, 8 Jun 2022 16:06:12 GMT, Weijun Wang wrote: >> Mark Powers has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 11 commits: >> >> - Merge >> - fourth iteration >> - Merge >> - Merge >> - third iteration >> - Merge >> - Merge >> - second iteration >> - Merge >> - Merge >> - ... and 1 more: https://git.openjdk.org/jdk/compare/4d6fb515...44140a01 > > src/java.base/share/classes/java/security/Signature.java line 271: > >> 269: (algorithm + " Signature not available"); >> 270: } >> 271: // try services until we find a Spi or a working Signature >> subclass > > Is `Spi` pronounced `spy` or "S-P-I"? Reversing change: "an Spi" is correct. - PR: https://git.openjdk.org/jdk/pull/8319
Re: RFR: JDK-8285263 Minor cleanup could be done in java.security [v5]
On Wed, 8 Jun 2022 15:53:06 GMT, Weijun Wang wrote:
>> Mark Powers has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 11 commits:
>>
>> - Merge
>> - fourth iteration
>> - Merge
>> - Merge
>> - third iteration
>> - Merge
>> - Merge
>> - second iteration
>> - Merge
>> - Merge
>> - ... and 1 more: https://git.openjdk.org/jdk/compare/4d6fb515...44140a01
>
> src/java.base/share/classes/java/security/Provider.java line 1098:
>
>> 1096: boolean matches(String type, String algorithm) {
>> 1097: return (Objects.equals(this.type, type)) &&
>> 1098: (Objects.equals(this.originalAlgorithm, algorithm));
>
> In fact, I'm not sure why the original code is brave enough to use `==`
> instead of `equals`. If you do believe it's a real bug (and can write a
> regression test), let's fix it in a real bug fix. Otherwise, keep using `==`
> and add a comment saying it's correct.
The `ServiceKey` class has `equals` and `matches` methods. The `matches` method
returns true if two objects contain identical String pointers. The `equals`
method returns true if String pointers are different but have the same value.
This is by design, so it would be a mistake to take IntelliJ's suggestion here.
Keeping the `==` and adding a comment.
Good catch.
> src/java.base/share/classes/java/security/SecureClassLoader.java line 221:
>
>> 219: // only), and the fragment is not considered.
>> 220: CodeSourceKey key = new CodeSourceKey(cs);
>> 221: /* not used */
>
> What is not used? `key1`? How about just rename it to `unused`?
Renamed `key1` to `unused`.
> src/java.base/share/classes/java/security/SecureClassLoader.java line 235:
>
>> 233: }
>> 234:
>> 235: private record CodeSourceKey(CodeSource cs) {
>
> Really necessary?
No. It was just an IntelliJ suggestion. I have no preference.
> src/java.base/share/classes/java/security/SecureRandom.java line 905:
>
>> 903: private static final Pattern pattern =
>> 904: Pattern.compile(
>> 905: "\\s*([\\S&&[^:,]]*)(:([\\S&&[^,]]*))?\\s*(,(.*))?");
>
> Don't quite understand this change. Can you explain?
The author must have thought `:` and `,` were metacharacters that needed to be
escaped to turn them into ordinary characters. I verified with a small java
program that` \:` and` \,` are equivalent to `;` and `,`.
The IntelliJ complaint is "Redundant character escape `'\:'`".
> src/java.base/share/classes/java/security/UnresolvedPermission.java line 369:
>
>> 367: this.certs != null && that.certs == null ||
>> 368: this.certs != null &&
>> 369:this.certs.length != that.certs.length) {
>
> I see you keep a lot of parentheses in other places. For example, `(!r)`.
This is an IntelliJ suggestion. I don't know why it complains about some cases
and ignores others. Perhaps it has something to do with readability.
> src/java.base/share/classes/java/security/cert/CertPathValidator.java line
> 335:
>
>> 333: String cpvtype =
>> 334: AccessController.doPrivileged((PrivilegedAction) ()
>> ->
>> 335: Security.getProperty(CPV_TYPE));
>
> See too many, maybe we need a dedicated method in
> `sun.security.util.SecurityProperties`?
A privilegedGetProperty method? That would be the subject of a new bug I think.
-
PR: https://git.openjdk.org/jdk/pull/8319
Re: RFR: JDK-8288207: Enhance MalformedURLException in Uri.parseCompat
On Fri, 10 Jun 2022 12:16:17 GMT, Matthias Baesken wrote: > When trying to construct an LdapURL object with a bad input string (in this > example the _ in ad_jbs is causing issues), and not using > the backward compatibility flag -Dcom.sun.jndi.ldapURLParsing="legacy" we run > into the exception below : > > import com.sun.jndi.ldap.LdapURL; > > String url = "ldap://ad_jbs.ttt.net:389/xyz;; // bad input string containing _ > LdapURL ldapUrl = new LdapURL(url); > > > java --add-opens java.naming/com.sun.jndi.ldap=ALL-UNNAMED LdapParseUrlTest > Exception in thread "main" javax.naming.NamingException: Cannot parse url: > ldap://ad_jbs.ttt.net:389/xyz [Root exception is > java.net.MalformedURLException: unsupported authority: ad_jbs.ttt.net:389] > at java.naming/com.sun.jndi.ldap.LdapURL.(LdapURL.java:115) > at LdapParseUrlTest.main(LdapParseUrlTest.java:9) > Caused by: java.net.MalformedURLException: unsupported authority: > ad_jbs.ttt.net:389 > at java.naming/com.sun.jndi.toolkit.url.Uri.parseCompat(Uri.java:367) > at java.naming/com.sun.jndi.toolkit.url.Uri.parse(Uri.java:230) > at java.naming/com.sun.jndi.toolkit.url.Uri.init(Uri.java:174) > at java.naming/com.sun.jndi.ldap.LdapURL.(LdapURL.java:105) > > I would like to add the host and port info to the exception (in the example > it is host:port of URI:null:-1] ) so that it is directly visible that the > input caused the construction of a URI > with "special"/problematic host and port values. I'll take a look from the security side but may need a few days to review and possibly collaborate with others if I have concerns. - PR: https://git.openjdk.org/jdk/pull/9126
Re: RFR: 8286779: javax.crypto.CryptoPolicyParser#isConsistent always returns 'true' [v3]
On Fri, 10 Jun 2022 16:19:05 GMT, Hai-May Chao wrote: >> Please review a small fix in CryptoPolicyParser class that it should not >> pass “processedPermissions” parameter by value. >> Ran MACH5 tier1 and tier2 without failures. > > Hai-May Chao has updated the pull request incrementally with one additional > commit since the last revision: > > Fixed copyright test/jdk/javax/crypto/CryptoPermissions/InconsistentEntries.java line 38: > 36: import java.security.Security; > 37: > 38: // This is a manual test to test a custom “default_local.policy" > containing inconsistent entries Thanks for the instructions. For manual test, I would rather have these as a System.out on test start than a comment here. This helps engineers who look at jtr file for failure analysis as they generally won't refer test source code to understand missing steps. - PR: https://git.openjdk.org/jdk/pull/8985
Integrated: 8224768: Test ActalisCA.java fails
On Thu, 9 Jun 2022 00:33:27 GMT, Rajan Halade wrote: > Updated with new test artifacts from CA. This pull request has now been integrated. Changeset: c6dd2ab9 Author:Rajan Halade URL: https://git.openjdk.org/jdk/commit/c6dd2ab9d72298b1e25ee811b1e200f6a0fdc933 Stats: 198 lines in 2 files changed: 8 ins; 38 del; 152 mod 8224768: Test ActalisCA.java fails Reviewed-by: mullan - PR: https://git.openjdk.org/jdk/pull/9097
Re: RFR: 8224768: Test ActalisCA.java fails [v2]
On Fri, 10 Jun 2022 15:09:58 GMT, Rajan Halade wrote: >> Updated with new test artifacts from CA. > > Rajan Halade has updated the pull request incrementally with one additional > commit since the last revision: > > Adjust hard wrap to 90 chars Marked as reviewed by mullan (Reviewer). - PR: https://git.openjdk.org/jdk/pull/9097
Re: RFR: 8286779: javax.crypto.CryptoPolicyParser#isConsistent always returns 'true' [v3]
> Please review a small fix in CryptoPolicyParser class that it should not pass > “processedPermissions” parameter by value. > Ran MACH5 tier1 and tier2 without failures. Hai-May Chao has updated the pull request incrementally with one additional commit since the last revision: Fixed copyright - Changes: - all: https://git.openjdk.org/jdk/pull/8985/files - new: https://git.openjdk.org/jdk/pull/8985/files/d8950465..2e8f5d95 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8985=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8985=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/8985.diff Fetch: git fetch https://git.openjdk.org/jdk pull/8985/head:pull/8985 PR: https://git.openjdk.org/jdk/pull/8985
Re: RFR: 8286779: javax.crypto.CryptoPolicyParser#isConsistent always returns 'true' [v2]
On Thu, 9 Jun 2022 22:54:20 GMT, Hai-May Chao wrote:
>> src/java.base/share/classes/javax/crypto/CryptoPolicyParser.java line 202:
>>
>>> 200: if (!processedPermissions.isEmpty()) {
>>> 201: throw new ParsingException(st.lineno(), "Inconsistent
>>> policy");
>>> 202: }
>>
>> Instead of setting the `allPermEntryFound` flag, what if you instead put an
>> entry for `CryptoAllPermission.ALG_NAME` in `processedPermissions` here.
>> Then if there are more entries after this, I think `isConsistent` will catch
>> it in the following code:
>>
>>
>> if (processedPermissions.containsKey(CryptoAllPermission.ALG_NAME)) {
>> return false;
>> }
>
> Yes, with the `allPermEntryFound` flag, the current fix would not require to
> put the `javax.crypto.CryptoAllPermission` entry in `processedPermissions`.
> So `processedPermissions` will be used to keep
> `javax.crypto.CryptoPermission` entries and is updated by `isConsistent()`,
> and no need to deal with `javax.crypto.CryptoAllPermission` entry. I’d like
> to keep it as-is if there is no objection.
Sure, I think that's reasonable.
-
PR: https://git.openjdk.org/jdk/pull/8985
Re: RFR: 8286779: javax.crypto.CryptoPolicyParser#isConsistent always returns 'true' [v2]
On Tue, 7 Jun 2022 20:52:33 GMT, Hai-May Chao wrote: >> Please review a small fix in CryptoPolicyParser class that it should not >> pass “processedPermissions” parameter by value. >> Ran MACH5 tier1 and tier2 without failures. > > Hai-May Chao has updated the pull request incrementally with two additional > commits since the last revision: > > - Inconsistent entries test > - Inconsistent entries test Marked as reviewed by mullan (Reviewer). test/jdk/javax/crypto/CryptoPermissions/InconsistentEntries.java line 2: > 1: /* > 2: * Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved. Update year to 2022. - PR: https://git.openjdk.org/jdk/pull/8985
Re: RFR: 8224768: Test ActalisCA.java fails [v2]
> Updated with new test artifacts from CA. Rajan Halade has updated the pull request incrementally with one additional commit since the last revision: Adjust hard wrap to 90 chars - Changes: - all: https://git.openjdk.org/jdk/pull/9097/files - new: https://git.openjdk.org/jdk/pull/9097/files/d800a30d..5bb68432 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=9097=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=9097=00-01 Stats: 18 lines in 1 file changed: 0 ins; 3 del; 15 mod Patch: https://git.openjdk.org/jdk/pull/9097.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9097/head:pull/9097 PR: https://git.openjdk.org/jdk/pull/9097
Integrated: 8288132: Update test artifacts in QuoVadis CA interop tests
On Thu, 9 Jun 2022 17:31:29 GMT, Rajan Halade wrote: > Updated test artifacts. Test will continue to fail intermittently with what > appears to be issue in CA infra. JDK-8277855 will track it. This pull request has now been integrated. Changeset: 3ee1e605 Author:Rajan Halade URL: https://git.openjdk.org/jdk/commit/3ee1e60595171be0dd8bda47d96e0a1268cdc461 Stats: 191 lines in 1 file changed: 21 ins; 0 del; 170 mod 8288132: Update test artifacts in QuoVadis CA interop tests Reviewed-by: mullan - PR: https://git.openjdk.org/jdk/pull/9110
Re: RFR: 8288132: Update test artifacts in QuoVadis CA interop tests [v2]
> Updated test artifacts. Test will continue to fail intermittently with what > appears to be issue in CA infra. JDK-8277855 will track it. Rajan Halade has updated the pull request incrementally with one additional commit since the last revision: Adjust hard wrap to 90 chars - Changes: - all: https://git.openjdk.org/jdk/pull/9110/files - new: https://git.openjdk.org/jdk/pull/9110/files/c21d7fb2..74159299 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=9110=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=9110=00-01 Stats: 42 lines in 1 file changed: 0 ins; 15 del; 27 mod Patch: https://git.openjdk.org/jdk/pull/9110.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9110/head:pull/9110 PR: https://git.openjdk.org/jdk/pull/9110
Integrated: 8271838: AmazonCA.java interop test fails
On Thu, 9 Jun 2022 18:59:36 GMT, Rajan Halade wrote: > Updated test certificates to new from CA. I did a loop run of this test and > don't see the intermittent failure anymore. This pull request has now been integrated. Changeset: 512db0ff Author:Rajan Halade URL: https://git.openjdk.org/jdk/commit/512db0ff31a0a1a2bd8805964ba3d06e2cbfb9e9 Stats: 245 lines in 1 file changed: 0 ins; 51 del; 194 mod 8271838: AmazonCA.java interop test fails Reviewed-by: mullan - PR: https://git.openjdk.org/jdk/pull/9111
Re: RFR: 8271838: AmazonCA.java interop test fails [v2]
> Updated test certificates to new from CA. I did a loop run of this test and > don't see the intermittent failure anymore. Rajan Halade has updated the pull request incrementally with one additional commit since the last revision: adjust hard wrap to 90 chars - Changes: - all: https://git.openjdk.org/jdk/pull/9111/files - new: https://git.openjdk.org/jdk/pull/9111/files/4a248529..592679bd Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=9111=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=9111=00-01 Stats: 16 lines in 1 file changed: 0 ins; 8 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/9111.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9111/head:pull/9111 PR: https://git.openjdk.org/jdk/pull/9111
Re: RFR: 8288132: Update test artifacts in QuoVadis CA interop tests
On Thu, 9 Jun 2022 17:31:29 GMT, Rajan Halade wrote: > Updated test artifacts. Test will continue to fail intermittently with what > appears to be issue in CA infra. JDK-8277855 will track it. Marked as reviewed by mullan (Reviewer). test/jdk/security/infra/java/security/cert/CertPathValidator/certification/QuoVadisCA.java line 62: > 60: > 61: // Owner: CN=DigiCert QuoVadis TLS ICA QV Root CA 1 G3, O="DigiCert, > 62: // Inc", C=US Can you put the "Inc", C=US" at the end of the previous line instead of on a new line, or alternatively indent it so it is under "CN:"? I think it would be more readable. Same comment applies to all the test certificates. test/jdk/security/infra/java/security/cert/CertPathValidator/certification/QuoVadisCA.java line 66: > 64: // Serial number: 2837d5c3c2b57294becf99afe8bbdcd1bb0b20f1 > 65: // Valid from: Wed Jan 06 12:50:51 PST 2021 until: Sat Jan 04 12:50:51 > 66: // PST 2031 Can you put the "PST 2031" at the end of the previous line instead of on a new line? I think it would be more readable. Same comment applies to all the test certificates. - PR: https://git.openjdk.org/jdk/pull/9110
Re: RFR: JDK-8288207: Enhance MalformedURLException in Uri.parseCompat
On Fri, 10 Jun 2022 12:16:17 GMT, Matthias Baesken wrote:
> When trying to construct an LdapURL object with a bad input string (in this
> example the _ in ad_jbs is causing issues), and not using
> the backward compatibility flag -Dcom.sun.jndi.ldapURLParsing="legacy" we run
> into the exception below :
>
> import com.sun.jndi.ldap.LdapURL;
>
> String url = "ldap://ad_jbs.ttt.net:389/xyz;; // bad input string containing _
> LdapURL ldapUrl = new LdapURL(url);
>
>
> java --add-opens java.naming/com.sun.jndi.ldap=ALL-UNNAMED LdapParseUrlTest
> Exception in thread "main" javax.naming.NamingException: Cannot parse url:
> ldap://ad_jbs.ttt.net:389/xyz [Root exception is
> java.net.MalformedURLException: unsupported authority: ad_jbs.ttt.net:389]
> at java.naming/com.sun.jndi.ldap.LdapURL.(LdapURL.java:115)
> at LdapParseUrlTest.main(LdapParseUrlTest.java:9)
> Caused by: java.net.MalformedURLException: unsupported authority:
> ad_jbs.ttt.net:389
> at java.naming/com.sun.jndi.toolkit.url.Uri.parseCompat(Uri.java:367)
> at java.naming/com.sun.jndi.toolkit.url.Uri.parse(Uri.java:230)
> at java.naming/com.sun.jndi.toolkit.url.Uri.init(Uri.java:174)
> at java.naming/com.sun.jndi.ldap.LdapURL.(LdapURL.java:105)
>
> I would like to add the host and port info to the exception (in the example
> it is host:port of URI:null:-1] ) so that it is directly visible that the
> input caused the construction of a URI
> with "special"/problematic host and port values.
`URISyntaxException`/`MalformedURLException` usually contains the whole URL -
so in this case, because we're parsing a URL, I believe the added information
would not leak more sensitive data - especially since I'd expect URI.getHost()
to be always `null` and `URI.getPort()` to be always `-1` in this case.
That is - this exception is thrown when the authority is parsed as a reg_name,
as opposed to server-base, because the provided host name (or what looks like a
host name) contains a character that is not allowed by java.net.URI in a host
name.
jshell> URI.create("ldap://a_b.com:389/foo;);
$1 ==> ldap://a_b.com:389/foo
jshell> $1.getAuthority()
$2 ==> "a_b.com:389"
jshell> $1.getHost()
$3 ==> null
As a point of comparison, here is what URISyntaxException looks like if the
authority contains a character which is not legal at all in authority:
jshell> new URI("ldap://a_%b.com:389/foo;);
| Exception java.net.URISyntaxException: Malformed escape pair at index 9:
ldap://a_%b.com:389/foo
|at URI$Parser.fail (URI.java:2973)
I agree we should wait for someone from security-dev to chime in though.
I might question whether the added "null:-1" information is really helpful, or
just as confusing however.
-
PR: https://git.openjdk.org/jdk/pull/9126
Re: RFR: JDK-8288207: Enhance MalformedURLException in Uri.parseCompat
On Fri, 10 Jun 2022 13:41:48 GMT, Matthias Baesken wrote:
> Hi Alan , sure we could use something like the already existing hostInfo of
> property jdk.includeInException private static final boolean
> enhancedExceptionText = SecurityProperties.includedInExceptions("hostInfo");
> and make the enhancement optional/switchable this way. On the other hand we
> already print the url (_**Cannot parse url: ldap://ad_jbs.ttt.net:389/xyz**_
> ) in the existing exception text so I wonder what additional problem the
> added info would bring? That's why I did not use the property so far. But if
> you think there could be special cases were it would be problematic to have
> the enhancement, I'll add the usage of the property.
This is a security sensitive area and not possible to discuss all issues in JBS
or in this PR. If this code is changed then it will require someone from
security-dev to review.
-
PR: https://git.openjdk.org/jdk/pull/9126
Re: RFR: JDK-8288207: Enhance MalformedURLException in Uri.parseCompat
On Fri, 10 Jun 2022 13:15:11 GMT, Alan Bateman wrote:
> We have to be cautious about leaking security sensitive configuration in
> exception messages. Can you look at the security property
> jdk.includeInException (conf/security/java.security) and usages in the JDK
> for ideas on how this might be implemented as opt-in?
Hi Alan , sure we could use something like the already existing hostInfo of
property jdk.includeInException
private static final boolean enhancedExceptionText =
SecurityProperties.includedInExceptions("hostInfo");
and make the enhancement optional/switchable this way.
On the other hand we already print the url (_**Cannot parse url:
ldap://ad_jbs.ttt.net:389/xyz**_ ) in the existing exception text so I wonder
what additional problem the added info would bring? That's why I did not use
the property so far.
But if you think there could be special cases were it would be problematic to
have the enhancement, I'll add the usage of the property.
-
PR: https://git.openjdk.org/jdk/pull/9126
Re: RFR: 8271838: AmazonCA.java interop test fails
On Thu, 9 Jun 2022 18:59:36 GMT, Rajan Halade wrote: > Updated test certificates to new from CA. I did a loop run of this test and > don't see the intermittent failure anymore. Marked as reviewed by mullan (Reviewer). test/jdk/security/infra/java/security/cert/CertPathValidator/certification/AmazonCA.java line 107: > 105: // Serial number: 75a5dd4b767bedc94a4239da65ed9dfef8218 > 106: // Valid from: Fri Dec 17 12:21:50 PST 2021 until: Tue Jan 17 > 12:21:50 > 107: // PST 2023 Can you put the "PST 2023" at the end of the previous line instead of on a new line? I think it would be more readable. Same comment applies to all the test certificates. - PR: https://git.openjdk.org/jdk/pull/9111
Re: RFR: JDK-8288207: Enhance MalformedURLException in Uri.parseCompat
On Fri, 10 Jun 2022 12:16:17 GMT, Matthias Baesken wrote: > When trying to construct an LdapURL object with a bad input string (in this > example the _ in ad_jbs is causing issues), and not using > the backward compatibility flag -Dcom.sun.jndi.ldapURLParsing="legacy" we run > into the exception below : > > import com.sun.jndi.ldap.LdapURL; > > String url = "ldap://ad_jbs.ttt.net:389/xyz;; // bad input string containing _ > LdapURL ldapUrl = new LdapURL(url); > > > java --add-opens java.naming/com.sun.jndi.ldap=ALL-UNNAMED LdapParseUrlTest > Exception in thread "main" javax.naming.NamingException: Cannot parse url: > ldap://ad_jbs.ttt.net:389/xyz [Root exception is > java.net.MalformedURLException: unsupported authority: ad_jbs.ttt.net:389] > at java.naming/com.sun.jndi.ldap.LdapURL.(LdapURL.java:115) > at LdapParseUrlTest.main(LdapParseUrlTest.java:9) > Caused by: java.net.MalformedURLException: unsupported authority: > ad_jbs.ttt.net:389 > at java.naming/com.sun.jndi.toolkit.url.Uri.parseCompat(Uri.java:367) > at java.naming/com.sun.jndi.toolkit.url.Uri.parse(Uri.java:230) > at java.naming/com.sun.jndi.toolkit.url.Uri.init(Uri.java:174) > at java.naming/com.sun.jndi.ldap.LdapURL.(LdapURL.java:105) > > I would like to add the host and port info to the exception (in the example > it is host:port of URI:null:-1] ) so that it is directly visible that the > input caused the construction of a URI > with "special"/problematic host and port values. We have to be cautious about leaking security sensitive configuration in exception messages. Can you look at the security property jdk.includeInException (conf/security/java.security) and usages in the JDK for ideas on how this might be implemented as opt-in? - PR: https://git.openjdk.org/jdk/pull/9126
Re: RFR: 8287696: Avoid redundant Hashtable.containsKey call in JarVerifier.doneWithMeta
On Sat, 28 May 2022 12:00:00 GMT, Andrey Turbanov wrote: > Hashtable doesn't allow `null` values. So, instead of pair > `containsKey`/`remove` calls, we can directly call `remove` and then compare > result with `null`. > https://github.com/openjdk/jdk/blob/2c461acfebd28fe5ef62805cbb004f91a3b18f08/src/java.base/share/classes/java/util/jar/JarVerifier.java#L433-L436 The changes to doneWithMeta() seem reasonable and the other changes remove unused code so look OK to me - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.org/jdk/pull/8935
Redefined EXIT_FAILURE macro in /java.security.jgss/windows/native/libw2k_lsa_auth/NativeCreds.c
In NativeCreds.c there seems to be a redefined macro for EXIT_FAILURE (line
51)
#undef LSA_SUCCESS
#define LSA_SUCCESS(Status) ((Status) >= 0)
#define EXIT_FAILURE -1 // mdu <-
As far as I can see, it's a redundant definition not used anywhere other
than in a call to ExitProcess that's been commented out on line 826:
if (0 == dwRes) {
printf("LSA: FormatMessage failed with %d\n", GetLastError());
// ExitProcess(EXIT_FAILURE);
}
This definition is also not correct, since EXIT_FAILURE on Windows is
defined as 1, not -1. Would it be safe to remove this unused macro
definition?
best regards,
Julian
