Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[DRILL-6581] C++ Client SSL Implementation Fixes/Improvements #1366

Closed
wants to merge 3 commits into from
Closed

[DRILL-6581] C++ Client SSL Implementation Fixes/Improvements #1366

wants to merge 3 commits into from

Conversation

superbstreak
Copy link
Contributor

@superbstreak superbstreak commented Jul 6, 2018 •
edited

  • Fix: Hostname verification doesnt function as expected.
    > Host and port passed in to the callback are always empty. Parsing the connection string before we use the host name.
    > Connection string port (31010) is not required for the hostname verification.
  • Fix: Certificate load verification exceptions are swallowed and not propagated.
  • Improvement: SSL V3 is not disabled.
    > Disabled SSLv2 and SSLv3.
  • Improvement: Hostname verification will throw the same error message as certificate exception.
    > Separated them to allow better user error handling/debugging.
  • Improvement: Create individual error messages to allow error handling of the application using the client and follows the standard of the rest of the errors
  • Improvement: [DRILL-6586] Add SSL Hostname verification with zookeeper connection mode support
  • Improvement: [DRILL-6587] Added support for custom SSL CTX Options

@superbstreak superbstreak force-pushed the DRILL-6581 branch 3 times, most recently from 69dc0e3 to 786c026 Compare July 7, 2018 01:19
superbstreak
superbstreak commented Jul 7, 2018
View reviewed changes
contrib/native/client/src/clientlib/channel.hpp Outdated
/// @brief Check the certiticate hostname verification status.
///
/// @return if the verification has failed.
const bool HasCertHostnameVerificationError() const {return m_hasCertHostnameVerificationErr;}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should put these inside SSLChannelContext since these are SSL related items

superbstreak
superbstreak commented Jul 7, 2018
View reviewed changes
contrib/native/client/src/clientlib/channel.hpp Outdated
@@ -170,7 +190,22 @@ class UserProperties;
try{
m_pSocket->protocolHandshake(useSystemConfig);
} catch (boost::system::system_error e) {
status = handleError(CONN_HANDSHAKE_FAILED, e.what());
const std::string errmsg(e.what());
if (m_pContext->HasCertHostnameVerificationError()){
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sure m_psocket is actually ssl socket before we check for ssl specific exceptions. Currently, there is no impact since the normal socket's protoHandshake doesn't do any task. But should take care of it.

@superbstreak superbstreak force-pushed the DRILL-6581 branch 3 times, most recently from d2415a4 to 2fa865a Compare July 9, 2018 08:45
@priteshm
Copy link

priteshm commented Jul 9, 2018

@parthchandra can you please review this?

sohami
sohami reviewed Jul 10, 2018
View reviewed changes
contrib/native/client/src/clientlib/channel.cpp Outdated
@@ -211,6 +211,21 @@ ChannelContext* ChannelFactory::getChannelContext(channelType_t t, DrillUserProp
}

pChannelContext = new SSLChannelContext(props, tlsVersion, verifyMode);

if (props->isPropSet(USERPROP_CUSTOM_SSLCTXOPTIONS)){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to check isPropSet since you are already checking for !sslOptions.empty() below.

Also I have a question regarding these custom SSL Context options. Based on documentation here it helps to achieve workaround for listed bugs. But that depends upon the internal implementation of SSL if handling for those work around is available or not. If the handling is available then it will be used since this option is set during m_SSLContext creation (See here). So it doesn't look like we need to have this separate custom option setter ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an optional settings that the client application can set on top of the default. And also based on the version of boost the client is compiled against, some configurations are not available. So the user using this function should be aware of the limitation of the version of boost the client is compiled against and set the flags accordingly.

contrib/native/client/src/clientlib/channel.hpp Outdated

// Sets the result back to the context.
context->SetCertHostnameVerificationStatus(verified);
return verified && in_preverified;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just return the verified status here not (verified && in_preverified)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, redundant! Thanks!

contrib/native/client/src/clientlib/channel.hpp Outdated
if (!(((SSLChannelContext_t *)m_pContext)->GetCertificateHostnameVerificationStatus())){
return handleError(
CONN_HANDSHAKE_FAILED,
getMessage(ERR_CONN_SSL_CN));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be useful to preserve the original error message in this case as well. Please change to getMessage(ERR_CONN_SSL_CN, errmsg)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do Thanks!

contrib/native/client/src/clientlib/channel.hpp Outdated
namespace
{
// The error message to indicate certificate verification failure.
#define DRILL_BOOST_SSL_CERT_VERIFY_FAILED "handshake: certificate verify failed\0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can rely on this error string. Instead it would be good to use something like below for decoding ssl errors.
https://stackoverflow.com/questions/9828066/how-to-decipher-a-boost-asio-ssl-error-code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I was concern about this bit, too. Will do, thanks!

@superbstreak superbstreak force-pushed the DRILL-6581 branch 2 times, most recently from d3fc5cf to de8478b Compare July 11, 2018 01:09
@superbstreak
Copy link
Contributor Author

@sohami updated. Please review.

@superbstreak superbstreak force-pushed the DRILL-6581 branch 2 times, most recently from 1f05410 to 4df6745 Compare July 11, 2018 09:50
sohami
sohami approved these changes Jul 11, 2018
View reviewed changes
Copy link
Contributor

@sohami sohami left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this. +1 LGTM.

@sohami sohami closed this in 94186fc Jul 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sohami sohami sohami approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@superbstreak @priteshm @sohami