kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From a...@apache.org
Subject [kudu] 03/03: KUDU-1900: add loopback check and test
Date Wed, 20 Feb 2019 18:29:41 GMT
This is an automated email from the ASF dual-hosted git repository.

adar pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git

commit 6278a2e0590c17ff2c56d0f3d3070c9c4acd10db
Author: gsolovyev <gsolovyev@ve0518.halxg.cloudera.com>
AuthorDate: Fri Feb 8 08:54:16 2019 -0800

    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 <adar@cloudera.com>
    Tested-by: Kudu Jenkins
---
 src/kudu/integration-tests/security-itest.cc | 150 ++++++++++++++++++++++-----
 src/kudu/util/net/net_util.cc                |  19 ++++
 src/kudu/util/net/net_util.h                 |  12 +++
 src/kudu/util/net/sockaddr.cc                |   7 +-
 src/kudu/util/net/socket.cc                  |   7 +-
 5 files changed, 163 insertions(+), 32 deletions(-)

diff --git a/src/kudu/integration-tests/security-itest.cc b/src/kudu/integration-tests/security-itest.cc
index acd6466..3b9b0a7 100644
--- a/src/kudu/integration-tests/security-itest.cc
+++ b/src/kudu/integration-tests/security-itest.cc
@@ -19,6 +19,7 @@
 
 #include <cstdio>
 #include <memory>
+#include <ostream>
 #include <string>
 #include <vector>
 
@@ -54,6 +55,7 @@
 #include "kudu/util/net/net_util.h"
 #include "kudu/util/net/sockaddr.h"
 #include "kudu/util/path_util.h"
+#include "kudu/util/slice.h"
 #include "kudu/util/status.h"
 #include "kudu/util/subprocess.h"
 #include "kudu/util/test_macros.h"
@@ -314,36 +316,124 @@ TEST_F(SecurityITest, TestWorldReadablePrivateKey) {
       credentials_name));
 }
 
+Status AssignIPToClient(bool external) {
+  // If the test does not require an external IP address
+  // assign loopback IP to FLAGS_local_ip_for_outbound_sockets.
+  if (!external) {
+    FLAGS_local_ip_for_outbound_sockets = "127.0.0.1";
+    return Status::OK();
+  }
+
+  // Try finding an external IP address to assign to
+  // FLAGS_local_ip_for_outbound_sockets.
+  vector<Network> local_networks;
+  RETURN_NOT_OK(GetLocalNetworks(&local_networks));
+
+  for (const auto& network : local_networks) {
+    if (!network.IsLoopback()) {
+      FLAGS_local_ip_for_outbound_sockets = network.GetAddrAsString();
+      // Found and assigned an external IP.
+      return Status::OK();
+    }
+  }
+
+  // Could not find an external IP.
+  return Status::NotFound("Could not find an external IP.");
+}
+
+// Descriptive constants for test parameters.
+const bool LOOPBACK_ENCRYPTED = true;
+const bool LOOPBACK_PLAIN = false;
+const bool TOKEN_PRESENT = true;
+const bool TOKEN_MISSING = false;
+const bool LOOPBACK_CLIENT_IP = false;
+const bool EXTERNAL_CLIENT_IP = true;
+const char* const AUTH_REQUIRED = "required";
+const char* const AUTH_DISABLED = "disabled";
+const char* const ENCRYPTION_REQUIRED = "required";
+const char* const ENCRYPTION_DISABLED = "disabled";
+
 struct AuthTokenIssuingTestParams {
   const BindMode bind_mode;
   const string rpc_authentication;
   const string rpc_encryption;
   const bool rpc_encrypt_loopback_connections;
+  const bool force_external_client_ip;
   const bool authn_token_present;
 };
+
 class AuthTokenIssuingTest :
     public SecurityITest,
     public ::testing::WithParamInterface<AuthTokenIssuingTestParams> {
 };
