feat(security): handle SASL_LIST_MECHANISMS by server_negotiation

[levy5307]

server_negotiation receives SASL_LIST_MECHANISMS request, and reply the mechanism that it supports.

 negotiation process:

                                                  client                             server
                                                     |---   SASL_LIST_MECHANISMS    -->|
                                                     |<-- SASL_LIST_MECHANISMS_RESP ---|
                                                     |---  SASL_SELECT_MECHANISMS  --->|
                                                     |<-- SASL_SELECT_MECHANISMS_OK ---|
                                                     |                                 |
                                                     |------    SASL_INITIATE   ------>|
                                                     |                                 |
                                                     |<----     SASL_CHALLENGE    -----|
                                                     |-----      SASL_RESPONSE    ---->|
                                                     |                                 |
                                                     |              .....              |
                                                     |                                 |
                                                     |<----     SASL_CHALLENGE    -----|
                                                     |-----      SASL_RESPONSE    ---->|
                                                     |                                 |        (authentication will succeed
                                                     |                                 |        if all chanllenges passed)
                                                     | <---        SASL_SUCC      -----|
    (client won't response                           |                                 |
     if servers says ok)                             |                                 |
                                                     | ----         RPC_CALL      ---> |
                                                     | <---         RPC_RESP       ----|

[neverchanje]

Why is rpc_session holding a "server_negotiation" with no data? What gonna be inside server_negotiation?

As far as I see server_negotiation is a set of functions without data. I wonder why you cannot use the negotiation_service simply?

void negotiation_service::on_negotiation_request(negotiation_rpc rpc)
{
    dassert(!rpc.dsn_request()->io_session->is_client(),
            "only server session receive negotiation request");

    // reply SASL_AUTH_DISABLE if auth is not enable
    if (!security::FLAGS_enable_auth) {
        rpc.response().status = negotiation_status::type::SASL_AUTH_DISABLE;
        return;
    }

//    server_negotiation *srv_negotiation =
//        dynamic_cast<server_negotiation *>(rpc.dsn_request()->io_session->get_negotiation());
//    srv_negotiation->handle_request(rpc);

    // What new implementation looks like
    handle_request(rpc.dsn_request()->io_session);
}


void negotiation_service::handle_request(rpc_session session)
{
    if (session->get_negotiation_status() == negotiation_status::type::SASL_LIST_MECHANISMS) {
        on_list_mechanisms(rpc);
        return;
    }
}

[levy5307]

server_negotiation have some members. I will add them in later pull requests.

[neverchanje]

Can you list them? Just check if the overall design is good. I don't like the current design because I see server_negotiation is completely useless.

[levy5307]

There are two members in server_negotiation:
std::string _selected_mechanism is the mechanism selected by negotiation
std::unique_ptr<sasl_conn_t, sasl_deleter> _sasl_conn is the sasl connection context

[neverchanje]

Ok. You gave a quite bad name. So the class negotiation is actually "sasl_context". I think the latter is more descriptive. I don't have to check the code to know the overall design.

“server_negatiation” is "server_sasl_context".

[levy5307]

No, it's not only sasl_context. The class negotiation should also do negotiation work to select mechansim.

[neverchanje]

Suggested change

	server_negotiation *srv_negotiation =
	dynamic_cast<server_negotiation *>(rpc.dsn_request()->io_session->get_negotiation());
	server_negotiation *srv_negotiation =
	dynamic_cast<server_negotiation *>(rpc.dsn_request()->io_session->get_negotiation());

dynamic_cast introduces RTTI, which unaffordable to the critical path(rpc negotiation). Use static_cast instead.

auto srv_negotiation = static_cast<server_negotiation *>(rpc.dsn_request()->io_session->get_negotiation());

https://stackoverflow.com/questions/579887/how-expensive-is-rtti

[levy5307]

But the RTTI is used for judging whether the cast is safe or not in dynamic_cast. In 《effective C++》, dynamic_cast is recommended

[neverchanje]

It depends on whether you need runtime type check in production, and whether you can afford the penalty of RTTI. If yourself can ensure the safety, use static_cast is enough and efficient.

https://github.com/apache/kudu/blob/62123b097abf14d5a71fb3d26c0909ce2bfde9f4/src/kudu/gutil/casts.h#L69

kudu has a utility called down_cast which is dynamic_cast when it's in debug mode, and turns to static_cast when it's compiled in release mode.

[levy5307]

I can make sure the safety, but software is a huge project, and someone else may change these code.
By the way, I think the implementation of kudu is best practise.