-
Notifications
You must be signed in to change notification settings - Fork 272
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
Use BytesRestResponse constructor with contentType in asRestResponse #3717
Use BytesRestResponse constructor with contentType in asRestResponse #3717
Conversation
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
src/main/java/org/opensearch/security/filter/SecurityResponse.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3717 +/- ##
==========================================
+ Coverage 64.88% 66.26% +1.37%
==========================================
Files 292 292
Lines 20776 20806 +30
Branches 3409 3411 +2
==========================================
+ Hits 13481 13787 +306
+ Misses 5606 5318 -288
- Partials 1689 1701 +12
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick turn around
- do we have an issue tracking the bug that is pushing for this change?
- what about adding/modifying a test that validates the correct content type header is received by the client?
@peternied @DarshitChanpura is working on adding tests to this PR that will fail if the response does not have the expected content-type (from before 2.11). @scrawfor99 is working on cataloging APIs or different scenarios where the content-type of the response object changed in 2.11 from prior versions. The issue to track that is #3718 |
Signed-off-by: Stephen Crawford <steecraw@amazon.com>
Signed-off-by: Stephen Crawford <steecraw@amazon.com>
Add Security response tests
I added some tests for Craig's fix. I noted on my merge to Craig's branch but will note here too: There is a known failing test I marked. I do not like that we don't pick a standard default type and would like us to do so. I would pick JSON and that is what I wrote the test for, but I can be overruled to use a different type or keep the behavior in the split way it is now. Edit: I cleaned it so the test passes but I am still grumpy about it and wish it was standard. |
src/test/java/org/opensearch/security/filter/SecurityResponseTests.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Stephen Crawford <steecraw@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two small corrections of naming that I missed when I swapped the tests to pass. Otherwise looks good to me. Thanks Craig :)
src/test/java/org/opensearch/security/filter/SecurityResponseTests.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/security/filter/SecurityResponseTests.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
90255b7
…ests.java Co-authored-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com> Signed-off-by: Craig Perkins <craig5008@gmail.com>
…ests.java Co-authored-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com> Signed-off-by: Craig Perkins <craig5008@gmail.com>
The backport to
To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/security/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/security/backport-2.x
# Create a new branch
git switch --create backport/backport-3717-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 b7f47b1f516bee03a7dfaeec3a9133e4eafa3a73
# Push it to GitHub
git push --set-upstream origin backport/backport-3717-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/security/backport-2.x Then, create a pull request where the |
The backport to
To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/security/backport-2.11 2.11
# Navigate to the new working tree
pushd ../.worktrees/security/backport-2.11
# Create a new branch
git switch --create backport/backport-3717-to-2.11
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 b7f47b1f516bee03a7dfaeec3a9133e4eafa3a73
# Push it to GitHub
git push --set-upstream origin backport/backport-3717-to-2.11
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/security/backport-2.11 Then, create a pull request where the |
…pensearch-project#3717) Modifies `SecurityResponse.asRestResponse` to use the corresponding constructor for `BytesRestResponse` if the `SecurityResponse` contains the content-type header. This adds a test that fails before the change and demonstrates how using the constructor of BytesRestResponse without content-type defaults to text/plain --------- Signed-off-by: Craig Perkins <cwperx@amazon.com> Signed-off-by: Stephen Crawford <steecraw@amazon.com> Signed-off-by: Craig Perkins <craig5008@gmail.com> Co-authored-by: Stephen Crawford <steecraw@amazon.com> Co-authored-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com> (cherry picked from commit b7f47b1)
final String negotiateResponseBody = getNegotiateResponseBody(); | ||
if (negotiateResponseBody != null) { | ||
responseBody = negotiateResponseBody; | ||
headers.putAll(SecurityResponse.CONTENT_TYPE_APP_JSON); | ||
contentType = XContentType.JSON.mediaType(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should understand who populates the header if we explicitly don't populate here?
this.body = body; | ||
this.contentType = contentType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
call other constructor here
if (getHeaders() != null) { | ||
getHeaders().forEach(restResponse::addHeader); | ||
getHeaders().entrySet().forEach(entry -> { entry.getValue().forEach(value -> restResponse.addHeader(entry.getKey(), value)); }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should understand why the header approach was not working and which part of the code is populating the content type header now?
…pensearch-project#3717) Modifies `SecurityResponse.asRestResponse` to use the corresponding constructor for `BytesRestResponse` if the `SecurityResponse` contains the content-type header. This adds a test that fails before the change and demonstrates how using the constructor of BytesRestResponse without content-type defaults to text/plain --------- Signed-off-by: Craig Perkins <cwperx@amazon.com> Signed-off-by: Stephen Crawford <steecraw@amazon.com> Signed-off-by: Craig Perkins <craig5008@gmail.com> Co-authored-by: Stephen Crawford <steecraw@amazon.com> Co-authored-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com> (cherry picked from commit b7f47b1)
… asRestResponse (opensearch-project#3717) (opensearch-project#3721) Backport opensearch-project#3717 to 2.11 Signed-off-by: Craig Perkins <cwperx@amazon.com>
…requests (#3418) (#3675) ### Description Includes: - Backport f7c47af of #3418 - Backport 2dab119 of #3717 - Backport f27dee2 of #3583 --- 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. ```mermaid graph TD oA["Receive Request Headers<br>(Orginal)"] --> oB[Decompress Request] oB --> oC[RestHandler] oC --> osrf[Intercept Request] subgraph sp[Security Plugin] osrf --> oD[Check Authorization] oD --> oE{Authorized?} oE -->|Yes| oF[Process and Respond] oE -->|No| oG[Reject Request] end oF --> oH[Forward to Request Handler] H["Receive Request Headers<br>(Updated)"] --> I[HeaderVerifier] subgraph nsp[Security Plugin] I --> J{Authorized?} J -->|Yes| K[Decompress Request] J -->|No| N[Save Early Response] end K --> L[RestHandler] N --> L L --> M[Intercept Request] subgraph n2sp[Security Plugin] M --> n2D["Check Authorization<br>(Cached)"] n2D --> nE{Authorized?} nE -->|Yes| nF[Process and Respond] nE -->|No| nG[Reject Request] end nF --> nH[Forward to Request Handler] class oA,oB old; class H,I,K,N,n2D new; classDef old fill:#f9d0c4,stroke:#f28b82; classDef new fill:#cfe8fc,stroke:#68a9ef; ``` ### Issues Resolved - Related #3559 ### Check List - [X] New functionality includes testing - [ ] ~New functionality has been documented~ - [X] Commits are signed per the DCO using --signoff 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](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin). --------- Signed-off-by: Peter Nied <petern@amazon.com> Signed-off-by: Craig Perkins <cwperx@amazon.com> Signed-off-by: Craig Perkins <craig5008@gmail.com> Signed-off-by: Peter Nied <peternied@hotmail.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Signed-off-by: Darshit Chanpura <dchanp@amazon.com> Co-authored-by: Craig Perkins <cwperx@amazon.com> Co-authored-by: opensearch-trigger-bot[bot] <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Darshit Chanpura <dchanp@amazon.com>
Description
Modifies
SecurityResponse.asRestResponse
to use the corresponding constructor forBytesRestResponse
if theSecurityResponse
contains the content-type header. This adds a test that fails before the change and demonstrates how using the constructor of BytesRestResponse without content-type defaults to text/plainBug fix
Check List
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.