8290368: Introduce LDAP and RMI protocol-specific object factory filters to JNDI implementation by AlekseiEfimov · Pull Request #10578 · openjdk/jdk

AlekseiEfimov · 2022-10-05T15:23:43Z

Summary of the change

This change introduces new system and security properties for specifying factory filters for the JNDI/LDAP and the JNDI/RMI JDK provider implementations.

These new properties allow more granular control over the set of object factories allowed to reconstruct Java objects from LDAP and RMI contexts.

The new factory filters are supplementing the existing jdk.jndi.object.factoriesFilter global factories filter to determine if a specific object factory is permitted to instantiate objects for the given protocol.

Links:

List of code changes

Implementation for two new system and security properties have been added to the com.sun.naming.internal.ObjectFactoriesFilter class
java.security and module-info.java files have been updated with a documentation for the new properties
To keep API of javax.naming.spi.NamingManager and javax.naming.spi.DirectoryManager classes unmodified a new internal com.sun.naming.internal.NamingManagerHelper class has been introduced. All getObjectInstance calls have been updated to use the new helper class.

NamingManagerHelper changes

Changes performed to construct the NamingManagerHelper class:

DirectoryManager.getObjectInstance -> NamingManagerHelper.getDirObjectInstance. Dependant methods were also moved to the NamingManagerHelper class
NamingManager.getObjectInstance -> NamingManagerHelper.getObjectInstance. Methods responsible for setting/getting object factory builder were moved to the NamingManagerHelper class too.

Test changes

New tests have been added for checking that new factory filters can be used to restrict reconstruction of Java objects from LDAP and RMI contexts:

LDAP protocol specific test: test/jdk/com/sun/jndi/ldap/objects/factory/LdapFactoriesFilterTest.java
RMI protocol specific test: test/jdk/com/sun/jndi/rmi/registry/objects/RmiFactoriesFilterTest.java
Existing test/jdk/javax/naming/module/RunBasic.java test has been updated to allow test-specific factories filter used to reconstruct objects from the test LDAP server.

Testing

tier1-tier3 and JNDI regression/JCK tests not showing any failures related to this change.
No failures observed for the modified regression tests.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires a CSR request to be approved

Issues

JDK-8290368: Introduce LDAP and RMI protocol-specific object factory filters to JNDI implementation
JDK-8291556: Introduce LDAP and RMI protocol-specific object factory filters to JNDI implementation (CSR)

Reviewers

Daniel Fuchs (@dfuch - Reviewer) ⚠️ Review applies to 4449dda1
Roger Riggs (@RogerRiggs - Reviewer) ⚠️ Review applies to 528489b0
Jaikiran Pai (@jaikiran - Reviewer) ⚠️ Review applies to 4449dda1

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10578/head:pull/10578
$ git checkout pull/10578

Update a local copy of the PR:
$ git checkout pull/10578
$ git pull https://git.openjdk.org/jdk pull/10578/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10578

View PR using the GUI difftool:
$ git pr show -t 10578

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10578.diff

…ers to JNDI implementation

…t intersect.

bridgekeeper · 2022-10-05T15:25:38Z

👋 Welcome back aefimov! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-10-05T15:28:55Z

@AlekseiEfimov The following labels will be automatically applied to this pull request:

core-libs
security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-10-05T15:32:44Z

Webrevs

dfuch · 2022-10-06T11:12:31Z

I am glad to see this RFE. It looks like a big change but most of it is actually reorganisation of internal code, so thanks for explaining its making off :-) It helps a lot for the review. I have looked at the code and I believe it looks good. I don't see any alternative to the reorganisation that could make the changes smaller - so I'm OK with the proposed solution. Thanks for documenting the new properties in their respective module info, as well as in the security properties file.

I had only a cursory look at the tests but they seem comprehensive. Good work.

dfuch · 2022-10-06T11:14:16Z

I am approving on the condition to make sure that all JNDI tests (as well as tier1, tier2, tier3) are run before integrating.

dfuch · 2022-10-06T11:15:58Z

Also please get an approval from a security-libs maintainer for the changes to the security properties file before integrating.

RogerRiggs

Nice work, but a few comments.

RogerRiggs · 2022-10-06T16:10:37Z

src/java.naming/share/classes/com/sun/naming/internal/ObjectFactoriesFilter.java

