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

[POC] /app/* requests authenticate against Elasticsearch #17724

Closed
wants to merge 6 commits into from

Conversation

kobelb
Copy link
Contributor

@kobelb kobelb commented Apr 16, 2018

To remedy #9583 in a more holistic manner where we don't have to ensure every API endpoint returns the WWW-Authenticate header properly, this PR performs a "pre-flight" request against Elasticsearch using the current request to ensure the user is authenticated. If they aren't authenticated, then we redirect them to the application root where we can ensure that endpoint sets the headers properly for all child paths.

This conceptually will prevent most APIs from having to handle the WWW-Authenticate headers properly themselves, but there are some scenarios where the user will have to refresh the page to get the auth redirect.

Whenever the user hits a route that would render a specific application,
if we aren't using any auth with HapiJS, execute a pre-flight request
against ES, and if we get a 401 then redirect the user to the very
baseRoute which will do the 401 with the WWW-Authenticate header and
then redirect the user back to the app.
@elasticmachine
Copy link
Contributor

💔 Build Failed

@spalger
Copy link
Contributor

spalger commented Apr 16, 2018

Perhaps HEAD /.kibana would be sufficient?

@kobelb
Copy link
Contributor Author

kobelb commented Apr 17, 2018

@spalger that's interesting... do you know how HEAD requests are auth'ed against ES? It looks like performing a HEAD request similar to the following that don't exist would potentially work, but that seems really weird:

curl -k -I https://localhost:9200/idefinitelydontexist-* -u joe:changeme

@spalger
Copy link
Contributor

spalger commented Apr 17, 2018

HEAD requests should perform just like GET requests except skip over sending the body, so that could work too.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@kobelb kobelb requested review from spalger and epixa April 18, 2018 17:54
@kobelb
Copy link
Contributor Author

kobelb commented Apr 18, 2018

@spalger @epixa how do you guys feel about this approach? If we're alright with it, I'll write the necessary unit/integration tests.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

Couple notes, but my primary feedback is that I'm not a huge fan of the redirect, the new redirectApp query string param, or the duplication of logic (wrt. maintenance mostly).

Are we sending users back to / because the path that we send the first challenge response is really important? What if we had a pre handler that was shared between these two methods and both / and /app/* sent challenge responses?

If nothing else can we unify the logic for fetching the challenge response rather than recreating similar methods in both places?

});
return null;
} catch (err) {
if (err.statusCode !== 401) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This negative condition confused me at first, I think flipping the branches would make the intention more obvious.

@@ -16,8 +16,29 @@ import { setupXsrf } from './xsrf';
export default async function (kbnServer, server, config) {
server = kbnServer.server = new Hapi.Server();

const getAuthChallengeResponse = async (req) => {
if (req.auth.strategy || req.auth.mode) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind adding a comment that explains the intention behind this check?

@trevan
Copy link
Contributor

trevan commented Apr 18, 2018

@kobelb, couldn't you do this with a custom authentication plugin for hapi? You could then make that the default strategy and all routes (including new plugin routes) would have to go through this authentication.

@kobelb
Copy link
Contributor Author

kobelb commented Apr 19, 2018

We're sending the users back to "/" because of the way that Authentication headers are "cached" and provided by the browser.

If we were to respond with a 401 and the WWW-Authenticate header from say "/app/kibana", then the browser caches the Authentication header and passes it automatically to every URL that is equal to or a sub-path of "/app/kibana". So you'll see the browser automatically sending them to routes like "/app/kibana/something" automatically. However, when the browser goes to something like "/app/monitoring" this Authentication header won't be included by default. If the "/app/monitoring" route responds with a 401 and the WWW-Authentication header, then the browser will use the cached basic auth information to automatically supply then in a subsequent request and then all future requests to subpaths of "/app/monitoring" will get the Authentication header sent to them by default. However, if the route doesn't respond with the WWW-Authenticate header, then the 401 surfaces to the end-user.

By redirecting to "/" before we set this header, the Authentication header that is set here will be sent to all paths inside Kibana automatically.

@trevan yeah, we could use a HapiJS authentication plugin to do this pre-flight authorization check on every route. Initially, I didn't pursue this approach because of the overhead of doing this pre-flight request on every endpoint when most of them actually handle the 401s appropriately, and I didn't want to incur this additional overhead. It's a solution that is potentially worth pursuing.

To be honest, I'm not that big of a fan of this solution that I've proposed, and I'd prefer we either make every route handle the 401s appropriately, or move to a more comprehensive solution, perhaps the one that @trevan mentioned.

@trevan
Copy link
Contributor

trevan commented Apr 19, 2018

@kobelb, I think one way to reduce the overhead is to check if the request has an authentication header. If a request authentication header is available, then the 401 WWW-Authenticate header should have already been sent back in an earlier response.

@kobelb
Copy link
Contributor Author

kobelb commented May 3, 2018

Closing this to revise the approach and I'll throw up another PR.

@kobelb kobelb closed this May 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants