[impala] branch master updated: IMPALA-8869: Fix handling of HTTP keep-alive when returning 401

[ta...@apache.org]

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 4b5ea53  IMPALA-8869: Fix handling of HTTP keep-alive when returning 401
4b5ea53 is described below

commit 4b5ea534d25b7d18d6ecb71d90c18efda99c1b29
Author: Thomas Tauber-Marshall <tm...@cloudera.com>
AuthorDate: Thu Aug 15 15:24:30 2019 -0700

    IMPALA-8869: Fix handling of HTTP keep-alive when returning 401
    
    Recent work has added support for HTTP authentication both for the
    thrift hs2 interface and for the webserver. In both cases, we
    mishandle HTTP keep-alive semantics when returning a 401 because we
    close the connection but don't return a 'Connection: close' header,
    even though we're using HTTP/1.1 where keep-alive is assumed, which
    can cause clients to incorrectly believe that the connection has
    remained open.
    
    For the webserver, the fix is to enable keep-alive in squeasel so
    that the connection isn't closed after the 401 is returned.
    
    For the thrift hs2 interface, we throw an exception after the 401
    which results in the connection being closed because otherwise it's
    tricky with the way thrift is structured to ensure that the
    unauthorized request isn't processed. So, the fix here is to return a
    'Connection: close' header.
    
    Testing:
    - Ran existing HTTP auth tests.
    - Manually tested in a cluster with connections to Impala proxied
      through Apache Knox.
    
    Change-Id: I3d5f80dbcde5b623a1d0586b5d763a062dd21afa
    Reviewed-on: http://gerrit.cloudera.org:8080/14076
    Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
    Reviewed-by: Thomas Tauber-Marshall <tm...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
 be/src/transport/THttpServer.cpp | 3 ++-
 be/src/util/webserver.cc         | 3 +++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/be/src/transport/THttpServer.cpp b/be/src/transport/THttpServer.cpp
index 8db0a1c..7e20d8c 100644
--- a/be/src/transport/THttpServer.cpp
+++ b/be/src/transport/THttpServer.cpp
@@ -269,7 +269,8 @@ std::string THttpServer::getTimeRFC1123() {
 
 void THttpServer::returnUnauthorized() {
   std::ostringstream h;
-  h << "HTTP/1.1 401 Unauthorized" << CRLF << "Date: " << getTimeRFC1123() << CRLF;
+  h << "HTTP/1.1 401 Unauthorized" << CRLF << "Date: " << getTimeRFC1123() << CRLF
+    << "Connection: close" << CRLF;
   vector<string> return_headers = callbacks_.return_headers_fn();
   for (const string& header : return_headers) {
     h << header << CRLF;
diff --git a/be/src/util/webserver.cc b/be/src/util/webserver.cc
index f222bdf..559346b 100644
--- a/be/src/util/webserver.cc
+++ b/be/src/util/webserver.cc
@@ -386,6 +386,9 @@ Status Webserver::Start() {
   options.push_back("enable_directory_listing");
   options.push_back("no");
 
+  options.push_back("enable_keep_alive");
+  options.push_back("yes");
+
   // Options must be a NULL-terminated list
   options.push_back(nullptr);