[impala] 05/05: IMPALA-9423: Add keys to cookie components

[tm...@apache.org]

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 49774fd2e14343dbb5f32c71655f91dae6e6daac
Author: Thomas Tauber-Marshall <tm...@cloudera.com>
AuthorDate: Tue Feb 25 14:06:13 2020 -0800

    IMPALA-9423: Add keys to cookie components
    
    Impala's HTTP authentication cookie format is similar, but not
    identical, to the format used by other Hadoop components.
    
    This has caused us some problems when other Hadoop systems make
    assumptions about the cookie format. In particular, KNOX-2223 made a
    change to Apache Knox's cookie handling that works for cookies
    generated by Hive and HDFS but breaks cookie handling for Impala when
    requests are proxied through Knox.
    
    This match modifies our cookie format slightly to more closely
    resemble Hive and HDFS's format by adding keys to the individual
    components of the cookie value, fixing the regression caused by
    KNOX-2223 and hopefully reducing the probability of another such
    regression being introduced in the future.
    
    Testing:
    - Ran existing cookie auth tests. Updated one test to fix an issue
      introduced by IMPALA-8899 where we were no longer testing what we
      thought we were.
    - Manualy tested in a cluster with Apache Knox.
    
    Change-Id: I73f37a4c4f28edf35f4b8195d3f03a2f178f3d0b
    Reviewed-on: http://gerrit.cloudera.org:8080/15301
    Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
 be/src/rpc/cookie-util.cc                          | 22 +++++++++++++++++-----
 .../apache/impala/customcluster/LdapHS2Test.java   |  2 +-
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/be/src/rpc/cookie-util.cc b/be/src/rpc/cookie-util.cc
index d8a0ca9..e488f30 100644
--- a/be/src/rpc/cookie-util.cc
+++ b/be/src/rpc/cookie-util.cc
@@ -36,8 +36,14 @@ using namespace strings;
 namespace impala {
 
 // Used to separate values in cookies. All generated cookies will be of the form:
-// <signature>&<username>&<timestamp>&<random number>
+// <signature>&u=<username>&t=<timestamp>&r=<random number>
+// This format was chosen to imitate the cookie format used by other Hadoop system such as
+// Hive in order to facilitate interoperability with systems like Knox. See for example:
+// service/src/java/org/apache/hive/service/auth/HttpAuthUtils.java in Hive
 static const string COOKIE_SEPARATOR = "&";
+static const string USERNAME_KEY = "u=";
+static const string TIMESTAMP_KEY = "t=";
+static const string RAND_KEY = "r=";
 
 // Cookies generated and processed by the HTTP server will be of the form:
 // COOKIE_NAME=<cookie>
@@ -98,17 +104,23 @@ Status AuthenticateCookie(
     if (cookie_value_split.size() != 3) {
       return Status("The cookie value has an invalid format.");
     }
+    string timestamp;
+    if (!TryStripPrefixString(cookie_value_split[1], TIMESTAMP_KEY, &timestamp)) {
+      return Status("The cookie timestamp value has an invalid format.");
+    }
     StringParser::ParseResult result;
     int64_t create_time = StringParser::StringToInt<int64_t>(
-        cookie_value_split[1].c_str(), cookie_value_split[1].length(), &result);
+        timestamp.c_str(), timestamp.length(), &result);
     if (result != StringParser::PARSE_SUCCESS) {
       return Status("Could not parse cookie timestamp.");
     }
     // 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) {
+      if (!TryStripPrefixString(cookie_value_split[0], USERNAME_KEY, username)) {
+        return Status("The cookie username value has an invalid format.");
+      }
       // We've successfully authenticated.
-      *username = cookie_value_split[0];
       return Status::OK();
     } else {
       return Status("Cookie is past its max lifetime.");
@@ -122,8 +134,8 @@ 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());
+  string cookie_value = StrCat(USERNAME_KEY, username, COOKIE_SEPARATOR, TIMESTAMP_KEY,
+      MonotonicMillis(), COOKIE_SEPARATOR, RAND_KEY, rand());
   uint8_t signature[AuthenticationHash::HashLen()];
   Status compute_status =
       hash.Compute(reinterpret_cast<const uint8_t*>(cookie_value.data()),
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 c362de6..1789709 100644
--- a/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
+++ b/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
@@ -227,7 +227,7 @@ public class LdapHS2Test {
         "eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHg=&impala&0&0" // incorrect signature
     };
     for (String cookieStr : badCookies) {
-      headers.put("Cookie", "impala.hs2.auth=" + cookieStr);
+      headers.put("Cookie", "impala.auth=" + cookieStr);
       transport.setCustomHeaders(headers);
       try {
         TOpenSessionReq openReq5 = new TOpenSessionReq();