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

Add test that confirms when Rest and Transport authorization checks overlap #3112

Closed
4 tasks done
Tracked by #2590
peternied opened this issue Aug 7, 2023 · 6 comments
Closed
4 tasks done
Tracked by #2590
Labels
triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.

Comments

@peternied
Copy link
Member

peternied commented Aug 7, 2023

Coming from #2867, want to confirm that REST authorization and Transport authorization are both checked when there is an overlap which is expected during migration scenarios.

Exit Criteria

  • Add a test that confirms when REST is authorized and Transport is unauthorized
  • Add a test that confirms when REST is authorized and Transport is authorized
  • Add a test that confirms when REST is unauthorized and Transport is unauthorized
  • Add a test that confirms when REST is unauthorized and Transport is authorized
@github-actions github-actions bot added the untriaged Require the attention of the repository maintainers and may need to be prioritized label Aug 7, 2023
@DarshitChanpura
Copy link
Member

Add a test that confirms when REST is authorized and Transport is unauthorized

This case was verified via this test. This test checks for legacy vs new implementation.

Add a test that confirms when REST is authorized and Transport is authorized

This case was verified via this test

Add a test that confirms when REST is unauthorized and Transport is unauthorized

Request will never reach Transport Layer, if they are unauthorized at REST layer. This case was verified via this test

Add a test that confirms when REST is unauthorized and Transport is authorized

Request will never reach Transport Layer, if they are unauthorized at REST layer. And so this scenario will never exist, and a test cannot be implemented for this.

Current design suggests that all new named routes have an option to support legacy action names which were originally evaluated at TransportLayer via PrivilegesEvaluator. I'll be more than happy to modify existing tests with any additional details as required.

@peternied
Copy link
Member Author

These scenarios are not correct , the whoami API is implemented as a Rest handler action - not a transport action. We need to be sure that overlapping functionality between different actions is still handled as expected.

@DarshitChanpura
Copy link
Member

the whoami API is implemented as a Rest handler action - not a transport action

There is a corresponding WhoAmI transport action to the rest API. This request does touch the SecurityFilter class, however it is made pass-through at Transport Layer authorization: https://github.com/opensearch-project/security/blob/f299a5f9094597648fbb147e74166d48090df6c9/src/main/java/org/opensearch/security/filter/SecurityFilter.java#L207C24-L207C24.

I'll look into a way to add a test to make this clearer.

We need to be sure that overlapping functionality between different actions is still handled as expected.

Agreed. One thing to note, by design, requests which fail authorization at REST Layer will not reach Transport Layer. The idea behind this is that REST Layer authorization will only come into play in case of NamedRoutes (which support REST authorization). For all other routes (we refer to those as legacy routes), the flow remains as it was prior to REST authz introduction. (Verified via different tests in WhoAmITests file)

@peternied
Copy link
Member Author

Here is some pseudo code of what I think this test could look like, we will need to register some plugins that exist only during the test scenario to go through the registration process and test invariants in this scenario.

public void testOverlappingAuthz() {
   // Setup plugin with transport layer action, named: `Foo`
   // Setup plugin with a rest handler also has legacyName: `Foo`, and has name `NewFoo` calls transport action Foo
   // Setup user1 with role mapping that links to role with permission for `NewFoo`

   // Make request as user1 against rest endpoint for NewFoo
   ... 
   // Setup user with role mapping that links to role with permission for `Foo`   

   // Make request as user1 against rest endpoint for NewFoo
}

@stephen-crawford
Copy link
Contributor

[Triage] Thanks for filing this issue @peternied. At this time, it looks like this issue is actionable. Going to label this as triaged.

@stephen-crawford stephen-crawford added triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. and removed untriaged Require the attention of the repository maintainers and may need to be prioritized labels Aug 14, 2023
@DarshitChanpura
Copy link
Member

DarshitChanpura commented Aug 23, 2023

Closing as all relevant tests will be covered by #3226 under #2928

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.
Projects
None yet
Development

No branches or pull requests

3 participants