----------------------------------------------------------- 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) <https://reviews.apache.org/r/58224/#comment246520> 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) <https://reviews.apache.org/r/58224/#comment246521> How about "Validate that the ...", Force seems to suggest we can force it to happen? 3rdparty/libprocess/src/process.cpp Lines 220-221 (patched) <https://reviews.apache.org/r/58224/#comment246522> 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) <https://reviews.apache.org/r/58224/#comment246531> 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) <https://reviews.apache.org/r/58224/#comment246537> 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) <https://reviews.apache.org/r/58224/#comment246533> 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) <https://reviews.apache.org/r/58224/#comment246536> We could refer to the flag help for examples? 3rdparty/libprocess/src/process.cpp Lines 2882 (patched) <https://reviews.apache.org/r/58224/#comment246535> 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) <https://reviews.apache.org/r/58224/#comment246534> 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 > >
