mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 58224: Optionally verify the source IP address for libprocess messages.
Date Tue, 02 May 2017 00:34:21 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message