Add early rejection from RestHandler for unauthorized requests#3418

Merged

peternied merged 120 commits intoopensearch-project:mainfrom

cwperks:improve-compressed-handling

Oct 6, 2023

Member

cwperks commented Sep 27, 2023 •

edited by peternied

Loading

Description

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.

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

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.

peternied and others added 2 commits

September 27, 2023 21:54


          [Draft] Resource usage case test

6286f7f

Execute with `./gradlew integrationTest --tests \
  org.opensearch.security.ResourceFocusedTests -i`

Test case output in
build/reports/tests/integrationTest/classes/org.opensearch.security.ResourceFocusedTests.html
is best for reviewing the memory utilization stats.

Signed-off-by: Peter Nied <petern@amazon.com>


          Use DelegatingRestHandler to ensure that the original handler's imple…

ef660b1

…mentations are delegated to

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

cwperks changed the title ~~Optionally decompress request~~ Improve compressed request handling

cwperks mentioned this pull request

Allow customization of netty channel handles before and during decompression opensearch-project/OpenSearch#10261

Merged

6 tasks

cwperks and others added 17 commits

September 27, 2023 22:18


          Switch to create

cf327b5

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


          Remove Sharable

a3bd919

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


          Move Conditional Decompressor to core

b1343e1

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


          Removed http channel

ab693c6

Signed-off-by: Peter Nied <petern@amazon.com>


          Add request source from the netty channel

6abd939

Signed-off-by: Peter Nied <petern@amazon.com>


          Plumb SecurityRequest into HTTPAuthenticator interface

da8d0cb

Signed-off-by: Peter Nied <petern@amazon.com>


          Fix all tests

26c6b83

Signed-off-by: Peter Nied <petern@amazon.com>


          Remove DelegatingRestHandler changes for now

7f6b364

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


          Prevent build break for generalized testing associated with SecurityR…

a825d83

…equest

Signed-off-by: Peter Nied <petern@amazon.com>


          Spotless

095d4e5

Signed-off-by: Peter Nied <petern@amazon.com>


          Disable createHeaderVerifier so builds can pass

6580e7f

Signed-off-by: Peter Nied <petern@amazon.com>


          Remove duplicated code

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


          Implement SecurityRequest gaps

8c44278

Signed-off-by: Peter Nied <petern@amazon.com>


          Fix spotless

9b5fe42

Signed-off-by: Peter Nied <petern@amazon.com>


          Redo how reRequestAuthentication works

9f369cb

Signed-off-by: Peter Nied <petern@amazon.com>


          Nix that idea

46358a2

Signed-off-by: Peter Nied <petern@amazon.com>


          Cleaning up some

fa98ba9

Signed-off-by: Peter Nied <petern@amazon.com>

peternied mentioned this pull request

[Feature] Improve compressed request handling opensearch-project/OpenSearch#10260

Closed

cwperks and others added 8 commits

October 2, 2023 11:14


          Run CI with respective branch in core

98c53ad

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


          Run on push

d9e5f4f

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


          Correct branch

eb33281

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


          Mild switch to RequestChannel

9844df9

Signed-off-by: Peter Nied <petern@amazon.com>


          Same step

ee54fbb

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


          Remove test

37a251b

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


          Remove should not happen

3b2925b

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


          Handle unsupported scenarios

cafe1c7

Signed-off-by: Peter Nied <petern@amazon.com>


          Fix spotless issues

308645c

Signed-off-by: Peter Nied <petern@amazon.com>

peternied approved these changes

View reviewed changes

DarshitChanpura approved these changes

View reviewed changes

Member

DarshitChanpura left a comment

LGTM. Left some non-blocking questions.

src/main/java/org/opensearch/security/ssl/http/netty/Netty4HttpRequestHeaderVerifier.java Show resolved Hide resolved

src/main/java/org/opensearch/security/ssl/http/netty/Netty4ConditionalDecompressor.java Show resolved Hide resolved

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

RyanL1997 approved these changes

View reviewed changes

Collaborator

RyanL1997 left a comment

Approving, since I had some context from #3430. I just left a minor comment for my knowledge, and we can address that separately.

