Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/22462 )
Change subject: IMPALA-13687: Support shared secret key for cookies ...................................................................... Patch Set 9: (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() && FLAGS_max_cookie_lifetime_s > 0) { We should throw an error when !FLAGS_cookie_secret_file.empty() && FLAGS_max_cookie_lifetime_s == 0. Otherwise, Impala will use the default hash when the admins intended to use the cookie secret, and they likely will miss the log message "Using default cookie secret". 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 SHA256 digest except for 1 byte and then puts a newline character on the end. In that case, n will be equal to SHA256_DIGEST_LENGTH but it won't be a valid digest. Do we need to handle that situation here or will later code catch it? 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? 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: Thread.sleep(1000); // Wait for the key to be reloaded. Nit: this could look for the log message "Loaded authentication key from ", but this sleep is also fine here. If it takes longer than a second to load 32 bytes, there is a big problem somewhere else. http://gerrit.cloudera.org:8080/#/c/22462/9/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java@1066 PS9, Line 1066: // Open a session which will get username 'Test1Ldap'. Nit: Comment should be something about a failure is expected. 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: SecureRandom.getInstanceStrong().nextBytes(key); Moving this initialization to before the thread starts will ensure generating the randomness does not cause any flakiness due to taking too long to generate. 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: } The above pattern to generate a random hash appears in several JUnit tests. Can it be refactored into a utility method that all the JUnit tests call? -- 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: 9 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: Fri, 05 Sep 2025 23:04:49 +0000 Gerrit-HasComments: Yes
