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

Add function to enable localhost communication only from env var #190

Merged
merged 5 commits into from
Oct 18, 2019

Conversation

BMarchi
Copy link
Contributor

@BMarchi BMarchi commented Oct 11, 2019

PR to implement network traffic restriction for localhost. There are no test yet since this is a mock open for implementation discussion. FastRTPS implementation

  • Linux Build Status
  • Arch Build Status
  • Windows Build Status
  • OSX Build Status

Signed-off-by: Brian Ezequiel Marchi <brian.marchi65@gmail.com>
Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

How about to only offer a C function bool use_localhost_only()? That satisfies the current use case. Also in case we want to extend the semantic in the future we can simply offer another function to return more complex information (which we haven't determined yet how they look and if things like a list of hostnames can be implemented across RMW impl.).

rmw/include/rmw/host.h Outdated Show resolved Hide resolved
rmw/src/host.c Outdated Show resolved Hide resolved
rmw/src/host.c Outdated Show resolved Hide resolved
rmw/include/rmw/host.h Outdated Show resolved Hide resolved
@BMarchi
Copy link
Contributor Author

BMarchi commented Oct 11, 2019

I wanted a third return option, that's why I didn't use the bool one, so proper behavior can be done if the variable has a wrong value. I guess I can remove the possibility to return the host list without parsing and just return the code. If we want list parsing for the future then we add another function as you said. Does it sound good for you?

@dirk-thomas
Copy link
Member

I guess I can remove the possibility to return the host list without parsing and just return the code.

Sounds good to me.

#define RMW_ENV_VAR_NOT_DEFINED_OR_EMPTY 0
#define RMW_LOCAL_HOST_ENABLED 1
#define RMW_INVALID_ALLOWED_HOSTS 2

I am not sure how to name / value the three return codes for something like use_localhost_only though.

Instead of LOCAL_HOST_ENABLED I would propose LOCALHOST_ONLY since it not only states that localhost is enabled but that it is the only enabled target.

I would be worried that callers interpret the numeric return value as a boolean. So if LOCALHOST_ONLY is non-zero I would wish the two other return codes would both be zero (which is obviously not an option).

Maybe a signature like rc use_localhost_only(bool *) would be more natural to use? The rc is OK / 0 or non-zero / UNSUPPORTED_VALUE. And the bool gets set either way - in the case of an unsupported value or the env var not set at all the default value of our choice.

[All this hops are only "necessary" because we try to use a env var with a wider value space than we are comfortable to actually specify atm. I still think just using an env var ROS_ENABLE_NETWORK is the much cleaner choice for the time being - with a simple boolean getter function - no extras.]

@BMarchi
Copy link
Contributor Author

BMarchi commented Oct 15, 2019

After reading the discussion in the issue, it seems that people by majority prefers the boolean rather than a list of hosts. So I'll make the changes accordingly!

Signed-off-by: Brian Ezequiel Marchi <brian.marchi65@gmail.com>
rmw/CMakeLists.txt Outdated Show resolved Hide resolved
rmw/include/rmw/host.h Outdated Show resolved Hide resolved
rmw/include/rmw/host.h Outdated Show resolved Hide resolved
rmw/src/host.c Outdated Show resolved Hide resolved
Signed-off-by: Brian Ezequiel Marchi <brian.marchi65@gmail.com>
rmw/include/rmw/host.h Outdated Show resolved Hide resolved
Signed-off-by: Brian Ezequiel Marchi <brian.marchi65@gmail.com>
@dirk-thomas
Copy link
Member

With the RMW API exposing the flag as an argument of the create node function (same as the domain ID) I would argue this function should actually be implemented in rcl instead (same as where the domain id env var is being checked).

Signed-off-by: Brian Ezequiel Marchi <brian.marchi65@gmail.com>
@BMarchi BMarchi changed the title Add function to check for allowed hosts from env var Add function to enable localhost communication only from env var Oct 18, 2019
@hidmic hidmic merged commit 3850771 into master Oct 18, 2019
@delete-merged-branch delete-merged-branch bot deleted the BMarchi/restrict_traffic_to_local_host branch October 18, 2019 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants