Fix issue connecting with prometheus by wrapping with AccessController.doPrivilegedChecked #5061
+2
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…r.doPrivilegedChecked Signed-off-by: Craig Perkins <cwperx@amazon.com>
cwperks
requested review from
GumpacG,
LantaoJin,
MaxKsyunz,
RyanL1997,
Swiddis,
YANG-DB,
Yury-Fridlyand,
acarbonetto,
anirudha,
dai-chen,
derek-ho,
forestmvey,
joshuali925,
kavithacm,
mengweieric,
noCharger,
penghuo,
ps48,
qianheng-aws,
seankao-az,
vamsimanohar,
ykmr1224 and
yuancu
as code owners
Contributor
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdded security privilege checking to the HTTP client's metric retrieval operation by wrapping the execution within Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
lezzago
approved these changes
Jan 20, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@direct-query-core/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClientImpl.java`:
- Line 179: Add a centralized helper in PrometheusClientImpl named something
like executePrivileged(Request request, OkHttpClient client) that wraps
AccessController.doPrivilegedChecked(() -> client.newCall(request).execute())
and throws IOException; then replace every direct call to
prometheusHttpClient.newCall(request).execute() and
alertmanagerHttpClient.newCall(request).execute() across methods queryRange,
query, getLabels, getLabel, getAllMetrics, getSeries, queryExemplars, getAlerts,
getRules, getAlertmanagerAlerts, getAlertmanagerAlertGroups,
getAlertmanagerReceivers, getAlertmanagerSilences, and
createAlertmanagerSilences to call executePrivileged(request,
prometheusHttpClient) or executePrivileged(request, alertmanagerHttpClient)
respectively so the security wrapper is applied consistently.
| logger.debug("queryUrl: " + queryUrl); | ||
| Request request = new Request.Builder().url(queryUrl).build(); | ||
| Response response = this.prometheusHttpClient.newCall(request).execute(); | ||
| Response response = AccessController.doPrivilegedChecked(() -> this.prometheusHttpClient.newCall(request).execute()); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all HTTP call sites in the file that may need the same fix
echo "=== HTTP calls using prometheusHttpClient ==="
rg -n "prometheusHttpClient\.newCall" direct-query-core/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClientImpl.java
echo ""
echo "=== HTTP calls using alertmanagerHttpClient ==="
rg -n "alertmanagerHttpClient\.newCall" direct-query-core/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClientImpl.java
echo ""
echo "=== Calls already wrapped with doPrivilegedChecked ==="
rg -n "doPrivilegedChecked" direct-query-core/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClientImpl.javaRepository: opensearch-project/sql
Length of output: 1543
🏁 Script executed:
#!/bin/bash
# Extract method signatures to confirm they are distinct methods
echo "=== Method signatures at the call sites ==="
rg -B 5 "prometheusHttpClient\.newCall|alertmanagerHttpClient\.newCall" direct-query-core/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClientImpl.java | grep -E "^\s*(public|private)\s+|prometheusHttpClient\.newCall|alertmanagerHttpClient\.newCall" | head -50Repository: opensearch-project/sql
Length of output: 1308
🏁 Script executed:
#!/bin/bash
# Extract method signatures more clearly
echo "=== Method signatures with line numbers ==="
rg -B 10 "prometheusHttpClient\.newCall|alertmanagerHttpClient\.newCall" \
direct-query-core/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClientImpl.java \
| grep -E "^\s*[0-9]+-\s*(public|private)" | head -20Repository: opensearch-project/sql
Length of output: 109
🏁 Script executed:
#!/bin/bash
# Read the file with line numbers to see the method context
head -330 direct-query-core/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClientImpl.java | tail -150 | cat -nRepository: opensearch-project/sql
Length of output: 7759
🏁 Script executed:
#!/bin/bash
# Read the first part of the file to see queryRange and other methods
head -200 direct-query-core/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClientImpl.java | cat -nRepository: opensearch-project/sql
Length of output: 9152
🏁 Script executed:
#!/bin/bash
# Check context around line 179 for any comments explaining the wrapper
sed -n '168,183p' direct-query-core/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClientImpl.java | cat -nRepository: opensearch-project/sql
Length of output: 993
Inconsistent application of the security fix across HTTP call sites.
The AccessController.doPrivilegedChecked wrapper is applied only to getAllMetrics (line 179), but 13 other methods in this class make HTTP calls using the same pattern (prometheusHttpClient.newCall(request).execute() or alertmanagerHttpClient.newCall(request).execute()). If the SecurityException is triggered by socket connections in getAllMetrics, it will likely occur in other methods making identical HTTP calls:
queryRange(line 95)query(line 130)getLabels(line 150)getLabel(line 165)getSeries(line 198)queryExemplars(line 215)getAlerts(line 226)getRules(line 239)getAlertmanagerAlerts(line 252)getAlertmanagerAlertGroups(line 265)getAlertmanagerReceivers(line 277)getAlertmanagerSilences(line 289)createAlertmanagerSilences(line 305)
Extract a helper method to centralize the privileged execution and apply it consistently:
Suggested approach
private Response executePrivileged(Request request, OkHttpClient client) throws IOException {
return AccessController.doPrivilegedChecked(() -> client.newCall(request).execute());
}Then use it consistently across all HTTP call sites:
-Response response = this.prometheusHttpClient.newCall(request).execute();
+Response response = executePrivileged(request, this.prometheusHttpClient);🤖 Prompt for AI Agents
In
`@direct-query-core/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClientImpl.java`
at line 179, Add a centralized helper in PrometheusClientImpl named something
like executePrivileged(Request request, OkHttpClient client) that wraps
AccessController.doPrivilegedChecked(() -> client.newCall(request).execute())
and throws IOException; then replace every direct call to
prometheusHttpClient.newCall(request).execute() and
alertmanagerHttpClient.newCall(request).execute() across methods queryRange,
query, getLabels, getLabel, getAllMetrics, getSeries, queryExemplars, getAlerts,
getRules, getAlertmanagerAlerts, getAlertmanagerAlertGroups,
getAlertmanagerReceivers, getAlertmanagerSilences, and
createAlertmanagerSilences to call executePrivileged(request,
prometheusHttpClient) or executePrivileged(request, alertmanagerHttpClient)
respectively so the security wrapper is applied consistently.
Collaborator
There was a problem hiding this comment.
Thanks for the change. The change itself looks good to me, however, I'm also curious the following:
- among all these components in the above list, why only the Prometheus got impacted?
- why this just be spotted by now?
Member
|
@Swiddis can you help to review this? I remember you had a PR to remove all privilegedcheck wrappers in 3.x branch. Is the current fixing valid? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes an issue connecting to a prometheus connector.
Before:
After:
Full Stack Trace
Check List
--signoffor-s.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.