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 4: Code-Review+1 (6 comments) LGTM overall. There is one question left to clarify. I wanted to make sure we are on the same page. Thanks! 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@222 PS3, Line 222: prom_.reset(new MiniPrometheus(opts)); > Ah yeas this is a very good point! Thank you for the update. Alright, so it's not possible to access Prometheus-formatted metrics endpoints without configuring bearer token when SPNEGO is enabled, right? 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: > Reading memcmp's https://man7.org/linux/man-pages/man3/memcmp.3.html I'm curious: did this came from AI? That looks a bit overblown. I don't think it's feasible to mount a timing-based side-channel attack on this given the client input/output always goes through the network, where variation in latency is unpredictable (i.e. almost random) and orders of magnitude more than timing difference for comparison of x and 10x bytes via memcmp() when x is quite small (10-20 bytes, what's quite expected for a token). But I might be missing something here. However, using CRYPTO_memcmp wouldn't hurt in any case. I suggested memcmp() since I was thinking it could help to get rid of if/else macro. But it's not a big deal, especially compared with warm and fuzzy feeling of being more secure when using CRYPTO_memcmp() instead of memcmp() here :) http://gerrit.cloudera.org:8080/#/c/24278/3/src/kudu/server/webserver.cc@158 PS3, Line 158: return CRYPTO_memcmp(a, b, a_len) == 0; : #else : // XOR-accumulate loop: reads every byte regardless of content, keeping the : // comparison constant-time. Regular memcmp() cannot be used because its : // execution time > Same as above. Moreover added also comment that explain this choice. Thanks for adding the comment -- should be easier to understand for the readers. http://gerrit.cloudera.org:8080/#/c/24278/4/src/kudu/server/webserver.cc File src/kudu/server/webserver.cc: http://gerrit.cloudera.org:8080/#/c/24278/4/src/kudu/server/webserver.cc@647 PS4, Line 647: bool is_prometheus_path = false; : if (!prometheus_token_.empty()) { : shared_lock l(lock_); : is_prometheus_path = ContainsKey(prometheus_paths_, request_info->uri); : } : if (is_prometheus_path) { With this approach, we implicitly enforce users to configure Prometheus authn with bearer tokens when SPNEGO authn is enabled for the rest of the embedded webserver, right? If so, maybe we should be more explicit about enforcing this rule via group flag validators? Otherwise, /metrics_prometheus endpoint becomes unavailable with SPNEGO enabled but Prometheus tokens non-configured, and any attempt to fetch data from the endpoint fails with an error about SPNEGO which might be misleading. Alternatively, we might allow access to /prometheus_metrics and SD endpoint in master's webserver without requiring bearer token authentication when the rest of the server is protected by SPNEGO. What do you think? 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@203 PS3, Line 203: t bool has_tls = !FLAGS_webserver_certif > Yes I did not consider this, thanks for bringing this to my attention. The Cool -- thanks for addressing this! Now it's more consistent with the RFC. 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: } > Not necessarily. I just figured that it would just add more operational bur That was only a curiosity -- I was trying to understand whether it's some sort of requirement from the Prometheus side. Thank you for clarifying on this. I agree that having a single token for both end-points is less operational burden, so keeping a single token makes more sense to me as of now. -- 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: 4 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: Wed, 13 May 2026 17:49:03 +0000 Gerrit-HasComments: Yes
