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

Using the server's basePath when building the SAML ACS #51391

Merged
merged 5 commits into from
Nov 25, 2019

Conversation

kobelb
Copy link
Contributor

@kobelb kobelb commented Nov 21, 2019

Previously, we were using the request's basePath when building the ACS. This would cause the space prefix to be included when building the ACS. This isn't an issue when using xpack.security.authc.saml.realm, only when using the deprecated xpack.security.public.* settings.

This was accidentally and indirectly fixed by #44513 because the even more complicated (but improved) SAML dance redirects the user to /${serverBasePath}/api/security/saml/start before starting the SAML authentication. However, I didn't want to rely on this incase something else changes.

Resolves #51337

"Release Note: Resolving SAML bug when building the ACS when xpack.security.public.* settings are used and a server.basePath are configured"

@kobelb
Copy link
Contributor Author

kobelb commented Nov 21, 2019

@elasticmachine merge upstream

@kobelb kobelb added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:fix labels Nov 21, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@azasypkin azasypkin added Feature:Security/Authentication Platform Security - Authentication Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! and removed v8.0.0 labels Nov 25, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security)

@kobelb
Copy link
Contributor Author

kobelb commented Nov 25, 2019

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💔 Build Failed

@azasypkin
Copy link
Member

azasypkin commented Nov 25, 2019

CI failure: Error: Snapshots for 7.6.0/latest are not available 😬 #51608

@azasypkin
Copy link
Member

ACK: reviewing

Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with green CI (once #51608 is merged). Sorry for introducing this bug and thanks a lot for fixing it!

@azasypkin
Copy link
Member

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@kobelb kobelb merged commit bfa0cb2 into elastic:7.x Nov 25, 2019
@kobelb kobelb deleted the fix/acs-base-path branch November 25, 2019 20:29
kobelb added a commit to kobelb/kibana that referenced this pull request Nov 25, 2019
* Using the server's basePath when building the SAML ACS

* Fixing getACS call
kobelb added a commit that referenced this pull request Nov 25, 2019
* Using the server's basePath when building the SAML ACS

* Fixing getACS call
@kobelb kobelb added release_note:fix and removed release_note:skip Skip the PR/issue when compiling release notes labels Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Security/Authentication Platform Security - Authentication release_note:fix Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.5.1 v7.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants