Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/22462 )
Change subject: IMPALA-13687: Support shared secret key for cookies ...................................................................... Patch Set 5: (5 comments) http://gerrit.cloudera.org:8080/#/c/22462/3/be/src/rpc/auth-provider.h File be/src/rpc/auth-provider.h: http://gerrit.cloudera.org:8080/#/c/22462/3/be/src/rpc/auth-provider.h@179 PS3, Line 179: std::unique_ptr<AuthenticationHash> hash_; > Actually allowing the last hash for an unlimited period seems to contradict Generally rotating the hash is done in response to a security event. In that case you don't want the old hash to be valid. I suppose there are circumstances where you might auto-rotate it to anticipate security events, but I'm not proposing we address them in this ticket. The concern then would be that an in-progress action might be interrupted by rotating the hash. There are two areas this could happen - webserver interaction via the UI. Prompting a new login makes sense after rotating the hash. - Thrift HTTP server actions (presumably HS2-HTTP protocol). These already need to handle headless authentication, so receiving a new cookie should be transparent to the client. I'd prefer not to implement validity for the old hash at the moment. http://gerrit.cloudera.org:8080/#/c/22462/3/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/22462/3/be/src/rpc/authentication.cc@1408 PS3, Line 1408: int val = LDAP_OPT_X_TLS_ALLOW; > Yes, since FLAGS_max_cookie_lifetime_s == 0 turns off cookie auth. Althoug Done http://gerrit.cloudera.org:8080/#/c/22462/3/be/src/util/openssl-util.cc File be/src/util/openssl-util.cc: http://gerrit.cloudera.org:8080/#/c/22462/3/be/src/util/openssl-util.cc@218 PS3, Line 218: ret = OpenSSLErr("PEM_read_bio_X509", Substitute("unexpected error '$0' while " > percpu_rwlock has non-trivial overhead since it creates one rwlock per CPU I've added a rwlock. It's likely multiple clients will hit this at once, while updating the hash is an uncommon operation. percpu_rwlock doesn't make sense, because the data it's guarding isn't cpu-local and we don't need multiple updaters. http://gerrit.cloudera.org:8080/#/c/22462/3/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java File fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java: http://gerrit.cloudera.org:8080/#/c/22462/3/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java@987 PS3, Line 987: // Use HttpClient so we can access the returned cookie. Final array is used > Yes. Added a comment about this. http://gerrit.cloudera.org:8080/#/c/22462/3/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java@992 PS3, Line 992: if (response.containsHeader("Set-Cookie")) { > Please consider adding else { fail("cookie not provided") } here to assert After the cookie is set we won't get back Set-Cookie in the response. I've added an equivalent assertion. -- To view, visit http://gerrit.cloudera.org:8080/22462 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie2e2345f771608069407e9dcf7ed4697fc0214e7 Gerrit-Change-Number: 22462 Gerrit-PatchSet: 5 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Wed, 27 Aug 2025 21:13:07 +0000 Gerrit-HasComments: Yes
