Re: RFR: 8290367: Update default value and extend the scope of com.sun.jndi.ldap.object.trustSerialData system property [v2]
On Mon, 12 Sep 2022 10:53:10 GMT, Jaikiran Pai wrote:
> is there a convention in our documentation where we state that the value is
> case insensitive?
I think there isn't. Therefore, it will be cleaner to update the property
description in `module-info.java` to something like: `"the system property
value can be set to {@code true} (case insensitive)"`
And to update the test to check the `case insensitive` clause:
* @run main/othervm -Dcom.sun.jndi.ldap.object.trustSerialData=TrUe
* RemoteLocationAttributeTest
Thanks
-
PR: https://git.openjdk.org/jdk/pull/10228
Re: RFR: 8290367: Update default value and extend the scope of com.sun.jndi.ldap.object.trustSerialData system property [v2]
On Mon, 12 Sep 2022 12:06:07 GMT, Jaikiran Pai wrote:
>> Aleksei Efimov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add run for the SP w/o value, formatting/wording updates
>
> test/jdk/com/sun/jndi/ldap/objects/RemoteLocationAttributeTest.java line 64:
>
>> 62: SocketAddress sockAddr = new InetSocketAddress(
>> 63: InetAddress.getLoopbackAddress(), 0);
>> 64: serverSocket.bind(sockAddr);
>
> Perhaps we should `close()` this `serverSocket` in a finally block to cleanly
> shutdown?
Good idea, thanks. Will add it to try-with-resources statement, like:
`try (serverSocket) {
`
-
PR: https://git.openjdk.org/jdk/pull/10228
Re: RFR: 8290367: Update default value and extend the scope of com.sun.jndi.ldap.object.trustSerialData system property [v2]
On Fri, 9 Sep 2022 16:55:44 GMT, Aleksei Efimov wrote: >> ### Summary of the change >> >> The LDAP Naming Service Provider implementation's default settings are >> changed to disallow deserialization and reconstruction of Java objects from >> different LDAP attributes (RFC 2713). Currently, only the deserialization is >> controlled by the `com.sun.jndi.ldap.object.trustSerialData` system >> property, and it is allowed by default. >> The change proposed here switches the default value of the` >> com.sun.jndi.ldap.object.trustSerialData `system property to `"false"`, and >> also extends its scope to cover the reconstruction of RMI remote objects >> from the `javaRemoteLocation` LDAP attribute. >> >> CSR for this change can be viewed >> [here](https://bugs.openjdk.org/browse/JDK-8290369). >> >> ### List of code changes >> - Switch the default value of the 'com.sun.jndi.ldap.object.trustSerialData' >> system property to "false". >> >> - Extend the scope of the property to also cover the reconstruction of RMI >> remote objects from the deprecated 'javaRemoteLocation' LDAP attribute. >> >> - Document the support for `javaRemoteLocation` and the >> `javaReferenceAddress` LDAP attributes in `java.naming`'s module-info. >> >> ### Test changes >> - New `test/jdk/com/sun/jndi/ldap/objects/RemoteLocationAttributeTest.java` >> test has been added to test that `com.sun.jndi.ldap.object.trustSerialData` >> system property can be used to control reconstruction of RMI objects from >> the `javaRemoteLocation` LDAP attribute. >> >> - `test/jdk/javax/naming/module/RunBasic.java` was modified to pass >> `com.sun.jndi.ldap.object.trustSerialData=true` to the sub-tests that rely >> on reconstruction/deserialization from LDAP attributes. >> >> - During the update for `test/jdk/javax/naming/module/RunBasic.java`, it was >> spotted that sub-tests apps launched in separate processes were returning >> the '0' exit value irrelevant to their execution status. All these sub-tests >> were modified to throw an exception when failure is observed. It helps to >> ensure that the exit value of launched process is not '0' for failed >> sub-tests. >> >> ### Testing >> >> `tier1`-`tier3` and JNDI regression/JCK tests not showing any failures >> related to this change. >> No failures observed for the modified regression tests. > > Aleksei Efimov has updated the pull request incrementally with one additional > commit since the last revision: > > Add run for the SP w/o value, formatting/wording updates test/jdk/com/sun/jndi/ldap/objects/RemoteLocationAttributeTest.java line 64: > 62: SocketAddress sockAddr = new InetSocketAddress( > 63: InetAddress.getLoopbackAddress(), 0); > 64: serverSocket.bind(sockAddr); Perhaps we should `close()` this `serverSocket` in a finally block to cleanly shutdown? - PR: https://git.openjdk.org/jdk/pull/10228
Re: RFR: 8290367: Update default value and extend the scope of com.sun.jndi.ldap.object.trustSerialData system property [v2]
On Mon, 12 Sep 2022 10:48:01 GMT, Jaikiran Pai wrote: >> Aleksei Efimov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add run for the SP w/o value, formatting/wording updates > > src/java.naming/share/classes/com/sun/jndi/ldap/VersionHelper.java line 60: > >> 58: >> 59: // System property to control whether classes is allowed to be >> loaded from >> 60: // 'javaSerializedData' attribute > > Hello Aleksei, should we also update this code comment to make a mention of > the additional attributes this system property now controls? Hi Jai, Thanks for spotting that - will update this and other code comments in the `VersionHelper` class. - PR: https://git.openjdk.org/jdk/pull/10228
Re: RFR: 8290367: Update default value and extend the scope of com.sun.jndi.ldap.object.trustSerialData system property [v2]
On Fri, 9 Sep 2022 16:55:44 GMT, Aleksei Efimov wrote: >> ### Summary of the change >> >> The LDAP Naming Service Provider implementation's default settings are >> changed to disallow deserialization and reconstruction of Java objects from >> different LDAP attributes (RFC 2713). Currently, only the deserialization is >> controlled by the `com.sun.jndi.ldap.object.trustSerialData` system >> property, and it is allowed by default. >> The change proposed here switches the default value of the` >> com.sun.jndi.ldap.object.trustSerialData `system property to `"false"`, and >> also extends its scope to cover the reconstruction of RMI remote objects >> from the `javaRemoteLocation` LDAP attribute. >> >> CSR for this change can be viewed >> [here](https://bugs.openjdk.org/browse/JDK-8290369). >> >> ### List of code changes >> - Switch the default value of the 'com.sun.jndi.ldap.object.trustSerialData' >> system property to "false". >> >> - Extend the scope of the property to also cover the reconstruction of RMI >> remote objects from the deprecated 'javaRemoteLocation' LDAP attribute. >> >> - Document the support for `javaRemoteLocation` and the >> `javaReferenceAddress` LDAP attributes in `java.naming`'s module-info. >> >> ### Test changes >> - New `test/jdk/com/sun/jndi/ldap/objects/RemoteLocationAttributeTest.java` >> test has been added to test that `com.sun.jndi.ldap.object.trustSerialData` >> system property can be used to control reconstruction of RMI objects from >> the `javaRemoteLocation` LDAP attribute. >> >> - `test/jdk/javax/naming/module/RunBasic.java` was modified to pass >> `com.sun.jndi.ldap.object.trustSerialData=true` to the sub-tests that rely >> on reconstruction/deserialization from LDAP attributes. >> >> - During the update for `test/jdk/javax/naming/module/RunBasic.java`, it was >> spotted that sub-tests apps launched in separate processes were returning >> the '0' exit value irrelevant to their execution status. All these sub-tests >> were modified to throw an exception when failure is observed. It helps to >> ensure that the exit value of launched process is not '0' for failed >> sub-tests. >> >> ### Testing >> >> `tier1`-`tier3` and JNDI regression/JCK tests not showing any failures >> related to this change. >> No failures observed for the modified regression tests. > > Aleksei Efimov has updated the pull request incrementally with one additional > commit since the last revision: > > Add run for the SP w/o value, formatting/wording updates src/java.naming/share/classes/com/sun/jndi/ldap/VersionHelper.java line 63: > 61: String trustSerialDataSp = getPrivilegedProperty( > 62: "com.sun.jndi.ldap.object.trustSerialData", "false"); > 63: trustSerialData = "true".equalsIgnoreCase(trustSerialDataSp); More of a question - is there a convention in our documentation where we state that the value is case insensitive? Right now, the `module-info.java` uses the literals `false` and `true` and doesn't talk about case insensitivity. Should it? - PR: https://git.openjdk.org/jdk/pull/10228
Re: RFR: 8290367: Update default value and extend the scope of com.sun.jndi.ldap.object.trustSerialData system property [v2]
On Fri, 9 Sep 2022 16:55:44 GMT, Aleksei Efimov wrote: >> ### Summary of the change >> >> The LDAP Naming Service Provider implementation's default settings are >> changed to disallow deserialization and reconstruction of Java objects from >> different LDAP attributes (RFC 2713). Currently, only the deserialization is >> controlled by the `com.sun.jndi.ldap.object.trustSerialData` system >> property, and it is allowed by default. >> The change proposed here switches the default value of the` >> com.sun.jndi.ldap.object.trustSerialData `system property to `"false"`, and >> also extends its scope to cover the reconstruction of RMI remote objects >> from the `javaRemoteLocation` LDAP attribute. >> >> CSR for this change can be viewed >> [here](https://bugs.openjdk.org/browse/JDK-8290369). >> >> ### List of code changes >> - Switch the default value of the 'com.sun.jndi.ldap.object.trustSerialData' >> system property to "false". >> >> - Extend the scope of the property to also cover the reconstruction of RMI >> remote objects from the deprecated 'javaRemoteLocation' LDAP attribute. >> >> - Document the support for `javaRemoteLocation` and the >> `javaReferenceAddress` LDAP attributes in `java.naming`'s module-info. >> >> ### Test changes >> - New `test/jdk/com/sun/jndi/ldap/objects/RemoteLocationAttributeTest.java` >> test has been added to test that `com.sun.jndi.ldap.object.trustSerialData` >> system property can be used to control reconstruction of RMI objects from >> the `javaRemoteLocation` LDAP attribute. >> >> - `test/jdk/javax/naming/module/RunBasic.java` was modified to pass >> `com.sun.jndi.ldap.object.trustSerialData=true` to the sub-tests that rely >> on reconstruction/deserialization from LDAP attributes. >> >> - During the update for `test/jdk/javax/naming/module/RunBasic.java`, it was >> spotted that sub-tests apps launched in separate processes were returning >> the '0' exit value irrelevant to their execution status. All these sub-tests >> were modified to throw an exception when failure is observed. It helps to >> ensure that the exit value of launched process is not '0' for failed >> sub-tests. >> >> ### Testing >> >> `tier1`-`tier3` and JNDI regression/JCK tests not showing any failures >> related to this change. >> No failures observed for the modified regression tests. > > Aleksei Efimov has updated the pull request incrementally with one additional > commit since the last revision: > > Add run for the SP w/o value, formatting/wording updates src/java.naming/share/classes/com/sun/jndi/ldap/VersionHelper.java line 60: > 58: > 59: // System property to control whether classes is allowed to be > loaded from > 60: // 'javaSerializedData' attribute Hello Aleksei, should we also update this code comment to make a mention of the additional attributes this system property now controls? - PR: https://git.openjdk.org/jdk/pull/10228
Re: RFR: 8290367: Update default value and extend the scope of com.sun.jndi.ldap.object.trustSerialData system property [v2]
On Fri, 9 Sep 2022 16:55:44 GMT, Aleksei Efimov wrote: >> ### Summary of the change >> >> The LDAP Naming Service Provider implementation's default settings are >> changed to disallow deserialization and reconstruction of Java objects from >> different LDAP attributes (RFC 2713). Currently, only the deserialization is >> controlled by the `com.sun.jndi.ldap.object.trustSerialData` system >> property, and it is allowed by default. >> The change proposed here switches the default value of the` >> com.sun.jndi.ldap.object.trustSerialData `system property to `"false"`, and >> also extends its scope to cover the reconstruction of RMI remote objects >> from the `javaRemoteLocation` LDAP attribute. >> >> CSR for this change can be viewed >> [here](https://bugs.openjdk.org/browse/JDK-8290369). >> >> ### List of code changes >> - Switch the default value of the 'com.sun.jndi.ldap.object.trustSerialData' >> system property to "false". >> >> - Extend the scope of the property to also cover the reconstruction of RMI >> remote objects from the deprecated 'javaRemoteLocation' LDAP attribute. >> >> - Document the support for `javaRemoteLocation` and the >> `javaReferenceAddress` LDAP attributes in `java.naming`'s module-info. >> >> ### Test changes >> - New `test/jdk/com/sun/jndi/ldap/objects/RemoteLocationAttributeTest.java` >> test has been added to test that `com.sun.jndi.ldap.object.trustSerialData` >> system property can be used to control reconstruction of RMI objects from >> the `javaRemoteLocation` LDAP attribute. >> >> - `test/jdk/javax/naming/module/RunBasic.java` was modified to pass >> `com.sun.jndi.ldap.object.trustSerialData=true` to the sub-tests that rely >> on reconstruction/deserialization from LDAP attributes. >> >> - During the update for `test/jdk/javax/naming/module/RunBasic.java`, it was >> spotted that sub-tests apps launched in separate processes were returning >> the '0' exit value irrelevant to their execution status. All these sub-tests >> were modified to throw an exception when failure is observed. It helps to >> ensure that the exit value of launched process is not '0' for failed >> sub-tests. >> >> ### Testing >> >> `tier1`-`tier3` and JNDI regression/JCK tests not showing any failures >> related to this change. >> No failures observed for the modified regression tests. > > Aleksei Efimov has updated the pull request incrementally with one additional > commit since the last revision: > > Add run for the SP w/o value, formatting/wording updates @AlekseiEfimov The CSR is well written, and the update to the module-info.java look good to me. The code changes and tests changes look good. I'm glad to see this change. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.org/jdk/pull/10228
Re: RFR: 8290367: Update default value and extend the scope of com.sun.jndi.ldap.object.trustSerialData system property [v2]
On Fri, 9 Sep 2022 15:29:22 GMT, Roger Riggs wrote:
> Is the case of -Dcom.sun.jndi.ldap.object.trustSerialData (w/o a value) well
> defined.
Deserialization and reconstruction of Java objects are only allowed when the
system property is set to "true". That's how its defined in `java.naming`
module info:
> "To allow the deserialization or reconstruction of java objects from {@code
> javaSerializedData},
> {@code javaRemoteLocation} or {@code javaReferenceAddress} attributes,
> the system property value can be set to {@code true}"
>
Other values and no value do not allow deserialization and reconstruction.
> Does it need a test.
Yes - I've updated the test with additional `run` tag with no value set for
`com.sun.jndi.ldap.object.trustSerialData`, also fixed the indentation in jtreg
header.
-
PR: https://git.openjdk.org/jdk/pull/10228
Re: RFR: 8290367: Update default value and extend the scope of com.sun.jndi.ldap.object.trustSerialData system property [v2]
> ### Summary of the change > > The LDAP Naming Service Provider implementation's default settings are > changed to disallow deserialization and reconstruction of Java objects from > different LDAP attributes (RFC 2713). Currently, only the deserialization is > controlled by the `com.sun.jndi.ldap.object.trustSerialData` system property, > and it is allowed by default. > The change proposed here switches the default value of the` > com.sun.jndi.ldap.object.trustSerialData `system property to `"false"`, and > also extends its scope to cover the reconstruction of RMI remote objects from > the `javaRemoteLocation` LDAP attribute. > > CSR for this change can be viewed > [here](https://bugs.openjdk.org/browse/JDK-8290369). > > ### List of code changes > - Switch the default value of the 'com.sun.jndi.ldap.object.trustSerialData' > system property to "false". > > - Extend the scope of the property to also cover the reconstruction of RMI > remote objects from the deprecated 'javaRemoteLocation' LDAP attribute. > > - Document the support for `javaRemoteLocation` and the > `javaReferenceAddress` LDAP attributes in `java.naming`'s module-info. > > ### Test changes > - New `test/jdk/com/sun/jndi/ldap/objects/RemoteLocationAttributeTest.java` > test has been added to test that `com.sun.jndi.ldap.object.trustSerialData` > system property can be used to control reconstruction of RMI objects from the > `javaRemoteLocation` LDAP attribute. > > - `test/jdk/javax/naming/module/RunBasic.java` was modified to pass > `com.sun.jndi.ldap.object.trustSerialData=true` to the sub-tests that rely on > reconstruction/deserialization from LDAP attributes. > > - During the update for `test/jdk/javax/naming/module/RunBasic.java`, it was > spotted that sub-tests apps launched in separate processes were returning the > '0' exit value irrelevant to their execution status. All these sub-tests were > modified to throw an exception when failure is observed. It helps to ensure > that the exit value of launched process is not '0' for failed sub-tests. > > ### Testing > > `tier1`-`tier3` and JNDI regression/JCK tests not showing any failures > related to this change. > No failures observed for the modified regression tests. Aleksei Efimov has updated the pull request incrementally with one additional commit since the last revision: Add run for the SP w/o value, formatting/wording updates - Changes: - all: https://git.openjdk.org/jdk/pull/10228/files - new: https://git.openjdk.org/jdk/pull/10228/files/40868838..faec04e6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10228=01 - incr: https://webrevs.openjdk.org/?repo=jdk=10228=00-01 Stats: 7 lines in 2 files changed: 2 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/10228.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10228/head:pull/10228 PR: https://git.openjdk.org/jdk/pull/10228
