implement ROS_IP=localhost behavior for roscpp, and for outbound rospy connections #452

codebot · 2014-06-29T05:39:23Z

The wiki doc for ROS_IP: http://wiki.ros.org/ROS/EnvironmentVariables says that setting ROS_IP=localhost will "prevent remote components from being able to talk to your local component." This can be useful in some settings. However, this behavior was previously only implemented for inbound rospy topic connections and services. This pull request implements it for inbound and outbound roscpp connections, and for outbound rospy connections.

…onnections to other hosts

…nings to print out

…ocalhost is set, and only connect to local interfaces if so. this is the behavior implied by the ROS_IP documentation, which was previously only implemented for inbound connections.

ros-pull-request-builder · 2014-06-29T12:22:53Z

Test passed.
Refer to this link for build results: http://jenkins.ros.org/job/_pull_request-indigo-ros_comm/36/

gerkey · 2014-06-29T18:26:27Z

Anywhere that we check for ROS_IP=='localhost', we should also check for ROS_HOSTNAME=='localhost'. They're used interchangeably (which is confusing, but then it's always been confusing to support two variables for this purpose).

gerkey · 2014-06-29T18:31:55Z

clients/roscpp/src/libros/transport/transport_tcp.cpp

+  {
+    // we must verify that the requested connection is to localhost
+    char our_hostname[256] = {0};
+    gethostname(our_hostname, sizeof(our_hostname)-1);


What about the case where we're asked to connect to a node via one of our own non-loopback IP addresses? E.g., we're in 'localhost' mode and we hear about a publisher at '192.168.1.100', which is our own address? That connection should be allowed. I think that we need to iterate over all of our own IP addresses, plus our hostname (maybe fully-qualified and not?), plus the special cases of 'localhost', '127.*', etc. Basically, we need to match on any host/address string that legitimately matches our machine.

Looking at the Python version, it seems that we need some form of rosgraph.network.is_local_address().

Same for the UDP case below.

dirk-thomas · 2014-06-29T18:42:27Z

According to the docs (http://wiki.ros.org/ROS/EnvironmentVariables#ROS_IP.2BAC8-ROS_HOSTNAME) ROS_IP should only be set to an IP address:

Use ROS_IP if you are specifying an IP address, and ROS_HOSTNAME if you are specifying a host name.

So I don't think that setting ROS_IP=localhost is valid. The patch should only check the ROS_HOSTNAME instead.

codebot · 2014-06-29T20:06:15Z

OK thanks for the feedback. I thought that localhost was being treated as a
"magic" IP string in rospy, but I agree it is more logical to implement it
for ROS_HOSTNAME as well (and to check all interface addresses, like rospy
does). I will update the PR to incorporate all of this feedback.
On Jun 29, 2014 11:42 AM, "Dirk Thomas" notifications@github.com wrote:

According to the docs (
http://wiki.ros.org/ROS/EnvironmentVariables#ROS_IP.2BAC8-ROS_HOSTNAME)
ROS_IP should only be set to an IP address:

Use ROS_IP if you are specifying an IP address, and ROS_HOSTNAME if you
are specifying a host name.

So I don't think that setting ROS_IP=localhost is valid. The patch should
only check the ROS_HOSTNAME instead.

—
Reply to this email directly or view it on GitHub
#452 (comment).

…ache them to compare against incoming connection requests.

codebot · 2014-06-29T22:55:02Z

not sure what the Jenkins failures mean, but anyway I think that I have incorporated the feedback and it is not quite as ugly as my first attempt. I added a new file, transport.cpp, which factors all of this into one place. The local hostname/addresses are cached in the Transport class and spun through when needed.

dirk-thomas · 2014-06-30T19:07:20Z

@ros-pull-request-builder

ros-pull-request-builder · 2014-06-30T19:25:06Z

Test passed.
Refer to this link for build results: http://jenkins.ros.org/job/_pull_request-indigo-ros_comm/42/

dirk-thomas · 2014-06-30T20:50:34Z

clients/roscpp/src/libros/transport/transport.cpp

+/*
+ * Software License Agreement (BSD License)
+ *
+ *  Copyright (c) 2014, Open Source Robotics Foundation


Open Source Robotics Foundation, Inc.

dirk-thomas · 2014-06-30T20:54:40Z

clients/roscpp/src/libros/transport/transport.cpp

+      return true; // hooray
+  }
+  ROS_WARN("ROS_HOSTNAME / ROS_IP is set to only allow local connections, so "
+           "a requested connection to %s is being rejected.", host.c_str());


Please put single quotes around the %s to make it more readable:

.. connection to '%s' is being ...

Thank you; these changes are now incorporated into the PR.

On Mon, Jun 30, 2014 at 1:54 PM, Dirk Thomas notifications@github.com
wrote:

In clients/roscpp/src/libros/transport/transport.cpp:

+bool Transport::isHostAllowed(const std::string &host)
+{

if (!only_localhost_allowed_)

return true; // doesn't matter; we'll connect to anybody

if (host.length() >= 4 && host.substr(0, 4) == std::string("127."))

return true; // ipv4 localhost

// now, loop through the list of valid hostnames and see if we find it

for (std::vectorstd::string::iterator it = allowed_hosts_.begin();

it != allowed_hosts_.end(); ++it)

{

if (host == *it)

return true; // hooray

}

ROS_WARN("ROS_HOSTNAME / ROS_IP is set to only allow local connections, so "

"a requested connection to %s is being rejected.", host.c_str());

Please put single quotes around the %s to make it more readable:

.. connection to '%s' is being ...

—
Reply to this email directly or view it on GitHub
https://github.com/ros/ros_comm/pull/452/files#r14375058.

…ix license company name

dirk-thomas · 2014-06-30T21:25:29Z

clients/roscpp/include/ros/transport/transport.h

 };

 }

 #endif // ROSCPP_TRANSPORT_H
+


The file already has a trailing newline. Please remove the additional one.

done

On Mon, Jun 30, 2014 at 2:25 PM, Dirk Thomas notifications@github.com
wrote:

In clients/roscpp/include/ros/transport/transport.h:

};

}

#endif // ROSCPP_TRANSPORT_H
+

The file already has a trailing newline. Please remove the additional one.

—
Reply to this email directly or view it on GitHub
https://github.com/ros/ros_comm/pull/452/files#r14376694.

…x, not 'localhost'

dirk-thomas · 2014-07-01T05:20:05Z

@ros-pull-request-builder

ros-pull-request-builder · 2014-07-01T05:44:10Z

Test passed.
Refer to this link for build results: http://jenkins.ros.org/job/_pull_request-indigo-ros_comm/47/

dirk-thomas · 2014-07-09T17:08:20Z

clients/roscpp/src/libros/transport/transport.cpp

+  if (ros_hostname_env && !strcmp(ros_hostname_env, "localhost"))
+    only_localhost_allowed_ = true;
+  else if (ros_ip_env && !strncmp(ros_ip_env, "127.", 4))
+    only_localhost_allowed_ = true;


This line also needs to work with IPv6 addresses.

dirk-thomas · 2014-07-09T18:59:18Z

clients/rospy/src/rospy/impl/tcpros_base.py

@@ -518,6 +518,16 @@ def connect(self, dest_addr, dest_port, endpoint_id, timeout=None):
        @type  timeout: float
        @raise TransportInitError: if unable to create connection
        """
+        # first make sure that if ROS_IP=localhost, we will not attempt


Sorry, for bother about more details... but the comment should refer to ROS_HOSTNAME.

dirk-thomas · 2014-07-10T00:15:58Z

@ros-pull-request-builder

ros-pull-request-builder · 2014-07-10T00:38:41Z

Test failed.
Refer to this link for build results: http://jenkins.ros.org/job/_pull_request-indigo-ros_comm/62/

Update: this failure is due to a flaky unit test and I will therefore ignore it and still merge the patch.

wjwwood · 2014-07-10T19:26:44Z

+1 for Indigo

dirk-thomas · 2014-07-10T19:39:23Z

Squashed and merged in 8713a1e

dirk-thomas · 2014-07-10T19:42:12Z

@CodeBot9000 Can you please update the documentation in the wiki to clarify the previous behavior as well as the new behavior as of Indigo (and post the link to the wiki changes here)?

ros-pull-request-builder · 2014-07-10T19:59:26Z

Test passed.
Refer to this link for build results: http://jenkins.ros.org/job/_pull_request-indigo-ros_comm/66/

codebot added 7 commits June 27, 2014 17:27

check ROS_IP for localhost and do the right thing when binding

8bfa94f

fix byte ordering in INADDR_LOOPBACK for ipv4

af94ba8

ensure that when ROS_IP is localhost, nodes will not start outbound c…

2529a4f

…onnections to other hosts

also verify host for outbound UDPROS connections

3c6a4cb

tweak rospy warning logic so that ROS_IP=localhost does not cause war…

3d41e2c

…nings to print out

allow numeric hostnames for localhost on localhost-only connections

f2fc36f

rospy tweak to cause tcpros to check outbound connections if ROS_IP=l…

3bfaf6a

…ocalhost is set, and only connect to local interfaces if so. this is the behavior implied by the ROS_IP documentation, which was previously only implemented for inbound connections.

gerkey reviewed Jun 29, 2014
View reviewed changes

codebot added 3 commits June 29, 2014 15:33

refactor to reduce repetition. iterate through local interfaces and c…

18ee16c

…ache them to compare against incoming connection requests.

identify ipv4 127.* stub to imply local-only connections

0097b24

refactor bind code to reduce repetition

0b806b6

dirk-thomas reviewed Jun 30, 2014
View reviewed changes

only allow ROS_HOSTNAME (not ROS_IP) to be set to localhost in rospy

67cdd40

dirk-thomas reviewed Jun 30, 2014
View reviewed changes

codebot added 2 commits June 30, 2014 13:58

const correctness for Transport() functions

e4c5a28

finish const correctness of Tranport() function implementation, and f…

76c478f

…ix license company name

dirk-thomas reviewed Jun 30, 2014
View reviewed changes

codebot added 2 commits June 30, 2014 14:36

remove trailing newline

4a29e84

only look for ROS_HOSTNAME=localhost; only look for ROS_IP to be 127.…

3f51fc6

…x, not 'localhost'

dirk-thomas reviewed Jul 9, 2014
View reviewed changes

add ipv6 loopback address check. revert whitespace.

be21c4e

dirk-thomas reviewed Jul 9, 2014
View reviewed changes

fix comment to refer to ROS_HOSTNAME instead of ROS_IP

df99e1f

dirk-thomas added the enhancement label Jul 10, 2014

dirk-thomas closed this Jul 10, 2014

dirk-thomas mentioned this pull request Jul 18, 2014

Fixed bug when ifa_addr is NULL #465

Merged

dirk-thomas mentioned this pull request Mar 4, 2015

Fixed Transport constructor getifaddrs memory leak #570

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement ROS_IP=localhost behavior for roscpp, and for outbound rospy connections #452

implement ROS_IP=localhost behavior for roscpp, and for outbound rospy connections #452

codebot commented Jun 29, 2014

ros-pull-request-builder commented Jun 29, 2014

gerkey commented Jun 29, 2014

gerkey Jun 29, 2014

dirk-thomas commented Jun 29, 2014

codebot commented Jun 29, 2014

codebot commented Jun 29, 2014

dirk-thomas commented Jun 30, 2014

ros-pull-request-builder commented Jun 30, 2014

dirk-thomas Jun 30, 2014

dirk-thomas Jun 30, 2014

codebot Jun 30, 2014

dirk-thomas Jun 30, 2014

codebot Jun 30, 2014

dirk-thomas commented Jul 1, 2014

ros-pull-request-builder commented Jul 1, 2014

dirk-thomas Jul 9, 2014

dirk-thomas Jul 9, 2014

dirk-thomas commented Jul 10, 2014

ros-pull-request-builder commented Jul 10, 2014

wjwwood commented Jul 10, 2014

dirk-thomas commented Jul 10, 2014

dirk-thomas commented Jul 10, 2014

ros-pull-request-builder commented Jul 10, 2014

implement ROS_IP=localhost behavior for roscpp, and for outbound rospy connections #452

implement ROS_IP=localhost behavior for roscpp, and for outbound rospy connections #452

Conversation

codebot commented Jun 29, 2014

ros-pull-request-builder commented Jun 29, 2014

gerkey commented Jun 29, 2014

Choose a reason for hiding this comment

dirk-thomas commented Jun 29, 2014

codebot commented Jun 29, 2014

codebot commented Jun 29, 2014

dirk-thomas commented Jun 30, 2014

ros-pull-request-builder commented Jun 30, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirk-thomas commented Jul 1, 2014

ros-pull-request-builder commented Jul 1, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirk-thomas commented Jul 10, 2014

ros-pull-request-builder commented Jul 10, 2014

wjwwood commented Jul 10, 2014

dirk-thomas commented Jul 10, 2014

dirk-thomas commented Jul 10, 2014

ros-pull-request-builder commented Jul 10, 2014