Dan Burkert has posted comments on this change. Change subject: KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs ......................................................................
Patch Set 32: (9 comments) http://gerrit.cloudera.org:8080/#/c/6514/32/src/kudu/rpc/server_negotiation.cc File src/kudu/rpc/server_negotiation.cc: Line 60: DEFINE_string(trusted_subnets, We discussed offline about this, just going to write up what we decided: If the user specifies the flag explicitly, then we won't add in unroutable IPs or local subnets. Instead, we'll just use the address blocks specified. PS32, Line 65: can completely disable this restriction I think this sentence will read better as "Set it to '0.0.0.0/0' to allow unauthenticated connections from all remote IP addresses." Line 76: for (auto const& t : strings::Split(value, ",", strings::SkipEmpty())) { > warning: the variable '__end' is copy-constructed from a const reference bu It's typical to make the iteration variable a 'const auto&', which means the reference is to a non-mutable object. PS32, Line 212: public nit: publicly Also, add a reference to the flag that controls it, like "... prohibited. See --trusted_subnets flag for more information." Line 728: "from public routable IPs are prohibited", Same here Line 916: Status s = Network::ParseCIDRStrings(FLAGS_trusted_subnets, &trusted_subnets); You can probably just wrap this in a KUDU_CHECK_OK call. It shouldn't ever fail since it's already been validated, right? http://gerrit.cloudera.org:8080/#/c/6514/32/src/kudu/util/net/net_util.h File src/kudu/util/net/net_util.h: Line 96: // (same as network byte order) add period to end of sentence. http://gerrit.cloudera.org:8080/#/c/6514/32/src/kudu/util/net/sockaddr.h File src/kudu/util/net/sockaddr.h: Line 73: bool WithinNetwork(const Network& netaddr) const; What do you think about defining this operator on Network instead of Sockaddr? It's debatable, but I think it makes sense to keep it there. Line 76: bool WithinNetworks(const std::vector<Network>& netaddrs) const; I'm not sure it's worth defining this, since it's easily done with std::any_of on a variety of input collections. -- 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: 32 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
