messaging_service: gossiper verbs should be encrypted regardless of topology knowledge

[kbr-]

When a node creates a connection to another node in messaging_service::get_rpc_client it decides whether the connection should be encrypted by calculating a must_encrypt boolean (

scylladb/message/messaging_service.cc

Line 718 in d85f731

auto must_encrypt = [&] {

). If inter-DC/rack encryption is enabled, we will not encrypt connections between nodes in the same DC/rack. However, a node might now know the topology of another node yet. In that case it will encrypt the connection even though they might be in the same DC/rack; there's no way to check the latter. (and the connection will eventually be dropped and replaced with a new connection once topology is known)

Similarly, when a node receives a connection attempt on unencrypted port and inter/DC-rack encryption is enabled, the node will verify whether the connection attempt comes from an address in the same DC-rack (messaging_service::do_start_listen;

scylladb/message/messaging_service.cc

Line 309 in d85f731

so.filter_connection = [this](const seastar::socket_address& caddr) {

). If not, it will reject the attempt. When writing this issue, on master branch, if the node does not have the topology of the sender's address, so it's not able to verify whether it's the same DC/rack, it will call on_internal_error (

scylladb/locator/token_metadata.cc

Line 1341 in d85f731

on_internal_error(tlogger, format("Node {} is not in topology", ep));

; which is another problem that needs fixing; we should just drop these attempts if we don't know the topology).

As explained in #11942 (comment): there is no guarantee that if a node X knows the topology of node Y, Y also knows the topology of X. So it may happen that X attempts unencrypted communication with Y, but Y will reject it. I obtained a situation where a node called on_internal_error (described in the previous paragraph) because it received unencrypted communication attempt from a node whose topology was yet unknown when trying to fix #11780, with #11942.

We need to guarantee that Y can eventually learn the topology of X. But we cannot give such guarantee if Y will reject any communication attempts from X! Perhaps Y could learn the topology of X from other nodes. But can we guarantee that Y can receive communication from other node? What if it told other nodes its topology, and then there was a failure which caused it not to learn the topology of others (dropped messages, a crash + restart before the knowledge was persisted etc.)?

The system is hard to reason about in the current state, and it's hard to prove if any such guarantees are given.

At some point there was the following piece of code in the definition of must_encrypt in get_rpc_client:

        // if we have dc/rack encryption but this is gossip, we should
        // use tls anyway, to avoid having mismatched ideas on which 
        // group we/client are in. 
        if (verb >= messaging_verb::GOSSIP_DIGEST_SYN && verb <= messaging_verb::GOSSIP_SHUTDOWN) {
            return true;
        }

i.e., when we create an RPC client due to a gossiper verb, we encrypt the communication (even though the nodes may live in the same DC/rack).

Later it was removed (7bdad47).

However, the check may be necessary for liveness: it's easy to see that it guarantees that every node can eventually learn every other node's topology through gossiping; nodes accept encrypted communication, they only reject unencrypted communication if they cannot verify that it's inter-DC/rack.

It's also not a performance problem to encrypt the RPC client used for gossiper verbs, because it doesn't contain any hot/data path verbs, only rare metadata verbs:

static constexpr unsigned do_get_rpc_client_idx(messaging_verb verb) {
    switch (verb) {
    case messaging_verb::GOSSIP_DIGEST_SYN:
    case messaging_verb::GOSSIP_DIGEST_ACK:
    case messaging_verb::GOSSIP_DIGEST_ACK2:
    case messaging_verb::GOSSIP_SHUTDOWN:
    case messaging_verb::GOSSIP_ECHO:
    case messaging_verb::GOSSIP_GET_ENDPOINT_STATES:
    case messaging_verb::GET_SCHEMA_VERSION:
    case messaging_verb::GROUP0_PEER_EXCHANGE:
    case messaging_verb::GROUP0_MODIFY_CONFIG:
    case messaging_verb::GET_GROUP0_UPGRADE_STATE:
        return 0;

Finally, the check combined with #11942 will let me fix #11780.

[kbr-]

cc @xemul

[avikivity]

@kbr-scylla looks like a real problem, but we've never seen it in the field. How come? Is it a regression? If so, from which version?

[kbr-scylla]

I only remember that this area of code was being refactored&changed quite often on master at that time (@xemul 's topology refactors etc.). I don't recall there being an actual problem in a release.

[avikivity]

Not backporting per above.