Hao Hao 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 th Done 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 Neg Done Line 923: } > how about a case with an authenticated connection from a public routable IP Done 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 se Done 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: Done PS19, Line 157: encryption_ == RpcEncryption::DISABLED) { > not quite sure whether this is right. I would have guessed that this would Done PS19, Line 879: unauthenticated > this doesn't quite match the logic, if we're also checking encryption. ie t Done 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 Done 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 net Done 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 subne It looks like "1.2.3.4/8" is legal address https://en.wikipedia.org/wiki/Classless_Inter-Domain_Routing. Will add one test. 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 Done Line 236: freeifaddrs(ifap); > can you use MakeScopedCleanup here so that the freeifaddrs is placed as clo Done Line 333: // Strip any whitespace from the cidr_addr. > should the stripping behavior be part of the ParseString below? Done Line 340: return (s.ok() && success); > should verify that bits <= 32 Done 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, n Done 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? Done PS19, Line 130: NetworkByteOrder::FromHost32(~(0xffffffff >> bits)) > hm, I think I would have expected to see "(1ULL << bits) - 1" Not sure if "(1ULL << bits) - 1" will work here, as net_mask should be big endian? 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' Done 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 tes Done -- 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
