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

allow blocking whole subnets #5363

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@moneromooo-monero
Copy link
Contributor

commented Mar 29, 2019

Note that while the subnet is a type of network address, it does not get serialized, to avoid issues with a peer sending those in a peerlist. I'm not sure whether this is best that way, or with a filter-on-receive in the caller. This seems safer though.

@moneromooo-monero moneromooo-monero changed the title Subnet allow blocking whole subnets Mar 29, 2019

@@ -218,6 +252,10 @@ namespace net_utils
return this_ref.template serialize_addr<net::tor_address>(is_store_, stg, hparent_section);
case address_type::i2p:
return this_ref.template serialize_addr<net::i2p_address>(is_store_, stg, hparent_section);
#if 0

This comment has been minimized.

Copy link
@vtnerd

vtnerd Mar 31, 2019

Contributor

Remove?

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Apr 1, 2019

Author Contributor

Well, I thought this would make it apparent that it was missing on purpose. This is something I was not sure was best in or out.

This comment has been minimized.

Copy link
@vtnerd

vtnerd Apr 2, 2019

Contributor

A short comment would help if the intent was not include.

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Apr 3, 2019

Author Contributor

It's not included in the enum now.

END_KV_SERIALIZE_MAP()
};

inline bool operator==(const ipv4_network_subnet& lhs, const ipv4_network_subnet& rhs) noexcept

This comment has been minimized.

Copy link
@vtnerd

vtnerd Mar 31, 2019

Contributor

Why is this replacing instead of adding the functions?

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Apr 1, 2019

Author Contributor

er, this was not intended, I'll fix.

@@ -248,7 +248,8 @@ namespace nodetool
void change_max_in_public_peers(size_t count);
virtual bool block_host(const epee::net_utils::network_address &adress, time_t seconds = P2P_IP_BLOCKTIME);
virtual bool unblock_host(const epee::net_utils::network_address &address);
virtual std::map<std::string, time_t> get_blocked_hosts() { CRITICAL_REGION_LOCAL(m_blocked_hosts_lock); return m_blocked_hosts; }
virtual bool is_host_blocked(const epee::net_utils::network_address &address, time_t *seconds) { CRITICAL_REGION_LOCAL(m_blocked_hosts_lock); return !is_remote_host_allowed(address, seconds); }

This comment has been minimized.

Copy link
@vtnerd

vtnerd Mar 31, 2019

Contributor

Why have this alias? It does not appear to be overriding any base class implementation either.

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Apr 1, 2019

Author Contributor

Because it's a public entry point for this, which just piggy backs on the base class stuff, and its override is private below.

// manually loop since we have to check for subnets
const time_t now = time(nullptr);
std::map<epee::net_utils::network_address, time_t>::iterator it;
for (it = m_blocked_hosts.begin(); it != m_blocked_hosts.end(); )

This comment has been minimized.

Copy link
@vtnerd

vtnerd Mar 31, 2019

Contributor

Why have a std::map if find operations no longer work?

A ipv4_network_address instance should be merged into a ipv4_network_subnet instance such that the find operation still works. Turning a logarithmic operation into a linear one seems undesirable in this context, although hopefully most nodes have a low blocked hosts count.

boost::variant<ipv4_network_subnet, std::string> would work with minimal effort. Or a "stripped down" copy-and-paste of network_address that has special assignment for ipv4_network_address that merges into ipv4_network_subnet would work too. Only serialization, operator< and str() methods need to be copied really since there is no such concept of port with this type.

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Apr 1, 2019

Author Contributor

No real reason. It could be made a deque instead.

You'll have to elaborate about your last paragraph, I'm not sure what you're suggesting.

This comment has been minimized.

Copy link
@vtnerd

vtnerd Apr 2, 2019

Contributor

You'll have to elaborate about your last paragraph, I'm not sure what you're suggesting.

boost::variant has an operator< that sorts by element type first so it can be used in a std::map with find:

