Alexey Serbin has posted comments on this change. Change subject: KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs ......................................................................
Patch Set 5: (8 comments) http://gerrit.cloudera.org:8080/#/c/6514/5//COMMIT_MSG Commit Message: PS5, Line 9: This rejects unauthenticated connections from publicly routable IPs, even if : authentication and encryption are not configured. An unsafe flag : 'allow_unauthenticated_public_connections' is provided to enable unauthenticated : connections from publicly routable IPs. Even though it is highly recommended to : disable it. nit: could you keep the lines under 72 chars length? You can get more information on the commit guidelines at https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project#_commit_guidelines That doc is referenced from http://kudu.apache.org/docs/contributing.html#_submitting_patches http://gerrit.cloudera.org:8080/#/c/6514/5/src/kudu/rpc/server_negotiation.cc File src/kudu/rpc/server_negotiation.cc: PS5, Line 149: Status s = RejectUntrustedPublicConnection(addr); : if (!s.ok()) return s; nit: why not just RETURN_NOT_OK(RejectUntrustedPublicConnection(addr)); PS5, Line 679: Status s = RejectUntrustedPublicConnection(addr); : if (!s.ok()) return s; RETURN_NOT_OK(RejectUntrustedPublicConnection(addr)); PS5, Line 858: = Status::OK(); nit: Status::OK() is set by the default constructor, so no assignment is needed here. PS5, Line 861: is unacceptable are prohibited http://gerrit.cloudera.org:8080/#/c/6514/5/src/kudu/util/net/sockaddr.cc File src/kudu/util/net/sockaddr.cc: PS5, Line 114: Sockaddr::IsPrivateAddress What about link-local addresses like 169.254.0.0/16 except for first and last /24 networks in the range (i.e. 169.254.1.0 -- 169.254.254.0) ? PS5, Line 115: uint32 first_byte, sec_byte; : first_byte = NetworkByteOrder::FromHost32(addr_.sin_addr.s_addr) >> 24; : sec_byte = (NetworkByteOrder::FromHost32(addr_.sin_addr.s_addr) >> 16) & 0xff; nit: why not to initialize the variables at the point of definition, like uint32_t first_byte = NetworkByteOrder::FromHost32(addr_.sin_addr.s_addr) >> 24; uint32_t second_byte = (NetworkByteOrder::FromHost32(addr_.sin_addr.s_addr) >> 16) & 0xff; http://gerrit.cloudera.org:8080/#/c/6514/5/src/kudu/util/net/sockaddr.h File src/kudu/util/net/sockaddr.h: PS5, Line 71: // 10.0.0.0 - 10.255.255.255, : // 172.16.0.0 - 172.31.255.255, : // 192.168.0.0 - 192.168.255.255. What about link-local IPv4 addresses (RFC 6890 and RFC 3927)? -- 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: 5 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: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
