[Impala-ASF-CR] IMPALA-10234: Add support for cookie authentication to impala-shell

["Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>]

Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/16660 )

Change subject: IMPALA-10234: Add support for cookie authentication to impala-shell
......................................................................


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/16660/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/16660/3//COMMIT_MSG@14
PS3, Line 14: - Unit tests were added to test cookie handling methods.
Can you also add a check against the metrics in LdapImpalaShellTest.java, eg. like what I've done in this patch: https://github.com/twmarshall/impala/commit/d74cd55713f7bdd1de629c64d8b4c3a9e08b2785


http://gerrit.cloudera.org:8080/#/c/16660/3//COMMIT_MSG@15
PS3, Line 15: - Tested e2e manually.
It would be good to be sure that we've tested this with the main HTTP proxies that Impala is expected to work with (Knox and nginx). Probably this is fine to leave as a follow up task.


http://gerrit.cloudera.org:8080/#/c/16660/3/shell/ImpalaHttpClient.py
File shell/ImpalaHttpClient.py:

http://gerrit.cloudera.org:8080/#/c/16660/3/shell/ImpalaHttpClient.py@251
PS3, Line 251: # A '401 Unauthorized' response might mean that we tried cookie-based authentication
             :     # with an expired cookie.
We should explicitly detect this by looking at the returned headers for a impala.auth cookie with a max-age of 0 (see GetDeleteCookie() in authentication-util.cc)


http://gerrit.cloudera.org:8080/#/c/16660/3/shell/impala_client.py
File shell/impala_client.py:

http://gerrit.cloudera.org:8080/#/c/16660/3/shell/impala_client.py@406
PS3, Line 406:           return {"Authorization": basic_auth_header}
So I think the way most clients usually handle this is to just send the Authorization header on every request, even if there's an auth cookie. There's basically no downside (just a very small amount of additional data transmitted per request), and it avoids having to re-send the payload if the cookie is rejected.

It would also allow you to make this patch a lot simpler, I think, eg. you wouldn't need the 'custom_headers_func'.



-- 
To view, visit http://gerrit.cloudera.org:8080/16660
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb0bc6e0f58f236866ca9913a2e63d97d5148f51
Gerrit-Change-Number: 16660
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 02 Nov 2020 17:39:12 +0000
Gerrit-HasComments: Yes