Hi Sorabh, regarding your question:
> > 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 ? > I think we add the fix (if needed) in 1.12 and support forward compatibility post 1.12+. As far as I know Drill never claimed it is backward compatible and explicitly discourages users to use different Drill versions in a cluster as well as for client and server. Kind regards Arina On Wed, Nov 1, 2017 at 5:48 PM, Laurent Goujon <laur...@dremio.com> wrote: > My comments inline: > > On Tue, Oct 31, 2017 at 6:11 PM, Sorabh Hamirwasia <shamirwa...@mapr.com> > wrote: > > > - if using Kerberos, things are a bit different: even if a MITM > intercepts > > > > the token, only the server can decode it properly and set up encryption > so > > that only client can use it (a proxy server would not be able to decode > the > > traffic). So what you need to ensure is that you actually use Kerberos as > > the only authentication mechanism, and that the channel is encrypted (if > > channel is not encryted, see above). This is things you should do by > > configuring the client by not sending the password (no need to), only > > authorize Kerberos authentication, and verify that encryption is enabled > > (which is already done I believe). > > > > > > [Sorabh] - You are correct about the Kerberos preventing MITM which is > > what I mentioned in the last response. But this is guaranteed if client > and > > server reach to the point of SASL handshake in their communication, since > > SASL handshake exchanges all the bits related to Kerberos protocol. > Before > > that point is reached there are still few messages which is exchanged > > between DrillClient and Drillbit to detect whether server side needs > > authentication or not and what are supported mechanisms. This is where > the > > security flaw can cause client to believe Authentication is successfully > > completed (even when Drillbit/DrillClient are authenticating using > Kerberos > > with or without encryption). This is what patch for DRILL-5582 is trying > to > > address. > > > > > You still haven't answered what is the issue/security risk for the client > here: sure the client didn't authenticate with the server, but at the same > time it didn't get access to the server either... > > Also, it doesn't take very long to modify the rogue server to fake a SASL > authentication. So, now you are "authenticated", but still not to the right > server... > > > > > > > As for the other issue at hand (compatibility between 1.11 client and > 1.10 > > server), I am not sure to understand the proposed fix: is the logic you > > proposed to be added to 1.10 server? why not simply add the missing enum > > value (and that's it! no more values after that!)? > > > > > > [Sorabh] - Just adding the missing enum value in 1.10 will not help since > > in future if any other enum value is introduced then the same issue will > be > > seen. Moreover 1.10 doesn't support Encryption so that enum value should > > not be added in that release. Instead the fix is to treat the return > value > > of UNKNOWN_SASL_SERVER while retrieving messages from future client as > > valid default value and take decision based on that. > > > > But 1.10.1 could consider that a client supporting encryption also support > authentication? The code is already here in 1.10 btw: > https://github.com/apache/drill/blob/1.10.0/exec/java- > exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java#L297 > > Alternatively, you could just check that the field sasl_support is set and > not check the value alltogether. I'm not convinced you need to do some > extra logic around UNKNOWN_SASL_SERVER which would just keep people > confused (although it doesn't seem something you need to apply to 1.11 or > higher) > > > > > > > > > Thanks, > > Sorabh > > > > ________________________________ > > From: Laurent Goujon <laur...@dremio.com> > > Sent: Tuesday, October 31, 2017 5:42 PM > > To: dev > > Cc: Arina Lelchieva; sudhe...@apache.org > > Subject: Re: Drill SASL Forward Compatibility > > > > Regarding DRILL-5582 patch which broke compatibility with 1.9 version > > (which is less than a year old): > > I'm still not clear what we are trying to prevent here: stealing user > > credentials and/or data? connecting to a rogue server which could return > > corrupted data to the user? The patch gives a false sense of security > > because it prevents none of it: > > - if using password-based authentication, client is sending it in clear > in > > the initial handshake so I guess it's already game over! > > - You could configure a client to not sent password over wire by choosing > > another authentication algorithm, BUT if there's no encryption of the > > channel, any data can be intercepted and rewritten. And of course, the > > rogue server could actually run queries on behalf of the user... > > - if using Kerberos, things are a bit different: even if a MITM > intercepts > > the token, only the server can decode it properly and set up encryption > so > > that only client can use it (a proxy server would not be able to decode > the > > traffic). So what you need to ensure is that you actually use Kerberos as > > the only authentication mechanism, and that the channel is encrypted (if > > channel is not encryted, see above). This is things you should do by > > configuring the client by not sending the password (no need to), only > > authorize Kerberos authentication, and verify that encryption is enabled > > (which is already done I believe). > > > > For comparison, HTTP protocol (using it since it is one of the most used > > public protocol) has no issue with client sending an authentication > header > > to a remote server, without knowing based on the server response if > > authentication happened. > > > > As for the other issue at hand (compatibility between 1.11 client and > 1.10 > > server), I am not sure to understand the proposed fix: is the logic you > > proposed to be added to 1.10 server? why not simply add the missing enum > > value (and that's it! no more values after that!)? > > > > Laurent > > > > On Tue, Oct 31, 2017 at 3:12 PM, Sorabh Hamirwasia <shamirwa...@mapr.com > > > > wrote: > > > > > Hi Laurent, > > > > > > We are preventing 2 cases here: > > > > > > * False positive for successful authentication. Even though MITM > > > attack can happen after successful authentication, but client and > server > > > involved here has ensure successful authentication handshake has taken > > > place. With the security flaw there can always be false positive on > > client > > > side thinking authentication was successful even though that might not > be > > > the case. > > > * False positive for successful handshake with encryption > capability: > > > In cases when server is properly configured to support encryption, MITM > > > attack tweaking handshake response and making client to believe that > > after > > > successful handshake all communications will be encrypted is again > > another > > > bad false positive. > > > > > > IMHO Drill Client should be able to validate when server says that > > > authentication is completed then it's actually completed, at least > until > > > the point SASL Handshake is initiated, rather than blindly trusting it. > > > This is because if Drill client doesn't guarantees that, then it's > making > > > the support for protocol like Kerberos weaker which prevent's from any > > MITM > > > attack at handshake level. Whereas mechanisms like PLAIN are still > prone > > to > > > MITM even during SASL handshake. > > > > > > As far as forward compatibility is concerned there are few things: > > > > > > * AFAIK DrillClient & Drillbit doesn't have any concept of > supporting > > > different RPC versions across releases.They are forced to talk on same > > RPC > > > versions else connection will fail. Once we have that I think then we > > will > > > be able to clearly justify or provide the matrix of backward and > forward > > > compatibility across releases. > > > * We can put the check based on supported_methods to detect if > server > > > side supports SASL or not. But this is again just a work around not > > proper > > > solution. With workaround there can still be similar security holes as > > > PLAIN mechanism itself is prone to MITM. Given 1.9 is 3 releases behind > > > now, not sure if we still want to support that combination. > > > * At least for compatibility between Drill 1.11 client and Drill > 1.10 > > > server, I think the fix should be made which is mentioned in first > email > > of > > > this thread. > > > > > > Thanks, > > > Sorabh > > > > > > ________________________________ > > > From: Laurent Goujon <laur...@dremio.com> > > > Sent: Tuesday, October 31, 2017 9:38:13 AM > > > To: dev > > > Cc: Arina Lelchieva; sudhe...@apache.org > > > Subject: Re: Drill SASL Forward Compatibility > > > > > > See my answers inline. > > > > > > On Tue, Oct 31, 2017 at 1:40 AM, Sorabh Hamirwasia < > shamirwa...@mapr.com > > > > > > wrote: > > > > > > > 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 ? > > > > > > > > > > My bad, I thought SASL_MESSAGE was added to the list of supported > > methods, > > > but it's not. Alternatively you could check for > authenticationMechanisms > > > which should be not empty if version >= 1.10 and authentication is > turned > > > on. > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > What are we preventing in the bad case? if this is credentials or data > > > interception, a MITM could simply act as proxy to intercept all of it > > since > > > traffic is not encrypted. If we were to prevent the loss of > credentials, > > > the solution to avoid transmitting the credentials in clear in the > first > > > place. For that, we don't need a protocol change but: > > > - disable plain authentication, and use something like Kerberos or > > > CRAM-MD5/SCRAM > > > - make sure the password is not sent in the initial handshake (if using > > > Kerberos, there should no credentials to send over in the first place) > > > > > > > > > > > > > > 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. > > > > > > > > > > The "wrong" behavior was what allowed for compatibility with older > > servers > > > in the original design... > > > > > > > > > > > > > > [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 > > > > > > > > > > > > > > >