std::map<
  boost::variant<epee::net_utils::ipv4_network_subnet, std::string>, time_t
> m_blocked_hosts;
...

bool is_remote_host_allowed(const epee::net_utils::network_address &address, time_t *t)
{
  using epee::net_utils;

  boost::variant<ipv4_network_subnet, std::string> host;
  switch (address.get_type_id())
  {
    case address_type::ipv4:
      host = ipv4_network_subnet{address.as<ipv4_network_address>().ip(), 32};
      break;
    default:
      host = address.host_str();
      break;
  }
  auto it = m_blocked_hosts.find(host);
}

which would keep the logarithmic lookup intact. One small benefit over the current mainline code is that when address is an ipv4 address, no temporary memory allocations need to be made anymore.

The other technique I suggested (copying network_address) would be a little more work than I first thought since existing network address types implemented operator< and operator== in terms of host AND port.

And so I think this patch changed lookup behavior slightly with operator== usage instead of find(address.host_str()). The issue would only occur with non IPv4 addresses.

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Apr 3, 2019

Author Contributor

OK, that looks alright. Any reason for using std::string rather than network_address, beyond it is the current way ? Most places will just call host_str, then lookup with that, it seems like a waste.

This comment has been minimized.

Copy link
@vtnerd

vtnerd May 3, 2019

Contributor

I used std::string in that example because it was the easiest technique to update the existing implementation. The problem is that operator== and operator< use exact matching with port whereas comparing host_str() output only compares the host.

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero May 3, 2019

Author Contributor

Re-reading your code, I think it doesn't work in this case:
m_blocked_hosts contains 1.2.3.0/24
Queried address is 1.2.3.4
Then host gets set to 1.2.3.4/32, which should be blocked, but isn't.

This comment has been minimized.

Copy link
@vtnerd

vtnerd Jul 11, 2019

Contributor

Changing from a log search to a linear search is still unfortunate. "Stuffing" this table should be tough remotely, although I haven't thought about it thoroughly.

And the above code should have been m_blocked_hosts.lower_bound(host);. And the other issues:

  • The value in ipv4_network_subnet is network order but needs to be host order for proper searching
  • A search must be done against an operator<(subnet, ip) so IP to subnet is converted properly. This requires one of:
    • Moving to C++14
    • Updating minimum boost version
    • Using a sorted std::vector instead
{ return ip() == other.ip(); }
bool matches(const ipv4_network_address &address) const;

constexpr uint32_t ip() const noexcept { return m_ip & ~(0xffffffffull << m_mask); }

This comment has been minimized.

Copy link
@vtnerd

vtnerd Mar 31, 2019

Contributor

Does this function make sense? Its technically not a valid IP most often.

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Apr 1, 2019

Author Contributor

OK, I'll rename it.

bool equal(const ipv4_network_subnet& other) const noexcept;
bool less(const ipv4_network_subnet& other) const noexcept;
constexpr bool is_same_host(const ipv4_network_subnet& other) const noexcept
{ return ip() == other.ip(); }

This comment has been minimized.

Copy link
@vtnerd

vtnerd Mar 31, 2019

Contributor

Is it easier to just compare the ip and masks without the extra bit twiddling?

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Apr 1, 2019

Author Contributor

I suppose it could if you were to make the underlying ip canonical on ctor. It's just the same thing really, just in different places.

@@ -42,7 +42,8 @@ namespace net_utils
ipv4 = 1,
ipv6 = 2,
i2p = 3,
tor = 4
tor = 4,
ipv4subnet = 5,

This comment has been minimized.

Copy link
@vtnerd

vtnerd Mar 31, 2019

Contributor

This seems like an abuse of the interface - its no longer representing a single address endpoint and is now representing a range of inputs. In the majority of contexts where network_address is being used, this new variant will not be wanted.

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Apr 1, 2019

Author Contributor

So what do you suggest instead ?

@moneromooo-monero moneromooo-monero force-pushed the moneromooo-monero:subnet branch from 274ece1 to 450a7ac Apr 1, 2019

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

