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

[BUG] auth/http/saml/Saml2SettingsProvider.java still using _opendistro/ instead of _plugins/ #1941

Closed
kzinas-adv opened this issue Jul 14, 2022 · 6 comments
Assignees
Labels
bug Something isn't working triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.

Comments

@kzinas-adv
Copy link

What is the bug?
A clear and concise description of the bug.
opensearch-project/security-dashboards-plugin in version 2.1.0 changed SAML2 authentication endpoint to _plugins/_security/saml/acs
while opensearch-project/security use old one /_opendistro/_security/saml/acs
This creates clash as users need modify this place by hand to get SAML working.

How can one reproduce the bug?
Steps to reproduce the behavior:
Use opensearch 2.1.0 with SAML

What is the expected behavior?
A clear and concise description of what you expected to happen.
Get opensearch 2.2.0 working out of the box.

security-dashboards-plugin BUG issue:
opensearch-project/security-dashboards-plugin#1031

@kzinas-adv kzinas-adv added bug Something isn't working untriaged Require the attention of the repository maintainers and may need to be prioritized labels Jul 14, 2022
@kzinas-adv
Copy link
Author

Would it be enough such patch?

diff --git a/src/main/java/com/amazon/dlic/auth/http/saml/Saml2SettingsProvider.java b/src/main/java/com/amazon/dlic/auth/http/saml/Saml2SettingsProvider.java
index 52745695..3bd0b7cc 100644
--- a/src/main/java/com/amazon/dlic/auth/http/saml/Saml2SettingsProvider.java
+++ b/src/main/java/com/amazon/dlic/auth/http/saml/Saml2SettingsProvider.java
@@ -214,9 +214,9 @@ public class Saml2SettingsProvider {
     private String buildAssertionConsumerEndpoint(String dashboardsRoot) {
 
         if (dashboardsRoot.endsWith("/")) {
-            return dashboardsRoot + "_opendistro/_security/saml/acs";
+            return dashboardsRoot + "_plugins/_security/saml/acs";
         } else {
-            return dashboardsRoot + "/_opendistro/_security/saml/acs";
+            return dashboardsRoot + "/_plugins/_security/saml/acs";
         }
     }

@kzinas-adv kzinas-adv changed the title [BUG] auth/http/saml/Saml2SettingsProvider.java still using _opendistro instead of _plugin/ [BUG] auth/http/saml/Saml2SettingsProvider.java still using _opendistro/ instead of _plugins/ Jul 14, 2022
@kzinas-adv
Copy link
Author

kzinas-adv commented Jul 18, 2022

Will be fixed once PR #1936 will be commited

@kzinas-adv kzinas-adv reopened this Jul 18, 2022
@cliu123 cliu123 removed the untriaged Require the attention of the repository maintainers and may need to be prioritized label Jul 19, 2022
@davidlago davidlago added triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. WIP labels Oct 10, 2022
@peternied peternied removed the WIP label Jan 30, 2023
@peternied peternied added the untriaged Require the attention of the repository maintainers and may need to be prioritized label Jul 31, 2023
@peternied
Copy link
Member

Lets put some more attention back onto this issue during the triage meeting. All the history boils down to we need to make this change backwards compatible with both paths and when it was originally rolled out it wasn't compatible

@stephen-crawford
Copy link
Contributor

[Triage] This issue should be reviewed again since it is not currently backwards compatible. Let's have someone pick up the issue to become BWC.

@stephen-crawford stephen-crawford removed the untriaged Require the attention of the repository maintainers and may need to be prioritized label Jul 31, 2023
@RyanL1997 RyanL1997 self-assigned this Aug 25, 2023
@peternied
Copy link
Member

@RyanL1997 You might want to look into multiple endpoints, seems like it is supported in SAML 2.0 [1] [2].

@RyanL1997
Copy link
Collaborator

Update on Aug/30/2023

Transferring the internal discussion result here: security team decided to deprecate all the legacy prefix thats related to SAML usages all at once at the next major version. Plz use the issue (#3271) for better tracking.

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

No branches or pull requests

6 participants