Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Improve compressed request handling #10260

Closed
peternied opened this issue Sep 27, 2023 · 4 comments · Fixed by opensearch-project/security#3418
Closed

[Feature] Improve compressed request handling #10260

peternied opened this issue Sep 27, 2023 · 4 comments · Fixed by opensearch-project/security#3418
Assignees
Labels
enhancement Enhancement or improvement to existing feature or request v2.11.0 Issues and PRs related to version 2.11.0

Comments

@peternied
Copy link
Member

Is your feature request related to a problem? Please describe.

In some cases, a request is considered invalid from its headers and its unnecessary to inspect the body of the request. For compressed requests, decompressed bodies can sometimes be quite large so its a benefit to skip decompression if the body is only being discarded.

Describe the solution you'd like
There should be some mechanism that is accessible by plugins to provide guidance to continue processing a request before more resources are used.

Describe alternatives you've considered

@peternied
Copy link
Member Author

peternied commented Oct 2, 2023

We been exploring two approaches to address how compressed requests are handled within our system. Here's a overview of both:

Header Verifier with early request exit [1]

Craig is looking into adjusting the timing and manner in which certain components, specifically the "rest controller components", are triggered within the core of OpenSearch. In essence, this approach brings existing request functionalities into broader usage.

Pros:

  • Minimizes the changes needed in plugins that are connected downstream.

Cons:

  • It blurs the distinctiveness of the "request" feature's design, potentially introducing unexpected behaviors.

PRs

Minimal Header Verifier [2]

Peter is focusing on the security side of things. Specifically, he's rethinking how the security plugin responds to verification (or "authentication") requests.

Pros:

  • The core system remains mostly untouched, preserving its stability.
  • Tidies up sections of the security plugin that were previously a bit convoluted.

Cons:

  • Requires a significant overhaul of the security plugin, which could be time-consuming and might introduce temporary instability.

PRs

As we delve deeper, we'll keep everyone updated on our progress and the potential implications of each approach. Feedback and thoughts are always welcome!

@reta
Copy link
Collaborator

reta commented Oct 2, 2023

In some cases, a request is considered invalid from its headers and its unnecessary to inspect the body of the request.

@peternied could you please give an example of invalid request? I am thinking about wrong auth credentials fe, is that the case?

I think if we need to inspect headers and reject the request even before consuming the content, there are two options:

  • HttpRequestDecoder that decodes the request, including headers
  • HttpObjectAggregator that aggregates the request from chunk with begin / aggregate / end lifecycle

Something to be aware of, with #9672 the request body is consumed lazily (as a stream), so the headers could be consulted before processing the request body.

@peternied
Copy link
Member Author

@reta Here is an example of what a request could look likehttps://github.com/opensearch-project/security/pull/3417/files#r1343153212

POST /*/_search
{
"items": [
    {"a": 123, "b":123, ...},
    {"a": 123, "b":123, ...},
    ... repeated many times ...
]

I think having validation processes that only use headers vs a latter part of the pipeline get the request body (whole vs stream) seems like a good way to separate these concerns of validation vs processing while lowers the resource usage on invalid / rejected requests

@peternied
Copy link
Member Author

Updating this issue; @cwperks and I have been trying to figure out how to best approach this problem, earlier this was kind of called top-down vs bottoms-up, we we have landed on a design pattern that is the meeting place between both of these points.

By allowing a header verification step to the pipeline along with allowing the decompressor to be wrapped, plugins that are engaged in the network configuration can fine tune their behavior. The business logic of to process criteria for inclusion/exclusion solely are declared within the security plugin.

If there is any behavior that needs to pass information between these system states it is encapsulated wholly in the plugin codebase.

The following pull requests are part of the total delivery to address this feature:

peternied added a commit to opensearch-project/security that referenced this issue Oct 6, 2023
Previously unauthorized requests were fully processed and rejected once
they reached the RestHandler. This allocations more memory and resources
for these requests that might not be useful if they are already detected
as unauthorized. Using the headerVerifer and decompressor customization
from [1], perform an early authorization check when only the headers are
available, save an 'early response' for transmission and do not perform
the decompression on the request to speed up closing out the connection.

- Resolves opensearch-project/OpenSearch#10260

Signed-off-by: Peter Nied <petern@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <craig5008@gmail.com>
Co-authored-by: Peter Nied <petern@amazon.com>
peternied added a commit to peternied/security that referenced this issue Oct 6, 2023
… requests (opensearch-project#3418)

Previously unauthorized requests were fully processed and rejected once
they reached the RestHandler. This allocations more memory and resources
for these requests that might not be useful if they are already detected
as unauthorized. Using the headerVerifer and decompressor customization
from [1], perform an early authorization check when only the headers are
available, save an 'early response' for transmission and do not perform
the decompression on the request to speed up closing out the connection.

- Resolves opensearch-project/OpenSearch#10260

Signed-off-by: Peter Nied <petern@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <craig5008@gmail.com>
Co-authored-by: Peter Nied <petern@amazon.com>
peternied added a commit to opensearch-project/security that referenced this issue Oct 7, 2023
… requests (#3418) (#3495)

### Description

Backport of 6b0b682 from #3418

Previously unauthorized requests were fully processed and rejected once
they reached the RestHandler. This allocations more memory and resources
for these requests that might not be useful if they are already detected
as unauthorized. Using the headerVerifer and decompressor customization
from [1], perform an early authorization check when only the headers are
available, save an 'early response' for transmission and do not perform
the decompression on the request to speed up closing out the connection.

- Resolves opensearch-project/OpenSearch#10260

Signed-off-by: Peter Nied <petern@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <craig5008@gmail.com>
Co-authored-by: Craig Perkins <cwperx@amazon.com>
opensearch-trigger-bot bot pushed a commit to opensearch-project/security that referenced this issue Oct 7, 2023
… requests (#3418) (#3495)

### Description

Backport of 6b0b682 from #3418

Previously unauthorized requests were fully processed and rejected once
they reached the RestHandler. This allocations more memory and resources
for these requests that might not be useful if they are already detected
as unauthorized. Using the headerVerifer and decompressor customization
from [1], perform an early authorization check when only the headers are
available, save an 'early response' for transmission and do not perform
the decompression on the request to speed up closing out the connection.

- Resolves opensearch-project/OpenSearch#10260

Signed-off-by: Peter Nied <petern@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <craig5008@gmail.com>
Co-authored-by: Craig Perkins <cwperx@amazon.com>
(cherry picked from commit f7c47af)
peternied added a commit to peternied/security that referenced this issue Nov 9, 2023
…requests (opensearch-project#3418) (opensearch-project#3495)

Backport of 6b0b682 from opensearch-project#3418

Previously unauthorized requests were fully processed and rejected once
they reached the RestHandler. This allocations more memory and resources
for these requests that might not be useful if they are already detected
as unauthorized. Using the headerVerifer and decompressor customization
from [1], perform an early authorization check when only the headers are
available, save an 'early response' for transmission and do not perform
the decompression on the request to speed up closing out the connection.

- Resolves opensearch-project/OpenSearch#10260

Signed-off-by: Peter Nied <petern@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <craig5008@gmail.com>
Co-authored-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Peter Nied <petern@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement or improvement to existing feature or request v2.11.0 Issues and PRs related to version 2.11.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants