Refactor and extend NAT with UDP support #9

TMVector · 2016-07-13T12:29:43Z

Refactor the NAT to reflect the approach of an applications software engineer. Also add support for UDP, timeouts, TCP state, and TIME_WAIT.

Was previously removed, but only in part for some reason.

No longer needed because constructor parameters can be provided from config

Tests for connection timeout and connection close. Using Scapy instead of netcat or other so that the OS doesn't keep us from using the same port because of TIME_WAIT on closed connections.

- Move into a separate subfolder - Split into separate files - Provide strongly typed access to packets via the PacketEncapsulation class - Make ingoing and outgoing methods, data, etc. more homogenous - Separate the space of addresses for different protocols - Remove connections when they close - Remove connections after a timeout

When running commands in xterm, the exitcode is that of xterm, not the command. Use named pipes to fix this while still displaying output in separate windows for the user.

Conflicts: Paxifax.cs examples/NAT.cs examples/Test.cs

The second scapy test was not failing when it should. Fixed to make sure we are actually testing the transmission of packets after a Fin close.

And fix bug with the TCP state - was specifying the wrong direction to the state transition function. Tests should all pass now

TMVector · 2016-07-13T18:55:01Z

There appears to be some problems with UDP. Working on it.

niksu · 2016-07-13T19:55:05Z

Thanks for the heads-up. How did the problem come to light?

On 2016-07-13 19:55, TMVector wrote:

There appears to be some problems with UDP. Working on it.

You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub [1], or mute the
thread [2].

Links:

[1] #9 (comment)
[2]
https://github.com/notifications/unsubscribe/ABDTTg0ppd-LoqzGXHiPdLSpxDDcJYnjks5qVTSFgaJpZM4JLXUX

http://www.cl.cam.ac.uk/~ns441/

TMVector · 2016-07-13T20:05:01Z

It had escaped my attention that I only test TCP currently, so it only became apparent when I started doing performance testing with iperf. The forwarded packets have an unknown ethertype according to tcpdump.

There's also a problem when forwarding TCP packets of the max size. SharpPcap says it can't send it because it's too long. It's possible the UDP problem is also related to large packets, since I've only tested with iperf so far.

niksu · 2016-07-13T20:06:48Z

Commit cca28fc seems more general than the subject of the PR ("Refactor and extend NAT with UDP support"), so out of interest what need does this commit address, and how did that need arise or get noticed?

niksu · 2016-07-13T20:11:23Z

Pax.sln

@@ -0,0 +1,59 @@
+


Nice, this patch seems to target this issue

This was an accidental add. I will remove it from this PR - I think it would be better to add it separately without all my user-specific stuff and change/remove build.sh at the same time.

In case you haven't used it before, you might want to experiment with
git's "cherrypick" feature to tidy up commits.

On 2016-07-14 10:57, TMVector wrote:

In Pax.sln [1]:

@@ -0,0 +1,59 @@
+

This was an accidental add. I will remove it from this PR - I think it
would be better to add it separately without all my user-specific
stuff and change/remove build.sh at the same time.

You are receiving this because you commented.
Reply to this email directly, view it on GitHub [2], or mute the
thread [3].

Links:

[1] #9 (comment)
[2]
https://github.com/niksu/pax/pull/9/files/7f29135a9261e8b25f7b9c3ec6cc9f4b3b0add3a..f7ac43cbd665a50aff815bdb5eb1eb81e6b9d99b#r70779301
[3]
https://github.com/notifications/unsubscribe/ABDTTg7gHDpESySYbNfbwKf4oaKkQvjuks5qVgf5gaJpZM4JLXUX

http://www.cl.cam.ac.uk/~ns441/

What's your opinion on changing commits in this PR? I think the comments on diffs would become orphaned?

I agree that it's useful to preserve history. I wonder what "best
practices" there are for doing this. The best idea I could come up with
is to branch this PR into a different PR and carry out the cherrypicking
there, and reference the second PR from the first PR, and close the
first PR without merging, and continue developing the second PR.

On 2016-07-14 11:21, TMVector wrote:

In Pax.sln [1]:

@@ -0,0 +1,59 @@
+

What's your opinion on changing commits in this PR? I think the
comments on diffs would become orphaned?

You are receiving this because you commented.
Reply to this email directly, view it on GitHub [2], or mute the
thread [3].

Links:

[1] #9 (comment)
[2]
https://github.com/niksu/pax/pull/9/files/7f29135a9261e8b25f7b9c3ec6cc9f4b3b0add3a..f7ac43cbd665a50aff815bdb5eb1eb81e6b9d99b#r70782326
[3]
https://github.com/notifications/unsubscribe/ABDTTslJkLs2j3Kzo40WL2X0FeFPJegOks5qVg2zgaJpZM4JLXUX

http://www.cl.cam.ac.uk/~ns441/

TMVector · 2016-07-13T20:22:58Z

