security-dashboards-plugin icon indicating copy to clipboard operation
security-dashboards-plugin copied to clipboard

[BUG] v2.1 security plugin is still using _opendistro/_security/saml/acs

Open hpkuppuraj opened this issue 2 years ago • 11 comments

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>

hpkuppuraj avatar Jul 13 '22 09:07 hpkuppuraj

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";
        }
    }

agabrys avatar Jul 13 '22 09:07 agabrys

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

hpkuppuraj avatar Jul 13 '22 09:07 hpkuppuraj

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 🙂

agabrys avatar Jul 13 '22 09:07 agabrys

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.

peternied avatar Jul 15 '22 17:07 peternied

The PR in security plugin is needed to resolve this issue: https://github.com/opensearch-project/security/pull/1936.

cliu123 avatar Jul 19 '22 19:07 cliu123

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.

cliu123 avatar Aug 09 '22 17:08 cliu123

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?

dparker18 avatar Sep 15 '22 19:09 dparker18

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

kzinas-adv avatar Sep 18 '22 07:09 kzinas-adv

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"]

cwperks avatar Aug 15 '23 17:08 cwperks

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

peternied avatar Aug 15 '23 18:08 peternied

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

cwperks avatar Aug 15 '23 20:08 cwperks