Cassandra
  1. Cassandra
  2. CASSANDRA-5545

Add SASL authentication to CQL native protocol

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Fix Version/s: 2.0 beta 1
    • Component/s: None
    • Labels:
      None

      Description

      Adding hooks for SASL authentication would make it much easier to integrate with external auth providers, such as Kerberos & NTLM.

        Activity

        Hide
        Sam Tunnicliffe added a comment - - edited

        The attached patch (against trunk) adds new message types for SASL negotiation between CQL client & server. In this patch, SaslAuthBridge represents the interface between SASL & IAuthencator, while
        the helper class org.apache.cassandra.transport.sasl.Sasl acts as a registry of which SaslAuthBridge implementation goes with which IAuthenticator. PasswordAuthenticator, and any other custom IAuthenticator implementation which receives a username/password pair via Credentials message or thrift login() call, can be associated with PlainTextSaslAuthBridge. This is done automatically for PasswordAuthenticator, so there should be no server side changes for clusters without custom authentication.

        Implementors of custom authenticators which do not receive credentials in the same way & format as PasswordAuthenticator will need to provide their own SaslAuthBridge to extract the credentials from a SaslServer instance. Depending on the format required by the IAuthenticaor, this may involve creating or wrapping a SaslServer implementation. See AbstractSaslServer/AbstractSaslAuthBridge & the PlainText* implementations for an example.

        Show
        Sam Tunnicliffe added a comment - - edited The attached patch (against trunk) adds new message types for SASL negotiation between CQL client & server. In this patch, SaslAuthBridge represents the interface between SASL & IAuthencator, while the helper class org.apache.cassandra.transport.sasl.Sasl acts as a registry of which SaslAuthBridge implementation goes with which IAuthenticator. PasswordAuthenticator, and any other custom IAuthenticator implementation which receives a username/password pair via Credentials message or thrift login() call, can be associated with PlainTextSaslAuthBridge. This is done automatically for PasswordAuthenticator, so there should be no server side changes for clusters without custom authentication. Implementors of custom authenticators which do not receive credentials in the same way & format as PasswordAuthenticator will need to provide their own SaslAuthBridge to extract the credentials from a SaslServer instance. Depending on the format required by the IAuthenticaor, this may involve creating or wrapping a SaslServer implementation. See AbstractSaslServer/AbstractSaslAuthBridge & the PlainText* implementations for an example.
        Hide
        Sylvain Lebresne added a comment -

        We initially suggested to add SASL indeed but skipped it initially mainly for lack of time/effort. So I definitively agree that switching to SASL exchanges instead of our current custom one way message for the protocol v2 is a good idea.

        On the patch, the principle looks good to me, but I would suggest going with a much simpler interface on our end and let people deal with all the subtlety of the java SASL API. After all, the only authentication we provide out the box is plain text authentication. Schematically, all that "supporting" SASL require is an interface to issue byte[] challenges from byte[] client responses. So I would suggest simply adding 2 new methods to IAuthenticator:

        SASLAuthenticator newAuthenticator();
        

        where SASLAuthenticator would be something like:

        public interface SASLAuthenticator
        {
            public String getMechanismName();
            public byte[] evaluateResponse(byte[] clientResponse);
        }
        

        and the current plain text authenticator could add a very simple implementation of that (without creating a full blown java.security.sasl.SaslServer).

        This will change the IAuthenticator interface to add a new method, but I think supporting a new method is not a lot to ask for people having custom authenticator today, especially since if you don't care about the binary protocol SASL authentication, you can just have that method return null to start with.

        Show
        Sylvain Lebresne added a comment - We initially suggested to add SASL indeed but skipped it initially mainly for lack of time/effort. So I definitively agree that switching to SASL exchanges instead of our current custom one way message for the protocol v2 is a good idea. On the patch, the principle looks good to me, but I would suggest going with a much simpler interface on our end and let people deal with all the subtlety of the java SASL API. After all, the only authentication we provide out the box is plain text authentication. Schematically, all that "supporting" SASL require is an interface to issue byte[] challenges from byte[] client responses. So I would suggest simply adding 2 new methods to IAuthenticator: SASLAuthenticator newAuthenticator(); where SASLAuthenticator would be something like: public interface SASLAuthenticator { public String getMechanismName(); public byte[] evaluateResponse(byte[] clientResponse); } and the current plain text authenticator could add a very simple implementation of that (without creating a full blown java.security.sasl.SaslServer). This will change the IAuthenticator interface to add a new method, but I think supporting a new method is not a lot to ask for people having custom authenticator today, especially since if you don't care about the binary protocol SASL authentication, you can just have that method return null to start with.
        Hide
        Sam Tunnicliffe added a comment -

        As per the previous comment, the new patch extends IAuthenticator & adds a SaslAuthenticator interface, although this is very slightly different to the one Sylvain describes. It also adds a very simple SaslAuthenticator implementation to be used by PasswordAuthenticator. Also, a check in o.a.c.transport.Server.run() so that we don't inadvertedly start the server if it requires authentication but the configured IAuthenticator doesn't provide a SaslAuthenticator.

        Show
        Sam Tunnicliffe added a comment - As per the previous comment, the new patch extends IAuthenticator & adds a SaslAuthenticator interface, although this is very slightly different to the one Sylvain describes. It also adds a very simple SaslAuthenticator implementation to be used by PasswordAuthenticator. Also, a check in o.a.c.transport.Server.run() so that we don't inadvertedly start the server if it requires authentication but the configured IAuthenticator doesn't provide a SaslAuthenticator.
        Hide
        Aleksey Yeschenko added a comment -

        Can we do this without alterting IAuthenticator? Why not make SASLAuthenticator extend IAuthenticator (and make Legacy and Password authenticators extend SASLAuthenticator instead of IAuthenticator). And replace that null check with instanceof?

        I don't like this trend of modifying IAuthenticator with every major release. Last time it was necessary, but this time we can avoid it.

        Show
        Aleksey Yeschenko added a comment - Can we do this without alterting IAuthenticator? Why not make SASLAuthenticator extend IAuthenticator (and make Legacy and Password authenticators extend SASLAuthenticator instead of IAuthenticator). And replace that null check with instanceof? I don't like this trend of modifying IAuthenticator with every major release. Last time it was necessary, but this time we can avoid it.
        Hide
        Sylvain Lebresne added a comment -

        It feels weird to have SaslAuthenticator return a string map and go through the "old style" authenticate method since true SASL mechanism will do the actual authentication during the evaluateResponse() call (and so may have to hack the return of getCredentials() to make their authenticate() method work properly). Instead, I'd prefer changing the SaslAuthenticator API to:

        public interface SaslAuthenticator
        {
            public byte[] evaluateResponse(byte[] clientResponse) throws AuthenticationException;
            public boolean isComplete();
            public AuthenticatedUser getAuthenticatedUser();
        }
        

        We would then change ClientState.login to just take an AuthenticatedUser parameters, and the call to authenticate() would be moved to the thrift sid (and in CredentialsMessage).

        That way authenticate() is a thrift/protocol v1 only method and can be made to throw an error for authenticator that don't care about that (of course, in the case of PlainTextSaslAuthenticator, it can just call authenticate internally).

        Other small remarks/nits:

        • We really need authentication to throw AuthenticationException (as in my suggestion above), not SaslException since the later is not known by the protocol (which will send it to the client as a "server error" (i.e. a bug server side), which is not the case).
        • We need to refuse SASL_RESPONSE messages in v1 and AUTHENTICATE messages in v2 (just throwing a ProtocolException in their respective decode method would be fine).
        • Might be worth reseting the saslAuthenticator to null in ServerConnection once authentication is comple to have it garbage collected?
        • Nit: few minor code style related fix (indentation for try in SaslResponse)
        • Nit: I'd have move SaslAuthenticator and PlainTextSaslAuthenticator to the org.apache.cassandra.auth package directly (and would have make PlainTextSaslAuthenticator a private static inner class in PasswordAuthenticator in fact).
        Show
        Sylvain Lebresne added a comment - It feels weird to have SaslAuthenticator return a string map and go through the "old style" authenticate method since true SASL mechanism will do the actual authentication during the evaluateResponse() call (and so may have to hack the return of getCredentials() to make their authenticate() method work properly). Instead, I'd prefer changing the SaslAuthenticator API to: public interface SaslAuthenticator { public byte[] evaluateResponse(byte[] clientResponse) throws AuthenticationException; public boolean isComplete(); public AuthenticatedUser getAuthenticatedUser(); } We would then change ClientState.login to just take an AuthenticatedUser parameters, and the call to authenticate() would be moved to the thrift sid (and in CredentialsMessage). That way authenticate() is a thrift/protocol v1 only method and can be made to throw an error for authenticator that don't care about that (of course, in the case of PlainTextSaslAuthenticator, it can just call authenticate internally). Other small remarks/nits: We really need authentication to throw AuthenticationException (as in my suggestion above), not SaslException since the later is not known by the protocol (which will send it to the client as a "server error" (i.e. a bug server side), which is not the case). We need to refuse SASL_RESPONSE messages in v1 and AUTHENTICATE messages in v2 (just throwing a ProtocolException in their respective decode method would be fine). Might be worth reseting the saslAuthenticator to null in ServerConnection once authentication is comple to have it garbage collected? Nit: few minor code style related fix (indentation for try in SaslResponse) Nit: I'd have move SaslAuthenticator and PlainTextSaslAuthenticator to the org.apache.cassandra.auth package directly (and would have make PlainTextSaslAuthenticator a private static inner class in PasswordAuthenticator in fact).
        Hide
        Sylvain Lebresne added a comment -

        Why not make SASLAuthenticator extend IAuthenticator

        The problem is that SASLAuthenticator is intrinsically stateful, i.e. we need a new for each new authentication. And IAuthenticator has methods like setup() that somewhat suggest there is just one IAuthenticator.

        Show
        Sylvain Lebresne added a comment - Why not make SASLAuthenticator extend IAuthenticator The problem is that SASLAuthenticator is intrinsically stateful, i.e. we need a new for each new authentication. And IAuthenticator has methods like setup() that somewhat suggest there is just one IAuthenticator.
        Hide
        Aleksey Yeschenko added a comment -

        That still doesn't mean that IAuthenticator modification is necessary. At worst this means two new interface: one extending IAuthenticator, with newAuthenticator(), and another one - SaslAuthenticator from the patch (that one should be nested inside ISaslAwareAuthenticator).

        Show
        Aleksey Yeschenko added a comment - That still doesn't mean that IAuthenticator modification is necessary. At worst this means two new interface: one extending IAuthenticator, with newAuthenticator(), and another one - SaslAuthenticator from the patch (that one should be nested inside ISaslAwareAuthenticator).
        Hide
        Sylvain Lebresne added a comment -

        Fair enough, I'm fine doing that.

        Show
        Sylvain Lebresne added a comment - Fair enough, I'm fine doing that.
        Hide
        Sam Tunnicliffe added a comment -

        Version 3 patch with comments addressed

        Show
        Sam Tunnicliffe added a comment - Version 3 patch with comments addressed
        Hide
        Sylvain Lebresne added a comment -

        Alright, lgtm. Committed (after rebase and having updated the protocol spec), thanks.

        Show
        Sylvain Lebresne added a comment - Alright, lgtm. Committed (after rebase and having updated the protocol spec), thanks.
        Hide
        Sylvain Lebresne added a comment -

        Actually, I've just realized that we were throwing away the last challenge of the server (once authentication is complete), but SASL requires that we sent it to the client since it may contains final information required by the client to finalize authentication on its size.

        So I think for completeness sake we need a new AUTH_SUCCESS message that ships that last information rather than just a READY message. My bad for suggesting otherwise.

        Anyway, I've committed the patch that I attach here for the record that adds this AUTH_SUCCESS message. The patch also allow tokens to be > 64k (it uses an int instead of a short for the size) because that's what I wrote in the spec, and while I doubt any authenticator would need more than 64K tokens, there is no point in risking it in that case.

        If someone disagrees with that last patch, please feel free to voice yourself.

        Show
        Sylvain Lebresne added a comment - Actually, I've just realized that we were throwing away the last challenge of the server (once authentication is complete), but SASL requires that we sent it to the client since it may contains final information required by the client to finalize authentication on its size. So I think for completeness sake we need a new AUTH_SUCCESS message that ships that last information rather than just a READY message. My bad for suggesting otherwise. Anyway, I've committed the patch that I attach here for the record that adds this AUTH_SUCCESS message. The patch also allow tokens to be > 64k (it uses an int instead of a short for the size) because that's what I wrote in the spec, and while I doubt any authenticator would need more than 64K tokens, there is no point in risking it in that case. If someone disagrees with that last patch, please feel free to voice yourself.
        Hide
        Sam Tunnicliffe added a comment -

        That final patch broke the build. I've attached 2 further patches:

        0001 fixes compilation errors from 0001-Adds-AUTH_SUCCESS...
        0002 renames SASL_CHALLENGE & SASL_RESPONSE to be consistent with AUTH_SUCCESS

        Show
        Sam Tunnicliffe added a comment - That final patch broke the build. I've attached 2 further patches: 0001 fixes compilation errors from 0001-Adds-AUTH_SUCCESS... 0002 renames SASL_CHALLENGE & SASL_RESPONSE to be consistent with AUTH_SUCCESS
        Hide
        Sylvain Lebresne added a comment -

        My bad for committing without building. Fix committed in any way, thanks.

        Show
        Sylvain Lebresne added a comment - My bad for committing without building. Fix committed in any way, thanks.

          People

          • Assignee:
            Sam Tunnicliffe
            Reporter:
            Sam Tunnicliffe
            Reviewer:
            Sylvain Lebresne
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development