I wanted to add more configurable options for NAT, and it wasn't much more effort. I think it was for the timeout originally.

I wanted to provide more versatile config deserialisation, but I thought it probably best done separately.

niksu · 2016-07-13T20:25:46Z

examples/nat_wiring.json

@@ -7,6 +7,7 @@
        "next_outside_hop_mac" : "00:00:00:00:00:09",
        "tcp_start_port" : "35000",
        "udp_start_port" : "35000",
+        "tcp_time_wait" : "00:00:05",


Out of interest have you tried experimenting with larger timeouts and large numbers of connections? It would be interesting to see how the system's behaviour changes (degrades) under load.

Not yet, but I plan to add scripted performance tests for throughput and timing. Obviously these are limited when run in Mininet, but would certainly be more intense than the current tests.

niksu · 2016-07-13T20:47:33Z

examples/Nat/TcpState.cs

+    public bool ClosedFromOutside { get { return OutInConnection == TcpDirectionalState.None || OutInConnection == TcpDirectionalState.FinAck; } }
+    public bool CanBeClosed { get { return ClosedFromInside && ClosedFromOutside; } }
+
+    public void UpdateState(TcpPacket packet, bool packetFromInside)


What happens if we receive a repeated Syn travelling in the same direction? (From inside to out, say) Is the machine reset? That is, can evil person (call them E) break user A's connection to outside server B, when E sends the NAT a Syn addressed to B (and spoofed to appear to come from A)?

niksu · 2016-07-13T21:17:44Z

Thanks for the PR, it systematises the NAT implementation and makes various improvements. I've made a first pass through the code, looking at each commit.
I very much like the Scapy-based testing you're developing in parallel to the functionality.
You also mentioned using iperf earlier in the thread, do you currently run that from inside mininet, generating traffic going from one node to another?

TMVector · 2016-07-15T18:16:14Z

This PR is being closed as #12 replaces it.

TMVector added 14 commits July 11, 2016 15:10

Add support for optional constuctor parameters

cca28fc

Add limited support for Nullables to better support optional parameters

56a113a

Was previously removed, but only in part for some reason.

Remove Nested_NAT

aa3496b

No longer needed because constructor parameters can be provided from config

Add tests using Scapy

7f29135

Tests for connection timeout and connection close. Using Scapy instead of netcat or other so that the OS doesn't keep us from using the same port because of TIME_WAIT on closed connections.

Add xml comments

26b95a1

Update comment to note we don't yet handle RST

5df8ee3

Add licence & copyright info at top of files

52b64d2

Add note about handling RST packets

a844223

Add TCP TIME_WAIT support

5592cd2

Fix exitcode for xterm in tests

f115be2

When running commands in xterm, the exitcode is that of xterm, not the command. Use named pipes to fix this while still displaying output in separate windows for the user.

Merge remote-tracking branch 'refs/remotes/upstream/master'

6f58250

Conflicts: Paxifax.cs examples/NAT.cs examples/Test.cs

Fix nat test #2 and add option to leave terminals open

d324c1f

The second scapy test was not failing when it should. Fixed to make sure we are actually testing the transmission of packets after a Fin close.

Make TIME_WAIT configurable

d648c71

And fix bug with the TCP state - was specifying the wrong direction to the state transition function. Tests should all pass now

TMVector force-pushed the extend-nat branch from 34e260c to d648c71 Compare July 13, 2016 15:20

Fix UDP checksums

1d5f6ad

niksu reviewed Jul 13, 2016
View reviewed changes

TMVector mentioned this pull request Jul 15, 2016

Refactor and extend NAT with UDP support #12

Merged

TMVector closed this Jul 15, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and extend NAT with UDP support #9

Refactor and extend NAT with UDP support #9

TMVector commented Jul 13, 2016

TMVector commented Jul 13, 2016

niksu commented Jul 13, 2016

Links:

TMVector commented Jul 13, 2016 •

edited

Loading

niksu commented Jul 13, 2016

niksu Jul 13, 2016

TMVector Jul 14, 2016

niksu Jul 14, 2016

TMVector Jul 14, 2016

niksu Jul 15, 2016

TMVector commented Jul 13, 2016 •

edited

Loading

niksu Jul 13, 2016

TMVector Jul 14, 2016

niksu Jul 13, 2016

niksu commented Jul 13, 2016

TMVector commented Jul 15, 2016

Refactor and extend NAT with UDP support #9

Refactor and extend NAT with UDP support #9

Conversation

TMVector commented Jul 13, 2016

TMVector commented Jul 13, 2016

niksu commented Jul 13, 2016

Links:

TMVector commented Jul 13, 2016 • edited Loading

niksu commented Jul 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Links:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Links:

TMVector commented Jul 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niksu commented Jul 13, 2016

TMVector commented Jul 15, 2016

TMVector commented Jul 13, 2016 •

edited

Loading

TMVector commented Jul 13, 2016 •

edited

Loading