[GitHub] [nifi] turcsanyip commented on a diff in pull request #6317: NIFI-10375: If a class is not allowed in the AllowListClassLoader by …

[GitBox <gi...@apache.org>]

turcsanyip commented on code in PR #6317:
URL: https://github.com/apache/nifi/pull/6317#discussion_r952364712


##########
nifi-stateless/nifi-stateless-bootstrap/src/main/java/org/apache/nifi/stateless/bootstrap/AllowListClassLoader.java:
##########
@@ -37,36 +43,92 @@
  * </p>
  */
 public class AllowListClassLoader extends ClassLoader {
-    private final Set<String> allowed;
+    private static final Logger logger = LoggerFactory.getLogger(AllowListClassLoader.class);
+
+    private final Set<String> allowedClassNames;
+    private final List<String> allowedModulePrefixes = Arrays.asList("java.", "jdk.");

Review Comment:
   I think the same module check would work on Java 9+ JDK too so it may make sense to use it instead of loading the classes from `jmod` files and to have a unified approach for Java 9+ JDK/JRE.



##########
nifi-stateless/nifi-stateless-bootstrap/src/main/java/org/apache/nifi/stateless/bootstrap/AllowListClassLoader.java:
##########
@@ -37,36 +43,92 @@
  * </p>
  */
 public class AllowListClassLoader extends ClassLoader {
-    private final Set<String> allowed;
+    private static final Logger logger = LoggerFactory.getLogger(AllowListClassLoader.class);
+
+    private final Set<String> allowedClassNames;
+    private final List<String> allowedModulePrefixes = Arrays.asList("java.", "jdk.");
 
     public AllowListClassLoader(final ClassLoader parent, final Set<String> allowed) {
         super(parent);
-        this.allowed = allowed;
+        this.allowedClassNames = allowed;
     }
 
     /**
      * @return the set of all Class names that will not be blocked from loading by the parent
      */
     public Set<String> getClassesAllowed() {
-        return Collections.unmodifiableSet(allowed);
+        return Collections.unmodifiableSet(allowedClassNames);
     }
 
     @Override
     protected Class<?> loadClass(final String name, final boolean resolve) throws ClassNotFoundException {
-        if (allowed.contains(name)) {
+        if (allowedClassNames.contains(name)) {
             return super.loadClass(name, resolve);
         }
 
+        final Class<?> found = super.loadClass(name, false);
+        if (found != null) {
+            final boolean allowed = isClassAllowed(name, found);
+            if (allowed) {
+                if (resolve) {
+                    super.resolveClass(found);
+                }
+
+                return found;
+            }
+        }

Review Comment:
   `super.loadClass()` throws `NoClassDefFoundError` when it cannot load the class (tested on Java 11 JDK/JRE).
   So we would need something like this:
   ```
           try {
               final Class<?> found = super.loadClass(name, false);
               if (found != null) {
                   final boolean allowed = isClassAllowed(name, found);
                   if (allowed) {
                       if (resolve) {
                           super.resolveClass(found);
                       }
   
                       return found;
                   }
               }
           } catch (NoClassDefFoundError e) {
               // throw ClassNotFoundException at the end in this case too
           }
   ```
   `found != null` check may be not needed because `super.loadClass()` either returns the class or throws an error/exception, I believe.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org