[kudu-CR] KUDU-1900: add loopback check and test
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12474 ) Change subject: KUDU-1900: add loopback check and test .. KUDU-1900: add loopback check and test This change modifies Socket::IsLoopbackConnection to check if remote IP is in 127.0.0.0/8 subnet. With this change all connections from 127.0.0.0/8 are treated as local. This change adds force_external_client_ip parameter to AuthTokenIssuingTest. Before this change, AuthTokenIssuingTest relied on Kudu treating connections between non-matching addresses in 127.0.0.0/8 subnet as external. With this change, the test forces Kudu client to use an non-loopback IP address for test cases that verify external connections. If an external IP address is not available, test cases that require an external IP will be skipped. For convenience, this change adds two static utility methods (HostPort::IsLoopback and HostPort::AddrToString), and refactors Sockaddr::host and Sockaddr::IsAnyLocalAddress to use these static methods. Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Reviewed-on: http://gerrit.cloudera.org:8080/12474 Reviewed-by: Adar Dembo Tested-by: Kudu Jenkins --- M src/kudu/integration-tests/security-itest.cc M src/kudu/util/net/net_util.cc M src/kudu/util/net/net_util.h M src/kudu/util/net/sockaddr.cc M src/kudu/util/net/socket.cc 5 files changed, 163 insertions(+), 32 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 16 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-1900: add loopback check and test
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12474 ) Change subject: KUDU-1900: add loopback check and test .. Patch Set 15: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 15 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 19 Feb 2019 21:36:33 + Gerrit-HasComments: No
[kudu-CR] KUDU-1900: add loopback check and test
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12474 to look at the new patch set (#15). Change subject: KUDU-1900: add loopback check and test .. KUDU-1900: add loopback check and test This change modifies Socket::IsLoopbackConnection to check if remote IP is in 127.0.0.0/8 subnet. With this change all connections from 127.0.0.0/8 are treated as local. This change adds force_external_client_ip parameter to AuthTokenIssuingTest. Before this change, AuthTokenIssuingTest relied on Kudu treating connections between non-matching addresses in 127.0.0.0/8 subnet as external. With this change, the test forces Kudu client to use an non-loopback IP address for test cases that verify external connections. If an external IP address is not available, test cases that require an external IP will be skipped. For convenience, this change adds two static utility methods (HostPort::IsLoopback and HostPort::AddrToString), and refactors Sockaddr::host and Sockaddr::IsAnyLocalAddress to use these static methods. Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f --- M src/kudu/integration-tests/security-itest.cc M src/kudu/util/net/net_util.cc M src/kudu/util/net/net_util.h M src/kudu/util/net/sockaddr.cc M src/kudu/util/net/socket.cc 5 files changed, 163 insertions(+), 32 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/12474/15 -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 15 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/12474 ) Change subject: KUDU-1900: add loopback check and test .. Patch Set 10: (4 comments) http://gerrit.cloudera.org:8080/#/c/12474/14/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/12474/14/src/kudu/integration-tests/security-itest.cc@330 PS14, Line 330: KUDU_RETURN_NOT_OK_LOG(GetLocalNetworks(_networks), ERROR, > You can use just RETURN_NOT_OK; the KUDU_ prefix is only really intended fo Done http://gerrit.cloudera.org:8080/#/c/12474/14/src/kudu/util/net/net_util.h File src/kudu/util/net/net_util.h: http://gerrit.cloudera.org:8080/#/c/12474/14/src/kudu/util/net/net_util.h@96 PS14, Line 96: const > No need for const. Done http://gerrit.cloudera.org:8080/#/c/12474/14/src/kudu/util/net/net_util.cc File src/kudu/util/net/net_util.cc: http://gerrit.cloudera.org:8080/#/c/12474/14/src/kudu/util/net/net_util.cc@311 PS14, Line 311: std:: > Drop the std:: prefix. Done http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/util/net/socket.cc File src/kudu/util/net/socket.cc: http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/util/net/socket.cc@304 PS10, Line 304: return true; > Do we need to further condition this local.IsAnyLocalAddress()? I'm guessin I think, we do not need to condition this on local.isAnyLocalAddress, because there is no possible scenario where the remote is a loopback and the connection is coming from outside. If a network interface is seeing a connection from 127.x.x.x, this connection is guaranteed to be local. However, the condition "local.IsAnyLocalAddress() AND NOT remote.IsAnyLocalAddress()" does not guarantee that the connection is local. Also, here is the conversation between Dan and Todd on KUDU-1900 about this: D: So the check would become 'remote addr is in 127.0.0.0/8 OR remote addr == local addr'? I think that should be safe. T: Yea, I think if the remote address is in that loopback subnet that you mentioned we can skip TLS. -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 10 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 19 Feb 2019 19:02:47 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12474 ) Change subject: KUDU-1900: add loopback check and test .. Patch Set 14: (4 comments) http://gerrit.cloudera.org:8080/#/c/12474/14/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/12474/14/src/kudu/integration-tests/security-itest.cc@330 PS14, Line 330: KUDU_RETURN_NOT_OK(GetLocalNetworks(_networks)); You can use just RETURN_NOT_OK; the KUDU_ prefix is only really intended for the C++ thirdparty client (to ensure that our macros are "namespaced" and won't collide with someone else's RETURN_NOT_OK). http://gerrit.cloudera.org:8080/#/c/12474/14/src/kudu/util/net/net_util.h File src/kudu/util/net/net_util.h: http://gerrit.cloudera.org:8080/#/c/12474/14/src/kudu/util/net/net_util.h@96 PS14, Line 96: const No need for const. http://gerrit.cloudera.org:8080/#/c/12474/14/src/kudu/util/net/net_util.cc File src/kudu/util/net/net_util.cc: http://gerrit.cloudera.org:8080/#/c/12474/14/src/kudu/util/net/net_util.cc@311 PS14, Line 311: std:: Drop the std:: prefix. http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/util/net/socket.cc File src/kudu/util/net/socket.cc: http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/util/net/socket.cc@304 PS10, Line 304: return true; > Do we need to further condition this local.IsAnyLocalAddress()? I'm guessin Could you address this question? -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 14 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 19 Feb 2019 18:38:37 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12474 to look at the new patch set (#14). Change subject: KUDU-1900: add loopback check and test .. KUDU-1900: add loopback check and test This change modifies Socket::IsLoopbackConnection to check if remote IP is in 127.0.0.0/8 subnet. With this change all connections from 127.0.0.0/8 are treated as local. This change adds force_external_client_ip parameter to AuthTokenIssuingTest. Before this change, AuthTokenIssuingTest relied on Kudu treating connections between non-matching addresses in 127.0.0.0/8 subnet as external. With this change, the test forces Kudu client to use an non-loopback IP address for test cases that verify external connections. If an external IP address is not available, test cases that require an external IP will be skipped. For convenience, this change adds two static utility methods (HostPort::IsLoopback and HostPort::AddrToString), and refactors Sockaddr::host and Sockaddr::IsAnyLocalAddress to use these static methods. Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f --- M src/kudu/integration-tests/security-itest.cc M src/kudu/util/net/net_util.cc M src/kudu/util/net/net_util.h M src/kudu/util/net/sockaddr.cc M src/kudu/util/net/socket.cc 5 files changed, 163 insertions(+), 32 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/12474/14 -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 14 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has posted comments on this change. (
http://gerrit.cloudera.org:8080/12474 )
Change subject: KUDU-1900: add loopback check and test
..
Patch Set 10:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/integration-tests/security-itest.cc@330
PS10, Line 330: KUDU_RETURN_NOT_OK_LOG(GetLocalNetworks(_networks),
ERROR,
> Let's take an example: an operation whose execution makes a tree-like graph
Got it. Thanks a lot for the explanation!
http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/integration-tests/security-itest.cc@334
PS10, Line 334: if (!network.IsLoopBack()) {
> Nit: indentation
Done
--
To view, visit http://gerrit.cloudera.org:8080/12474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f
Gerrit-Change-Number: 12474
Gerrit-PatchSet: 10
Gerrit-Owner: Greg Solovyev
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 19 Feb 2019 18:04:19 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12474 ) Change subject: KUDU-1900: add loopback check and test .. Patch Set 13: (1 comment) http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/integration-tests/security-itest.cc@330 PS10, Line 330: KUDU_RETURN_NOT_OK_LOG(GetLocalNetworks(_networks), ERROR, > My reason for using KUDU_RETURN_NOT_OK_LOG instead of just KUDU_RETURN_NOT_ Let's take an example: an operation whose execution makes a tree-like graph of function calls. Every function called returns a Status, and every call site is wrapped in RETURN_NOT_OK. The top-most node in the tree is the function that began the operation. It does not wrap the first call with RETURN_NOT_OK; instead, it stores the result in a local Status variable, and LOGs it, converts it into an RPC response, or does something else with it. Why this structure? The idea is for all the intermediate layers to act as conduits for a bad Status, leaving the logging up to the root. This makes sense; it is best positioned to know whether logging is appropriate or not. If every layer wrapped in _LOG, there'd be a lot of duplicate logging for this one operation. Moreover, trying to make good logging decisions in intermediate layers is hard because they're often reused for different purposes by different code paths, some of which may want logging and some of which don't. Pushing the bad Status up the call stack and delegating the logging decision to your caller is the best way to ensure that the right decision is always made. An analogy here is Java's exception handling: when you throw an exception, you only catch it when you want to handle it and/or log; you wouldn't typically catch it, log, then rethrow. In this particular case, _LOG will trigger duplicate logging: once here, and once on L461. As for RETURN_NOT_OK_PREPEND, we use that when we want an intermediate layer to add information to the bad Status. It won't affect whether a bad Status is logged or not; it'll just modify the message that gets logged. In this case, if L461 logs, it'll very obviously be either from L342 (if the message contains "Could not find an external IP") or L330 (some other message), so _PREPEND doesn't seem necessary inasfar as identifying the true error source. -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 13 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sat, 16 Feb 2019 04:30:47 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has posted comments on this change. (
http://gerrit.cloudera.org:8080/12474 )
Change subject: KUDU-1900: add loopback check and test
..
Patch Set 10:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/integration-tests/security-itest.cc@330
PS10, Line 330: KUDU_RETURN_NOT_OK_LOG(GetLocalNetworks(_networks),
ERROR,
> Use RETURN_NOT_OK_PREPEND instead; that's what we typically do.
My reason for using KUDU_RETURN_NOT_OK_LOG instead of just KUDU_RETURN_NOT_OK
is so that when a test fails because of some misconfiguration of network, it is
easier to figure out why it failed.
I have also noticed that we never use KUDU_RETURN_NOT_OK_LOG anywhere. What is
the reason for using _PREPEND instead of _LOG here?
http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/integration-tests/security-itest.cc@334
PS10, Line 334: if (!network.IsLoopBack()) {
> Nit: indentation
Done
http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/util/net/net_util.h
File src/kudu/util/net/net_util.h:
http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/util/net/net_util.h@95
PS10, Line 95: // Returns true if addr is within 127.0.0.0/8 range.
: static bool IsLoopBack(const uint32_t addr);
:
: // Returns dotted-decimal ('1.2.3.4') representation of IP
address in addr.
: static std::string AddrToString(const uint32_t addr);
> Nit: not really seeing why these args should be const; they're passed by va
Done
http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/util/net/net_util.h@141
PS10, Line 141: bool IsLoopBack() const;
> Nit: existing code (see socket.{cc,h}) refers to it as "Loopback", so let's
Done
--
To view, visit http://gerrit.cloudera.org:8080/12474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f
Gerrit-Change-Number: 12474
Gerrit-PatchSet: 10
Gerrit-Owner: Greg Solovyev
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 16 Feb 2019 02:18:24 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12474 to look at the new patch set (#13). Change subject: KUDU-1900: add loopback check and test .. KUDU-1900: add loopback check and test This change modifies Socket::IsLoopbackConnection to check if remote IP is in 127.0.0.0/8 subnet. With this change all connections from 127.0.0.0/8 are treated as local. This change adds force_external_client_ip parameter to AuthTokenIssuingTest. Before this change, AuthTokenIssuingTest relied on Kudu treating connections between non-matching addresses in 127.0.0.0/8 subnet as external. With this change, the test forces Kudu client to use an non-loopback IP address for test cases that verify external connections. If an external IP address is not available, test cases that require an external IP will be skipped. This change adds a test case to AuthTokenIssuingTest which verifies that auth token is passed through unencrypted local connections. For convenience, this change adds two static utility methods (HostPort::IsLoopback and HostPort::AddrToString), and refactors Sockaddr::host and Sockaddr::IsAnyLocalAddress to use these static methods. Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f --- M src/kudu/integration-tests/security-itest.cc M src/kudu/util/net/net_util.cc M src/kudu/util/net/net_util.h M src/kudu/util/net/sockaddr.cc M src/kudu/util/net/socket.cc 5 files changed, 164 insertions(+), 32 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/12474/13 -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 13 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-1900: add loopback check and test
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12474 )
Change subject: KUDU-1900: add loopback check and test
..
Patch Set 12:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/integration-tests/security-itest.cc@330
PS10, Line 330: KUDU_RETURN_NOT_OK_LOG(GetLocalNetworks(_networks),
ERROR,
Use RETURN_NOT_OK_PREPEND instead; that's what we typically do.
Or just RETURN_NOT_OK; it's just a test so the extra debuggability of the
message prefix isn't important.
http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/integration-tests/security-itest.cc@334
PS10, Line 334: if (!network.IsLoopBack()) {
Nit: indentation
http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/util/net/net_util.h
File src/kudu/util/net/net_util.h:
http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/util/net/net_util.h@95
PS10, Line 95: // Returns true if addr is within 127.0.0.0/8 range.
: static bool IsLoopBack(const uint32_t addr);
:
: // Returns dotted-decimal ('1.2.3.4') representation of IP
address in addr.
: static std::string AddrToString(const uint32_t addr);
Nit: not really seeing why these args should be const; they're passed by value
anyway so there's no effect on the caller's copy.
http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/util/net/net_util.h@141
PS10, Line 141: bool IsLoopBack() const;
Nit: existing code (see socket.{cc,h}) refers to it as "Loopback", so let's
retain that convention.
Above too.
http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/util/net/socket.cc
File src/kudu/util/net/socket.cc:
http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/util/net/socket.cc@304
PS10, Line 304: return true;
Do we need to further condition this local.IsAnyLocalAddress()? I'm guessing
the answer is 'no' because it is assumed to be the case, but wanted to double
check as it's something Alexey mentioned in KUDU-1900 (though Dan did not).
--
To view, visit http://gerrit.cloudera.org:8080/12474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f
Gerrit-Change-Number: 12474
Gerrit-PatchSet: 12
Gerrit-Owner: Greg Solovyev
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 16 Feb 2019 02:03:47 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12474 to look at the new patch set (#12). Change subject: KUDU-1900: add loopback check and test .. KUDU-1900: add loopback check and test This change modifies Socket::IsLoopbackConnection to check if remote IP is in 127.0.0.0/8 subnet. With this change all connections from 127.0.0.0/8 are treated as local. This change adds force_external_client_ip parameter to AuthTokenIssuingTest. Before this change, AuthTokenIssuingTest relied on Kudu treating connections between non-matching addresses in 127.0.0.0/8 subnet as external. With this change, the test forces Kudu client to use an non-loopback IP address for test cases that verify external connections. If an external IP address is not available, test cases that require an external IP will be skipped. This change adds a test case to AuthTokenIssuingTest which verifies that auth token is passed through unencrypted local connections. For convenience, this change adds two static utility methods (HostPort::IsLoopBack and HostPort::AddrToString), and refactors Sockaddr::host and Sockaddr::IsAnyLocalAddress to use these static methods. Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f --- M src/kudu/integration-tests/security-itest.cc M src/kudu/util/net/net_util.cc M src/kudu/util/net/net_util.h M src/kudu/util/net/sockaddr.cc M src/kudu/util/net/socket.cc 5 files changed, 164 insertions(+), 32 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/12474/12 -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 12 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/12474 ) Change subject: KUDU-1900: add loopback check and test .. Patch Set 11: I also ran security-itest on my mac with wifi turned off and I see that test cases that require external IP are being skipped with "Skipping external connection test." message in test log. -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 11 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sat, 16 Feb 2019 01:17:22 + Gerrit-HasComments: No
[kudu-CR] KUDU-1900: add loopback check and test
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12474 to look at the new patch set (#11). Change subject: KUDU-1900: add loopback check and test .. KUDU-1900: add loopback check and test This change modifies Socket::IsLoopbackConnection to check if remote IP is in 127.0.0.0/8 subnet. With this change all connections from 127.0.0.0/8 are treated as local. This change adds force_external_client_ip parameter to AuthTokenIssuingTest. Before this change, AuthTokenIssuingTest relied on Kudu treating connections between non-matching addresses in 127.0.0.0/8 subnet as external. With this change, the test forces Kudu client to use an non-loopback IP address for test cases that verify external connections. If an external IP address is not available, test cases that require an external IP will be skipped. This change adds a test case to AuthTokenIssuingTest which verifies that auth token is passed through unencrypted local connections. For convenience, this change adds two static utility methods (HostPort::IsLoopBack and HostPort::AddrToString), and refactors Sockaddr::host and Sockaddr::IsAnyLocalAddress to use these static methods. Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f --- M src/kudu/integration-tests/security-itest.cc M src/kudu/util/net/net_util.cc M src/kudu/util/net/net_util.h M src/kudu/util/net/sockaddr.cc M src/kudu/util/net/socket.cc 5 files changed, 164 insertions(+), 32 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/12474/11 -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 11 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-1900: add loopback check and test
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12474 to look at the new patch set (#10). Change subject: KUDU-1900: add loopback check and test .. KUDU-1900: add loopback check and test This change modifies Socket::IsLoopbackConnection to check if remote IP is in 127.0.0.0/8 subnet. With this change all connections from 127.0.0.0/8 are treated as local. This change adds force_external_client_ip parameter to AuthTokenIssuingTest. Before this change, AuthTokenIssuingTest relied on Kudu treating connections between non-matching addresses in 127.0.0.0/8 subnet as external. With this change, the test forces Kudu client to use an non-loopback IP address for test cases that verify external connections. If an external IP address is not available, test cases that require an external IP will be skipped. This change adds a test case to AuthTokenIssuingTest which verifies that auth token is passed through unencrypted local connections. For convenience, this change adds two static utility methods (HostPort::IsLoopBack and HostPort::AddrToString), and refactors Sockaddr::host and Sockaddr::IsAnyLocalAddress to use these static methods. Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f --- M src/kudu/integration-tests/security-itest.cc M src/kudu/util/net/net_util.cc M src/kudu/util/net/net_util.h M src/kudu/util/net/sockaddr.cc M src/kudu/util/net/socket.cc 5 files changed, 164 insertions(+), 31 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/12474/10 -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 10 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/12474 ) Change subject: KUDU-1900: add loopback check and test .. Patch Set 9: Code-Review-1 I am going to upload one more change eliminating duplication of code between SockAddr and Network -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 9 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 15 Feb 2019 23:57:46 + Gerrit-HasComments: No
[kudu-CR] KUDU-1900: add loopback check and test
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12474 to look at the new patch set (#9). Change subject: KUDU-1900: add loopback check and test .. KUDU-1900: add loopback check and test This change modifies Socket::IsLoopbackConnection to check if remote IP is in 127.0.0.0/8 subnet. With this change all connections from 127.0.0.0/8 are treated as local. This change adds force_external_client_ip parameter to AuthTokenIssuingTest. Before this change, AuthTokenIssuingTest relied on Kudu treating connections between non-matching addresses in 127.0.0.0/8 subnet as external. With this change, the test forces Kudu client to use an non-loopback IP address for test cases that verify external connections. If an external IP address is not available, test cases that require an external IP will be skipped. This change adds a test case to AuthTokenIssuingTest which verifies that auth token is passed through unencrypted local connections. For convenience, this change also adds two members to Network class: Network::IsLoopBack Network GetAddrAsString Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f --- M src/kudu/integration-tests/security-itest.cc M src/kudu/util/net/net_util.cc M src/kudu/util/net/net_util.h M src/kudu/util/net/socket.cc 4 files changed, 148 insertions(+), 27 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/12474/9 -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 9 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-1900: add loopback check and test
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12474 to look at the new patch set (#7). Change subject: KUDU-1900: add loopback check and test .. KUDU-1900: add loopback check and test This change modifies Socket::IsLoopbackConnection to check if remote IP is in 127.0.0.0/8 subnet. With this change all connections from 127.0.0.0/8 are treated as local. This change adds force_external_client_ip parameter to AuthTokenIssuingTest. Before this change, AuthTokenIssuingTest relied on Kudu treating connections between non-matching addresses in 127.0.0.0/8 subnet as external. With this change, the test forces Kudu client to use an non-loopback IP address for test cases that verify external connections. If an external IP address is not available, test cases that require an external IP will be skipped. This change adds a test case to AuthTokenIssuingTest which verifies that auth token is passed through unencrypted local connections. For convenience, this change also adds two members to Network class: Network::IsLoopBack Network GetAddrAsString Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f --- M src/kudu/integration-tests/security-itest.cc M src/kudu/util/net/net_util.cc M src/kudu/util/net/net_util.h M src/kudu/util/net/socket.cc 4 files changed, 148 insertions(+), 27 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/12474/7 -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 7 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-1900: add loopback check and test
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12474 to look at the new patch set (#8). Change subject: KUDU-1900: add loopback check and test .. KUDU-1900: add loopback check and test This change modifies Socket::IsLoopbackConnection to check if remote IP is in 127.0.0.0/8 subnet. With this change all connections from 127.0.0.0/8 are treated as local. This change adds force_external_client_ip parameter to AuthTokenIssuingTest. Before this change, AuthTokenIssuingTest relied on Kudu treating connections between non-matching addresses in 127.0.0.0/8 subnet as external. With this change, the test forces Kudu client to use an non-loopback IP address for test cases that verify external connections. If an external IP address is not available, test cases that require an external IP will be skipped. This change adds a test case to AuthTokenIssuingTest which verifies that auth token is passed through unencrypted local connections. For convenience, this change also adds two members to Network class: Network::IsLoopBack Network GetAddrAsString Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f --- M src/kudu/integration-tests/security-itest.cc M src/kudu/util/net/net_util.cc M src/kudu/util/net/net_util.h M src/kudu/util/net/socket.cc 4 files changed, 148 insertions(+), 27 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/12474/8 -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 8 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has posted comments on this change. (
http://gerrit.cloudera.org:8080/12474 )
Change subject: KUDU-1900: add loopback check and test
..
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@337
PS6, Line 337: if (GetLocalNetworks(_networks).ok()) {
> That's not a hard stop though; by skipping the remainder of the test, it'll
Done
--
To view, visit http://gerrit.cloudera.org:8080/12474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f
Gerrit-Change-Number: 12474
Gerrit-PatchSet: 6
Gerrit-Owner: Greg Solovyev
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 15 Feb 2019 22:38:39 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has posted comments on this change. (
http://gerrit.cloudera.org:8080/12474 )
Change subject: KUDU-1900: add loopback check and test
..
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@341
PS6, Line 341: if ((NetworkByteOrder::FromHost32(addr) >> 24) != 127) {
> That'd be fine too; any solution that works within our existing abstraction
Done
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@342
PS6, Line 342: char s[INET_ADDRSTRLEN];
: inet_ntop(AF_INET, , s, INET_ADDRSTRLEN);
> Not really. My view is that I want as much platform-specific code (i.e. all
Done
--
To view, visit http://gerrit.cloudera.org:8080/12474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f
Gerrit-Change-Number: 12474
Gerrit-PatchSet: 6
Gerrit-Owner: Greg Solovyev
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 15 Feb 2019 18:16:23 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/12474 ) Change subject: KUDU-1900: add loopback check and test .. Patch Set 6: (3 comments) http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@330 PS6, Line 330: LOG(INFO) << "Using 127.0.0.1 for outbound sockets"; > nit: how useful is this information? I guess it was just for initial debug pardon, forgot to remove. This was for debugging purpose while I was trying to figure out how this code worked http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@361 PS6, Line 361: const char* AUTH_REQUIRED > nit: to protect the pointer itself from modifications, use Done http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@363 PS6, Line 363: RPC_ > nit: the auth-related stuff is RPC-related as well, so since you have just Done -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 6 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 15 Feb 2019 18:11:43 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12474 ) Change subject: KUDU-1900: add loopback check and test .. Patch Set 6: (3 comments) http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@330 PS6, Line 330: LOG(INFO) << "Using 127.0.0.1 for outbound sockets"; nit: how useful is this information? I guess it was just for initial debugging since the other case (external IP address) is not logged. If that's true, consider not logging it at all. Our tests are too chatty, and sometimes it looks like a flood by non-so-relevant information. http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@361 PS6, Line 361: const char* AUTH_REQUIRED nit: to protect the pointer itself from modifications, use const char* const AUTH_REQUIRED here and below http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@363 PS6, Line 363: RPC_ nit: the auth-related stuff is RPC-related as well, so since you have just AUTH_REQUIRED and AUTH_DISABLED, maybe use just ENCRYPTION_REQUIRED and ENCRYPTION_DISABLED -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 6 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 15 Feb 2019 06:21:47 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12474 )
Change subject: KUDU-1900: add loopback check and test
..
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@341
PS6, Line 341: if ((NetworkByteOrder::FromHost32(addr) >> 24) != 127) {
> Should I instantiate Sockaddr and use Sockaddr::IsAnyLocalAddress instead?
That'd be fine too; any solution that works within our existing abstractions is
preferable.
--
To view, visit http://gerrit.cloudera.org:8080/12474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f
Gerrit-Change-Number: 12474
Gerrit-PatchSet: 6
Gerrit-Owner: Greg Solovyev
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 15 Feb 2019 01:30:58 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has posted comments on this change. (
http://gerrit.cloudera.org:8080/12474 )
Change subject: KUDU-1900: add loopback check and test
..
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@341
PS6, Line 341: if ((NetworkByteOrder::FromHost32(addr) >> 24) != 127) {
> Likewise, this should be encapsulated in a new function in Network. Maybe s
Should I instantiate Sockaddr and use Sockaddr::IsAnyLocalAddress instead?
--
To view, visit http://gerrit.cloudera.org:8080/12474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f
Gerrit-Change-Number: 12474
Gerrit-PatchSet: 6
Gerrit-Owner: Greg Solovyev
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 15 Feb 2019 01:23:25 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12474 )
Change subject: KUDU-1900: add loopback check and test
..
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@337
PS6, Line 337: if (GetLocalNetworks(_networks).ok()) {
> I think, effectively, this is a hard stop to the current test case. If GetL
That's not a hard stop though; by skipping the remainder of the test, it'll be
marked as PASSED.
A hard stop would be an ASSERT failure that'd mark the test as FAILED. Or a
CHECK failure that crashes the test.
Put another way: does !GetLocalNetworks.ok() signify an unexpected error in the
platform environment? Or an error that might be expected and fully
deterministic on some platforms? Based on a reading of the GetLocalNetworks
code, it looks like the former to me, in which case we should treat it the same
way as e.g. a failure to read a test file: fail the test.
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@342
PS6, Line 342: char s[INET_ADDRSTRLEN];
: inet_ntop(AF_INET, , s, INET_ADDRSTRLEN);
> I wasn't sure if creating new members inside Network is warranted until the
Not really. My view is that I want as much platform-specific code (i.e. all
those headers you had to add) out of random tests, and squirreled away behind
util classes. The Network class purports to provide an abstraction for
interacting with an IPv4 network; adding more methods to it rounds out the
abstraction and makes it more useful for future use cases.
It's especially non-concerning given that the two methods I suggested are
read-only; they just provide slightly different views/transforms of the
abstraction.
--
To view, visit http://gerrit.cloudera.org:8080/12474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f
Gerrit-Change-Number: 12474
Gerrit-PatchSet: 6
Gerrit-Owner: Greg Solovyev
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 15 Feb 2019 00:51:02 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has posted comments on this change. (
http://gerrit.cloudera.org:8080/12474 )
Change subject: KUDU-1900: add loopback check and test
..
Patch Set 6:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@376
PS2, Line 376: public SecurityITest,
> external
Done
http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@378
PS2, Line 378: };
> The initialization isn't necessary; getifaddrs() is going to overwrite 'ifa
Done
http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@380
PS2, Line 380: INSTANTIATE_TEST_CASE_P(, AuthTokenIssuingTest,
::testing::ValuesIn(
> Would be cleaner if this could early-out rather than introduce a new nested
Done
http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@382
PS2, Line 382: // The following 3 test cases cover passing authn token
over an
> Wrap, too long.
Done
http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@383
PS2, Line 383: // encrypted loopback connection.
> Use C++-style casts here (static_cast or reinterpret_cast).
Done
http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@388
PS2, Line 388:
> Probably clearer as:
Done
http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@392
PS2, Line 392: { BindMode::LOOPBACK, AUTH_DISABLED,
RPC_ENCRYPTION_DISABLED,
> Use RAII principles: set up a SCOPED_CLEANUP that calls freeifaddrs() when
Done
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@325
PS6, Line 325: bool assignIPToClient(bool external) {
> Should use full camel-case: AssignIPToClient.
Done
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@337
PS6, Line 337: if (GetLocalNetworks(_networks).ok()) {
> In the context of a test, I think we should hard stop if we encounter an un
I think, effectively, this is a hard stop to the current test case. If
GetLocalNetworks fails, this function returns FALSE and the caller will issue a
warning and move on to the next test case.
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@342
PS6, Line 342: char s[INET_ADDRSTRLEN];
: inet_ntop(AF_INET, , s, INET_ADDRSTRLEN);
> Could you encapsulate this into a new function inside Network? Seems like N
I wasn't sure if creating new members inside Network is warranted until there
is at least one non-test use case for them and until there is at least more
than one use case for them. In other words, wouldn't this be premature
optimization?
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@463
PS6, Line 463:
> Nit: extra space here.
Done
--
To view, visit http://gerrit.cloudera.org:8080/12474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f
Gerrit-Change-Number: 12474
Gerrit-PatchSet: 6
Gerrit-Owner: Greg Solovyev
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 15 Feb 2019 00:35:28 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12474 )
Change subject: KUDU-1900: add loopback check and test
..
Patch Set 6:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@325
PS6, Line 325: bool assignIPToClient(bool external) {
Should use full camel-case: AssignIPToClient.
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@337
PS6, Line 337: if (GetLocalNetworks(_networks).ok()) {
In the context of a test, I think we should hard stop if we encounter an
unexpected situation, such as GetLocalNetworks failing.
To that end, could you modify this function to return a Status, wrap this call
in RETURN_NOT_OK, and communicate the success or failure of assignment via a
bool* OUT parameter?
Alternatively, you could treat an OK return result as meaning that all calls
succeeded and a local IP was assigned, and a NotFound return result as a
failure in assignment.
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@338
PS6, Line 338: for (vector::iterator it = local_networks.begin();
: it != local_networks.end(); ++it) {
You should use a range-based for loop, new to C++11:
for (const auto& network : local_networks) {
https://en.cppreference.com/w/cpp/language/range-for has more info.
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@341
PS6, Line 341: if ((NetworkByteOrder::FromHost32(addr) >> 24) != 127) {
Likewise, this should be encapsulated in a new function in Network. Maybe
something like "IsLocalhost"?
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@342
PS6, Line 342: char s[INET_ADDRSTRLEN];
: inet_ntop(AF_INET, , s, INET_ADDRSTRLEN);
Could you encapsulate this into a new function inside Network? Seems like
Network::ParseCIDRString is similar (though it does the opposite work).
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@463
PS6, Line 463:
Nit: extra space here.
--
To view, visit http://gerrit.cloudera.org:8080/12474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f
Gerrit-Change-Number: 12474
Gerrit-PatchSet: 6
Gerrit-Owner: Greg Solovyev
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 15 Feb 2019 00:22:43 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/12490 ) Change subject: KUDU-1900: add loopback check and test .. Patch Set 1: pardon, messed up Change-Id and created a separate review -- To view, visit http://gerrit.cloudera.org:8080/12490 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I74b41235af1d1d8da3506f8c7ac93f226d093040 Gerrit-Change-Number: 12490 Gerrit-PatchSet: 1 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 14 Feb 2019 23:46:27 + Gerrit-HasComments: No
[kudu-CR] KUDU-1900: add loopback check and test
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12474 to look at the new patch set (#6). Change subject: KUDU-1900: add loopback check and test .. KUDU-1900: add loopback check and test This change modifies Socket::IsLoopbackConnection to check if remote IP is in 127.0.0.0/8 subnet. With this change all connections from 127.0.0.0/8 are treated as local. This change adds force_external_client_ip parameter to AuthTokenIssuingTest. Before this change, AuthTokenIssuingTest relied on Kudu treating connections between non-matching addresses in 127.0.0.0/8 subnet as external. With this change, the test forces Kudu client to use an non-loopback IP address for test cases that verify external connections. If an external IP address is not available, test cases that require an external IP will be skipped. This change adds a test case to AuthTokenIssuingTest which verifies that auth token is passed through unencrypted local connections. Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f --- M src/kudu/integration-tests/security-itest.cc M src/kudu/util/net/socket.cc 2 files changed, 138 insertions(+), 27 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/12474/6 -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 6 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has abandoned this change. ( http://gerrit.cloudera.org:8080/12490 ) Change subject: KUDU-1900: add loopback check and test .. Abandoned -- To view, visit http://gerrit.cloudera.org:8080/12490 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: I74b41235af1d1d8da3506f8c7ac93f226d093040 Gerrit-Change-Number: 12490 Gerrit-PatchSet: 1 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12490 Change subject: KUDU-1900: add loopback check and test .. KUDU-1900: add loopback check and test This change modifies Socket::IsLoopbackConnection to check if remote IP is in 127.0.0.0/8 subnet. With this change all connections from 127.0.0.0/8 are treated as local. This change adds force_external_client_ip parameter to AuthTokenIssuingTest. Before this change, AuthTokenIssuingTest relied on Kudu treating connections between non-matching addresses in 127.0.0.0/8 subnet as external. With this change, the test forces Kudu client to use an non-loopback IP address for test cases that verify external connections. If an external IP address is not available, test cases that require an external IP will be skipped. This change adds a test case to AuthTokenIssuingTest which verifies that auth token is passed through unencrypted local connections. Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f address comments Change-Id: I74b41235af1d1d8da3506f8c7ac93f226d093040 --- M src/kudu/integration-tests/security-itest.cc M src/kudu/util/net/socket.cc 2 files changed, 138 insertions(+), 27 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/12490/1 -- To view, visit http://gerrit.cloudera.org:8080/12490 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I74b41235af1d1d8da3506f8c7ac93f226d093040 Gerrit-Change-Number: 12490 Gerrit-PatchSet: 1 Gerrit-Owner: Greg Solovyev
[kudu-CR] KUDU-1900: add loopback check and test
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12474 )
Change subject: KUDU-1900: add loopback check and test
..
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@335
PS5, Line 335: struct ifaddrs *ifap;
: if (getifaddrs() > -1) {
: SCOPED_CLEANUP({
: freeifaddrs(ifap);
: });
: for (struct ifaddrs *ifa = ifap; ifa; ifa = ifa->ifa_next) {
: if (ifa->ifa_addr == nullptr || ifa->ifa_netmask == nullptr
: || ifa->ifa_addr->sa_family != AF_INET)
: continue;
:
: struct sockaddr_in *addr_in = reinterpret_cast(ifa->ifa_addr);
: if
((NetworkByteOrder::FromHost32(addr_in->sin_addr.s_addr) >> 24) != 127) {
: char s[INET_ADDRSTRLEN];
: inet_ntop(AF_INET, &(addr_in->sin_addr), s,
INET_ADDRSTRLEN);
: FLAGS_local_ip_for_outbound_sockets = string(s,
arraysize(s));
: // Found and assigned an external IP.
: return true;
: }
: }
: }
> I would not be too much concerned with efficiency in this particular case b
I didn't even realize we had existing code for this in net_util.{h,cc]. Yes, we
should _definitely_ reuse that code. Efficiency isn't a concern in an
integration test like this.
--
To view, visit http://gerrit.cloudera.org:8080/12474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f
Gerrit-Change-Number: 12474
Gerrit-PatchSet: 5
Gerrit-Owner: Greg Solovyev
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 14 Feb 2019 22:38:56 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12474 ) Change subject: KUDU-1900: add loopback check and test .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@406 PS5, Line 406: true > What is the combination of rpc_encrypt_loopback_connections, rpc_authentica The idea behind passing or not passing authn token over the connection is simple: if the issuer (or just the holder) of the token knows that the connection is confidential, the token can be passed over the connection. Otherwise, the holder of the token should not pass the token over the connection. The 'confidential' means that there is no risk of capturing the token by sniffers over the wire. And we should assume that all loopback connections are confidential, I think. I suggest to leave this as is now, and maybe address in a separate patch. -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 5 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 14 Feb 2019 20:49:26 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/12474 ) Change subject: KUDU-1900: add loopback check and test .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@406 PS5, Line 406: true > Something is fishy here: if in case of encrypted loopback connection betwee What is the combination of rpc_encrypt_loopback_connections, rpc_authentication and rpc_encryption flags that will result in authn token not being passed from local connection to local connection? -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 5 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 14 Feb 2019 20:25:08 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/12474 )
Change subject: KUDU-1900: add loopback check and test
..
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@335
PS5, Line 335: struct ifaddrs *ifap;
: if (getifaddrs() > -1) {
: SCOPED_CLEANUP({
: freeifaddrs(ifap);
: });
: for (struct ifaddrs *ifa = ifap; ifa; ifa = ifa->ifa_next) {
: if (ifa->ifa_addr == nullptr || ifa->ifa_netmask == nullptr
: || ifa->ifa_addr->sa_family != AF_INET)
: continue;
:
: struct sockaddr_in *addr_in = reinterpret_cast(ifa->ifa_addr);
: if
((NetworkByteOrder::FromHost32(addr_in->sin_addr.s_addr) >> 24) != 127) {
: char s[INET_ADDRSTRLEN];
: inet_ntop(AF_INET, &(addr_in->sin_addr), s,
INET_ADDRSTRLEN);
: FLAGS_local_ip_for_outbound_sockets = string(s,
arraysize(s));
: // Found and assigned an external IP.
: return true;
: }
: }
: }
> Yes, but it will be less efficient, because GetLocalNetworks will loop thro
I would not be too much concerned with efficiency in this particular case
because it's just a test. However, if you feel strong about this, maybe think
about making it more generic and moving into net_util.{h,cc}.
http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@375
PS5, Line 375: encrypted
> So, that's something I would like to ask you back :) When we set --rpc_encr
Effectively, the application of the --rpc_encrypt_loopback_connections=true
flag is gated by the -rpc_encryption=disabled setting (see
shttps://github.com/apache/kudu/blob/4e2451dbf18db1faf9545ac1f9663d378b9f5efe/src/kudu/rpc/server_negotiation.cc#L509)
http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@406
PS5, Line 406: true
> hm... yes, there is something wrong here. Token should not be passed here
Something is fishy here: if in case of encrypted loopback connection between
the client and the server the authz token is not present (as I can see it from
line 403), then it's a bit surprising that the token is present in this case.
At least, it would be nice to add some comment explaining why that's the case.
And it seems I know why:
https://github.com/apache/kudu/blob/4e2451dbf18db1faf9545ac1f9663d378b9f5efe/src/kudu/rpc/negotiation.cc#L279
Probably, that's a bug. Let's just add a comment and move further -- I think
it's worth addressing that issue in a separate changelist.
--
To view, visit http://gerrit.cloudera.org:8080/12474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f
Gerrit-Change-Number: 12474
Gerrit-PatchSet: 5
Gerrit-Owner: Greg Solovyev
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 14 Feb 2019 19:48:22 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/12474 ) Change subject: KUDU-1900: add loopback check and test .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@406 PS5, Line 406: true > No, this is not a typo. This test passes. A token is passed over unencrypte hm... yes, there is something wrong here. Token should not be passed here -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 5 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 14 Feb 2019 19:15:11 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/12474 ) Change subject: KUDU-1900: add loopback check and test .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@406 PS5, Line 406: true > Is this typo? No, this is not a typo. This test passes. A token is passed over unencrypted local connection from 127.0.0.0 (client) to 127.x.x.x (mini cluster) -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 5 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 14 Feb 2019 18:36:57 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has posted comments on this change. (
http://gerrit.cloudera.org:8080/12474 )
Change subject: KUDU-1900: add loopback check and test
..
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@375
PS5, Line 375: encrypted
> Wait, but what about { BindMode::LOOPBACK, "disabled", "disabled", true, f
So, that's something I would like to ask you back :) When we set
--rpc_encryption=disabled and --rpc_encrypt_loopback_connections=true will a
loopback connection be encrypted?
--
To view, visit http://gerrit.cloudera.org:8080/12474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f
Gerrit-Change-Number: 12474
Gerrit-PatchSet: 5
Gerrit-Owner: Greg Solovyev
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 14 Feb 2019 18:33:04 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has posted comments on this change. (
http://gerrit.cloudera.org:8080/12474 )
Change subject: KUDU-1900: add loopback check and test
..
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@335
PS5, Line 335: struct ifaddrs *ifap;
: if (getifaddrs() > -1) {
: SCOPED_CLEANUP({
: freeifaddrs(ifap);
: });
: for (struct ifaddrs *ifa = ifap; ifa; ifa = ifa->ifa_next) {
: if (ifa->ifa_addr == nullptr || ifa->ifa_netmask == nullptr
: || ifa->ifa_addr->sa_family != AF_INET)
: continue;
:
: struct sockaddr_in *addr_in = reinterpret_cast(ifa->ifa_addr);
: if
((NetworkByteOrder::FromHost32(addr_in->sin_addr.s_addr) >> 24) != 127) {
: char s[INET_ADDRSTRLEN];
: inet_ntop(AF_INET, &(addr_in->sin_addr), s,
INET_ADDRSTRLEN);
: FLAGS_local_ip_for_outbound_sockets = string(s,
arraysize(s));
: // Found and assigned an external IP.
: return true;
: }
: }
: }
> Is it possible to call kudu::GetLocalNetworks() and then just work with the
Yes, but it will be less efficient, because GetLocalNetworks will loop through
all local interfaces and put them into an array, then this test will have to
loop through the array and find non-loopback interfaces. At the same time, we
are talking about the efficiency of 2-3 repetitions, so I don't have a strong
opinion here
--
To view, visit http://gerrit.cloudera.org:8080/12474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f
Gerrit-Change-Number: 12474
Gerrit-PatchSet: 5
Gerrit-Owner: Greg Solovyev
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 14 Feb 2019 18:30:19 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/12474 )
Change subject: KUDU-1900: add loopback check and test
..
Patch Set 5:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12474/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12474/5//COMMIT_MSG@23
PS5, Line 23: unencripted auth token
Seems to be a typo. Also, what is unencrypted auth token? Per my knowledge,
we never encrypt authn/authz tokens themselves.
http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@335
PS5, Line 335: struct ifaddrs *ifap;
: if (getifaddrs() > -1) {
: SCOPED_CLEANUP({
: freeifaddrs(ifap);
: });
: for (struct ifaddrs *ifa = ifap; ifa; ifa = ifa->ifa_next) {
: if (ifa->ifa_addr == nullptr || ifa->ifa_netmask == nullptr
: || ifa->ifa_addr->sa_family != AF_INET)
: continue;
:
: struct sockaddr_in *addr_in = reinterpret_cast(ifa->ifa_addr);
: if
((NetworkByteOrder::FromHost32(addr_in->sin_addr.s_addr) >> 24) != 127) {
: char s[INET_ADDRSTRLEN];
: inet_ntop(AF_INET, &(addr_in->sin_addr), s,
INET_ADDRSTRLEN);
: FLAGS_local_ip_for_outbound_sockets = string(s,
arraysize(s));
: // Found and assigned an external IP.
: return true;
: }
: }
: }
Is it possible to call kudu::GetLocalNetworks() and then just work with the
result vector to extract non-loopback addresses? While doing so, feel
free to add new useful functions into src/kudu/util/net/net_util.{h,cc}
http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@375
PS5, Line 375: encrypted
Wait, but what about { BindMode::LOOPBACK, "disabled", "disabled", true,
false, false, } ? That's the case when connections are not encrypted, right?
http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@382
PS5, Line 382: { BindMode::LOOPBACK, "required", "required", false,
false, true, },
: { BindMode::LOOPBACK, "disabled", "required", false,
false, true, },
: { BindMode::LOOPBACK, "disabled", "disabled", false,
false, true, },
When it became 3 boolean parameters, it's harder to read this matrix. Maybe,
define some boolean constants with sound names for better readability and use
them instead of just false/true?
With that a configuration entry would look like:
{ BindMode::LOOPBACK, "required", "required", LOOPBACK_UNENCRYPTED,
CLIENT_IP_EXTERNAL, TOKEN_PRESENT, }
http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@406
PS5, Line 406: true
Is this typo?
http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@437
PS5, Line 437: if (!assignIPToClient(params.force_external_client_ip)) {
: LOG(WARNING) << "Skipping external connection test, because
the host does "
: "not have an external network interface.";
: return;
: }
Is it possible to make this check before starting cluster (i.e. StartCluster()
at line 429)?
--
To view, visit http://gerrit.cloudera.org:8080/12474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f
Gerrit-Change-Number: 12474
Gerrit-PatchSet: 5
Gerrit-Owner: Greg Solovyev
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 14 Feb 2019 07:06:12 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12474 to look at the new patch set (#5). Change subject: KUDU-1900: add loopback check and test .. KUDU-1900: add loopback check and test This change modifies Socket::IsLoopbackConnection to check if remote IP is in 127.0.0.0/8 subnet. With this change all connections from 127.0.0.0/8 are treated as local. This change adds force_external_client_ip parameter to AuthTokenIssuingTest. Before this change, AuthTokenIssuingTest relied on Kudu treating connections between non-matching addresses in 127.0.0.0/8 subnet as external. With this change, the test forces Kudu client to use an non-loopback IP address for test cases that verify external connections. If an external IP address is not available, test cases that require an external IP will be skipped. This change adds a test case to AuthTokenIssuingTest which verifies that unencripted auth token is passed through unencrypted local connections. Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f --- M src/kudu/integration-tests/security-itest.cc M src/kudu/util/net/socket.cc 2 files changed, 95 insertions(+), 26 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/12474/5 -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 5 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-1900: add loopback check and test
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12474 to look at the new patch set (#4). Change subject: KUDU-1900: add loopback check and test .. KUDU-1900: add loopback check and test This change modifies Socket::IsLoopbackConnection to check if remote IP is in 127.0.0.0/8 subnet. With this change all connections from 127.0.0.0/8 are treated as local. This change adds force_external_client_ip parameter to AuthTokenIssuingTest. Before this change, AuthTokenIssuingTest relied on Kudu treating connections between non-matching addresses in 127.0.0.0/8 subnet as external. With this change, the test forces Kudu client to use an non-loopback IP address for test cases that verify external connections. If an external IP address is not available, test cases that require an external IP will be skipped. This change adds a test case to AuthTokenIssuingTest which verifies that unencripted auth token is passed through unencrypted local connections. Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f --- M src/kudu/integration-tests/security-itest.cc M src/kudu/util/net/socket.cc 2 files changed, 93 insertions(+), 26 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/12474/4 -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 4 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-1900: add loopback check and test
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12474 to look at the new patch set (#3). Change subject: KUDU-1900: add loopback check and test .. KUDU-1900: add loopback check and test This change modifies Socket::IsLoopbackConnection to check if remote IP is in 127.0.0.0/8 subnet. With this change all connections from 127.0.0.0/8 are treated as local. This change adds force_external_client_ip parameter to AuthTokenIssuingTest. Before this change, AuthTokenIssuingTest relied on Kudu treating connections between non-matching addresses in 127.0.0.0/8 subnet as external. With this change, the test forces Kudu client to use an non-loopback IP address for test cases that verify external connections. If an external IP address is not available, test cases that require an external IP will be skipped. This change adds a test case to AuthTokenIssuingTest which verifies that unencripted auth token is passed through unencrypted local connections. Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f --- M src/kudu/integration-tests/security-itest.cc M src/kudu/util/net/socket.cc 2 files changed, 92 insertions(+), 26 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/12474/3 -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 3 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/12474 ) Change subject: KUDU-1900: add loopback check and test .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@18 PS2, Line 18: #include : #include > Is this approach portable to macOS? This works on macOS as well. I've ran the tests with external IP on my mac. -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 2 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 14 Feb 2019 04:29:01 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/12474 ) Change subject: KUDU-1900: add loopback check and test .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@331 PS2, Line 331: INSTANTIATE_TEST_CASE_P(, AuthTokenIssuingTest, ::testing::ValuesIn( > Would it be possible to use ::testing::Combine() to generate this more clea After another look at these, I think I should remove all cases with UNIQUE_LOOPBACK and replace some of them with cases that use an external IP. It looks to me that the only reason for introducing UNIQUE_LOOPBACK into this test was fooling Kudu into thinking that 127.0.0.1 to 127.x.x.x is a non-loopback connection. -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 2 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 14 Feb 2019 01:22:15 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12474 )
Change subject: KUDU-1900: add loopback check and test
..
Patch Set 2:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@18
PS2, Line 18: #include
: #include
Is this approach portable to macOS?
http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@331
PS2, Line 331: INSTANTIATE_TEST_CASE_P(, AuthTokenIssuingTest,
::testing::ValuesIn(
Would it be possible to use ::testing::Combine() to generate this more clearly?
Could we structure the generators such that we get the right combinations (i.e.
nearly the Cartesian product)?
http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@376
PS2, Line 376: // an extnernal IP, so that the connection is not considered
to be local
external
http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@378
PS2, Line 378: struct ifaddrs *ifap = nullptr;
The initialization isn't necessary; getifaddrs() is going to overwrite 'ifap'.
http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@380
PS2, Line 380: if (getifaddrs() > -1) {
Would be cleaner if this could early-out rather than introduce a new nested
scope. Maybe decompose all of this into a helper method?
http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@382
PS2, Line 382: if (ifa->ifa_addr == nullptr || ifa->ifa_netmask ==
nullptr || ifa->ifa_addr->sa_family != AF_INET) continue;
Wrap, too long.
http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@383
PS2, Line 383: struct sockaddr_in *addr_in = (struct sockaddr_in
*)ifa->ifa_addr;
Use C++-style casts here (static_cast or reinterpret_cast).
http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@388
PS2, Line 388: FLAGS_local_ip_for_outbound_sockets.assign(s,
INET_ADDRSTRLEN);
Probably clearer as:
FLAGS_foo = string(s, arraysize(s));
http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@392
PS2, Line 392: freeifaddrs(ifap);
Use RAII principles: set up a SCOPED_CLEANUP that calls freeifaddrs() when it
goes out of scope, declared just after getifaddrs() is known to have succeeded.
http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/util/net/socket.cc
File src/kudu/util/net/socket.cc:
http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/util/net/socket.cc@302
PS2, Line 302: // Check if remote address is in 127.0.0.0/8 subnet
Nit: terminate with a period.
Applies to the new comments in the test too.
http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/util/net/socket.cc@303
PS2, Line 303: if(
Nit: if (remote
--
To view, visit http://gerrit.cloudera.org:8080/12474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f
Gerrit-Change-Number: 12474
Gerrit-PatchSet: 2
Gerrit-Owner: Greg Solovyev
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 13 Feb 2019 22:53:15 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12474 to look at the new patch set (#2). Change subject: KUDU-1900: add loopback check and test .. KUDU-1900: add loopback check and test This change modifies Socket::IsLoopbackConnection to check if remote IP is in 127.0.0.0/8 subnet. With this change all connections from 127.0.0.0/8 are treated as local. This change adds force_external_client_ip parameter to AuthTokenIssuingTest. Before this change, AuthTokenIssuingTest relied on Kudu treating connections between non-matching addresses in 127.0.0.0/8 subnet as external. With this change, the test forces Kudu client to use an non-loopback IP address for test cases that verify external connections. If an external IP address is not available, test cases that require an external IP will be skipped. This change adds a test case to AuthTokenIssuingTest which verifies that unencripted auth token is passed through unencrypted local connections. Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f --- M src/kudu/integration-tests/security-itest.cc M src/kudu/util/net/socket.cc 2 files changed, 51 insertions(+), 19 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/12474/2 -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 2 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12474 Change subject: KUDU-1900: add loopback check and test .. KUDU-1900: add loopback check and test This change modifies Socket::IsLoopbackConnection to check if remote IP is in 127.0.0.0/8 subnet. With this change all connections from 127.0.0.0/8 are treated as local. This change adds force_external_client_ip parameter to AuthTokenIssuingTest. Before this change, AuthTokenIssuingTest relied on Kudu treating connections between non-matching addresses in 127.0.0.0/8 subnet as external. With this change, the test forces Kudu client to use an non-loopback IP address for test cases that verify external connections. If an external IP address is not available, test cases that require an external IP will be skipped. This change adds a test case to AuthTokenIssuingTest which verifies that unencripted auth token is passed through unencrypted local connections. Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f --- M src/kudu/integration-tests/security-itest.cc M src/kudu/util/net/socket.cc 2 files changed, 51 insertions(+), 19 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/12474/1 -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 1 Gerrit-Owner: Greg Solovyev
