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

Fix unconsumed parameter exception when authenticating with jwtUrlParameter #3975

Merged
merged 10 commits into from
Feb 22, 2024

Conversation

cwperks
Copy link
Member

@cwperks cwperks commented Jan 23, 2024

Description

Discovered this bug when analyzing #3949. There is a regression in authentication with the jwtUrlParameter where it fails to consume the parameter and responds with illegal_argument_exception: contains unrecognized parameter:

Suite: Test class org.opensearch.security.http.JwtAuthenticationTests
  2> java.lang.AssertionError: Expected status code is '200', but was '400'. Response body '{"error":{"root_cause":[{"type":"illegal_argument_exception","reason":"request [/_opendistro/_security/authinfo] contains unrecognized parameter: [token]"}],"type":"illegal_argument_exception","reason":"request [/_opendistro/_security/authinfo] contains unrecognized parameter: [token]"},"status":400}'.
    Expected: <200>
         but: was <400>
        at __randomizedtesting.SeedInfo.seed([100EFB1C515D00AA:4401D6965CA5081E]:0)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at org.opensearch.test.framework.cluster.TestRestClient$HttpResponse.assertStatusCode(TestRestClient.java:422)
        at org.opensearch.security.http.JwtAuthenticationTests.shouldAuthenticateWithJwtTokenInUrl_positive(JwtAuthenticationTests.java:163)

The regression was introduced with the HeaderVerifier changes. With the HeaderVerifier, the HTTP Authenticators are authenticating with a lower-level request object, a NettyRequest. After the request is authenticated, it is then handled by the SecurityRestFilter using the higher-level OpenSearchRequest. The SecurityRestFilter is where authentication was previously performed before the HeaderVerifier changes. The JWT Authenticator "consumes" the jwtUrlParameter, but since its operating on the lower-level request object it is not properly consuming the parameter.

This change takes advantage of the channel attributes to carry the CONSUMED_PARAMS from the header verifier to the security rest filter where they are then consumed on the RestRequest object to prevent the illegal_argument_exception.

  • 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.

@cwperks cwperks changed the title Fix unconsumed parameter exception when authentication with jwtUrlParameter Fix unconsumed parameter exception when authenticating with jwtUrlParameter Jan 23, 2024
Copy link
Member

@DarshitChanpura DarshitChanpura left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ty @cwperks for this contribution! left a small nit to improve readability. +1 to Peter's comment about keeping the interface void of any potential abstraction leakage.

Copy link
Contributor

@stephen-crawford stephen-crawford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outside of the comments from Peter and DC it looks good to me. One place where I wanted to double check we handle an edge case.

Copy link

codecov bot commented Jan 31, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (157d137) 65.56% compared to head (21422b3) 65.87%.
Report is 20 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3975      +/-   ##
==========================================
+ Coverage   65.56%   65.87%   +0.31%     
==========================================
  Files         298      298              
  Lines       21246    21307      +61     
  Branches     3457     3469      +12     
==========================================
+ Hits        13930    14037     +107     
+ Misses       5595     5534      -61     
- Partials     1721     1736      +15     
Files Coverage Δ
...rg/opensearch/security/filter/SecurityRequest.java 100.00% <ø> (ø)
...opensearch/security/filter/SecurityRestFilter.java 70.37% <100.00%> (+1.13%) ⬆️
...rch/security/http/SecurityHttpServerTransport.java 100.00% <100.00%> (ø)
...sl/http/netty/Netty4HttpRequestHeaderVerifier.java 85.71% <100.00%> (+0.34%) ⬆️
...a/org/opensearch/security/filter/NettyRequest.java 91.17% <90.00%> (+3.17%) ⬆️
.../opensearch/security/filter/OpenSearchRequest.java 73.33% <66.66%> (-3.59%) ⬇️

... and 9 files with indirect coverage changes

Copy link
Member

@DarshitChanpura DarshitChanpura left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me @cwperks . Not sure why the codecov checks are failing would you please check that.

cwperks and others added 5 commits February 1, 2024 12:03
@DarshitChanpura DarshitChanpura merged commit ccea744 into opensearch-project:main Feb 22, 2024
82 checks passed
@DarshitChanpura DarshitChanpura added the backport 2.x backport to 2.x branch label Feb 22, 2024
opensearch-trigger-bot bot pushed a commit that referenced this pull request Feb 22, 2024
…ameter (#3975)

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Peter Nied <[email protected]>
Signed-off-by: Peter Nied <[email protected]>
Co-authored-by: Peter Nied <[email protected]>
Co-authored-by: Peter Nied <[email protected]>
(cherry picked from commit ccea744)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
cwperks added a commit that referenced this pull request Feb 23, 2024
… with jwtUrlParameter (#4065)

Backport ccea744 from #3975.

---------

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Peter Nied <[email protected]>
Signed-off-by: Peter Nied <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Peter Nied <[email protected]>
Co-authored-by: Peter Nied <[email protected]>
Co-authored-by: Craig Perkins <[email protected]>
dlin2028 pushed a commit to dlin2028/security that referenced this pull request May 1, 2024
…ameter (opensearch-project#3975)


Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Peter Nied <[email protected]>
Signed-off-by: Peter Nied <[email protected]>
Co-authored-by: Peter Nied <[email protected]>
Co-authored-by: Peter Nied <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants