[GitHub] [ignite] alex-plekhanov commented on a change in pull request #7159: IGNITE-12283 Access restriction to IgniteKernal

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

alex-plekhanov commented on a change in pull request #7159:
URL: https://github.com/apache/ignite/pull/7159#discussion_r415639441



##########
File path: modules/core/src/main/java/org/apache/ignite/Ignition.java
##########
@@ -538,7 +556,43 @@ public static Ignite ignite(@Nullable String name) throws IgniteIllegalStateExce
      * @throws IllegalArgumentException Thrown if current thread is not an {@link IgniteThread}.
      */
     public static Ignite localIgnite() throws IgniteIllegalStateException, IllegalArgumentException {
-        return IgnitionEx.localIgnite();
+        return sandboxIgniteProxy(IgnitionEx.localIgnite());
+    }
+
+    /**
+     * @param ignite Ignite.
+     * @return Ignite component proxy if the Ignite Sandbox is enabled.
+     */
+    private static Ignite sandboxIgniteProxy(Ignite ignite) {
+        return sandboxIgniteProxy(ignite, true);
+    }
+
+    /**
+     * @param ignite Ignite.
+     * @return Ignite component proxy if the Ignite Sandbox is enabled.
+     */
+    private static Ignite sandboxIgniteProxy(Ignite ignite, boolean doInsideSandboxCheck) {
+        if (ignite instanceof IgniteEx && (!doInsideSandboxCheck || insideSandbox())) {
+            return AccessController.doPrivileged((PrivilegedAction<Ignite>)
+                () -> SandboxIgniteComponentProxy.proxy(Ignite.class, ignite)
+            );
+        }
+
+        return ignite;
+    }
+
+    /**
+     * @return True if current thread runs inside the Ignite Sandbox.
+     */
+    private static boolean insideSandbox() {
+        if(!IgniteSecurityProcessor.hasSandboxedNodes())

Review comment:
       Space after `if`

##########
File path: modules/core/src/main/java/org/apache/ignite/Ignition.java
##########
@@ -538,7 +556,43 @@ public static Ignite ignite(@Nullable String name) throws IgniteIllegalStateExce
      * @throws IllegalArgumentException Thrown if current thread is not an {@link IgniteThread}.
      */
     public static Ignite localIgnite() throws IgniteIllegalStateException, IllegalArgumentException {
-        return IgnitionEx.localIgnite();
+        return sandboxIgniteProxy(IgnitionEx.localIgnite());
+    }
+
+    /**
+     * @param ignite Ignite.
+     * @return Ignite component proxy if the Ignite Sandbox is enabled.
+     */
+    private static Ignite sandboxIgniteProxy(Ignite ignite) {
+        return sandboxIgniteProxy(ignite, true);
+    }
+
+    /**
+     * @param ignite Ignite.
+     * @return Ignite component proxy if the Ignite Sandbox is enabled.
+     */
+    private static Ignite sandboxIgniteProxy(Ignite ignite, boolean doInsideSandboxCheck) {
+        if (ignite instanceof IgniteEx && (!doInsideSandboxCheck || insideSandbox())) {
+            return AccessController.doPrivileged((PrivilegedAction<Ignite>)
+                () -> SandboxIgniteComponentProxy.proxy(Ignite.class, ignite)
+            );
+        }
+
+        return ignite;
+    }
+
+    /**
+     * @return True if current thread runs inside the Ignite Sandbox.
+     */
+    private static boolean insideSandbox() {

Review comment:
       Let's move this method to `SecurityUtils` and rename to `isInsideSandbox`

##########
File path: modules/core/src/main/java/org/apache/ignite/Ignition.java
##########
@@ -490,7 +498,17 @@ public static Ignite ignite() throws IgniteIllegalStateException {
      * @return List of all grids started so far.
      */
     public static List<Ignite> allGrids() {
-        return IgnitionEx.allGrids();
+        List<Ignite> res = IgnitionEx.allGrids();
+
+        if (res == null || res.isEmpty())

Review comment:
       `F.isEmpty()`

##########
File path: modules/core/src/main/java/org/apache/ignite/Ignition.java
##########
@@ -538,7 +556,43 @@ public static Ignite ignite(@Nullable String name) throws IgniteIllegalStateExce
      * @throws IllegalArgumentException Thrown if current thread is not an {@link IgniteThread}.
      */
     public static Ignite localIgnite() throws IgniteIllegalStateException, IllegalArgumentException {
-        return IgnitionEx.localIgnite();
+        return sandboxIgniteProxy(IgnitionEx.localIgnite());
+    }
+
+    /**
+     * @param ignite Ignite.
+     * @return Ignite component proxy if the Ignite Sandbox is enabled.
+     */
+    private static Ignite sandboxIgniteProxy(Ignite ignite) {
+        return sandboxIgniteProxy(ignite, true);
+    }
+
+    /**
+     * @param ignite Ignite.
+     * @return Ignite component proxy if the Ignite Sandbox is enabled.
+     */
+    private static Ignite sandboxIgniteProxy(Ignite ignite, boolean doInsideSandboxCheck) {

Review comment:
       `doInsideSandboxCheck` flag name seems confusing. Let's remove this flag and rename the method to something like `wrapToProxy`, move `insideSandbox` check to another method and rename this method to something like `wrapToProxyIfNeeded`

##########
File path: modules/core/src/test/java/org/apache/ignite/internal/processors/security/sandbox/IgnitionComponentProxyTest.java
##########
@@ -0,0 +1,191 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.security.sandbox;
+
+import java.lang.reflect.Proxy;
+import java.security.AllPermission;
+import java.security.Permissions;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.UUID;
+import java.util.function.Supplier;
+import org.apache.ignite.Ignite;
+import org.apache.ignite.IgniteException;
+import org.apache.ignite.Ignition;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.util.typedef.G;
+import org.junit.Test;
+
+import static org.apache.ignite.plugin.security.SecurityPermissionSetBuilder.ALLOW_ALL;
+
+/**
+ * If the Ignite Sandbox is enabled, then {@link Ignition} methods that return existing instances of Ignite should
+ * return a component proxy.
+ */
+public class IgnitionComponentProxyTest extends AbstractSandboxTest {
+    /** Server node name. */
+    private static final String SRV = "sandbox.IgnitionComponentProxyTest";
+    /** Client node name. */
+    private static final String CLNT = "clnt";
+
+    /** {@inheritDoc} */
+    @Override protected void prepareCluster() throws Exception {
+        startGrid(SRV, ALLOW_ALL, false);
+
+        Permissions perms = new Permissions();
+
+        perms.add(new AllPermission());
+
+        startGrid(CLNT, ALLOW_ALL, perms, true);
+    }
+
+    /**
+     *
+     */
+    @Test
+    public void testLocalIgnite() {
+        Supplier<Ignite> s = new Supplier<Ignite>() {
+            @Override public Ignite get() {
+                return Ignition.localIgnite();
+            }
+        };
+
+        check(s);
+    }
+
+    /**
+     *
+     */
+    @Test
+    public void testGetOrStart() {
+        Supplier<Ignite> s = new Supplier<Ignite>() {
+            @Override public Ignite get() {
+                try {
+                    return Ignition.getOrStart(Ignition.localIgnite().configuration());
+                }
+                catch (Exception e) {
+                    throw new IgniteException(e);
+                }
+            }
+        };
+
+        check(s);
+    }
+
+    /**
+     *
+     */
+    @Test
+    public void testIgniteWithName() {
+        Supplier<Ignite> s = new Supplier<Ignite>() {
+            @Override public Ignite get() {
+                return Ignition.ignite(SRV);
+            }
+        };
+
+        check(s);
+    }
+
+    /**
+     *
+     */
+    @Test
+    public void testIgniteWithID() {
+        final UUID srvId = grid(SRV).localNode().id();
+
+        Supplier<Ignite> s = new Supplier<Ignite>() {
+            @Override public Ignite get() {
+                return Ignition.ignite(srvId);
+            }
+        };
+
+        check(s);
+    }
+
+    /**
+     *
+     */
+    @Test
+    public void testAllGrids() {
+        Supplier<Collection<Ignite>> s = new Supplier<Collection<Ignite>>() {
+            @Override public Collection<Ignite> get() {
+                return Ignition.allGrids();
+            }
+        };
+
+        checkCollection(s);
+    }
+
+    /**
+     *
+     */
+    @Test
+    public void testStart(){
+        Supplier<Ignite> s = new Supplier<Ignite>() {
+            @Override public Ignite get() {
+                try {
+                    return Ignition.start(getConfiguration("node_" + G.allGrids().size()));
+                }
+                catch (Exception e) {
+                    throw new IgniteException(e);
+                }
+            }
+        };
+
+        check(s);
+    }
+
+    /**
+     * @param s Supplier that returns an instance of Ignite.
+     */
+    private void check(final Supplier<Ignite> s) {
+        Supplier<Collection<Ignite>> supplier = new Supplier<Collection<Ignite>>() {
+            @Override public Collection<Ignite> get() {
+                return Collections.singleton(s.get());
+            }
+        };
+
+        checkCollection(supplier);
+    }
+
+    /**
+     * @param s Supplier that returns a collection of Ignite instances.
+     */
+    private void checkCollection(Supplier<Collection<Ignite>> s) {
+        Ignite srv = grid(SRV);
+
+        Ignite clnt = grid(CLNT);
+        //Checks that inside the sandbox we should get a proxied instance of Ignite.
+        clnt.compute(clnt.cluster().forNodeId(srv.cluster().localNode().id()))
+            .broadcast(() -> {
+                Collection<Ignite> nodes = s.get();
+
+                for (Ignite node : nodes) {
+                    assertTrue(Proxy.isProxyClass(node.getClass()));
+                    assertFalse(node instanceof IgniteEx);
+                }
+            });
+        //If we run outside of the sandbox, we should get an instance of IgniteEx.

Review comment:
       NL before comment and space after `//`

##########
File path: modules/core/src/test/java/org/apache/ignite/testframework/junits/GridAbstractTest.java
##########
@@ -1439,7 +1439,7 @@ protected void waitForDiscovery(Ignite... ignites) throws IgniteCheckedException
      */
     protected IgniteEx grid(String name) {
         if (!isRemoteJvm(name))
-            return (IgniteEx)G.ignite(name);
+            return (IgniteEx)IgnitionEx.grid(name);

Review comment:
       Why do we need it? I think it can fail only if we cast to `IgniteEx` inside test and can affect only sandbox tests, all other tests should work well. If we cast to `IgniteEx` inside sandbox test we should better rewrite this logic. 

##########
File path: modules/core/src/test/java/org/apache/ignite/internal/processors/security/sandbox/IgnitionComponentProxyTest.java
##########
@@ -0,0 +1,191 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.security.sandbox;
+
+import java.lang.reflect.Proxy;
+import java.security.AllPermission;
+import java.security.Permissions;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.UUID;
+import java.util.function.Supplier;
+import org.apache.ignite.Ignite;
+import org.apache.ignite.IgniteException;
+import org.apache.ignite.Ignition;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.util.typedef.G;
+import org.junit.Test;
+
+import static org.apache.ignite.plugin.security.SecurityPermissionSetBuilder.ALLOW_ALL;
+
+/**
+ * If the Ignite Sandbox is enabled, then {@link Ignition} methods that return existing instances of Ignite should
+ * return a component proxy.
+ */
+public class IgnitionComponentProxyTest extends AbstractSandboxTest {
+    /** Server node name. */
+    private static final String SRV = "sandbox.IgnitionComponentProxyTest";
+    /** Client node name. */
+    private static final String CLNT = "clnt";
+
+    /** {@inheritDoc} */
+    @Override protected void prepareCluster() throws Exception {
+        startGrid(SRV, ALLOW_ALL, false);
+
+        Permissions perms = new Permissions();
+
+        perms.add(new AllPermission());
+
+        startGrid(CLNT, ALLOW_ALL, perms, true);
+    }
+
+    /**
+     *
+     */
+    @Test
+    public void testLocalIgnite() {
+        Supplier<Ignite> s = new Supplier<Ignite>() {
+            @Override public Ignite get() {
+                return Ignition.localIgnite();
+            }
+        };
+
+        check(s);
+    }
+
+    /**
+     *
+     */
+    @Test
+    public void testGetOrStart() {
+        Supplier<Ignite> s = new Supplier<Ignite>() {
+            @Override public Ignite get() {
+                try {
+                    return Ignition.getOrStart(Ignition.localIgnite().configuration());
+                }
+                catch (Exception e) {
+                    throw new IgniteException(e);
+                }
+            }
+        };
+
+        check(s);
+    }
+
+    /**
+     *
+     */
+    @Test
+    public void testIgniteWithName() {
+        Supplier<Ignite> s = new Supplier<Ignite>() {
+            @Override public Ignite get() {
+                return Ignition.ignite(SRV);
+            }
+        };
+
+        check(s);
+    }
+
+    /**
+     *
+     */
+    @Test
+    public void testIgniteWithID() {
+        final UUID srvId = grid(SRV).localNode().id();
+
+        Supplier<Ignite> s = new Supplier<Ignite>() {
+            @Override public Ignite get() {
+                return Ignition.ignite(srvId);
+            }
+        };
+
+        check(s);
+    }
+
+    /**
+     *
+     */
+    @Test
+    public void testAllGrids() {
+        Supplier<Collection<Ignite>> s = new Supplier<Collection<Ignite>>() {
+            @Override public Collection<Ignite> get() {
+                return Ignition.allGrids();
+            }
+        };
+
+        checkCollection(s);
+    }
+
+    /**
+     *
+     */
+    @Test
+    public void testStart(){
+        Supplier<Ignite> s = new Supplier<Ignite>() {
+            @Override public Ignite get() {
+                try {
+                    return Ignition.start(getConfiguration("node_" + G.allGrids().size()));
+                }
+                catch (Exception e) {
+                    throw new IgniteException(e);
+                }
+            }
+        };
+
+        check(s);
+    }
+
+    /**
+     * @param s Supplier that returns an instance of Ignite.
+     */
+    private void check(final Supplier<Ignite> s) {
+        Supplier<Collection<Ignite>> supplier = new Supplier<Collection<Ignite>>() {
+            @Override public Collection<Ignite> get() {
+                return Collections.singleton(s.get());
+            }
+        };
+
+        checkCollection(supplier);
+    }
+
+    /**
+     * @param s Supplier that returns a collection of Ignite instances.
+     */
+    private void checkCollection(Supplier<Collection<Ignite>> s) {
+        Ignite srv = grid(SRV);
+
+        Ignite clnt = grid(CLNT);
+        //Checks that inside the sandbox we should get a proxied instance of Ignite.

Review comment:
       NL before comment and space after `//`

##########
File path: modules/core/src/main/java/org/apache/ignite/Ignition.java
##########
@@ -538,7 +556,43 @@ public static Ignite ignite(@Nullable String name) throws IgniteIllegalStateExce
      * @throws IllegalArgumentException Thrown if current thread is not an {@link IgniteThread}.
      */
     public static Ignite localIgnite() throws IgniteIllegalStateException, IllegalArgumentException {
-        return IgnitionEx.localIgnite();
+        return sandboxIgniteProxy(IgnitionEx.localIgnite());
+    }
+
+    /**
+     * @param ignite Ignite.
+     * @return Ignite component proxy if the Ignite Sandbox is enabled.
+     */
+    private static Ignite sandboxIgniteProxy(Ignite ignite) {
+        return sandboxIgniteProxy(ignite, true);
+    }
+
+    /**
+     * @param ignite Ignite.
+     * @return Ignite component proxy if the Ignite Sandbox is enabled.
+     */
+    private static Ignite sandboxIgniteProxy(Ignite ignite, boolean doInsideSandboxCheck) {
+        if (ignite instanceof IgniteEx && (!doInsideSandboxCheck || insideSandbox())) {

Review comment:
       Do we really need `IgniteEx` check? I can't imagine in which cases `IgnitionEx` will return something else.

##########
File path: modules/core/src/main/java/org/apache/ignite/Ignition.java
##########
@@ -490,7 +498,17 @@ public static Ignite ignite() throws IgniteIllegalStateException {
      * @return List of all grids started so far.
      */
     public static List<Ignite> allGrids() {
-        return IgnitionEx.allGrids();
+        List<Ignite> res = IgnitionEx.allGrids();
+
+        if (res == null || res.isEmpty())
+            return res;
+
+        Ignite ignite = res.get(0);

Review comment:
       We should check every node (or none of them, see comment about `IgniteEx`), but not only the first node. 

##########
File path: modules/core/src/test/java/org/apache/ignite/internal/processors/security/sandbox/IgnitionComponentProxyTest.java
##########
@@ -0,0 +1,191 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.security.sandbox;
+
+import java.lang.reflect.Proxy;
+import java.security.AllPermission;
+import java.security.Permissions;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.UUID;
+import java.util.function.Supplier;
+import org.apache.ignite.Ignite;
+import org.apache.ignite.IgniteException;
+import org.apache.ignite.Ignition;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.util.typedef.G;
+import org.junit.Test;
+
+import static org.apache.ignite.plugin.security.SecurityPermissionSetBuilder.ALLOW_ALL;
+
+/**
+ * If the Ignite Sandbox is enabled, then {@link Ignition} methods that return existing instances of Ignite should
+ * return a component proxy.
+ */
+public class IgnitionComponentProxyTest extends AbstractSandboxTest {
+    /** Server node name. */
+    private static final String SRV = "sandbox.IgnitionComponentProxyTest";
+    /** Client node name. */
+    private static final String CLNT = "clnt";
+
+    /** {@inheritDoc} */
+    @Override protected void prepareCluster() throws Exception {
+        startGrid(SRV, ALLOW_ALL, false);
+
+        Permissions perms = new Permissions();
+
+        perms.add(new AllPermission());
+
+        startGrid(CLNT, ALLOW_ALL, perms, true);
+    }
+
+    /**
+     *
+     */
+    @Test
+    public void testLocalIgnite() {
+        Supplier<Ignite> s = new Supplier<Ignite>() {
+            @Override public Ignite get() {
+                return Ignition.localIgnite();
+            }
+        };
+
+        check(s);
+    }
+
+    /**
+     *
+     */
+    @Test
+    public void testGetOrStart() {
+        Supplier<Ignite> s = new Supplier<Ignite>() {
+            @Override public Ignite get() {
+                try {
+                    return Ignition.getOrStart(Ignition.localIgnite().configuration());
+                }
+                catch (Exception e) {
+                    throw new IgniteException(e);
+                }
+            }
+        };
+
+        check(s);
+    }
+
+    /**
+     *
+     */
+    @Test
+    public void testIgniteWithName() {
+        Supplier<Ignite> s = new Supplier<Ignite>() {
+            @Override public Ignite get() {
+                return Ignition.ignite(SRV);
+            }
+        };
+
+        check(s);
+    }
+
+    /**
+     *
+     */
+    @Test
+    public void testIgniteWithID() {
+        final UUID srvId = grid(SRV).localNode().id();
+
+        Supplier<Ignite> s = new Supplier<Ignite>() {
+            @Override public Ignite get() {
+                return Ignition.ignite(srvId);
+            }
+        };
+
+        check(s);
+    }
+
+    /**
+     *
+     */
+    @Test
+    public void testAllGrids() {
+        Supplier<Collection<Ignite>> s = new Supplier<Collection<Ignite>>() {
+            @Override public Collection<Ignite> get() {
+                return Ignition.allGrids();
+            }
+        };
+
+        checkCollection(s);
+    }
+
+    /**
+     *
+     */
+    @Test
+    public void testStart(){
+        Supplier<Ignite> s = new Supplier<Ignite>() {
+            @Override public Ignite get() {
+                try {
+                    return Ignition.start(getConfiguration("node_" + G.allGrids().size()));
+                }
+                catch (Exception e) {
+                    throw new IgniteException(e);
+                }
+            }
+        };
+
+        check(s);
+    }
+
+    /**
+     * @param s Supplier that returns an instance of Ignite.
+     */
+    private void check(final Supplier<Ignite> s) {
+        Supplier<Collection<Ignite>> supplier = new Supplier<Collection<Ignite>>() {
+            @Override public Collection<Ignite> get() {
+                return Collections.singleton(s.get());
+            }
+        };
+
+        checkCollection(supplier);
+    }
+
+    /**
+     * @param s Supplier that returns a collection of Ignite instances.
+     */
+    private void checkCollection(Supplier<Collection<Ignite>> s) {
+        Ignite srv = grid(SRV);
+
+        Ignite clnt = grid(CLNT);
+        //Checks that inside the sandbox we should get a proxied instance of Ignite.
+        clnt.compute(clnt.cluster().forNodeId(srv.cluster().localNode().id()))
+            .broadcast(() -> {
+                Collection<Ignite> nodes = s.get();
+
+                for (Ignite node : nodes) {
+                    assertTrue(Proxy.isProxyClass(node.getClass()));

Review comment:
       Proxy it's not a goal itself. We should check that we can perform some operations using these ignite instances which we can't perform without proxy.




----------------------------------------------------------------
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.

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