Issue #659 - add P2P security options

[jmjatlanta]

This PR for Issue #659 adds 2 command line options to witness_node:

1. The parameter 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. The parameter disable-peer-advertising will respond to a request for your list of peers with an empty list.

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

The "disable-peer-advertising" is an existing field, now accessible from the command line. NOTE: This could be used as an attack vector, so you may not want to use this. A better option is to provide an option to randomize the returned list of nodes.

Outstanding tasks:

• Add tests to verify these parameters work as designed
• Randomize results returned when nodes ask for list of nodes

[abitmore]

Looks fine so far.

[jmjatlanta]

I am thinking some may want "disable-peer-advertising", so I'm thinking of keeping that there, and adding "advertise-only-seeds"

IMHO a randomized list of nodes could be an attack vector (ask twice, get different results, you know they're randomizing). But sending back the list of embedded seed nodes could mean (1) they are selectively advertising or (2) they just fired up their node or (3) they are not allowing incoming connections and are white-listing.

A better option than randomizing? I look forward to hearing your input.

[abitmore]

To be honest I don't know what's the best approach at this moment.

[jmjatlanta]

I have implemented a strategy in line with this comment: #659 (comment)

I am having difficulty with the "list" algorithm, as we currently don't store the configuration strings of seed nodes and their related fc::ip::endpoint together.

A first option is to store the URL string with the fc::ip::endpoint. If we did, we could iterate through the endpoints, and only advertise the connections that are in the seed list. But that means some extra storage in the database. And I'm not sure we want to do that.

A second option is to guess. If the string URL in the seed list matches the fc::ip::endpoint exactly, add it to the list. Otherwise, do not. This will not work for advertising a node with multiple endpoints.

A third option is to not implement the list algorithm right now.

I would like the opinions of others on the goods and bads of these options, or other options that I haven't thought of.

[pmconrad]

I understood the "list" option to be an independently configurable list, not the list of seed nodes used by the node. I think the purpose of #659 is to hide information, and the list of actual seeds is most likely the thing you want to hide.

[jmjatlanta]

Thank you for the clarification. So the "list" option would require a separate parameter that had nodes you want to connect to (for the included statistics), but separate from "seed-nodes"? That makes sense, I just want to verify.

[pmconrad]

Separate parameter, yes. But not necessarily "to connect to", only for advertising them.

[pmconrad]

@jmjatlanta what's your status here?

[jmjatlanta]

Sorry. This has fallen off my radar. It is back on it now. I will provide an update soon. Thanks for the reminder.

[jmjatlanta]

The listalgorithm now grabs all the advertise-peer-list parameters and only advertises those. Note: What is passed in is validated. So invalid URLs will be logged then ignored.

In this example, remote P2P nodes would receive 2 peers when sending an address_request_message:

witness_node --advertise-peer-algorithm=list --advertise-peer-list=127.0.0.1:8090 --advertise-peer-list=10.0.1.1:8090

I believe the mechanism to be sound, but the interface could use some work. Would these values normally be in the .ini file, or passed on the command line? What would be the preferred way to build this list? Parse a delimited string or individually as above?

[abitmore]

I believe the mechanism to be sound, but the interface could use some work. Would these values normally be in the .ini file, or passed on the command line?

Both, see https://github.com/bitshares/bitshares-core/blob/test-2.0.190225/libraries/app/application.cpp#L966-L976.

What would be the preferred way to build this list? Parse a delimited string or individually as above?

We have both in use, see https://github.com/bitshares/bitshares-core/blob/test-2.0.190225/libraries/app/application.cpp#L125-L187.

[pmconrad]

Please don't c&p code from other branches for conflict resolution. Your branch is now inconsistent in that it adds options that are not used anywhere.

For work in progress it is cleaner to rebase on current develop (or hardfork).
If code has already been partially reviewed then merging the target branch is better.
Cherry-picking changes can also be an option in specific cases.

[jmjatlanta]

Please don't c&p code from other branches for conflict resolution. Your branch is now inconsistent in that it adds options that are not used anywhere.

Sorry for making this ticket a tutorial, but I am afraid I do not understand your comment. If I break your comment into 3 options, I chose option 2. Are you saying I shouldn't have? And that I should have used option 1 instead? Thanks in advance for the clarification.

[abitmore]

@jmjatlanta the last commit in this PR is a merge of develop branch, so perhaps leaving the word "merge" in the commit comment would make it clearer for reviewers; also, it's best to describe what conflicts are resolved and how you've resolved them in the commit comment. That said, don't make a merge looks like a "c&p". ;)

[pmconrad]

PR build fails in travis with compile errors. You may have to rebase again.

[pmconrad]

make chain_test still failing in PR.

[jmjatlanta]

make chain_test still failing in PR.

Error is

/home/travis/build/bitshares/bitshares-core/tests/tests/p2p_node_tests.cpp: In member function ‘virtual void test_peer::send_message(const graphene::net::message&, size_t)’:
/home/travis/build/bitshares/bitshares-core/tests/tests/p2p_node_tests.cpp:178:58: error: invalid static_cast from type ‘const little_uint32_buf_t {aka const boost::endian::endian_buffer<(boost::endian::order)1, unsigned int, 32ul>}’ to type ‘uint32_t {aka unsigned int}’
       if ( static_cast<uint32_t>(message_to_send.msg_type) == graphene::net::address_message::type )
                                                          ^

I have tried several combinations, but am stumped on how boost::endian has gotten involved. Still researching.

[abitmore]

The newest commit history looks like a mess, IMHO better rebase :)

Use .value() to convert little_uint32_buf_t to uint32, see https://github.com/bitshares/bitshares-core/pull/1714/files#diff-76b01416e354adac72143229e73775d8R133.

[jmjatlanta]

I will get rid of all these extra commits once travis is happy. Sorry. Evidently my intellisense was throwing me off. It is saying both sides are uint32_t. I guess g++ is a more forgiving on implicit conversions, and clang is more picky.

[jmjatlanta]

Use .value() to convert little_uint32_buf_t to uint32, see https://github.com/bitshares/bitshares-core/pull/1714/files#diff-76b01416e354adac72143229e73775d8R133.

I would if I could. g++ says it is a uint32, whereas clang (apparently) thinks it is a boost::endian. This is frustrating, as I do not have clang on my dev box. Perhaps I have it on my mac.

[abitmore]

@jmjatlanta it IS an endian in develop branch. Please rebase to latest develop before trying again.

[jmjatlanta]

Something has been screwed over during all my re-basing. Now when I rebase it says there is nothing to do. I think it may be due to my branch being on a forked repository, or perhaps a previous merge conflict that was not resolved correctly. I am not sure. I am moving my changes over to a new branch to see if that works better.

I feel like Yosemite Sam... https://www.youtube.com/watch?v=b9AOVdov5lE

[abitmore]

My recommendation: always backup current branch by creating a new branch before rebase.

git branch backup-of-current-branch
git rebase the-new-base

Alternatively, you can check git reflog to find lost branches.

[pmconrad]

The catch with subsequent rebases is that if you're on branch A and do git rebase B then git rebase C, you will end up with a branch A that has the changes from both the original A and B on top of C.
The correct way to do this is to use git rebase --onto C B in the second step.

[jmjatlanta]

The correct way to do this is to use git rebase --onto C B in the second step.

Thank you for that. I will put that in my notes, along with the recommendation of @abitmore.

I have cleaned things up in another branch. See PR #1764