[1004] Wrap service loader calls in privileged actions if the securit… #1005

jamezp · 2021-08-03T22:02:02Z

…y manager is enabled. Also ensure the LinkageError for the ClientBuilder is thrown if a security manager is enabled.

Resolves #1004

Signed-off-by: James R. Perkins jperkins@redhat.com

jamezp · 2021-08-03T22:35:56Z

I forgot to mention, if this seems acceptable I can port the changes over to whichever branches desired.

arjantijms · 2021-08-04T10:39:41Z

@jamezp do note that SecurityManager, AccessController etc have been deprecated for removal in JDK 17. So we may want to remove such privilege actions, not add them.

jansupol · 2021-08-04T12:33:48Z

There is quite a lot of code duplication in the world of suppliers.

JEP 411 deprecates the SecurityManager in JDK 17, but for JDK 17, it means a warning is issued. Jakarta REST 3.1 is supposed to be working with JDK 11-17 which still has the SecurityManager functional. But I'm surprised noone complained about this untill now (2.0 was released with EE 7)

@jamezp You seem to use the words may/might, is this a real issue or the could-potentially-happen one?

jamezp · 2021-08-04T14:36:02Z

@jamezp do note that SecurityManager, AccessController etc have been deprecated for removal in JDK 17. So we may want to remove such privilege actions, not add them.

Yes, I thought about that before sending this. However, there still is no clear path for a replacement that I'm aware of. I know that some installations require the security manager to be enabled.

The real purpose of this is so RESTEasy no longer has to use a fork of the spec :)

jamezp · 2021-08-04T14:39:23Z

There is quite a lot of code duplication in the world of suppliers.

JEP 411 deprecates the SecurityManager in JDK 17, but for JDK 17, it means a warning is issued. Jakarta REST 3.1 is supposed to be working with JDK 11-17 which still has the SecurityManager functional. But I'm surprised noone complained about this untill now (2.0 was released with EE 7)

Yes. AFAIK there is no replacement for the security manager at this point. I have a feeling it won't be removed anytime soon :)

@jamezp You seem to use the words may/might, is this a real issue or the could-potentially-happen one?

Sorry, yes this is a real issue. I've not tried in other application servers, but I can say in WildFly if you run with a security manager then there are failures. There is a fix in a fork which WildFly and JBoss EAP use. However it would be nice to let the fork die and use the spec itself :)

mkarg · 2021-08-04T17:16:18Z

I'd rather vote for removal of security manager instead of correcting it.

spericas · 2021-08-04T19:17:51Z

@jamezp What is the use case for running WildFly under a security manager? The JDK team seems to simply by JEP 411 that this is no longer needed/used in server-side Java.

andymc12 · 2021-08-04T19:27:05Z

I know that we (IBM) have customers who require Java 2 security for their app servers - even in Java/Jakarta EE. Personally, I've never been a fan of Java 2 security, but we do have some users who need it. I believe most of our Java 2 security customers are governmental users or are contractors for governments that require it. Those requirements might start to be lifted now that Java 17 is deprecating the security manager, but in general (and until we get a little closer to prereq'ing Java 17), I'm in favor of adding doPrivs where necessary.

jamezp · 2021-08-04T19:31:39Z

@jamezp What is the use case for running WildFly under a security manager? The JDK team seems to simply by JEP 411 that this is no longer needed/used in server-side Java.

@spericas I can't say I know the requirements specifically for WildFly, but I know we (Red Hat) have JBoss EAP customers that are required to run the container with the security manager enabled.

I agree with @andymc12 that I don't like it, but I don't really have a choice but to support it :)

mkarg · 2021-08-05T21:29:15Z

jaxrs-api/src/main/java/jakarta/ws/rs/client/ClientBuilder.java

