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 <tmarsh...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Tue, 18 Jun 2019 19:19:31 +0000 Gerrit-HasComments: Yes
