Re: Review Request 58224: Optionally verify the source IP address for libprocess messages.

2017-06-01 Thread Benjamin Mahler 


> On May 31, 2017, 8:14 p.m., Andrew Schwartzmeyer wrote:
> > Does not build on Windows. Please ensure that the tests include _at least_ 
> > a compilation on Windows before committing. The build is broken now.

I don't think we can expect users to compile on windows, OS X and linux for all 
of their patches. We can notice reviewbot telling us the windows build is 
broken. But even then, without a submit queue gating commits, we're going to 
accidentally break the build from time to time, we should just be sure to fix 
quickly.

I think here we were operating with good intentions, since it's highly 
surprising that this breaks the windows build due to PCH, we couldn't have 
anticipated that there was something windows specific here. I would like to 
understand why the build broke, it seems weird to me that we have to change a 
.cpp static global name.


- Benjamin


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58224/#review176508
---


On May 10, 2017, 6:06 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58224/
> ---
> 
> (Updated May 10, 2017, 6:06 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7401
> https://issues.apache.org/jira/browse/MESOS-7401
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In general, libprocess is unable to validate that a peer
> is a legitimate owner of the UPID it claims in a libprocess
> message. This change adds a check that the IP address in the
> UPID matches the peer address. This makes spoofing the UPID
> harder (eg. to send authenticated messages), but also breaks
> some legitimate configurations, particularly on multihomed
> hosts.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 96ce7dbc486a2f1d55d2238a8a102bf024b12b1c 
> 
> 
> Diff: https://reviews.apache.org/r/58224/diff/11/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25). Light manual testing.
> 
> With LIBPROCESS_require_peer_address_ip_match=true, all Mesos tests pass 
> except ``ExamplesTest.DiskFullFramework``, however enabling this will 
> definitely break some libprocess APIs (though not in the way that Mesos uses 
> them) and legitimate multi-homed configurations. Note that setting 
> LIBPROCESS_ip=127.0.0.1 makes you multihomed for this purpose, which is why 
> ``ExamplesTest.DiskFullFramework`` breaks.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 58224: Optionally verify the source IP address for libprocess messages.

2017-05-31 Thread Andrew Schwartzmeyer 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58224/#review176508
---



Does not build on Windows. Please ensure that the tests include _at least_ a 
compilation on Windows before committing. The build is broken now.

- Andrew Schwartzmeyer


On May 10, 2017, 6:06 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58224/
> ---
> 
> (Updated May 10, 2017, 6:06 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7401
> https://issues.apache.org/jira/browse/MESOS-7401
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In general, libprocess is unable to validate that a peer
> is a legitimate owner of the UPID it claims in a libprocess
> message. This change adds a check that the IP address in the
> UPID matches the peer address. This makes spoofing the UPID
> harder (eg. to send authenticated messages), but also breaks
> some legitimate configurations, particularly on multihomed
> hosts.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 96ce7dbc486a2f1d55d2238a8a102bf024b12b1c 
> 
> 
> Diff: https://reviews.apache.org/r/58224/diff/11/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25). Light manual testing.
> 
> With LIBPROCESS_require_peer_address_ip_match=true, all Mesos tests pass 
> except ``ExamplesTest.DiskFullFramework``, however enabling this will 
> definitely break some libprocess APIs (though not in the way that Mesos uses 
> them) and legitimate multi-homed configurations. Note that setting 
> LIBPROCESS_ip=127.0.0.1 makes you multihomed for this purpose, which is why 
> ``ExamplesTest.DiskFullFramework`` breaks.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 58224: Optionally verify the source IP address for libprocess messages.

2017-05-30 Thread Benjamin Mahler 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58224/#review176360
---


Fix it, then Ship it!





3rdparty/libprocess/src/process.cpp
Lines 217 (patched)


Do we allow "1" as the boolean flag value? Maybe let's just say "if set" to 
avoid confusion about this being a number flag instead of a boolean flag?



3rdparty/libprocess/src/process.cpp
Line 955 (original), 970-971 (patched)


Why change this in this patch?



3rdparty/libprocess/src/process.cpp
Lines 2877-2878 (patched)


How about a CHECK_SOME of request->client?



3rdparty/libprocess/src/process.cpp
Lines 2877-2878 (patched)


Could use some comments here to make it more obvious w.r.t. to address vs 
ip address:

```
// If the client address is not an IP address (e.g. coming
// from a domain socket), we also reject the message.
Try client_ip_address =
  network::convert(request->client.get());
```


- Benjamin Mahler


On May 10, 2017, 6:06 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58224/
> ---
> 
> (Updated May 10, 2017, 6:06 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7401
> https://issues.apache.org/jira/browse/MESOS-7401
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In general, libprocess is unable to validate that a peer
> is a legitimate owner of the UPID it claims in a libprocess
> message. This change adds a check that the IP address in the
> UPID matches the peer address. This makes spoofing the UPID
> harder (eg. to send authenticated messages), but also breaks
> some legitimate configurations, particularly on multihomed
> hosts.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 96ce7dbc486a2f1d55d2238a8a102bf024b12b1c 
> 
> 
> Diff: https://reviews.apache.org/r/58224/diff/11/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25). Light manual testing.
> 
> With LIBPROCESS_require_peer_address_ip_match=true, all Mesos tests pass 
> except ``ExamplesTest.DiskFullFramework``, however enabling this will 
> definitely break some libprocess APIs (though not in the way that Mesos uses 
> them) and legitimate multi-homed configurations. Note that setting 
> LIBPROCESS_ip=127.0.0.1 makes you multihomed for this purpose, which is why 
> ``ExamplesTest.DiskFullFramework`` breaks.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 58224: Optionally verify the source IP address for libprocess messages.

2017-05-11 Thread Benjamin Mahler 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58224/#review174754
---



Currently we require getting the peer each time we receive some requests from 
the socket. This patch can use the `Request->client` directly, without needing 
to introduce the session change, without making the performance worse.

Since the performance isn't being made worse, how about we look into the peer 
caching optimization after adding this feature?


3rdparty/libprocess/src/process.cpp
Lines 217-221 (patched)


Can we exactly match the description you added to docs/configuration.md in 
the subsequent patch? Seems nice to have them in sync, especially since it's 
not that long.



3rdparty/libprocess/src/process.cpp
Lines 586 (patched)


Can you make this a pointer? We disallow static non-POD variables.



3rdparty/libprocess/src/process.cpp
Lines 2886-2891 (patched)


At this point, you can look at `request->client` rather than 
`session->peer`. This would avoid the need for introducing session for now, and 
doesn't make the performance worse.

Separately from this feature, we could figure out how to cache the peer to 
avoid getting each each time requests are decoded, in order to improve the 
performance.


- Benjamin Mahler


On May 10, 2017, 6:06 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58224/
> ---
> 
> (Updated May 10, 2017, 6:06 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7401
> https://issues.apache.org/jira/browse/MESOS-7401
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In general, libprocess is unable to validate that a peer
> is a legitimate owner of the UPID it claims in a libprocess
> message. This change adds a check that the IP address in the
> UPID matches the peer address. This makes spoofing the UPID
> harder (eg. to send authenticated messages), but also breaks
> some legitimate configurations, particularly on multihomed
> hosts.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 96ce7dbc486a2f1d55d2238a8a102bf024b12b1c 
> 
> 
> Diff: https://reviews.apache.org/r/58224/diff/9/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25). Light manual testing.
> 
> With LIBPROCESS_require_peer_address_ip_match=true, all Mesos tests pass 
> except ``ExamplesTest.DiskFullFramework``, however enabling this will 
> definitely break some libprocess APIs (though not in the way that Mesos uses 
> them) and legitimate multi-homed configurations. Note that setting 
> LIBPROCESS_ip=127.0.0.1 makes you multihomed for this purpose, which is why 
> ``ExamplesTest.DiskFullFramework`` breaks.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 58224: Optionally verify the source IP address for libprocess messages.

2017-05-10 Thread James Peach 


> On May 2, 2017, 12:34 a.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp
> > Lines 2878-2879 (patched)
> > 
> >
> > We could refer to the flag help for examples?

I updated the text a bit.


> On May 2, 2017, 12:34 a.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp
> > Lines 2883-2884 (patched)
> > 
> >
> > How about:
> > 
> > UPID IP address validation failed: Message from X was sent from IP Y.

This is an improvement, I made this change.


- James


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58224/#review173536
---


On May 10, 2017, 6:06 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58224/
> ---
> 
> (Updated May 10, 2017, 6:06 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7401
> https://issues.apache.org/jira/browse/MESOS-7401
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In general, libprocess is unable to validate that a peer
> is a legitimate owner of the UPID it claims in a libprocess
> message. This change adds a check that the IP address in the
> UPID matches the peer address. This makes spoofing the UPID
> harder (eg. to send authenticated messages), but also breaks
> some legitimate configurations, particularly on multihomed
> hosts.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 96ce7dbc486a2f1d55d2238a8a102bf024b12b1c 
> 
> 
> Diff: https://reviews.apache.org/r/58224/diff/8/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25). Light manual testing.
> 
> With LIBPROCESS_require_peer_address_ip_match=true, all Mesos tests pass 
> except ``ExamplesTest.DiskFullFramework``, however enabling this will 
> definitely break some libprocess APIs (though not in the way that Mesos uses 
> them) and legitimate multi-homed configurations. Note that setting 
> LIBPROCESS_ip=127.0.0.1 makes you multihomed for this purpose, which is why 
> ``ExamplesTest.DiskFullFramework`` breaks.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 58224: Optionally verify the source IP address for libprocess messages.

2017-05-10 Thread James Peach 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58224/
---

(Updated May 10, 2017, 6:06 p.m.)


Review request for mesos and Benjamin Mahler.


Bugs: MESOS-7401
https://issues.apache.org/jira/browse/MESOS-7401


Repository: mesos


Description
---

In general, libprocess is unable to validate that a peer
is a legitimate owner of the UPID it claims in a libprocess
message. This change adds a check that the IP address in the
UPID matches the peer address. This makes spoofing the UPID
harder (eg. to send authenticated messages), but also breaks
some legitimate configurations, particularly on multihomed
hosts.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp 96ce7dbc486a2f1d55d2238a8a102bf024b12b1c 


Diff: https://reviews.apache.org/r/58224/diff/8/

Changes: https://reviews.apache.org/r/58224/diff/7-8/


Testing
---

make check (Fedora 25). Light manual testing.

With LIBPROCESS_require_peer_address_ip_match=true, all Mesos tests pass except 
``ExamplesTest.DiskFullFramework``, however enabling this will definitely break 
some libprocess APIs (though not in the way that Mesos uses them) and 
legitimate multi-homed configurations. Note that setting 
LIBPROCESS_ip=127.0.0.1 makes you multihomed for this purpose, which is why 
``ExamplesTest.DiskFullFramework`` breaks.


Thanks,

James Peach

Re: Review Request 58224: Optionally verify the source IP address for libprocess messages.

2017-05-09 Thread James Peach 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58224/#review174344
---




3rdparty/libprocess/src/process.cpp
Lines 955 (patched)


This is a bug. If we hit this, we will not re-arm the accept event and 
won't accept any new connections.


- James Peach


On May 4, 2017, 3:52 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58224/
> ---
> 
> (Updated May 4, 2017, 3:52 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7401
> https://issues.apache.org/jira/browse/MESOS-7401
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In general, libprocess is unable to validate that a peer
> is a legitimate owner of the UPID it claims in a libprocess
> message. This change adds a check that the IP address in the
> UPID matches the peer address. This makes spoofing the UPID
> harder (eg. to send authenticated messages), but also breaks
> some legitimate configurations, particularly on multihomed
> hosts.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> f5b666f894215cb1861c244c94b382e0739bc5c9 
>   docs/configuration.md 79cada3c9403881bf257d653f721d32e55607a7f 
> 
> 
> Diff: https://reviews.apache.org/r/58224/diff/7/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25). Light manual testing.
> 
> With LIBPROCESS_require_peer_address_ip_match=true, all Mesos tests pass 
> except ``ExamplesTest.DiskFullFramework``, however enabling this will 
> definitely break some libprocess APIs (though not in the way that Mesos uses 
> them) and legitimate multi-homed configurations. Note that setting 
> LIBPROCESS_ip=127.0.0.1 makes you multihomed for this purpose, which is why 
> ``ExamplesTest.DiskFullFramework`` breaks.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 58224: Optionally verify the source IP address for libprocess messages.

2017-05-04 Thread James Peach 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58224/
---

(Updated May 4, 2017, 3:52 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Added documentation.


Bugs: MESOS-7401
https://issues.apache.org/jira/browse/MESOS-7401


Repository: mesos


Description
---

In general, libprocess is unable to validate that a peer
is a legitimate owner of the UPID it claims in a libprocess
message. This change adds a check that the IP address in the
UPID matches the peer address. This makes spoofing the UPID
harder (eg. to send authenticated messages), but also breaks
some legitimate configurations, particularly on multihomed
hosts.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp f5b666f894215cb1861c244c94b382e0739bc5c9 
  docs/configuration.md 79cada3c9403881bf257d653f721d32e55607a7f 


Diff: https://reviews.apache.org/r/58224/diff/7/

Changes: https://reviews.apache.org/r/58224/diff/6-7/


Testing
---

make check (Fedora 25). Light manual testing.

With LIBPROCESS_require_peer_address_ip_match=true, all Mesos tests pass except 
``ExamplesTest.DiskFullFramework``, however enabling this will definitely break 
some libprocess APIs (though not in the way that Mesos uses them) and 
legitimate multi-homed configurations. Note that setting 
LIBPROCESS_ip=127.0.0.1 makes you multihomed for this purpose, which is why 
``ExamplesTest.DiskFullFramework`` breaks.


Thanks,

James Peach

Re: Review Request 58224: Optionally verify the source IP address for libprocess messages.

2017-05-04 Thread Mesos Reviewbot 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58224/#review173903
---



Patch looks great!

Reviews applied: [58977, 58928, 58224]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On May 4, 2017, 12:31 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58224/
> ---
> 
> (Updated May 4, 2017, 12:31 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7401
> https://issues.apache.org/jira/browse/MESOS-7401
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In general, libprocess is unable to validate that a peer
> is a legitimate owner of the UPID it claims in a libprocess
> message. This change adds a check that the IP address in the
> UPID matches the peer address. This makes spoofing the UPID
> harder (eg. to send authenticated messages), but also breaks
> some legitimate configurations, particularly on multihomed
> hosts.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> f5b666f894215cb1861c244c94b382e0739bc5c9 
> 
> 
> Diff: https://reviews.apache.org/r/58224/diff/6/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25). Light manual testing.
> 
> With LIBPROCESS_require_peer_address_ip_match=true, all Mesos tests pass 
> except ``ExamplesTest.DiskFullFramework``, however enabling this will 
> definitely break some libprocess APIs (though not in the way that Mesos uses 
> them) and legitimate multi-homed configurations. Note that setting 
> LIBPROCESS_ip=127.0.0.1 makes you multihomed for this purpose, which is why 
> ``ExamplesTest.DiskFullFramework`` breaks.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 58224: Optionally verify the source IP address for libprocess messages.

2017-05-03 Thread James Peach 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58224/
---

(Updated May 4, 2017, 12:31 a.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Rebase and make pricess flags static.


Bugs: MESOS-7401
https://issues.apache.org/jira/browse/MESOS-7401


Repository: mesos


Description
---

In general, libprocess is unable to validate that a peer
is a legitimate owner of the UPID it claims in a libprocess
message. This change adds a check that the IP address in the
UPID matches the peer address. This makes spoofing the UPID
harder (eg. to send authenticated messages), but also breaks
some legitimate configurations, particularly on multihomed
hosts.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp f5b666f894215cb1861c244c94b382e0739bc5c9 


Diff: https://reviews.apache.org/r/58224/diff/6/

Changes: https://reviews.apache.org/r/58224/diff/5-6/


Testing (updated)
---

make check (Fedora 25). Light manual testing.

With LIBPROCESS_require_peer_address_ip_match=true, all Mesos tests pass except 
``ExamplesTest.DiskFullFramework``, however enabling this will definitely break 
some libprocess APIs (though not in the way that Mesos uses them) and 
legitimate multi-homed configurations. Note that setting 
LIBPROCESS_ip=127.0.0.1 makes you multihomed for this purpose, which is why 
``ExamplesTest.DiskFullFramework`` breaks.


Thanks,

James Peach

Re: Review Request 58224: Optionally verify the source IP address for libprocess messages.

2017-05-03 Thread James Peach 


> On May 2, 2017, 12:34 a.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp
> > Lines 480-492 (patched)
> > 
> >
> > I'm wondering if we can eliminate the need for this via global flag 
> > access and peer caching in the Socket implementation, see other comments.

Fixed the global flag, filed MESOS-7452 for the peer address caching.


> On May 2, 2017, 12:34 a.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp
> > Line 942 (original), 965 (patched)
> > 
> >
> > Per our offline discussion, could we acheive the elimination of 
> > `getpeername`-per-read by having the `Socket` perform this optimization for 
> > a connected socket? That would also avoid the need to carry the peer around.

I filed [MESOS-7452](https://issues.apache.org/jira/browse/MESOS-7452) to 
implement the `Socket` peer address caching.


- James


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58224/#review173536
---


On May 3, 2017, 10:35 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58224/
> ---
> 
> (Updated May 3, 2017, 10:35 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7401
> https://issues.apache.org/jira/browse/MESOS-7401
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In general, libprocess is unable to validate that a peer
> is a legitimate owner of the UPID it claims in a libprocess
> message. This change adds a check that the IP address in the
> UPID matches the peer address. This makes spoofing the UPID
> harder (eg. to send authenticated messages), but also breaks
> some legitimate configurations, particularly on multihomed
> hosts.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> f5b666f894215cb1861c244c94b382e0739bc5c9 
> 
> 
> Diff: https://reviews.apache.org/r/58224/diff/5/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25). Light manual testing.
> 
> With LIBPROCESS_pin_peer_address=true, all Mesos tests pass except 
> ``ExamplesTest.DiskFullFramework``, however enabling this will definitely 
> break some libprocess APIs (though not in the way that Mesos uses them) and 
> legitimate multi-homed configurations. Note that setting 
> LIBPROCESS_ip=127.0.0.1 makes you multihomed for this purpose, which is why 
> ``ExamplesTest.DiskFullFramework`` breaks.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 58224: Optionally verify the source IP address for libprocess messages.

2017-05-03 Thread James Peach 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58224/
---

(Updated May 3, 2017, 10:35 p.m.)


Review request for mesos and Benjamin Mahler.


Bugs: MESOS-7401
https://issues.apache.org/jira/browse/MESOS-7401


Repository: mesos


Description
---

In general, libprocess is unable to validate that a peer
is a legitimate owner of the UPID it claims in a libprocess
message. This change adds a check that the IP address in the
UPID matches the peer address. This makes spoofing the UPID
harder (eg. to send authenticated messages), but also breaks
some legitimate configurations, particularly on multihomed
hosts.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp f5b666f894215cb1861c244c94b382e0739bc5c9 


Diff: https://reviews.apache.org/r/58224/diff/5/

Changes: https://reviews.apache.org/r/58224/diff/4-5/


Testing
---

make check (Fedora 25). Light manual testing.

With LIBPROCESS_pin_peer_address=true, all Mesos tests pass except 
``ExamplesTest.DiskFullFramework``, however enabling this will definitely break 
some libprocess APIs (though not in the way that Mesos uses them) and 
legitimate multi-homed configurations. Note that setting 
LIBPROCESS_ip=127.0.0.1 makes you multihomed for this purpose, which is why 
``ExamplesTest.DiskFullFramework`` breaks.


Thanks,

James Peach

Re: Review Request 58224: Optionally verify the source IP address for libprocess messages.

2017-05-03 Thread Mesos Reviewbot 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58224/#review173718
---



Patch looks great!

Reviews applied: [58928, 58224]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On May 2, 2017, 9:02 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58224/
> ---
> 
> (Updated May 2, 2017, 9:02 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7401
> https://issues.apache.org/jira/browse/MESOS-7401
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In general, libprocess is unable to validate that a peer
> is a legitimate owner of the UPID it claims in a libprocess
> message. This change adds a check that the IP address in the
> UPID matches the peer address. This makes spoofing the UPID
> harder (eg. to send authenticated messages), but also breaks
> some legitimate configurations, particularly on multihomed
> hosts.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> f5b666f894215cb1861c244c94b382e0739bc5c9 
> 
> 
> Diff: https://reviews.apache.org/r/58224/diff/4/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25). Light manual testing.
> 
> With LIBPROCESS_pin_peer_address=true, all Mesos tests pass except 
> ``ExamplesTest.DiskFullFramework``, however enabling this will definitely 
> break some libprocess APIs (though not in the way that Mesos uses them) and 
> legitimate multi-homed configurations. Note that setting 
> LIBPROCESS_ip=127.0.0.1 makes you multihomed for this purpose, which is why 
> ``ExamplesTest.DiskFullFramework`` breaks.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 58224: Optionally verify the source IP address for libprocess messages.

2017-05-02 Thread James Peach 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58224/
---

(Updated May 2, 2017, 9:02 p.m.)


Review request for mesos and Benjamin Mahler.


Bugs: MESOS-7401
https://issues.apache.org/jira/browse/MESOS-7401


Repository: mesos


Description
---

In general, libprocess is unable to validate that a peer
is a legitimate owner of the UPID it claims in a libprocess
message. This change adds a check that the IP address in the
UPID matches the peer address. This makes spoofing the UPID
harder (eg. to send authenticated messages), but also breaks
some legitimate configurations, particularly on multihomed
hosts.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp f5b666f894215cb1861c244c94b382e0739bc5c9 


Diff: https://reviews.apache.org/r/58224/diff/4/

Changes: https://reviews.apache.org/r/58224/diff/3-4/


Testing
---

make check (Fedora 25). Light manual testing.

With LIBPROCESS_pin_peer_address=true, all Mesos tests pass except 
``ExamplesTest.DiskFullFramework``, however enabling this will definitely break 
some libprocess APIs (though not in the way that Mesos uses them) and 
legitimate multi-homed configurations. Note that setting 
LIBPROCESS_ip=127.0.0.1 makes you multihomed for this purpose, which is why 
``ExamplesTest.DiskFullFramework`` breaks.


Thanks,

James Peach

Re: Review Request 58224: Optionally verify the source IP address for libprocess messages.

2017-05-02 Thread James Peach 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58224/
---

(Updated May 2, 2017, 5:59 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Rebased and split tests.


Bugs: MESOS-7401
https://issues.apache.org/jira/browse/MESOS-7401


Repository: mesos


Description
---

In general, libprocess is unable to validate that a peer
is a legitimate owner of the UPID it claims in a libprocess
message. This change adds a check that the IP address in the
UPID matches the peer address. This makes spoofing the UPID
harder (eg. to send authenticated messages), but also breaks
some legitimate configurations, particularly on multihomed
hosts.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp f5b666f894215cb1861c244c94b382e0739bc5c9 


Diff: https://reviews.apache.org/r/58224/diff/3/

Changes: https://reviews.apache.org/r/58224/diff/2-3/


Testing
---

make check (Fedora 25). Light manual testing.

With LIBPROCESS_pin_peer_address=true, all Mesos tests pass except 
``ExamplesTest.DiskFullFramework``, however enabling this will definitely break 
some libprocess APIs (though not in the way that Mesos uses them) and 
legitimate multi-homed configurations. Note that setting 
LIBPROCESS_ip=127.0.0.1 makes you multihomed for this purpose, which is why 
``ExamplesTest.DiskFullFramework`` breaks.


Thanks,

James Peach

Re: Review Request 58224: Optionally verify the source IP address for libprocess messages.

2017-05-01 Thread Benjamin Mahler 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58224/#review173536
---



Can you pull the test changes out into a separate patch? Overall looks pretty 
good, just some details to sort through first.


3rdparty/libprocess/src/process.cpp
Lines 217 (patched)


How about `validate_peer_address`? Pin to me seems to suggest something 
that was floating across multiple things is now fixed to specific things.



3rdparty/libprocess/src/process.cpp
Lines 218 (patched)


How about "Validate that the ...", Force seems to suggest we can force it 
to happen?



3rdparty/libprocess/src/process.cpp
Lines 220-221 (patched)


Is it possible to elaborate on which configurations this will break? I'm 
pretty naive here and don't know, so would love to have a more comprehensive 
explanation. :)



3rdparty/libprocess/src/process.cpp
Lines 217-220 (original), 225-229 (patched)


Generally it seems that we should have global access to the flags from 
within libprocess, or if not the flags, the runtime properties that are deduced 
from the flags (the former would be simpler for now).

In terms of impacting this patch, it would mean that we don't need to pass 
the `validate_peer_address` bit around?



3rdparty/libprocess/src/process.cpp
Lines 480-492 (patched)


I'm wondering if we can eliminate the need for this via global flag access 
and peer caching in the Socket implementation, see other comments.



3rdparty/libprocess/src/process.cpp
Line 942 (original), 965 (patched)


Per our offline discussion, could we acheive the elimination of 
`getpeername`-per-read by having the `Socket` perform this optimization for a 
connected socket? That would also avoid the need to carry the peer around.



3rdparty/libprocess/src/process.cpp
Lines 2878-2879 (patched)


We could refer to the flag help for examples?



3rdparty/libprocess/src/process.cpp
Lines 2882 (patched)


Hm.. is this the best error code for this? It seems like a client error 
rather than a server error?



3rdparty/libprocess/src/process.cpp
Lines 2883-2884 (patched)


How about:

UPID IP address validation failed: Message from X was sent from IP Y.


- Benjamin Mahler


On May 1, 2017, 11:40 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58224/
> ---
> 
> (Updated May 1, 2017, 11:40 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7401
> https://issues.apache.org/jira/browse/MESOS-7401
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In general, libprocess is unable to validate that a peer
> is a legitimate owner of the UPID it claims in a libprocess
> message. This change adds a check that the IP address in the
> UPID matches the peer address. This makes spoofing the UPID
> harder (eg. to send authenticated messages), but also breaks
> some legitimate configurations, particularly on multihomed
> hosts.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 92efa915414c2a38b18de99858c66b63e757f63c 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> bf90c7e78fd50ad7e16cc0a69a248ba71e2a7115 
>   3rdparty/libprocess/src/tests/test_linkee.cpp 
> 921d67695bc0e4d601e9f74fbc625d69bf36ba50 
> 
> 
> Diff: https://reviews.apache.org/r/58224/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25). Light manual testing.
> 
> With LIBPROCESS_pin_peer_address=true, all Mesos tests pass except 
> ``ExamplesTest.DiskFullFramework``, however enabling this will definitely 
> break some libprocess APIs (though not in the way that Mesos uses them) and 
> legitimate multi-homed configurations. Note that setting 
> LIBPROCESS_ip=127.0.0.1 makes you multihomed for this purpose, which is why 
> ``ExamplesTest.DiskFullFramework`` breaks.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 58224: Optionally verify the source IP address for libprocess messages.

2017-04-18 Thread Mesos Reviewbot 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58224/#review172303
---



Patch looks great!

Reviews applied: [58517, 58224]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On April 18, 2017, 9:11 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58224/
> ---
> 
> (Updated April 18, 2017, 9:11 p.m.)
> 
> 
> Review request for mesos and Mesos Reviewbot.
> 
> 
> Bugs: MESOS-7401
> https://issues.apache.org/jira/browse/MESOS-7401
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In general, libprocess is unable to validate that a peer
> is a legitimate owner of the UPID it claims in a libprocess
> message. This change adds a check that the IP address in the
> UPID matches the peer address. This makes spoofing the UPID
> harder (eg. to send authenticated messages), but also breaks
> some legitimate configurations, particularly on multihomed
> hosts.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 92efa915414c2a38b18de99858c66b63e757f63c 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> bf90c7e78fd50ad7e16cc0a69a248ba71e2a7115 
>   3rdparty/libprocess/src/tests/test_linkee.cpp 
> 921d67695bc0e4d601e9f74fbc625d69bf36ba50 
> 
> 
> Diff: https://reviews.apache.org/r/58224/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25). Light manual testing.
> 
> With LIBPROCESS_pin_peer_address=true, all Mesos tests pass except 
> ``ExamplesTest.DiskFullFramework``, however enabling this will definitely 
> break some libprocess APIs (though not in the way that Mesos uses them) and 
> legitimate multi-homed configurations. Note that setting 
> LIBPROCESS_ip=127.0.0.1 makes you multihomed for this purpose, which is why 
> ``ExamplesTest.DiskFullFramework`` breaks.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 58224: Optionally verify the source IP address for libprocess messages.

2017-04-18 Thread James Peach 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58224/
---

(Updated April 18, 2017, 9:11 p.m.)


Review request for mesos and Mesos Reviewbot.


Bugs: MESOS-7401
https://issues.apache.org/jira/browse/MESOS-7401


Repository: mesos


Description
---

In general, libprocess is unable to validate that a peer
is a legitimate owner of the UPID it claims in a libprocess
message. This change adds a check that the IP address in the
UPID matches the peer address. This makes spoofing the UPID
harder (eg. to send authenticated messages), but also breaks
some legitimate configurations, particularly on multihomed
hosts.


Diffs
-

  3rdparty/libprocess/src/process.cpp 92efa915414c2a38b18de99858c66b63e757f63c 
  3rdparty/libprocess/src/tests/process_tests.cpp 
bf90c7e78fd50ad7e16cc0a69a248ba71e2a7115 
  3rdparty/libprocess/src/tests/test_linkee.cpp 
921d67695bc0e4d601e9f74fbc625d69bf36ba50 


Diff: https://reviews.apache.org/r/58224/diff/2/


Testing
---

make check (Fedora 25). Light manual testing.

With LIBPROCESS_pin_peer_address=true, all Mesos tests pass except 
``ExamplesTest.DiskFullFramework``, however enabling this will definitely break 
some libprocess APIs (though not in the way that Mesos uses them) and 
legitimate multi-homed configurations. Note that setting 
LIBPROCESS_ip=127.0.0.1 makes you multihomed for this purpose, which is why 
``ExamplesTest.DiskFullFramework`` breaks.


Thanks,

James Peach

Re: Review Request 58224: Optionally verify the source IP address for libprocess messages.

2017-04-18 Thread James Peach 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58224/
---

(Updated April 18, 2017, 9:02 p.m.)


Review request for mesos and Mesos Reviewbot.


Changes
---

Rebased, added options, fixed tests and addressed review feedback.


Summary (updated)
-

Optionally verify the source IP address for libprocess messages.


Repository: mesos


Description (updated)
---

In general, libprocess is unable to validate that a peer
is a legitimate owner of the UPID it claims in a libprocess
message. This change adds a check that the IP address in the
UPID matches the peer address. This makes spoofing the UPID
harder (eg. to send authenticated messages), but also breaks
some legitimate configurations, particularly on multihomed
hosts.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp 92efa915414c2a38b18de99858c66b63e757f63c 
  3rdparty/libprocess/src/tests/process_tests.cpp 
bf90c7e78fd50ad7e16cc0a69a248ba71e2a7115 
  3rdparty/libprocess/src/tests/test_linkee.cpp 
921d67695bc0e4d601e9f74fbc625d69bf36ba50 


Diff: https://reviews.apache.org/r/58224/diff/2/

Changes: https://reviews.apache.org/r/58224/diff/1-2/


Testing (updated)
---

make check (Fedora 25). Light manual testing.

With LIBPROCESS_pin_peer_address=true, all Mesos tests pass except 
``ExamplesTest.DiskFullFramework``, however enabling this will definitely break 
some libprocess APIs (though not in the way that Mesos uses them) and 
legitimate multi-homed configurations. Note that setting 
LIBPROCESS_ip=127.0.0.1 makes you multihomed for this purpose, which is why 
``ExamplesTest.DiskFullFramework`` breaks.


Thanks,

James Peach