+
 INSTANTIATE_TEST_CASE_P(, AuthTokenIssuingTest, ::testing::ValuesIn(
     vector<AuthTokenIssuingTestParams>{
-      { BindMode::LOOPBACK, "required", "required", true,  true,  },
-      { BindMode::LOOPBACK, "required", "required", false, true,  },
-      //BindMode::LOOPBACK, "required", "disabled": non-acceptable
-      //BindMode::LOOPBACK, "required", "disabled": non-acceptable
-      { BindMode::LOOPBACK, "disabled", "required", true,  true,  },
-      { BindMode::LOOPBACK, "disabled", "required", false, true,  },
-      { BindMode::LOOPBACK, "disabled", "disabled", true,  false, },
-      { BindMode::LOOPBACK, "disabled", "disabled", false, true,  },
+      // The following 3 test cases cover passing authn token over an
+      // encrypted loopback connection.
+      // Note: AUTH_REQUIRED with ENCRYPTION_DISABLED is not
+      // an acceptable configuration.
+      { BindMode::LOOPBACK, AUTH_REQUIRED, ENCRYPTION_REQUIRED,
+        LOOPBACK_ENCRYPTED, LOOPBACK_CLIENT_IP, TOKEN_PRESENT, },
+
+      { BindMode::LOOPBACK, AUTH_DISABLED, ENCRYPTION_REQUIRED,
+        LOOPBACK_ENCRYPTED, LOOPBACK_CLIENT_IP, TOKEN_PRESENT, },
+
+      { BindMode::LOOPBACK, AUTH_DISABLED, ENCRYPTION_DISABLED,
+        LOOPBACK_ENCRYPTED, LOOPBACK_CLIENT_IP, TOKEN_MISSING, },
+
+      // The following 3 test cases cover passing authn token over an
+      // unencrypted loopback connection.
+      { BindMode::LOOPBACK, AUTH_REQUIRED, ENCRYPTION_REQUIRED,
+        LOOPBACK_PLAIN, LOOPBACK_CLIENT_IP, TOKEN_PRESENT, },
+
+      { BindMode::LOOPBACK, AUTH_DISABLED, ENCRYPTION_REQUIRED,
+        LOOPBACK_PLAIN, LOOPBACK_CLIENT_IP, TOKEN_PRESENT, },
+
+      { BindMode::LOOPBACK, AUTH_DISABLED, ENCRYPTION_DISABLED,
+        LOOPBACK_PLAIN, LOOPBACK_CLIENT_IP, TOKEN_PRESENT, },
+
+      // The following 3 test cases cover passing authn token over an
+      // external connection.
+      { BindMode::LOOPBACK, AUTH_REQUIRED, ENCRYPTION_REQUIRED,
+        LOOPBACK_PLAIN, EXTERNAL_CLIENT_IP, TOKEN_PRESENT, },
+
+      { BindMode::LOOPBACK, AUTH_DISABLED, ENCRYPTION_REQUIRED,
+        LOOPBACK_PLAIN, EXTERNAL_CLIENT_IP, TOKEN_PRESENT, },
+
+      { BindMode::LOOPBACK, AUTH_DISABLED, ENCRYPTION_DISABLED,
+        LOOPBACK_PLAIN, EXTERNAL_CLIENT_IP, TOKEN_MISSING, },
+
+      // The following 3 test cases verify that enforcement of encryption
+      // for loopback connections does not affect external connections.
+      { BindMode::LOOPBACK, AUTH_REQUIRED, ENCRYPTION_REQUIRED,
+        LOOPBACK_ENCRYPTED, EXTERNAL_CLIENT_IP, TOKEN_PRESENT, },
+
+      { BindMode::LOOPBACK, AUTH_DISABLED, ENCRYPTION_REQUIRED,
+        LOOPBACK_ENCRYPTED, EXTERNAL_CLIENT_IP, TOKEN_PRESENT, },
+
+      { BindMode::LOOPBACK, AUTH_DISABLED, ENCRYPTION_DISABLED,
+        LOOPBACK_ENCRYPTED, EXTERNAL_CLIENT_IP, TOKEN_MISSING, },
 #if defined(__linux__)
-      { BindMode::UNIQUE_LOOPBACK, "required", "required", true,  true,  },
-      { BindMode::UNIQUE_LOOPBACK, "required", "required", false, true,  },
-      //BindMode::UNIQUE_LOOPBACK, "required", "disabled": non-acceptable
-      //BindMode::UNIQUE_LOOPBACK, "required", "disabled": non-acceptable
-      { BindMode::UNIQUE_LOOPBACK, "disabled", "required", true,  true,  },
-      { BindMode::UNIQUE_LOOPBACK, "disabled", "required", false, true,  },
-      { BindMode::UNIQUE_LOOPBACK, "disabled", "disabled", true,  false, },
-      { BindMode::UNIQUE_LOOPBACK, "disabled", "disabled", false, false, },
+      // The following 6 test cases verify that a connection from 127.0.0.1
+      // to another loopback address is treated as a loopback connection.
+      { BindMode::UNIQUE_LOOPBACK, AUTH_REQUIRED, ENCRYPTION_REQUIRED,
+        LOOPBACK_ENCRYPTED, LOOPBACK_CLIENT_IP, TOKEN_PRESENT, },
+
+      { BindMode::UNIQUE_LOOPBACK, AUTH_DISABLED, ENCRYPTION_REQUIRED,
+        LOOPBACK_ENCRYPTED, LOOPBACK_CLIENT_IP, TOKEN_PRESENT, },
+
+      { BindMode::UNIQUE_LOOPBACK, AUTH_DISABLED, ENCRYPTION_DISABLED,
+        LOOPBACK_ENCRYPTED, LOOPBACK_CLIENT_IP, TOKEN_MISSING, },
+
+      { BindMode::UNIQUE_LOOPBACK, AUTH_REQUIRED, ENCRYPTION_REQUIRED,
+        LOOPBACK_PLAIN, LOOPBACK_CLIENT_IP, TOKEN_PRESENT, },
+
+      { BindMode::UNIQUE_LOOPBACK, AUTH_DISABLED, ENCRYPTION_REQUIRED,
+        LOOPBACK_PLAIN, LOOPBACK_CLIENT_IP, TOKEN_PRESENT, },
+
+      { BindMode::UNIQUE_LOOPBACK, AUTH_DISABLED, ENCRYPTION_DISABLED,
+        LOOPBACK_PLAIN, LOOPBACK_CLIENT_IP, TOKEN_PRESENT, },
 #endif
     }
 ));
