Check services in legacy security providers by KostasTsiounis · Pull Request #1197 · ibmruntimes/openj9-openjdk-jdk

KostasTsiounis · 2026-03-30T15:45:08Z

Legacy security providers do not use the more modern getService() and putService() methods, but rather other more map based methods.

Further checks are added to restrict through RestrictedSecurity the registration and retrieval of services through legacy providers.

Tests are, also, added to verify these checks.

Signed-off-by: Kostas Tsiounis kostas.tsiounis@ibm.com

KostasTsiounis · 2026-04-08T20:41:32Z

I moved the mock provider in the test/lib directory to allow putting it under a package. This is needed because only fully-qualified names are allowed when declaring providers in RestrictedSecurity mode profiles.

Legacy security providers do not use the more modern getService() and putService() methods, but rather other more map based methods. Further checks are added to restrict through RestrictedSecurity the registration and retrieval of services through legacy providers. Tests are, also, added to verify these checks. Signed-off-by: Kostas Tsiounis <kostas.tsiounis@ibm.com>

keithc-ca · 2026-04-13T20:22:36Z

+            Set<Map.Entry<Object, Object>> entrySet = entrySet();
+            Collection<Object> valuesToReturn = new ArrayList<>(entrySet.size());
+            for (Map.Entry<Object, Object> entry : entrySet) {
+                if (checkRestrictedSecurityKey(entry.getKey())) {
+                    // We're in restricted security mode which allows this service
+                    // or provider info, so add it to list of values to be returned.
+                    valuesToReturn.add(entry.getValue());
+                }
+            }
+            return Collections.unmodifiableCollection(valuesToReturn);


The set should have already been filtered by entrySet(); this only needs to collect the values:

return entrySet().stream().map(entry -> entry.getValue()).toList();

Changed to suggestion mostly. I changed the last part to be collect(Collectors.toUnmodifiableList()) instead of toList() since that's what the method is supposed to return.

keithc-ca · 2026-04-13T20:51:14Z

