[GitHub] drill pull request #997: DRILL-5582: C++ Client: [Threat Modeling] Drillbit ...
Github user sohami commented on a diff in the pull request: https://github.com/apache/drill/pull/997#discussion_r145319769 --- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp --- @@ -518,6 +518,22 @@ bool DrillClientImpl::clientNeedsEncryption(const DrillUserProperties* userPrope return needsEncryption; } +/* + * Checks if the client has explicitly expressed interest in authenticated connections only. + * If the USERPROP_PASSWORD or USERPROP_AUTH_MECHANISM connection string properties are set, + * then it is implied that the client wants authentication. + */ +bool DrillClientImpl::clientNeedsAuthentication(const DrillUserProperties* userProperties) { +bool needsAuthentication = false; +if(!userProperties) { +return false; +} +needsAuthentication = userProperties->isPropSet(USERPROP_PASSWORD) || +userProperties->isPropSet(USERPROP_AUTH_MECHANISM); --- End diff -- I think we should also check if the `password & auth parameter` value is not empty string. ---
[GitHub] drill pull request #997: DRILL-5582: C++ Client: [Threat Modeling] Drillbit ...
Github user sohami commented on a diff in the pull request: https://github.com/apache/drill/pull/997#discussion_r145319760 --- Diff: contrib/native/client/src/clientlib/saslAuthenticatorImpl.cpp --- @@ -145,6 +145,8 @@ int SaslAuthenticatorImpl::init(const std::vector& mechanisms, exec authMechanismToUse = value; } } +// clientNeedsAuth cannot be false if the code above picks an authMechanism --- End diff -- clientNeedsAuth --> clientNeedsAuthentication ---
[GitHub] drill pull request #997: DRILL-5582: C++ Client: [Threat Modeling] Drillbit ...
Github user parthchandra commented on a diff in the pull request: https://github.com/apache/drill/pull/997#discussion_r145317288 --- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp --- @@ -595,6 +611,12 @@ connectionStatus_t DrillClientImpl::validateHandshake(DrillUserProperties* prope switch(this->m_handshakeStatus) { case exec::user::SUCCESS: +// Check if client needs auth/encryption and server is not requiring it +if(clientNeedsAuthentication(properties) || clientNeedsEncryption(properties)) { --- End diff -- Generally, all error messages come from errmsgs.cpp so we can localize them when we need to. ---
[GitHub] drill pull request #997: DRILL-5582: C++ Client: [Threat Modeling] Drillbit ...
Github user parthchandra commented on a diff in the pull request: https://github.com/apache/drill/pull/997#discussion_r145317403 --- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp --- @@ -595,6 +611,12 @@ connectionStatus_t DrillClientImpl::validateHandshake(DrillUserProperties* prope switch(this->m_handshakeStatus) { case exec::user::SUCCESS: +// Check if client needs auth/encryption and server is not requiring it --- End diff -- Not too clear about the SASL flow, but I assume that if the server returning SUCCESS is sufficient to assume that there is no auth required by the server. ---
[GitHub] drill pull request #997: DRILL-5582: C++ Client: [Threat Modeling] Drillbit ...
GitHub user bitblender opened a pull request: https://github.com/apache/drill/pull/997 DRILL-5582: C++ Client: [Threat Modeling] Drillbit may be spoofed by ⦠â¦an attacker and this may lead to data being written to the attacker's target instead of Drillbit You can merge this pull request into a Git repository by running: $ git pull https://github.com/bitblender/drill KM-DRILL-5582 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/drill/pull/997.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #997 commit 488ebefd4a2d096c9f02cbcdfd8c6984901b3444 Author: karthik Date: 2017-10-17T23:18:45Z DRILL-5582: C++ Client: [Threat Modeling] Drillbit may be spoofed by an attacker and this may lead to data being written to the attacker's target instead of Drillbit ---