Re: Review Request 49401: Updated certificate validation to check 'IP Address' SAN.

2016-07-04 Thread Till Toenshoff 

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

(Updated July 4, 2016, 4:09 p.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 (updated)
---

Allows the verification of X509 certificates based on an IP address
instead of a hostname. Introduces a new environment variable;
\`SSL_VERIFY_IPADD\` which, when set to \`true\` will enable the
peer certificate verification to additionally rely on the IP
address of a connection.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 
1dbdaa80d0726bcaa1cefc89c57bb7609b64 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
19d9ae59f1b474accaec924c4069c6b1ce995b46 
  3rdparty/libprocess/src/openssl.hpp 7d5502545ec5f8c495bd1d3f58a0f4b71bcb3386 
  3rdparty/libprocess/src/openssl.cpp 0f62aa6ade1c95e506fd06aa4806557ba6583433 

Diff: https://reviews.apache.org/r/49401/diff/


Testing
---

make check on OSX and various linux distros.


Thanks,

Till Toenshoff

Re: Review Request 49401: Updated certificate validation to check 'IP Address' SAN.

2016-06-30 Thread Till Toenshoff 

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

(Updated June 30, 2016, 9:52 p.m.)


Review request for mesos, Adam B, Albert Strasheim, Artem Harutyunyan, Joris 
Van Remoortere, and Lukas Loesche.


Changes
---

addressed all applicable comments - thanks @joris, much appreciated.


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


Repository: mesos


Description
---

Allows the verification of X509 certificates based on an IP address
instead of a hostname. Introduces a new environment variable;
`SSL_VERIFY_IPADD` which, when set to `true` will enable the
peer certificate verification to additionally rely on the IP
address of a connection.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 1dbdaa8 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 19d9ae5 
  3rdparty/libprocess/src/openssl.hpp 7d55025 
  3rdparty/libprocess/src/openssl.cpp 0f62aa6 

Diff: https://reviews.apache.org/r/49401/diff/


Testing
---

make check on OSX and various linux distros.


Thanks,

Till Toenshoff

Re: Review Request 49401: Updated certificate validation to check 'IP Address' SAN.

2016-06-30 Thread Till Toenshoff 


> On June 30, 2016, 3:58 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/libevent_ssl_socket.cpp, line 527
> > 
> >
> > "verify the SSL connection later"
> > =>
> > "verify the certificate later"
> > ?

That was a copy from the old comment on the peer_hostname. Will adapt 
both accordingly.


> On June 30, 2016, 3:58 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/openssl.cpp, lines 700-712
> > 
> >
> > This is unfortunate.
> > Can you sync with @mcypark to see if we can clean this up?

Brainstormed a lil with MPark. Even if we had a `strings::join` for `Option` 
taking `isNone()` into account - which we currently don't - we'ld still not 
really reach the goal yet. We'ld further on have to have to something like this:
```
  Option hostname_ =
hostname.isSome() ? "hostname " + hostname.get() : Option::none;

  Option ip_ = ip.isSome() ? "IP " + ip.get() : Option::none;

  return Error("Could not verify presented certificate with " +
   strings::join(", ", hostname_, ip_));
```

However, we might actually have a demand for such `strings::join` - seems worth 
checking our codebase at some point for patterns similar to mine - also getting 
other places more concise thereby.


- Till


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


On June 30, 2016, 2 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49401/
> ---
> 
> (Updated June 30, 2016, 2 p.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
> ---
> 
> Allows the verification of X509 certificates based on an IP address
> instead of a hostname. Introduces a new environment variable;
> `SSL_VERIFY_IPADD` which, when set to `true` will enable the
> peer certificate verification to additionally rely on the IP
> address of a connection.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 1dbdaa8 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 19d9ae5 
>   3rdparty/libprocess/src/openssl.hpp 7d55025 
>   3rdparty/libprocess/src/openssl.cpp 0f62aa6 
> 
> Diff: https://reviews.apache.org/r/49401/diff/
> 
> 
> Testing
> ---
> 
> make check on OSX and various linux distros.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 49401: Updated certificate validation to check 'IP Address' SAN.

2016-06-30 Thread Joris Van Remoortere 

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


Fix it, then Ship it!





3rdparty/libprocess/src/libevent_ssl_socket.cpp (line 527)


"verify the SSL connection later"
=>
"verify the certificate later"
?



3rdparty/libprocess/src/libevent_ssl_socket.cpp (lines 1046 - 1047)


Can you check the wrapping on this?



3rdparty/libprocess/src/openssl.cpp (line 85)


Do we need to clarify that this is an extension?



3rdparty/libprocess/src/openssl.cpp (lines 409 - 410)


This seems confusing.
If the flag is false, then we do verify by IP?



3rdparty/libprocess/src/openssl.cpp (line 612)


Should we be doing this before we do the NUL check below?



3rdparty/libprocess/src/openssl.cpp (line 628)


Depending on how you structure the above logs, consider printing out the 
matched value here.



3rdparty/libprocess/src/openssl.cpp (line 680)


Can we expand `CN`?



3rdparty/libprocess/src/openssl.cpp (line 689)


Same



3rdparty/libprocess/src/openssl.cpp (lines 700 - 712)


This is unfortunate.
Can you sync with @mcypark to see if we can clean this up?


- Joris Van Remoortere


On June 30, 2016, 2 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49401/
> ---
> 
> (Updated June 30, 2016, 2 p.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
> ---
> 
> Allows the verification of X509 certificates based on an IP address
> instead of a hostname. Introduces a new environment variable;
> `SSL_VERIFY_IPADD` which, when set to `true` will enable the
> peer certificate verification to additionally rely on the IP
> address of a connection.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 1dbdaa8 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 19d9ae5 
>   3rdparty/libprocess/src/openssl.hpp 7d55025 
>   3rdparty/libprocess/src/openssl.cpp 0f62aa6 
> 
> Diff: https://reviews.apache.org/r/49401/diff/
> 
> 
> Testing
> ---
> 
> make check on OSX and various linux distros.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 49401: Updated certificate validation to check 'IP Address' SAN.

2016-06-30 Thread Till Toenshoff 

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

(Updated June 30, 2016, 2 p.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 (updated)
---

Allows the verification of X509 certificates based on an IP address
instead of a hostname. Introduces a new environment variable;
`SSL_VERIFY_IPADD` which, when set to `true` will enable the
peer certificate verification to additionally rely on the IP
address of a connection.


Diffs
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 1dbdaa8 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 19d9ae5 
  3rdparty/libprocess/src/openssl.hpp 7d55025 
  3rdparty/libprocess/src/openssl.cpp 0f62aa6 

Diff: https://reviews.apache.org/r/49401/diff/


Testing
---

make check on OSX and various linux distros.


Thanks,

Till Toenshoff

Re: Review Request 49401: Updated certificate validation to check 'IP Address' SAN.

2016-06-30 Thread Till Toenshoff 

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

(Updated June 30, 2016, 2 p.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 (updated)
---

Allows the verification of X509 certificates based on an IP address
instead of a hostname. Introduces a new environment variable;
`SSL_VERIFY_IPADD` which, when set to `true` will enable the
peer certificate verification to additionally relies on the IP
address of a connection.


Diffs
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 1dbdaa8 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 19d9ae5 
  3rdparty/libprocess/src/openssl.hpp 7d55025 
  3rdparty/libprocess/src/openssl.cpp 0f62aa6 

Diff: https://reviews.apache.org/r/49401/diff/


Testing
---

make check on OSX and various linux distros.


Thanks,

Till Toenshoff

Re: Review Request 49401: Updated certificate validation to check 'IP Address' SAN.

2016-06-30 Thread Till Toenshoff 

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

(Updated June 30, 2016, 1:40 p.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
---

Allows the verification of X509 certificates based on an IP address
instead of a hostname. Introduces a new environment variable;
`SSL_VERIFY_IPADD` which, when set to `true` will disable any
attempts to reverse-/lookup the hostname for certificate validation.
Instead the peer certificate verification then relies on the IP
address of a connection.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 1dbdaa8 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 19d9ae5 
  3rdparty/libprocess/src/openssl.hpp 7d55025 
  3rdparty/libprocess/src/openssl.cpp 0f62aa6 

Diff: https://reviews.apache.org/r/49401/diff/


Testing
---

make check on OSX and various linux distros.


Thanks,

Till Toenshoff

Re: Review Request 49401: Updated certificate validation to check 'IP Address' SAN.

2016-06-30 Thread Lukas Loesche 

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




3rdparty/libprocess/src/libevent_ssl_socket.cpp (line 515)


Why does turning on support for IP Address verification turn off support 
for Hostname verification? Shouldn't it be an added functionality not a 
replacement? Or am I reading the code wrong?


- Lukas Loesche


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/49401/
> ---
> 
> (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
> ---
> 
> Allows the verification of X509 certificates based on an IP address
> instead of a hostname. Introduces a new environment variable;
> `SSL_VERIFY_IPADD` which, when set to `true` will disable any
> attempts to reverse-/lookup the hostname for certificate validation.
> Instead the peer certificate verification then relies on the IP
> address of a connection.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 1dbdaa8 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 19d9ae5 
>   3rdparty/libprocess/src/openssl.hpp 7d55025 
>   3rdparty/libprocess/src/openssl.cpp 0f62aa6 
> 
> Diff: https://reviews.apache.org/r/49401/diff/
> 
> 
> Testing
> ---
> 
> make check on OSX and various linux distros.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Review Request 49401: Updated certificate validation to check 'IP Address' SAN.

2016-06-29 Thread Till Toenshoff 

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

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

Allows the verification of X509 certificates based on an IP address
instead of a hostname. Introduces a new environment variable;
`SSL_VERIFY_IPADD` which, when set to `true` will disable any
attempts to reverse-/lookup the hostname for certificate validation.
Instead the peer certificate verification then relies on the IP
address of a connection.


Diffs
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 1dbdaa8 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 19d9ae5 
  3rdparty/libprocess/src/openssl.hpp 7d55025 
  3rdparty/libprocess/src/openssl.cpp 0f62aa6 

Diff: https://reviews.apache.org/r/49401/diff/


Testing
---

make check on OSX and various linux distros.


Thanks,

Till Toenshoff