[GitHub] drill pull request #997: DRILL-5582: C++ Client: [Threat Modeling] Drillbit ...

[sohami]

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

[sohami]

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

[parthchandra]

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

[parthchandra]

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

[bitblender]

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




---