You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by tm...@apache.org on 2019/08/06 04:18:57 UTC
[impala] 04/04: IMPALA-8828: Support impersonation via http paths
This is an automated email from the ASF dual-hosted git repository.
tmarshall pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
commit bbe064ec194aff4ecf1e794bd4071df4ea4be166
Author: Thomas Tauber-Marshall <tm...@cloudera.com>
AuthorDate: Thu Aug 1 04:29:52 2019 +0000
IMPALA-8828: Support impersonation via http paths
This patch allows clients that connect over the HTTP server to specify
the 'doAs' parameter in the provided path in order to perform
impersonation.
The existing rules for impersonation are applied, i.e.
authorized_proxy_user_config or authorized_proxy_group_config must be
set with the appropriate values for impersonation to be successful.
Testing:
- Added a FE test that verifies impersonation works as expected with
impala-shell and ldap.
- Manually tested with Apache Knox.
Change-Id: I20b9c2e2d106530732f1c52f8d3d1ecc24ae4bd6
Reviewed-on: http://gerrit.cloudera.org:8080/13994
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
be/src/rpc/auth-provider.h | 4 +-
be/src/rpc/authentication.cc | 76 +++++++++++++++++--
be/src/rpc/thrift-server.h | 1 +
be/src/service/impala-hs2-server.cc | 13 +++-
be/src/transport/THttpServer.cpp | 12 ++-
be/src/transport/THttpServer.h | 49 ++++++------
.../impala/customcluster/LdapImpalaShellTest.java | 87 ++++++++++++++++++----
7 files changed, 182 insertions(+), 60 deletions(-)
diff --git a/be/src/rpc/auth-provider.h b/be/src/rpc/auth-provider.h
index 1807eb9..095ecac 100644
--- a/be/src/rpc/auth-provider.h
+++ b/be/src/rpc/auth-provider.h
@@ -194,9 +194,7 @@ class NoAuthProvider : public AuthProvider {
const boost::shared_ptr<ThriftServer::ConnectionContext>& connection_ptr,
ThriftServer::TransportType underlying_transport_type,
apache::thrift::transport::TTransport* underlying_input_transport,
- apache::thrift::transport::TTransport* underlying_output_transport) {
- connection_ptr->username = "";
- }
+ apache::thrift::transport::TTransport* underlying_output_transport);
virtual bool is_secure() { return false; }
};
diff --git a/be/src/rpc/authentication.cc b/be/src/rpc/authentication.cc
index ef8e1db..a7c04ee 100644
--- a/be/src/rpc/authentication.cc
+++ b/be/src/rpc/authentication.cc
@@ -26,6 +26,7 @@
#include <boost/filesystem.hpp>
#include <gutil/casts.h>
#include <gutil/strings/escaping.h>
+#include <gutil/strings/split.h>
#include <gutil/strings/strip.h>
#include <gutil/strings/substitute.h>
#include <random>
@@ -47,6 +48,7 @@
#include "transport/THttpServer.h"
#include "transport/TSaslClientTransport.h"
#include "util/auth-util.h"
+#include "util/coding-util.h"
#include "util/debug-util.h"
#include "util/error-util.h"
#include "util/network-util.h"
@@ -577,6 +579,31 @@ vector<string> ReturnHeaders(ThriftServer::ConnectionContext* connection_context
return std::move(connection_context->return_headers);
}
+// Takes the path component of an HTTP request and parses it. For now, we only care about
+// the 'doAs' parameter.
+bool HttpPathFn(ThriftServer::ConnectionContext* connection_context, const string& path,
+ string* err_msg) {
+ // 'path' should be of the form '/.*[?<key=value>[&<key=value>...]]'
+ vector<string> split = Split(path, delimiter::Limit("?", 1));
+ if (split.size() == 2) {
+ for (auto pair : Split(split[1], "&")) {
+ vector<string> key_value = Split(pair, delimiter::Limit("=", 1));
+ if (key_value.size() == 2 && key_value[0] == "doAs") {
+ string decoded;
+ if (!UrlDecode(key_value[1], &decoded)) {
+ *err_msg = Substitute(
+ "Could not decode 'doAs' parameter from HTTP request with path: $0", path);
+ return false;
+ } else {
+ connection_context->do_as_user = decoded;
+ }
+ break;
+ }
+ }
+ }
+ return true;
+}
+
namespace {
// SASL requires mutexes for thread safety, but doesn't implement
@@ -1014,18 +1041,22 @@ void SecureAuthProvider::SetupConnectionContext(
case ThriftServer::HTTP: {
THttpServer* http_input_transport =
down_cast<THttpServer*>(underlying_input_transport);
+ THttpServer* http_output_transport =
+ down_cast<THttpServer*>(underlying_output_transport);
+ THttpServer::HttpCallbacks callbacks;
+ callbacks.path_fn = std::bind(
+ HttpPathFn, connection_ptr.get(), std::placeholders::_1, std::placeholders::_2);
+ callbacks.return_headers_fn = std::bind(ReturnHeaders, connection_ptr.get());
if (has_ldap_) {
- http_input_transport->setBasicAuthFn(
- std::bind(BasicAuth, connection_ptr.get(), std::placeholders::_1));
+ callbacks.basic_auth_fn =
+ std::bind(BasicAuth, connection_ptr.get(), std::placeholders::_1);
}
if (!principal_.empty()) {
- http_input_transport->setNegotiateAuthFn(std::bind(NegotiateAuth,
- connection_ptr.get(), std::placeholders::_1, std::placeholders::_2));
- THttpServer* http_output_transport =
- down_cast<THttpServer*>(underlying_input_transport);
- http_output_transport->setReturnHeadersFn(
- std::bind(ReturnHeaders, connection_ptr.get()));
+ callbacks.negotiate_auth_fn = std::bind(NegotiateAuth, connection_ptr.get(),
+ std::placeholders::_1, std::placeholders::_2);
}
+ http_input_transport->setCallbacks(callbacks);
+ http_output_transport->setCallbacks(callbacks);
break;
}
default:
@@ -1060,6 +1091,35 @@ Status NoAuthProvider::WrapClientTransport(const string& hostname,
return Status::OK();
}
+void NoAuthProvider::SetupConnectionContext(
+ const boost::shared_ptr<ThriftServer::ConnectionContext>& connection_ptr,
+ ThriftServer::TransportType underlying_transport_type,
+ TTransport* underlying_input_transport, TTransport* underlying_output_transport) {
+ connection_ptr->username = "";
+ switch (underlying_transport_type) {
+ case ThriftServer::BINARY:
+ // Intentionally blank - since there's no security, there's nothing to set up here.
+ break;
+ case ThriftServer::HTTP: {
+ THttpServer* http_input_transport =
+ down_cast<THttpServer*>(underlying_input_transport);
+ THttpServer* http_output_transport =
+ down_cast<THttpServer*>(underlying_input_transport);
+ THttpServer::HttpCallbacks callbacks;
+ // Even though there's no security, we set up some callbacks, eg. to allow
+ // impersonation over unsecured connections for testing purposes.
+ callbacks.path_fn = std::bind(
+ HttpPathFn, connection_ptr.get(), std::placeholders::_1, std::placeholders::_2);
+ callbacks.return_headers_fn = std::bind(ReturnHeaders, connection_ptr.get());
+ http_input_transport->setCallbacks(callbacks);
+ http_output_transport->setCallbacks(callbacks);
+ break;
+ }
+ default:
+ LOG(FATAL) << Substitute("Bad transport type: $0", underlying_transport_type);
+ }
+}
+
Status AuthManager::Init() {
ssl_socket_factory_.reset(new TSSLSocketFactory(TLSv1_0));
diff --git a/be/src/rpc/thrift-server.h b/be/src/rpc/thrift-server.h
index e4bae51..8c43a37 100644
--- a/be/src/rpc/thrift-server.h
+++ b/be/src/rpc/thrift-server.h
@@ -95,6 +95,7 @@ class ThriftServer {
struct ConnectionContext {
TUniqueId connection_id;
Username username;
+ Username do_as_user;
TNetworkAddress network_address;
std::string server_name;
/// Used to pass HTTP headers generated by the input transport to the output transport
diff --git a/be/src/service/impala-hs2-server.cc b/be/src/service/impala-hs2-server.cc
index 9717963..b50a213 100644
--- a/be/src/service/impala-hs2-server.cc
+++ b/be/src/service/impala-hs2-server.cc
@@ -309,10 +309,15 @@ void ImpalaServer::OpenSession(TOpenSessionResp& return_val,
state->kudu_latest_observed_ts = 0;
// If the username was set by a lower-level transport, use it.
- const ThriftServer::Username& username =
- ThriftServer::GetThreadConnectionContext()->username;
- if (!username.empty()) {
- state->connected_user = username;
+ const ThriftServer::ConnectionContext* connection_context =
+ ThriftServer::GetThreadConnectionContext();
+ if (!connection_context->username.empty()) {
+ state->connected_user = connection_context->username;
+ if (!connection_context->do_as_user.empty()) {
+ state->do_as_user = connection_context->do_as_user;
+ Status status = AuthorizeProxyUser(state->connected_user, state->do_as_user);
+ HS2_RETURN_IF_ERROR(return_val, status, SQLSTATE_GENERAL_ERROR);
+ }
} else {
state->connected_user = request.username;
}
diff --git a/be/src/transport/THttpServer.cpp b/be/src/transport/THttpServer.cpp
index 182ed1d..fb2ff11 100644
--- a/be/src/transport/THttpServer.cpp
+++ b/be/src/transport/THttpServer.cpp
@@ -128,6 +128,10 @@ bool THttpServer::parseStatusLine(char* status) {
*http = '\0';
if (strcmp(method, "POST") == 0) {
+ string err_msg;
+ if (!callbacks_.path_fn(string(path), &err_msg)) {
+ throw TTransportException(err_msg);
+ }
// POST method ok, looking for content.
return true;
} else if (strcmp(method, "OPTIONS") == 0) {
@@ -182,7 +186,7 @@ void THttpServer::headersDone() {
// all supported auth types.
bool authorized = false;
if (has_ldap_ && (!got_negotiate_auth || !has_kerberos_)) {
- if (basic_auth_fn_(basic_auth_token)) {
+ if (callbacks_.basic_auth_fn(basic_auth_token)) {
authorized = true;
if (metrics_enabled_) total_basic_auth_success_->Increment(1);
} else {
@@ -190,7 +194,7 @@ void THttpServer::headersDone() {
}
} else if (has_kerberos_ && (!got_basic_auth || !has_ldap_)) {
bool is_complete;
- if (negotiate_auth_fn_(negotiate_auth_token, &is_complete)) {
+ if (callbacks_.negotiate_auth_fn(negotiate_auth_token, &is_complete)) {
// If 'is_complete' is false we want to return a 401.
authorized = is_complete;
if (is_complete && metrics_enabled_) total_negotiate_auth_success_->Increment(1);
@@ -220,7 +224,7 @@ void THttpServer::flush() {
<< "Server: Thrift/" << VERSION << CRLF << "Access-Control-Allow-Origin: *" << CRLF
<< "Content-Type: application/x-thrift" << CRLF << "Content-Length: " << len << CRLF
<< "Connection: Keep-Alive" << CRLF;
- vector<string> return_headers = return_headers_fn_();
+ vector<string> return_headers = callbacks_.return_headers_fn();
for (const string& header : return_headers) {
h << header << CRLF;
}
@@ -261,7 +265,7 @@ std::string THttpServer::getTimeRFC1123() {
void THttpServer::returnUnauthorized() {
std::ostringstream h;
h << "HTTP/1.1 401 Unauthorized" << CRLF << "Date: " << getTimeRFC1123() << CRLF;
- vector<string> return_headers = return_headers_fn_();
+ vector<string> return_headers = callbacks_.return_headers_fn();
for (const string& header : return_headers) {
h << header << CRLF;
}
diff --git a/be/src/transport/THttpServer.h b/be/src/transport/THttpServer.h
index de057b5..c205b59 100644
--- a/be/src/transport/THttpServer.h
+++ b/be/src/transport/THttpServer.h
@@ -33,16 +33,28 @@ namespace transport {
class THttpServer : public THttpTransport {
public:
- // Function that takes a base64 encoded string of the form 'username:password' and
- // returns true if authentication is successful.
- typedef std::function<bool(const std::string&)> BasicAuthFn;
-
- // Function that takes the value from a 'Authorization: Negotiate' header. Returns true
- // if successful and sets 'is_complete' to true if negoation is done.
- typedef std::function<bool(const std::string&, bool* is_complete)> NegotiateAuthFn;
-
- // Function that returns a list of headers to return to the client.
- typedef std::function<std::vector<std::string>()> ReturnHeadersFn;
+ struct HttpCallbacks {
+ public:
+ // Function that takes the value from a 'Authorization: Basic' header. Returns true
+ // if authentication is successful. Must be set if 'has_ldap_' is true.
+ std::function<bool(const std::string&)> basic_auth_fn =
+ [&](const std::string&) { DCHECK(false); return false; };
+
+ // Function that takes the value from a 'Authorization: Negotiate' header. Returns
+ // true if successful and sets 'is_complete' to true if negoation is done. Must be set
+ // if 'has_kerberos_' is true.
+ std::function<bool(const std::string&, bool* is_complete)> negotiate_auth_fn =
+ [&](const std::string&, bool*) { DCHECK(false); return false; };
+
+ // Function that returns a list of headers to return to the client.
+ std::function<std::vector<std::string>()> return_headers_fn =
+ [&]() { return std::vector<std::string>(); };
+
+ // Function that takes the path component of an HTTP request. Returns false and sets
+ // 'err_msg' if an error is encountered.
+ std::function<bool(const std::string& path, std::string* err_msg)> path_fn =
+ [&](const std::string&, std::string*) { return true; };
+ };
THttpServer(boost::shared_ptr<TTransport> transport, bool has_ldap, bool has_kerberos,
bool metrics_enabled, impala::IntCounter* total_basic_auth_success,
@@ -54,9 +66,7 @@ public:
virtual void flush();
- void setBasicAuthFn(const BasicAuthFn& fn) { basic_auth_fn_ = fn; }
- void setNegotiateAuthFn(const NegotiateAuthFn& fn) { negotiate_auth_fn_ = fn; }
- void setReturnHeadersFn(const ReturnHeadersFn& fn) { return_headers_fn_ = fn; }
+ void setCallbacks(const HttpCallbacks& callbacks) { callbacks_ = callbacks; }
protected:
void readHeaders();
@@ -68,12 +78,6 @@ protected:
void returnUnauthorized();
private:
- static bool dummyBasicAuthFn(const std::string&) { return false; }
- static bool dummyNegotiateAuthFn(const std::string&, bool*) { return false; }
- static std::vector<std::string> dummyReturnHeadersFn() {
- return std::vector<std::string>();
- }
-
// If either of the following is true, a '401 - Unauthorized' will be returned to the
// client on requests that do not contain a valid 'Authorization' header. If 'has_ldap_'
// is true, 'Basic' auth headers will be processed, and if 'has_kerberos_' is true
@@ -81,12 +85,7 @@ protected:
bool has_ldap_ = false;
bool has_kerberos_ = false;
- // Called with the base64 encoded authorization from a 'Authorization: Basic' header.
- BasicAuthFn basic_auth_fn_ = &dummyBasicAuthFn;
- // Called with the value from a 'Authorization: Negotiate' header.
- NegotiateAuthFn negotiate_auth_fn_ = &dummyNegotiateAuthFn;
- // Called during flush() to get additional headers to return.
- ReturnHeadersFn return_headers_fn_ = &dummyReturnHeadersFn;
+ HttpCallbacks callbacks_;
// The value from the 'Authorization' header.
std::string auth_value_ = "";
diff --git a/fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java b/fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java
index f41f27a..be6b20d 100644
--- a/fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java
+++ b/fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java
@@ -53,14 +53,22 @@ public class LdapImpalaShellTest {
// These correspond to the values in fe/src/test/resources/users.ldif
private static final String testUser_ = "Test1Ldap";
private static final String testPassword_ = "12345";
+ private static final String testUser2_ = "Test2Ldap";
+ private static final String testPassword2_ = "abcde";
+
+ // The cluster will be set up to allow testUser_ to act as a proxy for delegateUser_.
+ // Includes a special character to test HTTP path encoding.
+ private static final String delegateUser_ = "proxyUser$";
@Before
public void setUp() throws Exception {
String uri =
String.format("ldap://localhost:%s", serverRule.getLdapServer().getPort());
String dn = "cn=#UID,ou=Users,dc=myorg,dc=com";
- String ldapArgs = String.format("--enable_ldap_auth --ldap_uri='%s' "
- + "--ldap_bind_pattern='%s' --ldap_passwords_in_clear_ok", uri, dn);
+ String ldapArgs = String.format(
+ "--enable_ldap_auth --ldap_uri='%s' --ldap_bind_pattern='%s' " +
+ "--ldap_passwords_in_clear_ok --authorized_proxy_user_config=%s=%s",
+ uri, dn, testUser_, delegateUser_);
int ret = CustomClusterRunner.StartImpalaCluster(ldapArgs);
assertEquals(ret, 0);
}
@@ -74,32 +82,34 @@ public class LdapImpalaShellTest {
* Helper to run a shell command 'cmd'. If 'shouldSucceed' is true, the command
* is expected to succeed, failure otherwise.
*/
- private void runShellCommand(String[] cmd, boolean shouldSucceed) throws Exception {
+ private void runShellCommand(String[] cmd, boolean shouldSucceed, String expectedOut,
+ String expectedErr) throws Exception {
Runtime rt = Runtime.getRuntime();
Process process = rt.exec(cmd);
// Collect the stderr.
BufferedReader input = new BufferedReader(
new InputStreamReader(process.getErrorStream()));
- StringBuffer stderr = new StringBuffer();
+ StringBuffer stderrBuf = new StringBuffer();
String line;
while ((line = input.readLine()) != null) {
- stderr.append(line);
- stderr.append('\n');
+ stderrBuf.append(line);
+ stderrBuf.append('\n');
}
+ String stderr = stderrBuf.toString();
+ assertTrue(stderr, stderr.contains(expectedErr));
// Collect the stdout (which has the resultsets).
input = new BufferedReader(new InputStreamReader(process.getInputStream()));
- StringBuffer stdout = new StringBuffer();
+ StringBuffer stdoutBuf = new StringBuffer();
while ((line = input.readLine()) != null) {
- stdout.append(line);
- stdout.append('\n');
+ stdoutBuf.append(line);
+ stdoutBuf.append('\n');
}
int expectedReturn = shouldSucceed ? 0 : 1;
assertEquals(stderr.toString(), expectedReturn, process.waitFor());
- // If the query succeeds, assert that the output contains the correct
- // username.
+ // If the query succeeds, assert that the output is correct.
if (shouldSucceed) {
- String temp = stdout.toString();
- assertTrue(temp, temp.contains(testUser_));
+ String stdout = stdoutBuf.toString();
+ assertTrue(stdout, stdout.contains(expectedOut));
}
}
@@ -127,11 +137,56 @@ public class LdapImpalaShellTest {
for (String protocol: protocolsToTest) {
protocol = String.format(protocolTemplate, protocol);
validCommand[1] = protocol;
- runShellCommand(validCommand, /*shouldSucceed*/ true);
+ runShellCommand(validCommand, /*shouldSucceed*/ true, testUser_,
+ "Starting Impala Shell using LDAP-based authentication");
invalidCommand[1] = protocol;
- runShellCommand(invalidCommand, /*shouldSucceed*/ false);
+ runShellCommand(
+ invalidCommand, /*shouldSucceed*/ false, "", "Not connected to Impala");
commandWithoutAuth[1] = protocol;
- runShellCommand(commandWithoutAuth, /*shouldSucceed*/ false);
+ runShellCommand(
+ commandWithoutAuth, /*shouldSucceed*/ false, "", "Not connected to Impala");
}
}
+
+ private String[] buildCommand(
+ String query, String protocol, String user, String password, String httpPath) {
+ String[] command = {"impala-shell.sh", "--protocol=" + protocol, "--ldap",
+ "--auth_creds_ok_in_clear", "--user=" + user,
+ "--ldap_password_cmd=printf " + password, "--query=" + query,
+ "--http_path=" + httpPath};
+ return command;
+ }
+
+ /**
+ * Tests user impersonation over the HTTP protocol by using the HTTP path to specify the
+ * 'doAs' parameter.
+ */
+ @Test
+ public void testHttpImpersonation() throws Exception {
+ String invalidDelegateUser = "invalid-delegate-user";
+ String query = "select logged_in_user()";
+ String errTemplate = "User '%s' is not authorized to delegate to '%s'";
+
+ // Run with an invalid proxy user.
+ String[] command = buildCommand(
+ query, "hs2-http", testUser2_, testPassword2_, "/?doAs=" + delegateUser_);
+ runShellCommand(command, /* shouldSucceed */ false, "",
+ String.format(errTemplate, testUser2_, delegateUser_));
+
+ // Run with a valid proxy user but invalid delegate user.
+ command = buildCommand(
+ query, "hs2-http", testUser_, testPassword_, "/?doAs=" + invalidDelegateUser);
+ runShellCommand(command, /* shouldSucceed */ false, "",
+ String.format(errTemplate, testUser_, invalidDelegateUser));
+
+ // 'doAs' parameter that cannot be decoded.
+ command = buildCommand(
+ query, "hs2-http", testUser_, testPassword_, "/?doAs=%");
+ runShellCommand(command, /* shouldSucceed */ false, "", "Not connected to Impala");
+
+ // Successfully delegate.
+ command = buildCommand(
+ query, "hs2-http", testUser_, testPassword_, "/?doAs=" + delegateUser_);
+ runShellCommand(command, /* shouldSucceed */ true, delegateUser_, "");
+ }
}