src/main/java/org/opensearch/security/ssl/http/netty/Netty4HttpRequestHeaderVerifier.java Show resolved Hide resolved

DarshitChanpura added backport 2.x backport 2.11 labels

peternied merged commit 6b0b682 into opensearch-project:main

Contributor

opensearch-trigger-bot bot commented Oct 6, 2023

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-3418-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 6b0b682da2cb0feabb54489731472a05af1d4b1b
# Push it to GitHub
git push --set-upstream origin backport/backport-3418-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-3418-to-2.x.

opensearch-trigger-bot bot added the backport-failed label

Contributor

opensearch-trigger-bot bot commented Oct 6, 2023

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-3418-to-2.11
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 6b0b682da2cb0feabb54489731472a05af1d4b1b
# Push it to GitHub
git push --set-upstream origin backport/backport-3418-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-3418-to-2.11.

Member

peternied commented Oct 6, 2023

Working on the 2.11 backport...

peternied added a commit to peternied/security that referenced this pull request


          [Backport 2.11] Add early rejection from RestHandler for unauthorized…

e7a7c6a

… 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 mentioned this pull request

[Backport 2.11] Add early rejection from RestHandler for unauthorized requests (#3418) #3495

Merged

3 tasks

peternied added a commit that referenced this pull request


          [Backport 2.11] Add early rejection from RestHandler for unauthorized…

f7c47af

… 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 that referenced this pull request


          [Backport 2.11] Add early rejection from RestHandler for unauthorized…

c312ad2

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

cwperks mentioned this pull request

[Testing] Test Netty4HttpHeaderVerifier with different types of persistent connections #3507

Open

3 tasks

stephen-crawford pushed a commit that referenced this pull request


          [Backport 2.x] Add early rejection from RestHandler for unauthorized …

0de30e2

…requests (#3418) (#3496)

Backport f7c47af from #3495

---------

Signed-off-by: Peter Nied <petern@amazon.com>
Co-authored-by: Peter Nied <petern@amazon.com>

peternied added a commit to peternied/security that referenced this pull request


          [Backport 2.x] Add early rejection from RestHandler for unauthorized …

8b74d6f

…requests (opensearch-project#3418) (opensearch-project#3496)

Backport f7c47af from opensearch-project#3495

---------

Signed-off-by: Peter Nied <petern@amazon.com>
Co-authored-by: Peter Nied <petern@amazon.com>

cwperks mentioned this pull request

Add the means to extract the contextual properties from HttpChannel, TcpChannel and TrasportChannel without excessive typecasting opensearch-project/OpenSearch#10562

Merged

7 tasks

This was referenced Oct 12, 2023

[Draft] Resource usage case test #3417

Closed

[Backport 1.3] Add early rejection from RestHandler for unauthorized requests #3559

Closed

cwperks mentioned this pull request

[Regression] Request decompression was broken in 2.11 opensearch-project/OpenSearch#10802

Closed

peternied added a commit to peternied/security that referenced this pull request


          [Backport 1.3] Add early rejection from RestHandler for unauthorized …

f12a66b

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

peternied mentioned this pull request

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

Merged

3 tasks

willyborankin pushed a commit that referenced this pull request


          [Backport 1.3] Add early rejection from RestHandler for unauthorized …

c608fae

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

This was referenced Jan 23, 2024

Fix unconsumed parameter exception when authenticating with jwtUrlParameter #3975

Merged

[BUG] Unconsumed parameter exception when authenticating with jwtUrlParameter #3977

Closed

DarshitChanpura removed the backport-failed label

cwperks mentioned this pull request

Ensure support of the transport-nio by security plugin (HTTP) opensearch-project/OpenSearch#16474

Merged

3 tasks

nibix mentioned this pull request

[BUG] - Rare and random unexpected 401 with correct credentials #5544

Open

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

willyborankin willyborankin left review comments

DarshitChanpura DarshitChanpura approved these changes

RyanL1997 RyanL1997 approved these changes

cliu123 Awaiting requested review from cliu123

davidlago Awaiting requested review from davidlago

stephen-crawford Awaiting requested review from stephen-crawford

reta Awaiting requested review from reta reta is a code owner

+1 more reviewer

peternied peternied approved these changes

Labels