Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add P2P security options #1764

Open
wants to merge 27 commits into
base: develop
from

Conversation

@jmjatlanta
Copy link
Contributor

commented May 17, 2019

Fixes #659

This PR adds 3 command line options to witness_node:

  1. 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.
  2. advertise-peer-algorithm determines how peers are selected to be advertised.
  3. advertise-peer-listworks in conjuction with some of the peer algorithms.

The "accept-incoming-connections" is an existing field in the node configuration file, now accessible from the command line.

The peer algorithms that can be used are:

  1. nothing which will respond to the caller with an empty list
  2. list which will respond with the list provided by advertise-peer-list
  3. exclude_list which will respond with all nodes except those in advertise-peer-list
  4. if no peer algorithm is provided, all nodes are advertised as they were before this enhancement.

@abitmore abitmore added this to the 3.2.0 - Feature Release milestone May 17, 2019

@abitmore abitmore added this to In development in Feature Release (3.2.0) via automation May 17, 2019

@pmconrad
Copy link
Contributor

left a comment

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 Show resolved Hide resolved
@jmjatlanta

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

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

This comment has been minimized.

Copy link
Member

commented May 20, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

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 Outdated Show resolved Hide resolved
@abitmore

This comment has been minimized.

Copy link
Member

commented May 20, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

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

This comment has been minimized.

Copy link
Member

commented May 21, 2019

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

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

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!

@pmconrad pmconrad added this to In development in Feature Release (3.3.0) via automation Jun 19, 2019

@pmconrad pmconrad removed this from In development in Feature Release (3.2.0) Jun 19, 2019

@jmjatlanta jmjatlanta force-pushed the jmj_659c branch from 659a8ae to 23bba63 Jul 4, 2019

@abitmore
Copy link
Member

left a comment

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 Outdated Show resolved Hide resolved
libraries/net/node_impl.hxx Outdated Show resolved Hide resolved
libraries/net/node_impl.hxx Outdated Show resolved Hide resolved
libraries/net/node_impl.hxx Outdated Show resolved Hide resolved
@jmjatlanta

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

@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.

@jmjatlanta jmjatlanta requested a review from pmconrad Jul 5, 2019

libraries/net/node.cpp Outdated Show resolved Hide resolved
libraries/net/include/graphene/net/node.hpp Outdated Show resolved Hide resolved
libraries/net/include/graphene/net/node.hpp Outdated Show resolved Hide resolved
libraries/net/include/graphene/net/node.hpp Outdated Show resolved Hide resolved
libraries/net/node.cpp Outdated Show resolved Hide resolved
libraries/net/include/graphene/net/node.hpp Outdated Show resolved Hide resolved
libraries/net/node.cpp Outdated Show resolved Hide resolved
tests/common/genesis_file_util.hpp Show resolved Hide resolved
libraries/net/node.cpp Outdated Show resolved Hide resolved
libraries/app/application.cpp Outdated Show resolved Hide resolved
libraries/app/application.cpp Outdated Show resolved Hide resolved
libraries/net/include/graphene/net/node.hpp Outdated Show resolved Hide resolved
libraries/net/node.cpp Show resolved Hide resolved
libraries/net/node.cpp Outdated Show resolved Hide resolved
libraries/net/node.cpp Outdated Show resolved Hide resolved
libraries/net/node.cpp Show resolved Hide resolved
libraries/net/node_impl.hxx Outdated Show resolved Hide resolved
libraries/net/include/graphene/net/node.hpp Outdated Show resolved Hide resolved
libraries/net/node_impl.hxx Outdated Show resolved Hide resolved

@ryanRfox ryanRfox moved this from In development to In testing in Feature Release (3.3.0) Jul 16, 2019

libraries/net/node.cpp Outdated Show resolved Hide resolved
@abitmore

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

I just noticed a thing: if a peer is in our exclude list, we shouldn't ask another peer to check this peer's firewall status, otherwise we'll expose this peer's address. Is this the current behavior?

@jmjatlanta jmjatlanta force-pushed the jmj_659c branch from 328336d to a0fc692 Aug 26, 2019

@jmjatlanta

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

Note: the conclusion was to remove firewall check and current_connections_request/response.

I am seeking clarification. Reading the above with little context makes it sound like the ability for nodes to request a firewall check or ask for a list of connected nodes is something we no longer want. I doubt we want to make such a change.

Removing the ability to request a firewall check is not as big of a deal, although it is an API change.

Removing the ability to request current connections would change the "p2p" to "client/server". I know that is not what we want, so I must not be understanding correctly.

@jmjatlanta

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2019

As I imagined, I misunderstood. Thecurrent_connections stuff is unused. P2P uses address_info instead. Removed the firewall and current_connections code.

A good reference doc: https://github.com/bitshares/bitshares-core/wiki/P2P-network-protocol

@abitmore

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

@jmjatlanta you directly removed the message types, I think it would cause the new nodes to be incompatible to old nodes which may lead to unintended disconnections although I'm not sure, since firewall check messages are used among the nodes, while current_connection messages are probably not used so far. Worse, if we want to add new message types in the future, the enum values will be reused unexpectedly. IMHO the correct solution should be:

  • keep the definition of the messages
  • keep the enums
  • keep the on_message call back functions
  • ignore the messages in the on_message functions.
@abitmore

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

After a hard fork, when we're sure that all nodes are running the new code so those messages won't be transmitted in the network, we can do the cleanups, but not now.

@jmjatlanta

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

@jmjatlanta you directly removed the message types, I think it would cause the new nodes to be incompatible to old nodes which may lead to unintended disconnections

Yes, I believe you are right. I had some misconceptions about what happens when someone makes a call we don't understand. I will roll back and redo.

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

Node currently ignores unknown messages in the range 5000..5099, so it would be OK to rip out the code completely. This should be equivalent to making the on_xxx_message handlers empty.
It is correct that it would be dangerous if we re-used the numbers before the next consensus upgrade, but I think that's rather unlikely. Perhaps keep the enum values only, with a comment like // until bitshares-core-0.3.x.

@jmjatlanta jmjatlanta force-pushed the jmj_659c branch from 4d64d2b to 636c866 Aug 30, 2019

@jmjatlanta

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

I took the safer approach and just removed the guts of the on_message calls. If all agree that the aggressive approach is safe, I will do another round of elimination. Please look carefully at the network_mapper code. I am unsure if it is still used, so unsure if we want to do more (i.e. renaming the collection, examining the diagrams).

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

Wrt network mapper I think the changes are ok. Don't put too much effort in it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.