SCMP echo problem #79

CHANGELOG.md

@@ -47,6 +47,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - Fixed internal raw path parsing and IP parsing.
   [#77](https://github.com/scionproto-contrib/jpan/pull/77)
 - Improved bootstrap logging [#83](https://github.com/scionproto-contrib/jpan/pull/83)
+- Fixed SCMP meta data reporting wrong remote port.
+  [#79](https://github.com/scionproto-contrib/jpan/issues/79)
 
 ## [0.1.1] - 2024-05-10
 

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

@@ -64,6 +64,8 @@
 
     int i1 = data.getInt(4);
     int i2 = data.getInt(8);
+    int nextHeader = ByteUtil.readInt(i1, 0, 8);
+    InternalConstants.HdrTypes hdrType = InternalConstants.HdrTypes.parse(nextHeader);
     int hdrLen = ByteUtil.readInt(i1, 8, 8);
     int hdrLenBytes = hdrLen * 4;
     int dl = ByteUtil.readInt(i2, 10, 2);
@@ -107,11 +109,22 @@
       reversePathInPlace(ByteBuffer.wrap(path));
     }
 
-    // get remote port from UDP overlay
+    // get remote port from UDP or SCMP payload
     data.position(hdrLenBytes);
-    int srcPort = Short.toUnsignedInt(data.getShort());
-    int dstPort = Short.toUnsignedInt(data.getShort());
-    // TODO handle SCMP packets
+    int srcPort;
+    int dstPort;
+    if (hdrType == InternalConstants.HdrTypes.UDP) {
+      // get remote port from UDP overlay
+      srcPort = Short.toUnsignedInt(data.getShort());
+      dstPort = Short.toUnsignedInt(data.getShort());
+    } else if (hdrType == InternalConstants.HdrTypes.SCMP) {
+      data.position(hdrLenBytes + 4);
+      // ports will be swapped further down
+      dstPort = Short.toUnsignedInt(data.getShort());
+      srcPort = Constants.SCMP_PORT;
+    } else {
+      throw new UnsupportedOperationException();
+    }
 
     // rewind to original offset
     data.position(pos);

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

@@ -32,6 +32,7 @@
 import org.scion.jpan.demo.inspector.ScionPacketInspector;
 import org.scion.jpan.internal.ScionHeaderParser;
 import org.scion.jpan.testutil.MockNetwork;
+import org.scion.jpan.testutil.MockScmpHandler;
 
 public class SCMPTest {
   private static final byte[] PING_ERROR_4_51_HK = {
@@ -104,27 +105,45 @@ void echo() throws IOException {
   @Test
   void echo_localAS_BR() throws IOException {
     testEcho(this::getPathToLocalAS_BR);
+    assertEquals(1, MockNetwork.getAndResetForwardCount()); // 1!
+    assertEquals(1, MockScmpHandler.getAndResetAnswerTotal());
+  }
+
+  @Test
+  void echo_localAS_BR_30041() throws IOException {
+    testEcho(this::getPathToLocalAS_BR_30041);
+    assertEquals(0, MockNetwork.getAndResetForwardCount()); // 0!
+    assertEquals(1, MockScmpHandler.getAndResetAnswerTotal());
   }
 
   @Test
   void echo_localAS_SVC() {
-    assertThrows(ScionException.class, () -> testEcho(this::getPathToLocalAS_SVC));
+    // Sending to local service address makes no sense, hence the exception.
+    // SVC addresses are interpreted by border routers, so we would need to send to a border router
+    // first.
+    Exception e = assertThrows(ScionException.class, () -> testEcho(this::getPathToLocalAS_SVC));
+    assertTrue(e.getMessage().contains("Cannot ping service address in local AS:"), e.getMessage());
   }
 
-  private void testEcho(Supplier<Path> path) throws IOException {
+  private void testEcho(Supplier<Path> pathSupplier) throws IOException {
     MockNetwork.startTiny();
     MockNetwork.answerNextScmpEchos(1);
     try (ScmpChannel channel = Scmp.createChannel()) {
       channel.setScmpErrorListener(scmpMessage -> fail(scmpMessage.getTypeCode().getText()));
       channel.setOption(ScionSocketOptions.SCION_API_THROW_PARSER_FAILURE, true);
       byte[] data = new byte[] {1, 2, 3, 4, 5};
-      Scmp.EchoMessage result = channel.sendEchoRequest(path.get(), 42, ByteBuffer.wrap(data));
+      Path path = pathSupplier.get();
+      Scmp.EchoMessage result = channel.sendEchoRequest(path, 42, ByteBuffer.wrap(data));
       assertEquals(42, result.getSequenceNumber());
       assertEquals(Scmp.TypeCode.TYPE_129, result.getTypeCode());
       assertTrue(result.getNanoSeconds() > 0);
       assertTrue(result.getNanoSeconds() < 100_000_000); // 10 ms
       assertArrayEquals(data, result.getData());
       assertFalse(result.isTimedOut());
+      Path returnPath = result.getPath();
+      assertEquals(path.getRemoteAddress(), returnPath.getRemoteAddress());
+      assertEquals(Constants.SCMP_PORT, returnPath.getRemotePort());
+      assertEquals(path.getRemoteIsdAs(), returnPath.getRemoteIsdAs());
     } finally {
       MockNetwork.stopTiny();
     }
@@ -318,26 +337,25 @@ private Path getPathTo112(InetAddress dstAddress) {
   }
 
   private Path getPathToLocalAS_BR() {
-    ScionService service = Scion.defaultService();
-    long dstIA = service.getLocalIsdAs();
-    try {
-      // Border router address
-      InetAddress addr = InetAddress.getByName(MockNetwork.BORDER_ROUTER_HOST);
-      int port = MockNetwork.BORDER_ROUTER_PORT1;
-      List<Path> paths = service.getPaths(dstIA, new InetSocketAddress(addr, port));
-      return paths.get(0);
-    } catch (UnknownHostException e) {
-      throw new RuntimeException(e);
-    }
+    // Border router address
+    return getPathToLocalAS(MockNetwork.BORDER_ROUTER_IPV4, MockNetwork.BORDER_ROUTER_PORT1);
+  }
+
+  private Path getPathToLocalAS_BR_30041() {
+    // Border router address
+    return getPathToLocalAS(MockNetwork.BORDER_ROUTER_IPV4, Constants.SCMP_PORT);
   }
 
   private Path getPathToLocalAS_SVC() {
+    return getPathToLocalAS("0.0.0.0", Constants.SCMP_PORT);
+  }
+
+  private Path getPathToLocalAS(String addressStr, int port) {
     ScionService service = Scion.defaultService();
     long dstIA = service.getLocalIsdAs();
     try {
       // Service address
-      InetAddress addr = InetAddress.getByAddress(new byte[] {0, 0, 0, 0});
-      int port = Constants.SCMP_PORT;
+      InetAddress addr = InetAddress.getByName(addressStr);
       List<Path> paths = service.getPaths(dstIA, new InetSocketAddress(addr, port));
       return paths.get(0);
     } catch (UnknownHostException e) {
@@ -348,6 +366,7 @@ private Path getPathToLocalAS_SVC() {
   @Test
   void setUpScmpResponder_echo() throws IOException, InterruptedException {
     MockNetwork.startTiny();
+    MockScmpHandler.stop(); // Shut down SCMP handler
     Path path = getPathTo112(InetAddress.getLoopbackAddress());
     // sender is in 110; responder is in 112
     try (ScmpChannel sender = Scmp.createChannel()) {
@@ -380,6 +399,7 @@ void setUpScmpResponder_echo() throws IOException, InterruptedException {
   @Test
   void setUpScmpResponder_echo_blocked() throws IOException, InterruptedException {
     MockNetwork.startTiny();
+    MockScmpHandler.stop(); // Shut down SCMP handler
     Path path = getPathTo112(InetAddress.getLoopbackAddress());
     // sender is in 110; responder is in 112
     try (ScmpChannel sender = Scmp.createChannel()) {

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

@@ -43,9 +43,21 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+/**
+ * The mock network is a simplified version of the test network available in scionproto. The mock is
+ * primarily used to run the "tiny" network. Some simplifications:<br>
+ *
+ * <p>- The mock has only two "border routers". They act as border routers for _all_ ASes. There are
+ * two border routers to allow having multiple links between ASes.<br>
+ * - The mock border routers forward traffic directly to the target AS, even if there is no direct
+ * link in the topology.<br>
+ * - The IP on both sides of the BR (link), at least by default, the same.<br>
+ * - the border routers do only marginal verification on packets.<br>
+ */
 public class MockNetwork {
 
-  public static final String BORDER_ROUTER_HOST = "127.0.0.1";
+  public static final String BORDER_ROUTER_IPV4 = "127.0.0.1";
+  public static final String BORDER_ROUTER_IPv6 = "::1";
   public static final String TINY_SRV_ADDR_1 = "127.0.0.112";
   public static final byte[] TINY_SRV_ADDR_BYTES_1 = {127, 0, 0, 112};
   public static final int TINY_SRV_PORT_1 = 22233;
@@ -75,26 +87,31 @@ public static synchronized void startTiny() {
   }
 
   public static synchronized void startTiny(boolean localIPv4, boolean remoteIPv4) {
-    startTiny(localIPv4, remoteIPv4, Mode.DAEMON);
+    startTiny(
+        localIPv4 ? BORDER_ROUTER_IPV4 : BORDER_ROUTER_IPv6,
+        remoteIPv4 ? BORDER_ROUTER_IPV4 : BORDER_ROUTER_IPv6,
+        Mode.DAEMON);
   }
 
   public static synchronized void startTiny(Mode mode) {
-    startTiny(true, true, mode);
+    startTiny(BORDER_ROUTER_IPV4, BORDER_ROUTER_IPV4, mode);
   }
 
-  private static synchronized void startTiny(boolean localIPv4, boolean remoteIPv4, Mode mode) {
+  private static synchronized void startTiny(String localIP, String remoteIP, Mode mode) {
     if (routers != null) {
       throw new IllegalStateException();
     }
 
     routers = Executors.newFixedThreadPool(2);
 
+    MockScmpHandler.start();
+
     List<MockBorderRouter> brList = new ArrayList<>();
     brList.add(
-        new MockBorderRouter(0, BORDER_ROUTER_PORT1, BORDER_ROUTER_PORT2, localIPv4, remoteIPv4));
+        new MockBorderRouter(0, BORDER_ROUTER_PORT1, BORDER_ROUTER_PORT2, localIP, remoteIP));
     brList.add(
         new MockBorderRouter(
-            1, BORDER_ROUTER_PORT1 + 10, BORDER_ROUTER_PORT2 + 10, localIPv4, remoteIPv4));
+            1, BORDER_ROUTER_PORT1 + 10, BORDER_ROUTER_PORT2 + 10, localIP, remoteIP));
 
     barrier = new CountDownLatch(brList.size());
     for (MockBorderRouter br : brList) {
@@ -111,7 +128,7 @@ private static synchronized void startTiny(boolean localIPv4, boolean remoteIPv4
 
     List<InetSocketAddress> brAddrList =
         brList.stream()
-            .map(mBR -> new InetSocketAddress(BORDER_ROUTER_HOST, mBR.getPort1()))
+            .map(mBR -> new InetSocketAddress(BORDER_ROUTER_IPV4, mBR.getPort1()))
             .collect(Collectors.toList());
     try {
       daemon = MockDaemon.createForBorderRouter(brAddrList).start();
@@ -162,6 +179,9 @@ public static synchronized void stopTiny() {
       }
       routers = null;
     }
+
+    MockScmpHandler.stop();
+
     dropNextPackets.getAndSet(0);
     answerNextScmpEchos.getAndSet(0);
     scmpErrorOnNextPacket.set(null);
@@ -234,23 +254,23 @@ class MockBorderRouter implements Runnable {
   private final String name;
   private final int port1;
   private final int port2;
-  private final boolean ipv4_1;
-  private final boolean ipv4_2;
+  private final String ip1;
+  private final String ip2;
 
-  MockBorderRouter(int id, int port1, int port2, boolean ipv4_1, boolean ipv4_2) {
+  MockBorderRouter(int id, int port1, int port2, String ip1, String ip2) {
     this.id = id;
     this.name = "BorderRouter-" + id;
     this.port1 = port1;
     this.port2 = port2;
-    this.ipv4_1 = ipv4_1;
-    this.ipv4_2 = ipv4_2;
+    this.ip1 = ip1;
+    this.ip2 = ip2;
   }
 
   @Override
   public void run() {
     Thread.currentThread().setName(name);
-    InetSocketAddress bind1 = new InetSocketAddress(ipv4_1 ? "localhost" : "::1", port1);
-    InetSocketAddress bind2 = new InetSocketAddress(ipv4_2 ? "localhost" : "::1", port2);
+    InetSocketAddress bind1 = new InetSocketAddress(ip1, port1);
+    InetSocketAddress bind2 = new InetSocketAddress(ip2, port2);
     try (DatagramChannel chnLocal = DatagramChannel.open().bind(bind1);
         DatagramChannel chnRemote = DatagramChannel.open().bind(bind2);
         Selector selector = Selector.open()) {
@@ -260,7 +280,7 @@ public void run() {
       chnRemote.register(selector, SelectionKey.OP_READ, chnLocal);
       ByteBuffer buffer = ByteBuffer.allocate(66000);
       MockNetwork.barrier.countDown();
-      logger.info(name + " started on ports " + bind1 + " <-> " + bind2);
+      logger.info("{} started on ports {} <-> {}", name, bind1, bind2);
 
       while (true) {
         if (selector.select() == 0) {
@@ -287,9 +307,9 @@ public void run() {
               continue;
             }
 
-            if (MockNetwork.scmpErrorOnNextPacket.get() != null) {
-              sendScmp(
-                  MockNetwork.scmpErrorOnNextPacket.getAndSet(null), buffer, srcAddress, incoming);
+            Scmp.TypeCode errorCode = MockNetwork.scmpErrorOnNextPacket.getAndSet(null);
+            if (errorCode != null) {
+              sendScmp(errorCode, buffer, srcAddress, incoming);
               iter.remove();
               continue;
             }
@@ -299,11 +319,11 @@ public void run() {
                 forwardPacket(buffer, srcAddress, outgoing);
                 break;
               case SCMP:
-                handleScmp(buffer, srcAddress, incoming, outgoing);
+                handleScmp(buffer, srcAddress, outgoing);
                 break;
               default:
                 logger.error(
-                    "HDR not supported: " + PackageVisibilityHelper.getNextHdr(buffer).code());
+                    "HDR not supported: {}", PackageVisibilityHelper.getNextHdr(buffer).code());
                 throw new UnsupportedOperationException();
             }
           }
@@ -321,25 +341,15 @@ private void forwardPacket(ByteBuffer buffer, SocketAddress srcAddress, Datagram
       throws IOException {
     InetSocketAddress dstAddress = PackageVisibilityHelper.getDstAddress(buffer);
     logger.info(
-        name
-            + " forwarding "
-            + buffer.remaining()
-            + " bytes from "
-            + srcAddress
-            + " to "
-            + dstAddress);
+        "{} forwarding {} bytes from {} to {}", name, buffer.remaining(), srcAddress, dstAddress);
 
     outgoing.send(buffer, dstAddress);
     buffer.clear();
     MockNetwork.nForwardTotal.incrementAndGet();
     MockNetwork.nForwards.incrementAndGet(id);
   }
 
-  private void handleScmp(
-      ByteBuffer buffer,
-      SocketAddress srcAddress,
-      DatagramChannel incoming,
-      DatagramChannel outgoing)
+  private void handleScmp(ByteBuffer buffer, SocketAddress srcAddress, DatagramChannel outgoing)
       throws IOException {
     buffer.position(ScionHeaderParser.extractHeaderLength(buffer));
     Scmp.Type type0 = ScmpParser.extractType(buffer);
@@ -351,6 +361,9 @@ private void handleScmp(
       return;
     }
 
+    // TODO If we have implemented proper IPs for border routers we should read the dst from
+    //   the packet and then decide whether to answer or not.
+    //   InetSocketAddress dstAddress = PackageVisibilityHelper.getDstAddress(buffer);
     // ignore SCMP requests unless we are instructed to answer them
     if (type0 == Scmp.Type.INFO_128 && MockNetwork.answerNextScmpEchos.get() == 0) {
       buffer.rewind();
@@ -359,38 +372,14 @@ private void handleScmp(
     }
     MockNetwork.answerNextScmpEchos.decrementAndGet();
 
+    // relay to ScmpHandler
     buffer.rewind();
-    InetSocketAddress dstAddress = PackageVisibilityHelper.getDstAddress(buffer);
-    // From here on we use linear reading using the buffer's position() mechanism
-    buffer.position(ScionHeaderParser.extractHeaderLength(buffer));
-    Path path = PackageVisibilityHelper.getResponsePath(buffer, (InetSocketAddress) srcAddress);
-    Scmp.Type type = ScmpParser.extractType(buffer);
-    Scmp.Message scmpMsg = PackageVisibilityHelper.createMessage(type, path);
-    ScmpParser.consume(buffer, scmpMsg);
-    logger.info(
-        " received SCMP " + scmpMsg.getTypeCode().name() + " " + scmpMsg.getTypeCode().getText());
-
-    if (scmpMsg instanceof Scmp.EchoMessage) {
-      // send back!
-      // This is very basic:
-      // - we always answer regardless of whether we are actually the destination.
-      // - We do not invert path / addresses
-      sendScmp(Scmp.TypeCode.TYPE_129, buffer, srcAddress, incoming);
-    } else if (scmpMsg instanceof Scmp.TracerouteMessage) {
-      answerTraceRoute(buffer, srcAddress, incoming);
-    } else {
-      // forward error
-      logger.info(
-          name
-              + " forwarding SCMP error "
-              + scmpMsg.getTypeCode().getText()
-              + " from "
-              + srcAddress
-              + " to "
-              + dstAddress);
-      outgoing.send(buffer, dstAddress);
-      buffer.clear();
-    }
+
+    InetAddress scmpIP = InetAddress.getLoopbackAddress();
+    InetSocketAddress dst = new InetSocketAddress(scmpIP, Constants.SCMP_PORT);
+    logger.info("{} relaying {} bytes from {} to {}", name, buffer.remaining(), srcAddress, dst);
+    outgoing.send(buffer, dst);
+    buffer.clear();
   }
 
   private void sendScmp(