+
+import java.security.Provider;
+
+public class LegacyProvider extends Provider {


test/lib/jdk/test/lib/LegacyProvider.java:33: warning: [serial] serializable class LegacyProvider has no definition of serialVersionUID public class LegacyProvider extends Provider { ^

I didn't see any of the other helper classes have anything to imitate. How do you think I should handle this? Just assign 1L since it won't be actually serialized?

You can pick a UID at random; eclipse suggested this:

private static final long serialVersionUID = 5997431841873045817L;

keithc-ca · 2026-04-14T18:38:27Z

 import java.util.function.BiConsumer;
 import java.util.function.BiFunction;
 import java.util.function.Function;
+import java.util.stream.Collectors;


Please move this after line 48 (which is out of place, but from upstream).

keithc-ca · 2026-04-14T18:40:26Z

        checkInitialized();
+
+        if (RestrictedSecurity.isEnabled()) {
+            return entrySet().stream().map(entry -> entry.getValue()).collect(Collectors.toUnmodifiableList());


Please use the toList() convenience method (which promises an unmodifiable list):

return entrySet().stream().map(entry -> entry.getValue()).toList();

keithc-ca · 2026-04-14T18:41:35Z

+        }
        return super.get(key);


Please add a blank line between lines 671 and 672, like other updated methods.

keithc-ca · 2026-04-14T19:20:06Z

    private Object implPut(Object key, Object value) {
        if (!checkLegacy(key)) return null;

+        if (!canRestrictedSecurityServiceBeRegistered(key)) {


Please explain the rules regarding which of isRestrictedSecurityServiceAllowed() and canRestrictedSecurityServiceBeRegistered() should be used. implCompute() uses the former, while implPut(), implReplace() and implReplaceAll() use the latter. This doesn't appear to be consistent.

The idea is that when doing put() or replace() you already know the value to be inserted, so you are not actually retrieving any information and should thus be allowed to be used from any place of your code (i.e., canRestrictedSecurityServiceBeRegistered()). But with compute() and all of its variants, one can use it to just retrieve an existing value, which should only be allowed from the specific class defined in the constraint (i.e., isRestrictedSecurityServiceAllowed()).

What I just noticed though is that merge() does a thing very similar to compute(), so I might have to switch that to behave like it.

I don't think the result is consistent yet; examples include:

keys() and keySet() may yield a different elements than the keys in entrySet()

forEach() exposes entries that would not be included in entrySet()

merge() and put() allow storing entries that would not be permitted via compute*()

I don't think the result is consistent yet; examples include:

keys() and keySet() may yield a different elements than the keys in entrySet()

forEach() exposes entries that would not be included in entrySet()

merge() and put() allow storing entries that would not be permitted via compute*()

Regarding each of the points:

The whole idea is to restrict getting services that potentially should not be allowed in a specific setting. Getting the keys doesn't give you access to any such services, thus the decision to not restrict keys() and keySet(). I could restrict that too, I just think it might be defeating the purpose. I would also need to add a debug path for cases where we want to see what is available through the provider.

I actually missed that. I will update forEach().

Like I mentioned before, I will update merge() to match compute(), but I think there should be a difference between them and put(), because we need to allow users to register from wherever but only retrieve from explicitly specified classes. I know that I'm hindering the functionality of compute() and merge() that could be used to just update/register new values, unless I manually check whether the supplied BiFunction is a tautology (i.e., just returns the same value) and it's just used to get a value. Do you think that would make sense to do?

The whole idea is to restrict getting services that potentially should not be allowed in a specific setting

Perhaps I'm missing something. How does restricting access to keys or values affect access to services offered by a given provider?

Perhaps I'm missing something. How does restricting access to keys or values affect access to services offered by a given provider?

This whole change was inspired by a use case of someone using a legacy Kafka provider, where services were inserted using put() and then retrieved using get() and subsequently being initialized through reflection. In these cases RestrictedSecurity wasn't able to enforce constraints from the loaded profile. Of course, we couldn't just check these two methods.

As far as keys() goes, my initial thought process was that it could not possibly affect the aforementioned scenario. I will, however, restrict it too for consistency reasons.

If one has access to a service to be able to insert it via put() it's too late to prevent distribution of that service to others.

It's not clear that it's possible to completely restrict access to services by participants that employ reflection. The most direct route might be to reflect Properties.map and operate directly on that object, bypassing anything we might do the implementation of Provider.

How much effort should we expend on this when it can always be circumvented?

keithc-ca · 2026-04-14T19:21:11Z

+
+    private boolean canRestrictedSecurityServiceBeRegistered(Object key) {
+        Service service = createServiceFromKey(key);
+        return ((service == null) || RestrictedSecurity.canServiceBeRegistered(service));


Please remove the outermost parentheses, here and on line 1078.

keithc-ca · 2026-04-14T19:22:04Z

+ *
+ * ===========================================================================
+ */
+


Please omit this blank line.

keithc-ca · 2026-04-14T19:23:08Z

+
+import java.security.Provider;
+
+public class LegacyProvider extends Provider {


You can pick a UID at random; eclipse suggested this:

private static final long serialVersionUID = 5997431841873045817L;

keithc-ca · 2026-04-14T19:23:38Z

+ * the RestrictedSecurity mode.
+ */
+public class LegacyProvider extends Provider {
+    public LegacyProvider() {


This triggers "this-escape" warnings; making the class final addresses that.

keithc-ca · 2026-07-15T14:06:09Z

How does this, and RestrictedSecurity in general, interact with eec1769?

KostasTsiounis force-pushed the register_legacy branch 2 times, most recently from 376f20b to ca852dd Compare March 30, 2026 15:53

keithc-ca self-requested a review March 30, 2026 20:31

keithc-ca requested changes Mar 30, 2026

View reviewed changes

KostasTsiounis force-pushed the register_legacy branch from 540ae55 to d9b23db Compare April 1, 2026 17:17

KostasTsiounis force-pushed the register_legacy branch from 9280b3d to b08544a Compare April 8, 2026 20:32

KostasTsiounis force-pushed the register_legacy branch from b08544a to 76ec020 Compare April 8, 2026 20:43

KostasTsiounis requested a review from keithc-ca April 8, 2026 20:44

keithc-ca requested changes Apr 8, 2026

View reviewed changes

Address review comments

d66c1ce

KostasTsiounis force-pushed the register_legacy branch from 08b7050 to d66c1ce Compare April 9, 2026 19:25

Fix formatting

1d28c48

keithc-ca requested changes Apr 9, 2026

View reviewed changes

Comment thread closed/test/jdk/openj9/internal/security/TestLegacyProviders.java Outdated

Comment thread closed/test/jdk/openj9/internal/security/TestLegacyProviders.java

Address review comments 2

6f0cce5

keithc-ca requested changes Apr 10, 2026

View reviewed changes

KostasTsiounis force-pushed the register_legacy branch from 9ecf603 to 83447a7 Compare April 13, 2026 18:24

KostasTsiounis requested a review from keithc-ca April 13, 2026 18:31

keithc-ca requested changes Apr 13, 2026

View reviewed changes

keithc-ca reviewed Apr 13, 2026

View reviewed changes

Comment thread test/lib/jdk/test/lib/LegacyProvider.java Outdated

keithc-ca reviewed Apr 13, 2026

View reviewed changes

KostasTsiounis added 3 commits April 14, 2026 14:01

Address review comments 3

ee18afb

Create and return new set every time

32c8c07

Address review comments 4

95bc3c1

KostasTsiounis force-pushed the register_legacy branch from f9860c3 to 95bc3c1 Compare April 14, 2026 18:01

KostasTsiounis requested a review from keithc-ca April 14, 2026 18:05

keithc-ca requested changes Apr 14, 2026

View reviewed changes


		import java.security.Provider;

		public class LegacyProvider extends Provider {

Uh oh!

Conversation

KostasTsiounis commented Mar 30, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KostasTsiounis commented Apr 8, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

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!

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!

keithc-ca commented Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development