Handle CJDNS from LookupSubNet() #27071
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
There was a problem hiding this comment.
Concept ACK to fixing this - however, I'm unsure about how the approach fits into the larger architecture:
I think of netbase as a module for mostly context-free basic low-level methods - this PR moves IsReachable to it and saves its state in a global variable, accessing it both from different threads within the node and the GUI. Shouldn't the information flow between the Node and the GUI rather go through node/interfaces instead of having a shared global variable?
Did you consider the alternative approach of letting banman be agnostic of CJDNS (everything stays IPv6, CJDNS addresses get a /128 netmask just like single IPv6 addresses), and add special "flipping" logic to the points where we need to compare actual addresses from peers with the results from banman? I didn't try it yet so not sure if viable, but that was my first idea of how to fix the issue of not being able to ban CJDNS peers.
src/netbase.cpp
Outdated
| @@ -694,6 +694,7 @@ bool LookupSubNet(const std::string& subnet_str, CSubNet& subnet_out) | |||
| CNetAddr addr; | |||
|
|
|||
| if (LookupHost(str_addr, addr, /*fAllowLookup=*/false)) { | |||
| addr = MaybeFlipIPv6toCJDNS(CService{addr, /*port=*/0}); | |||
There was a problem hiding this comment.
Now that MaybeFlipIPv6toCJDNS is available in netbase, would it make sense (maybe in follow-ups) to also use it in the Lookup() functions, not just LookupSubNet) and reduce the number of occurrences we have to call MaybeFlipIPv6toCJDNS from outside it?
There was a problem hiding this comment.
Oh, well. I feel like MaybeFlipIPv6toCJDNS() is warranted in some of the lower level Lookup*() functions. But they are called from so many places, the change would be very invasive. I have to gather some bravery in order to look into that. If that is to be done, maybe it should be preceded by some simplification of the Lookup*() family of functions. I get a headache every time I have to deal with:
bool LookupHost(const std::string& name, std::vector<CNetAddr>& vIP, unsigned int nMaxSolutions, bool fAllowLookup, DNSLookupFn dns_lookup_function = g_dns_lookup);
bool LookupHost(const std::string& name, CNetAddr& addr, bool fAllowLookup, DNSLookupFn dns_lookup_function = g_dns_lookup);
bool Lookup(const std::string& name, std::vector<CService>& vAddr, uint16_t portDefault, bool fAllowLookup, unsigned int nMaxSolutions, DNSLookupFn dns_lookup_function = g_dns_lookup);
bool Lookup(const std::string& name, CService& addr, uint16_t portDefault, bool fAllowLookup, DNSLookupFn dns_lookup_function = g_dns_lookup);
CService LookupNumeric(const std::string& name, uint16_t portDefault = 0, DNSLookupFn dns_lookup_function = g_dns_lookup);
bool LookupIntern(const std::string& name, std::vector<CNetAddr>& vIP, unsigned int nMaxSolutions, bool fAllowLookup, DNSLookupFn dns_lookup_function);
I was thinking about the same, but
I am not sure
CSubNet::Match() mix IPv6 and CJDNS--- a/src/netaddress.cpp
+++ b/src/netaddress.cpp
@@ -1039,24 +1039,40 @@ CSubNet::CSubNet(const CNetAddr& addr) : CSubNet()
/**
* @returns True if this subnet is valid, the specified address is valid, and
* the specified address belongs in this subnet.
*/
bool CSubNet::Match(const CNetAddr &addr) const
{
- if (!valid || !addr.IsValid() || network.m_net != addr.m_net)
+ if (!valid || !addr.IsValid()) {
return false;
+ }
- switch (network.m_net) {
+ Network subnet_net = network.m_net;
+ Network addr_net = addr.m_net;
+
+ if ((subnet_net == NET_IPV6 && addr_net == NET_CJDNS) ||
+ (subnet_net == NET_CJDNS && addr_net == NET_IPV6)) {
+ // Treat both as CJDNS.
+ subnet_net = NET_CJDNS;
+ addr_net = NET_CJDNS;
+ }
+
+ if (subnet_net != addr_net) {
+ return false;
+ }
+
+ switch (subnet_net) {
case NET_IPV4:
case NET_IPV6:
break;
case NET_ONION:
case NET_I2P:
- case NET_CJDNS:
case NET_INTERNAL:
return addr == network;
+ case NET_CJDNS:
+ return addr.m_addr == network.m_addr;
case NET_UNROUTABLE:
case NET_MAX:
return false;
}
assert(network.m_addr.size() == addr.m_addr.size());Note also that the current PR also resolves two other issues, in addition to the banman one - listed as items in the OP. |
Yes, within |
9b656a5 to
7a93d7b
Compare
|
|
| NET_ONION, | ||
| NET_I2P, | ||
| NET_CJDNS, | ||
| NET_INTERNAL |
There was a problem hiding this comment.
9e47818 As BIP155 networks are added or removed over time, m_reachable could go out of sync with enum Network in netaddress.h ... perhaps construct it programmatically from enum Network (now or in a follow-up)?
There was a problem hiding this comment.
There is no built in construct in the language to do that and I do not like NET_MAX which I perceive as a hack.
There was a problem hiding this comment.
Yes, I agree with you about NET_MAX but using it to populate m_reachable may still be preferable to having a silent dependency where we need to keep ReachableNets::m_reachable in netbase.h manually in sync with the Network enumerators in netaddress.h. If you prefer the latter approach, perhaps comment next to ReachableNets::m_reachable that it needs to be kept in sync.
There was a problem hiding this comment.
What about something like the following as a followup, aiming to workaround the limitation in C++ in general:
C++ enum + a way to list all values + ToString() method, encapsulated
namespace Nets
{
enum Net : uint8_t {
UNROUTABLE,
IPV4,
IPV6,
ONION,
I2P,
CJDNS,
INTERNAL
};
static constexpr std::array<Net, 7> all{UNROUTABLE, IPV4, IPV6, ONION, I2P, CJDNS, INTERNAL};
std::string ToString(Net net)
{
switch (net) {
case UNROUTABLE: return "UNROUTABLE";
case IPV4: return "IPV4";
case IPV6: return "IPV6";
case ONION: return "ONION";
case I2P: return "I2P";
case CJDNS: return "CJDNS";
case INTERNAL: return "INTERNAL";
}
}
};
...
printf("IPV6 = %d\n", Nets::IPV6);
for (const auto& net: Nets::all) {
std::cout << Nets::ToString(net) << std::endl;
}7a93d7b to
8991ed2
Compare
|
|
|
Just thinking aloud:
It follows that banman is doing something that does not make sense (e.g. subnetting tor). This is the root of the problem. I think if that is eradicated, then the rest will untangle by itself. What this means - store two ban maps in Hmm... |
|
Well, it works, but since it does something nonsensical (e.g. subnetting tor) it could be a source of confusion or ultimately, bugs. IMO it shouldn't be doing that. I think the existence and implementation of bool CConnman::DisconnectNode(const CNetAddr& addr)
{
return DisconnectNode(CSubNet(addr));
}would better iterate I will try to allow only IPv[46] |
There was a problem hiding this comment.
It follows that banman is doing something that does not make sense (e.g. subnetting tor). This is the root of the problem.
Not sure if I agree that this is the root of the problem - I think conceptually it's fine to internally treat single addresses as a special case of a subnet (like <ipv4>/32 or <ipv6>/128), and just prevent nonsensical things during user input.
In my opinion, the root of the problem is that fc... addresses are sometimes interpreted as CJDNS, and sometimes as IPv6, depending on startup options. Since large parts of the networking code were designed before CJDNS support was introduced, this makes workarounds like MaybeFlipIPv6toCJDNS() and global availability of reachability necessary.
However, I still feel strongly that having a global, non-constant variable shared by the GUI and node is something we should absolutely avoid.
Another possibility to achieve this would be to pass a bool cjdnsreachable as a variable to LookupSubNet() and add IsReachable() to node/interfaces.cpp. With this, there could still be a global g_reachable_nets used for the call sites within node, but the information to the GUI would flow through the interface.
A similar approach was chosen for the proxy, which is also state from netbase: The GUI code calls getProxy from node/interfaces, which then calls GetProxy() to retrieve the proxy info from netbase. The GUI doesn't use GetProxy() directly.
8991ed2 to
cd5a940
Compare
cd5a940 to
c2a86ba
Compare
|
Invalidates ACK from @jonatack @mzumsande, actually there are two problems - I am talking about Tor subnets and you are talking about a global variable used by the GUI. I will try to mimic the proxy in order to address the latter. |
In case you plan to do that, or the Tor subnets issue, in a follow-up: re-ACK c2a86ba per |
|
@mzumsande, it is possible to avoid calling |
The fuzz testing framework runs as if `-cjdnsreachable` is set and in
this case addresses like `{net=IPv6, addr=fc...}` are not possible.
`vfLimited`, `IsReachable()`, `SetReachable()` need not be in the `net` module. Move them to `netbase` because they will be needed in `LookupSubNet()` to possibly flip the result to CJDNS (if that network is reachable). In the process, encapsulate them in a class. `NET_UNROUTABLE` and `NET_INTERNAL` are no longer ignored when adding or removing reachable networks. This was unnecessary.
It need not be in the `net` module and we need to call it from `LookupSubNet()`, thus move it to `netbase`.
6ff2241 to
2f3f793
Compare
|
|
There was a problem hiding this comment.
ACK 2f3f793
In 0994d45 the commit message might be out of date regarding unbanSelectedNode after the merge of bitcoin-core/gui#717.
A couple questions and a comment, feel free to ignore
All callers of `LookupSubNet()` need the result to be of CJDNS type if `-cjdnsreachable` is set and the address begins with `fc`: * `NetWhitelistPermissions::TryParse()`: otherwise `-whitelist=` fails to white list CJDNS addresses: when a CJDNS peer connects to us, it will be matched against IPv6 `fc...` subnet and the match will never succeed. * `BanMapFromJson()`: CJDNS bans are stored as just IPv6 addresses in `banlist.json`. Upon reading from disk they have to be converted back to CJDNS, otherwise, after restart, a ban entry like (`fc00::1`, IPv6) would not match a peer (`fc00::1`, CJDNS). * `setban()` (in `rpc/net.cpp`): otherwise `setban fc.../mask add` would add an IPv6 entry to BanMan. Subnetting does not make sense for CJDNS addresses, thus treat `fc.../mask` as invalid `CSubNet`. The result of `LookupHost()` has to be converted for the case of banning a single host. * `InitHTTPAllowList()`: not necessary since before this change `-rpcallowip=fc...` would match IPv6 subnets against IPv6 peers even if they started with `fc`. But because it is necessary for the above, `HTTPRequest::GetPeer()` also has to be adjusted to return CJDNS peer, so that now CJDNS peers are matched against CJDNS subnets.
2f3f793 to
0e6f6eb
Compare
|
Updated! |
|
Code review ACK 0e6f6eb |
|
ACK 0e6f6eb |
|
This crashes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=63434 |
LookupSubNet()would treat addresses that start withfcas IPv6 even if-cjdnsreachableis set. This creates the following problems where it is called:NetWhitelistPermissions::TryParse(): otherwise-whitelist=fails to white list CJDNS addresses: when a CJDNS peer connects to us, it will be matched against IPv6fc...subnet and the match will never succeed.BanMapFromJson(): CJDNS bans are stored as just IPv6 addresses inbanlist.json. Upon reading from disk they have to be converted back to CJDNS, otherwise, after restart, a ban entry like (fc00::1, IPv6) would not match a peer (fc00::1, CJDNS).RPCConsole::unbanSelectedNode(): in the GUI the ban entries go throughCSubNet::ToString()and back viaLookupSubNet(). Then it must match whatever is stored inBanMan, otherwise it is impossible to unban via the GUI.These were uncovered by #26859.
Thus, flip the result of
LookupSubNet()to CJDNS if the network base address starts withfcand-cjdnsreachableis set. Since subnetting/masking does not make sense for CJDNS (the address is "random" bytes, like Tor and I2P, there is no hierarchy) treatfc.../maskas an invalidCSubNet.To achieve that,
MaybeFlipIPv6toCJDNS()has to be moved fromnettonetbaseand thus alsoIsReachable(). In the process of movingIsReachable(),SetReachable()andvfLimited[]encapsulate those in a class.