Don´t start SHIM if port range = ALL (#139)

* Don't start SHIM for port range ALL

---------

Co-authored-by: Tilmann Zäschke <tilmann.zaeschke@inf.ethz.ch>

CHANGELOG.md

@@ -9,8 +9,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ## [Unreleased]
 
 ### TODO for 0.4.0
+- testDomainSearchResolver_invalidHost() takes 4-5 seconds!
 - ShimTest: TODO the serverService(null) should be removed once SHIM becomes the default
-- SHIM TODO: Do not start SHIM if port-range=ALL
 - Cache paths
 - Fix @Disabled tests
 - Create handling for SCMP errors 5 + 6 (interface down, connectivity down). Subclasses?
@@ -67,6 +67,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
   - Caching for getExternalIP()
   - ShowpathsDemo output
   - Spurious failures of SCMP exception handling tests
+- Do not start SHIM if dispatcher port range is ALL
+  [#139](https://github.com/scionproto-contrib/jpan/pull/139)
 
 ## [0.3.0] - 2024-10-09
 

README.md

@@ -350,9 +350,10 @@ is configurable, see next section.
 
 ### SHIM
 
-Every JPAN application will try to start a 
+The SHIM is required to support the `dispatched_ports` feature in topo files.
+Every JPAN application will try to start a  
 [SHIM dispatcher](https://docs.scion.org/en/latest/dev/design/router-port-dispatch.html)
-on port 30041. The SHIM is required to support the `dispathed_ports` feature in topo files.
+on port 30041, unless the port range is set to `all`. 
 
 A SHIM does no traffic checking, it blindly forwards every parseable packet to the inscribed SCION 
 destination address. That means a JPAN SHIM will act as a SHIM for all applications on a machine.
@@ -366,12 +367,12 @@ Whether a SHIM is started can be controlled with a configuration option, see bel
 
 ### Other Options
 
-| Option                                                                                               | Java property                                     | Environment variable                            | Default value      |
-|------------------------------------------------------------------------------------------------------|---------------------------------------------------|-------------------------------------------------|--------------------|
+| Option                                                                                                               | Java property                                     | Environment variable                            | Default value      |
+|----------------------------------------------------------------------------------------------------------------------|---------------------------------------------------|-------------------------------------------------|--------------------|
 | Path expiry margin. Before sending a packet a new path is requested if the path is about to expire within X seconds. | `org.scion.pathExpiryMargin`                      | `SCION_PATH_EXPIRY_MARGIN`                      | 10                 |
-| Location of `hosts` file. Multiple location can be specified separated by `;`.                       | `org.scion.hostsFiles`                            | `SCION_HOSTS_FILES`                             | `/etc/scion/hosts` |
-| Start SHIM.                                                                                          | `org.scion.shim`                                  | `SCION_SHIM`                                    | `true`             |
-| Minimize segment requests to local AS at the cost of reduced range of path available.                | `org.scion.resolver.experimentalMinimizeRequests` | `EXPERIMENTAL_SCION_RESOLVER_MINIMIZE_REQUESTS` | `false`            |
+| Location of `hosts` file. Multiple location can be specified separated by `;`.                                       | `org.scion.hostsFiles`                            | `SCION_HOSTS_FILES`                             | `/etc/scion/hosts` |
+| Start SHIM. If not set, SHIM will be started unless the dispatcher port range is set to `all`.                       | `org.scion.shim`                                  | `SCION_SHIM`                                    |                    |
+| Minimize segment requests to local AS at the cost of reduced range of path available.                                | `org.scion.resolver.experimentalMinimizeRequests` | `EXPERIMENTAL_SCION_RESOLVER_MINIMIZE_REQUESTS` | `false`            |
 
 `EXPERIMENTAL_SCION_RESOLVER_MINIMIZE_REQUESTS` is a non-standard option that request CORE segments only of other 
 path can be constructed. This may reduce response time when requesting new paths. It is very likely,

src/main/java/org/scion/jpan/ScionService.java

@@ -128,9 +128,9 @@ protected ScionService(String addressOrHost, Mode mode) {
       segmentStub = SegmentLookupServiceGrpc.newBlockingStub(channel);
     }
     shutdownHook = addShutdownHook();
-    Shim.install(this);
     try {
       getLocalIsdAs(); // Init
+      checkStartShim();
     } catch (RuntimeException e) {
       // If this fails for whatever reason we want to make sure that the channel is closed.
       try {
@@ -142,6 +142,16 @@ protected ScionService(String addressOrHost, Mode mode) {
     }
   }
 
+  private void checkStartShim() {
+    // Start SHIM unless we have port range 'ALL'. However, config overrides this setting.
+    String config = ScionUtil.getPropertyOrEnv(Constants.PROPERTY_SHIM, Constants.ENV_SHIM);
+    boolean hasAllPorts = getLocalPortRange().hasPortRangeALL();
+    boolean start = config != null ? Boolean.parseBoolean(config) : !hasAllPorts;
+    if (start) {
+      Shim.install(this);
+    }
+  }
+
   /**
    * Returns the default instance of the ScionService. The default instance is connected to the
    * daemon that is specified by the default properties or environment variables.

src/main/java/org/scion/jpan/internal/LocalTopology.java

@@ -281,6 +281,10 @@ public boolean hasPortRange() {
       return portMin >= 1 && portMax <= 65535 && portMax >= portMin;
     }
 
+    public boolean hasPortRangeALL() {
+      return portMin == 1 && portMax == 65535;
+    }
+
     public InetSocketAddress mapToLocalPort(InetSocketAddress address) {
       if (address.getPort() == Constants.SCMP_PORT
           || (address.getPort() >= portMin && address.getPort() <= portMax)) {

src/main/java/org/scion/jpan/internal/Shim.java

@@ -54,13 +54,8 @@ private Shim(ScionService service, int port) {
   public static void install(ScionService service) {
     synchronized (singleton) {
       if (singleton.get() == null) {
-        boolean flag =
-            ScionUtil.getPropertyOrEnv(
-                Constants.PROPERTY_SHIM, Constants.ENV_SHIM, Constants.DEFAULT_SHIM);
-        if (flag) {
-          singleton.set(Shim.newBuilder(service).build());
-          singleton.get().start();
-        }
+        singleton.set(Shim.newBuilder(service).build());
+        singleton.get().start();
       }
     }
   }

src/test/java/org/scion/jpan/api/ScionServiceTest.java

@@ -75,8 +75,8 @@ void testDaemonCreationIPv6() throws IOException {
     try (Scion.CloseableService client = Scion.newServiceWithDaemon(daemonAddr)) {
       Path path = client.getPaths(dstIA, dstAddress).get(0);
       assertNotNull(path);
-      // local AS + path
-      assertEquals(2, MockDaemon.getAndResetCallCount());
+      // port-range + local AS + path
+      assertEquals(3, MockDaemon.getAndResetCallCount());
     } finally {
       MockDaemon.closeDefault();
     }
@@ -91,8 +91,8 @@ void testDaemonCreationIPv4() throws IOException {
     try (Scion.CloseableService client = Scion.newServiceWithDaemon(daemonAddr)) {
       Path path = client.getPaths(dstIA, dstAddress).get(0);
       assertNotNull(path);
-      // local AS + path
-      assertEquals(2, MockDaemon.getAndResetCallCount());
+      // port-range + local AS + path
+      assertEquals(3, MockDaemon.getAndResetCallCount());
     } finally {
       MockDaemon.closeDefault();
     }
@@ -119,18 +119,14 @@ void getPath() throws IOException {
       //    0: 2 561850441793808
       //    0: 1 561850441793810
       assertEquals("/127.0.0.10:31004", path.getFirstHopAddress().toString());
-      // assertEquals(srcIA, path.getSourceIsdAs());
       assertEquals(dstIA, path.getRemoteIsdAs());
       assertEquals(36, path.getRawPath().length);
 
       assertEquals("127.0.0.10:31004", path.getMetadata().getInterface().getAddress());
       assertEquals(2, path.getMetadata().getInterfacesList().size());
-      // assertEquals(1, viewer.getInternalHopsList().size());
-      // assertEquals(0, viewer.getMtu());
-      // assertEquals(0, viewer.getLinkTypeList().size());
 
-      // localAS & path
-      assertEquals(2, MockDaemon.getAndResetCallCount());
+      // port-range + local AS + path
+      assertEquals(3, MockDaemon.getAndResetCallCount());
     } finally {
       MockDaemon.closeDefault();
     }
@@ -159,12 +155,11 @@ void getPaths() throws IOException {
       assertEquals(1, paths.size());
       for (Path path : paths) {
         assertEquals("/127.0.0.10:31004", path.getFirstHopAddress().toString());
-        // assertEquals(srcIA, path.getSourceIsdAs());
         assertEquals(dstIA, path.getRemoteIsdAs());
       }
 
-      // get local AS, get PATH
-      assertEquals(2, MockDaemon.getAndResetCallCount());
+      // port-range + local AS + path
+      assertEquals(3, MockDaemon.getAndResetCallCount());
     } finally {
       MockDaemon.closeDefault();
     }
@@ -189,16 +184,15 @@ void getPaths_localAS() throws IOException {
       //  Path:  exp=1708596832 / 2024-02-22T10:13:52Z  mtu=1472
       //  Path: first hop =
       //          raw: []
-      //  raw: {}
       assertEquals(1, paths.size());
       Path path = paths.get(0);
       InetAddress addr = path.getRemoteAddress();
       InetSocketAddress sAddr = new InetSocketAddress(addr, path.getRemotePort());
       assertEquals(sAddr, path.getFirstHopAddress());
       assertEquals(dstIA, path.getRemoteIsdAs());
 
-      // get local AS, get PATH
-      assertEquals(2, MockDaemon.getAndResetCallCount());
+      // get port-range + local AS + path
+      assertEquals(3, MockDaemon.getAndResetCallCount());
     } finally {
       MockDaemon.closeDefault();
     }

src/test/java/org/scion/jpan/api/ScionTest.java

@@ -93,8 +93,8 @@ void defaultService_daemon() throws IOException {
       ScionService service = Scion.defaultService();
       Path path = service.getPaths(dstIA, dstAddress).get(0);
       assertNotNull(path);
-      // local AS + path
-      assertEquals(2, MockDaemon.getAndResetCallCount());
+      // port-range + local AS + path
+      assertEquals(3, MockDaemon.getAndResetCallCount());
     } finally {
       Scion.closeDefault();
       MockDaemon.closeDefault();

src/test/java/org/scion/jpan/internal/HeaderComposeTest.java

@@ -123,6 +123,7 @@ void testCompose() throws IOException {
       }
       assertEquals(packetBytes[i], p.get(i), "Mismatch at position " + i);
     }
-    assertEquals(2, MockDaemon.getAndResetCallCount());
+    // port-range + local AS + path
+    assertEquals(3, MockDaemon.getAndResetCallCount());
   }
 }

src/test/java/org/scion/jpan/internal/InspectorComposeTest.java

@@ -129,6 +129,7 @@ void testCompose() throws IOException {
       }
       assertEquals(packetBytes[i], data.get(i), "Mismatch at position " + i);
     }
-    assertEquals(2, MockDaemon.getAndResetCallCount());
+    // port-range + local AS + path
+    assertEquals(3, MockDaemon.getAndResetCallCount());
   }
 }

src/test/java/org/scion/jpan/internal/ShimTest.java

@@ -145,6 +145,22 @@ void testForwardingUDP() {
     assertEquals(1 * 2 * 10, MockNetwork.getAndResetForwardCount());
   }
 
+  @Test
+  void testShim_notStartedForALL() throws IOException {
+    // Do not start SHIM in an AS with port range ALL
+    MockNetwork.startTiny();
+    ScionService service = null;
+    try {
+      service = Scion.newServiceWithTopologyFile("topologies/dispatcher-port-range-all.json");
+      try (ScionDatagramChannel unused = ScionDatagramChannel.open(service)) {
+        assertFalse(Shim.isInstalled());
+      }
+    } finally {
+      service.close();
+      MockNetwork.stopTiny();
+    }
+  }
+
   @Test
   void testForwardingUDP_LocalAS_remoteInsideRange() {
     testForwardingUDP_LocalAS(31000, 0);

src/test/java/org/scion/jpan/testutil/MockBorderRouter.java

@@ -22,9 +22,7 @@
 import java.nio.channels.SelectionKey;
 import java.nio.channels.Selector;
 import java.util.Iterator;
-import org.scion.jpan.Constants;
 import org.scion.jpan.PackageVisibilityHelper;
-import org.scion.jpan.ScionUtil;
 import org.scion.jpan.Scmp;
 import org.scion.jpan.demo.inspector.ScionPacketInspector;
 import org.scion.jpan.demo.inspector.ScmpHeader;
@@ -125,8 +123,7 @@ public void run() {
   private void forwardPacket(ByteBuffer buffer, SocketAddress srcAddress, DatagramChannel outgoing)
       throws IOException {
     InetSocketAddress dstAddress = PackageVisibilityHelper.getDstAddress(buffer);
-    if (ScionUtil.getPropertyOrEnv(
-        Constants.PROPERTY_SHIM, Constants.ENV_SHIM, Constants.DEFAULT_SHIM)) {
+    if (MockNetwork.useShim()) {
       dstAddress = MockNetwork.asInfo.mapDispatcherPorts(dstAddress);
     }
     logger.info(

src/test/java/org/scion/jpan/testutil/MockNetwork.java

@@ -110,8 +110,7 @@ private static synchronized void startTiny(String localTopo, String remoteTopo,
     String remoteIP =
         mock.asInfoLocal.getBorderRouterAddressByIA(ScionUtil.parseIA(TINY_SRV_ISD_AS));
     remoteIP = IPHelper.extractIP(remoteIP);
-    if (!ScionUtil.getPropertyOrEnv(
-        Constants.PROPERTY_SHIM, Constants.ENV_SHIM, Constants.DEFAULT_SHIM)) {
+    if (!useShim()) {
       // Do not start a SCMP handler on 30041 if we want to use SHIMs.
       // The SHIM also includes its own SCMP handler.
       MockScmpHandler.start(remoteIP);
@@ -223,6 +222,12 @@ private MockNetwork(String localTopo, String remoteTopo) {
     asInfoLocal.connectWith(asInfoRemote);
   }
 
+  public static boolean useShim() {
+    String config = ScionUtil.getPropertyOrEnv(Constants.PROPERTY_SHIM, Constants.ENV_SHIM);
+    boolean hasAllPorts = mock.asInfoLocal.getPortRange().hasPortRangeALL();
+    return config != null ? Boolean.parseBoolean(config) : !hasAllPorts;
+  }
+
   public static InetSocketAddress getBorderRouterAddress1() {
     return mock.localAddress[0];
   }