You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by ta...@apache.org on 2019/08/25 17:12:40 UTC
[impala] branch master updated: IMPALA-8584: Add cookie support to
the HTTP HS2 server
This is an automated email from the ASF dual-hosted git repository.
tarmstrong pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
The following commit(s) were added to refs/heads/master by this push:
new 5fde84a IMPALA-8584: Add cookie support to the HTTP HS2 server
5fde84a is described below
commit 5fde84a7aba9284004b481b41fe6f23d11527b6f
Author: Thomas Tauber-Marshall <tm...@cloudera.com>
AuthorDate: Tue Jun 11 15:17:25 2019 -0700
IMPALA-8584: Add cookie support to the HTTP HS2 server
This patch modifies the HTTP HS2 server to accept cookies for
authentication in order to avoid having to authenticate every request
through LDAP or Kerberos.
It adds a flag, --max_cookie_lifetime_s, that determines how long
generated cookies are valid for. Setting the flag to 0 disables cookie
support.
The cookies include a SHA256 HMAC signature that it used to verify
them. They also have a timestamp that is used to determine if they
have expired. If a cookie is successfully verified and hasn't expired,
the username contained in the cookie is set on the connection.
Each impalad uses its own key to generate the signature, so clients
that reconnect to a different impalad will have to reauthenticate.
On a single impalad cookies are valid across sessions and connections.
A new cookie is generated and sent back with the Set-Cookie header
on each request that was authenticated without using a cookie.
Cookies are of the form:
impala.hs2.auth=<cookie>;HttpOnly;MaxAge=<max_cookie_lifetime_s>
<optional ';Secure' flag>
where:
cookie = <signature>&<username>&<create timestamp>&<random number>
and 'signature' is the SHA256 HMAC of the rest of the cookie
The 'Secure' flag, which indicates to clients that the cookie should
only be sent over secure connections, is omitted if
'--ldap_passwords_in_clear_ok' is true. This is intended only for
testing.
It also adds the metrics:
impala.thrift-server.hiveserver2-http-frontend.total-cookie-auth-success
impala.thrift-server.hiveserver2-http-frontend.total-cookie-auth-failure
Testing:
- Added tests to the FE LDAP tests that use the metrics to verify
successful and failed cookie attempts.
Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
Reviewed-on: http://gerrit.cloudera.org:8080/13672
Reviewed-by: Thomas Tauber-Marshall <tm...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
be/src/rpc/CMakeLists.txt | 1 +
be/src/rpc/auth-provider.h | 4 +
be/src/rpc/authentication.cc | 26 +++-
be/src/rpc/cookie-util.cc | 162 +++++++++++++++++++++
be/src/rpc/cookie-util.h | 36 +++++
be/src/transport/THttpServer.cpp | 117 +++++++++------
be/src/transport/THttpServer.h | 68 +++++----
be/src/util/openssl-util.cc | 31 ++++
be/src/util/openssl-util.h | 24 +++
common/thrift/metrics.json | 20 +++
.../apache/impala/customcluster/LdapHS2Test.java | 65 ++++++++-
.../apache/impala/customcluster/LdapJdbcTest.java | 69 +++++++--
.../org/apache/impala/service/JdbcTestBase.java | 24 +--
13 files changed, 541 insertions(+), 106 deletions(-)
diff --git a/be/src/rpc/CMakeLists.txt b/be/src/rpc/CMakeLists.txt
index fa76059..98bb912 100644
--- a/be/src/rpc/CMakeLists.txt
+++ b/be/src/rpc/CMakeLists.txt
@@ -28,6 +28,7 @@ set_source_files_properties(${RPC_TEST_PROTO_SRCS} PROPERTIES GENERATED TRUE)
add_library(Rpc
authentication.cc
${COMMON_PROTO_SRCS}
+ cookie-util.cc
impala-service-pool.cc
rpc-mgr.cc
rpc-trace.cc
diff --git a/be/src/rpc/auth-provider.h b/be/src/rpc/auth-provider.h
index 3b5dc29..72b2763 100644
--- a/be/src/rpc/auth-provider.h
+++ b/be/src/rpc/auth-provider.h
@@ -24,6 +24,7 @@
#include "common/status.h"
#include "rpc/thrift-server.h"
+#include "util/openssl-util.h"
#include "util/promise.h"
namespace sasl { class TSasl; }
@@ -169,6 +170,9 @@ class SecureAuthProvider : public AuthProvider {
/// function as a client.
bool needs_kinit_;
+ /// Used to generate and verify signatures for cookies.
+ AuthenticationHash hash_;
+
/// One-time kerberos-specific environment variable setup. Called by InitKerberos().
Status InitKerberosEnv() WARN_UNUSED_RESULT;
};
diff --git a/be/src/rpc/authentication.cc b/be/src/rpc/authentication.cc
index 1d253d8..eace146 100644
--- a/be/src/rpc/authentication.cc
+++ b/be/src/rpc/authentication.cc
@@ -44,6 +44,7 @@
#include "kudu/security/gssapi.h"
#include "kudu/security/init.h"
#include "rpc/auth-provider.h"
+#include "rpc/cookie-util.h"
#include "rpc/thrift-server.h"
#include "transport/THttpServer.h"
#include "transport/TSaslClientTransport.h"
@@ -83,6 +84,8 @@ DECLARE_string(krb5_debug_file);
// Defined in kudu/security/init.cc
DECLARE_bool(use_system_auth_to_local);
+DECLARE_int64(max_cookie_lifetime_s);
+
DEFINE_string(sasl_path, "", "Colon separated list of paths to look for SASL "
"security library plugins.");
DEFINE_bool(enable_ldap_auth, false,
@@ -500,8 +503,8 @@ static int SaslGetPath(void* context, const char** path) {
return SASL_OK;
}
-bool BasicAuth(
- ThriftServer::ConnectionContext* connection_context, const std::string& base64) {
+bool BasicAuth(ThriftServer::ConnectionContext* connection_context,
+ const AuthenticationHash& hash, const std::string& base64) {
if (base64.empty()) {
connection_context->return_headers.push_back("WWW-Authenticate: Basic");
return false;
@@ -526,6 +529,9 @@ bool BasicAuth(
if (ret) {
// Authenication was successful, so set the username on the connection.
connection_context->username = username;
+ // Create a cookie to return.
+ connection_context->return_headers.push_back(
+ Substitute("Set-Cookie: $0", GenerateCookie(username, hash)));
return true;
}
connection_context->return_headers.push_back("WWW-Authenticate: Basic");
@@ -538,7 +544,7 @@ bool BasicAuth(
// 'is_complete' to indicate if more steps are needed. Returns false if an error was
// encountered and the connection should be closed.
bool NegotiateAuth(ThriftServer::ConnectionContext* connection_context,
- const std::string& header_token, bool* is_complete) {
+ const AuthenticationHash& hash, const std::string& header_token, bool* is_complete) {
std::string token;
// Note: according to RFC 2616, the correct format for the header is:
// 'Authorization: Negotiate <token>'. However, beeline incorrectly adds an additional
@@ -565,6 +571,9 @@ bool NegotiateAuth(ThriftServer::ConnectionContext* connection_context,
} else {
// Authentication was successful, so set the username on the connection.
connection_context->username = username;
+ // Create a cookie to return.
+ connection_context->return_headers.push_back(
+ Substitute("Set-Cookie: $0", GenerateCookie(username, hash)));
}
}
} else {
@@ -954,8 +963,9 @@ Status SecureAuthProvider::GetServerTransportFactory(
if (underlying_transport_type == ThriftServer::HTTP) {
bool has_kerberos = !principal_.empty();
- factory->reset(
- new THttpServerTransportFactory(server_name, metrics, has_ldap_, has_kerberos));
+ bool use_cookies = FLAGS_max_cookie_lifetime_s > 0;
+ factory->reset(new THttpServerTransportFactory(
+ server_name, metrics, has_ldap_, has_kerberos, use_cookies));
return Status::OK();
}
@@ -1047,13 +1057,15 @@ void SecureAuthProvider::SetupConnectionContext(
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());
+ callbacks.cookie_auth_fn = std::bind(
+ AuthenticateCookie, connection_ptr.get(), hash_, std::placeholders::_1);
if (has_ldap_) {
callbacks.basic_auth_fn =
- std::bind(BasicAuth, connection_ptr.get(), std::placeholders::_1);
+ std::bind(BasicAuth, connection_ptr.get(), hash_, std::placeholders::_1);
}
if (!principal_.empty()) {
callbacks.negotiate_auth_fn = std::bind(NegotiateAuth, connection_ptr.get(),
- std::placeholders::_1, std::placeholders::_2);
+ hash_, std::placeholders::_1, std::placeholders::_2);
}
http_input_transport->setCallbacks(callbacks);
http_output_transport->setCallbacks(callbacks);
diff --git a/be/src/rpc/cookie-util.cc b/be/src/rpc/cookie-util.cc
new file mode 100644
index 0000000..20130be
--- /dev/null
+++ b/be/src/rpc/cookie-util.cc
@@ -0,0 +1,162 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "rpc/cookie-util.h"
+
+#include <gutil/strings/escaping.h>
+#include <gutil/strings/split.h>
+#include <gutil/strings/strcat.h>
+#include <gutil/strings/strip.h>
+
+#include "util/network-util.h"
+#include "util/openssl-util.h"
+#include "util/string-parser.h"
+
+DECLARE_bool(ldap_passwords_in_clear_ok);
+DEFINE_int64(max_cookie_lifetime_s, 24 * 60 * 60,
+ "Maximum amount of time in seconds that an authentication cookie will remain valid. "
+ "Setting to 0 disables use of cookies. Defaults to 1 day.");
+
+using namespace strings;
+
+namespace impala {
+
+// Used to separate values in cookies. All generated cookies will be of the form:
+// <signature>&<username>&<timestamp>&<random number>
+static const string COOKIE_SEPARATOR = "&";
+
+// Cookies generated and processed by the HTTP server will be of the form:
+// COOKIE_NAME=<cookie>
+static const string COOKIE_NAME = "impala.hs2.auth";
+
+// The maximum lenth for the base64 encoding of a SHA256 hash.
+static const int SHA256_BASE64_LEN =
+ CalculateBase64EscapedLen(AuthenticationHash::HashLen(), /* do_padding */ true);
+
+// Since we only return cookies with a single name, well behaved clients should only ever
+// return one cookie to us. To accomodate non-malicious but poorly behaved clients, we
+// allow for checking a limited number of cookies, up to MAX_COOKIES_TO_CHECK or until we
+// find the first one with COOKIE_NAME.
+static const int MAX_COOKIES_TO_CHECK = 5;
+
+bool AuthenticateCookie(ThriftServer::ConnectionContext* connection_context,
+ const AuthenticationHash& hash, const string& cookie_header) {
+ string error_str = "";
+ // The 'Cookie' header allows sending multiple name/value pairs separated by ';'.
+ vector<string> cookies = strings::Split(cookie_header, ";");
+ if (cookies.size() > MAX_COOKIES_TO_CHECK) {
+ LOG(WARNING) << "Received cookie header with large number of cookies: "
+ << cookie_header << ". Only checking the first " << MAX_COOKIES_TO_CHECK
+ << " cookies.";
+ }
+ for (int i = 0; i < cookies.size() && i < MAX_COOKIES_TO_CHECK; ++i) {
+ string cookie_pair = cookies[i];
+ StripWhiteSpace(&cookie_pair);
+ string cookie;
+ if (!TryStripPrefixString(cookie_pair, StrCat(COOKIE_NAME, "="), &cookie)) {
+ error_str = Substitute("Did not find expected cookie name: $0", COOKIE_NAME);
+ continue;
+ }
+ // Split the cookie into the signature and the cookie value.
+ vector<string> cookie_split = Split(cookie, delimiter::Limit(COOKIE_SEPARATOR, 1));
+ if (cookie_split.size() != 2) {
+ error_str = "The cookie has an invalid format.";
+ goto error;
+ }
+ const string& base64_signature = cookie_split[0];
+ const string& cookie_value = cookie_split[1];
+
+ string signature;
+ if (!WebSafeBase64Unescape(base64_signature, &signature)) {
+ error_str = "Unable to decode base64 signature.";
+ goto error;
+ }
+ if (signature.length() != AuthenticationHash::HashLen()) {
+ error_str = "Signature is an incorrect length.";
+ goto error;
+ }
+ bool verified = hash.Verify(reinterpret_cast<const uint8_t*>(cookie_value.data()),
+ cookie_value.length(), reinterpret_cast<const uint8_t*>(signature.data()));
+ if (!verified) {
+ error_str = "The signature is incorrect.";
+ goto error;
+ }
+
+ // Split the cookie value into username, timestamp, and random number.
+ vector<string> cookie_value_split = Split(cookie_value, COOKIE_SEPARATOR);
+ if (cookie_value_split.size() != 3) {
+ error_str = "The cookie value has an invalid format.";
+ goto error;
+ }
+ StringParser::ParseResult result;
+ int64_t create_time = StringParser::StringToInt<int64_t>(
+ cookie_value_split[1].c_str(), cookie_value_split[1].length(), &result);
+ if (result != StringParser::PARSE_SUCCESS) {
+ error_str = "Could not parse cookie timestamp.";
+ goto error;
+ }
+ // Check that the timestamp contained in the cookie is recent enough for the cookie
+ // to still be valid.
+ if (MonotonicMillis() - create_time <= FLAGS_max_cookie_lifetime_s * 1000) {
+ // We've successfully authenticated.
+ connection_context->username = cookie_value_split[0];
+ return true;
+ } else {
+ error_str = "Cookie is past its max lifetime.";
+ goto error;
+ }
+ }
+
+error:
+ LOG(INFO) << "Invalid cookie provided: " << cookie_header
+ << " from: " << TNetworkAddressToString(connection_context->network_address)
+ << ": " << error_str;
+ return false;
+}
+
+string GenerateCookie(const string& username, const AuthenticationHash& hash) {
+ // Its okay to use rand() here even though its a weak RNG because being able to guess
+ // the random numbers generated won't help an attacker. The important thing is that
+ // we're using a strong RNG to create the key and a strong HMAC function.
+ string cookie_value =
+ StrCat(username, COOKIE_SEPARATOR, MonotonicMillis(), COOKIE_SEPARATOR, rand());
+ uint8_t signature[AuthenticationHash::HashLen()];
+ Status compute_status =
+ hash.Compute(reinterpret_cast<const uint8_t*>(cookie_value.data()),
+ cookie_value.length(), signature);
+ if (!compute_status.ok()) {
+ LOG(ERROR) << "Failed to compute cookie signature: " << compute_status;
+ return "";
+ }
+ DCHECK_EQ(SHA256_BASE64_LEN, 44);
+ char base64_signature[SHA256_BASE64_LEN + 1];
+ WebSafeBase64Escape(signature, AuthenticationHash::HashLen(), base64_signature,
+ SHA256_BASE64_LEN, /* do_padding */ true);
+ base64_signature[SHA256_BASE64_LEN] = '\0';
+
+ const char* secure_flag = ";Secure";
+ if (FLAGS_ldap_passwords_in_clear_ok) {
+ // If the user specified password can be sent without TLS/SSL, don't include the
+ // 'Secure' flag, which indicates the cookie should only be returned over secured
+ // connections. This is for testing only.
+ secure_flag = "";
+ }
+ return Substitute("$0=$1$2$3;HttpOnly;MaxAge=$4$5", COOKIE_NAME, base64_signature,
+ COOKIE_SEPARATOR, cookie_value, FLAGS_max_cookie_lifetime_s, secure_flag);
+}
+
+} // namespace impala
diff --git a/be/src/rpc/cookie-util.h b/be/src/rpc/cookie-util.h
new file mode 100644
index 0000000..93adb2d
--- /dev/null
+++ b/be/src/rpc/cookie-util.h
@@ -0,0 +1,36 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.xb
+
+#pragma once
+
+#include "rpc/thrift-server.h"
+
+namespace impala {
+
+class AuthenticationHash;
+
+// Takes a single 'key=value' pair from a 'Cookie' header and attempts to verify its
+// signature with 'hash'. If verification is successful and the cookie is still valid,
+// sets the corresponding username on 'connection_context' and returns true.
+bool AuthenticateCookie(ThriftServer::ConnectionContext* connection_context,
+ const AuthenticationHash& hash, const std::string& cookie_header);
+
+// Generates and returns a cookie containing the username set on 'connection_context' and
+// a signature generated with 'hash'.
+std::string GenerateCookie(const std::string& username, const AuthenticationHash& hash);
+
+} // namespace impala
diff --git a/be/src/transport/THttpServer.cpp b/be/src/transport/THttpServer.cpp
index 7e20d8c..e1de7bf 100644
--- a/be/src/transport/THttpServer.cpp
+++ b/be/src/transport/THttpServer.cpp
@@ -41,39 +41,41 @@ using namespace std;
using strings::Substitute;
THttpServerTransportFactory::THttpServerTransportFactory(const std::string server_name,
- impala::MetricGroup* metrics, bool has_ldap, bool has_kerberos)
+ impala::MetricGroup* metrics, bool has_ldap, bool has_kerberos, bool use_cookies)
: has_ldap_(has_ldap),
has_kerberos_(has_kerberos),
+ use_cookies_(use_cookies),
metrics_enabled_(metrics != nullptr) {
if (metrics_enabled_) {
if (has_ldap_) {
- total_basic_auth_success_ =
+ http_metrics_.total_basic_auth_success_ =
metrics->AddCounter(Substitute("$0.total-basic-auth-success", server_name), 0);
- total_basic_auth_failure_ =
+ http_metrics_.total_basic_auth_failure_ =
metrics->AddCounter(Substitute("$0.total-basic-auth-failure", server_name), 0);
}
if (has_kerberos_) {
- total_negotiate_auth_success_ = metrics->AddCounter(
+ http_metrics_.total_negotiate_auth_success_ = metrics->AddCounter(
Substitute("$0.total-negotiate-auth-success", server_name), 0);
- total_negotiate_auth_failure_ = metrics->AddCounter(
+ http_metrics_.total_negotiate_auth_failure_ = metrics->AddCounter(
Substitute("$0.total-negotiate-auth-failure", server_name), 0);
}
+ if (use_cookies_) {
+ http_metrics_.total_cookie_auth_success_ =
+ metrics->AddCounter(Substitute("$0.total-cookie-auth-success", server_name), 0);
+ http_metrics_.total_cookie_auth_failure_ =
+ metrics->AddCounter(Substitute("$0.total-cookie-auth-failure", server_name), 0);
+ }
}
}
THttpServer::THttpServer(boost::shared_ptr<TTransport> transport, bool has_ldap,
- bool has_kerberos, bool metrics_enabled, impala::IntCounter* total_basic_auth_success,
- impala::IntCounter* total_basic_auth_failure,
- impala::IntCounter* total_negotiate_auth_success,
- impala::IntCounter* total_negotiate_auth_failure)
+ bool has_kerberos, bool use_cookies, bool metrics_enabled, HttpMetrics* http_metrics)
: THttpTransport(transport),
has_ldap_(has_ldap),
has_kerberos_(has_kerberos),
+ use_cookies_(use_cookies),
metrics_enabled_(metrics_enabled),
- total_basic_auth_success_(total_basic_auth_success),
- total_basic_auth_failure_(total_basic_auth_failure),
- total_negotiate_auth_success_(total_negotiate_auth_success),
- total_negotiate_auth_failure_(total_negotiate_auth_failure) {}
+ http_metrics_(http_metrics) {}
THttpServer::~THttpServer() {
}
@@ -106,6 +108,8 @@ void THttpServer::parseHeader(char* header) {
} else if ((has_ldap_ || has_kerberos_)
&& THRIFT_strncasecmp(header, "Authorization", sz) == 0) {
auth_value_ = string(value);
+ } else if (use_cookies_ && THRIFT_strncasecmp(header, "Cookie", sz) == 0) {
+ cookie_value_ = string(value);
}
}
@@ -168,49 +172,68 @@ void THttpServer::headersDone() {
return;
}
- // Determine what type of auth header we got.
- StripWhiteSpace(&auth_value_);
- string stripped_basic_auth_token;
- bool got_basic_auth =
- TryStripPrefixString(auth_value_, "Basic ", &stripped_basic_auth_token);
- string basic_auth_token = got_basic_auth ? move(stripped_basic_auth_token) : "";
- string stripped_negotiate_auth_token;
- bool got_negotiate_auth =
- TryStripPrefixString(auth_value_, "Negotiate ", &stripped_negotiate_auth_token);
- string negotiate_auth_token =
- got_negotiate_auth ? move(stripped_negotiate_auth_token) : "";
- // We can only have gotten one type of auth header.
- DCHECK(!got_basic_auth || !got_negotiate_auth);
-
- // For each auth type we support, we call the auth callback if the didn't get a header
- // of the other auth type or if the other auth type isn't supported. This way, if a
- // client select a supported auth method, they'll only get return headers for that
- // method, but if they didn't specify a valid auth method or they didn't provide a
- // 'Authorization' header at all, they'll get back 'WWW-Authenticate' return headers for
- // all supported auth types.
bool authorized = false;
- if (has_ldap_ && (!got_negotiate_auth || !has_kerberos_)) {
- if (callbacks_.basic_auth_fn(basic_auth_token)) {
+ // Try authenticating with cookies first.
+ if (use_cookies_ && !cookie_value_.empty()) {
+ StripWhiteSpace(&cookie_value_);
+ if (callbacks_.cookie_auth_fn(cookie_value_)) {
authorized = true;
- if (metrics_enabled_) total_basic_auth_success_->Increment(1);
- } else {
- if (got_basic_auth && metrics_enabled_) total_basic_auth_failure_->Increment(1);
+ if (metrics_enabled_) http_metrics_->total_cookie_auth_success_->Increment(1);
+ } else if (metrics_enabled_) {
+ http_metrics_->total_cookie_auth_failure_->Increment(1);
}
}
- if (has_kerberos_ && (!got_basic_auth || !has_ldap_)) {
- bool 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);
- } else {
- if (got_negotiate_auth && metrics_enabled_) {
- total_negotiate_auth_failure_->Increment(1);
+
+ // If cookie auth wasn't successful, try to auth with the 'Authorization' header.
+ if (!authorized) {
+ // Determine what type of auth header we got.
+ StripWhiteSpace(&auth_value_);
+ string stripped_basic_auth_token;
+ bool got_basic_auth =
+ TryStripPrefixString(auth_value_, "Basic ", &stripped_basic_auth_token);
+ string basic_auth_token = got_basic_auth ? move(stripped_basic_auth_token) : "";
+ string stripped_negotiate_auth_token;
+ bool got_negotiate_auth =
+ TryStripPrefixString(auth_value_, "Negotiate ", &stripped_negotiate_auth_token);
+ string negotiate_auth_token =
+ got_negotiate_auth ? move(stripped_negotiate_auth_token) : "";
+ // We can only have gotten one type of auth header.
+ DCHECK(!got_basic_auth || !got_negotiate_auth);
+
+ // For each auth type we support, we call the auth callback if the didn't get a header
+ // of the other auth type or if the other auth type isn't supported. This way, if a
+ // client select a supported auth method, they'll only get return headers for that
+ // method, but if they didn't specify a valid auth method or they didn't provide a
+ // 'Authorization' header at all, they'll get back 'WWW-Authenticate' return headers
+ // for all supported auth types.
+ if (has_ldap_ && (!got_negotiate_auth || !has_kerberos_)) {
+ if (callbacks_.basic_auth_fn(basic_auth_token)) {
+ authorized = true;
+ if (metrics_enabled_) http_metrics_->total_basic_auth_success_->Increment(1);
+ } else {
+ if (got_basic_auth && metrics_enabled_) {
+ http_metrics_->total_basic_auth_failure_->Increment(1);
+ }
+ }
+ }
+ if (has_kerberos_ && (!got_basic_auth || !has_ldap_)) {
+ bool 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_) {
+ http_metrics_->total_negotiate_auth_success_->Increment(1);
+ }
+ } else {
+ if (got_negotiate_auth && metrics_enabled_) {
+ http_metrics_->total_negotiate_auth_failure_->Increment(1);
+ }
}
}
}
auth_value_ = "";
+ cookie_value_ = "";
if (!authorized) {
returnUnauthorized();
throw TTransportException("HTTP auth failed.");
diff --git a/be/src/transport/THttpServer.h b/be/src/transport/THttpServer.h
index c205b59..11dc8ef 100644
--- a/be/src/transport/THttpServer.h
+++ b/be/src/transport/THttpServer.h
@@ -27,8 +27,26 @@ namespace apache {
namespace thrift {
namespace transport {
+struct HttpMetrics {
+ // If 'has_ldap_' is true, metrics for the number of successful and failed Basic
+ // auth attempts.
+ impala::IntCounter* total_basic_auth_success_ = nullptr;
+ impala::IntCounter* total_basic_auth_failure_ = nullptr;
+
+ // If 'has_kerberos_' is true, metrics for the number of successful and failed Negotiate
+ // auth attempts.
+ impala::IntCounter* total_negotiate_auth_success_ = nullptr;
+ impala::IntCounter* total_negotiate_auth_failure_ = nullptr;
+
+ // If 'use_cookies_' is true, metrics for the number of successful and failed cookie
+ // auth attempts.
+ impala::IntCounter* total_cookie_auth_success_ = nullptr;
+ impala::IntCounter* total_cookie_auth_failure_ = nullptr;
+};
+
/*
- * Implements server side work for http connections, including support for BASIC auth.
+ * Implements server side work for http connections, including support for Basic auth,
+ * SPNEGO, and cookies.
*/
class THttpServer : public THttpTransport {
public:
@@ -54,13 +72,15 @@ public:
// '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; };
+
+ // Function that takes the value from the 'Cookie' header and returns true if
+ // authentication is successful.
+ std::function<bool(const std::string&)> cookie_auth_fn =
+ [&](const std::string&) { return false; };
};
THttpServer(boost::shared_ptr<TTransport> transport, bool has_ldap, bool has_kerberos,
- bool metrics_enabled, impala::IntCounter* total_basic_auth_success,
- impala::IntCounter* total_basic_auth_failure,
- impala::IntCounter* total_negotiate_auth_success,
- impala::IntCounter* total_negotiate_auth_failure);
+ bool use_cookies, bool metrics_enabled, HttpMetrics* http_metrics);
virtual ~THttpServer();
@@ -90,12 +110,15 @@ protected:
// The value from the 'Authorization' header.
std::string auth_value_ = "";
- // Metrics
- bool metrics_enabled_;
- impala::IntCounter* total_basic_auth_success_ = nullptr;
- impala::IntCounter* total_basic_auth_failure_ = nullptr;
- impala::IntCounter* total_negotiate_auth_success_ = nullptr;
- impala::IntCounter* total_negotiate_auth_failure_ = nullptr;
+ // If true, the value of any 'Cookie' header will be passed to 'cookie_auth_fn' to
+ // attempt to authenticate before calling other auth functions.
+ bool use_cookies_ = false;
+
+ // The value from the 'Cookie' header.
+ std::string cookie_value_ = "";
+
+ bool metrics_enabled_ = false;
+ HttpMetrics* http_metrics_ = nullptr;
};
/**
@@ -106,34 +129,23 @@ public:
THttpServerTransportFactory() {}
THttpServerTransportFactory(const std::string server_name, impala::MetricGroup* metrics,
- bool has_ldap, bool has_kerberos);
+ bool has_ldap, bool has_kerberos, bool use_cookies);
virtual ~THttpServerTransportFactory() {}
- /**
- * Wraps the transport into a buffered one.
- */
virtual boost::shared_ptr<TTransport> getTransport(boost::shared_ptr<TTransport> trans) {
- return boost::shared_ptr<TTransport>(new THttpServer(trans, has_ldap_, has_kerberos_,
- metrics_enabled_, total_basic_auth_success_, total_basic_auth_failure_,
- total_negotiate_auth_success_, total_negotiate_auth_failure_));
+ return boost::shared_ptr<TTransport>(new THttpServer(
+ trans, has_ldap_, has_kerberos_, use_cookies_, metrics_enabled_, &http_metrics_));
}
private:
bool has_ldap_ = false;
bool has_kerberos_ = false;
+ bool use_cookies_ = false;
+ // Metrics for every transport produced by this factory.
bool metrics_enabled_ = false;
-
- // If 'has_ldap_' is true, metrics for the number of successful and failed Basic
- // auth ettempts for every transport produced by this factory.
- impala::IntCounter* total_basic_auth_success_ = nullptr;
- impala::IntCounter* total_basic_auth_failure_ = nullptr;
-
- // If 'has_kerberos_' is true, metrics for the number of successful and failed Negotiate
- // auth ettempts for every transport produced by this factory.
- impala::IntCounter* total_negotiate_auth_success_ = nullptr;
- impala::IntCounter* total_negotiate_auth_failure_ = nullptr;
+ HttpMetrics http_metrics_;
};
}
}
diff --git a/be/src/util/openssl-util.cc b/be/src/util/openssl-util.cc
index da583cf..d661caf 100644
--- a/be/src/util/openssl-util.cc
+++ b/be/src/util/openssl-util.cc
@@ -156,6 +156,37 @@ bool IntegrityHash::Verify(const uint8_t* data, int64_t len) const {
return memcmp(hash_, test_hash.hash_, sizeof(hash_)) == 0;
}
+AuthenticationHash::AuthenticationHash() {
+ uint64_t next_key_num = keys_generated.Add(1);
+ if (next_key_num % RNG_RESEED_INTERVAL == 0) {
+ SeedOpenSSLRNG();
+ }
+ RAND_bytes(key_, sizeof(key_));
+}
+
+Status AuthenticationHash::Compute(const uint8_t* data, int64_t len, uint8_t* out) const {
+ uint32_t out_len;
+ uint8_t* result =
+ HMAC(EVP_sha256(), key_, SHA256_DIGEST_LENGTH, data, len, out, &out_len);
+ if (result == nullptr) {
+ return OpenSSLErr("HMAC", "computing");
+ }
+ DCHECK_EQ(out_len, HashLen());
+ DCHECK_EQ(ERR_peek_error(), 0) << "Did not clear OpenSSL error queue";
+ return Status::OK();
+}
+
+bool AuthenticationHash::Verify(
+ const uint8_t* data, int64_t len, const uint8_t* signature) const {
+ uint8_t out[HashLen()];
+ Status compute_status = Compute(data, len, out);
+ if (!compute_status.ok()) {
+ LOG(ERROR) << "Failed to compute hash for verification: " << compute_status;
+ return false;
+ }
+ return memcmp(signature, out, HashLen()) == 0;
+}
+
void EncryptionKey::InitializeRandom() {
uint64_t next_key_num = keys_generated.Add(1);
if (next_key_num % RNG_RESEED_INTERVAL == 0) {
diff --git a/be/src/util/openssl-util.h b/be/src/util/openssl-util.h
index d9f0da2..c7a30e4 100644
--- a/be/src/util/openssl-util.h
+++ b/be/src/util/openssl-util.h
@@ -80,6 +80,30 @@ class IntegrityHash {
uint8_t hash_[SHA256_DIGEST_LENGTH];
};
+/// Stores a random key that it can use to calculate and verify HMACs of data buffers for
+/// authentication, eg. checking signatures of cookies. A SHA256 hash is used internally.
+class AuthenticationHash {
+ public:
+ AuthenticationHash();
+
+ /// Computes the HMAC of 'data', which has length 'len', and stores it in 'out', which
+ /// must already be allocated with a length of HashLen() bytes. Returns an error if
+ /// computing the hash was unsuccessful.
+ Status Compute(const uint8_t* data, int64_t len, uint8_t* out) const WARN_UNUSED_RESULT;
+
+ /// Computes the HMAC of 'data', which has length 'len', and returns true if it matches
+ /// 'signature', which is expected to have length HashLen().
+ bool Verify(const uint8_t* data, int64_t len,
+ const uint8_t* signature) const WARN_UNUSED_RESULT;
+
+ /// Returns the length in bytes of the generated hashes. Currently we always use SHA256.
+ static int HashLen() { return SHA256_DIGEST_LENGTH; }
+
+ private:
+ /// An AES 256-bit key.
+ uint8_t key_[SHA256_DIGEST_LENGTH];
+};
+
/// The key and initialization vector (IV) required to encrypt and decrypt a buffer of
/// data. This should be regenerated for each buffer of data.
///
diff --git a/common/thrift/metrics.json b/common/thrift/metrics.json
index 1ce9264..ed01f67 100644
--- a/common/thrift/metrics.json
+++ b/common/thrift/metrics.json
@@ -1141,6 +1141,26 @@
"kind": "COUNTER",
"key": "impala.thrift-server.hiveserver2-http-frontend.total-basic-auth-failure"
},
+ {
+ "description": "The number of HiveServer2 HTTP API connection requests to this Impala Daemon that were successfully authenticated using a cookie.",
+ "contexts": [
+ "IMPALAD"
+ ],
+ "label": "HiveServer2 HTTP API Connection Cookie Auth Success",
+ "units": "NONE",
+ "kind": "COUNTER",
+ "key": "impala.thrift-server.hiveserver2-http-frontend.total-cookie-auth-success"
+ },
+ {
+ "description": "The number of HiveServer2 HTTP API connection requests to this Impala Daemon that provided an invalid cookie.",
+ "contexts": [
+ "IMPALAD"
+ ],
+ "label": "HiveServer2 HTTP API Connection Cookie Auth Failure",
+ "units": "NONE",
+ "kind": "COUNTER",
+ "key": "impala.thrift-server.hiveserver2-http-frontend.total-cookie-auth-failure"
+ },
{
"description": "The number of HiveServer2 HTTP API connection requests to this Impala Daemon that were successfully authenticated with Kerberos",
"contexts": [
diff --git a/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java b/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
index acf2835..f2d1ea0 100644
--- a/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
+++ b/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
@@ -105,6 +105,16 @@ public class LdapHS2Test {
assertEquals(expectedBasicAuthFailure, actualBasicAuthFailure);
}
+ private void verifyCookieMetrics(
+ long expectedCookieAuthSuccess, long expectedCookieAuthFailure) throws Exception {
+ long actualCookieAuthSuccess = (long) metrics.getMetric(
+ "impala.thrift-server.hiveserver2-http-frontend.total-cookie-auth-success");
+ assertEquals(expectedCookieAuthSuccess, actualCookieAuthSuccess);
+ long actualCookieAuthFailure = (long) metrics.getMetric(
+ "impala.thrift-server.hiveserver2-http-frontend.total-cookie-auth-failure");
+ assertEquals(expectedCookieAuthFailure, actualCookieAuthFailure);
+ }
+
/**
* Tests LDAP authentication to the HTTP hiveserver2 endpoint.
*/
@@ -166,18 +176,67 @@ public class LdapHS2Test {
// - invalid base64 encoded value
// - Invalid mechanism
int numFailures = 0;
- for (String authStr : new String[] {"Basic VGVzdDJMZGFwOjEyMzQ1",
- "Basic invalid-base64", "Negotiate VGVzdDFMZGFwOjEyMzQ1"}) {
+ for (String authStr :
+ new String[] {"Basic VGVzdDJMZGFwOjEyMzQ1", "Basic invalid-base64"}) {
// Attempt to authenticate with an invalid password.
headers.put("Authorization", authStr);
transport.setCustomHeaders(headers);
try {
TOpenSessionReq openReq3 = new TOpenSessionReq();
TOpenSessionResp openResp3 = client.OpenSession(openReq);
+ fail("Exception exception.");
+ } catch (Exception e) {
++numFailures;
verifyMetrics(8, numFailures);
- fail("Exception exception.");
+ assertEquals(e.getMessage(), "HTTP Response code: 401");
+ }
+ }
+
+ // Attempt to authenticate with a different mechanism. SHould fail, but won't
+ // increment the total-basic-auth-failure metric because its not considered a 'Basic'
+ // auth attempt.
+ headers.put("Authorization", "Negotiate VGVzdDFMZGFwOjEyMzQ1");
+ transport.setCustomHeaders(headers);
+ try {
+ TOpenSessionReq openReq3 = new TOpenSessionReq();
+ TOpenSessionResp openResp3 = client.OpenSession(openReq);
+ fail("Exception exception.");
+ } catch (Exception e) {
+ verifyMetrics(8, numFailures);
+ assertEquals(e.getMessage(), "HTTP Response code: 401");
+ }
+
+ // Attempt to authenticate with a bad cookie and valid user/password, should succeed.
+ headers.put("Authorization", "Basic VGVzdDJMZGFwOmFiY2Rl");
+ headers.put("Cookie", "invalid-cookie");
+ transport.setCustomHeaders(headers);
+ TOpenSessionReq openReq4 = new TOpenSessionReq();
+ TOpenSessionResp openResp4 = client.OpenSession(openReq);
+ // We should see one more successful connection and one failed cookie attempt.
+ verifyMetrics(9, numFailures);
+ int numCookieFailures = 1;
+ verifyCookieMetrics(0, numCookieFailures);
+
+ // Attempt to authenticate with no username/password and some bad cookies.
+ headers.remove("Authorization");
+ String[] badCookies = new String[] {
+ "invalid-format", // invalid cookie format
+ "x&impala&0&0", // signature value that is invalid base64
+ "eA==&impala&0&0", // signature decodes to an incorrect length
+ "eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHg=&impala&0&0" // incorrect signature
+ };
+ for (String cookieStr : badCookies) {
+ headers.put("Cookie", "impala.hs2.auth=" + cookieStr);
+ transport.setCustomHeaders(headers);
+ try {
+ TOpenSessionReq openReq5 = new TOpenSessionReq();
+ TOpenSessionResp openResp5 = client.OpenSession(openReq);
+ fail("Exception exception from cookie: " + cookieStr);
} catch (Exception e) {
+ // We should see both another failed cookie attempt.
+ ++numCookieFailures;
+ verifyMetrics(9, numFailures);
+ verifyCookieMetrics(0, numCookieFailures);
assertEquals(e.getMessage(), "HTTP Response code: 401");
}
}
diff --git a/fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java b/fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java
index c841b0b..ddc8b00 100644
--- a/fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java
+++ b/fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java
@@ -26,6 +26,7 @@ import java.sql.Connection;
import java.sql.ResultSet;
import java.sql.SQLException;
+import com.google.common.collect.Range;
import org.apache.directory.server.core.annotations.CreateDS;
import org.apache.directory.server.core.annotations.CreatePartition;
import org.apache.directory.server.annotations.CreateLdapServer;
@@ -33,7 +34,8 @@ import org.apache.directory.server.annotations.CreateTransport;
import org.apache.directory.server.core.annotations.ApplyLdifFiles;
import org.apache.directory.server.core.integ.CreateLdapServerRule;
import org.apache.impala.testutil.ImpalaJdbcClient;
-import org.junit.After;
+import org.apache.impala.util.Metrics;
+import org.junit.AfterClass;
import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Test;
@@ -56,31 +58,60 @@ public class LdapJdbcTest extends JdbcTestBase {
private static final String testUser_ = "Test1Ldap";
private static final String testPassword_ = "12345";
+ private static final Range<Long> zero = Range.closed(0L, 0L);
+ private static final Range<Long> one = Range.closed(1L, 1L);
+
+ Metrics metrics = new Metrics();
+
public LdapJdbcTest(String connectionType) { super(connectionType); }
- @Before
- public void setUp() throws Exception {
+ public void setUp(String extraArgs) 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);
- int ret = CustomClusterRunner.StartImpalaCluster(ldapArgs);
+ String impalaArgs = String.format("--enable_ldap_auth --ldap_uri='%s' "
+ + "--ldap_bind_pattern='%s' --ldap_passwords_in_clear_ok %s", uri, dn, extraArgs);
+ int ret = CustomClusterRunner.StartImpalaCluster(impalaArgs);
assertEquals(ret, 0);
con_ = createConnection(
ImpalaJdbcClient.getLdapConnectionStr(connectionType_, testUser_, testPassword_));
+ if (connectionType_.equals("http")) {
+ // There should have been one successful connection auth to create the session.
+ verifyMetrics(one, zero, zero, zero);
+ }
}
- @After
- public void cleanUp() throws Exception {
- super.cleanUp();
- CustomClusterRunner.StartImpalaCluster();
+ private void verifyMetrics(Range<Long> expectedBasicSuccess,
+ Range<Long> expectedBasicFailure, Range<Long> expectedCookieSuccess,
+ Range<Long> expectedCookieFailure) throws Exception {
+ long actualBasicSuccess = (long) metrics.getMetric(
+ "impala.thrift-server.hiveserver2-http-frontend.total-basic-auth-success");
+ assertTrue("Expected: " + expectedBasicSuccess + ", Actual: " + actualBasicSuccess,
+ expectedBasicSuccess.contains(actualBasicSuccess));
+ long actualBasicFailure = (long) metrics.getMetric(
+ "impala.thrift-server.hiveserver2-http-frontend.total-basic-auth-failure");
+ assertTrue("Expected: " + expectedBasicFailure + ", Actual: " + actualBasicFailure,
+ expectedBasicFailure.contains(actualBasicFailure));
+
+ long actualCookieSuccess = (long) metrics.getMetric(
+ "impala.thrift-server.hiveserver2-http-frontend.total-cookie-auth-success");
+ assertTrue("Expected: " + expectedCookieSuccess + ", Actual: " + actualCookieSuccess,
+ expectedCookieSuccess.contains(actualCookieSuccess));
+ long actualCookieFailure = (long) metrics.getMetric(
+ "impala.thrift-server.hiveserver2-http-frontend.total-cookie-auth-failure");
+ assertTrue("Expected: " + expectedCookieFailure + ", Actual: " + actualCookieFailure,
+ expectedCookieFailure.contains(actualCookieFailure));
}
@Test
public void testLoggedInUser() throws Exception {
+ setUp("");
ResultSet rs = con_.createStatement().executeQuery("select logged_in_user() user");
+ if (connectionType_.equals("http")) {
+ // After the initial auth, the driver should use cookies for all other requests.
+ verifyMetrics(one, zero, Range.atLeast(1L), zero);
+ }
assertTrue(rs.next());
assertEquals(rs.getString("user"), testUser_);
assertFalse(rs.next());
@@ -88,6 +119,7 @@ public class LdapJdbcTest extends JdbcTestBase {
@Test
public void testFailedConnection() throws Exception {
+ setUp("");
try {
Connection con = createConnection(ImpalaJdbcClient.getLdapConnectionStr(
connectionType_, testUser_, "invalid-password"));
@@ -104,4 +136,21 @@ public class LdapJdbcTest extends JdbcTestBase {
assertTrue(e.getMessage().contains("Could not open client transport"));
}
}
+
+ @Test
+ public void testExpireCookies() throws Exception {
+ if (connectionType_.equals("http")) {
+ setUp("--max_cookie_lifetime_s=1");
+ // Sleep long enough for the cookie returned in the initial connection to expire.
+ Thread.sleep(2000);
+ ResultSet rs = con_.createStatement().executeQuery("select logged_in_user() user");
+ // The driver should have supplied an incorrect cookie at least once, requiring at
+ // least one more auth to LDAP. There may also have been some successful cookie
+ // attempts, depending on timing.
+ verifyMetrics(Range.atLeast(2L), zero, Range.atLeast(0L), Range.atLeast(1L));
+ assertTrue(rs.next());
+ assertEquals(rs.getString("user"), testUser_);
+ assertFalse(rs.next());
+ }
+ }
}
diff --git a/fe/src/test/java/org/apache/impala/service/JdbcTestBase.java b/fe/src/test/java/org/apache/impala/service/JdbcTestBase.java
index 91e529f..f6619cb 100644
--- a/fe/src/test/java/org/apache/impala/service/JdbcTestBase.java
+++ b/fe/src/test/java/org/apache/impala/service/JdbcTestBase.java
@@ -65,18 +65,20 @@ public abstract class JdbcTestBase {
dropTestTable(tableName);
}
- con_.close();
- assertTrue("Connection should be closed", con_.isClosed());
-
- Exception expectedException = null;
- try {
- con_.createStatement();
- } catch (Exception e) {
- expectedException = e;
+ if (con_ != null) {
+ con_.close();
+ assertTrue("Connection should be closed", con_.isClosed());
+
+ Exception expectedException = null;
+ try {
+ con_.createStatement();
+ } catch (Exception e) {
+ expectedException = e;
+ }
+
+ assertNotNull("createStatement() on closed connection should throw exception",
+ expectedException);
}
-
- assertNotNull("createStatement() on closed connection should throw exception",
- expectedException);
}
protected static Connection createConnection(String connStr) throws Exception {