router: STUN implementation #4740
Conversation
…r on same port as SCION dataplane
- less overhead - less modification of STUN library - no disadvantages expected
|
This change is |
There was a problem hiding this comment.
Sounds mostly good. I very much appreciated that you implemented the STUN detection as an error case and handle it on the Slow path. That makes the extra cost zero. Maybe you could consider also gating the feature with a command-line flag. I can imagine cases where users do not want to host the stun service, just to reduce attack surfaces no matter how small. Doc-wise, is the STUN/SCION headers overlap explained anywhere? The fact the STUN packet looks like a SCION packet with Next Header type 2 and Header length 1, needs to be memorialized somewhere if it hasn't been, and your code should refer to it, so that we never break that invariant accidentally.
Reviewed 15 of 15 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jdslab)
demo/stun/test-client/main.go line 1 at r1 (raw file):
package main
Copyright? License?
demo/stun/test-server/main.go line 1 at r1 (raw file):
package main
Ditto
demo/stun/test.py line 4 at r1 (raw file):
# Copyright 2024 ETH Zurich #
- It's new code, right?
demo/stun/test.py line 114 at r1 (raw file):
def _run(self): time.sleep(10) # wait for everything to start up
How reliable is that time guess? Can we probe readiness instead? It is OK for a test take 10 seconds or even much more, but provided we don't have too many of those.
| if err != nil { | ||
| return serrors.Wrap("processing STUN packet", err) | ||
| } | ||
| resp := stun.Response(txid, pkt.SrcAddr.AddrPort()) |
There was a problem hiding this comment.
We might need to make some changes to support this nowadays. The connected links do not set the packet's srcAddr because it is (until now) not used for anything. We might need to change that. Must the address really be repeated in the stun packet?
There was a problem hiding this comment.
Unfortunately, yes. Sending back the underlay source address (as seen by the router) is at the core of how STUN works:
"The STUN server copies that source transport address into an XOR-MAPPED-ADDRESS attribute in the STUN Binding response and sends the Binding response back to the STUN client."
There was a problem hiding this comment.
Duh! I'm stupid. Yeah, the address must make it back to the client somehow. Should we move the stun server directly into the underlay? After all, IP NAT is an underlay business. It would have no reason to exist without an IP-based underlay at least. Either way. I can re-expose the src to the upper layer if need be.
There was a problem hiding this comment.
Right, the underlay seems like the logical place for the STUN functionality, provided we can also achieve some degree of reuse across various UDP/IP-based underlay implementations. I'm not yet deep enough in the details of the new underlay structure to say whether this would work without hitting additional issues, though.
Modification to border router dataplane to process STUN requests, as described in the design document and discussion.