Here's a version which removes the enum. Ignoring the placeholder try/catch to determine whether an entry is a subnet, is that more like what you intend ? If so, the placeholder can be made into something better.

bool ipv4_network_subnet::is_local() const { return net_utils::is_ip_local(subnet()); }
bool ipv4_network_subnet::matches(const ipv4_network_address &address) const
{
return (address.ip() & ~(0xffffffffull << m_mask)) == subnet();

This comment has been minimized.

Copy link
@vtnerd

vtnerd Jul 11, 2019

Contributor

I believe this is broken on big endian machines.

return make_error_code(net::error::invalid_mask);
if (mask > 32)
return make_error_code(net::error::invalid_mask);
return {epee::net_utils::ipv4_network_subnet{ip, (uint8_t)mask}};

This comment has been minimized.

Copy link
@vtnerd

vtnerd Jul 11, 2019

Contributor

This is a re-hash of an old comment but this is now potentially returning subnets where they cannot be used. Its partially due to the hacking of network_address for something that isn't a network address. I haven't gone through some of the code paths, but Im not sure what happens if someone passes a /24, etc., into the "exclusive node" options, etc. I think this returns an error on all such paths (hopefully).

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Jul 11, 2019

Author Contributor

Yes, good point. I did not think about this.

@moneromooo-monero moneromooo-monero force-pushed the moneromooo-monero:subnet branch from 450a7ac to 53c0e75 Jul 11, 2019

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

I've changed the way it works, subnets are now totally separate from addresses. This should... address (har har) the issues with confusion and API abuse. Also the constant time lookup in m_blocked_hosts. There's still linear time lookup in m_blocked_subnets, but that one's manual only, no auto addition, so it'll be empty or very short.

@vtnerd

vtnerd approved these changes Jul 16, 2019

}
return false;
if (it != m_blocked_hosts.end())

This comment has been minimized.

Copy link
@vtnerd

vtnerd Jul 16, 2019

Contributor

Is this if block (starting on 176) secretly an else { ... } for the if statement on line 168?

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Jul 16, 2019

Author Contributor

Kinda, yes. I changed it, since it makes the code slightly faster and no more complex.

if (cntxt.m_remote_address.get_type_id() != epee::net_utils::ipv4_network_address::get_type_id())
return true;
auto ipv4_address = cntxt.m_remote_address.template as<epee::net_utils::ipv4_network_address>();
if (subnet.matches(ipv4_address))

This comment has been minimized.

Copy link
@vtnerd

vtnerd Jul 16, 2019

Contributor

Probably not worth doing (since this might need merging), but an overload for ipv4_network_subnet::matches could the network_address type so that this pattern wouldn't have to be followed each time. Putting a \TODO somewhere might help tag it (although I think those often just get ignored).

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Jul 16, 2019

Author Contributor

You mean a bool matches(const network_address&) which would go the type_id check and cast itself ?

This comment has been minimized.

Copy link
@vtnerd

vtnerd Jul 16, 2019

Contributor

Yes.

b.ip = ip;
b.seconds = i->second - now;
res.bans.push_back(b);
}
}
std::map<epee::net_utils::ipv4_network_subnet, time_t> blocked_subnets = m_p2p.get_blocked_subnets();
for (std::map<epee::net_utils::ipv4_network_subnet, time_t>::const_iterator i = blocked_subnets.begin(); i != blocked_subnets.end(); ++i)

This comment has been minimized.

Copy link
@vtnerd

vtnerd Jul 16, 2019

Contributor

I don't see why a foreach range wasn't used here - would have been less typing anyway.

moneromooo-monero added some commits Mar 28, 2019

p2p: store network address directly in blocked host list
rather than their string representation

@moneromooo-monero moneromooo-monero force-pushed the moneromooo-monero:subnet branch from 53c0e75 to 65c4004 Jul 16, 2019

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

Squashed and rebased on master.

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