Re: Review Request 52031: Added openssl error string output to initializing failures.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On Sept. 22, 2016, 9:27 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52031/
> ---
> 
> (Updated Sept. 22, 2016, 9:27 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Bugs: MESOS-5320
> https://issues.apache.org/jira/browse/MESOS-5320
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the human readable openssl error messages for failure cases. Also
> fixes a spacing nit in one of the existing messages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 
> 
> Diff: https://reviews.apache.org/r/52031/diff/
> 
> 
> Testing
> ---
> 
> make check && functional testing
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 52031: Added openssl error string output to initializing failures.

[Joris Van Remoortere]

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


Ship it!




Ship It!

- Joris Van Remoortere


On Sept. 22, 2016, 9:27 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52031/
> ---
> 
> (Updated Sept. 22, 2016, 9:27 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Bugs: MESOS-5320
> https://issues.apache.org/jira/browse/MESOS-5320
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the human readable openssl error messages for failure cases. Also
> fixes a spacing nit in one of the existing messages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 
> 
> Diff: https://reviews.apache.org/r/52031/diff/
> 
> 
> Testing
> ---
> 
> make check && functional testing
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 52031: Added openssl error string output to initializing failures.

[Till Toenshoff]

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

(Updated Sept. 22, 2016, 9:27 a.m.)


Review request for mesos, Joris Van Remoortere and Joseph Wu.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Adds the human readable openssl error messages for failure cases. Also
fixes a spacing nit in one of the existing messages.


Diffs (updated)
-

  3rdparty/libprocess/src/openssl.cpp c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 

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


Testing
---

make check && functional testing


Thanks,

Till Toenshoff

Re: Review Request 52031: Added openssl error string output to initializing failures.

[Till Toenshoff]

> On Sept. 20, 2016, 1:23 a.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/openssl.cpp, lines 481-482
> > 
> >
> > Is there any information we can provide here about where we are looking 
> > for the defaults to help the user identify the problem?
> 
> Till Toenshoff wrote:
> The defaults are baked into the openssl libraries at compile-time. The 
> user may override those using openssl's `SSL_CERT_FILE` and `SSL_CERT_DIR`. 
> There seems to be no public way to extract those paths back out to get them 
> displayed.
> 
> Quick background: that information is obviously attached to the context, 
> internally that specific certificate stuff is handled by the 
> `X509_STORE`-API. The above call effectively attaches a new cert store to our 
> context and populates it with the content of the given file/dir path. The 
> result is a (bunch of) certificate/s attached. The source path however is 
> unknown later on - at least from the API point of view. So all we could 
> possibly show here are the context attached certificates but not their source 
> locations.
> 
> Till Toenshoff wrote:
> The documentation totally stays silent on `X509_get_default_cert_file` 
> and `X509_get_default_cert_dir`. However after checking their 
> implementations, to me it seems as if they would never return the value/s of 
> user-environment supplied overrides (e.g. `SSL_CERT_FILE`) but only the baked 
> in defaults. So instead of being helpful, in cases where the user used the 
> OpenSSL specific environment variables the output of those functions would be 
> even more confusing. In other words, if the user set `SSL_CERT_FILE` towards 
> `/foo/bar/cert.pem`, calling `X509_get_default_cert_file` would yield the 
> baked in default (e.g. `SSLCERTS:cert.pem`).

At this point I only see the following as an option for displaying helpful 
information:
Emulate the internal openssl bizzlogic by first checking the env var 
`SSL_CERT_FILE` for content - if set, return that one in our debug logging -- 
if not set, return the baked in default. See 
https://github.com/openssl/openssl/blob/master/crypto/x509/by_file.c#L50 for 
the details on their implementation.


- Till


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


On Sept. 19, 2016, 1:13 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52031/
> ---
> 
> (Updated Sept. 19, 2016, 1:13 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Bugs: MESOS-5320
> https://issues.apache.org/jira/browse/MESOS-5320
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the human readable openssl error messages for failure cases. Also
> fixes a spacing nit in one of the existing messages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 
> 
> Diff: https://reviews.apache.org/r/52031/diff/
> 
> 
> Testing
> ---
> 
> make check && functional testing
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 52031: Added openssl error string output to initializing failures.

[Till Toenshoff]

> On Sept. 20, 2016, 1:23 a.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/openssl.cpp, lines 481-482
> > 
> >
> > Is there any information we can provide here about where we are looking 
> > for the defaults to help the user identify the problem?
> 
> Till Toenshoff wrote:
> The defaults are baked into the openssl libraries at compile-time. The 
> user may override those using openssl's `SSL_CERT_FILE` and `SSL_CERT_DIR`. 
> There seems to be no public way to extract those paths back out to get them 
> displayed.
> 
> Quick background: that information is obviously attached to the context, 
> internally that specific certificate stuff is handled by the 
> `X509_STORE`-API. The above call effectively attaches a new cert store to our 
> context and populates it with the content of the given file/dir path. The 
> result is a (bunch of) certificate/s attached. The source path however is 
> unknown later on - at least from the API point of view. So all we could 
> possibly show here are the context attached certificates but not their source 
> locations.

The documentation totally stays silent on `X509_get_default_cert_file` and 
`X509_get_default_cert_dir`. However after checking their implementations, to 
me it seems as if they would never return the value/s of user-environment 
supplied overrides (e.g. `SSL_CERT_FILE`) but only the baked in defaults. So 
instead of being helpful, in cases where the user used the OpenSSL specific 
environment variables the output of those functions would be even more 
confusing. In other words, if the user set `SSL_CERT_FILE` towards 
`/foo/bar/cert.pem`, calling `X509_get_default_cert_file` would yield the baked 
in default (e.g. `SSLCERTS:cert.pem`).


- Till


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


On Sept. 19, 2016, 1:13 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52031/
> ---
> 
> (Updated Sept. 19, 2016, 1:13 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Bugs: MESOS-5320
> https://issues.apache.org/jira/browse/MESOS-5320
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the human readable openssl error messages for failure cases. Also
> fixes a spacing nit in one of the existing messages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 
> 
> Diff: https://reviews.apache.org/r/52031/diff/
> 
> 
> Testing
> ---
> 
> make check && functional testing
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 52031: Added openssl error string output to initializing failures.

[Till Toenshoff]

> On Sept. 20, 2016, 1:23 a.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/openssl.cpp, lines 481-482
> > 
> >
> > Is there any information we can provide here about where we are looking 
> > for the defaults to help the user identify the problem?

The defaults are baked into the openssl libraries at compile-time. The user may 
override those using openssl's `SSL_CERT_FILE` and `SSL_CERT_DIR`. There seems 
to be no public way to extract those paths back out to get them displayed.

Quick background: that information is obviously attached to the context, 
internally that specific certificate stuff is handled by the `X509_STORE`-API. 
The above call effectively attaches a new cert store to our context and 
populates it with the content of the given file/dir path. The result is a 
(bunch of) certificate/s attached. The source path however is unknown later on 
- at least from the API point of view. So all we could possibly show here are 
the context attached certificates but not their source locations.


- Till


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


On Sept. 19, 2016, 1:13 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52031/
> ---
> 
> (Updated Sept. 19, 2016, 1:13 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Bugs: MESOS-5320
> https://issues.apache.org/jira/browse/MESOS-5320
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the human readable openssl error messages for failure cases. Also
> fixes a spacing nit in one of the existing messages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 
> 
> Diff: https://reviews.apache.org/r/52031/diff/
> 
> 
> Testing
> ---
> 
> make check && functional testing
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 52031: Added openssl error string output to initializing failures.

[Till Toenshoff]

> On Sept. 19, 2016, 7:02 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/src/openssl.cpp, lines 509-510
> > 
> >
> > I'd prefer the parentheses containing the `stringify(error)` to mention 
> > OpenSSL, since the number of ambiguous/confusing otherwise.  
> > 
> > i.e. `(OpenSSL error #___): ...`
> > 
> > Ditto other error messages.
> 
> Till Toenshoff wrote:
> I agree, those are confusing. On a second thought, shall we simply remove 
> those error-code-numbers? I was trying to follow the initial example but now 
> feel we should complete get rid of those as we can expect the error string to 
> be helpful.
> 
> Joseph Wu wrote:
> I think the error codes might be useful for debugging.  I'm guessing the 
> error strings may change between OpenSSL versions (I wonder if there's 
> localization too), but the error codes will not.
> 
> Joris Van Remoortere wrote:
> The error codes are very helpful. Please don't remove them. As Joseph has 
> suggested, they're the easiest way to search for references on google, 
> openssl mailing list, etc.

Ok, you guys got me convinced :)  thanks!


- Till


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


On Sept. 19, 2016, 1:13 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52031/
> ---
> 
> (Updated Sept. 19, 2016, 1:13 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Bugs: MESOS-5320
> https://issues.apache.org/jira/browse/MESOS-5320
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the human readable openssl error messages for failure cases. Also
> fixes a spacing nit in one of the existing messages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 
> 
> Diff: https://reviews.apache.org/r/52031/diff/
> 
> 
> Testing
> ---
> 
> make check && functional testing
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 52031: Added openssl error string output to initializing failures.

[Joris Van Remoortere]

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



Agree with @Joseph's comments about keeping the error codes.


3rdparty/libprocess/src/openssl.cpp (lines 481 - 482)


Is there any information we can provide here about where we are looking for 
the defaults to help the user identify the problem?


- Joris Van Remoortere


On Sept. 19, 2016, 1:13 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52031/
> ---
> 
> (Updated Sept. 19, 2016, 1:13 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Bugs: MESOS-5320
> https://issues.apache.org/jira/browse/MESOS-5320
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the human readable openssl error messages for failure cases. Also
> fixes a spacing nit in one of the existing messages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 
> 
> Diff: https://reviews.apache.org/r/52031/diff/
> 
> 
> Testing
> ---
> 
> make check && functional testing
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 52031: Added openssl error string output to initializing failures.

[Joris Van Remoortere]

> On Sept. 19, 2016, 7:02 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/src/openssl.cpp, lines 509-510
> > 
> >
> > I'd prefer the parentheses containing the `stringify(error)` to mention 
> > OpenSSL, since the number of ambiguous/confusing otherwise.  
> > 
> > i.e. `(OpenSSL error #___): ...`
> > 
> > Ditto other error messages.
> 
> Till Toenshoff wrote:
> I agree, those are confusing. On a second thought, shall we simply remove 
> those error-code-numbers? I was trying to follow the initial example but now 
> feel we should complete get rid of those as we can expect the error string to 
> be helpful.
> 
> Joseph Wu wrote:
> I think the error codes might be useful for debugging.  I'm guessing the 
> error strings may change between OpenSSL versions (I wonder if there's 
> localization too), but the error codes will not.

The error codes are very helpful. Please don't remove them. As Joseph has 
suggested, they're the easiest way to search for references on google, openssl 
mailing list, etc.


- Joris


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


On Sept. 19, 2016, 1:13 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52031/
> ---
> 
> (Updated Sept. 19, 2016, 1:13 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Bugs: MESOS-5320
> https://issues.apache.org/jira/browse/MESOS-5320
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the human readable openssl error messages for failure cases. Also
> fixes a spacing nit in one of the existing messages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 
> 
> Diff: https://reviews.apache.org/r/52031/diff/
> 
> 
> Testing
> ---
> 
> make check && functional testing
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 52031: Added openssl error string output to initializing failures.

[Joseph Wu]

> On Sept. 19, 2016, 12:02 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/src/openssl.cpp, lines 509-510
> > 
> >
> > I'd prefer the parentheses containing the `stringify(error)` to mention 
> > OpenSSL, since the number of ambiguous/confusing otherwise.  
> > 
> > i.e. `(OpenSSL error #___): ...`
> > 
> > Ditto other error messages.
> 
> Till Toenshoff wrote:
> I agree, those are confusing. On a second thought, shall we simply remove 
> those error-code-numbers? I was trying to follow the initial example but now 
> feel we should complete get rid of those as we can expect the error string to 
> be helpful.

I think the error codes might be useful for debugging.  I'm guessing the error 
strings may change between OpenSSL versions (I wonder if there's localization 
too), but the error codes will not.


- Joseph


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


On Sept. 19, 2016, 6:13 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52031/
> ---
> 
> (Updated Sept. 19, 2016, 6:13 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Bugs: MESOS-5320
> https://issues.apache.org/jira/browse/MESOS-5320
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the human readable openssl error messages for failure cases. Also
> fixes a spacing nit in one of the existing messages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 
> 
> Diff: https://reviews.apache.org/r/52031/diff/
> 
> 
> Testing
> ---
> 
> make check && functional testing
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 52031: Added openssl error string output to initializing failures.

[Till Toenshoff]

> On Sept. 19, 2016, 7:02 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/src/openssl.cpp, lines 509-510
> > 
> >
> > I'd prefer the parentheses containing the `stringify(error)` to mention 
> > OpenSSL, since the number of ambiguous/confusing otherwise.  
> > 
> > i.e. `(OpenSSL error #___): ...`
> > 
> > Ditto other error messages.

I agree, those are confusing. On a second thought, shall we simply remove those 
error-code-numbers? I was trying to follow the initial example but now feel we 
should complete get rid of those as we can expect the error string to be 
helpful.


- Till


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


On Sept. 19, 2016, 1:13 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52031/
> ---
> 
> (Updated Sept. 19, 2016, 1:13 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Bugs: MESOS-5320
> https://issues.apache.org/jira/browse/MESOS-5320
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the human readable openssl error messages for failure cases. Also
> fixes a spacing nit in one of the existing messages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 
> 
> Diff: https://reviews.apache.org/r/52031/diff/
> 
> 
> Testing
> ---
> 
> make check && functional testing
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 52031: Added openssl error string output to initializing failures.

[Joseph Wu]

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


Fix it, then Ship it!





3rdparty/libprocess/src/openssl.cpp (lines 467 - 468)


And if you decide to add that error code label (see below comment)... this 
is the only place that has the same pattern.



3rdparty/libprocess/src/openssl.cpp (lines 509 - 510)


I'd prefer the parentheses containing the `stringify(error)` to mention 
OpenSSL, since the number of ambiguous/confusing otherwise.  

i.e. `(OpenSSL error #___): ...`

Ditto other error messages.


- Joseph Wu


On Sept. 19, 2016, 6:13 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52031/
> ---
> 
> (Updated Sept. 19, 2016, 6:13 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Bugs: MESOS-5320
> https://issues.apache.org/jira/browse/MESOS-5320
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the human readable openssl error messages for failure cases. Also
> fixes a spacing nit in one of the existing messages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 
> 
> Diff: https://reviews.apache.org/r/52031/diff/
> 
> 
> Testing
> ---
> 
> make check && functional testing
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>