Todd Lipcon has posted comments on this change. Change subject: KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs ......................................................................
Patch Set 19: (19 comments) http://gerrit.cloudera.org:8080/#/c/6514/19/src/kudu/rpc/negotiation-test.cc File src/kudu/rpc/negotiation-test.cc: PS19, Line 155: 192.169 since this is supposed to be testing a public IP, I think it's confusing that it looks so close to one of the private blocks. How about using 8.8.8.8 (a google DNS server that a lot of people might have memorized) PS19, Line 220: unique_ptr<Socket> server_socket = desc.use_test_socket ? : unique_ptr<Socket>(new NegotiationTestSocket()) : : unique_ptr<Socket>(new Socket()); would be shorter to just write server_socket(desc.use_test_socket ? new NegotiationTestSocket() : new Socket()) right? Line 923: } how about a case with an authenticated connection from a public routable IP? http://gerrit.cloudera.org:8080/#/c/6514/19/src/kudu/rpc/server_negotiation.cc File src/kudu/rpc/server_negotiation.cc: Line 61: DEFINE_bool(allow_unauthenticated_public_connections, false, Is this option actually still relevant if we have trusted_subnets? Isn't setting this to 'true' equivalent to adding 0.0.0.0/0 as a trusted subnet? PS19, Line 62: "Allow connections from unauthenticated public routable IPs. If enabled, " : "any people on the internet can connect and browse the data, which is " : "very dangerous. It is highly recommended to keep it disabled. I'd suggest rephrasing this as: Allow unauthenticated connections from public routable IP addresses. If this is enabled and network access is not otherwise restricted by a firewall, malicious users may be able to gain unauthorized access. It is highly recommended to keep this option disabled. PS19, Line 157: encryption_ == RpcEncryption::DISABLED) { not quite sure whether this is right. I would have guessed that this would be based on whether encryption is negotiated, not based on the server side config. PS19, Line 879: unauthenticated this doesn't quite match the logic, if we're also checking encryption. ie the client could be authenticated, but have disabled encryption, and still reach this error, right? http://gerrit.cloudera.org:8080/#/c/6514/19/src/kudu/security/init.cc File src/kudu/security/init.cc: PS19, Line 500: vector<string> addrs; : RETURN_NOT_OK(GetNetworkAddresses(&addrs)); : string val = StrCat(FLAGS_trusted_subnets, JoinStrings(addrs, ",")); : SetCommandLineOptionWithMode("trusted_subnets", : val.c_str(), : google::SET_FLAGS_DEFAULT); it strikes me as a little strange to be writing back into the flags rather than having some global variable which contains the parsed info. Per the suggestion elsewhere, if we had a structured way of storing the subnets, then we could have a global with pre-parsed data, which is populated from this initialization code http://gerrit.cloudera.org:8080/#/c/6514/19/src/kudu/security/init.h File src/kudu/security/init.h: Line 34: Status InitTrustedSubnets(); I think it would be better to encapsulate all of the logic somewhere in netutil, and use something like std::once_call to do the initialization. That way we don't need to explicitly init it, and all the related code could be in one place http://gerrit.cloudera.org:8080/#/c/6514/19/src/kudu/util/net/net_util-test.cc File src/kudu/util/net/net_util-test.cc: Line 99: EXPECT_TRUE(addr.WithInSubnet("10.0.0.0/8")); in these tests it seems like you're always testing the case where the subnet address ends in enough 0 bits that any non-masked bits are 0. I'm not sure what the definition of a valid CIDR address is -- is it legal to refer to a network like "1.2.3.4/8"? Or is it only legal to parse one like "1.0.0.0/8"? We should respect whatever the actual spec says. http://gerrit.cloudera.org:8080/#/c/6514/19/src/kudu/util/net/net_util.cc File src/kudu/util/net/net_util.cc: PS19, Line 229: ==A nit: spaces around ==s Line 236: freeifaddrs(ifap); can you use MakeScopedCleanup here so that the freeifaddrs is placed as close as possible to the getifaddrs() call? that way if we added some early-return in the loop, we wouldn't leak. Line 333: // Strip any whitespace from the cidr_addr. should the stripping behavior be part of the ParseString below? Line 340: return (s.ok() && success); should verify that bits <= 32 http://gerrit.cloudera.org:8080/#/c/6514/19/src/kudu/util/net/net_util.h File src/kudu/util/net/net_util.h: PS19, Line 110: // Returns the local network addresses. : Status GetNetworkAddresses(std::vector<std::string>* addresses); The docs should be clearer here to indicate that it's returning networks, not individual addresses. I think if you added some struct for a Network (combination of an address and a mask) and had this return a vector of those structs, it would be easier to use and have less string-related overhead. http://gerrit.cloudera.org:8080/#/c/6514/19/src/kudu/util/net/sockaddr.cc File src/kudu/util/net/sockaddr.cc: PS19, Line 118: std::pair<string, string> p = strings::Split(net_cidr_addr, : strings::delimiter::Limit("/", 1)); : : // Strip any whitespace from the cidr_addr. : StripWhiteSpace(&p.first); : Sockaddr net_addr; : Status s = net_addr.ParseString(p.first, 0); : : uint32_t bits; : bool success = SimpleAtoi(p.second, &bits); this seems to be duplicated code from elsewhere. Can you share this code? eg perhaps some struct like Subnet which has an address and mask, with corresponding function to parse (which can return a status) and a ToString? PS19, Line 130: NetworkByteOrder::FromHost32(~(0xffffffff >> bits)) hm, I think I would have expected to see "(1ULL << bits) - 1" http://gerrit.cloudera.org:8080/#/c/6514/19/src/kudu/util/net/sockaddr.h File src/kudu/util/net/sockaddr.h: PS19, Line 71: WithIn nit: "Within" rather than 'WithIn' http://gerrit.cloudera.org:8080/#/c/6514/19/src/kudu/util/net/socket.h File src/kudu/util/net/socket.h: Line 100: virtual Status GetPeerAddress(Sockaddr *cur_addr) const; Can you add a comment here that it's virtual so it can be overridden by tests? -- To view, visit http://gerrit.cloudera.org:8080/6514 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6c3fbb5491785874c5701d6c9d866949cfac905e Gerrit-PatchSet: 19 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Harsh J <ha...@harshj.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
