Update SAML Endpoint #1936

hpkuppuraj · 2022-07-13T10:08:06Z

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/"
ProtocolBinding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST"
AssertionConsumerServiceURL="https://domain/_opendistro/_security/saml/acs"
>
saml:Issueropensearch-saml</saml:Issuer>
<samlp:NameIDPolicy Format="urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified"
AllowCreate="true"
/>
</samlp:AuthnRequest>

Seems like after version 2.1.0 both enpoints: /_opendistro/_security/saml/acs and /_plugins/_security/saml/acs not working.
If using first-one as IDP acs URL getting: {"statusCode":404,"error":"Not Found","message":"Not Found"} - probbably expected after opensearch-project/security-dashboards-plugin#895
But if switching to 2nd one /_plugins/_security/saml/acs getting {"statusCode":500,"error":"Internal Server Error","message":"Internal Error"}

Rolling back Kibana to version 2.0.1 but leaving Opensearch version: 2.1.0 helps the issue a bit as /_opendistro/_security/saml/acs starts to work again

Description

[Describe what this change achieves]

Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)
Bug Fix
Why these changes are required?
Login Dashboard v2.1 with SAML authentication
What is the old behavior before changes and new behavior after changes?
Old behaviour doesnt allow dashboard v2.1 to login with saml authentication

Issues Resolved

[List any issues this PR will resolve]

Is this a backport? If so, please add backport PR # and/or commits #

Testing

[Please provide details of testing done: unit testing, integration testing and manual testing]

Check List

New functionality includes testing
New functionality has been documented
Commits are signed per the DCO using --signoff

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.

peternied · 2022-07-13T17:04:54Z

@hpkuppuraj Thanks for this pull request, could you please see about fixing the DCO errors, link?

peternied · 2022-07-13T17:07:19Z

Note; I said that this changed was set against the 2.1 branch, lets merge this against main and then the maintainers will backport the change to the next release version

cliu123 · 2022-07-13T17:56:15Z

@hpkuppuraj Thank you so much for the finding and the PR!
This would be a breaking change with changing things like ACS URL even though it would align the behavior with the documentation. If an old version cluster without this change gets upgraded to a new version with this change, the SAML authN would be broken. So IMHO, this will be a great change for 3.0.0 release, and the documentation should be fixed instead till 3.0.0.

hpkuppuraj · 2022-07-14T03:18:44Z

@peternied , Thanks for highlighting the signoff part which i have missed at commits. I think i have signed the commits that I have made. Looks like still DCO error there.

codecov-commenter · 2022-07-14T04:46:27Z

Codecov Report

Merging #1936 (bc2c87f) into main (d9bd0dd) will decrease coverage by 0.01%.
The diff coverage is 50.00%.

@@             Coverage Diff              @@
##               main    #1936      +/-   ##
============================================
- Coverage     61.00%   60.99%   -0.02%     
  Complexity     3233     3233              
============================================
  Files           256      256              
  Lines         18088    18088              
  Branches       3224     3224              
============================================
- Hits          11035    11033       -2     
- Misses         5470     5471       +1     
- Partials       1583     1584       +1

