[FEATURE] Add support for headers for ExtensionRestRequest #431
Comments
|
Note the comment in // HERE BE DRAGONS - DO NOT INCLUDE HEADERS
// SEE https://github.com/opensearch-project/OpenSearch/issues/4429
new ExtensionRestRequest(method, path, params, contentType, content, requestIssuerIdentity),We cannot send the headers unfiltered. In particular we need to remove security-sensitive information such as basic authentication (we do not want to send user/password to extensions). So yes, we need headers. No we can't just copy them blindly from the REST request. Yes this need a lot of eyes on it to make sure we get it right. Tagging @davidlago @peternied @cwperks @scrawfor99 to provide 8 more eyes on this issue. |
|
Directly linking the issue in the quoted comment above for your clicking ease: opensearch-project/OpenSearch#4429 |
|
Hi @owaiskazi19, thank you for filing this issue. As @dbwiddis mentioned, there is a lot of complexity that comes into play when you are trying to deal with passing any headers to extensions. I am not sure if there have been discussions elsewhere on GitHub about potential solutions but I don't know that I see an obvious choice. Unfortunately, it comes down to restricting information enough but not too much. One possibility is to allow for a moving definition of what "too much" is. We could make extension installation come with an explanation of the information which the extension uses. Users would then need to agree or decline to accept that use. Unfortunately, this does not really solve the problem of "what is the right amount" but instead punts it down the road and places the burden of judgement on the end user. It is also somewhat counter-productive to a major idea of extensions which seeks to make the installation of extensions lightweight and stress free... Perhaps you have some thoughts on how you would be interested in seeing these restrictions handled? Your perspective as an extension author would be invaluable. Thank you. |
|
This sounds like a CORS problem where the admin of the OpenSearch Cluster needs to be empowered to define what headers are permissible. This may be done with different levels of granularity. Maybe by extension or by route + verb? Access-Control-Allow-Headers is a CORS header used to reply to a client making a pre-flight OPTIONS request with what headers the server permits: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Headers CORS example with nginx: https://enable-cors.org/server_nginx.html Tomcat CORS filter: https://tomcat.apache.org/tomcat-7.0-doc/config/filter.html#CORS_Filter |
My first thought was "hey, that sounds like a good idea, let a user configure if they want to pass, say, the I think there is already some built-in header filtering functionality that can be leveraged, but IMO we should always, without user override, strip the RFC 7235 "Authentication" header, if not others. |
|
@dbwiddis Do we have a full list of headers to block always? |
Do we want to use Levenshtein Distance as a filter? Do I actually get to implement one of those job interview algorithms? :D Seems a better/safer idea to explicitly define a list of headers we need/allow (possibly make it a configurable setting). @owaiskazi19 WDYT? |
Just for my understanding how are we filtering headers currently in OpenSearch/Security? |
|
Hi @owaiskazi19, I found 2 areas in the security plugin that relate to HTTP Headers:
|
|
Thanks @cwperks. Looks like for OpenSearch also we are filtering with |
|
Hey @owaiskazi19 I will implement this, can you give me a list of the headers you need for your AD search detector work? |
One such use case I have encountered for SearchDetector is this. Will add more once I start the work for SearchDetector. |
|
@owaiskazi19 I'm still not sure where actual headers are required to be passed from the The one case you have identified above calls
That comes from the URI, like this. Params are already passed so you should be able to fetch that using something like Looking into the entire public static XContentType parseContentType(List<String> header) {
if (header == null || header.isEmpty()) {
return null;
} else if (header.size() > 1) {
throw new IllegalArgumentException("only one Content-Type header should be provided");
}That is only called from two places. One is the singleton list generated from the param (not headers) above. The other is the constructor which sets the instance field final XContentType xContentType;
try {
xContentType = parseContentType(headers.get("Content-Type"));
} catch (final IllegalArgumentException e) {
throw new ContentTypeHeaderException(e);
}This instance field value is already included in the ExtensionRestRequest created from the original RestRequest, so the result of that header parsing is also available to you. So for this particular use case, I don't think you need the headers, and this issue shouldn't be a blocker. Certainly there is room to implement some of these methods on ExtensionRestRequest and I can work on doing that. I've wondered if ExtensionRestRequest should extend RestRequest and this seems like a valid reason for why that should be; I think going that way may be the best way to get you the method you need. WDYT? |
I think this is probably the best path forward. Here are the constructor needs: protected RestRequest(
NamedXContentRegistry xContentRegistry,
Map<String, String> params,
String path,
Map<String, List<String>> headers,
HttpRequest httpRequest,
HttpChannel httpChannel
) {We already pass If we just do a denyList for the headers mentioned above AND an allowlist currently only containing Content-Type but which we can carefully add to, this would be plug-and-play (and reduce the diff even more). |
|
Hello @dbwiddis, I would like to take this issue |
|
Hey @nassipkali thanks for stepping up! When I initially implemented Rest Handling on Extensions I used one class ( So the approach we will be taking is twofold:
So here's a step-by-step:
You will use the getters on the parameter protected RestRequest(
NamedXContentRegistry xContentRegistry,
Map<String, String> params,
String path,
Map<String, List<String>> headers,
HttpRequest httpRequest,
HttpChannel httpChannel
) {
This should be enough info to get started, let me know if you have any questions! |
|
Oh, almost forgot.... after you do the above you'll replace the parameter in this line with the new request you created: And you'll be replacing the handling in HelloWorld example to the new class. One thing that will definitely change is the xContentParser will be simplified to match the way it currently is done in any of the OpenSearch classes extending |
|
@nassipkali no, 430 will be dealing with work in the SDKClient. The only interaction would be in actual REST handler implementation where when processing a RestRequest you used some information in it to send to client.execute(). |
|
@nassipkali I have opened draft PRs on OpenSearch and SDK for you to refer and get started. The work is not fully finished but the changes will help to get moving and then you can create new PRs or push it to my existing PRs. Anyway you like |
|
@dbwiddis @owaiskazi19 how denyList and allowList should be configured? Is it predefined or it should be pass as params or read from config file |
|
@dbwiddis I'm stuck on the theoretical part of the problem. I think I'm missing some technical details on how I should filter headers. |
We don't have them yet. They can be defined however you want. For now, and for simplicity, it's probably easier to just leave a TODO comment about implementing them later, and just create a new headers object from scratch, containing only the value of the Content Type header.
The headers are in a Alternately, since we are only doing that one header, you could call |
|
Optionally, if you wanted an allowlist variable, you could do restRequest.getHeaders().entrySet().stream()
.filter(e -> !denyList.contains(e.getKey())
.filter(e -> allowList.contains("*") || allowlist.contains(e.getKey()))
.collect(Collectors.toMap()); |
Thanks, I will try something like this. |
|
Also note I just looked them up and fixed the actual headers we want to allow/block:
|
|
Hey @nassipkali I'm going to take on the 2nd part of this issue (creating the new |
Ok then I send pull request to OpenSearch |
How I can use constructor of RestRequest if it has protected modifier? |
Just create the public static RestRequest request(NamedXContentRegistry xContentRegistry, HttpRequest httpRequest, HttpChannel httpChannel) |
|
And to be clear on the dividing line between our work, I will expect you to create that RestRequest object in the |
|
I spent a few minutes seeing what would happen if I passed around the Also looking at that class there are a lot of things we just don't need. So while I still think we need that object, I think we can just have an object wrapped inside the Also since we're not passing it around anywhere, we can just use an anonymous subclass, like this code in public RestExecuteOnExtensionResponse handleRestExecuteOnExtensionRequest(final ExtensionRestRequest request) {
ExtensionRestHandler restHandler = extensionRestPathRegistry.getHandler(request.method(), request.path());
if (restHandler == null) {
return new RestExecuteOnExtensionResponse(
NOT_FOUND,
TEXT_CONTENT_TYPE,
String.join(" ", "No handler for", request.method().name(), request.path()).getBytes(UTF_8),
emptyMap(),
emptyList(),
false
);
}
request.setRestRequest(RestRequest.request(
sdkXContentRegistry.getRegistry(),
new HttpRequest() {
@Override
public Method method() {
return request.method();
}
@Override
public String uri() {
// for now, path strips query off uri but probably want to pass the whole uri
return request.path();
}
@Override
public BytesReference content() {
return request.content();
}
@Override
public Map<String, List<String>> getHeaders() {
// for now, once we have a header object can use that
return Map.of("Content-Type", List.of(request.getXContentType().mediaType()));
}
@Override
public List<String> strictCookies() {
return Collections.emptyList();
}
@Override
public HttpVersion protocolVersion() {
// This is two integers, should add to the request
return null;
}
@Override
public HttpRequest removeHeader(String header) {
// we don't use
return null;
}
@Override
public HttpResponse createResponse(RestStatus status, BytesReference content) {
return null;
}
@Override
public Exception getInboundException() {
// we don't use
return null;
}
@Override
public void release() {}
@Override
public HttpRequest releaseAndCopy() {
// we don't use
return null;
}
},
null
); |
|
And after playing with this a bit more, the best option is not to put this in the request handler, but in the |
|
(We still need to set the xContentRegistry in the handler.) |
|
And after going around in circles I'm back to realizing changing the ExtensionRestRequest is already doing the same BWC breaking stuff that I was trying to avoid, and making it a lot more complicated. SO back to the original plan of a new SDKRestContent object. |
What does SDKRestContent? |
|
Typo, or lack of coffee. I was still thinking xContentRegistry. WE need a |
|
Fixed in #643 and companion PRs. |
Is your feature request related to a problem?
Currently, there's no provision to fetch headers from ExtensionRestRequest which is required for various features of Anomaly Detector plugin and for the future.
What solution would you like?
Create
header- To get the value of the headergetAllHeaderValues- To get all values for the headergetHeaders- To get all of the headers and values associated with the headersThese methods are similar to what RestRequest currently has.
What alternatives have you considered?
A clear and concise description of any alternative solutions or features you've considered.
Do you have any additional context?
Add any other context or screenshots about the feature request here.
The text was updated successfully, but these errors were encountered: