You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Gordon Sim <gs...@redhat.com> on 2014/06/23 23:03:32 UTC
Review Request 22890: Allow SSL hostname verification to be disabled in c++
client
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22890/
-----------------------------------------------------------
Review request for qpid, Andrew Stitcher and Pavel Moravec.
Repository: qpid
Description
-------
Adds a connection option ignore_ssl_hostname_verification_failure, which if set to true will cause a connect attempt to proceed even if the hostname connecting to does not match the peers certificate.
Diffs
-----
/trunk/qpid/cpp/src/qpid/client/ConnectionSettings.h 1604917
/trunk/qpid/cpp/src/qpid/client/ConnectionSettings.cpp 1604917
/trunk/qpid/cpp/src/qpid/client/SslConnector.cpp 1604917
/trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp 1604917
/trunk/qpid/cpp/src/qpid/messaging/ConnectionOptions.cpp 1604917
/trunk/qpid/cpp/src/qpid/messaging/amqp/SslTransport.cpp 1604917
/trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.h 1604917
/trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.cpp 1604917
Diff: https://reviews.apache.org/r/22890/diff/
Testing
-------
Thanks,
Gordon Sim
Re: Review Request 22890: Allow SSL hostname verification to be disabled in
c++ client
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22890/#review46519
-----------------------------------------------------------
Ship it!
Ship It!
- Andrew Stitcher
On June 24, 2014, 7:33 a.m., Gordon Sim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22890/
> -----------------------------------------------------------
>
> (Updated June 24, 2014, 7:33 a.m.)
>
>
> Review request for qpid, Andrew Stitcher and Pavel Moravec.
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Adds a connection option ignore_ssl_hostname_verification_failure, which if set to true will cause a connect attempt to proceed even if the hostname connecting to does not match the peers certificate.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/client/ConnectionSettings.h 1604917
> /trunk/qpid/cpp/src/qpid/client/ConnectionSettings.cpp 1604917
> /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp 1604917
> /trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp 1604917
> /trunk/qpid/cpp/src/qpid/messaging/ConnectionOptions.cpp 1604917
> /trunk/qpid/cpp/src/qpid/messaging/amqp/SslTransport.cpp 1604917
> /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.h 1604917
> /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.cpp 1604917
>
> Diff: https://reviews.apache.org/r/22890/diff/
>
>
> Testing
> -------
>
> make test passes
>
>
> Thanks,
>
> Gordon Sim
>
>
Re: Review Request 22890: Allow SSL hostname verification to be disabled in
c++ client
Posted by Gordon Sim <gs...@redhat.com>.
> On June 24, 2014, 1:05 p.m., Kenneth Giusti wrote:
> > Just FYI: the pure python client has a similar feature. Its configuration parameter is a boolean called "ssl_skip_hostname_check":
> >
> > self.ssl_skip_hostname_check = options.get("ssl_skip_hostname_check", False)
The reason I chose 'ignore hostname verification failure' is that with NSS you can't actually skip the check, you can just choose to ignore it. The difference is perhaps irrelevant to users. However on the other hand none of the ssl options between c++ and python are aligned anyway.
- Gordon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22890/#review46521
-----------------------------------------------------------
On June 24, 2014, 7:33 a.m., Gordon Sim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22890/
> -----------------------------------------------------------
>
> (Updated June 24, 2014, 7:33 a.m.)
>
>
> Review request for qpid, Andrew Stitcher and Pavel Moravec.
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Adds a connection option ignore_ssl_hostname_verification_failure, which if set to true will cause a connect attempt to proceed even if the hostname connecting to does not match the peers certificate.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/client/ConnectionSettings.h 1604917
> /trunk/qpid/cpp/src/qpid/client/ConnectionSettings.cpp 1604917
> /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp 1604917
> /trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp 1604917
> /trunk/qpid/cpp/src/qpid/messaging/ConnectionOptions.cpp 1604917
> /trunk/qpid/cpp/src/qpid/messaging/amqp/SslTransport.cpp 1604917
> /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.h 1604917
> /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.cpp 1604917
>
> Diff: https://reviews.apache.org/r/22890/diff/
>
>
> Testing
> -------
>
> make test passes
>
>
> Thanks,
>
> Gordon Sim
>
>
Re: Review Request 22890: Allow SSL hostname verification to be disabled in
c++ client
Posted by Kenneth Giusti <kg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22890/#review46521
-----------------------------------------------------------
Ship it!
Just FYI: the pure python client has a similar feature. Its configuration parameter is a boolean called "ssl_skip_hostname_check":
self.ssl_skip_hostname_check = options.get("ssl_skip_hostname_check", False)
- Kenneth Giusti
On June 24, 2014, 7:33 a.m., Gordon Sim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22890/
> -----------------------------------------------------------
>
> (Updated June 24, 2014, 7:33 a.m.)
>
>
> Review request for qpid, Andrew Stitcher and Pavel Moravec.
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Adds a connection option ignore_ssl_hostname_verification_failure, which if set to true will cause a connect attempt to proceed even if the hostname connecting to does not match the peers certificate.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/client/ConnectionSettings.h 1604917
> /trunk/qpid/cpp/src/qpid/client/ConnectionSettings.cpp 1604917
> /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp 1604917
> /trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp 1604917
> /trunk/qpid/cpp/src/qpid/messaging/ConnectionOptions.cpp 1604917
> /trunk/qpid/cpp/src/qpid/messaging/amqp/SslTransport.cpp 1604917
> /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.h 1604917
> /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.cpp 1604917
>
> Diff: https://reviews.apache.org/r/22890/diff/
>
>
> Testing
> -------
>
> make test passes
>
>
> Thanks,
>
> Gordon Sim
>
>
Re: Review Request 22890: Allow SSL hostname verification to be disabled in
c++ client
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22890/
-----------------------------------------------------------
(Updated June 24, 2014, 7:33 a.m.)
Review request for qpid, Andrew Stitcher and Pavel Moravec.
Changes
-------
Changes as requested by Andrew.
Repository: qpid
Description
-------
Adds a connection option ignore_ssl_hostname_verification_failure, which if set to true will cause a connect attempt to proceed even if the hostname connecting to does not match the peers certificate.
Diffs (updated)
-----
/trunk/qpid/cpp/src/qpid/client/ConnectionSettings.h 1604917
/trunk/qpid/cpp/src/qpid/client/ConnectionSettings.cpp 1604917
/trunk/qpid/cpp/src/qpid/client/SslConnector.cpp 1604917
/trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp 1604917
/trunk/qpid/cpp/src/qpid/messaging/ConnectionOptions.cpp 1604917
/trunk/qpid/cpp/src/qpid/messaging/amqp/SslTransport.cpp 1604917
/trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.h 1604917
/trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.cpp 1604917
Diff: https://reviews.apache.org/r/22890/diff/
Testing (updated)
-------
make test passes
Thanks,
Gordon Sim
Re: Review Request 22890: Allow SSL hostname verification to be disabled in
c++ client
Posted by Gordon Sim <gs...@redhat.com>.
> On June 23, 2014, 9:21 p.m., Andrew Stitcher wrote:
> > This mostly looks fine - just a few comments below:
> >
> > Unless you are planning to do this work for windows too you should raise a JIRA for this work on that platform. Is there a JIRA that this relates to?
Not yet, but I'll raise one.
> On June 23, 2014, 9:21 p.m., Andrew Stitcher wrote:
> > /trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp, line 158
> > <https://reviews.apache.org/r/22890/diff/1/?file=615200#file615200line158>
> >
> > [Personal opinion] I think that it would be easier to remember the option if it began with "ssl-" like all the other ssl related options. I'd suggest "ssl-ignore-hostname-verification-failure".
> On June 23, 2014, 9:21 p.m., Andrew Stitcher wrote:
> > /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.cpp, line 147
> > <https://reviews.apache.org/r/22890/diff/1/?file=615204#file615204line147>
> >
> > I'm not sure this should be at warning level. Since the user had to specifically request this behaviour it is expected, so warning might be too noisy - perhaps info?
> >
> > BTW In the failure case do we log enough information to really figure out what is wrong? If not then maybe we should always go through this callback with the verification flag passed in here somehow. This way we can log a better error message.
With the wrong hostname at present (or with the option off), the error message is 'Unable to communicate securely with peer: requested domain name does not match the server's certificate.'
- Gordon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22890/#review46449
-----------------------------------------------------------
On June 23, 2014, 9:03 p.m., Gordon Sim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22890/
> -----------------------------------------------------------
>
> (Updated June 23, 2014, 9:03 p.m.)
>
>
> Review request for qpid, Andrew Stitcher and Pavel Moravec.
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Adds a connection option ignore_ssl_hostname_verification_failure, which if set to true will cause a connect attempt to proceed even if the hostname connecting to does not match the peers certificate.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/client/ConnectionSettings.h 1604917
> /trunk/qpid/cpp/src/qpid/client/ConnectionSettings.cpp 1604917
> /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp 1604917
> /trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp 1604917
> /trunk/qpid/cpp/src/qpid/messaging/ConnectionOptions.cpp 1604917
> /trunk/qpid/cpp/src/qpid/messaging/amqp/SslTransport.cpp 1604917
> /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.h 1604917
> /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.cpp 1604917
>
> Diff: https://reviews.apache.org/r/22890/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gordon Sim
>
>
Re: Review Request 22890: Allow SSL hostname verification to be disabled in
c++ client
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22890/#review46449
-----------------------------------------------------------
This mostly looks fine - just a few comments below:
Unless you are planning to do this work for windows too you should raise a JIRA for this work on that platform. Is there a JIRA that this relates to?
/trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp
<https://reviews.apache.org/r/22890/#comment81844>
[Personal opinion] I think that it would be easier to remember the option if it began with "ssl-" like all the other ssl related options. I'd suggest "ssl-ignore-hostname-verification-failure".
/trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.cpp
<https://reviews.apache.org/r/22890/#comment81843>
I'm not sure this should be at warning level. Since the user had to specifically request this behaviour it is expected, so warning might be too noisy - perhaps info?
BTW In the failure case do we log enough information to really figure out what is wrong? If not then maybe we should always go through this callback with the verification flag passed in here somehow. This way we can log a better error message.
- Andrew Stitcher
On June 23, 2014, 9:03 p.m., Gordon Sim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22890/
> -----------------------------------------------------------
>
> (Updated June 23, 2014, 9:03 p.m.)
>
>
> Review request for qpid, Andrew Stitcher and Pavel Moravec.
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Adds a connection option ignore_ssl_hostname_verification_failure, which if set to true will cause a connect attempt to proceed even if the hostname connecting to does not match the peers certificate.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/client/ConnectionSettings.h 1604917
> /trunk/qpid/cpp/src/qpid/client/ConnectionSettings.cpp 1604917
> /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp 1604917
> /trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp 1604917
> /trunk/qpid/cpp/src/qpid/messaging/ConnectionOptions.cpp 1604917
> /trunk/qpid/cpp/src/qpid/messaging/amqp/SslTransport.cpp 1604917
> /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.h 1604917
> /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.cpp 1604917
>
> Diff: https://reviews.apache.org/r/22890/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gordon Sim
>
>