Hi Laurent, Thanks for pointing issue with <= 1.9 version Drillbit, I looked into supported_methods field and it doesn't advertise SASL support using that [1][2]. Do you mean that checking if supported_methods list is non-empty should suffice since it was introduced in 1.10 ?
For security flaw let's consider an example. Let say client is connecting to a Drillbit with authentication (and or encryption) enabled for Kerberos mechanism. The message flow will happen something like below: Good Case: * DrillClient sends Handshake Request to Drillbit * Drillbit sends the response back to DrillClient with AUTH_REQD as status * DrillClient exchange SASL handshake messages with Drillbit. * Once handshake is successful DrillClient is connected to secure Drillbit. * App using DrillClient has actually established a connection to secure Drillbit with authentication (and or encryption) and can submit it's query. Bad Case: * Step 1 as above * Step 2 as above. This message was intercepted by MITM and status was changed to SUCCESS. * Without the recent change DrillClient will not initiate SASL handshake and will return connection successful. * App using DrillClient will think it has successfully connected to secure Drillbit which is NOT the case. I think what you are pointing out is even in good case and in authentication only scenario, even if connection is successful, the messages between DrillClient and Drillbit can still be intercepted since they will be in plain text. The only way to avoid that is using encryption. But the fix was more of to avoid wrong behavior in that case too where connection should fail, instead of client just relying on server response. [1]: https://github.com/apache/drill/blob/1.11.0/exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java#L254 [2]: https://github.com/apache/drill/blob/1.11.0/exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java#L89 Thanks, Sorabh ________________________________ From: Laurent Goujon <laur...@dremio.com> Sent: Monday, October 30, 2017 5:47 PM To: dev Cc: Arina Lelchieva; sudhe...@apache.org Subject: Re: Drill SASL Forward Compatibility Regarding DRILL-5582, I see that fix as a breakage of the work to maintain compatibility for an newer client to connect to a older version of the server. Or put it differently: current (master) client does not connect anymore to a server not supporting SASL (<=1.9). Note that the client could detect if the server supports SASL as it is advertised in the supported_methods field of the BitToUserHandshake, and it would restore compatibility, but it seems the fix was done in response to a potential security flaw (although I have to admin not sure what issue it does prevent since it is still possible for a MITM to intercept all traffic between a client and a server). Laurent On Mon, Oct 30, 2017 at 5:18 PM, Sorabh Hamirwasia <shamirwa...@mapr.com> wrote: > Hi All, > > We recently added a check (as part of DRILL-5582<https://issues. > apache.org/jira/browse/DRILL-5582>) on DrillClient side to enforce that > if client showed intent for authentication and Drillbit say's it doesn't > require authentication then connection will fail with proper error message. > > > With this change we found a hidden issue w.r.t forward compatibility of > Drill. New clients running on 1.11+ authenticating to older Drillbit > running on 1.10 are treated as client running without any SASL support or > (<=1.9 version). The root cause for this issue is as follows: > > > In 1.10 a new field SASL_SUPPORT was introduced in Handshake message > between DrillCilent and Drillbit. The supported values for that field are: > > > Drill 1.10: [1] > > > enum SASL_SUPPORT { > UNKNOWN_SASL_SUPPORT = 0; > SASL_AUTH = 1; > } > > > Drill 1.11/1.12: [2] > > > enum SASL_SUPPORT { > UNKNOWN_SASL_SUPPORT = 0; > SASL_AUTH = 1; > SASL_PRIVACY = 2; > } > > A 1.11 client always has SASL_PRIVACY set in handshake. A 1.10 Drillbit > getting the message interprets the value as UNKNOWN_SASL_SUPPORT [3]. In > 1.10 Drillbit treats that value as an indication of older client < 1.10 > [4], and it will try to authenticate using the 1.9 mechanism and send the > SUCCESS/FAILURE state in Handshake Response. The 1.12 DrillClient will fail > the connection as it will expect AUTH_REQUIRED from Drillbit instead of > SUCCESS/FAILURE. > > > The fix for this issue can be to use only absence of field as indication > of client < 1.10 and if the field is present but it evaluates to > UNKNOWN_SASL_SUPPORT value then Drillbit should consider corresponding > client to be of future version at least for authentication purpose and > speak SASL protocol. > > We have to either back port above forward compatibility fix into 1.10 and > 1.11 or just fix in current release and support forward compatibility post > 1.12+. > > > Arina/Sudheesh - Please suggest if the analysis and fix sounds good and > what are the releases we should consider the fix for. Considering 1.12 > release is planned soon can we take the fix in 1.12 release ? > > > > [1]: https://github.com/apache/drill/blob/1.10.0/protocol/ > src/main/protobuf/User.proto#L70 > > [2]: https://github.com/apache/drill/blob/1.11.0/protocol/ > src/main/protobuf/User.proto#L70 > > [3]: http://androiddevblog.com/protocol-buffers-pitfall- > adding-enum-values/ > > [4]: https://github.com/apache/drill/blob/1.10.0/exec/java- > exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java#L297 > > > Thanks, > Sorabh >
