Fix issue connecting with prometheus by wrapping with AccessController.doPrivilegedChecked #5061
Conversation
…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
📝 WalkthroughSummary by CodeRabbit
WalkthroughWrapped Prometheus and Alertmanager HTTP request executions in Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Client Code
participant PromClient as PrometheusClientImpl
participant AccessCtrl as AccessController
participant HttpClient as OkHttpClient
participant PromServer as Prometheus/Alertmanager
Caller->>PromClient: invoke query / getAlerts / ...
PromClient->>AccessCtrl: doPrivilegedChecked { execute HTTP call }
AccessCtrl->>HttpClient: invoke .newCall(request).execute()
HttpClient->>PromServer: HTTP request
PromServer-->>HttpClient: HTTP response
HttpClient-->>AccessCtrl: response
AccessCtrl-->>PromClient: privileged result
PromClient-->>Caller: return response/result
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
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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()); |
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.
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?
There was a problem hiding this comment.
among all these components in the above list, why only the Prometheus got impacted?
the components in the above list are also for prometheus. I just included them in this PR
I don't know why only Prometheus got impacted, or if any other components got impacted
why this just be spotted by now?
This issue does not occur with core + sql, it needs some other plugins that intercepts and filters requests to trigger. We found it when testing the distribution with all plugins installed
|
@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? |
Signed-off-by: Joshua Li <joshuali925@gmail.com>
|
seems github CI is down right now https://www.githubstatus.com/ |
7 participants
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.