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 11: (7 comments) http://gerrit.cloudera.org:8080/#/c/22462/9/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/22462/9/be/src/rpc/authentication.cc@1726 PS9, Line 1726: if (!FLAGS_cookie_secret_file.empty()) { > We should throw an error when !FLAGS_cookie_secret_file.empty() && FLAGS_ma max_cookie_lifetime_s doesn't seem to impact whether cookies are generated, just if they're accepted. It's an odd distinction, but I'm tempted to remove that part of the conditional. http://gerrit.cloudera.org:8080/#/c/22462/9/be/src/util/openssl-util.cc File be/src/util/openssl-util.cc: http://gerrit.cloudera.org:8080/#/c/22462/9/be/src/util/openssl-util.cc@306 PS9, Line 306: if (n != SHA256_DIGEST_LENGTH) { > One potential situation here is the file creator copies/pastes the entire S What would make it an invalid digest? It might not be intended, but it'd likely work. I'm tempted not to guard for that, because '\n' could be a valid byte in the binary text generated for this. http://gerrit.cloudera.org:8080/#/c/22462/9/be/src/util/openssl-util.cc@325 PS9, Line 325: int wd = inotify_add_watch( > What does this watch do if the file is deleted and re-created? When recreated, it will either trigger IN_CLOSE_WRITE or IN_MOVE_SELF. I do wonder if IN_MODIFY is necessary, but given that a valid file is small it should only trigger one redundant event. The tests I wrote generally overwrite the existing file. http://gerrit.cloudera.org:8080/#/c/22462/9/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/9/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java@1060 PS9, Line 1060: TOpenSessionReq openReq = new TOpenSessionReq(); > Nit: this could look for the log message "Loaded authentication key from ", Decided to leave this, log scraping has been a bit error-prone and we don't have any convenient helpers. http://gerrit.cloudera.org:8080/#/c/22462/9/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java@1066 PS9, Line 1066: } > Nit: Comment should be something about a failure is expected. Done http://gerrit.cloudera.org:8080/#/c/22462/9/fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java File fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java: http://gerrit.cloudera.org:8080/#/c/22462/9/fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java@352 PS9, Line 352: // Cookie auth is expected to succeed at least once, possibly many times. > Moving this initialization to before the thread starts will ensure generati Moved this to a helper and changed it to ThreadLocalRandom to prioritize performance since strong randomness isn't a requirement in testing. http://gerrit.cloudera.org:8080/#/c/22462/9/fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java File fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java: http://gerrit.cloudera.org:8080/#/c/22462/9/fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java@186 PS9, Line 186: // there will already be some successful auth attempts. > The above pattern to generate a random hash appears in several JUnit tests. Done -- 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: 11 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, 10 Sep 2025 23:02:36 +0000 Gerrit-HasComments: Yes
