drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Arina Yelchiyeva <arina.yelchiy...@gmail.com>
Subject Re: Drill SASL Forward Compatibility
Date Wed, 01 Nov 2017 15:55:34 GMT
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 <laurent@dremio.com> wrote:

> My comments inline:
>
> On Tue, Oct 31, 2017 at 6:11 PM, Sorabh Hamirwasia <shamirwasia@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 <laurent@dremio.com>
> > Sent: Tuesday, October 31, 2017 5:42 PM
> > To: dev
> > Cc: Arina Lelchieva; sudheesh@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 <shamirwasia@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 <laurent@dremio.com>
> > > Sent: Tuesday, October 31, 2017 9:38:13 AM
> > > To: dev
> > > Cc: Arina Lelchieva; sudheesh@apache.org
> > > Subject: Re: Drill SASL Forward Compatibility
> > >
> > > See my answers inline.
> > >
> > > On Tue, Oct 31, 2017 at 1:40 AM, Sorabh Hamirwasia <
> shamirwasia@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 <laurent@dremio.com>
> > > > Sent: Monday, October 30, 2017 5:47 PM
> > > > To: dev
> > > > Cc: Arina Lelchieva; sudheesh@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 <
> > shamirwasia@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
> > > > >
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message