-
Notifications
You must be signed in to change notification settings - Fork 159
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
[Enhancement] Add support for SAML ACS url with new prefix of _plugin
#1565
[Enhancement] Add support for SAML ACS url with new prefix of _plugin
#1565
Conversation
Signed-off-by: Ryan Liang <[email protected]>
Signed-off-by: Ryan Liang <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #1565 +/- ##
=======================================
Coverage 66.18% 66.18%
=======================================
Files 93 93
Lines 2339 2339
Branches 312 312
=======================================
Hits 1548 1548
Misses 722 722
Partials 69 69 |
@RyanL1997 is it possible to make it so that a user does not have to migrate config? i.e.
|
Update for the mitigation concern:How SAML Works:graph TD;
A[User] -->|Access Dashboard| B[OpenSearch Dashboard]
B -->|Redirect to IdP| C[Identity Provider]
C -->|Authenticate User| C
C -->|Generate SAML Assertion| D[ACS URL in OpenSearch Dashboard]
D -->|Validate SAML Assertion| E[OpenSearch Backend - security plugin]
E -->|Validate/Map User| B
B -->|Authenticated Session| A
Setting Up ACS URL ProblemAccording to the above description of setting up ACS URL, the problem we are facing is that the library we are using - SAML Java Toolkit, even with the latest version (2.9.0) is not supporting setup multiple ACS urls. Circling back to our fix, if we apply this change - opensearch-project/security#3246 into the security backend, it will only tell the IDP to use the url with the Solution1. Option 1: Switch library - switch to a library that support multiple ACS config
2. Option 2: As @cwperks mentioned above, we deprecate the legacy ACS url but with a work around on the configuration for legacy users. (Recommended)
|
_plugin
@RyanL1997 When you've got some cycles please create a PR to add ^^^ to https://github.com/opensearch-project/security/blob/main/ARCHITECTURE.md |
Nice call out. I will do that. |
Option 2- Configuration approach works, and if we choose to choose this approach, we need to merge the PR in following orders: |
Closing this according to opensearch-project/security#3271 |
Description
Enhance the router for saml endpoint to support both legacy (_opendistro) and new prefix (_plugin).
This PR is registering 2 new endpoints with
_plugin
prefix for handling saml acs + saml idp initiated acs. Compare to the legacy endpoints, the logic is the same, and it has been extracted out for better usage.Mitigation Concern
After merging this, we can kick off the replacement of the endpoint generation in backend codebase (source code). If we merge opensearch-project/security#3246, the new acs endpoint with
_plugin
prefix will be applied, however, the user with the legacy set up will need to add these endpoints into the allowlist for theiropensearch_dashboards.yml
. Here is a comparison:Category
[Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation]
Enhancement
Issues Resolved
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.