+        return checkInput("rmi", () -> clz);
+    }
+
+    private static boolean checkInput(String scheme, FactoryInfo factoryInfo) {


This construct in which the supplied lambda fills in the serialClass is pretty obscure.
Perhaps the variable name can be "serialClass" to match the only non-default method in ObjectInputFilter would give a better hint.

Thanks - serialClass name reads better.

src/java.naming/share/classes/com/sun/naming/internal/ObjectFactoriesFilter.java

test/jdk/com/sun/jndi/ldap/objects/factory/LdapFactoriesFilterTest.java

src/java.naming/share/classes/com/sun/naming/internal/ObjectFactoriesFilter.java

RogerRiggs

LGTM

jaikiran · 2022-10-13T12:29:54Z

src/java.naming/share/classes/com/sun/naming/internal/ObjectFactoriesFilter.java

     */
-    public static boolean canInstantiateObjectsFactory(Class<?> factoryClass) {
-        return checkInput(() -> factoryClass);
+    public static boolean checkGlobalFilter(Class<?> serialClass) {


I think the serialClass param should be renamed to factoryClass or something like that, since I think the serialClass reference comes from serialization/deserialization usage.

The name comes from ObjectInputFilter.FilterInfo - it's been renamed from factoryClassto make it clear that the supplied lambda fills-in the non-default ObjectInputFilter.FilterInfo.serialClass() method.

Thank you for that clarification, Aleksei. This sounds fine to me then.

jaikiran · 2022-10-13T12:34:47Z

src/java.base/share/conf/security/java.security

+# set of object factory classes which will be allowed to instantiate objects from object
+# references bound to LDAP contexts. The factory class named by the reference instance will
+# be matched against this filter. The filter property supports pattern-based filter syntax
+# with the same format as jdk.serialFilter.


Hello Aleksei, as far as I can see the jdk.serialFilter allows for additional details like limits (example: maxdepth=value). Should we note here that this JNDI specific property will ignore those configurations?

Yes, worth noting, but I would say they are "unused", not ignored.

jaikiran · 2022-10-13T12:37:36Z

src/java.base/share/conf/security/java.security

+# "java.naming/com.sun.jndi.ldap.**;!*".
+#
+# The default pattern value allows any object factory class defined in the java.naming module
+# to be specified by the reference instance, but rejects any other.


Should this instead say:

The default pattern value allows any object factory class belonging to the com.sun.jndi.ldap and its sub-packages of the java.naming module to be specified by the reference instance...?

I would prefer to keep a filter default value explanation sentences simpler since exact default values are listed just one line after.

What you have here is OK with me then.

jaikiran · 2022-10-13T13:18:58Z

src/java.naming/share/classes/javax/naming/spi/NamingManager.java

@@ -113,108 +108,14 @@ public class NamingManager {
     */
    public static synchronized void setObjectFactoryBuilder(


Perhaps remove the synchronized from this method and the getter, now that the synchronization has moved to the NamingManagerHelper class?

Agreed - I think it is safe to remove synchronized. I doubt that there is a code that relies on synchronized (javax.naming.spi.NamingManager.class) to prevent other code setting the factory builder (also it can be set only once), therefore it should be ok to remove synchronized.

src/java.naming/share/classes/com/sun/naming/internal/NamingManagerHelper.java

src/java.base/share/conf/security/java.security

src/java.naming/share/classes/com/sun/naming/internal/NamingManagerHelper.java

…ments/docs.

dfuch · 2022-10-14T16:35:28Z

src/java.base/share/conf/security/java.security

+# are unused.
 #
-# Each pattern is matched against the factory class name to allow or disallow it's
+# Each class name pattern is matched against the factory class name to allow or disallow its


It appears that for those protocols for which there is no specific filter, a factory class will be accepted only if the global filter returns ALLOWED - which contradicts what is written here (where it says that the class is allowed if it's not REJECTED). Is this something that has changed with this fix - or was the documentation wrong before?

Very good catch Daniel! It is with this fix and I believe the code needs to be change to match what is written for the global filter here:
What we've had before in checkInput:

private static boolean checkInput(FactoryInfo factoryInfo) { Status result = GLOBAL.checkInput(factoryInfo); return result != Status.REJECTED;

What we have now:

if (filter == GLOBAL_FILTER) { return globalResult == Status.ALLOWED; }

I think it needs to be changed to (to match the description for global filter):

if (filter == GLOBAL_FILTER) { return globalResult != Status.REJECTED; }

In the general composition of filters, it is preferable that UNDECIDED is treated as REJECTED.
That keeps unintentional holes in a filter from being permissive.

In the general composition of filters, it is preferable that UNDECIDED is treated as REJECTED.
That keeps unintentional holes in a filter from being permissive.

That is a good point Roger. The "java.security" file was updated (4449dda) to match the ObjectFactoriesFilter implementation, ie the global filter treats UNDECIDED as REJECTED. Also, the CSR has been updated to highlight that.

jaikiran

The latest state of this PR in 4449dda looks good to me. Thank you Aleksei.

openjdk · 2022-10-18T21:46:50Z

@AlekseiEfimov This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8290368: Introduce LDAP and RMI protocol-specific object factory filters to JNDI implementation

Reviewed-by: dfuchs, rriggs, jpai

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

AlekseiEfimov · 2022-10-19T14:40:25Z

/integrate

openjdk · 2022-10-19T14:41:42Z

Going to push as commit d37ce4c.
Since your change was applied there have been 2 commits pushed to the master branch:

21aeb9e: 8295429: Update harfbuzz md file
1d883c5: 8295417: Pass $AR to binutils cross-build

Your commit was automatically rebased without conflicts.

openjdk · 2022-10-19T14:42:10Z

@AlekseiEfimov Pushed as commit d37ce4c.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

AlekseiEfimov added 4 commits September 26, 2022 18:45

8290368: Introduce LDAP and RMI protocol-specific object factory filt…

c0c4320

…ers to JNDI implementation

Cleanup test comments. Add tests to check that LDAP/RMI filters do no…

dc17ccf

…t intersect.

More tests clean-up. Code/doc comments cleanup.

fd7c551

Additional comments/formatting cleanup.

1a5e83e

openjdk bot added csr Pull request needs approved CSR before integration rfr Pull request is ready for review labels Oct 5, 2022

openjdk bot added security security-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Oct 5, 2022

dfuch approved these changes Oct 6, 2022

View reviewed changes

Merge branch 'master' into JDK-8290368_protocol_specific_factory_filters

9eee3e7

RogerRiggs reviewed Oct 6, 2022

View reviewed changes

Refactor checkInput, better reporting for invalid filter patterns

091677e

dfuch suggested changes Oct 10, 2022

View reviewed changes

src/java.naming/share/classes/com/sun/naming/internal/ObjectFactoriesFilter.java Outdated Show resolved Hide resolved

Change checkInput to be the global filter centric

528489b

dfuch approved these changes Oct 10, 2022

View reviewed changes

RogerRiggs approved these changes Oct 11, 2022

View reviewed changes

jaikiran reviewed Oct 13, 2022

View reviewed changes

src/java.naming/share/classes/com/sun/naming/internal/NamingManagerHelper.java Outdated Show resolved Hide resolved

jaikiran reviewed Oct 13, 2022

View reviewed changes

src/java.base/share/conf/security/java.security Outdated Show resolved Hide resolved

jaikiran reviewed Oct 13, 2022

View reviewed changes

src/java.base/share/conf/security/java.security Outdated Show resolved Hide resolved

jaikiran reviewed Oct 13, 2022

View reviewed changes

src/java.naming/share/classes/com/sun/naming/internal/NamingManagerHelper.java Outdated Show resolved Hide resolved

AlekseiEfimov added 2 commits October 13, 2022 20:24

Remove factory builder synchronization from NamingManager. Update com…

5380604

…ments/docs.

Merge branch 'master' into JDK-8290368_protocol_specific_factory_filters

b384916

dfuch reviewed Oct 14, 2022

View reviewed changes

Update filter docs to match implementation.

4449dda

jaikiran approved these changes Oct 18, 2022

View reviewed changes

dfuch approved these changes Oct 18, 2022

View reviewed changes

openjdk bot removed the csr Pull request needs approved CSR before integration label Oct 18, 2022

openjdk bot added the ready Pull request is ready to be integrated label Oct 18, 2022

Merge branch 'master' into JDK-8290368_protocol_specific_factory_filters

fbc4d57

openjdk bot added the integrated Pull request has been integrated label Oct 19, 2022

openjdk bot closed this Oct 19, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 19, 2022

AlekseiEfimov deleted the JDK-8290368_protocol_specific_factory_filters branch March 21, 2023 15:42

		@@ -113,108 +108,14 @@ public class NamingManager {
		*/
		public static synchronized void setObjectFactoryBuilder(

Conversation

AlekseiEfimov commented Oct 5, 2022 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the change

List of code changes

NamingManagerHelper changes

Test changes

Testing

Progress

Issues

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Oct 5, 2022

Uh oh!

openjdk bot commented Oct 5, 2022

Uh oh!

mlbridge bot commented Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dfuch commented Oct 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dfuch commented Oct 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dfuch commented Oct 6, 2022

Uh oh!

RogerRiggs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RogerRiggs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaikiran Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dfuch Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaikiran left a comment

AlekseiEfimov commented Oct 5, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 5, 2022 •

edited

Loading

dfuch commented Oct 6, 2022 •

edited

Loading

dfuch commented Oct 6, 2022 •

edited

Loading

jaikiran Oct 13, 2022 •

edited

Loading

dfuch Oct 14, 2022 •

edited

Loading

openjdk bot commented Oct 18, 2022 •

edited

Loading