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] v2.1 security plugin is still using _opendistro/_security/saml/acs #1031

Open
hpkuppuraj opened this issue Jul 13, 2022 · 11 comments
Open
Labels
bug Something isn't working triaged v3.0.0

Comments

@hpkuppuraj
Copy link

I am setting up v2.1 cluster with saml authentication, all the configuration seems to be correct however when i try to login and found out that saml authrequest still builds ACS URL with _opendistro/_security/saml/acs instead of /_plugin/_security/saml/acs.
Below is what we extracted from the saml tracer plugin.

<samlp:AuthnRequest xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol"
                    xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion"
                    ID="ONELOGIN_XYZ"
                    Version="2.0"
                    IssueInstant="2022-07-13T08:55:10Z"
                    Destination="https://domain/trust/saml2/http-redirect/sso/<uuid>"
                    ProtocolBinding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST"
                    AssertionConsumerServiceURL="https://domain/_opendistro/_security/saml/acs"
                    >
    <saml:Issuer>opensearch-saml</saml:Issuer>
    <samlp:NameIDPolicy Format="urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified"
                        AllowCreate="true"
                        />
</samlp:AuthnRequest>
@agabrys
Copy link

agabrys commented Jul 13, 2022

I think that is the cause:
https://github.com/opensearch-project/security/blob/2.1.0.0/src/main/java/com/amazon/dlic/auth/http/saml/Saml2SettingsProvider.java#L214-L221

    private String buildAssertionConsumerEndpoint(String dashboardsRoot) {

        if (dashboardsRoot.endsWith("/")) {
            return dashboardsRoot + "_opendistro/_security/saml/acs";
        } else {
            return dashboardsRoot + "/_opendistro/_security/saml/acs";
        }
    }

@hpkuppuraj
Copy link
Author

Thanks @agabrys , now i see where it is being set. Will there be any fix soon?

@agabrys
Copy link

agabrys commented Jul 13, 2022

I'm not a mantainer 🙂 I've just checked what could be the cause, but I don't have time to work on it. Feel free to create a PR 🙂

@peternied
Copy link
Member

Thanks for filing @opensearch-project/security is looking into the path forward with our upcoming releases.

@hpkuppuraj @agabrys if you are interested in become a maintainer it's open to everyone.

@cliu123 cliu123 removed the untriaged label Jul 19, 2022
@cliu123
Copy link
Member

cliu123 commented Jul 19, 2022

The PR in security plugin is needed to resolve this issue: opensearch-project/security#1936.

@cliu123
Copy link
Member

cliu123 commented Aug 9, 2022

The endpoints need to be updated in both security plugin and security dashboards plugin in 3.0.0 as a breaking change as it would break existing SAML setup. labeling for 3.0.0.

@dparker18
Copy link

This is already a breaking change in 2.x. Currently there is no way to use SAML as far as I can tell. Is there a recommended workaround until 3.0.0 comes out?

@kzinas-adv
Copy link

This is already a breaking change in 2.x. Currently there is no way to use SAML as far as I can tell. Is there a recommended workaround until 3.0.0 comes out?

Use _opendistro for SAML endpoint (https://example.com/_opendistro/_security/saml/acs)
and

sed -i 's/_plugins/_opendistro/g' /usr/share/opensearch-dashboards/plugins/securityDashboards/server/auth/types/saml/routes.js

@cwperks
Copy link
Member

cwperks commented Aug 15, 2023

This should be updated from _opendistro/_security/saml/acs instead of /_plugin/_security/saml/acs in both the security repo and this repo in the same release to have this fully working. I believe this is the last instance of /_opendistro/... only endpoints in the security plugin[citation needed].

For a little history on this issue, a PR was merged in security-dashboards-plugin prior to 2.1 without the corresponding PR in the security repo. Since only half was released, the setup would never work because the frontend would try to call the assertion consumer service endpoint at /_plugin/_security/saml/acs and the endpoint didn't exist because the endpoint was still /_opendistro/_security/saml/acs

We should eliminate legacy language from the codebase, but this will be a breaking change since these endpoints need to be added to opensearch_dashboards.yml config file as pointed out in the documentation here: https://opensearch.org/docs/latest/security/authentication-backends/saml/#opensearch-dashboards-configuration

Dashboards installations that have the config: server.xsrf.allowlist: ["/_opendistro/_security/saml/acs", "/_opendistro/_security/saml/logout"] will need to migrate to server.xsrf.allowlist: ["/_plugins/_security/saml/acs", "/_plugins/_security/saml/logout"]

@peternied
Copy link
Member

@cwperks thanks for spending the time to pull in the details, could you update the description to have clear exit criteria or should we create other issues? Based on your recommendation its more complex than just changing out the url which is how the description reads.

@cwperks
Copy link
Member

cwperks commented Aug 15, 2023

@peternied I think the issue is actionable from the description on this issue but there should be a parent tracking issue to associate the one in this repo, the one in the security repo and a documentation issue to explain how to migrate to the new endpoint in the next major version.

@davidlago davidlago removed the WIP label Aug 16, 2023
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 v3.0.0
Projects
None yet
Development

No branches or pull requests

8 participants