Added way to block members using CIDR

[pierresouchay]

Added 2 new fields in configuration of memberlists to allow/disallow
others machines joining memberlist.

This will allow protecting some machines from joining a cluster and will increase security as well.

Will be useful to implement hashicorp/consul#5916
as suggested my @mkeller

[pierresouchay]

@mkeeler Is it what you had in mind in hashicorp/consul#5916 (comment) ?

[pierresouchay]

@mkeeler Could you have a look and tell me if it is Ok to achieve hashicorp/consul#5916 ?

[mkeeler]

Overall I think this would work. My one concern is all the string parsing and checks that would have to be done for every UDP packet or TCP connection. Would it still work if the CIDR checks were done in these two places instead (rough locations):

memberlist/state.go

Line 896 in e1138a6

state = &nodeState{

memberlist/state.go

Line 933 in e1138a6

// If DeadNodeReclaimTime is configured, check if enough time has elapsed since the node died.

In the first location we are handling an alive event for a new node. In the second location we are handling an alive event for an existing node but already know that either the IP or port has changed. What this doesn't do is CIDR validation when handling an alive event for a node whose IP or port has not been updated (which should be the normal and most prevalent case).

[pierresouchay]

@mkeeler DONE as you suggested

[mkeeler]

LGTM

[rboyer]

Please take a look at my comments and see if I've misunderstood anything about your intent.

[pierresouchay]

Please take a look at my comments and see if I've misunderstood anything about your intent.

I fixed issues you mentioned. Are you Ok with the implementation I made to protect NotifyAliveor do you have another suggestion?

[pierresouchay]

Ping @rboyer @mkeeler ?

[rboyer]

Apologies for the delay. I was having a hard time figuring out how to describe my remaining reservations with the proposed implementation but I think I've got it now.

Overall the best solution here is to always ensure you are using gossip encryption in your consul clusters which should prevent accidental joins, since the stray agents would lack the necessary keys to formulate valid memberlist messages. But I can see situations where someone may be in a situation where enabling gossip encryption isn't feasible and so something shaped like the proposed solution is beneficial.

I think the pairing of an allow and a deny configuration is offering too much complexity for a likely rarely used subfeature. The allow_write_http_from consul configuration option has the semantics that if you don't configure it at all it is default-allow, and if you configure anything it switches to default-deny. It does require that you have the ability to enumerate all allowable addresses in CIDR form sanely.

For straightforward behind-the-firewall setups this might be as straightforward as dumping all of the class-C networks (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, and an ipv6 equivalent if being used) into the allow list. If you are incapable of doing this because your IP addresses don't neatly fit into CIDR blocks, my assumption is that you are probably drifting into the territory where you would be better served with the gossip encryption option.

I'd like it if you could reduce the complexity here and only expose a single tunable AllowedCIDRs setting that defaults to nil with the semantics that it is default-allow unless configured to a non-empty slice of data.

When the feature is in default-allow mode it would also be nice to avoid incurring any additional operations or allocations that can be avoided (namely we should avoid parsing net.Addr.Network() into an interim net.IP when we know the user has not activated this CIDR feature at all.

[pierresouchay]

@rboyer Thank you for the review, I think I addressed all your points!

[rboyer]

Looks good. The only thing missing for me now that you've made the big changes is some test showcasing the feature successfully keeping awareness of a blocked node from making it into the memberlist of a node configured to block it.

[hanshasselberg]

@pierresouchay The only thing thats left, seem to be

[...] some test showcasing the feature successfully keeping awareness of a blocked node from making it into the memberlist of a node configured to block it.

[pierresouchay]

@i0rek yes, I have started to implement test, but I have many things to change, including the shell script to create sub networks for Mac, I'll find some time soon to work on this

[pierresouchay]

@i0rek DONE

[pierresouchay]

@rboyer I added the tests as required

[rboyer]

OK the last comments I left should be pretty quick to fix and after that we are golden.

[pierresouchay]

@rboyer Thank you for the review, I think I resolved all your issues

I actually kept eb90195 because the it might have some effect if a delegate has been setup

If you prefer to remove this 'double check', I'll revert it

[pierresouchay]

hu... fun, unit tests do not work anymore :(

[pierresouchay]

@rboyer Ok, I found the issue with the unit tests, now, Join() by default assert that Name / Addr is set by default, so my unit tests were not working anymore, but last commit fixes that.

On another topic, the change you asked to use:

func (c *Config) IPMustBeChecked() bool {
	return len(c.CIDRsAllowed) > 0
}

instead of cheking nil also broke another test (I fixed it), that was asserting that AllowedCIDRs = [] was blocking everything.

When I wrote it initially, I thought in terms of security that blocking everything is always a nice feature, but for the project itself, it might defeat its own purpose, so I don't think it is a big deal.

An finally, the unit tests are not run in the PR, not a big deal, but it might be a good addition

I think everything is ok now

[rboyer]

LGTM

[rboyer]

We can tag a new version of this after you start using this from serf or consul, in case you discover a need to do any last minute refactor of this feature while attempting to use it.

[pierresouchay]

@rboyer Thanks, I'll try the integration in Consul soon, but it should be straigforward

[pierresouchay]

@rboyer I made the PR in Consul some time ago, maybe you could have a look at hashicorp/consul#7628 ?

Thank you