Add P2P security options by jmjatlanta · Pull Request #1764 · bitshares/bitshares-core

jmjatlanta · 2019-05-17T17:37:01Z

Fixes #659

This PR adds startup options (available via both command line and the config.ini configuration file) to witness_node:

p2p-accept-incoming-connections will allow peers to request a connection to your node (default is true). Set to false, your node will not listen for incoming connections. The "accept-incoming-connections" is an existing field in the node configuration file (p2p/node_config.json), now accessible from the command line and the config.ini configuration file.
p2p-inbound-endpoint, used to specify the node's "external" IP address and listening port when it is behind DNAT or a reverse proxy.
p2p-connect-to-new-peers will allow the node to connect to new peers advertised by other peers (default is true). Set to false, the node will ignore all peer advertisements.
p2p-advertise-peer-algorithm determines how peers are selected to be advertised.
p2p-advertise-peer-endpoint and p2p-exclude-peer-endpoint work in conjunction with some of the peer algorithms.

The peer algorithms that can be used are:

nothing which will respond to the requesting peer with an empty list
list which will respond with a list of connected peers which are also in the list provided by p2p-advertise-peer-endpoint
exclude_list which will respond with a list of connected peers which are not in the list provided by p2p-exclude-peer-endpoint
all, or any other value, or if no peer algorithm is provided, all connected peers are advertised as they were before this enhancement.

Other changes and improvements:

Node will listen to the default port (1776) by default, rather than a random port. If p2p-endpoint option is not specified, when the default port is unavailable, the node will listen on a random port.
Node no longer performs firewall check requested by other peers, nor ask other peers to perform firewall check for itself or another peer. The node will perform firewall check / detection on its own on incoming peers.
Node will only accept an address_message if it has just requested one
Node only handles at most 200 addresses for each address_message
Added some gate-keeping code, to disconnect or ignore when received unexpected messages
Node will advertise other peers' inbound endpoints but not necessarily the remote endpoints of the current connections. If a peer's inbound port is 0, its address will still be advertised.
If node is not connected to any peer, it rechecks its seed node list every 5 minutes; otherwise it rechecks the seed node list every 1 hour (was always recheck every 3 hours)
Node will try to guess and verify the inbound peers' inbound (listening) endpoints
Peer's number_of_failed_connection_attempts will be halved on successful outbound connection
Refactored & optimized the network_mapper program
Added fc::ip::address::is_loopback_address() (127.*.*.*), fixed is_public_address() to detect loopback addresses

pmconrad

AFAICS this resolves only the second point in #659 .
The first point (connect only to trusted peers) should be addressed, too.

libraries/app/application.cpp

jmjatlanta · 2019-05-20T11:47:21Z

The first point (connect only to trusted peers) should be addressed, too.

I was thinking that listing seed_nodes, along with turning off accept_incoming_connections would keep you from connecting to others. But I forgot about the (possibly) existing connection database. So I will have to do something about that. Perhaps a parameter "clear-connection-db"

Other than the connection database, is there another way that connections are built? Do peers automatically ask each other for peers and attempt to connect? Or must they be prompted? I haven't looked, but will do so.

abitmore · 2019-05-20T13:24:41Z

Do peers automatically ask each other for peers and attempt to connect? Or must they be prompted?

Peers do talk to each other about possible new peers and try to connect to each other. I don't remember whether one peer will ask other peers for more peers though.

jmjatlanta · 2019-05-20T17:37:21Z

I can either add code to clear the database, or add code to not use it. Any opinion on which is better? I am thinking clearing the database is better. Who wants a stale database sitting around?

Or perhaps someone has a better idea to allow a node operator to only connect to the nodes he wants to.

libraries/app/application.cpp

abitmore · 2019-05-20T18:18:12Z

The program will add new entries to the in-memory database when running, but not only load data when starting, so only clearing on start doesn't make much sense.

Actually we're digging into the details now. I think you can find more from the code.

IIRC, according to the p2p protocol, your connected peers may

tell you that there is a new peer, so you can connect to it when you want,
- in this case your node should ignore it (don't add the new peer to internal db);
ask you to connect to another peer to detect if it's firewalled,
- in this case, if you only connected to trusted nodes, I think the trusted nodes should not ask you to do such job;
ask you to send them a peer list,
- trusted nodes should not do this as well.

jmjatlanta · 2019-05-20T20:08:22Z

IIRC, according to the p2p protocol, your connected peers may
* tell you that there is a new peer, so you can connect to it when you want,

I will add code to not connect if accept-incoming-connections is false.

* ask you to connect to another peer to detect if it's firewalled,

I will have to research if a response is required, or if I can simply ignore the request without consequences.

* ask you to send them a peer list,

That can now be controlled by the advertise-peer-list parameter.

ToDo:

Unit testing of on_address_message
Add logic to optionally ignore previous db

abitmore · 2019-05-21T10:20:01Z

IIRC, according to the p2p protocol, your connected peers may
* tell you that there is a new peer, so you can connect to it when you want,
I will add code to not connect if accept-incoming-connections is false.

IMHO, these are outgoing connections but not incoming connections.

pmconrad · 2019-05-21T14:11:43Z

I will add code to not connect if accept-incoming-connections is false.

Please use a different flag. These should be two distinct settings. (A node operator might want to have his own internal network of interconnected nodes that only connect to each other, and to some trusted public nodes.)

jmjatlanta · 2019-05-21T14:32:49Z

I will add code to not connect if accept-incoming-connections is false.

Please use a different flag. These should be two distinct settings. (A node operator might want to have his own internal network of interconnected nodes that only connect to each other, and to some trusted public nodes.)

How about a new parameter:

accept-connection-suggestions connect to nodes suggested by other nodes. Defaults to true.

jmjatlanta · 2019-05-21T14:42:02Z

I began to think about dropped connections to nodes. As it stands now, the parameters could be set up to where they will not get reconnected. I believe that if seed nodes are passed, and allow_incoming_connections is false, and accept-connection-suggestions is false, a reconnect cannot happen.

I believe that in such a situation, we must store the seed nodes (in memory?), and sort-of "replace" the currently implemented database of addresses with that list. That way reconnects (or perhaps initial connections) can happen if they are on the list.

As often happens, the implementation goes deeper than I expected. To make things clear, I will attempt to spec-out the parameters as I see how they should work, and let others review and adjust.

jmjatlanta · 2019-05-21T15:54:53Z

Posted a spec here: #659 (comment)

Please comment there, we'll hammer out the details, and then I'll attempt to implement. Thanks all for the input!

abitmore

At a glance the code looks good. However the coding style is not the best, sometime indentation is 2, sometimes it's 3, sometimes it's 4.

tests/common/genesis_file_util.hpp

libraries/net/node_impl.hxx

jmjatlanta · 2019-07-05T14:43:05Z

@abitmore thank you for the critiques. My apologies for the sloppy spacing. I believe I have corrected that and improved the readability of the code. No real functionality changes, the biggest change here is the moving of the node utility functions into its own file. Please scrutinize.