Impacted Files	Coverage Δ
...zon/dlic/auth/http/saml/Saml2SettingsProvider.java	`61.53% <50.00%> (ø)`
...security/auditlog/sink/ExternalOpenSearchSink.java	`59.25% <0.00%> (-2.47%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us.

DarshitChanpura · 2022-07-14T20:23:32Z

@hpkuppuraj I see there are 93 file changes here and some of them are from other PRs. Can you please rebase your branch against main so that the changes you intended to make are the only ones included.

JustinasKO · 2022-07-15T07:45:44Z

Maybe better to make both endpoints working? Doesn't seem to be much work and then it would not be breaking change. and in version 3.0 remove _opendistro one.

peternied · 2022-07-15T17:28:03Z

Maybe better to make both endpoints working? Doesn't seem to be much work and then it would not be breaking change. and in version 3.0 remove _opendistro one.

I would prefer this approach as suggested by @JustinasKO. @hpkuppuraj is this something you'd be able to pick up or would you like to take this on?

Note; there is another change like this in flight - #1949

hpkuppuraj · 2022-07-18T14:38:00Z

Maybe better to make both endpoints working? Doesn't seem to be much work and then it would not be breaking change. and in version 3.0 remove _opendistro one.

I would prefer this approach as suggested by @JustinasKO. @hpkuppuraj is this something you'd be able to pick up or would you like to take this on?

Note; there is another change like this in flight - #1949

Yes sure, let me explore on this and keep both the endpoints working.

cliu123 · 2022-07-19T19:27:14Z

@hpkuppuraj I see there are 93 file changes here and some of them are from other PRs. Can you please rebase your branch against main so that the changes you intended to make are the only ones included.

@hpkuppuraj Would you please follow up with this comment?

hpkuppuraj · 2022-07-21T13:39:45Z

@cliu123 , I am trying to figure how the PR gets the changes from other commits, but my local branch has just my changes. Could you please help on this. Shall i cancel this PR? and raise a new PR

JustinasKO · 2022-07-22T11:50:22Z

@hpkuppuraj just rebase your branch from main branch and only your changes will remain.

hpkuppuraj · 2022-07-23T09:14:57Z

@cliu123 , could you please confirm the changes. I believe now only one file change is shown. Also i am seeing DCO author signature mismatch now

nerijus · 2022-07-23T10:33:14Z

src/main/java/com/amazon/dlic/auth/http/saml/Saml2SettingsProvider.java

@@ -212,11 +212,11 @@ private SingleLogoutService findSingleLogoutService(IDPSSODescriptor idpSsoDescr
    }

    private String buildAssertionConsumerEndpoint(String dashboardsRoot) {
-
+        // Changed to fix bug in 2.1 


There is no need to have such comment in the source file, it belongs to the commit message.

DarshitChanpura · 2022-07-25T15:44:00Z

@cliu123 , could you please confirm the changes. I believe now only one file change is shown. Also i am seeing DCO author signature mismatch now

I can confirm that I see only 1 file change now. You can fix the DCO error by executing this command: git rebase --signoff HEAD~2 where 2 is the number of commits from current HEAD. Looking at your commit history, you've signed-off your latest commit with Signed-off-by: Hari Prasad Kuppuraj <51482940+hpkuppuraj@users.noreply.github.com> which is not correct. It should be done with your Github user (which is, I believe, what you did in the commit prior to the latest one).

hpkuppuraj · 2022-07-26T13:10:17Z

@DarshitChanpura , i dont have access to the account(company) i used to commit the first change, hence switched to the personal github account. Hence there is a discrepancy in the signature.

The commit that impacts is 4c432a4 which is not the 2 commit and signoff using different email id. With the rebase signoff to this commit i am not able to signoff with current github account.

Updated the ACS endpoint builder code components. [BUG] v2.1 security plugin is still using _opendistro/_security/saml/acs opensearch-project#1031 Signed-off-by: hari prasad <prasad.hari@lotuss.com> Signed-off-by: Peter Nied <petern@amazon.com>

peternied · 2022-07-26T16:46:57Z

@hpkuppuraj Thanks for your hard work on this pull request, I've cleaned up the commit history and fixed the DCO issues and pushed an update onto your fork, as soon as CI passes this can be merged.

cliu123

Thanks for the PR!
I have a few questions here:

Should this PR be closed if merging this one?
How was this change tested? Please provide information about testing in the PR description.
With this change, how would the existing clusters built with the old
ACS endpoint(/_opendistro/_security/saml/acs) work after upgrading to the version with this change?

DarshitChanpura · 2022-08-23T02:36:56Z

@hpkuppuraj Can you please update on the requested changes? Once they are addressed we can go ahead and merge this PR. Once again, thank you for your contribution!

peternied · 2022-09-06T19:38:19Z

@cliu123 Can you shepherd this change along with the frontend to make sure everything is in alignment?

DarshitChanpura · 2022-09-28T16:13:08Z

@hpkuppuraj Could you please address @cliu123 's Requested Changes so we can move forward with this PR

hpkuppuraj · 2022-10-11T08:15:43Z

Hi, apologies I am outstation and will be back to workstation tomorrow. Will resolve the code conflicts Get Outlook for iOS<https://aka.ms/o0ukef>

…

________________________________ From: Chang Liu ***@***.***> Sent: Wednesday, July 20, 2022 3:27:25 AM To: opensearch-project/security ***@***.***> Cc: Hari Prasad Kuppuraj ***@***.***>; Mention ***@***.***> Subject: Re: [opensearch-project/security] Update SAML Endpoint (PR #1936) @hpkuppuraj<https://github.com/hpkuppuraj> I see there are 93 file changes here and some of them are from other PRs. Can you please rebase your branch against main so that the changes you intended to make are the only ones included. @hpkuppuraj<https://github.com/hpkuppuraj> Would you please follow up with this comment? — Reply to this email directly, view it on GitHub<#1936 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AMIZCPFUOUXPR7PFDBOMYY3VU36R3ANCNFSM53OEUBPA>. You are receiving this because you were mentioned.Message ID: ***@***.***>

peternied · 2022-10-19T18:46:00Z

@opensearch-project/security this is a breaking change we've been considering for a while how do you feel about us taking it? I'm leaning towards no, while it would be nice to have consistent URLs, I would prefer we don't break folks in a hard to detect way.

Please vote 👍 👎 or comment on this issue, lets revisit this during our next triage.

peternied · 2022-10-25T14:04:19Z

I'm closing this pull request out as we aren't taking steps to get this merged. Thanks again for the contribution sorry it didn't work out on this change.

nerijus · 2022-10-25T14:11:21Z

Pity. Then opensearch-dashboards should be patched, because now we have to use every time we update it:

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

peternied · 2022-10-25T14:15:53Z

@nerijus from your comment it sounds SAML is broken in security-dashboards-plugin without this change? Can you create an issue

nerijus · 2022-10-25T14:22:32Z

Of course it is broken, that's why this PR was created. I'd still suggest applying this PR instead of changing dashboards.

Edit: I see it is no longer broken in opensearch-dashboards 2.3.0.

peternied · 2022-10-25T17:22:37Z

@cliu123 Could you shepherd this PR and determine how to follow up? This seems very bad (tm) and I'm out of the loop on the state of the SAML configuration in dashboards.

aoguan1990 · 2022-10-25T17:54:35Z

@cliu123 As per discussion, Endpoint: /_opendistro/_security/saml/acs is also used by Dashboards Security Plugin. Endpoint changed should be planed for both front-end and back-end plugin together.

cliu123 · 2022-10-25T18:20:33Z

This is a documentation issue that I'm creating. Users follow the documentation to configure _plugins/_security/saml/acs in IDP, but in plugins, it still uses /_opendistro/_security/saml/acs.

cwperks · 2022-10-25T18:31:53Z

@cliu123 The documentation website was reverted to reflect this: opensearch-project/documentation-website#877

There was a breaking change to SAML introduced in 2.2 that was reverted in 2.3 with this PR: opensearch-project/security-dashboards-plugin#895

The frontend switched to using /_plugins while this backend change was not applied.

Security dashboards plugin reverted to using /_opendistro/_security/saml/acs for the 2.x line, but it was my understanding that this change is planned for 3.0.0. Now that the main branch is building 3.0.0, what is preventing the merging of this change?

hpkuppuraj requested a review from a team July 13, 2022 10:08

peternied changed the title ~~2.1~~ Update SAML Endpoint Jul 13, 2022

peternied changed the base branch from 2.1 to main July 13, 2022 17:06

hpkuppuraj force-pushed the 2.1 branch from 1ee5a74 to 3251bc7 Compare July 14, 2022 02:21

kzinas-adv mentioned this pull request Jul 18, 2022

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

Closed

DarshitChanpura mentioned this pull request Jul 18, 2022

Revert "Replace _opendistro route with _plugins" opensearch-project/security-dashboards-plugin#1035

Merged

cliu123 mentioned this pull request Jul 19, 2022

[BUG] v2.1 security plugin is still using _opendistro/_security/saml/acs opensearch-project/security-dashboards-plugin#1031

Open

DarshitChanpura mentioned this pull request Jul 20, 2022

Revert "Revert "Replace _opendistro route with _plugins"" opensearch-project/security-dashboards-plugin#1038

Closed

nerijus reviewed Jul 23, 2022

View reviewed changes

peternied force-pushed the 2.1 branch from 2e9148c to bc2c87f Compare July 26, 2022 16:45

peternied approved these changes Jul 26, 2022

View reviewed changes

DarshitChanpura approved these changes Jul 27, 2022

View reviewed changes

cliu123 requested changes Jul 27, 2022

View reviewed changes

cwperks mentioned this pull request Aug 30, 2022

SAML auth uses legacy _opendistro route #2060

Open

peternied added the v3.0.0 label Sep 6, 2022

gdiazlo mentioned this pull request Sep 27, 2022

SAML logout/session renewal bugs wazuh/wazuh-dashboard-plugins#4595

Closed

2 tasks

peternied added the untriaged Require the attention of the repository maintainers and may need to be prioritized label Oct 19, 2022

peternied closed this Oct 25, 2022

gdiazlo mentioned this pull request Oct 31, 2022

SAML logout error "not found" wazuh/wazuh-dashboard-plugins#4779

Closed

1 task

Desvelao mentioned this pull request Apr 24, 2023

Azure Active Directory SSO documentation wrong paths wazuh/wazuh-documentation#6066

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update SAML Endpoint #1936

Update SAML Endpoint #1936

hpkuppuraj commented Jul 13, 2022 •

edited

Loading

peternied commented Jul 13, 2022

peternied commented Jul 13, 2022

cliu123 commented Jul 13, 2022 •

edited

Loading

hpkuppuraj commented Jul 14, 2022

codecov-commenter commented Jul 14, 2022 •

edited

Loading

DarshitChanpura commented Jul 14, 2022 •

edited

Loading

JustinasKO commented Jul 15, 2022

peternied commented Jul 15, 2022

hpkuppuraj commented Jul 18, 2022

cliu123 commented Jul 19, 2022

hpkuppuraj commented Jul 21, 2022 •

edited

Loading

JustinasKO commented Jul 22, 2022 •

edited

Loading

hpkuppuraj commented Jul 23, 2022

nerijus Jul 23, 2022

DarshitChanpura Jul 25, 2022

DarshitChanpura commented Jul 25, 2022

hpkuppuraj commented Jul 26, 2022 •

edited

Loading

peternied commented Jul 26, 2022

cliu123 left a comment •

edited

Loading

DarshitChanpura commented Aug 23, 2022 •

edited

Loading

peternied commented Sep 6, 2022

DarshitChanpura commented Sep 28, 2022

hpkuppuraj commented Oct 11, 2022 via email

peternied commented Oct 19, 2022

peternied commented Oct 25, 2022

nerijus commented Oct 25, 2022

peternied commented Oct 25, 2022

nerijus commented Oct 25, 2022 •

edited

Loading

peternied commented Oct 25, 2022

aoguan1990 commented Oct 25, 2022 •

edited

Loading

cliu123 commented Oct 25, 2022

cwperks commented Oct 25, 2022 •

edited

Loading

Update SAML Endpoint #1936

Update SAML Endpoint #1936

Conversation

hpkuppuraj commented Jul 13, 2022 • edited Loading

Description

Issues Resolved

Testing

Check List

peternied commented Jul 13, 2022

peternied commented Jul 13, 2022

cliu123 commented Jul 13, 2022 • edited Loading

hpkuppuraj commented Jul 14, 2022

codecov-commenter commented Jul 14, 2022 • edited Loading

Codecov Report

DarshitChanpura commented Jul 14, 2022 • edited Loading

JustinasKO commented Jul 15, 2022

peternied commented Jul 15, 2022

hpkuppuraj commented Jul 18, 2022

cliu123 commented Jul 19, 2022

hpkuppuraj commented Jul 21, 2022 • edited Loading

JustinasKO commented Jul 22, 2022 • edited Loading

hpkuppuraj commented Jul 23, 2022

nerijus Jul 23, 2022

Choose a reason for hiding this comment

DarshitChanpura Jul 25, 2022

Choose a reason for hiding this comment

DarshitChanpura commented Jul 25, 2022

hpkuppuraj commented Jul 26, 2022 • edited Loading

peternied commented Jul 26, 2022

cliu123 left a comment • edited Loading

Choose a reason for hiding this comment

DarshitChanpura commented Aug 23, 2022 • edited Loading

peternied commented Sep 6, 2022

DarshitChanpura commented Sep 28, 2022

hpkuppuraj commented Oct 11, 2022 via email

peternied commented Oct 19, 2022

peternied commented Oct 25, 2022

nerijus commented Oct 25, 2022

peternied commented Oct 25, 2022

nerijus commented Oct 25, 2022 • edited Loading

peternied commented Oct 25, 2022

aoguan1990 commented Oct 25, 2022 • edited Loading

cliu123 commented Oct 25, 2022

cwperks commented Oct 25, 2022 • edited Loading

hpkuppuraj commented Jul 13, 2022 •

edited

Loading

cliu123 commented Jul 13, 2022 •

edited

Loading

codecov-commenter commented Jul 14, 2022 •

edited

Loading

DarshitChanpura commented Jul 14, 2022 •

edited

Loading

hpkuppuraj commented Jul 21, 2022 •

edited

Loading

JustinasKO commented Jul 22, 2022 •

edited

Loading

hpkuppuraj commented Jul 26, 2022 •

edited

Loading

cliu123 left a comment •

edited

Loading

DarshitChanpura commented Aug 23, 2022 •

edited

Loading

nerijus commented Oct 25, 2022 •

edited

Loading

aoguan1990 commented Oct 25, 2022 •

edited

Loading

cwperks commented Oct 25, 2022 •

edited

Loading