+                            + " to " + targetTypeURL;
+                } else {
+                    errorMessage = AccessController.doPrivileged((PrivilegedAction<String>) () -> {
+                        ClassLoader loader = pClass.getClassLoader();


Maybe we should refactor into private method instead of duplicating code.

Should be done if I've understood correctly :)

spericas · 2021-08-06T15:51:29Z

@jamezp @andymc12 Just reviewed JEP 411. Looks to me that it would be OK to fix this for 3.1. We should probably review this for 4.0 as clearly a lot of this functionality is going away. I still think that the "security" provided by the Java Security Manager is a lot less useful in practice than on paper.

spericas

LGTM. Only comment is that findFirstService's could create an Action to avoid duplicating code blocks.

jamezp · 2021-08-12T14:42:26Z

LGTM. Only comment is that findFirstService's could create an Action to avoid duplicating code blocks.

Done.

andymc12

Overall, I like this changes - the performance changes and adding getClassLoader calls into doPriv blocks. I only have a question about whether ServiceLoader.load(...) and subsequent iterating requires a doPriv block. Thanks!

andymc12 · 2021-08-12T16:08:02Z

jaxrs-api/src/main/java/jakarta/ws/rs/client/FactoryFinder.java

+        if (System.getSecurityManager() == null) {
+            return action.run();
+        }
+        return AccessController.doPrivileged(action);


I understand putting the getClassLoader calls into a doPriv block, but I don't understand the ServiceLoader calls. Does that need to be in a doPriv? The javadoc doesn't mention that it throws a SecurityException.

The ServiceLoader.load() is actually safe, but it captures the AccessContrelContext is captured and during iteration a security exception could be thrown on Constructor.newInstance().

From the JavaDocs:

Service loaders always execute in the security context of the caller. Trusted system code should typically invoke the methods in this class, and the methods of the iterators which they return, from within a privileged security context.

andymc12 · 2021-08-12T16:08:59Z

jaxrs-api/src/main/java/jakarta/ws/rs/ext/FactoryFinder.java

+        if (System.getSecurityManager() == null) {
+            return action.run();
+        }
+        return AccessController.doPrivileged(action);


Same comment as above. Can we avoid the doPriv on the ServiceLoader.load(...) calls?

spericas · 2021-08-12T18:42:00Z

LGTM. Only comment is that findFirstService's could create an Action to avoid duplicating code blocks.

Done.

Note that PrivilegedAction is a functional interface, so I think you could simplify this using capturing lambdas for even better readability without the need inner classes.

…y manager is enabled. Also ensure the LinkageError for the ClientBuilder is thrown if a security manager is enabled. Resolves jakartaee#1004 Signed-off-by: James R. Perkins <jperkins@redhat.com>

jamezp · 2021-08-12T18:47:48Z

Note that PrivilegedAction is a functional interface, so I think you could simplify this using capturing lambdas for even better readability without the need inner classes.

Good point. Updated!

andymc12

LGTM - thanks!

spericas · 2021-09-07T18:21:07Z

Merging as it has been approved 25 days ago.

…y manager is enabled. Also ensure the LinkageError for the ClientBuilder is thrown if a security manager is enabled. (jakartaee#1005) Resolves jakartaee#1004 Signed-off-by: James R. Perkins <jperkins@redhat.com>

jansupol added the security label Aug 4, 2021

mkarg reviewed Aug 5, 2021

View reviewed changes

jamezp force-pushed the sm-fixes branch from 23de8ef to 58eea4c Compare August 6, 2021 18:45

spericas previously approved these changes Aug 12, 2021

View reviewed changes

jamezp dismissed spericas’s stale review via 0c59abc August 12, 2021 14:42

jamezp force-pushed the sm-fixes branch from 58eea4c to 0c59abc Compare August 12, 2021 14:42

andymc12 reviewed Aug 12, 2021

View reviewed changes

[1004] Wrap service loader calls in privileged actions if the securit…

35348b3

…y manager is enabled. Also ensure the LinkageError for the ClientBuilder is thrown if a security manager is enabled. Resolves jakartaee#1004 Signed-off-by: James R. Perkins <jperkins@redhat.com>

jamezp force-pushed the sm-fixes branch from 0c59abc to 35348b3 Compare August 12, 2021 18:46

andymc12 approved these changes Aug 12, 2021

View reviewed changes

spericas approved these changes Aug 13, 2021

View reviewed changes

spericas added this to the 3.1 milestone Sep 7, 2021

spericas added this to In progress in Jakarta REST 3.1 Sep 7, 2021

spericas merged commit 08e4014 into jakartaee:master Sep 7, 2021

spericas moved this from In progress to Done in Jakarta REST 3.1 Apr 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1004] Wrap service loader calls in privileged actions if the securit… #1005

[1004] Wrap service loader calls in privileged actions if the securit… #1005

jamezp commented Aug 3, 2021

jamezp commented Aug 3, 2021

arjantijms commented Aug 4, 2021

jansupol commented Aug 4, 2021

jamezp commented Aug 4, 2021

jamezp commented Aug 4, 2021

mkarg commented Aug 4, 2021

spericas commented Aug 4, 2021

andymc12 commented Aug 4, 2021

jamezp commented Aug 4, 2021

mkarg Aug 5, 2021

jamezp Aug 6, 2021

spericas commented Aug 6, 2021

spericas left a comment

jamezp commented Aug 12, 2021

andymc12 left a comment

andymc12 Aug 12, 2021

jamezp Aug 12, 2021 •

edited

andymc12 Aug 12, 2021

spericas commented Aug 12, 2021

jamezp commented Aug 12, 2021

andymc12 left a comment

spericas commented Sep 7, 2021

[1004] Wrap service loader calls in privileged actions if the securit… #1005

[1004] Wrap service loader calls in privileged actions if the securit… #1005

Conversation

jamezp commented Aug 3, 2021

jamezp commented Aug 3, 2021

arjantijms commented Aug 4, 2021

jansupol commented Aug 4, 2021

jamezp commented Aug 4, 2021

jamezp commented Aug 4, 2021

mkarg commented Aug 4, 2021

spericas commented Aug 4, 2021

andymc12 commented Aug 4, 2021

jamezp commented Aug 4, 2021

mkarg Aug 5, 2021

Choose a reason for hiding this comment

jamezp Aug 6, 2021

Choose a reason for hiding this comment

spericas commented Aug 6, 2021

spericas left a comment

Choose a reason for hiding this comment

jamezp commented Aug 12, 2021

andymc12 left a comment

Choose a reason for hiding this comment

andymc12 Aug 12, 2021

Choose a reason for hiding this comment

jamezp Aug 12, 2021 • edited

Choose a reason for hiding this comment

andymc12 Aug 12, 2021

Choose a reason for hiding this comment

spericas commented Aug 12, 2021

jamezp commented Aug 12, 2021

andymc12 left a comment

Choose a reason for hiding this comment

spericas commented Sep 7, 2021

jamezp Aug 12, 2021 •

edited