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

Properly handle very long URL fragments captured during SAML handshake #53478

Closed
azasypkin opened this issue Dec 18, 2019 · 6 comments
Closed
Assignees
Labels
bug Fixes for quality problems that affect the customer experience Feature:Security/Authentication Platform Security - Authentication Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more!

Comments

@azasypkin
Copy link
Member

azasypkin commented Dec 18, 2019

Looks like we've just hit NodeJS limitation on the size of the Request-Line + headers (see #53464). That means we can't pass very long Kibana URLs to the server via query string parameter.

One of the possible solutions is to just switch to POST. Obviously the only benefit here is that we won't hit the limit since this URL won't be preserved anyway (cookie limit is even smaller). Unfortunately it's not super convenient since such custom HTML views don't have access to the core HTTP services (e.g. fetch service that would automatically add necessary xsrf headers).

Alternatively we can just decide on the client side whether URL fragment is already too long and not send it at all (e.g. if it's already bigger than the default limit of 2kb) - that'd be easier and safer. I'm leaning towards this solution.

Any thoughts @elastic/kibana-security?

Blocks: #68885

@azasypkin azasypkin added bug Fixes for quality problems that affect the customer experience discuss Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! Feature:Security/Authentication Platform Security - Authentication labels Dec 18, 2019
@azasypkin azasypkin changed the title Properly handle very URL fragments captured during SAML handshake Properly handle very long URL fragments captured during SAML handshake Dec 18, 2019
@legrego
Copy link
Member

legrego commented Dec 18, 2019

Alternatively we can just decide on the client side whether URL fragment is already too long and not send it at all (e.g. if it's already bigger than the default limit of 2kb) - that'd be easier and safer. I'm leaning towards this solution.

I'm ok with this approach too. To clarify, this would result in users not landing on their initially requested deep-link into Kibana after authenticating (which was fixed in #44513), right?

@azasypkin
Copy link
Member Author

To clarify, this would result in users not landing on their initially requested deep-link into Kibana after authenticating (which was fixed in #44513), right?

Yeah, we'll just use the path portion of the URL that we capture at the previous step of the SAML handshake (e.g. /app/kibana without #/dashboard/722b74.... part, assuming it's within limits).

Basically we'd do same thing on the server side if Kibana server could accept that long URL via query string parameter, nothing will change for the user except that we'll workaround that limitation.

P.S. The amount of limitations we have to workaround for this seemingly simple functionality is enormous, we should do something with the way we store global state.

@legrego
Copy link
Member

legrego commented Dec 18, 2019

Thanks for confirming!

P.S. The amount of limitations we have to workaround for this seemingly simple functionality is enormous, we should do something with the way we store global state.

Yep, I completely agree...The url-based state we have today is very useful for "sharing what I'm looking at right now", but we're putting way too much in the URL right now. The opposite side of the spectrum, which I don't want to see, is the way you have to share case links in Salesforce. Nobody can figure it out the first time, and infrequent users have to relearn how to do this all the time.

@azasypkin
Copy link
Member Author

Yep, I completely agree...The url-based state we have today is very useful for "sharing what I'm looking at right now", but we're putting way too much in the URL right now. The opposite side of the spectrum, which I don't want to see, is the way you have to share case links in Salesforce. Nobody can figure it out the first time, and infrequent users have to relearn how to do this all the time.

That's a good point! We should find a good trade-off.

@kobelb
Copy link
Contributor

kobelb commented Dec 18, 2019

Given that we're trying to fix this in 7.5.x, I think putting a max url size on the client-side makes sense.

@azasypkin
Copy link
Member Author

Fixed in #68117

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Security/Authentication Platform Security - Authentication Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more!
Projects
None yet
Development

No branches or pull requests

3 participants