You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Thomas Marshall (Code Review)" <ge...@cloudera.org> on 2019/06/18 18:38:55 UTC
[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Thomas Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13672
Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server
......................................................................
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.
Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
---
M be/src/rpc/CMakeLists.txt
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
A be/src/rpc/cookie-mgr.cc
A be/src/rpc/cookie-mgr.h
M be/src/rpc/thrift-server.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M common/thrift/metrics.json
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
M fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java
M fe/src/test/java/org/apache/impala/service/JdbcTestBase.java
15 files changed, 486 insertions(+), 63 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/72/13672/1
--
To view, visit http://gerrit.cloudera.org:8080/13672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
Gerrit-Change-Number: 13672
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Todd Lipcon, Tim Armstrong, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13672
to look at the new patch set (#8).
Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server
......................................................................
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
---
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
A be/src/rpc/cookie-util.cc
A be/src/rpc/cookie-util.h
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M be/src/util/openssl-util.cc
M be/src/util/openssl-util.h
M common/thrift/metrics.json
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
M fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java
M fe/src/test/java/org/apache/impala/service/JdbcTestBase.java
13 files changed, 541 insertions(+), 106 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/72/13672/8
--
To view, visit http://gerrit.cloudera.org:8080/13672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
Gerrit-Change-Number: 13672
Gerrit-PatchSet: 8
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13672 )
Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server
......................................................................
Patch Set 6:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.h
File be/src/rpc/cookie-util.h:
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.h@29
PS5, Line 29: bool AuthenticateCookie(ThriftServer::ConnectionContext* connection_context,
> I think hash could be passed in as a const AuthenticationHash& in both thes
Done
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.h@34
PS5, Line 34: std::s
> std::string
Done
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc
File be/src/rpc/cookie-util.cc:
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@38
PS5, Line 38: in cooki
> timestamp
Done
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@49
PS5, Line 49:
> It'd be more consistent to invert this condition and do a goto.
Done
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@71
PS5, Line 71:
> signature
Done
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@81
PS5, Line 81: goto error;
> I'd probably prefer using StringParser::StringToInt() for the better error
Done
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@96
PS5, Line 96: }
> Maybe should be WARNING or INFO since it doesn't indicate the service itsel
Done
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@104
PS5, Line 104:
> We generally don't like rand() because it's not a particularly good quality
Yeah, I guess my thinking was that it doesn't really matter what RNG we use here, since being able to guess what number might come next doesn't get you anything and the generated numbers are returned in plain text anyways.
The important thing is that we're using a strong RNG to generate the key used for calculating the signature, which we are.
Of course, happy to change this if you have a suggestion of a better one to use.
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@104
PS5, Line 104: lifetime.
> Should this be MonotonicMillis() instead? Since we only use this for compar
Done
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@105
PS5, Line 105:
> Can we use a constant here instead of the magic number?
Done
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@112
PS5, Line 112: << ": " << error_str;
> Is this array size bounded? I think it would be safer to statically initial
Yes, the size is bounded, since hash_len is a constant. I rearranged things a bit to make this more obvious.
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@117
PS5, Line 117: string cookie_value =
> We're just pointing to constants, so could use a "const char*" and avoid al
Done
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/transport/THttpServer.h
File be/src/transport/THttpServer.h:
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/transport/THttpServer.h@32
PS5, Line 32: attempts
> "attempts" here and below
Done
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/util/openssl-util.h
File be/src/util/openssl-util.h:
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/util/openssl-util.h@99
PS5, Line 99: s
> Use a named constant?
Done
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/util/openssl-util.cc
File be/src/util/openssl-util.cc:
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/util/openssl-util.cc@167
PS5, Line 167: Status AuthenticationHash::Compute(const uint8_t* data, int64_t len, uint8_t* out) const {
> Looks like this methods could be const
Done
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/util/openssl-util.cc@169
PS5, Line 169: uint8_t* result =
> I think we might need to clean for and clean errors after the HMAC call. Ha
Done
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/util/openssl-util.cc@174
PS5, Line 174: DCHECK_EQ(out_len, HashLen());
> Looks like this methods could be const
Done
--
To view, visit http://gerrit.cloudera.org:8080/13672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
Gerrit-Change-Number: 13672
Gerrit-PatchSet: 6
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 22 Aug 2019 20:40:07 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Todd Lipcon, Tim Armstrong, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13672
to look at the new patch set (#6).
Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server
......................................................................
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
---
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
A be/src/rpc/cookie-util.cc
A be/src/rpc/cookie-util.h
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M be/src/util/openssl-util.cc
M be/src/util/openssl-util.h
M common/thrift/metrics.json
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
M fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java
M fe/src/test/java/org/apache/impala/service/JdbcTestBase.java
13 files changed, 528 insertions(+), 103 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/72/13672/6
--
To view, visit http://gerrit.cloudera.org:8080/13672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
Gerrit-Change-Number: 13672
Gerrit-PatchSet: 6
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13672 )
Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4226/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/13672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
Gerrit-Change-Number: 13672
Gerrit-PatchSet: 5
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 13 Aug 2019 00:43:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13672
to look at the new patch set (#2).
Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server
......................................................................
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.
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.
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.
Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
---
M be/src/rpc/CMakeLists.txt
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
A be/src/rpc/cookie-mgr.cc
A be/src/rpc/cookie-mgr.h
M be/src/rpc/thrift-server.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M common/thrift/metrics.json
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
M fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java
M fe/src/test/java/org/apache/impala/service/JdbcTestBase.java
15 files changed, 486 insertions(+), 63 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/72/13672/2
--
To view, visit http://gerrit.cloudera.org:8080/13672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
Gerrit-Change-Number: 13672
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13672 )
Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server
......................................................................
Patch Set 8: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/13672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
Gerrit-Change-Number: 13672
Gerrit-PatchSet: 8
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sun, 25 Aug 2019 05:43:55 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13672 )
Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server
......................................................................
Patch Set 1:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/3659/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/13672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
Gerrit-Change-Number: 13672
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 18 Jun 2019 19:20:08 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13672 )
Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server
......................................................................
Patch Set 7:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4351/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/13672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
Gerrit-Change-Number: 13672
Gerrit-PatchSet: 7
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 23 Aug 2019 23:30:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13672 )
Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server
......................................................................
Patch Set 6:
(2 comments)
LGTM. I had one final question about whether we need to be defensive about cookie verification to prevent it from using a lot of CPU in pathological cases.
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc
File be/src/rpc/cookie-util.cc:
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@104
PS5, Line 104:
> Yeah, I guess my thinking was that it doesn't really matter what RNG we use
I don't really have a better suggestion, it would probably be overcomplicating it to use something else. Maybe worth leaving a comment here that explains what you just said in your comment.
http://gerrit.cloudera.org:8080/#/c/13672/6/be/src/transport/THttpServer.cpp
File be/src/transport/THttpServer.cpp:
http://gerrit.cloudera.org:8080/#/c/13672/6/be/src/transport/THttpServer.cpp@181
PS6, Line 181: vector<string> cookies = strings::Split(cookie_value_, ";");
I thought about whether it would be a good idea to limit the number of cookies we check (or the aggregate size of the cookies) to prevent potentially pathological behaviour. It could potentially be quite expensive to check a large number of cookies.
--
To view, visit http://gerrit.cloudera.org:8080/13672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
Gerrit-Change-Number: 13672
Gerrit-PatchSet: 6
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 22 Aug 2019 21:13:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13672 )
Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4337/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/13672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
Gerrit-Change-Number: 13672
Gerrit-PatchSet: 6
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 22 Aug 2019 21:21:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Todd Lipcon, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13672
to look at the new patch set (#5).
Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server
......................................................................
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
---
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
A be/src/rpc/cookie-util.cc
A be/src/rpc/cookie-util.h
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M be/src/util/openssl-util.cc
M be/src/util/openssl-util.h
M common/thrift/metrics.json
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
M fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java
M fe/src/test/java/org/apache/impala/service/JdbcTestBase.java
13 files changed, 502 insertions(+), 103 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/72/13672/5
--
To view, visit http://gerrit.cloudera.org:8080/13672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
Gerrit-Change-Number: 13672
Gerrit-PatchSet: 5
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Todd Lipcon, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13672
to look at the new patch set (#4).
Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server
......................................................................
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.
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 not authenticated with 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
---
M be/src/rpc/CMakeLists.txt
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
A be/src/rpc/cookie-util.cc
A be/src/rpc/cookie-util.h
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M be/src/util/openssl-util.cc
M be/src/util/openssl-util.h
M common/thrift/metrics.json
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
M fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java
M fe/src/test/java/org/apache/impala/service/JdbcTestBase.java
19 files changed, 517 insertions(+), 72 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/72/13672/4
--
To view, visit http://gerrit.cloudera.org:8080/13672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
Gerrit-Change-Number: 13672
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13672 )
Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/3843/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/13672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
Gerrit-Change-Number: 13672
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 09 Jul 2019 04:34:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Todd Lipcon, Tim Armstrong, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13672
to look at the new patch set (#7).
Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server
......................................................................
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
---
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
A be/src/rpc/cookie-util.cc
A be/src/rpc/cookie-util.h
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M be/src/util/openssl-util.cc
M be/src/util/openssl-util.h
M common/thrift/metrics.json
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
M fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java
M fe/src/test/java/org/apache/impala/service/JdbcTestBase.java
13 files changed, 537 insertions(+), 102 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/72/13672/7
--
To view, visit http://gerrit.cloudera.org:8080/13672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
Gerrit-Change-Number: 13672
Gerrit-PatchSet: 7
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Todd Lipcon, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13672
to look at the new patch set (#3).
Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server
......................................................................
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.
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 not authenticated with 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
---
M be/src/rpc/CMakeLists.txt
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
A be/src/rpc/cookie-util.cc
A be/src/rpc/cookie-util.h
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M be/src/util/openssl-util.cc
M be/src/util/openssl-util.h
M common/thrift/metrics.json
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
M fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java
M fe/src/test/java/org/apache/impala/service/JdbcTestBase.java
19 files changed, 517 insertions(+), 72 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/72/13672/3
--
To view, visit http://gerrit.cloudera.org:8080/13672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
Gerrit-Change-Number: 13672
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13672 )
Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server
......................................................................
Patch Set 5:
(16 comments)
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.h
File be/src/rpc/cookie-util.h:
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.h@29
PS5, Line 29: bool AuthenticateCookie(ThriftServer::ConnectionContext* connection_context,
I think hash could be passed in as a const AuthenticationHash& in both these methods if the methods on AuthenticationHash were const qualified.
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.h@34
PS5, Line 34: string
std::string
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc
File be/src/rpc/cookie-util.cc:
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@38
PS5, Line 38: timstamp
timestamp
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@49
PS5, Line 49: if (TryStripPrefixString(cookie_header, StrCat(COOKIE_NAME, "="), &cookie)) {
It'd be more consistent to invert this condition and do a goto.
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@71
PS5, Line 71: signnature
signature
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@81
PS5, Line 81: long create_time = stol(cookie_value_split[1]);
I'd probably prefer using StringParser::StringToInt() for the better error handling. Non-blocking comment since the timestamp is generated by us and we'd hit an error anyway if it returned 0.
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@96
PS5, Line 96: LOG(ERROR) << "Invalid cookie provided: " << cookie_header
Maybe should be WARNING or INFO since it doesn't indicate the service itself malfunctioning.
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@104
PS5, Line 104: rand
We generally don't like rand() because it's not a particularly good quality RNG. I need to think a bit more about whether it's appropriate here.
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@104
PS5, Line 104: UnixMillis
Should this be MonotonicMillis() instead? Since we only use this for comparing against time of this server.
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@105
PS5, Line 105: 32
Can we use a constant here instead of the magic number?
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@112
PS5, Line 112: char base64_signature[base64_len + 1];
Is this array size bounded? I think it would be safer to statically initialise it to the known max size instead of allocating a variable-length array on the stack.
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@117
PS5, Line 117: string secure_flag = ";Secure";
We're just pointing to constants, so could use a "const char*" and avoid allocating the string.
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/transport/THttpServer.h
File be/src/transport/THttpServer.h:
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/transport/THttpServer.h@32
PS5, Line 32: ettempts
"attempts" here and below
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/util/openssl-util.h
File be/src/util/openssl-util.h:
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/util/openssl-util.h@99
PS5, Line 99: 32
Use a named constant?
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/util/openssl-util.cc
File be/src/util/openssl-util.cc:
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/util/openssl-util.cc@167
PS5, Line 167: bool AuthenticationHash::Compute(const uint8_t* data, int64_t len, uint8_t* out) {
Looks like this methods could be const
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/util/openssl-util.cc@174
PS5, Line 174: bool AuthenticationHash::Verify(
Looks like this methods could be const
--
To view, visit http://gerrit.cloudera.org:8080/13672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
Gerrit-Change-Number: 13672
Gerrit-PatchSet: 5
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 16 Aug 2019 01:21:17 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13672 )
Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server
......................................................................
Patch Set 8:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4844/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/13672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
Gerrit-Change-Number: 13672
Gerrit-PatchSet: 8
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sun, 25 Aug 2019 01:39:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13672 )
Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server
......................................................................
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>
---
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
A be/src/rpc/cookie-util.cc
A be/src/rpc/cookie-util.h
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M be/src/util/openssl-util.cc
M be/src/util/openssl-util.h
M common/thrift/metrics.json
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
M fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java
M fe/src/test/java/org/apache/impala/service/JdbcTestBase.java
13 files changed, 541 insertions(+), 106 deletions(-)
Approvals:
Thomas Tauber-Marshall: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/13672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
Gerrit-Change-Number: 13672
Gerrit-PatchSet: 9
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13672 )
Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server
......................................................................
Patch Set 8:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4361/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/13672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
Gerrit-Change-Number: 13672
Gerrit-PatchSet: 8
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sun, 25 Aug 2019 02:19:30 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13672 )
Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server
......................................................................
Patch Set 8: Code-Review+2
carrying forward
--
To view, visit http://gerrit.cloudera.org:8080/13672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
Gerrit-Change-Number: 13672
Gerrit-PatchSet: 8
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sun, 25 Aug 2019 01:38:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/13672 )
Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server
......................................................................
Patch Set 2:
(3 comments)
Just nits
http://gerrit.cloudera.org:8080/#/c/13672/2/be/src/rpc/cookie-mgr.h
File be/src/rpc/cookie-mgr.h:
http://gerrit.cloudera.org:8080/#/c/13672/2/be/src/rpc/cookie-mgr.h@56
PS2, Line 56: // The time, in Unix millis, that the cookies was created. Used to determine when to
nit: the cookie
http://gerrit.cloudera.org:8080/#/c/13672/2/be/src/rpc/cookie-mgr.h@69
PS2, Line 69: // Thread that periodically iterates over all cookkies and removes one that are past
nit: spelling -> cookies
http://gerrit.cloudera.org:8080/#/c/13672/2/be/src/rpc/cookie-mgr.cc
File be/src/rpc/cookie-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/13672/2/be/src/rpc/cookie-mgr.cc@54
PS2, Line 54: SleepForMs(FLAGS_max_cookie_lifetime_s * 10);
Should this be * 100 ?
--
To view, visit http://gerrit.cloudera.org:8080/13672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
Gerrit-Change-Number: 13672
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 20 Jun 2019 00:36:55 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13672 )
Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server
......................................................................
Patch Set 3:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/13672/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13672/2//COMMIT_MSG@7
PS2, Line 7: IMPALA-8584: Add cookie support to the HTTP HS2 server
> Some questions about the design here:
It turns out that Hive doesn't store the cookies, it just uses a SHA256 HMAC, as you suggested. This approach has various advantages, so I switched to that.
This solves your first concern - each client that authenticates without using a cookie with get its own cookie generated and sent back to it, and cookies aren't tied to sessions or connections.
For your second question, I think that people generally use sticky sessions. The hs2 sessions themselves aren't shared via the statestore, so reconnecting to a new impalad requires calling OpenSession(). So, my patch doesn't share the key. Of course, if we think sharing the key is an important optimization it would be straightforward to add.
http://gerrit.cloudera.org:8080/#/c/13672/2/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:
http://gerrit.cloudera.org:8080/#/c/13672/2/be/src/rpc/authentication.cc@546
PS2, Line 546: , &SaslMutexU
> Any idea what the source of this entropy is? Is it subject to entropy delet
Done
http://gerrit.cloudera.org:8080/#/c/13672/2/be/src/rpc/authentication.cc@562
PS2, Line 562: GENERAL_CALLBACKS[0].id = SASL_CB_LOG;
> Do we want any expiration on the cookie? Or is the default (session cookie)
Done
http://gerrit.cloudera.org:8080/#/c/13672/2/be/src/rpc/cookie-mgr.h
File be/src/rpc/cookie-mgr.h:
http://gerrit.cloudera.org:8080/#/c/13672/2/be/src/rpc/cookie-mgr.h@56
PS2, Line 56:
> nit: the cookie
Done
http://gerrit.cloudera.org:8080/#/c/13672/2/be/src/rpc/cookie-mgr.h@69
PS2, Line 69:
> nit: spelling -> cookies
Done
http://gerrit.cloudera.org:8080/#/c/13672/2/be/src/rpc/cookie-mgr.cc
File be/src/rpc/cookie-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/13672/2/be/src/rpc/cookie-mgr.cc@54
PS2, Line 54:
> Should this be * 100 ?
Done
http://gerrit.cloudera.org:8080/#/c/13672/2/be/src/rpc/cookie-mgr.cc@61
PS2, Line 61:
> hrm, this is odd. YOu can't just do .emplace(username, Cookie(cookie, UnixM
Done
http://gerrit.cloudera.org:8080/#/c/13672/2/be/src/transport/THttpServer.cpp
File be/src/transport/THttpServer.cpp:
http://gerrit.cloudera.org:8080/#/c/13672/2/be/src/transport/THttpServer.cpp@104
PS2, Line 104: } else if (THRIFT_strncasecmp(header, "Cookie", sz) == 0 && useCookies_) {
> the Cookie header can pass multiple cookies, separated by ';'. It doesn't s
Done
--
To view, visit http://gerrit.cloudera.org:8080/13672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
Gerrit-Change-Number: 13672
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 09 Jul 2019 01:22:33 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13672 )
Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server
......................................................................
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc
File be/src/rpc/cookie-util.cc:
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@104
PS5, Line 104:
> I don't really have a better suggestion, it would probably be overcomplicat
Done
http://gerrit.cloudera.org:8080/#/c/13672/6/be/src/transport/THttpServer.cpp
File be/src/transport/THttpServer.cpp:
http://gerrit.cloudera.org:8080/#/c/13672/6/be/src/transport/THttpServer.cpp@181
PS6, Line 181: if (metrics_enabled_) http_metrics_->total_cookie_auth_success_->Increment(1);
> I thought about whether it would be a good idea to limit the number of cook
Good point. A well behaved client should only ever be returning one cookie to us, but we probably still want to be at least a little flexible for not-quite-well-behaved clients.
We should probably only bother attempting to check the signature for a single cookie - we don't need to accommodate clients that are so poorly behaved as to provide us with multiple cookies matching our cookie name, since that's clearly way out of spec.
We probably also eventually will want to have some sort of rate limiting of requests or other restrictions on a per connection or per client host basis or similar to minimize possible ddos, either unintentional or malicious. That's a problem both for checking cookies and for ldap/kerberos auth attempts. Of course, the key motivation of all of this http client work is to allow for proxying of connections through something like Apache Knox, and in that case we can hopefully rely on the proxy to take the brunt of any misbehaved clients.
--
To view, visit http://gerrit.cloudera.org:8080/13672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
Gerrit-Change-Number: 13672
Gerrit-PatchSet: 7
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 23 Aug 2019 22:52:30 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13672 )
Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server
......................................................................
Patch Set 2:
(5 comments)
didn't look in detail yet, a couple high-level questions first
http://gerrit.cloudera.org:8080/#/c/13672/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13672/2//COMMIT_MSG@7
PS2, Line 7: IMPALA-8584: Add cookie support to the HTTP HS2 server
Some questions about the design here:
- by persisting the cookies in a bimap, what happens when you have multiple clients separately authenticating as the same user? It seems like you'd end up handing the same UUID cookie to all of the remote users, and then they'd all expire at the same time, causing them to all have to go back to LDAP to reauthenticate simultaneously, which might be problematic for a large number of clients. It might be better to just use unique session IDs, rather than maintaining the reverse mapping from username->uuid.
- Similar question: if Impala is behind a load balancer of some kind, when a user reconnects, they'll use the same cookie to talk to a different impalad. That other impalad won't find the cookie and will force them to re-auth each time they switch backends I guess? In typical deployments, do people reconnect frequently through the load balancer or tend to use sticky sessions? An alternative here would be to use HMAC signatures in the cookie, using a secret shared via the statestore or somesuch.
Any idea what Hive does, for comparison? Would be good to document a couple of these design points in the commit message or the JIRA.
http://gerrit.cloudera.org:8080/#/c/13672/2/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:
http://gerrit.cloudera.org:8080/#/c/13672/2/be/src/rpc/authentication.cc@546
PS2, Line 546: random_device
Any idea what the source of this entropy is? Is it subject to entropy deletion DOS attacks? (or entropy depletion associated perf problems?)
http://gerrit.cloudera.org:8080/#/c/13672/2/be/src/rpc/authentication.cc@562
PS2, Line 562: cookie = Substitute("impala.hs2.auth=$0", PrintId(cookie_id));
Do we want any expiration on the cookie? Or is the default (session cookie) appropriate?
Also, I would guess we want this to be an http-only and secure cookie (assuming https is enabled)? Might be some other settings that would be a good idea for security: see https://odino.org/security-hardening-http-cookies/ which has a bunch of recommendations.
http://gerrit.cloudera.org:8080/#/c/13672/2/be/src/rpc/cookie-mgr.cc
File be/src/rpc/cookie-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/13672/2/be/src/rpc/cookie-mgr.cc@61
PS2, Line 61: username_to_cookie_.emplace(std::piecewise_construct, std::forward_as_tuple(username),
hrm, this is odd. YOu can't just do .emplace(username, Cookie(cookie, UnixMillis()); here?
http://gerrit.cloudera.org:8080/#/c/13672/2/be/src/transport/THttpServer.cpp
File be/src/transport/THttpServer.cpp:
http://gerrit.cloudera.org:8080/#/c/13672/2/be/src/transport/THttpServer.cpp@104
PS2, Line 104: cookieValue_ = string(value);
the Cookie header can pass multiple cookies, separated by ';'. It doesn't seem like that's getting handled here? It might also be possible to pass multiple Cookie headers, but not sure about that one.
--
To view, visit http://gerrit.cloudera.org:8080/13672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
Gerrit-Change-Number: 13672
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 18 Jun 2019 19:19:31 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13672 )
Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/util/openssl-util.cc
File be/src/util/openssl-util.cc:
http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/util/openssl-util.cc@169
PS5, Line 169: uint8_t* result = HMAC(EVP_sha256(), key_, KEY_LEN, data, len, out, &out_len);
I think we might need to clean for and clean errors after the HMAC call. Hard to know with the insane OpenSSL APIs.
--
To view, visit http://gerrit.cloudera.org:8080/13672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
Gerrit-Change-Number: 13672
Gerrit-PatchSet: 5
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 16 Aug 2019 18:19:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13672 )
Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server
......................................................................
Patch Set 7: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13672/7/be/src/rpc/cookie-util.cc
File be/src/rpc/cookie-util.cc:
http://gerrit.cloudera.org:8080/#/c/13672/7/be/src/rpc/cookie-util.cc@62
PS7, Line 62: Recieved
typo: Received
--
To view, visit http://gerrit.cloudera.org:8080/13672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
Gerrit-Change-Number: 13672
Gerrit-PatchSet: 7
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 23 Aug 2019 23:49:19 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13672 )
Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server
......................................................................
Patch Set 3:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/3841/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/13672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4
Gerrit-Change-Number: 13672
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 09 Jul 2019 02:02:56 +0000
Gerrit-HasComments: No