mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joris Van Remoortere <joris.van.remoort...@gmail.com>
Subject Re: Review Request 49400: Extended utilities to render certificate extension for IP.
Date Thu, 30 Jun 2016 14:51:28 GMT

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


Fix it, then Ship it!





3rdparty/libprocess/include/process/ssl/utilities.hpp (line 61)
<https://reviews.apache.org/r/49400/#comment205465>

    how about:
    "... will use the ip for a subject alternative name `iPAddress` extension."



3rdparty/libprocess/include/process/ssl/utilities.hpp (line 73)
<https://reviews.apache.org/r/49400/#comment205466>

    mention the `iPAdrress` extension as per your higher comment



3rdparty/libprocess/src/ssl/utilities.cpp (line 217)
<https://reviews.apache.org/r/49400/#comment205468>

    Here you say `create`, below you say `malloc`



3rdparty/libprocess/src/ssl/utilities.cpp (line 234)
<https://reviews.apache.org/r/49400/#comment205467>

    Capitalize `Failed`



3rdparty/libprocess/src/ssl/utilities.cpp (line 239)
<https://reviews.apache.org/r/49400/#comment205469>

    Why should we abort it the family type was unsupported (`ip.get().in()` returns an error).
    
    Our function returns a `Try`, can we propagate the error?



3rdparty/libprocess/src/ssl/utilities.cpp (lines 240 - 243)
<https://reviews.apache.org/r/49400/#comment205470>

    I'm not sure I understand why this works.
    `in_addr.get().s_addr` is a uint. Aren't we supposed to be copying a string in this case?
    
    If it *is* supposed to be a binary IP this definitely deserves a comment. The documentation
doesn't make it clear to me that this can be binary instead of a string.



3rdparty/libprocess/src/ssl/utilities.cpp (line 247)
<https://reviews.apache.org/r/49400/#comment205471>

    Capitalize `Failed`



3rdparty/libprocess/src/ssl/utilities.cpp (line 252)
<https://reviews.apache.org/r/49400/#comment205475>

    If this transfers ownership of the memory for `alt_name` to the `alt_name_stack` can we
add a comment here.
    It's easier to follow the memory cleanup below if we know why we're not cleaning up the
`alt_name` by itself anymore.



3rdparty/libprocess/src/ssl/utilities.cpp (line 256)
<https://reviews.apache.org/r/49400/#comment205472>

    Capitalize `Failed`



3rdparty/libprocess/src/ssl/utilities.cpp (lines 266 - 267)
<https://reviews.apache.org/r/49400/#comment205476>

    Inconsistent use of `alt_name` in the error messages



3rdparty/libprocess/src/ssl/utilities.cpp (line 267)
<https://reviews.apache.org/r/49400/#comment205473>

    Capitalize `Failed`.


- Joris Van Remoortere


On June 30, 2016, 12:19 a.m., Till Toenshoff wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49400/
> -----------------------------------------------------------
> 
> (Updated June 30, 2016, 12:19 a.m.)
> 
> 
> Review request for mesos, Adam B, Albert Strasheim, Artem Harutyunyan, Joris Van Remoortere,
and Lukas Loesche.
> 
> 
> Bugs: MESOS-5724
>     https://issues.apache.org/jira/browse/MESOS-5724
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Adds the ability to render a subject alternative name based on a given
> IP address within a X509 certificate extension. Additionally the
> libprocess test suite makes use of this feature.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/ssl/gtest.hpp 5435ddd 
>   3rdparty/libprocess/include/process/ssl/utilities.hpp ad9ec5d 
>   3rdparty/libprocess/src/ssl/utilities.cpp d23f462 
> 
> Diff: https://reviews.apache.org/r/49400/diff/
> 
> 
> Testing
> -------
> 
> make check on OSX and various linux distros.
> 
> Functional testing by validating a rendered certificate;
> 
> ```
> openssl x509 -text -noout -in "temp_cert_file_name"
> ```
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>


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