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 5: Code-Review+1 (3 comments) Almost there! Let me know if I'm missing/misunderstanding what's going on in this test scenario: https://gerrit.cloudera.org/#/c/24278/4..5/src/kudu/server/webserver-test.cc@609 I'm ready to give +2 once the question above is clarified/addressed. Thanks! http://gerrit.cloudera.org:8080/#/c/24278/5/src/kudu/server/webserver-test.cc File src/kudu/server/webserver-test.cc: http://gerrit.cloudera.org:8080/#/c/24278/5/src/kudu/server/webserver-test.cc@609 PS5, Line 609: curl_.FetchURL(url_, &buf_); It seems I overlooked this in prior review iterations, but I don't see any Prometheus tokens supplied with this request. Am I missing something? http://gerrit.cloudera.org:8080/#/c/24278/5/src/kudu/server/webserver-test.cc@614 PS5, Line 614: One more scenario to validate is a request with invalid SPNEGO authn creds along with a good/valid Prometheus token -- this shouldn't allow accessing non-prometheus paths. If you aren't going to revv this PS futher than PS5, a separate changelist might be a good venue to take care of this. This is just for regression testing of future modifications: the current code seems to work as expected, IIUC. http://gerrit.cloudera.org:8080/#/c/24278/5/src/kudu/server/webserver.cc File src/kudu/server/webserver.cc: http://gerrit.cloudera.org:8080/#/c/24278/5/src/kudu/server/webserver.cc@673 PS5, Line 673: if (authz_header != nullptr && : strncmp(authz_header, kBearerPrefix, kBearerPrefixLen) == 0) { : const char* provided_token = authz_header + kBearerPrefixLen; : if (!ConstantTimeEquals(provided_token, strlen(provided_token), : prometheus_token_.c_str(), : prometheus_token_.size())) { : return reject_bearer("invalid bearer token"); : } A note on the HTTP OPTIONS method handling: it seems the current implementation does require auth creds for the OPTIONS method on the Prometheus endpoints, and it's a bit off w.r.t. the rest of the endpoints of the embedded webserver. I don't think we need to change anything right now because Prometheus itself doesn't use the OPTIONS method when scraping or discovering targets via HTTP-based SD, IIUC. Just wanted to share my observations; no action is needed at this point as far as I can see unless you are about to revv this patch past PS5 anyway. If revving and posting PS6+, consider adding a comment to explain why we don't follow the same policy w.r.t. auth creds for the OPTIONS method for Prometheus endpoints. -- 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: 5 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 22:47:29 +0000 Gerrit-HasComments: Yes
