net: introduce removal methods for net.BlockList

[zbo14]

Hello! This is my 1st PR and I'm not a C++ developer so bear with me :)

High level changes

Currently, to "remove" a rule from net.BlockList, one must create a new BlockList and add every other existing rule to it. This PR introduces removeAddress(), removeRange(), and removeSubnet() methods to lib/internal/blocklist.js so users can easily modify rule sets.

Adding redundant addresses, ranges, or subnets no longer creates multiple rules in the BlockList. However, this behavior doesn't apply to a subnet that overlaps another subnet or a subnet that's equivalent to a range. The modification to rule-adding behavior eases rule removal discussed in the next section.

Low level changes

To achieve the functionality mentioned, this PR adds SocketAddressRange and SocketAddressMask classes to src/node_sockaddr*. These classes have their own hashing implementations so their instances can be uniquely mapped to rules. SocketAddressRangeRule and SocketAddressMaskRule now utilize these classes, respectively.

SocketAddressBlocklist now has separate mappings for address, range, and subnet rules. This way, SocketAddressBlocklist can uniquely identify an address, range, or subnet and its corresponding rule. This makes rule removal easier but also compels a change to rule-adding behavior where redundant addresses, ranges, or subnets do not create additional rules when compared to rules in the same category.

I updated the documentation and added test cases for rule removal and checking redundant addition of rules.

Some open questions

1. Is the general approach here ok?
2. Any missing test/edge cases?
3. Could/should SocketAddressRange and SocketAddressMask be exposed in the net API? If so, would it make sense to do this in a separate PR?
4. Any idioms or best practices I'm missing?

[tniessen]

Thank you, this looks like it could be a great first contribution!

[tniessen]

The documentation for these functions should specify whether they throw if no such rule was found, and they should probably clarify that, based on my understanding, they require exact matches. For example, it is not possible to remove a part of an existing range rule.

[zbo14]

@tniessen Good point, the removal methods don't currently throw if a matching rule isn't found. I think that behavior (i.e. throwing if rule not found) makes sense. I can implement / document it if that's alright?

[zbo14]

hmm or the method could return a boolean indicating whether a matching rule was removed?

[tniessen]

I'm not sure what the behavior of the adding functions is (I'm on mobile right now so can't test).

• If add throws if a rule already exists, remove should throw if no such rule exists.
• If add doesn't add duplicate rules, maybe this should return a boolean (similar to JS sets).
• If add does add duplicate rules, and if this removes exactly one of them (as opposed to all of them), then removal should throw if no rule exists that could be removed.

[zbo14]

Gotcha, so add used to create duplicate rules. I changed this behavior to ease rule removal, so add no longer creates duplicate rules. Maybe this change is undesirable. I could be missing context on why duplicate rules were allowed in the first place.

Otherwise, I agree with your points. Remove probably shouldn't throw since add doesn't. It should return a boolean like map, set, etc. as you mentioned.

[jasnell]

FWIW, Not including removal methods on a block list instance was intentional and I'd be -1 on adding them in the general case. The reason goes back to the original use case for this -- that I haven't yet, but still plan to, get back to, and that's allowing a node.js process to be started with a blocklist that user code cannot modify. e.g. something like node --block-ip 123.123.123.123 where the process has a default BlockList. I'd be ok with a way of creating a new modified BlockList starting from another, or having a separate mutable BlockList object but adding the removals does go against my original intent on this. That said, I won't block if folks generally want this. I just think it would be a mistake.

[zbo14]

@jasnell Thanks for providing some background. I have a few questions:

allowing a node.js process to be started with a blocklist that user code cannot modify

Were you planning to remove the add methods in the API then? My thought was having methods to remove rules made sense if there were methods to add rules. Of course, if the original intention was to make BlockList immutable- for instance, to instantiate it with a fixed set of rules, then I see how introducing removal methods would be counter productive.

having a separate mutable BlockList object

So another class like MutableBlockList exposed in net? Or perhaps it's better to leave that task to an npm package?

I just think it would be a mistake

By "mistake", it sounds to me like this could come back to bite developers in some way. Could you elaborate on this?

Finally, was there intention behind allowing duplicate rules? I removed the following behavior in this PR so it would be helpful to know the reasoning for this:

const list = new net.BlockList();

list.addAddress('127.0.0.1');
list.addAddress('127.0.0.1');

console.log(list);

// BlockList {
//   rules: [
//     'Address: IPv4 127.0.0.1',
//     'Address: IPv4 127.0.0.1'
//   ]
// }

Anyway, I don't want to step on your toes or take an ill-advised approach so I appreciate your feedback.

[jasnell]

No I wasn't planning on removing the add. Udder code should be allowed to make the policy more restrictive, just not less.

[targos]

No I wasn't planning on removing the add. Udder code should be allowed to make the policy more restrictive, just not less.

For a hypothetical --block-ip CLI flag, we could have a more restrictive subclass or a wrapper of BlockList. The object would be constructed by the runtime anyway (which doesn't have to use the public API).

[zbo14]

I'm working on a project right now that's using a BlockList as a dynamic "allow list". Perhaps this wasn't an envisioned use case, but having removal methods would help make the policy more restrictive.

Again, maybe I shouldn't be using BlockList for this. But there isn't an AllowList or a more generic ACL yet and BlockList implements pretty much everything I need (besides the removal methods).