Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port randomization fixes #239

Merged
merged 12 commits into from
Oct 7, 2023
Merged

Port randomization fixes #239

merged 12 commits into from
Oct 7, 2023

Conversation

jmwample
Copy link
Member

@jmwample jmwample commented Oct 4, 2023

There is an issue wrt. destination port randomization that conflates subtly different booleans controlling whether port randomization is enabled for a given connection.

  • Port randomization consistency for Unidirectional Registration - (w/ and w/o phantom support for randomization)
  • Port randomization consistency for Bidirectional Registration - (w/ and w/o phantom support for randomization)
    • it is not clear that the proper param object is being forwarded to the station to ensure that the client and station always agree on the expected destination port.
  • Copy Params from pointer instance instead of using provided pointer instance in set Params otherwise issues with values changing between connections causes unpredictable behavior.
  • XX IPv6 Phantom Selection bug that allows selection of IPv4 phantom when the number of IPv4 phantoms is sufficiently small.
    • XX changing this requires a new client Library version as it changes the phantom selection and we will need to support the old version still in order to be backward compatible.

refraction-networking/gotapdance#147

@jmwample
Copy link
Member Author

jmwample commented Oct 6, 2023

Sessions using bidirectional registrars do have consistency between the client and the station wrt port randomization as regprocessor.go -- which handles the intake of bidirectional registrations, and generating the RegistrationResponse that is both sent to the client and attached to the registration forwarded to the -- handles port randomization properly. When a bidirectional registrar is used the stations use the port in the attached registration response which the client is supposed to use as well.

When picking the destination port the regprocesser uses the station side transport interface and provides the transport parameters which is where the client indicates whether port randomization is enabled or not.

@jmwample
Copy link
Member Author

jmwample commented Oct 6, 2023

It turns out that making a copy of the params shared on init was not enough for item 3 because subsequent dials would still overwrite parameters within the Transport object if it is re-used, which we allow it to be.

The real solution was to add a public Parameters that is set by the user, which should be treated as immutable, unless it is explicitly set again using SetParams(). And then use an mutable internal parameter object that deep clones the user parameters on each dial. In this way internal state only persists for one session and each dial will start with consistent parameters matching caller expectations.

For things like bidirectional registration where it is necessary to overwrite some parameters within the session the function SetSessionParams() is used.

@jmwample
Copy link
Member Author

jmwample commented Oct 6, 2023

In general I think this maybe motivates a different interface for Transports more similar to Tor's pluggable transports where you have a Transport, but then when dial is called a function like transport.ClientFactory() is called to build a new instance of the implemented protocol with/from the original immutable config.

@jmwample
Copy link
Member Author

jmwample commented Oct 6, 2023

The 4th todo is not directly related to these fixes and should be handled separately as it is not required to get the fixed port randomization rolled. See #240

@jmwample jmwample requested a review from mingyech October 6, 2023 00:32
Copy link
Member

@mingyech mingyech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and port randomization works with the combinations of transports and registrars

Copy link
Member

@mingyech mingyech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and port randomization works with the combinations of transports and registrars

@jmwample jmwample merged commit 10023d9 into master Oct 7, 2023
11 checks passed
@jmwample jmwample deleted the port-rand-fix branch October 7, 2023 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants