Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]
On Thu, 27 May 2021 17:42:56 GMT, Phil Race wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> update FtpClient.java
>
> src/java.desktop/share/classes/java/awt/Component.java line 630:
>
>> 628: }
>> 629:
>> 630: @SuppressWarnings("removal")
>
> I'm confused. I thought the reason this wasn't done in the JEP implementation
> PR is because of refactoring
> that was needed because of the usage in this static block and you could not
> apply the annotation here.
> Yet it seems you are doing exactly what was supposed to be impossible with no
> refactoring.
> Can you explain ?
There *is* a tiny refactoring here: a new variable `s2` is introduced so the
2nd `doPrivileged` call on line 636 is now also in a declaration statement (for
`s2`) and therefore annotatable. Without this I cannot add the annotation on
line 635.
-
PR: https://git.openjdk.java.net/jdk/pull/4138
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v5]
On Thu, 27 May 2021 20:16:25 GMT, Weijun Wang wrote: >> Please review this implementation of [JEP >> 411](https://openjdk.java.net/jeps/411). >> >> The code change is divided into 3 commits. Please review them one by one. >> >> 1. >> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 >> The essential change for this JEP, including the `@Deprecate` annotations >> and spec change. It also update the default value of the >> `java.security.manager` system property to "disallow", and necessary test >> change following this update. >> 2. >> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 >> Manual changes to several files so that the next commit can be generated >> programatically. >> 3. >> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 >> Automatic changes to other source files to avoid javac warnings on >> deprecation for removal >> >> The 1st and 2nd commits should be reviewed carefully. The 3rd one is >> generated programmatically, see the comment below for more details. If you >> are only interested in a portion of the 3rd commit and would like to review >> it as a separate file, please comment here and I'll generate an individual >> webrev. >> >> Due to the size of this PR, no attempt is made to update copyright years for >> any file to minimize unnecessary merge conflict. >> >> Furthermore, since the default value of `java.security.manager` system >> property is now "disallow", most of the tests calling >> `System.setSecurityManager()` need to launched with >> `-Djava.security.manager=allow`. This is covered in a different PR at >> https://github.com/openjdk/jdk/pull/4071. >> >> Update: the deprecation annotations and javadoc tags, build, compiler, >> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are >> reviewed. Rest are 2d, awt, beans, sound, and swing. > > Weijun Wang has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains eight commits: > > - move one annotation to new method > - merge from master, two files removed, one needs merge > - keep only one systemProperty tag > - fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java > - feedback from Sean, Phil and Alan > - add supresswarnings annotations automatically > - manual change before automatic annotating > - 8266459: Implement JEP 411: Deprecate the Security Manager for Removal Two files were removed by JEP 403 and JEP 407, respectively. One method in `XMLSchemaFactory.java` got [its own](https://github.com/openjdk/jdk/commit/8c4719a58834dddcea39d69b199abf1aabf780e2#diff-593a224979eaff03e2a3df1863fcaf865364a31a2212cc0d1fe67a8458057857R429) `@SuppressWarnings` and have to be merged with the one here. Another file `ResourceBundle.java` had a portion of a method extracted into a [new method](https://github.com/openjdk/jdk/commit/a4c46e1e4f4f2f05c8002b2af683a390fc46b424#diff-59caf1a68085064b4b3eb4f6e33e440bb85ea93719f34660970e2d4eaf8ce469R3175) and the annotation must be moved there. - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v5]
> Please review this implementation of [JEP > 411](https://openjdk.java.net/jeps/411). > > The code change is divided into 3 commits. Please review them one by one. > > 1. > https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 > The essential change for this JEP, including the `@Deprecate` annotations > and spec change. It also update the default value of the > `java.security.manager` system property to "disallow", and necessary test > change following this update. > 2. > https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 > Manual changes to several files so that the next commit can be generated > programatically. > 3. > https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 > Automatic changes to other source files to avoid javac warnings on > deprecation for removal > > The 1st and 2nd commits should be reviewed carefully. The 3rd one is > generated programmatically, see the comment below for more details. If you > are only interested in a portion of the 3rd commit and would like to review > it as a separate file, please comment here and I'll generate an individual > webrev. > > Due to the size of this PR, no attempt is made to update copyright years for > any file to minimize unnecessary merge conflict. > > Furthermore, since the default value of `java.security.manager` system > property is now "disallow", most of the tests calling > `System.setSecurityManager()` need to launched with > `-Djava.security.manager=allow`. This is covered in a different PR at > https://github.com/openjdk/jdk/pull/4071. > > Update: the deprecation annotations and javadoc tags, build, compiler, > core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are > reviewed. Rest are 2d, awt, beans, sound, and swing. Weijun Wang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits: - move one annotation to new method - merge from master, two files removed, one needs merge - keep only one systemProperty tag - fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java - feedback from Sean, Phil and Alan - add supresswarnings annotations automatically - manual change before automatic annotating - 8266459: Implement JEP 411: Deprecate the Security Manager for Removal - Changes: https://git.openjdk.java.net/jdk/pull/4073/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4073=04 Stats: 2022 lines in 825 files changed: 1884 ins; 10 del; 128 mod Patch: https://git.openjdk.java.net/jdk/pull/4073.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4073/head:pull/4073 PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8267543: Post JEP 411 refactoring: security [v2]
On Tue, 25 May 2021 21:25:54 GMT, Sean Mullan wrote: > Update copyright dates to 2021, if applicable. Will do. Since this PR can only be integrated after its parent PR #4073 is integrated. I'll delay the copyright update by then. - PR: https://git.openjdk.java.net/jdk/pull/4172
Re: RFR: 8267543: Post JEP 411 refactoring: security [v2]
> For all modified files in #4073 having "security", "crypto", or "ssl" in > their names. Codes are refactored to bring a `@SuppressWarning` annotation > nearer to the deprecated API usage and also shrink the size of its target. > > I'll add a copyright year update commit before integration. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: one more change - Changes: - all: https://git.openjdk.java.net/jdk/pull/4172/files - new: https://git.openjdk.java.net/jdk/pull/4172/files/b22fd7d7..176f44e8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4172=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4172=00-01 Stats: 5 lines in 1 file changed: 3 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4172.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4172/head:pull/4172 PR: https://git.openjdk.java.net/jdk/pull/4172
Re: RFR: 8267543: Post JEP 411 refactoring: security [v2]
On Tue, 25 May 2021 13:44:30 GMT, Sean Mullan wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> one more change
>
> src/java.security.jgss/share/classes/sun/security/jgss/krb5/Krb5Context.java
> line 709:
>
>> 707: }
>> 708: if (GSSUtil.useSubjectCredsOnly(caller)) {
>> 709: @SuppressWarnings("removal")
>
> It looks like you missed removing the @SuppressWarnings("removal") from the
> initSecContext method above.
Oops. Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/4172
RFR: 8267543: Post JEP 411 refactoring: security
For all modified files in #4073 having "security", "crypto", or "ssl" in their names. Codes are refactored to bring a `@SuppressWarning` annotation nearer to the deprecated API usage and also shrink the size of its target. - Depends on: https://git.openjdk.java.net/jdk/pull/4073 Commit messages: - 8267543: Post JEP 411 refactoring: security Changes: https://git.openjdk.java.net/jdk/pull/4172/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4172=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8267543 Stats: 111 lines in 19 files changed: 34 ins; 35 del; 42 mod Patch: https://git.openjdk.java.net/jdk/pull/4172.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4172/head:pull/4172 PR: https://git.openjdk.java.net/jdk/pull/4172
Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]
On Mon, 24 May 2021 09:00:07 GMT, Daniel Fuchs wrote: > Thanks for taking in my suggestions for FtpClient. I have also reviewed the > changes to java.util.logging and JMX. I wish there was a way to handle > doPrivileged returning void more nicely. Maybe component maintainers will be > able to deal with them on a case-by-case basis later on. Assigning to a useless "dummy" variable looks ugly. Extracting the call to a method might make it faraway from its caller. In L114 of `FtpClient.java` I managed to invent a return value and I thought it was perfect. But you said it's "a bit strange". :-( - PR: https://git.openjdk.java.net/jdk/pull/4138
Integrated: 8267184: Add -Djava.security.manager=allow to tests calling System.setSecurityManager
On Mon, 17 May 2021 17:51:36 GMT, Weijun Wang wrote:
> Please review the test changes for [JEP
> 411](https://openjdk.java.net/jeps/411).
>
> With JEP 411 and the default value of `-Djava.security.manager` becoming
> `disallow`, tests calling `System.setSecurityManager()` need
> `-Djava.security.manager=allow` when launched. This PR covers such changes
> for tier1 to tier3 (except for the JCK tests).
>
> To make it easier to focus your review on the tests in your area, this PR is
> divided into multiple commits for different areas (~~serviceability~~,
> ~~hotspot-compiler~~, ~~i18n~~, ~~rmi~~, ~~javadoc~~, swing, 2d,
> ~~security~~, ~~hotspot-runtime~~, ~~nio~~, ~~xml~~, ~~beans~~,
> ~~core-libs~~, ~~net~~, ~~compiler~~, ~~hotspot-gc~~). Mostly the rule is the
> same as how Skara adds labels, but there are several small tweaks:
>
> 1. When a file is covered by multiple labels, only one is chosen. I make my
> best to avoid putting too many tests into `core-libs`. If a file is covered
> by `core-libs` and another label, I categorized it into the other label.
> 2. If a file is in `core-libs` but contains `/xml/` in its name, it's in the
> `xml` commit.
> 3. If a file is in `core-libs` but contains `/rmi/` in its name, it's in the
> `rmi` commit.
> 4. One file not covered by any label --
> `test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java` -- is in
> the `swing` commit.
>
> Due to the size of this PR, no attempt is made to update copyright years for
> all files to minimize unnecessary merge conflict.
>
> Please note that this PR can be integrated before the source changes for JEP
> 411, as the possible values of this system property was already defined long
> time ago in JDK 9.
>
> Most of the change in this PR is a simple adding of
> `-Djava.security.manager=allow` to the `@run main/othervm` line. Sometimes it
> was not `othervm` and we add one. Sometimes there's no `@run` at all and we
> add the line.
>
> There are several tests that launch another Java process that needs to call
> the `System.setSecurityManager()` method, and the system property is added to
> `ProcessBuilder`, `ProcessTools`, or the java command line (if the test is a
> shell test).
>
> 3 langtools tests are added into problem list due to
> [JDK-8265611](https://bugs.openjdk.java.net/browse/JDK-8265611).
>
> 2 SQL tests are moved because they need different options on the `@run` line
> but they are inside a directory that has a `TEST.properties`:
>
> rename test/jdk/java/sql/{testng/test/sql/othervm =>
> permissionTests}/DriverManagerPermissionsTests.java (93%)
> rename test/jdk/javax/sql/{testng/test/rowset/spi =>
> permissionTests}/SyncFactoryPermissionsTests.java (95%)
> ```
>
> The source change for JEP 411 is at https://github.com/openjdk/jdk/pull/4073.
This pull request has now been integrated.
Changeset: 640a2afd
Author:Weijun Wang
URL:
https://git.openjdk.java.net/jdk/commit/640a2afda36857410b7abf398af81e35430a62e7
Stats: 1028 lines in 852 files changed: 252 ins; 9 del; 767 mod
8267184: Add -Djava.security.manager=allow to tests calling
System.setSecurityManager
Co-authored-by: Lance Andersen
Co-authored-by: Weijun Wang
Reviewed-by: dholmes, alanb, dfuchs, mchung, mullan, prr
-
PR: https://git.openjdk.java.net/jdk/pull/4071
Integrated: 8267584: The java.security.krb5.realm system property only needs to be defined once
On Sun, 23 May 2021 20:42:24 GMT, Weijun Wang wrote:
> A system property only needs to be put in a `{@systemProperty}` tag where
> it's first defined. For this one, it's
> https://github.com/openjdk/jdk/blob/cf21c5ef116c136f09ac5be0d68f02553d0c7a70/src/java.security.jgss/share/classes/javax/security/auth/kerberos/package-info.java#L41.
This pull request has now been integrated.
Changeset: 838a0071
Author: Weijun Wang
URL:
https://git.openjdk.java.net/jdk/commit/838a0071030e9c8b9ab57df39a4e0384d433a2bc
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
8267584: The java.security.krb5.realm system property only needs to be defined
once
Reviewed-by: mullan
-
PR: https://git.openjdk.java.net/jdk/pull/4159
Re: RFR: 8266400: importkeystore fails to a password less pkcs12 keystore
On Wed, 19 May 2021 19:01:21 GMT, Hai-May Chao wrote: > Please review the fix to address keytool -importkeystore failure when > importing to a password-less PKCS12 keystore. Marked as reviewed by weijun (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4119
Re: RFR: 8267184: Add -Djava.security.manager=allow to tests calling System.setSecurityManager [v4]
> Please review the test changes for [JEP
> 411](https://openjdk.java.net/jeps/411).
>
> With JEP 411 and the default value of `-Djava.security.manager` becoming
> `disallow`, tests calling `System.setSecurityManager()` need
> `-Djava.security.manager=allow` when launched. This PR covers such changes
> for tier1 to tier3 (except for the JCK tests).
>
> To make it easier to focus your review on the tests in your area, this PR is
> divided into multiple commits for different areas (~~serviceability~~,
> ~~hotspot-compiler~~, ~~i18n~~, ~~rmi~~, ~~javadoc~~, swing, 2d,
> ~~security~~, ~~hotspot-runtime~~, ~~nio~~, ~~xml~~, ~~beans~~,
> ~~core-libs~~, ~~net~~, ~~compiler~~, ~~hotspot-gc~~). Mostly the rule is the
> same as how Skara adds labels, but there are several small tweaks:
>
> 1. When a file is covered by multiple labels, only one is chosen. I make my
> best to avoid putting too many tests into `core-libs`. If a file is covered
> by `core-libs` and another label, I categorized it into the other label.
> 2. If a file is in `core-libs` but contains `/xml/` in its name, it's in the
> `xml` commit.
> 3. If a file is in `core-libs` but contains `/rmi/` in its name, it's in the
> `rmi` commit.
> 4. One file not covered by any label --
> `test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java` -- is in
> the `swing` commit.
>
> Due to the size of this PR, no attempt is made to update copyright years for
> all files to minimize unnecessary merge conflict.
>
> Please note that this PR can be integrated before the source changes for JEP
> 411, as the possible values of this system property was already defined long
> time ago in JDK 9.
>
> Most of the change in this PR is a simple adding of
> `-Djava.security.manager=allow` to the `@run main/othervm` line. Sometimes it
> was not `othervm` and we add one. Sometimes there's no `@run` at all and we
> add the line.
>
> There are several tests that launch another Java process that needs to call
> the `System.setSecurityManager()` method, and the system property is added to
> `ProcessBuilder`, `ProcessTools`, or the java command line (if the test is a
> shell test).
>
> 3 langtools tests are added into problem list due to
> [JDK-8265611](https://bugs.openjdk.java.net/browse/JDK-8265611).
>
> 2 SQL tests are moved because they need different options on the `@run` line
> but they are inside a directory that has a `TEST.properties`:
>
> rename test/jdk/java/sql/{testng/test/sql/othervm =>
> permissionTests}/DriverManagerPermissionsTests.java (93%)
> rename test/jdk/javax/sql/{testng/test/rowset/spi =>
> permissionTests}/SyncFactoryPermissionsTests.java (95%)
> ```
>
> The source change for JEP 411 is at https://github.com/openjdk/jdk/pull/4073.
Weijun Wang 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 20 additional commits since the
last revision:
- Merge branch 'master' into 8267184
- feedback from Phil
reverted:
- adjust order of VM options
- test for awt
- test for hotspot-gc
- test for compiler
- test for net
- test for core-libs
- test for beans
- test for xml
- ... and 10 more: https://git.openjdk.java.net/jdk/compare/37f74de7...412264a0
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4071/files
- new: https://git.openjdk.java.net/jdk/pull/4071/files/9a3ec578..412264a0
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=4071=03
- incr: https://webrevs.openjdk.java.net/?repo=jdk=4071=02-03
Stats: 12227 lines in 453 files changed: 6978 ins; 3721 del; 1528 mod
Patch: https://git.openjdk.java.net/jdk/pull/4071.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4071/head:pull/4071
PR: https://git.openjdk.java.net/jdk/pull/4071
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v4]
On Sun, 23 May 2021 16:35:43 GMT, Sean Mullan wrote:
>> src/java.base/share/classes/java/lang/SecurityManager.java line 104:
>>
>>> 102: * method will throw an {@code UnsupportedOperationException}). If the
>>> 103: * {@systemProperty java.security.manager} system property is set to
>>> the
>>> 104: * special token "{@code allow}", then a security manager will not be
>>> set at
>>
>> Can/should the `{@systemProperty ...}` tag be used more than once for a
>> given system property? I thought it should be used only once, at the place
>> where the system property is defined. Maybe @jonathan-gibbons can offer some
>> more guidance on this.
>
> Good point. I would remove the extra @systemProperty tags on lines 103, 106,
> and 113. Also, in `System.setSecurityManager` there are 3 @systemProperty
> java.security.manager tags, so we should remove those too.
New commit pushed. There is only one `@systemProperty` tag now.
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v4]
> Please review this implementation of [JEP > 411](https://openjdk.java.net/jeps/411). > > The code change is divided into 3 commits. Please review them one by one. > > 1. > https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 > The essential change for this JEP, including the `@Deprecate` annotations > and spec change. It also update the default value of the > `java.security.manager` system property to "disallow", and necessary test > change following this update. > 2. > https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 > Manual changes to several files so that the next commit can be generated > programatically. > 3. > https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 > Automatic changes to other source files to avoid javac warnings on > deprecation for removal > > The 1st and 2nd commits should be reviewed carefully. The 3rd one is > generated programmatically, see the comment below for more details. If you > are only interested in a portion of the 3rd commit and would like to review > it as a separate file, please comment here and I'll generate an individual > webrev. > > Due to the size of this PR, no attempt is made to update copyright years for > any file to minimize unnecessary merge conflict. > > Furthermore, since the default value of `java.security.manager` system > property is now "disallow", most of the tests calling > `System.setSecurityManager()` need to launched with > `-Djava.security.manager=allow`. This is covered in a different PR at > https://github.com/openjdk/jdk/pull/4071. > > Update: the deprecation annotations and javadoc tags, build, compiler, > core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are > reviewed. Rest are 2d, awt, beans, sound, and swing. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: keep only one systemProperty tag - Changes: - all: https://git.openjdk.java.net/jdk/pull/4073/files - new: https://git.openjdk.java.net/jdk/pull/4073/files/c4221b5f..1f6ff6c4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4073=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4073=02-03 Stats: 8 lines in 2 files changed: 0 ins; 0 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/4073.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4073/head:pull/4073 PR: https://git.openjdk.java.net/jdk/pull/4073
RFR: 8267584: The java.security.krb5.realm system property only needs to be defined once
A system property only needs to be put in a `{@systemProperty}` tag where it's
first defined. For this one, it's
https://github.com/openjdk/jdk/blob/cf21c5ef116c136f09ac5be0d68f02553d0c7a70/src/java.security.jgss/share/classes/javax/security/auth/kerberos/package-info.java#L41.
-
Commit messages:
- 8267584: The java.security.krb5.realm system property only needs to be
defined once
Changes: https://git.openjdk.java.net/jdk/pull/4159/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4159=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8267584
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
Patch: https://git.openjdk.java.net/jdk/pull/4159.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4159/head:pull/4159
PR: https://git.openjdk.java.net/jdk/pull/4159
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Fri, 21 May 2021 20:43:05 GMT, Phil Race wrote:
> I haven't seen any response to this comment I made a couple of days ago and I
> suspect it got missed
>
> > Weijun Wang has updated the pull request incrementally with one additional
> > commit since the last revision:
> > fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
>
> test/jdk/java/awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java line 59:
>
> > 57: ProcessCommunicator
> > 58: .executeChildProcess(Consumer.class, new
> > String[0]);
> > 59: if (!"Hello".equals(processResults.getStdOut())) {
>
> Who or what prompted this change ?
I replied right in the thread but unfortunately GitHub does not display it at
the end of page.
This is because the process is now launched with
`-Djava.security.manager=allow` (because of another change in
https://github.com/openjdk/jdk/pull/4071), and a new warning is displayed in
stderr. Therefore I switched to stdout.
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]
> The code change refactors classes that have a `SuppressWarnings("removal")`
> annotation that covers more than 50KB of code. The big annotation is often
> quite faraway from the actual deprecated API usage it is suppressing, and
> with the annotation covering too big a portion it's easy to call other
> deprecated methods without being noticed.
>
> The code change shows some common solutions to avoid such too wide
> annotations:
>
> 1. Extract calls into a method and add annotation on that method
> 2. Assign the return value of a deprecated method call to a new local
> variable and add annotation on the declaration, and then assign that value to
> the original l-value.
> 3. Put declaration and assignment into a single statement if possible.
> 4. Rewrite code to achieve #3 above.
Weijun Wang has updated the pull request incrementally with one additional
commit since the last revision:
update FtpClient.java
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4138/files
- new: https://git.openjdk.java.net/jdk/pull/4138/files/d460efb8..60d97a4c
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=4138=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk=4138=01-02
Stats: 23 lines in 1 file changed: 0 ins; 12 del; 11 mod
Patch: https://git.openjdk.java.net/jdk/pull/4138.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4138/head:pull/4138
PR: https://git.openjdk.java.net/jdk/pull/4138
Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v2]
On Fri, 21 May 2021 15:37:48 GMT, Daniel Fuchs wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> typo on windows
>
> src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 550:
>
>> 548: * @throws IOException if the connection was unsuccessful.
>> 549: */
>> 550: @SuppressWarnings("removal")
>
> Could the scope of the annotation be further reduced by applying it to the
> two places where `doPrivileged` is called in this method?
I'll probably need to assign the doPriv result on L631 to a tmp variable and
then assign it to `s`. Are you OK with this ugliness? Update: Ah, I see you
already have similar suggestion in the next comment.
-
PR: https://git.openjdk.java.net/jdk/pull/4138
Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v2]
On Fri, 21 May 2021 15:35:05 GMT, Daniel Fuchs wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> typo on windows
>
> src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 120:
>
>> 118: vals[1] =
>> Integer.getInteger("sun.net.client.defaultConnectTimeout",
>> 300_000).intValue();
>> 119: return System.getProperty("file.encoding",
>> "ISO8859_1");
>> 120: }
>
> It is a bit strange that "file.encoding" seem to get a special treatment -
> but I guess that's OK.
You might say we thus avoid wasting the return value, as much as possible.
-
PR: https://git.openjdk.java.net/jdk/pull/4138
Re: RFR: 8267184: Add -Djava.security.manager=allow to tests calling System.setSecurityManager [v3]
> Please review the test changes for [JEP
> 411](https://openjdk.java.net/jeps/411).
>
> With JEP 411 and the default value of `-Djava.security.manager` becoming
> `disallow`, tests calling `System.setSecurityManager()` need
> `-Djava.security.manager=allow` when launched. This PR covers such changes
> for tier1 to tier3 (except for the JCK tests).
>
> To make it easier to focus your review on the tests in your area, this PR is
> divided into multiple commits for different areas (~~serviceability~~,
> ~~hotspot-compiler~~, ~~i18n~~, ~~rmi~~, ~~javadoc~~, swing, 2d,
> ~~security~~, ~~hotspot-runtime~~, ~~nio~~, ~~xml~~, ~~beans~~,
> ~~core-libs~~, ~~net~~, ~~compiler~~, ~~hotspot-gc~~). Mostly the rule is the
> same as how Skara adds labels, but there are several small tweaks:
>
> 1. When a file is covered by multiple labels, only one is chosen. I make my
> best to avoid putting too many tests into `core-libs`. If a file is covered
> by `core-libs` and another label, I categorized it into the other label.
> 2. If a file is in `core-libs` but contains `/xml/` in its name, it's in the
> `xml` commit.
> 3. If a file is in `core-libs` but contains `/rmi/` in its name, it's in the
> `rmi` commit.
> 4. One file not covered by any label --
> `test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java` -- is in
> the `swing` commit.
>
> Due to the size of this PR, no attempt is made to update copyright years for
> all files to minimize unnecessary merge conflict.
>
> Please note that this PR can be integrated before the source changes for JEP
> 411, as the possible values of this system property was already defined long
> time ago in JDK 9.
>
> Most of the change in this PR is a simple adding of
> `-Djava.security.manager=allow` to the `@run main/othervm` line. Sometimes it
> was not `othervm` and we add one. Sometimes there's no `@run` at all and we
> add the line.
>
> There are several tests that launch another Java process that needs to call
> the `System.setSecurityManager()` method, and the system property is added to
> `ProcessBuilder`, `ProcessTools`, or the java command line (if the test is a
> shell test).
>
> 3 langtools tests are added into problem list due to
> [JDK-8265611](https://bugs.openjdk.java.net/browse/JDK-8265611).
>
> 2 SQL tests are moved because they need different options on the `@run` line
> but they are inside a directory that has a `TEST.properties`:
>
> rename test/jdk/java/sql/{testng/test/sql/othervm =>
> permissionTests}/DriverManagerPermissionsTests.java (93%)
> rename test/jdk/javax/sql/{testng/test/rowset/spi =>
> permissionTests}/SyncFactoryPermissionsTests.java (95%)
> ```
>
> The source change for JEP 411 is at https://github.com/openjdk/jdk/pull/4073.
Weijun Wang has updated the pull request incrementally with one additional
commit since the last revision:
feedback from Phil
reverted:
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4071/files
- new: https://git.openjdk.java.net/jdk/pull/4071/files/bfa955ad..9a3ec578
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=4071=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk=4071=01-02
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/4071.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4071/head:pull/4071
PR: https://git.openjdk.java.net/jdk/pull/4071
Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager [v2]
On Fri, 21 May 2021 18:26:48 GMT, Phil Race wrote: >> Weijun Wang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> adjust order of VM options > > test/jdk/java/awt/FontClass/CreateFont/fileaccess/FontFile.java line 3: > >> 1: /* >> 2: * Copyright (c) 2008, 2021, Oracle and/or its affiliates. All rights >> reserved. >> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. > > Probably the (c) update was meant to be on the .sh file that is actually > modified. Oops, I'll back it out. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/4071
Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v2]
> The code change refactors classes that have a `SuppressWarnings("removal")`
> annotation that covers more than 50KB of code. The big annotation is often
> quite faraway from the actual deprecated API usage it is suppressing, and
> with the annotation covering too big a portion it's easy to call other
> deprecated methods without being noticed.
>
> The code change shows some common solutions to avoid such too wide
> annotations:
>
> 1. Extract calls into a method and add annotation on that method
> 2. Assign the return value of a deprecated method call to a new local
> variable and add annotation on the declaration, and then assign that value to
> the original l-value.
> 3. Put declaration and assignment into a single statement if possible.
> 4. Rewrite code to achieve #3 above.
Weijun Wang has updated the pull request incrementally with one additional
commit since the last revision:
typo on windows
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4138/files
- new: https://git.openjdk.java.net/jdk/pull/4138/files/e3f0405a..d460efb8
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=4138=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk=4138=00-01
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/4138.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4138/head:pull/4138
PR: https://git.openjdk.java.net/jdk/pull/4138
RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K
The code change refactors classes that have a `SuppressWarnings("removal")`
annotation that covers more than 50KB of code. The big annotation is often
quite faraway from the actual deprecated API usage it is suppressing, and with
the annotation covering too big a portion it's easy to call other deprecated
methods without being noticed.
-
Depends on: https://git.openjdk.java.net/jdk/pull/4073
Commit messages:
- 8267521: Post JEP 411 refactoring: maximum covering > 50K
Changes: https://git.openjdk.java.net/jdk/pull/4138/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4138=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8267521
Stats: 226 lines in 18 files changed: 142 ins; 29 del; 55 mod
Patch: https://git.openjdk.java.net/jdk/pull/4138.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4138/head:pull/4138
PR: https://git.openjdk.java.net/jdk/pull/4138
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Thu, 20 May 2021 02:06:46 GMT, Weijun Wang wrote: >> Well .. as an enhancement (P3 or otherwise) I can see it being dropped and >> definitely if it misses the fork, >> and I don't get why you can't do it here. And if it isn't done the JEP isn't >> really ready. >> I already pasted the patch for Component.java and it took about 60 seconds >> to do that. >> Then yes, you would have to deal with the fact that now you need to reapply >> your automated tool to the file but this is just work you'd have to have >> done anyway if it was already refactored. >> >> I only *noticed* Component and Container. And stopped there to raise the >> question. How many more cases are there ? > > I can make it a bug. > > I don't want to do it here is because it involves indefinite amount of manual > work and we will see everyone having their preferences. The more time we > spend on this PR the more likely there will be merge conflict. There are just > too many files here. > > And no matter if we include that change in this PR or after it, it will be > after the automatic conversion. In fact, the data about which cases are more > worth fixing come from the automatic conversion itself. Also, as the manual > work will be done part by part, if the automatic conversion is after it, > there will be rounds and rounds of history rewriting and force push. This is > quite unfriendly to the reviewers. > > Altogether, there are 117 class-level annotations. Unfortunately, > `java.awt.Component` is the one with the biggest class -- estimated number of > bytes that the annotation covers is over 380K. In the client area, the 2nd > place is `java.awt.Container`, and then we have `sun.font.SunFontManager`, > `java.awt.Window`, and `java.awt.Toolkit` which are over 100KB, and other 25 > classes over 10KB, and other 11 classes smaller than 10KB. By converting JDK-8267432 to a bug, there is an extra benefit that we can fix it after RDP. So I'll convert it now. - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 23:50:04 GMT, Phil Race wrote: >> I just made it P3 (P4 was the default value), and I will target it to 17 >> once JEP 411 is targeted 17. But I think it's probably not a good idea to >> include it inside *this* PR. There are some middle ground where it's >> debatable if a change is worth doing (Ex: which is uglier between an >> a-liitle-faraway-annotation and a temporary variable?) so it's not obvious >> what the scope of the refactoring can be. Thus it will be divided into >> smaller sub-tasks. It's not totally impossible that part of the work will be >> delayed to next release. > > Well .. as an enhancement (P3 or otherwise) I can see it being dropped and > definitely if it misses the fork, > and I don't get why you can't do it here. And if it isn't done the JEP isn't > really ready. > I already pasted the patch for Component.java and it took about 60 seconds to > do that. > Then yes, you would have to deal with the fact that now you need to reapply > your automated tool to the file but this is just work you'd have to have done > anyway if it was already refactored. > > I only *noticed* Component and Container. And stopped there to raise the > question. How many more cases are there ? I can make it a bug. I don't want to do it here is because it involves indefinite amount of manual work and we will see everyone having their preferences. The more time we spend on this PR the more likely there will be merge conflict. There are just too many files here. And no matter if we include that change in this PR or after it, it will be after the automatic conversion. In fact, the data about which cases are more worth fixing come from the automatic conversion itself. Also, as the manual work will be done part by part, if the automatic conversion is after it, there will be rounds and rounds of history rewriting and force push. This is quite unfriendly to the reviewers. Altogether, there are 117 class-level annotations. Unfortunately, `java.awt.Component` is the one with the biggest class -- estimated number of bytes that the annotation covers is over 380K. In the client area, the 2nd place is `java.awt.Container`, and then we have `sun.font.SunFontManager`, `java.awt.Window`, and `java.awt.Toolkit` which are over 100KB, and other 25 classes over 10KB, and other 11 classes smaller than 10KB. - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 22:04:57 GMT, Phil Race wrote: >> Correct, there are ways to modify the code to make it more >> annotation-friendly. We thought about whether it's good to do it before >> adding the annotations or after it. Our decision now is to do it after >> because it will be more easy to see why it's necessary and we can take time >> to do them little by little. A new enhancement at >> https://bugs.openjdk.java.net/browse/JDK-8267432 is filed. > > I don't think it is a separate P4 enhancement (?) that someone will (maybe) > do next release. > I think it should all be taken care of as part of this proposed change. I just made it P3 (P4 was the default value), and I will target it to 17 once JEP 411 is targeted 17. But I think it's probably not a good idea to include it inside *this* PR. There are some middle ground where it's debatable if a change is worth doing (Ex: which is uglier between an a-liitle-faraway-annotation and a temporary variable?) so it's not obvious what the scope of the refactoring can be. Thus it will be divided into smaller sub-tasks. It's not totally impossible that part of the work will be delayed to next release. - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 19:31:24 GMT, Phil Race wrote:
>> This happens when a deprecated method is called inside a static block. The
>> annotation can only be added to a declaration and here it must be the whole
>> class. The call in this file is
>>
>> s = java.security.AccessController.doPrivileged(
>> new
>> GetPropertyAction("awt.image.redrawrate"));
>
> That's a sad limitation of the annotation stuff then, but I don't think that
> it is insurmountable.
> You can define a static private method to contain this and call it from the
> static initializer block.
> Much better than applying the annotation to an entire class.
>
> --- a/src/java.desktop/share/classes/java/awt/Component.java
> +++ b/src/java.desktop/share/classes/java/awt/Component.java
> @@ -618,6 +618,17 @@ public abstract class Component implements
> ImageObserver, MenuContainer,
> */
> static boolean isInc;
> static int incRate;
> +
> +private static void initIncRate() {
> +String s = java.security.AccessController.doPrivileged(
> + new
> GetPropertyAction("awt.image.incrementaldraw"));
> +isInc = (s == null || s.equals("true"));
> +
> +s = java.security.AccessController.doPrivileged(
> + new GetPropertyAction("awt.image.redrawrate"));
> +incRate = (s != null) ? Integer.parseInt(s) : 100;
> +}
> +
> static {
> /* ensure that the necessary native libraries are loaded */
> Toolkit.loadLibraries();
> @@ -625,14 +636,7 @@ public abstract class Component implements
> ImageObserver, MenuContainer,
> if (!GraphicsEnvironment.isHeadless()) {
> initIDs();
> }
> -
> -String s = java.security.AccessController.doPrivileged(
> - new
> GetPropertyAction("awt.image.incrementaldraw"));
> -isInc = (s == null || s.equals("true"));
> -
> -s = java.security.AccessController.doPrivileged(
> -new
> GetPropertyAction("awt.image.redrawrate"));
> -incRate = (s != null) ? Integer.parseInt(s) : 100;
> +initIncRate();
> }
Correct, there are ways to modify the code to make it more annotation-friendly.
We thought about whether it's good to do it before adding the annotations or
after it. Our decision now is to do it after because it will be more easy to
see why it's necessary and we can take time to do them little by little. A new
enhancement at https://bugs.openjdk.java.net/browse/JDK-8267432 is filed.
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 18:44:06 GMT, Weijun Wang wrote:
>> Similar as the one above, it's because of
>>
>> static {
>> // Don't lazy-read because every app uses invalidate()
>> isJavaAwtSmartInvalidate = AccessController.doPrivileged(
>> new GetBooleanAction("java.awt.smartInvalidate"));
>> }
>
> We are thinking of more tweaks after this overall change to make the
> annotation more precise. For example, in this case we can assign the
> `doPrivileged` result to a local variable right at its declaration, and then
> assign it to `isJavaAwtSmartInvalidate`. Some people might think this is
> ugly. Such manual code changes need to done little by little to ease code
> reviewing.
I know it's not easy to read the commit and that's why I make the 3rd commit
totally automatic. Hopefully you have more confidence on the program than my
hand. All annotations are added to the nearest declarations.
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 18:39:10 GMT, Weijun Wang wrote:
>> src/java.desktop/share/classes/java/awt/Container.java line 97:
>>
>>> 95: * @since 1.0
>>> 96: */
>>> 97: @SuppressWarnings("removal")
>>
>> Same issue as with Component. a > 5,000 line file that uses AccessController
>> in just 4 places.
>>
>> Where else are you adding this to entire classes instead of the specific
>> site ?
>
> Similar as the one above, it's because of
>
> static {
> // Don't lazy-read because every app uses invalidate()
> isJavaAwtSmartInvalidate = AccessController.doPrivileged(
> new GetBooleanAction("java.awt.smartInvalidate"));
> }
We are thinking of more tweaks after this overall change to make the annotation
more precise. For example, in this case we can assign the `doPrivileged` result
to a local variable right at its declaration, and then assign it to
`isJavaAwtSmartInvalidate`. Some people might think this is ugly. Such manual
code changes need to done little by little to ease code reviewing.
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 18:26:25 GMT, Phil Race wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
>
> src/java.desktop/share/classes/java/awt/Component.java line 217:
>
>> 215: * @author Sami Shaio
>> 216: */
>> 217: @SuppressWarnings("removal")
>
> Why is this placed on the *entire class* ??
> This class is 10535 lines long and mentions AccessControl something like 8
> places it uses AccessController or AcessControlContext.
This happens when a deprecated method is called inside a static block. The
annotation can only be added to a declaration and here it must be the whole
class. The call in this file is
s = java.security.AccessController.doPrivileged(
new
GetPropertyAction("awt.image.redrawrate"));
> src/java.desktop/share/classes/java/awt/Container.java line 97:
>
>> 95: * @since 1.0
>> 96: */
>> 97: @SuppressWarnings("removal")
>
> Same issue as with Component. a > 5,000 line file that uses AccessController
> in just 4 places.
>
> Where else are you adding this to entire classes instead of the specific site
> ?
Similar as the one above, it's because of
static {
// Don't lazy-read because every app uses invalidate()
isJavaAwtSmartInvalidate = AccessController.doPrivileged(
new GetBooleanAction("java.awt.smartInvalidate"));
}
> test/jdk/java/awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java line 59:
>
>> 57: ProcessCommunicator
>> 58: .executeChildProcess(Consumer.class, new
>> String[0]);
>> 59: if (!"Hello".equals(processResults.getStdOut())) {
>
> Who or what prompted this change ?
The child process is started with `-Djava.security.manager=allow` (after the
other PR) and a warning will be printed to stderr. Therefore I move the message
to stdout.
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
> Please review this implementation of [JEP > 411](https://openjdk.java.net/jeps/411). > > The code change is divided into 3 commits. Please review them one by one. > > 1. > https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 > The essential change for this JEP, including the `@Deprecate` annotations > and spec change. It also update the default value of the > `java.security.manager` system property to "disallow", and necessary test > change following this update. > 2. > https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 > Manual changes to several files so that the next commit can be generated > programatically. > 3. > https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 > Automatic changes to other source files to avoid javac warnings on > deprecation for removal > > The 1st and 2nd commits should be reviewed carefully. The 3rd one is > generated programmatically, see the comment below for more details. If you > are only interested in a portion of the 3rd commit and would like to review > it as a separate file, please comment here and I'll generate an individual > webrev. > > Due to the size of this PR, no attempt is made to update copyright years for > any file to minimize unnecessary merge conflict. > > Furthermore, since the default value of `java.security.manager` system > property is now "disallow", most of the tests calling > `System.setSecurityManager()` need to launched with > `-Djava.security.manager=allow`. This is covered in a different PR at > https://github.com/openjdk/jdk/pull/4071. > > Update: the deprecation annotations and javadoc tags, build, compiler, > core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are > reviewed. Rest are 2d, awt, beans, sound, and swing. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java - Changes: - all: https://git.openjdk.java.net/jdk/pull/4073/files - new: https://git.openjdk.java.net/jdk/pull/4073/files/bb73093a..c4221b5f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4073=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4073=01-02 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/4073.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4073/head:pull/4073 PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v2]
On Tue, 18 May 2021 21:21:45 GMT, Weijun Wang wrote: >> Please review this implementation of [JEP >> 411](https://openjdk.java.net/jeps/411). >> >> The code change is divided into 3 commits. Please review them one by one. >> >> 1. >> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 >> The essential change for this JEP, including the `@Deprecate` annotations >> and spec change. It also update the default value of the >> `java.security.manager` system property to "disallow", and necessary test >> change following this update. >> 2. >> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 >> Manual changes to several files so that the next commit can be generated >> programatically. >> 3. >> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 >> Automatic changes to other source files to avoid javac warnings on >> deprecation for removal >> >> The 1st and 2nd commits should be reviewed carefully. The 3rd one is >> generated programmatically, see the comment below for more details. If you >> are only interested in a portion of the 3rd commit and would like to review >> it as a separate file, please comment here and I'll generate an individual >> webrev. >> >> Due to the size of this PR, no attempt is made to update copyright years for >> any file to minimize unnecessary merge conflict. >> >> Furthermore, since the default value of `java.security.manager` system >> property is now "disallow", most of the tests calling >> `System.setSecurityManager()` need to launched with >> `-Djava.security.manager=allow`. This is covered in a different PR at >> https://github.com/openjdk/jdk/pull/4071. >> >> Update: the deprecation annotations and javadoc tags, build, compiler, >> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are >> reviewed. Rest are 2d, awt, beans, sound, and swing. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > feedback from Sean, Phil and Alan A new commit fixing `awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java` test in tier4. The test compares stderr output with a known value but we have a new warning written to stderr now. It's now using stdout instead. @prrace, Please confirm this is acceptable. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager [v2]
> Please review the test changes for [JEP
> 411](https://openjdk.java.net/jeps/411).
>
> With JEP 411 and the default value of `-Djava.security.manager` becoming
> `disallow`, tests calling `System.setSecurityManager()` need
> `-Djava.security.manager=allow` when launched. This PR covers such changes
> for tier1 to tier3 (except for the JCK tests).
>
> To make it easier to focus your review on the tests in your area, this PR is
> divided into multiple commits for different areas (~~serviceability~~,
> ~~hotspot-compiler~~, ~~i18n~~, ~~rmi~~, ~~javadoc~~, swing, 2d,
> ~~security~~, ~~hotspot-runtime~~, ~~nio~~, ~~xml~~, ~~beans~~,
> ~~core-libs~~, ~~net~~, ~~compiler~~, ~~hotspot-gc~~). Mostly the rule is the
> same as how Skara adds labels, but there are several small tweaks:
>
> 1. When a file is covered by multiple labels, only one is chosen. I make my
> best to avoid putting too many tests into `core-libs`. If a file is covered
> by `core-libs` and another label, I categorized it into the other label.
> 2. If a file is in `core-libs` but contains `/xml/` in its name, it's in the
> `xml` commit.
> 3. If a file is in `core-libs` but contains `/rmi/` in its name, it's in the
> `rmi` commit.
> 4. One file not covered by any label --
> `test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java` -- is in
> the `swing` commit.
>
> Due to the size of this PR, no attempt is made to update copyright years for
> all files to minimize unnecessary merge conflict.
>
> Please note that this PR can be integrated before the source changes for JEP
> 411, as the possible values of this system property was already defined long
> time ago in JDK 9.
>
> Most of the change in this PR is a simple adding of
> `-Djava.security.manager=allow` to the `@run main/othervm` line. Sometimes it
> was not `othervm` and we add one. Sometimes there's no `@run` at all and we
> add the line.
>
> There are several tests that launch another Java process that needs to call
> the `System.setSecurityManager()` method, and the system property is added to
> `ProcessBuilder`, `ProcessTools`, or the java command line (if the test is a
> shell test).
>
> 3 langtools tests are added into problem list due to
> [JDK-8265611](https://bugs.openjdk.java.net/browse/JDK-8265611).
>
> 2 SQL tests are moved because they need different options on the `@run` line
> but they are inside a directory that has a `TEST.properties`:
>
> rename test/jdk/java/sql/{testng/test/sql/othervm =>
> permissionTests}/DriverManagerPermissionsTests.java (93%)
> rename test/jdk/javax/sql/{testng/test/rowset/spi =>
> permissionTests}/SyncFactoryPermissionsTests.java (95%)
> ```
>
> The source change for JEP 411 is at https://github.com/openjdk/jdk/pull/4073.
Weijun Wang has updated the pull request incrementally with one additional
commit since the last revision:
adjust order of VM options
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4071/files
- new: https://git.openjdk.java.net/jdk/pull/4071/files/900c28c0..bfa955ad
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=4071=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk=4071=00-01
Stats: 59 lines in 18 files changed: 8 ins; 6 del; 45 mod
Patch: https://git.openjdk.java.net/jdk/pull/4071.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4071/head:pull/4071
PR: https://git.openjdk.java.net/jdk/pull/4071
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v2]
> Please review this implementation of [JEP > 411](https://openjdk.java.net/jeps/411). > > The code change is divided into 3 commits. Please review them one by one. > > 1. > https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 > The essential change for this JEP, including the `@Deprecate` annotations > and spec change. It also update the default value of the > `java.security.manager` system property to "disallow", and necessary test > change following this update. > 2. > https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 > Manual changes to several files so that the next commit can be generated > programatically. > 3. > https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 > Automatic changes to other source files to avoid javac warnings on > deprecation for removal > > The 1st and 2nd commits should be reviewed carefully. The 3rd one is > generated programmatically, see the comment below for more details. If you > are only interested in a portion of the 3rd commit and would like to review > it as a separate file, please comment here and I'll generate an individual > webrev. > > Due to the size of this PR, no attempt is made to update copyright years for > any file to minimize unnecessary merge conflict. > > Furthermore, since the default value of `java.security.manager` system > property is now "disallow", most of the tests calling > `System.setSecurityManager()` need to launched with > `-Djava.security.manager=allow`. This is covered in a different PR at > https://github.com/openjdk/jdk/pull/4071. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: feedback from Sean, Phil and Alan - Changes: - all: https://git.openjdk.java.net/jdk/pull/4073/files - new: https://git.openjdk.java.net/jdk/pull/4073/files/eb6c566f..bb73093a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4073=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4073=00-01 Stats: 9 lines in 3 files changed: 4 ins; 1 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/4073.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4073/head:pull/4073 PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Tue, 18 May 2021 18:38:52 GMT, Weijun Wang wrote:
>> src/java.base/share/classes/java/security/AccessController.java line 877:
>>
>>> 875: @CallerSensitive
>>> 876: public static T doPrivileged(PrivilegedExceptionAction
>>> action,
>>> 877: @SuppressWarnings("removal")
>>> AccessControlContext context, Permission... perms)
>>
>> you might find it easier if you put the Permissions parameter on a new line.
>> There are several places in AccessController where the same thing happens.
>
> I'll try to update my auto-annotating program.
Turns out this only happens in this class:
$ rg '^\s*@SuppressWarnings("removal").*?,.'
src/java.base/share/classes/java/security/AccessController.java:449:
@SuppressWarnings("removal") AccessControlContext context, Permission... perms)
{
src/java.base/share/classes/java/security/AccessController.java:514:
@SuppressWarnings("removal") AccessControlContext context, Permission... perms)
{
src/java.base/share/classes/java/security/AccessController.java:877:
@SuppressWarnings("removal") AccessControlContext
context, Permission... perms)
I'll fix them manually.
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Tue, 18 May 2021 17:36:55 GMT, Alan Bateman wrote:
>> Please review this implementation of [JEP
>> 411](https://openjdk.java.net/jeps/411).
>>
>> The code change is divided into 3 commits. Please review them one by one.
>>
>> 1.
>> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>> The essential change for this JEP, including the `@Deprecate` annotations
>> and spec change. It also update the default value of the
>> `java.security.manager` system property to "disallow", and necessary test
>> change following this update.
>> 2.
>> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>> Manual changes to several files so that the next commit can be generated
>> programatically.
>> 3.
>> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>> Automatic changes to other source files to avoid javac warnings on
>> deprecation for removal
>>
>> The 1st and 2nd commits should be reviewed carefully. The 3rd one is
>> generated programmatically, see the comment below for more details. If you
>> are only interested in a portion of the 3rd commit and would like to review
>> it as a separate file, please comment here and I'll generate an individual
>> webrev.
>>
>> Due to the size of this PR, no attempt is made to update copyright years for
>> any file to minimize unnecessary merge conflict.
>>
>> Furthermore, since the default value of `java.security.manager` system
>> property is now "disallow", most of the tests calling
>> `System.setSecurityManager()` need to launched with
>> `-Djava.security.manager=allow`. This is covered in a different PR at
>> https://github.com/openjdk/jdk/pull/4071.
>
> src/java.base/share/classes/java/security/AccessController.java line 877:
>
>> 875: @CallerSensitive
>> 876: public static T doPrivileged(PrivilegedExceptionAction
>> action,
>> 877: @SuppressWarnings("removal")
>> AccessControlContext context, Permission... perms)
>
> you might find it easier if you put the Permissions parameter on a new line.
> There are several places in AccessController where the same thing happens.
My rule is that a new line is added if there's only whitespaces before the
insert position and the previous line does not end with a comma. In this case,
unless I move `Permission... perms` onto a new line that an annotation on a new
line looks like covering both `context` and `perms`.
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager
On Tue, 18 May 2021 05:48:56 GMT, Alan Bateman wrote: > The changes look okay but a bit inconsistent on where -Djava...=allow is > inserted for tests that already set other system properties or other > parameters. Not a correctness issue, just looks odd in several places, e.g. > > test/jdk/java/lang/System/LoggerFinder/BaseLoggerFinderTest/BaseLoggerFinderTest.java > - the tests sets the system properties after -Xbootclasspath/a: but the > change means it sets properties before and after. > > test/jdk/java/lang/Class/getResource/ResourcesTest.java - you've added it in > the middle of the module and class path parameters. > > For uses using ProcessTools then it seems to be very random. I've updated the 3 cases you mentioned and will go through more. Yes it looks good to group system property settings together. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/4071
Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager
On Tue, 18 May 2021 11:12:00 GMT, Daniel Fuchs wrote:
>> Please review the test changes for [JEP
>> 411](https://openjdk.java.net/jeps/411).
>>
>> With JEP 411 and the default value of `-Djava.security.manager` becoming
>> `disallow`, tests calling `System.setSecurityManager()` need
>> `-Djava.security.manager=allow` when launched. This PR covers such changes
>> for tier1 to tier3 (except for the JCK tests).
>>
>> To make it easier to focus your review on the tests in your area, this PR is
>> divided into multiple commits for different areas (~~serviceability~~,
>> ~~hotspot-compiler~~, i18n, rmi, javadoc, swing, 2d, security,
>> hotspot-runtime, nio, xml, beans, ~~core-libs~~, ~~net~~, compiler,
>> ~~hotspot-gc~~). Mostly the rule is the same as how Skara adds labels, but
>> there are several small tweaks:
>>
>> 1. When a file is covered by multiple labels, only one is chosen. I make my
>> best to avoid putting too many tests into `core-libs`. If a file is covered
>> by `core-libs` and another label, I categorized it into the other label.
>> 2. If a file is in `core-libs` but contains `/xml/` in its name, it's in the
>> `xml` commit.
>> 3. If a file is in `core-libs` but contains `/rmi/` in its name, it's in the
>> `rmi` commit.
>> 4. One file not covered by any label --
>> `test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java` -- is in
>> the `swing` commit.
>>
>> Due to the size of this PR, no attempt is made to update copyright years for
>> all files to minimize unnecessary merge conflict.
>>
>> Please note that this PR can be integrated before the source changes for JEP
>> 411, as the possible values of this system property was already defined long
>> time ago in JDK 9.
>>
>> Most of the change in this PR is a simple adding of
>> `-Djava.security.manager=allow` to the `@run main/othervm` line. Sometimes
>> it was not `othervm` and we add one. Sometimes there's no `@run` at all and
>> we add the line.
>>
>> There are several tests that launch another Java process that needs to call
>> the `System.setSecurityManager()` method, and the system property is added
>> to `ProcessBuilder`, `ProcessTools`, or the java command line (if the test
>> is a shell test).
>>
>> 3 langtools tests are added into problem list due to
>> [JDK-8265611](https://bugs.openjdk.java.net/browse/JDK-8265611).
>>
>> 2 SQL tests are moved because they need different options on the `@run` line
>> but they are inside a directory that has a `TEST.properties`:
>>
>> rename test/jdk/java/sql/{testng/test/sql/othervm =>
>> permissionTests}/DriverManagerPermissionsTests.java (93%)
>> rename test/jdk/javax/sql/{testng/test/rowset/spi =>
>> permissionTests}/SyncFactoryPermissionsTests.java (95%)
>> ```
>>
>> The source change for JEP 411 is at https://github.com/openjdk/jdk/pull/4073.
>
> test/jdk/javax/management/remote/mandatory/notif/NotificationEmissionTest.java
> line 34:
>
>> 32: * @run clean NotificationEmissionTest
>> 33: * @run build NotificationEmissionTest
>> 34: * @run main NotificationEmissionTest 1
>
> This test case (NotificationEmissionTest 1) calls
> `System.setProperty("java.security.policy", policyFile);` - even though it
> doesn't call System.setSecurityManager(); Should the @run command line for
> test case 1 be modified as well?
Or maybe the policy setting line should only be called when a security manager
is set? IIRC jtreg is able to restore system properties even in agentvm mode.
So maybe this really doesn't matter. We just want to modify as little as
possible in this PR.
-
PR: https://git.openjdk.java.net/jdk/pull/4071
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Mon, 17 May 2021 18:23:41 GMT, Weijun Wang wrote:
> Please review this implementation of [JEP
> 411](https://openjdk.java.net/jeps/411).
>
> The code change is divided into 3 commits. Please review them one by one.
>
> 1.
> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
> The essential change for this JEP, including the `@Deprecate` annotations
> and spec change. It also update the default value of the
> `java.security.manager` system property to "disallow", and necessary test
> change following this update.
> 2.
> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
> Manual changes to several files so that the next commit can be generated
> programatically.
> 3.
> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
> Automatic changes to other source files to avoid javac warnings on
> deprecation for removal
>
> The 1st and 2nd commits should be reviewed carefully. The 3rd one is
> generated programmatically, see the comment below for more details. If you
> are only interested in a portion of the 3rd commit and would like to review
> it as a separate file, please comment here and I'll generate an individual
> webrev.
>
> Due to the size of this PR, no attempt is made to update copyright years for
> any file to minimize unnecessary merge conflict.
>
> Furthermore, since the default value of `java.security.manager` system
> property is now "disallow", most of the tests calling
> `System.setSecurityManager()` need to launched with
> `-Djava.security.manager=allow`. This is covered in a different PR at
> https://github.com/openjdk/jdk/pull/4071.
The 3rd commit is the biggest one but it's all generated programmatically. All
changes are simply adding `@SupressWarnings("removal")` to a declaration.
Precisely, of all the diff hunks (leading whitespaces ignored), there are 1607
+ @SuppressWarnings("removal")
There are also 7 cases where an existing annotation is updated
= 2
- @SuppressWarnings("deprecation")
+ @SuppressWarnings({"removal","deprecation"})
= 1
- @SuppressWarnings("Convert2Lambda")
+ @SuppressWarnings({"removal","Convert2Lambda"})
= 1
- @SuppressWarnings({"unchecked", "CloneDeclaresCloneNotSupported"})
+ @SuppressWarnings({"removal","unchecked", "CloneDeclaresCloneNotSupported"})
= 1
- @SuppressWarnings("deprecation") // Use of RMISecurityManager
+ @SuppressWarnings({"removal","deprecation"}) // Use of RMISecurityManager
= 1
- @SuppressWarnings("unchecked") /*To suppress warning from line 1374*/
+ @SuppressWarnings({"removal","unchecked"}) /*To suppress warning from
line 1374*/
= 1
- @SuppressWarnings("fallthrough")
+ @SuppressWarnings({"removal","fallthrough"})
All other cases are new annotation embedded inline:
= 7
- AccessControlContext acc) {
+ @SuppressWarnings("removal") AccessControlContext acc) {
= 4
- AccessControlContext acc,
+ @SuppressWarnings("removal") AccessControlContext acc,
= 3
- AccessControlContext context,
+ @SuppressWarnings("removal") AccessControlContext context,
= 3
- AccessControlContext acc)
+ @SuppressWarnings("removal") AccessControlContext acc)
= 2
- try (InputStream stream = AccessController.doPrivileged(pa)) {
+ try (@SuppressWarnings("removal") InputStream stream =
AccessController.doPrivileged(pa)) {
= 2
- AccessControlContext context, Permission... perms) {
+ @SuppressWarnings("removal") AccessControlContext context, Permission...
perms) {
= 2
- } catch (java.security.AccessControlException e) {
+ } catch (@SuppressWarnings("removal") java.security.AccessControlException e)
{
= 2
- } catch (AccessControlException ace) {
+ } catch (@SuppressWarnings("removal") AccessControlException ace) {
= 2
- AccessControlContext context)
+ @SuppressWarnings("removal") AccessControlContext context)
= 2
- AccessControlContext acc) throws LoginException {
+ @SuppressWarnings("removal") AccessControlContext acc) throws LoginE
RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
Please review this implementation of [JEP 411](https://openjdk.java.net/jeps/411). The code change is divided into 3 commits. Please review them one by one. 1. https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 The essential change for this JEP, including the `@Deprecate` annotations and spec change. It also update the default value of the `java.security.manager` system property to "disallow", and necessary test change following this update. 2. https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 Manual changes to several files so that the next commit can be generated programatically. 3. https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 Automatic changes to other source files to avoid javac warnings on deprecation for removal The 1st and 2nd commits should be reviewed carefully. The 3rd one is generated programmatically, see the comment below for more details. If you are only interested in a portion of the 3rd commit and would like to review it as a separate file, please comment here and I'll generate an individual webrev. Due to the size of this PR, no attempt is made to update copyright years for any file to minimize unnecessary merge conflict. Furthermore, since the default value of `java.security.manager` system property is now "disallow", most of the tests calling `System.setSecurityManager()` need to launched with `-Djava.security.manager=allow`. This is covered in a different PR at https://github.com/openjdk/jdk/pull/4071. - Commit messages: - add supresswarnings annotations automatically - manual change before automatic annotating - 8266459: Implement JEP 411: Deprecate the Security Manager for Removal Changes: https://git.openjdk.java.net/jdk/pull/4073/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4073=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8266459 Stats: 2018 lines in 826 files changed: 1884 ins; 9 del; 125 mod Patch: https://git.openjdk.java.net/jdk/pull/4073.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4073/head:pull/4073 PR: https://git.openjdk.java.net/jdk/pull/4073
RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager
Please review the test changes for [JEP 411](https://openjdk.java.net/jeps/411).
With JEP 411 and the default value of `-Djava.security.manager` becoming
`disallow`, tests calling `System.setSecurityManager()` need
`-Djava.security.manager=allow` when launched. This PR covers such changes for
tier1 to tier3 (except for the JCK tests).
To make it easier to focus your review on the tests in your area, this PR is
divided into multiple commits for different areas. Mostly the rule is the same
as how Skara adds labels, but there are several small tweaks:
1. When a file is covered by multiple labels, only one is chosen. I make my
best to avoid putting too many tests into `core-libs`. If a file is covered by
`core-libs` and another label, I categorized it into the other label.
2. If a file is in `core-libs` but contains `/xml/` in its name, it's in the
`xml` commit.
3. If a file is in `core-libs` but contains `/rmi/` in its name, it's in the
`rmi` commit.
4. One file not covered by any label --
`test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java` -- is in the
`swing` commit.
Due to the size of this PR, no attempt is made to update copyright years for
all files to minimize unnecessary merge conflict.
Please note that this PR can be integrated before the source changes for JEP
411, as the possible values of this system property was already defined long
time ago in JDK 9.
Most of the change in this PR is a simple adding of
`-Djava.security.manager=allow` to the `@run main/othervm` line. Sometimes it
was not `othervm` and we add one. Sometimes there's no `@run` at all and we add
the line.
There are several tests that launch another Java process that needs to call the
`System.setSecurityManager()` method, and the system property is added to
`ProcessBuilder`, `ProcessTools`, or the java command line (if the test is a
shell test).
3 langtools tests are added into problem list due to
[JDK-8265611](https://bugs.openjdk.java.net/browse/JDK-8265611).
2 SQL tests are moved because they need different options on the `@run` line
but they are inside a directory that has a `TEST.properties`:
rename test/jdk/java/sql/{testng/test/sql/othervm =>
permissionTests}/DriverManagerPermissionsTests.java (93%)
rename test/jdk/javax/sql/{testng/test/rowset/spi =>
permissionTests}/SyncFactoryPermissionsTests.java (95%)
```
The source change for JEP 411 is at https://github.com/openjdk/jdk/pull/4073.
-
Commit messages:
- test for awt
- test for hotspot-gc
- test for compiler
- test for net
- test for core-libs
- test for beans
- test for xml
- test for nio
- test for hotspot-runtime
- test for security
- ... and 7 more: https://git.openjdk.java.net/jdk/compare/79b39445...900c28c0
Changes: https://git.openjdk.java.net/jdk/pull/4071/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4071=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8267184
Stats: 1024 lines in 852 files changed: 249 ins; 8 del; 767 mod
Patch: https://git.openjdk.java.net/jdk/pull/4071.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4071/head:pull/4071
PR: https://git.openjdk.java.net/jdk/pull/4071
Re: RFR: 8266225: jarsigner is using incorrect security property to show weakness of certs [v2]
On Thu, 6 May 2021 20:57:13 GMT, Hai-May Chao wrote: >> Please review the change to jarsigner so it uses certpath security property >> in order to properly display the weakness of the certificate algorithms. > > Hai-May Chao has updated the pull request incrementally with one additional > commit since the last revision: > > Test with new java.security file Marked as reviewed by weijun (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3905
Re: RFR: 8265426: Update java.security to use instanceof pattern variable [v5]
On Thu, 6 May 2021 14:42:20 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the >> `java.security` package to make use of the `instanceof` pattern variable? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8265426: changed order of equals check; refactored Identity.equals method Marked as reviewed by weijun (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3687
Re: RFR: 8266225: jarsigner is using incorrect security property to show weakness of certs
On Thu, 6 May 2021 16:49:33 GMT, Hai-May Chao wrote:
> Please review the change to jarsigner so it uses certpath security property
> in order to properly display the weakness of the certificate algorithms.
test/jdk/sun/security/tools/jarsigner/CheckSignerCertChain.java line 90:
> 88: // key, but not for its SHA1withRSA algorithm.
> 89: .shouldContain("Signature algorithm: SHA1withRSA,
> 1024-bit key (weak)")
> 90: .shouldHaveExitValue(0);
What does the test show before this fix?
I don't see `Security.setProperty` called or a new `java.security` file is
used. If `jdk.jar.dA` and `jdk.certpath.dA` are the same, then there's no way
to find out if the new code works.
-
PR: https://git.openjdk.java.net/jdk/pull/3905
Integrated: 8266293: Key protection using PBEWithMD5AndDES fails with "java.security.InvalidAlgorithmParameterException: Salt must be 8 bytes long"
On Fri, 30 Apr 2021 17:35:46 GMT, Weijun Wang wrote: > `PKCS12KeyStore` always uses a 20-byte salt in encryption but > PBEWithMD5AndDES only accepts 8-byte salt. With this code change, the salt > used for this algorithm will be 8 bytes. > > RFC 2898 only requires the salt to be at least 8 bytes, but I don't intend to > modify the `PBES1Core.java` to accept a long salt. Otherwise, a newly > generated PKCS #12 file using a long salt will not be recognized by an old > JDK. > > Also, although `PBES1Core.java` also take cares of another algorithm named > PBEWithMD5AndDESede but it's not usable in a PKCS #12 keystore as we have > not defined its Object Identifier anywhere. This pull request has now been integrated. Changeset: 04f71126 Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/04f71126479f9c39aa71e8aebe7196d72fc16796 Stats: 18 lines in 2 files changed: 15 ins; 0 del; 3 mod 8266293: Key protection using PBEWithMD5AndDES fails with "java.security.InvalidAlgorithmParameterException: Salt must be 8 bytes long" Reviewed-by: valeriep - PR: https://git.openjdk.java.net/jdk/pull/3822
Re: RFR: 8266293: Key protection using PBEWithMD5AndDES fails with "java.security.InvalidAlgorithmParameterException: Salt must be 8 bytes long" [v2]
On Thu, 6 May 2021 01:23:40 GMT, Valerie Peng wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> better comment
>
> src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java line 811:
>
>> 809: byte[] salt = getSalt();
>> 810: if (KnownOIDs.findMatch(algorithm) ==
>> KnownOIDs.PBEWithMD5AndDES) {
>> 811: // PBEWithMD5AndDES requires a 8-byte salt
>
> nit: maybe use "PBES1 scheme such as PBEWithMD5AndDES requires a 8-byte salt"
Sure. Updated.
-
PR: https://git.openjdk.java.net/jdk/pull/3822
Re: RFR: 8266293: Key protection using PBEWithMD5AndDES fails with "java.security.InvalidAlgorithmParameterException: Salt must be 8 bytes long" [v2]
> `PKCS12KeyStore` always uses a 20-byte salt in encryption but > PBEWithMD5AndDES only accepts 8-byte salt. With this code change, the salt > used for this algorithm will be 8 bytes. > > RFC 2898 only requires the salt to be at least 8 bytes, but I don't intend to > modify the `PBES1Core.java` to accept a long salt. Otherwise, a newly > generated PKCS #12 file using a long salt will not be recognized by an old > JDK. > > Also, although `PBES1Core.java` also take cares of another algorithm named > PBEWithMD5AndDESede but it's not usable in a PKCS #12 keystore as we have > not defined its Object Identifier anywhere. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: better comment - Changes: - all: https://git.openjdk.java.net/jdk/pull/3822/files - new: https://git.openjdk.java.net/jdk/pull/3822/files/6726d0be..df1ab373 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3822=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3822=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3822.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3822/head:pull/3822 PR: https://git.openjdk.java.net/jdk/pull/3822
Re: RFR: 8265426: Update java.security to use instanceof pattern variable [v3]
On Thu, 6 May 2021 11:52:15 GMT, Patrick Concannon
wrote:
>> Hi,
>>
>> Could someone please review my code for updating the code in the
>> `java.security` package to make use of the `instanceof` pattern variable?
>>
>> Kind regards,
>> Patrick
>
> Patrick Concannon has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8265426: Reverted parameter names; removed redundant parenthesis
Approved. Just some tiny comments. The other suggestion from @Punikekk also
looks fine. Thanks.
src/java.base/share/classes/java/security/cert/CertPath.java line 185:
> 183:
> 184: return other instanceof CertPath that
> 185: && this.type.equals(that.getType())
I know `type` should never be null but let's change as little as possible by
using `that.getType().equals(this.type)`.
src/java.base/share/classes/java/security/spec/EllipticCurve.java line 176:
> 174: && (field.equals(other.field)
> 175: && a.equals(other.a)
> 176: && b.equals(other.b));
I suppose there's no need to put the last 3 checks between "(" and ")".
-
Marked as reviewed by weijun (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3687
RFR: 8266293: Key protection using PBEWithMD5AndDES fails with "java.security.InvalidAlgorithmParameterException: Salt must be 8 bytes long"
`PKCS12KeyStore` always uses a 20-byte salt in encryption but PBEWithMD5AndDES only accepts 8-byte salt. With this code change, the salt used for this algorithm will be 8 bytes. RFC 2898 only requires the salt to be at least 8 bytes, but I don't intend to modify the `PBES1Core.java` to accept a long salt. Otherwise, a newly generated PKCS #12 file using a long salt will not be recognized by an old JDK. Also, although `PBES1Core.java` also take cares of another algorithm named PBEWithMD5AndDESede but it's not usable in a PKCS #12 keystore as we have not defined its Object Identifier anywhere. - Commit messages: - 8266293: Key protection using PBEWithMD5AndDES fails with "java.security.InvalidAlgorithmParameterException: Salt must be 8 bytes long" Changes: https://git.openjdk.java.net/jdk/pull/3822/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3822=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8266293 Stats: 18 lines in 2 files changed: 15 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/3822.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3822/head:pull/3822 PR: https://git.openjdk.java.net/jdk/pull/3822
Integrated: 8266220: keytool still prompt for store password on a password-less pkcs12 file if -storetype pkcs12 is specified
On Wed, 28 Apr 2021 15:07:14 GMT, Weijun Wang wrote: > It's awkward that for a password-less pkcs12 keystore, `keytool -list` does > not prompt for a password but `keytool -list -storetype pkcs12` does. This pull request has now been integrated. Changeset: 48bb996a Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/48bb996ac9098fc33f6d52e2af15448b12a19572 Stats: 44 lines in 2 files changed: 30 ins; 5 del; 9 mod 8266220: keytool still prompt for store password on a password-less pkcs12 file if -storetype pkcs12 is specified Reviewed-by: coffeys, hchao - PR: https://git.openjdk.java.net/jdk/pull/3764
Re: RFR: 8266220: keytool still prompt for store password on a password-less pkcs12 file if -storetype pkcs12 is specified
On Wed, 28 Apr 2021 15:07:14 GMT, Weijun Wang wrote: > It's awkward that for a password-less pkcs12 keystore, `keytool -list` does > not prompt for a password but `keytool -list -storetype pkcs12` does. New commit pushed. When the file is opened the second time, uses a local variable and close it quickly. The `ksStream` global variable is only assigned once and is always closed in a finally block. - PR: https://git.openjdk.java.net/jdk/pull/3764
Re: RFR: 8266220: keytool still prompt for store password on a password-less pkcs12 file if -storetype pkcs12 is specified [v2]
> It's awkward that for a password-less pkcs12 keystore, `keytool -list` does > not prompt for a password but `keytool -list -storetype pkcs12` does. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: close stream carefully - Changes: - all: https://git.openjdk.java.net/jdk/pull/3764/files - new: https://git.openjdk.java.net/jdk/pull/3764/files/b9c9b8ee..51bc2fc4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3764=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3764=00-01 Stats: 9 lines in 1 file changed: 3 ins; 3 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/3764.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3764/head:pull/3764 PR: https://git.openjdk.java.net/jdk/pull/3764
Re: RFR: 8266220: keytool still prompt for store password on a password-less pkcs12 file if -storetype pkcs12 is specified
On Wed, 28 Apr 2021 15:07:14 GMT, Weijun Wang wrote: > It's awkward that for a password-less pkcs12 keystore, `keytool -list` does > not prompt for a password but `keytool -list -storetype pkcs12` does. Test `sun/security/tools/keytool/KeyToolTest.java` failed on Windows. Looks like a file-not-closed issue. Will investigate if related to this code change. - PR: https://git.openjdk.java.net/jdk/pull/3764
RFR: 8266220: keytool still prompt for store password on a password-less pkcs12 file if -storetype pkcs12 is specified
It's awkward that for a password-less pkcs12 keystore, `keytool -list` does not prompt for a password but `keytool -list -storetype pkcs12` does. - Commit messages: - 8266220: keytool still prompt for store password on a password-less pkcs12 file if -storetype pkcs12 is specified Changes: https://git.openjdk.java.net/jdk/pull/3764/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3764=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8266220 Stats: 35 lines in 2 files changed: 27 ins; 2 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/3764.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3764/head:pull/3764 PR: https://git.openjdk.java.net/jdk/pull/3764
Re: RFR: 8265426: Update java.security to use instanceof pattern variable
On Mon, 26 Apr 2021 08:50:36 GMT, Patrick Concannon wrote: > Hi, > > Could someone please review my code for updating the code in the > `java.security` package to make use of the `instanceof` pattern variable? > > Kind regards, > Patrick Two comments: 1. Why not reuse the existing variable name (Ex: `t` in `Type t = (Type)obj`) as much as possible to avoid unnecessary renames? 2. I'm not sure if modifying argument name in a public API is a good idea. This leads to unnecessary javadoc changes. - PR: https://git.openjdk.java.net/jdk/pull/3687
Re: RFR: 8265426: Update java.security to use instanceof pattern variable
On Mon, 26 Apr 2021 08:50:36 GMT, Patrick Concannon wrote: > Hi, > > Could someone please review my code for updating the code in the > `java.security` package to make use of the `instanceof` pattern variable? > > Kind regards, > Patrick Two comments: 1. Why not reuse the existing variable name (Ex: `t` in `Type t = (Type)obj`) as much as possible to avoid unnecessary renames? 2. I'm not sure if modifying argument name in a public API is a good idea. This leads to unnecessary javadoc changes. - PR: https://git.openjdk.java.net/jdk/pull/3687
Integrated: 8258915: Temporary buffer cleanup
On Wed, 13 Jan 2021 21:32:07 GMT, Weijun Wang wrote: > Clean up temporary byte array, char array, and keyspec around keys and > passwords. > > No new regression test. This pull request has now been integrated. Changeset: f834557a Author: Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/f834557a Stats: 2020 lines in 79 files changed: 986 ins; 508 del; 526 mod 8258915: Temporary buffer cleanup Reviewed-by: valeriep - PR: https://git.openjdk.java.net/jdk/pull/2070
Re: RFR: 8258915: Temporary buffer cleanup [v10]
> Clean up temporary byte array, char array, and keyspec around keys and > passwords. > > No new regression test. Weijun Wang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 12 commits: - Merge branch 'master' into 8258915 - Merge - simpler spec creation, and one more clean - materials - TLS key generators - drbg only in patch2: unchanged: - cleanups for key generations - keyfactory operations on own keyspec - more wrap, less copy - rsa - ... and 2 more: https://git.openjdk.java.net/jdk/compare/657f1039...f3b0f298 - Changes: https://git.openjdk.java.net/jdk/pull/2070/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2070=09 Stats: 2020 lines in 79 files changed: 986 ins; 508 del; 526 mod Patch: https://git.openjdk.java.net/jdk/pull/2070.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2070/head:pull/2070 PR: https://git.openjdk.java.net/jdk/pull/2070
Integrated: 8264277: java.xml.crypto module should be granted FilePermission and SocketPermission
On Tue, 30 Mar 2021 13:02:33 GMT, Weijun Wang wrote: > These permissions are needed so that the URIDereferencer is able to read data > from a file system or a network. As the test shows, you still have to grant > the same type of permission to your application. This pull request has now been integrated. Changeset: 8bec6fe6 Author: Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/8bec6fe6 Stats: 140 lines in 2 files changed: 140 ins; 0 del; 0 mod 8264277: java.xml.crypto module should be granted FilePermission and SocketPermission Reviewed-by: mullan - PR: https://git.openjdk.java.net/jdk/pull/3266
Re: RFR: 8264277: java.xml.crypto module should be granted FilePermission and SocketPermission [v3]
> These permissions are needed so that the URIDereferencer is able to read data > from a file system or a network. As the test shows, you still have to grant > the same type of permission to your application. Weijun Wang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits: - proc path changed - Merge - 8264277: java.xml.crypto module should be granted FilePermission and SocketPermission 8264277: java.xml.crypto module should be granted FilePermission and SocketPermission - update XMLUtils (not used by tests here) - 8241306: Add SignatureMethodParameterSpec subclass for RSASSA-PSS params 8241306: Add SignatureMethodParameterSpec subclass for RSASSA-PSS params - Changes: https://git.openjdk.java.net/jdk/pull/3266/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3266=02 Stats: 140 lines in 2 files changed: 140 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3266.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3266/head:pull/3266 PR: https://git.openjdk.java.net/jdk/pull/3266
Integrated: 8241306: Add SignatureMethodParameterSpec subclass for RSASSA-PSS params
On Wed, 24 Mar 2021 21:36:21 GMT, Weijun Wang wrote: > This enhancement contains the following code changes: > > 1. Create a new public API `javax/xml/crypto/dsig/spec/RSAPSSParameterSpec` > and remove the internal one. > 2. Update marshaling and unmarshaling code inside `DOMRSAPSSSignatureMethod` > so it understands extra fields in `PSSParameterSpec` and is aware of the > defaults in both directions. > 3. Update `DOMSignedInfo` so that secure validation can restrict > `DigestMethod` used inside `RSAPSSParameterSpec` > 4. Tests This pull request has now been integrated. Changeset: 8dbf7aa1 Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/8dbf7aa1 Stats: 1319 lines in 11 files changed: 1159 ins; 110 del; 50 mod 8241306: Add SignatureMethodParameterSpec subclass for RSASSA-PSS params Reviewed-by: mullan - PR: https://git.openjdk.java.net/jdk/pull/3181
Re: RFR: 8264277: java.xml.crypto module should be granted FilePermission and SocketPermission [v2]
> These permissions are needed so that the URIDereferencer is able to read data > from a file system or a network. As the test shows, you still have to grant > the same type of permission to your application. Weijun Wang 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. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3266/files - new: https://git.openjdk.java.net/jdk/pull/3266/files/28f783e8..28f783e8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3266=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3266=00-01 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3266.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3266/head:pull/3266 PR: https://git.openjdk.java.net/jdk/pull/3266
Integrated: 8265227: Move Proc.java from security/testlibrary to test/lib
On Wed, 14 Apr 2021 18:12:57 GMT, Weijun Wang wrote: > I'd like to move this tool to test/lib inside a proper named package. This pull request has now been integrated. Changeset: c70589c6 Author: Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/c70589c6 Stats: 119 lines in 9 files changed: 96 ins; 9 del; 14 mod 8265227: Move Proc.java from security/testlibrary to test/lib Reviewed-by: rriggs, xuelei, rhalade, ssahoo - PR: https://git.openjdk.java.net/jdk/pull/3496
Re: RFR: 8265227: Move Proc.java from security/testlibrary to test/lib [v3]
> I'd like to move this tool to test/lib inside a proper named package. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: do not call internal method - Changes: - all: https://git.openjdk.java.net/jdk/pull/3496/files - new: https://git.openjdk.java.net/jdk/pull/3496/files/200ce3bd..72878e37 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3496=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3496=01-02 Stats: 21 lines in 5 files changed: 5 ins; 9 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/3496.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3496/head:pull/3496 PR: https://git.openjdk.java.net/jdk/pull/3496
Re: RFR: 8265227: Move Proc.java from security/testlibrary to test/lib [v2]
On Wed, 14 Apr 2021 20:48:07 GMT, Weijun Wang wrote: >> I'd like to move this tool to test/lib inside a proper named package. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > move to process and a test for itself New commit that uses a different method to find out `-add-exports` and `--add-opens` so there is no need to use an internal method. Otherwise, multiple other tests that have `@build jdk.test.lib.process.*` would fail without the extra `@modules` tag. - PR: https://git.openjdk.java.net/jdk/pull/3496
Re: RFR: 8265227: Move Proc.java from security/testlibrary to test/lib [v2]
> I'd like to move this tool to test/lib inside a proper named package. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: move to process and a test for itself - Changes: - all: https://git.openjdk.java.net/jdk/pull/3496/files - new: https://git.openjdk.java.net/jdk/pull/3496/files/4823a1c6..200ce3bd Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3496=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3496=00-01 Stats: 89 lines in 8 files changed: 82 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/3496.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3496/head:pull/3496 PR: https://git.openjdk.java.net/jdk/pull/3496
Re: RFR: 8265227: Move Proc.java from security/testlibrary to test/lib
On Wed, 14 Apr 2021 18:12:57 GMT, Weijun Wang wrote: > I'd like to move this tool to test/lib inside a proper named package. Well, we can certainly rename it. It's only used by several tests. Maybe `ChildProcess`? And we can certainly move it to a different package. I know it has some duplicate functions with other tools so it will be better to do some cleanup first. The best function of this class is it provides several methods (both for child and parent) to simplify inter-process message transport. - PR: https://git.openjdk.java.net/jdk/pull/3496
RFR: 8265227: Move Proc.java from security/testlibrary to test/lib
I'd like to move this tool to test/lib inside a proper named package. - Commit messages: - 8265227: Move Proc.java from security/testlibrary to test/lib Changes: https://git.openjdk.java.net/jdk/pull/3496/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3496=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8265227 Stats: 18 lines in 8 files changed: 10 ins; 1 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/3496.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3496/head:pull/3496 PR: https://git.openjdk.java.net/jdk/pull/3496
Re: RFR: 8241306: Add SignatureMethodParameterSpec subclass for RSASSA-PSS params [v7]
> This enhancement contains the following code changes: > > 1. Create a new public API `javax/xml/crypto/dsig/spec/RSAPSSParameterSpec` > and remove the internal one. > 2. Update marshaling and unmarshaling code inside `DOMRSAPSSSignatureMethod` > so it understands extra fields in `PSSParameterSpec` and is aware of the > defaults in both directions. > 3. Update `DOMSignedInfo` so that secure validation can restrict > `DigestMethod` used inside `RSAPSSParameterSpec` > 4. Tests Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: more spec clarification - Changes: - all: https://git.openjdk.java.net/jdk/pull/3181/files - new: https://git.openjdk.java.net/jdk/pull/3181/files/a28a8fa1..df55414c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3181=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3181=05-06 Stats: 12 lines in 2 files changed: 0 ins; 2 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/3181.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3181/head:pull/3181 PR: https://git.openjdk.java.net/jdk/pull/3181
Re: RFR: 8241306: Add SignatureMethodParameterSpec subclass for RSASSA-PSS params [v6]
On Tue, 13 Apr 2021 17:07:19 GMT, Sean Mullan wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> spec clarification
>
> src/java.xml.crypto/share/classes/javax/xml/crypto/dsig/SignatureMethod.java
> line 247:
>
>> 245: * as the signature algorithm, the default parameter as defined in
>> 246: * https://tools.ietf.org/html/rfc6931#section-2.3.9;>RFC
>> 6931 Section 2.3.9
>> 247: * is used and this default parameter will also be returned by the
>
> WE should mention/link to the type returned. Suggest breaking this into two
> sentences:
>
> `If the {@code params} parameter is {@code null} when calling {@link
> XMLSignatureFactory#newSignatureMethod} with {@code RSA_PSS} as the signature
> algorithm, the default parameter as defined in href="https://tools.ietf.org/html/rfc6931#section-2.3.9;>RFC 6931 Section
> 2.3.9 is used. This default parameter is represented as an {@link
> RSAPSSParameterSpec} type and returned by the {@link
> SignatureMethod#getParameterSpec()} method.`
OK, and some other tweaks. New commit pushed.
-
PR: https://git.openjdk.java.net/jdk/pull/3181
Integrated: 8265138: Simplify DerUtils::checkAlg
On Tue, 13 Apr 2021 14:36:46 GMT, Weijun Wang wrote: > This fix makes `DerUtils::checkAlg` simple and clear, no more raw `Object` > and ambiguous string arguments. This pull request has now been integrated. Changeset: 9cd5400d Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/9cd5400d Stats: 37 lines in 3 files changed: 3 ins; 1 del; 33 mod 8265138: Simplify DerUtils::checkAlg Reviewed-by: xuelei - PR: https://git.openjdk.java.net/jdk/pull/3467
Re: RFR: 8241306: Add SignatureMethodParameterSpec subclass for RSASSA-PSS params [v6]
> This enhancement contains the following code changes: > > 1. Create a new public API `javax/xml/crypto/dsig/spec/RSAPSSParameterSpec` > and remove the internal one. > 2. Update marshaling and unmarshaling code inside `DOMRSAPSSSignatureMethod` > so it understands extra fields in `PSSParameterSpec` and is aware of the > defaults in both directions. > 3. Update `DOMSignedInfo` so that secure validation can restrict > `DigestMethod` used inside `RSAPSSParameterSpec` > 4. Tests Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: spec clarification - Changes: - all: https://git.openjdk.java.net/jdk/pull/3181/files - new: https://git.openjdk.java.net/jdk/pull/3181/files/ce1678ea..a28a8fa1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3181=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3181=04-05 Stats: 67 lines in 5 files changed: 34 ins; 24 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/3181.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3181/head:pull/3181 PR: https://git.openjdk.java.net/jdk/pull/3181
Re: RFR: 8241306: Add SignatureMethodParameterSpec subclass for RSASSA-PSS params [v5]
On Mon, 12 Apr 2021 15:25:09 GMT, Weijun Wang wrote: >> This enhancement contains the following code changes: >> >> 1. Create a new public API `javax/xml/crypto/dsig/spec/RSAPSSParameterSpec` >> and remove the internal one. >> 2. Update marshaling and unmarshaling code inside `DOMRSAPSSSignatureMethod` >> so it understands extra fields in `PSSParameterSpec` and is aware of the >> defaults in both directions. >> 3. Update `DOMSignedInfo` so that secure validation can restrict >> `DigestMethod` used inside `RSAPSSParameterSpec` >> 4. Tests > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > no RSAPSSParameterSpec::toString and some test comments New commit pushed. If you're OK with the change, I'll update the CSR as well. - PR: https://git.openjdk.java.net/jdk/pull/3181
Re: RFR: 8241306: Add SignatureMethodParameterSpec subclass for RSASSA-PSS params [v2]
On Tue, 13 Apr 2021 14:22:47 GMT, Sean Mullan wrote:
>> You are right that the overridable methods are elsewhere
>> (`XMLSignatureFactory::newSignatureMethod` and
>> `SignatureMethod::getParameterSpec`), but I still feel it a little strange
>> to move the default parameter of one particular algorithm to the general
>> interface `SignatureMethod` (this is similar to talking about EC curve names
>> in `KeyPairGenerator`). It seems more natural to talk about this inside a
>> class which is specific to the RSASSA-PSS algorithm and
>> `RSAPSSParameterSpec` is the only public API we can find. We can add
>> something like "specify null if you want a default spec" to
>> `XMLSignatureFactory::newSignatureMethod` but it does not have enough space
>> to describe "how default values are determined" for each algorithm.
>>
>> I understand `@implSpec` is for implementations of a class or a method, but
>> does not mean the words must be put inside that exact class and method?
>> Maybe not necessarily.
>>
>> As for making `@implSpec` more specific, at signing time, we can only either
>> provide a `RSAPSSParameterSpec` or not one, so there is only one default
>> value which is SHA256_RSA_MGF1. On the other hand, at validation time we
>> might be parsing a partial-filled `RSAPSSParams` node and that's where
>> default salt and default trailer field show up.
>>
>> Also about the return value of `SignatureMethod::getParameterSpec`, are you
>> suggesting that for RSASSA-PSS it must be non null? This can be specified
>> somewhere but we need to find a place. For the existing `HMACParameterSpec`,
>> the method returns null if none is set. Do we treat that as no spec at all
>> (but for RSASSA-PSS there is always one)?
>>
>> My current opinion is that we still document all these in
>> `RSAPSSParameterSpec` but be very clear that this part is for
>> `XMLSignatureFactory::newSignatureMethod` and that part is for
>> `SignatureMethod::getParameterSpec`, etc, etc.
>
> I think it is worth asking the TCK team about this. After further thought, I
> am not sure `implSpec` is correct because the implementation is not in the
> classes, it is in the provider. I now think it needs to be part of the API
> contract, so that all implementations are compliant. `SignatureMethod`
> already defines the RSA_PSS algorithm constant, so it doesn't seem so
> out-of-place to put the specification there, something like:
>
>
> /**
> * The http://www.w3.org/2007/05/xmldsig-more#rsa-pss;>
> * RSASSA-PSS signature method algorithm URI.
> *
> * If the parameter is not specified when calling
> `XMLSignatureFactory.newSignatureMethod` with
> * RSA_PSS as the signature algorithm, the default parameter is used, which
> uses SHA-256 as the
> * {@code DigestMethod}, MGF1 with SHA-256 as the
> * {@code MaskGenerationFunction}, 32 as {@code SaltLength}, and 1 as
> * {@code TrailerField}. This is equivalent to the parameter-less signature
> * method {@link SignatureMethod#SHA256_RSA_MGF1 SHA256_RSA_MGF1} as defined
> * in https://tools.ietf.org/html/rfc6931#section-2.3.10;>RFC 6931.
> This default
> * parameter is returned by the `getParameterSpec` method.
> *
> * @since 17
> */
> String RSA_PSS = "http://www.w3.org/2007/05/xmldsig-more#rsa-pss;;
>
> Forget my comment about making `implSpec` more specific, I think that is now
> implied by RFC 3161. I also think the above specification would take care of
> my returning null comment, as all implementations would need to comply. I
> just didn't want applications to have to have code that checks for null and
> then don't know whether that means it is the default parameters or something
> else since it was implementation specific.
Great, I forgot about this string. This *is* a proper place to put the text.
-
PR: https://git.openjdk.java.net/jdk/pull/3181
RFR: 8265138: Simplify DerUtils::checkAlg
This fix makes `DerUtils::checkAlg` simple and clear, no more raw `Object` and ambiguous string arguments. - Commit messages: - 8265138: Simplify DerUtils::checkAlg Changes: https://git.openjdk.java.net/jdk/pull/3467/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3467=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8265138 Stats: 37 lines in 3 files changed: 3 ins; 1 del; 33 mod Patch: https://git.openjdk.java.net/jdk/pull/3467.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3467/head:pull/3467 PR: https://git.openjdk.java.net/jdk/pull/3467
Re: RFR: 8241306: Add SignatureMethodParameterSpec subclass for RSASSA-PSS params [v2]
On Mon, 12 Apr 2021 17:29:55 GMT, Sean Mullan wrote: >> I added the new lines as `@implNote` and kept the old `@implSpec` there >> (since it's still a requirement for implementations). New commit pushed. CSR >> updated as well. > > Ok, I understand now. I think `@implSpec` (and probably the `@implNote`) are > in the wrong class. `@implSpec` means the implementation of this class. But > this class is final and does not contain that logic. The logic of > specifying/returning the defaults is in the JDK (XMLDSig provider) > implementation of `SignatureMethod`. So I think it belongs there. In this > class, you could add a sentence/link to `SignatureMethod`, something like > "See SignatureMethod for how default values are determined when the > parameters are not specified." > > Also, I think the `@implSpec` needs to be more specific, and also cover the > cases where some, but not all of the parameters are specified and defaults > are then used. For this, you will need to be more general, as the default > salt length is based on what hash algorithm is being used. > > As for `@implNote`, this probably could use more discussion, but it might be > better to make this part of the specification. If some implementations can > return null, and others return defaults, it complicates the application's > logic. The RFC has clearly specified what the defaults should be, so maybe > the easiest thing to do is to make all implementations comply by also making > it part of the API contract, and hiding the XML details as to whether the > parameter was actually specified or not, which should not matter to > applications. You are right that the overridable methods are elsewhere (`XMLSignatureFactory::newSignatureMethod` and `SignatureMethod::getParameterSpec`), but I still feel it a little strange to move the default parameter of one particular algorithm to the general interface `SignatureMethod` (this is similar to talking about EC curve names in `KeyPairGenerator`). It seems more natural to talk about this inside a class which is specific to the RSASSA-PSS algorithm and `RSAPSSParameterSpec` is the only public API we can find. We can add something like "specify null if you want a default spec" to `XMLSignatureFactory::newSignatureMethod` but it does not have enough space to describe "how default values are determined" for each algorithm. I understand `@implSpec` is for implementations of a class or a method, but does not mean the words must be put inside that exact class and method? Maybe not necessarily. As for making `@implSpec` more specific, at signing time, we can only either provide a `RSAPSSParameterSpec` or not one, so there is only one default value which is SHA256_RSA_MGF1. On the other hand, at validation time we might be parsing a partial-filled `RSAPSSParams` node and that's where default salt and default trailer field show up. Also about the return value of `SignatureMethod::getParameterSpec`, are you suggesting that for RSASSA-PSS it must be non null? This can be specified somewhere but we need to find a place. For the existing `HMACParameterSpec`, the method returns null if none is set. Do we treat that as no spec at all (but for RSASSA-PSS there is always one)? My current opinion is that we still document all these in `RSAPSSParameterSpec` but be very clear that this part is for `XMLSignatureFactory::newSignatureMethod` and that part is for `SignatureMethod::getParameterSpec`, etc, etc. - PR: https://git.openjdk.java.net/jdk/pull/3181
Re: RFR: 8241306: Add SignatureMethodParameterSpec subclass for RSASSA-PSS params [v5]
> This enhancement contains the following code changes: > > 1. Create a new public API `javax/xml/crypto/dsig/spec/RSAPSSParameterSpec` > and remove the internal one. > 2. Update marshaling and unmarshaling code inside `DOMRSAPSSSignatureMethod` > so it understands extra fields in `PSSParameterSpec` and is aware of the > defaults in both directions. > 3. Update `DOMSignedInfo` so that secure validation can restrict > `DigestMethod` used inside `RSAPSSParameterSpec` > 4. Tests Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: no RSAPSSParameterSpec::toString and some test comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/3181/files - new: https://git.openjdk.java.net/jdk/pull/3181/files/82c29fea..ce1678ea Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3181=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3181=03-04 Stats: 34 lines in 2 files changed: 15 ins; 15 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/3181.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3181/head:pull/3181 PR: https://git.openjdk.java.net/jdk/pull/3181
Re: RFR: 8241306: Add SignatureMethodParameterSpec subclass for RSASSA-PSS params [v2]
On Mon, 12 Apr 2021 12:42:23 GMT, Sean Mullan wrote:
>> src/java.xml.crypto/share/classes/javax/xml/crypto/dsig/spec/RSAPSSParameterSpec.java
>> line 139:
>>
>>> 137:
>>> 138: @Override
>>> 139: public String toString() {
>>
>> Add specification.
>
> Actually, on second thought, I don't think we should override `toString`.
> None of the other spec classes override `toString`.
I'll remove it.
-
PR: https://git.openjdk.java.net/jdk/pull/3181
Re: RFR: 8241306: Add SignatureMethodParameterSpec subclass for RSASSA-PSS params [v4]
On Mon, 12 Apr 2021 13:23:44 GMT, Sean Mullan wrote: > The `@implSpec` looks good. I view the `@implNote` more like an `@apiNote` > though. API notes are for "[commentary, rationale, or examples pertaining to the API](https://bugs.openjdk.java.net/browse/JDK-8068562)". I'm not sure if `getParameterSpec()` must always return a non-null object when algorithm is RSASSA-PSS. The spec says "may be null if not specified". So I used "In this implementation". > test/lib/jdk/test/lib/security/XMLUtils.java line 63: > >> 61: import java.security.spec.PSSParameterSpec; >> 62: import java.util.*; >> 63: > > Can you add some comments about this class, e.g., "A collection of test > utility methods for parsing, validating and generating XML Signatures". OK for this and the next two. - PR: https://git.openjdk.java.net/jdk/pull/3181
Re: RFR: 8241306: Add SignatureMethodParameterSpec subclass for RSASSA-PSS params
On Wed, 24 Mar 2021 21:36:21 GMT, Weijun Wang wrote: > This enhancement contains the following code changes: > > 1. Create a new public API `javax/xml/crypto/dsig/spec/RSAPSSParameterSpec` > and remove the internal one. > 2. Update marshaling and unmarshaling code inside `DOMRSAPSSSignatureMethod` > so it understands extra fields in `PSSParameterSpec` and is aware of the > defaults in both directions. > 3. Update `DOMSignedInfo` so that secure validation can restrict > `DigestMethod` used inside `RSAPSSParameterSpec` > 4. Tests New commit pushed and CSR at https://bugs.openjdk.java.net/browse/JDK-8259575 updated. How do you find the `@implSpec` and `@implNote` in `RSAPSSParameterSpec.java`? - PR: https://git.openjdk.java.net/jdk/pull/3181
Re: RFR: 8241306: Add SignatureMethodParameterSpec subclass for RSASSA-PSS params [v4]
> This enhancement contains the following code changes: > > 1. Create a new public API `javax/xml/crypto/dsig/spec/RSAPSSParameterSpec` > and remove the internal one. > 2. Update marshaling and unmarshaling code inside `DOMRSAPSSSignatureMethod` > so it understands extra fields in `PSSParameterSpec` and is aware of the > defaults in both directions. > 3. Update `DOMSignedInfo` so that secure validation can restrict > `DigestMethod` used inside `RSAPSSParameterSpec` > 4. Tests Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: more digest methods and spec change - Changes: - all: https://git.openjdk.java.net/jdk/pull/3181/files - new: https://git.openjdk.java.net/jdk/pull/3181/files/5b88fff4..82c29fea Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3181=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3181=02-03 Stats: 8 lines in 2 files changed: 5 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/3181.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3181/head:pull/3181 PR: https://git.openjdk.java.net/jdk/pull/3181
Re: RFR: 8241306: Add SignatureMethodParameterSpec subclass for RSASSA-PSS params [v3]
On Fri, 9 Apr 2021 17:23:05 GMT, Sean Mullan wrote:
>> src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/SignatureBaseRSA.java
>> line 588:
>>
>>> 586:
>>> 587: public enum DigestAlgorithm {
>>> 588: //SHA1("SHA-1", DigestMethod.SHA1, 20),
>>
>> Do we want to support "SHA-1"? It's considered weak and not the default but
>> the RFC has not disabled it. Since we already have secure validation on by
>> default, it does seem to be a security issue.
>>
>> The "RSASSA-PSS without Parameters" section at
>> https://tools.ietf.org/html/rfc6931#section-2.3.10 also lists SHA-224 and
>> SHA3-**. We should probably support them as well, or at least make sure we
>> support the same algorithms in "without Parameters" and "with Parameters".
>
> I'm ok with not supporting SHA-1, although adding it would not be a security
> issue. It is blocked by default now, but it can be re-enabled, and SHA-1 in
> general is still available in the JDK.
>
> I'm fine with adding support for SHA-224 and SHA-3 as part of this issue. You
> can add support for all the algorithms that we have the underlying crypto
> support for.
Not sure if I got it, are you OK with adding SHA-1? It must be listed here to
be supported.
-
PR: https://git.openjdk.java.net/jdk/pull/3181
Re: RFR: 8241306: Add SignatureMethodParameterSpec subclass for RSASSA-PSS params [v3]
On Fri, 9 Apr 2021 16:44:07 GMT, Sean Mullan wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> spec word change, no hashCode and equals, test change
>
> src/java.xml.crypto/share/classes/javax/xml/crypto/dsig/spec/RSAPSSParameterSpec.java
> line 74:
>
>> 72: * {@code MaskGenerationFunction}, 32 as {@code SaltLength}, and 1 as
>> 73: * {@code TrailerField}. This is equivalent to the parameter-less
>> signature
>> 74: * method as defined by
>> http://www.w3.org/2007/05/xmldsig-more#sha256-rsa-MGF1.
>
> http://www.w3.org/2007/05/xmldsig-more#sha256-rsa-MGF1 is just a placeholder
> page for the namespace. I would instead link to
> `SignatureMethod.SHA256_RSA_MGF1` and also reference the RFC for more
> information. How about:
>
> `This is equivalent to the {@link SignatureMethod#SHA256_RSA_MGF1
> parameter-less signature method} as defined in href="https://www.ietf.org/rfc/rfc6931.txt#section-2.3.10;>RFC 6931.
> `
Correct.
How about:
This is equivalent to the parameter-less signature
method {@link SignatureMethod#SHA256_RSA_MGF1 SHA256_RSA_MGF1} as defined
in https://tools.ietf.org/html/rfc6931#section-2.3.10;>RFC 6931.
SHA256_RSA_MGF1 is not the only parameter-less method so I prefer showing its
name.
-
PR: https://git.openjdk.java.net/jdk/pull/3181
Integrated: 8264864: Multiple byte tag not supported by ASN.1 encoding
On Thu, 8 Apr 2021 01:06:47 GMT, Weijun Wang wrote: > This code change does not intend to support multiple byte tags. Instead, it > aims to fail more gracefully when such a tag is encountered. For `DerValue` > constructors from an encoding (type I), an `IOException` will be thrown since > it's already in the throws clause. For constructors from tag and value (type > II), an `IllegalArgumentException` will be thrown. All existing type II > callers inside JDK use tag numbers smaller than 31. This pull request has now been integrated. Changeset: 3d2b4cc5 Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/3d2b4cc5 Stats: 83 lines in 2 files changed: 82 ins; 0 del; 1 mod 8264864: Multiple byte tag not supported by ASN.1 encoding Reviewed-by: xuelei - PR: https://git.openjdk.java.net/jdk/pull/3391
Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v5]
> This code change does not intend to support multiple byte tags. Instead, it > aims to fail more gracefully when such a tag is encountered. For `DerValue` > constructors from an encoding (type I), an `IOException` will be thrown since > it's already in the throws clause. For constructors from tag and value (type > II), an `IllegalArgumentException` will be thrown. All existing type II > callers inside JDK use tag numbers smaller than 31. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: add offset info in exception message - Changes: - all: https://git.openjdk.java.net/jdk/pull/3391/files - new: https://git.openjdk.java.net/jdk/pull/3391/files/004b35e8..9cb908bf Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3391=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3391=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3391.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3391/head:pull/3391 PR: https://git.openjdk.java.net/jdk/pull/3391
Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v3]
On Thu, 8 Apr 2021 17:18:50 GMT, Jamil Nimeh wrote: >> I don't want to go on reading the following bytes to find out what the >> intended tag number is, because that somehow shows I do understand the >> encoding _a lot_ but still don't want to support it (well, actually I only >> understand _a little_). There are only 2 kinds of tags: one <= 30 and one >= >> 31. IMHO, the message has already expressed the meaning that we only support >> the 1st one. >> >> An alternative message I can think of is "Unsupported tag byte: 0xBF", but >> it looks too cryptic. > > I think that is fair. If you don't want to read ahead like that, what about > using the "offset" or "pos" field to give a message like "Tag number over 30 > at offset NN is not supported" (something like that, at least) Maybe don't > worry about the tag value itself, but at least the position in the data > stream. Just a suggestion only, no strong feelings about this either way. There _is_ an `offset` value here but I have really no idea if the user knows where to count from. If we say "offset" then we probably need to tell what data block we are talking about. What if the DerValue is just a portion of a bigger data block? That said, if you really like it, I can add an offset like "tag byte at offset ". I just hope the user can find it. - PR: https://git.openjdk.java.net/jdk/pull/3391
Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v3]
On Thu, 8 Apr 2021 16:58:24 GMT, Jamil Nimeh wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> update exception wordings
>
> src/java.base/share/classes/sun/security/util/DerValue.java line 322:
>
>> 320: tag = buf[pos++];
>> 321: if ((tag & 0x1f) == 0x1f) {
>> 322: throw new IOException("Tag number over 30 is not
>> supported");
>
> Would it be useful for these types of exception messages to either display
> the offending tag value or perhaps the tag offset? Just thinking it might be
> a nice thing for the recipient to know where in the DER encoding the issue is.
I don't want to go on reading the following bytes to find out what the intended
tag number is, because that somehow shows I do understand the encoding _a lot_
but still don't want to support it (well, actually I only understand _a
little_). There are only 2 kinds of tags: one <= 30 and one >= 31. IMHO, the
message has already expressed the meaning that we only support the 1st one.
An alternative message I can think of is "Unsupported tag byte: 0xBF", but it
looks too cryptic.
-
PR: https://git.openjdk.java.net/jdk/pull/3391
Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v3]
On Thu, 8 Apr 2021 15:53:10 GMT, Xue-Lei Andrew Fan wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> update exception wordings
>
> src/java.base/share/classes/sun/security/util/DerValue.java line 225:
>
>> 223: DerValue(byte tag, byte[] buffer, int start, int end, boolean
>> allowBER) {
>> 224: if ((tag & 0x1f) == 0x1f) {
>> 225: throw new IllegalArgumentException("Tag number 31 is not
>> supported");
>
> As number 31 just means the tag is bigger than 31, Is it more accuracy by
> using "Tag number over 30 is not supported"?
Well, it's a little delicate here. Even if we support multi-byte tag one day,
this constructor will still only be used to create a single-byte tag
`DerValue`, and it's illegal for a single byte tag to end with 0x1f. So the
words above is to remind people that they cannot create a tag number 31
`DerValue` just because it seems it still fits into the 5 bits. Precisely, the
words should be "this constructor only supports tag number between 0 and 30",
but... I'll choose your words.
-
PR: https://git.openjdk.java.net/jdk/pull/3391
Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v4]
> This code change does not intend to support multiple byte tags. Instead, it > aims to fail more gracefully when such a tag is encountered. For `DerValue` > constructors from an encoding (type I), an `IOException` will be thrown since > it's already in the throws clause. For constructors from tag and value (type > II), an `IllegalArgumentException` will be thrown. All existing type II > callers inside JDK use tag numbers smaller than 31. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: change exception message - Changes: - all: https://git.openjdk.java.net/jdk/pull/3391/files - new: https://git.openjdk.java.net/jdk/pull/3391/files/9b0f9db9..004b35e8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3391=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3391=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3391.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3391/head:pull/3391 PR: https://git.openjdk.java.net/jdk/pull/3391
Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v2]
On Thu, 8 Apr 2021 03:46:07 GMT, Xue-Lei Andrew Fan wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> make sure test fails before code change
>
> src/java.base/share/classes/sun/security/util/DerValue.java line 322:
>
>> 320: tag = buf[pos++];
>> 321: if ((tag & 0x1f) == 0x1f) {
>> 322: throw new IOException("Tag number cannot exceed 30");
>
> It may be safe if not support multiple bytes tag in the current
> implementation of JDK, especially the ASN.1 implementation is private.
> However, multiple bytes tag is a legal form of ASN.1 encoding, I think. It
> would be nice to have a comment to state that this form is not support yet,
> and we may consider it in the future if needed. It may be helpful for future
> code maintenance.
>
> The exception message, "Tag number cannot exceed 30", may be not accuracy. I
> think tag number can exceed 30 per the specification, but JDK does not
> support it yet because we did not run into such tags in practice. I may use
> some words like: "Tag number exceed 30 is not supported".
Messages updated. "exceed" is a verb and I'm not sure whether to choose
"exceeding" or "that exceeds" so finally use "over". Thanks!
-
PR: https://git.openjdk.java.net/jdk/pull/3391
Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v3]
> This code change does not intend to support multiple byte tags. Instead, it > aims to fail more gracefully when such a tag is encountered. For `DerValue` > constructors from an encoding (type I), an `IOException` will be thrown since > it's already in the throws clause. For constructors from tag and value (type > II), an `IllegalArgumentException` will be thrown. All existing type II > callers inside JDK use tag numbers smaller than 31. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: update exception wordings - Changes: - all: https://git.openjdk.java.net/jdk/pull/3391/files - new: https://git.openjdk.java.net/jdk/pull/3391/files/46b3700b..9b0f9db9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3391=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3391=01-02 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/3391.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3391/head:pull/3391 PR: https://git.openjdk.java.net/jdk/pull/3391
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v6]
On Thu, 8 Apr 2021 01:48:20 GMT, Hai-May Chao wrote: >> Please review the changes that adds the -signer option to keytool >> -genkeypair command. As key agreement algorithms do not have a signing >> algorithm, the specified signer's private key will be used to sign and >> generate a key agreement certificate. >> CSR review is at: https://bugs.openjdk.java.net/browse/JDK-8264325 > > Hai-May Chao has updated the pull request incrementally with one additional > commit since the last revision: > > test update with comment Well done. - Marked as reviewed by weijun (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3281
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v5]
On Thu, 8 Apr 2021 01:42:16 GMT, Hai-May Chao wrote:
>> test/jdk/sun/security/tools/keytool/GenKeyPairSigner.java line 96:
>>
>>> 94:
>>> 95: Certificate[] certChain = kstore.getCertificateChain("e1");
>>> 96: if (certChain.length != 2) {
>>
>> Try using `Asserts` class in `/test/lib` to make code simpler. Also, why not
>> throw an exception but call `System.exit(1)`? We usually do not call this
>> method in a test because the test framework must take great care so that
>> itself does not get terminated.
>
> Changed to throw the exception for errors. Meanwhile, the test is pretty
> straightforward/simple, and using if comparison should serve its testing need
> and it does not make the code complicated.
You can choose your style, but `Asserts.assertEquals(certChain.length, 2,
"Generated cert chain is in error")` is definitely simpler and will give you
more info when it fails.
-
PR: https://git.openjdk.java.net/jdk/pull/3281
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v5]
On Thu, 8 Apr 2021 01:48:20 GMT, Hai-May Chao wrote: >> In testSignerJKS() has made sure that the new entry created with new key >> password can *only* be accessed when a correct key password is provided in >> order to retrieve the corresponding signer’s private key. > > The new entry protected by the new key password is an existing function, and > its testing should have been covered. OK. - PR: https://git.openjdk.java.net/jdk/pull/3281
Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v2]
> This code change does not intend to support multiple byte tags. Instead, it > aims to fail more gracefully when such a tag is encountered. For `DerValue` > constructors from an encoding (type I), an `IOException` will be thrown since > it's already in the throws clause. For constructors from tag and value (type > II), an `IllegalArgumentException` will be thrown. All existing type II > callers inside JDK use tag numbers smaller than 31. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: make sure test fails before code change - Changes: - all: https://git.openjdk.java.net/jdk/pull/3391/files - new: https://git.openjdk.java.net/jdk/pull/3391/files/d25d3fb2..46b3700b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3391=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3391=00-01 Stats: 11 lines in 1 file changed: 6 ins; 1 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/3391.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3391/head:pull/3391 PR: https://git.openjdk.java.net/jdk/pull/3391
RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding
This code change does not intend to support multiple byte tags. Instead, it aims to fail more gracefully when such a tag is encountered. For `DerValue` constructors from an encoding (type I), an `IOException` will be thrown since it's already in the throws clause. For constructors from tag and value (type II), an `IllegalArgumentException` will be thrown. All existing type II callers inside JDK use tag numbers smaller than 31. - Commit messages: - 8264864: Multiple byte tag not supported by ASN.1 encoding Changes: https://git.openjdk.java.net/jdk/pull/3391/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3391=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8264864 Stats: 78 lines in 2 files changed: 77 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3391.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3391/head:pull/3391 PR: https://git.openjdk.java.net/jdk/pull/3391
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v5]
On Wed, 7 Apr 2021 23:17:53 GMT, Hai-May Chao wrote:
>> Please review the changes that adds the -signer option to keytool
>> -genkeypair command. As key agreement algorithms do not have a signing
>> algorithm, the specified signer's private key will be used to sign and
>> generate a key agreement certificate.
>> CSR review is at: https://bugs.openjdk.java.net/browse/JDK-8264325
>
> Hai-May Chao has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update with review comments
No comment on src side.
src/java.base/share/classes/sun/security/tools/keytool/CertAndKeyGen.java line
88:
> 86: */
> 87: public CertAndKeyGen (String keyType, String sigAlg, String
> providerName)
> 88: throws NoSuchAlgorithmException, NoSuchProviderException
Indent the line 8 spaces further.
test/jdk/sun/security/tools/keytool/GenKeyPairSigner.java line 96:
> 94:
> 95: Certificate[] certChain = kstore.getCertificateChain("e1");
> 96: if (certChain.length != 2) {
Try using `Asserts` class in `/test/lib` to make code simpler. Also, why not
throw an exception but call `System.exit(1)`? We usually do not call this
method in a test because the test framework must take great care so that itself
does not get terminated.
test/jdk/sun/security/tools/keytool/GenKeyPairSigner.java line 299:
> 297: System.exit(1);
> 298: }
> 299:
Since you are here, you can check if the new entry is indeed protected by the
new key password.
-
PR: https://git.openjdk.java.net/jdk/pull/3281
Integrated: 8262389: Use permitted_enctypes if default_tkt_enctypes or default_tgs_enctypes is not present
On Thu, 25 Feb 2021 20:56:42 GMT, Weijun Wang wrote: > When default_tkt_enctypes or default_tgs_enctypes, use the value of > permitted_enctypes if it exists. > > Please also review the CSR at > https://bugs.openjdk.java.net/browse/JDK-8262391 and release note at > https://bugs.openjdk.java.net/browse/JDK-8262401. This pull request has now been integrated. Changeset: eb5c097b Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/eb5c097b Stats: 60 lines in 2 files changed: 60 ins; 0 del; 0 mod 8262389: Use permitted_enctypes if default_tkt_enctypes or default_tgs_enctypes is not present Reviewed-by: mullan - PR: https://git.openjdk.java.net/jdk/pull/2729
Withdrawn: 8241306: Add SignatureMethodParameterSpec subclass for RSASSA-PSS params
On Wed, 24 Mar 2021 21:36:21 GMT, Weijun Wang wrote: > This enhancement contains the following code changes: > > 1. Create a new public API `javax/xml/crypto/dsig/spec/RSAPSSParameterSpec` > and remove the internal one. > 2. Update marshaling and unmarshaling code inside `DOMRSAPSSSignatureMethod` > so it understands extra fields in `PSSParameterSpec` and is aware of the > defaults in both directions. > 3. Update `DOMSignedInfo` so that secure validation can restrict > `DigestMethod` used inside `RSAPSSParameterSpec` > 4. Tests This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/3181
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v4]
On Fri, 2 Apr 2021 04:03:50 GMT, Xue-Lei Andrew Fan wrote: >> Maybe we don't need to resolve it in this code change. If we look carefully >> at RFC 8410 Sections 10.1 and 10.2, it shows the X25519 certificate in 10.2 >> is using the signer's SKID in 10.1 as its own SKID and it has no AKID. >> Currently, keytool will generate a new SKID and use signer's SKID as AKID. >> If we really want to generate a certificate that's identical to the one in >> the RFC, we'll need a way to tell keytool to omit the AKID (something like >> "-ext akid=none"). > >> Maybe we don't need to resolve it in this code change. If we look carefully >> at RFC 8410 Sections 10.1 and 10.2, it shows the X25519 certificate in 10.2 >> is using the signer's SKID in 10.1 as its own SKID and it has no AKID. >> Currently, keytool will generate a new SKID and use signer's SKID as AKID. >> If we really want to generate a certificate that's identical to the one in >> the RFC, we'll need a way to tell keytool to omit the AKID (something like >> "-ext akid=none"). > > Better to use AKID for certification path building. I don't mean we will remove it by default. Just think there needs a way to remove either AKID or SKID, because we always generate them automatically. - PR: https://git.openjdk.java.net/jdk/pull/3281
