Alexey Serbin 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: (13 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 argument then? 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: * cluster-wide SPENEGO authn for the embedded webserver, but no bearer token authentication for both the Prometheus SD and metrics scrape endpoints 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++ headers below http://gerrit.cloudera.org:8080/#/c/24278/2/src/kudu/server/webserver.cc File src/kudu/server/webserver.cc: http://gerrit.cloudera.org:8080/#/c/24278/2/src/kudu/server/webserver.cc@640 PS2, Line 640: ndResponse(co > ah yes this is true. extracted cmp logic into a function that has fallback I guess we no longer compile on RHEL/CentOS 6, but it's nice to keep things consistent. With the if/else based on OpenSSL version that Marton added in PS3, this should be good to go. 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? 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? http://gerrit.cloudera.org:8080/#/c/24278/3/src/kudu/server/webserver.cc@638 PS3, Line 638: << "." nit: could we drop the trailing period? 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 file (e.g., line 660 below)? 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? 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 corresponding length constant, and use this approach elsewhere as well? 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 private keys, etc.), we rather use a different approach: a command/binary that outputs the desired secret. For example, check out the --webserver_private_key_password_cmd flag. Should we use similar approach here as well? 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 returning 'false' if they supply Prometheus token but don't have the endpoints of the embedded webserver secured with TLS. BTW, RFC6750 mandates using TLS to protect bearer tokens, see https://datatracker.ietf.org/doc/html/rfc6750 To protect against token disclosure, confidentiality protection MUST be applied using TLS [RFC5246] with a ciphersuite that provides confidentiality and integrity protection. This requires that the communication interaction between the client and the authorization server, as well as the interaction between the client and the resource server, utilize confidentiality and integrity protection. Since TLS is mandatory to implement and to use with this specification, it is the preferred approach for preventing token disclosure via the communication channel. For those cases where the client is prevented from observing the contents of the token, token encryption MUST be applied in addition to the usage of TLS protection. 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? -- 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: Sun, 10 May 2026 05:54:04 +0000 Gerrit-HasComments: Yes
