Use BytesRestResponse constructor with contentType in asRestResponse #3717

cwperks · 2023-11-15T15:12:30Z

Description

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

Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)

Bug fix

Issues Resolved

Check List

New functionality includes testing
New functionality has been documented
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.

Signed-off-by: Craig Perkins <cwperx@amazon.com>

src/main/java/org/opensearch/security/filter/SecurityResponse.java

Signed-off-by: Craig Perkins <cwperx@amazon.com>

…tent-type

Signed-off-by: Craig Perkins <cwperx@amazon.com>

codecov · 2023-11-15T17:14:29Z

Codecov Report

Merging #3717 (bf52697) into main (ee66199) will increase coverage by 1.37%.
Report is 1 commits behind head on main.
The diff coverage is 77.77%.

❗ Current head bf52697 differs from pull request most recent head 538d2eb. Consider uploading reports for the commit 538d2eb to get more accurate results

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

Files	Coverage Δ
.../org/opensearch/security/auth/BackendRegistry.java	`77.92% <100.00%> (+15.38%)`	⬆️
...org/opensearch/security/httpclient/HttpClient.java	`86.13% <ø> (ø)`
...curity/securityconf/impl/AllowlistingSettings.java	`75.00% <100.00%> (ø)`
...curity/securityconf/impl/WhitelistingSettings.java	`75.00% <100.00%> (ø)`
...dlic/auth/http/saml/AuthTokenProcessorHandler.java	`50.28% <50.00%> (ø)`
...opensearch/security/filter/SecurityRestFilter.java	`83.84% <50.00%> (+14.61%)`	⬆️
...g/opensearch/security/filter/SecurityResponse.java	`92.59% <93.33%> (-0.27%)`	⬇️
...ic/auth/http/kerberos/HTTPSpnegoAuthenticator.java	`0.00% <0.00%> (ø)`

... and 35 files with indirect coverage changes

peternied

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?

cwperks · 2023-11-15T18:35:27Z

@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>

Add Security response tests

stephen-crawford · 2023-11-15T21:16:24Z

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

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

clean tests

Signed-off-by: Craig Perkins <cwperx@amazon.com>

stephen-crawford

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

DarshitChanpura

lgtm!

…ests.java Co-authored-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com> Signed-off-by: Craig Perkins <craig5008@gmail.com>

opensearch-trigger-bot · 2023-11-16T15:10:15Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

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 base branch is 2.x and the compare/head branch is backport/backport-3717-to-2.x.

opensearch-trigger-bot · 2023-11-16T15:10:17Z

The backport to 2.11 failed:

The process '/usr/bin/git' failed with exit code 1

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 base branch is 2.11 and the compare/head branch is backport/backport-3717-to-2.11.

…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)

kumjiten · 2023-11-16T14:52:29Z