@@ -355,25 +445,35 @@ TEST_P(AuthTokenIssuingTest, ChannelConfidentiality) {
   cluster_opts_.num_masters = 1;
   cluster_opts_.num_tablet_servers = 0;
   // --user-acl: just restoring back the default setting.
-  cluster_opts_.extra_master_flags.emplace_back("--user-acl=*");
+  cluster_opts_.extra_master_flags.emplace_back("--user_acl=*");
 
   const auto& params = GetParam();
+
+  // When testing external connections, make sure the client connects from
+  // an external IP, so that the connection is not considered to be local.
+  // When testing local connections, make sure that the client
+  // connects from standard loopback IP.
+  // Skip tests that require an external connection
+  // if the host does not have a non-loopback interface.
+  Status s = AssignIPToClient(params.force_external_client_ip);
+  if (s.IsNotFound()) {
+    LOG(WARNING) << s.message().ToString() << "\nSkipping external connection
test.";
+    return;
+  }
+  // Fail if GetLocalNetworks failed to determine network interfaces.
+  ASSERT_OK(s);
+
+  // Set flags and start cluster.
   cluster_opts_.bind_mode = params.bind_mode;
   cluster_opts_.extra_master_flags.emplace_back(
-      Substitute("--rpc-authentication=$0", params.rpc_authentication));
+      Substitute("--rpc_authentication=$0", params.rpc_authentication));
   cluster_opts_.extra_master_flags.emplace_back(
-      Substitute("--rpc-encryption=$0", params.rpc_encryption));
+      Substitute("--rpc_encryption=$0", params.rpc_encryption));
   cluster_opts_.extra_master_flags.emplace_back(
       Substitute("--rpc_encrypt_loopback_connections=$0",
                  params.rpc_encrypt_loopback_connections));
   ASSERT_OK(StartCluster());
 
-  // Make sure the client always connects from the standard loopback address.
-  // This is crucial when the master is running with UNIQUE_LOOPBACK mode: the
-  // test scenario expects the client connects from other than 127.0.0.1 address
-  // so the connection is not considered a 'loopback' one.
-  FLAGS_local_ip_for_outbound_sockets = "127.0.0.1";
-
   // In current implementation, KuduClientBuilder calls ConnectToCluster() on
   // the newly created instance of the KuduClient.
   client::sp::shared_ptr<KuduClient> client;
diff --git a/src/kudu/util/net/net_util.cc b/src/kudu/util/net/net_util.cc
index d735616..6e719d0 100644
--- a/src/kudu/util/net/net_util.cc
+++ b/src/kudu/util/net/net_util.cc
@@ -15,6 +15,7 @@
 // specific language governing permissions and limitations
 // under the License.
 
+#include <arpa/inet.h>
 #include <sys/socket.h>
 #include <ifaddrs.h>
 #include <limits.h>
@@ -249,6 +250,16 @@ string HostPort::ToCommaSeparatedString(const vector<HostPort>&
hostports) {
   return JoinStrings(hostport_strs, ",");
 }
 
