Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/24278 )
Change subject: KUDU-3693: Add bearer token auth for Prometheus ...................................................................... Patch Set 3: (12 comments) http://gerrit.cloudera.org:8080/#/c/24278/3/src/kudu/integration-tests/prometheus_auth-itest.cc File src/kudu/integration-tests/prometheus_auth-itest.cc: http://gerrit.cloudera.org:8080/#/c/24278/3/src/kudu/integration-tests/prometheus_auth-itest.cc@118 PS3, Line 118: .c_str() > UUIC, GetString() returns std::string, so why to call c_str() on the first Done http://gerrit.cloudera.org:8080/#/c/24278/3/src/kudu/integration-tests/prometheus_auth-itest.cc@222 PS3, Line 222: > Would be nice to verify on the possibility of the following configuration: Ah yeas this is a very good point! Reworked the auth itest to cover this area. (and also the TLS part which now has to be configured with bearer token) http://gerrit.cloudera.org:8080/#/c/24278/3/src/kudu/server/webserver-test.cc File src/kudu/server/webserver-test.cc: http://gerrit.cloudera.org:8080/#/c/24278/3/src/kudu/server/webserver-test.cc@20 PS3, Line 20: #include <string.h> > nit: convert into C++-stype <cstring> and move it to the set of other C++ h Done http://gerrit.cloudera.org:8080/#/c/24278/3/src/kudu/server/webserver.cc File src/kudu/server/webserver.cc: http://gerrit.cloudera.org:8080/#/c/24278/3/src/kudu/server/webserver.cc@156 PS3, Line 156: CRYPTO_memcmp(a, b, a_len) == 0; > Would regular memcmp() fit the bill here? Reading memcmp's https://man7.org/linux/man-pages/man3/memcmp.3.html caveat section is the root cause why I've opted for CRYPTO_memcmp. http://gerrit.cloudera.org:8080/#/c/24278/3/src/kudu/server/webserver.cc@158 PS3, Line 158: unsigned char diff = 0; : for (size_t i = 0; i < a_len; i++) { : diff |= static_cast<unsigned char>(a[i]) ^ static_cast<unsigned char>(b[i]); : } : return diff == 0; > Why not to use memcmp() instead of char-by-char comparison? Same as above. Moreover added also comment that explain this choice. Let me know if the updated comments and background look OK. http://gerrit.cloudera.org:8080/#/c/24278/3/src/kudu/server/webserver.cc@638 PS3, Line 638: << "." > nit: could we drop the trailing period? Done http://gerrit.cloudera.org:8080/#/c/24278/3/src/kudu/server/webserver.cc@644 PS3, Line 644: "Authorization" > Introduce a constant for this and use it here and at other places in this f Done http://gerrit.cloudera.org:8080/#/c/24278/3/src/kudu/server/webserver.cc@648 PS3, Line 648: static const char* const kBearerPrefix = "Bearer " > Make this constexpr? Done http://gerrit.cloudera.org:8080/#/c/24278/3/src/kudu/server/webserver.cc@649 PS3, Line 649: strlen(kBearerPrefix) > Once kBearerPrefix is constexpr, change this to const-eval function or the Done http://gerrit.cloudera.org:8080/#/c/24278/3/src/kudu/server/webserver_options.cc File src/kudu/server/webserver_options.cc: http://gerrit.cloudera.org:8080/#/c/24278/3/src/kudu/server/webserver_options.cc@103 PS3, Line 103: webserver_prometheus_token > For various secrets and other sensitive inputs (e.g., passwords for various Ah sorry I completely overlooked this. Makes sense, adopted the approach. Thanks! http://gerrit.cloudera.org:8080/#/c/24278/3/src/kudu/server/webserver_options.cc@203 PS3, Line 203: FLAGS_webserver_certificate_file.empty() > I'd rather put this into a separate category, reporting an error and return Yes I did not consider this, thanks for bringing this to my attention. The bearer token cmd flag validator now checks this. (also did rework the itest to have https in it) Thanks! http://gerrit.cloudera.org:8080/#/c/24278/3/src/kudu/util/mini_prometheus.cc File src/kudu/util/mini_prometheus.cc: http://gerrit.cloudera.org:8080/#/c/24278/3/src/kudu/util/mini_prometheus.cc@159 PS3, Line 159: if (!options_.bearer_token.empty()) { > BTW, do SD discovery and scrape tokens have to be the same? Not necessarily. I just figured that it would just add more operational burden(token rotation). But if you think there is value, I can separate it into two tokens. -- To view, visit http://gerrit.cloudera.org:8080/24278 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If8169196a2c49d64865095f3bc6da1d3bfbcecfb Gerrit-Change-Number: 24278 Gerrit-PatchSet: 3 Gerrit-Owner: Marton Greber <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Tue, 12 May 2026 10:58:25 +0000 Gerrit-HasComments: Yes