src/main/java/com/amazon/dlic/auth/http/kerberos/HTTPSpnegoAuthenticator.java

        final String negotiateResponseBody = getNegotiateResponseBody();
        if (negotiateResponseBody != null) {
            responseBody = negotiateResponseBody;
-            headers.putAll(SecurityResponse.CONTENT_TYPE_APP_JSON);
+            contentType = XContentType.JSON.mediaType();


We should understand who populates the header if we explicitly don't populate here?

kumjiten · 2023-11-16T14:55:17Z

src/main/java/org/opensearch/security/filter/SecurityResponse.java

        this.body = body;
+        this.contentType = contentType;


call other constructor here

kumjiten · 2023-11-16T14:56:09Z

src/main/java/org/opensearch/security/filter/SecurityResponse.java

        if (getHeaders() != null) {
-            getHeaders().forEach(restResponse::addHeader);
+            getHeaders().entrySet().forEach(entry -> { entry.getValue().forEach(value -> restResponse.addHeader(entry.getKey(), value)); });


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 (#3717) (#3721) Backport #3717 to 2.11 Signed-off-by: Craig Perkins <cwperx@amazon.com>

…asRestResponse (#3717) (#3722) Backport #3717 to 2.x --------- Signed-off-by: Craig Perkins <cwperx@amazon.com>

… 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>

cwperks added 2 commits November 15, 2023 10:07

Use BytesRestResponse constructor with contentType in asRestResponse

025aa8f

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Check for null

8452abe

Signed-off-by: Craig Perkins <cwperx@amazon.com>

shikharj05 reviewed Nov 15, 2023

View reviewed changes

src/main/java/org/opensearch/security/filter/SecurityResponse.java Outdated Show resolved Hide resolved

cwperks added 4 commits November 15, 2023 11:05

Explicitly pass contentType in SecurityResponse constructor

40770c9

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Merge branch 'return-expected-content-type2' into return-expected-con…

ce089e8

…tent-type

Get from list

49f9bee

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Correct assertion

bf52697

Signed-off-by: Craig Perkins <cwperx@amazon.com>

cwperks marked this pull request as ready for review November 15, 2023 17:04

cwperks requested review from cliu123, DarshitChanpura, davidlago, peternied, RyanL1997, stephen-crawford, reta and willyborankin as code owners November 15, 2023 17:04

davidlago mentioned this pull request Nov 15, 2023

[RELEASE] Release version 2.11.1 opensearch-project/opensearch-build#4161

Closed

70 tasks

peternied reviewed Nov 15, 2023

View reviewed changes

stephen-crawford and others added 3 commits November 15, 2023 16:06

Add Security response tests

5fe7396

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

remove prints

147be07

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

Merge pull request #9 from scrawfor99/returnExpectedType

e95cbe2

Add Security response tests

cwperks commented Nov 15, 2023

View reviewed changes

src/test/java/org/opensearch/security/filter/SecurityResponseTests.java Outdated Show resolved Hide resolved

stephen-crawford and others added 3 commits November 15, 2023 16:20

clean tests

87b1203

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

Merge pull request #10 from scrawfor99/returnExpectedType

b8477eb

clean tests

spotlessApply

5d2a32a

Signed-off-by: Craig Perkins <cwperx@amazon.com>

stephen-crawford previously approved these changes Nov 16, 2023

View reviewed changes

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

DarshitChanpura previously approved these changes Nov 16, 2023

View reviewed changes

cwperks dismissed stale reviews from DarshitChanpura and stephen-crawford via 90255b7 November 16, 2023 14:47

cwperks and others added 2 commits November 16, 2023 09:47

Update src/test/java/org/opensearch/security/filter/SecurityResponseT…

90255b7

…ests.java Co-authored-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com> Signed-off-by: Craig Perkins <craig5008@gmail.com>

Update src/test/java/org/opensearch/security/filter/SecurityResponseT…

538d2eb

…ests.java Co-authored-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com> Signed-off-by: Craig Perkins <craig5008@gmail.com>

stephen-crawford approved these changes Nov 16, 2023

View reviewed changes

cwperks added backport 2.x backport to 2.x branch backport 2.11 Backport to 2.11 branch labels Nov 16, 2023

DarshitChanpura approved these changes Nov 16, 2023

View reviewed changes

cwperks merged commit b7f47b1 into opensearch-project:main Nov 16, 2023
76 of 77 checks passed

opensearch-trigger-bot bot added the backport-failed label Nov 16, 2023

cwperks mentioned this pull request Nov 16, 2023

[Backport 2.11] Use BytesRestResponse constructor with contentType in asRestResponse (#3717) #3721

Merged

kumjiten reviewed Nov 16, 2023

View reviewed changes

cwperks mentioned this pull request Nov 16, 2023

[Backport 2.x] Use BytesRestResponse constructor with contentType in asRestResponse (#3717) #3722

Merged

peternied pushed a commit that referenced this pull request Nov 16, 2023

[Backport 2.11] Use BytesRestResponse constructor with contentType in…

2dab119

… asRestResponse (#3717) (#3721) Backport #3717 to 2.11 Signed-off-by: Craig Perkins <cwperx@amazon.com>

peternied pushed a commit that referenced this pull request Nov 17, 2023

[Backport 2.x] Use BytesRestResponse constructor with contentType in …

b7df32d

…asRestResponse (#3717) (#3722) Backport #3717 to 2.x --------- Signed-off-by: Craig Perkins <cwperx@amazon.com>

peternied removed the backport-failed label Nov 17, 2023

peternied mentioned this pull request Nov 21, 2023

[Backport 1.3] Add early rejection from RestHandler for unauthorized requests (#3418) #3675

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use BytesRestResponse constructor with contentType in asRestResponse #3717

Use BytesRestResponse constructor with contentType in asRestResponse #3717

cwperks commented Nov 15, 2023 •

edited by stephen-crawford

Loading

codecov bot commented Nov 15, 2023 •

edited

Loading

peternied left a comment

cwperks commented Nov 15, 2023

stephen-crawford commented Nov 15, 2023 •

edited

Loading

stephen-crawford left a comment

DarshitChanpura left a comment

opensearch-trigger-bot bot commented Nov 16, 2023

opensearch-trigger-bot bot commented Nov 16, 2023

kumjiten Nov 16, 2023

kumjiten Nov 16, 2023

kumjiten Nov 16, 2023

Use BytesRestResponse constructor with contentType in asRestResponse #3717

Use BytesRestResponse constructor with contentType in asRestResponse #3717

Conversation

cwperks commented Nov 15, 2023 • edited by stephen-crawford Loading

Description

Check List

codecov bot commented Nov 15, 2023 • edited Loading

Codecov Report

peternied left a comment

Choose a reason for hiding this comment

cwperks commented Nov 15, 2023

stephen-crawford commented Nov 15, 2023 • edited Loading

stephen-crawford left a comment

Choose a reason for hiding this comment

DarshitChanpura left a comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Nov 16, 2023

opensearch-trigger-bot bot commented Nov 16, 2023

kumjiten Nov 16, 2023

Choose a reason for hiding this comment

kumjiten Nov 16, 2023

Choose a reason for hiding this comment

kumjiten Nov 16, 2023

Choose a reason for hiding this comment

cwperks commented Nov 15, 2023 •

edited by stephen-crawford

Loading

codecov bot commented Nov 15, 2023 •

edited

Loading

stephen-crawford commented Nov 15, 2023 •

edited

Loading