+bool HostPort::IsLoopback(uint32_t addr) {
+    return (NetworkByteOrder::FromHost32(addr) >> 24) == 127;
+}
+
+string HostPort::AddrToString(uint32_t addr) {
+  char str[INET_ADDRSTRLEN];
+  ::inet_ntop(AF_INET, &addr, str, INET_ADDRSTRLEN);
+  return str;
+}
+
 Network::Network()
   : addr_(0),
     netmask_(0) {
@@ -293,6 +304,14 @@ Status Network::ParseCIDRStrings(const string& comma_sep_addrs,
   return Status::OK();
 }
 
+bool Network::IsLoopback() const {
+  return HostPort::IsLoopback(addr_);
+}
+
+string Network::GetAddrAsString() const {
+  return HostPort::AddrToString(addr_);
+}
+
 bool IsPrivilegedPort(uint16_t port) {
   return port <= 1024 && port != 0;
 }
diff --git a/src/kudu/util/net/net_util.h b/src/kudu/util/net/net_util.h
index 13fb512..b904b00 100644
--- a/src/kudu/util/net/net_util.h
+++ b/src/kudu/util/net/net_util.h
@@ -92,6 +92,12 @@ class HostPort {
   // "inverse" of ParseStrings().
   static std::string ToCommaSeparatedString(const std::vector<HostPort>& host_ports);
 
+  // Returns true if addr is within 127.0.0.0/8 range.
+  static bool IsLoopback(uint32_t addr);
+
+  // Returns dotted-decimal ('1.2.3.4') representation of IP address in addr.
+  static std::string AddrToString(uint32_t addr);
+
  private:
   std::string host_;
   uint16_t port_;
@@ -131,6 +137,12 @@ class Network {
   // Returns true if the address is within network.
   bool WithinNetwork(const Sockaddr& addr) const;
 
+  // Returns true if the network is within 127.0.0.0/8 range.
+  bool IsLoopback() const;
+
+  // Returns addr part of addr:mask pair as string.
+  std::string GetAddrAsString() const;
+
   // Parses a "addr/netmask" (CIDR notation) pair into this object.
   Status ParseCIDRString(const std::string& addr);
 
diff --git a/src/kudu/util/net/sockaddr.cc b/src/kudu/util/net/sockaddr.cc
index 6efc4d8..cc100c6 100644
--- a/src/kudu/util/net/sockaddr.cc
+++ b/src/kudu/util/net/sockaddr.cc
@@ -25,7 +25,6 @@
 #include <cstring>
 #include <string>
 
-#include "kudu/gutil/endian.h"
 #include "kudu/gutil/hash/builtin_type_hash.h"
 #include "kudu/gutil/port.h"
 #include "kudu/gutil/strings/substitute.h"
@@ -89,9 +88,7 @@ int Sockaddr::port() const {
 }
 
 std::string Sockaddr::host() const {
-  char str[INET_ADDRSTRLEN];
-  ::inet_ntop(AF_INET, &addr_.sin_addr, str, INET_ADDRSTRLEN);
-  return str;
+  return HostPort::AddrToString(addr_.sin_addr.s_addr);
 }
 
 const struct sockaddr_in& Sockaddr::addr() const {
@@ -107,7 +104,7 @@ bool Sockaddr::IsWildcard() const {
 }
 
 bool Sockaddr::IsAnyLocalAddress() const {
-  return (NetworkByteOrder::FromHost32(addr_.sin_addr.s_addr) >> 24) == 127;
+  return HostPort::IsLoopback(addr_.sin_addr.s_addr);
 }
 
 Status Sockaddr::LookupHostname(string* hostname) const {
diff --git a/src/kudu/util/net/socket.cc b/src/kudu/util/net/socket.cc
index bc3b525..2ffb5cf 100644
--- a/src/kudu/util/net/socket.cc
+++ b/src/kudu/util/net/socket.cc
@@ -299,8 +299,11 @@ bool Socket::IsLoopbackConnection() const {
   Sockaddr local, remote;
   if (!GetSocketAddress(&local).ok()) return false;
   if (!GetPeerAddress(&remote).ok()) return false;
-
-  // Compare without comparing ports.
+  // Check if remote address is in 127.0.0.0/8 subnet.
+  if (remote.IsAnyLocalAddress()) {
+    return true;
+  }
+  // Compare local and remote addresses without comparing ports.
   local.set_port(0);
   remote.set_port(0);
   return local == remote;


Mime
View raw message