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

Migrate /bootstrap.js endpoint to core #92784

Merged

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Feb 25, 2021

Summary

Fix #54375

This PR migrates the /bootstrap.js and /app/{id}/{any*} endpoints from legacy to core.

  • Add the authRequired: 'try' mode for routes (required for the bootstrap.js logic)
  • Add the http.auth.isEnabled API (required for the bootstrap.js logic)
  • Add the HttpResponseOptions.etag API (required for the bootstrap.js logic)
  • Migrate the /bootstrap.js endpoint to core
  • Migrate the /app/{id}/{any*} endpoint to core
  • Add RUM config to all httpResources routes
  • Get rid of the legacy uiRender mixin

Checklist

@pgayvallet pgayvallet force-pushed the kbn-54375-migrate-bootstrap-endpoint branch from 9ab1025 to cd32213 Compare February 25, 2021 14:05
{
path: '/bootstrap.js',
options: {
authRequired: 'optional',
Copy link
Member

Choose a reason for hiding this comment

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

@azasypkin,

This PR moves the /bootstrap.js endpoint out of legacy and into core -- as part of this change, the route went from hapi's authMode of try to optional.

This caused a lot of x-pack test failures, because some test suites ended up loading Kibana with an incorrect Authorization header (invalid credentials). In the legacy world where this used try, the invalid credentials allowed the /bootstrap.js request to succeed. Now that this route is using authMode: 'optional', the request fails when presented with invalid credentials.

We previously asked the core team to implement authMode: 'optional' for the KP, because we needed to detect if the user had a valid session or not, without directly relying on the existence of the session cookie (#58589). Now that we have server-side sessions, it's not clear to me if we need to keep these routes as optional, or if we can move them to try.

My gut feeling is that we don't need the behavior that optional gives us anymore, but I wanted to double check with you, since you're well-versed on the different auth flows and states.

Copy link
Contributor Author

@pgayvallet pgayvallet Mar 1, 2021

Choose a reason for hiding this comment

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

FWIW I went with adding the try auth mode for now.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the delayed reply, just caught up on the emails after PTO.

This caused a lot of x-pack test failures, because some test suites ended up loading Kibana with an incorrect Authorization header (invalid credentials).

Interesting, why do we do that? Is there any legitimate case when it makes sense?

We previously asked the core team to implement authMode: 'optional' for the KP, because we needed to detect if the user had a valid session or not, without directly relying on the existence of the session cookie (#58589).

Right, we wanted to abstract away from the fact that session is stored in the cookie running the Core authentication hook instead. But the hook shouldn't fail, or redirect to IdP (for SAML/OIDC), or automatically initiate new session (PKI/Kerberos) if credentials aren't provided.

Now that we have server-side sessions, it's not clear to me if we need to keep these routes as optional, or if we can move them to try.

I'm not sure how server-side sessions would change anything here to be honest, can you please elaborate a bit?

At that time we picked optional as seemingly the safest option (if you send any credentials you likely expect them to be respected) and the one with the behavior similar to the ES behavior when anonymous access is enabled there (requests without credentials are treated as anonymous and requests with invalid credentials are rejected). I still feel that try behavior is a bit confusing (any request credentials are attempted authentication, but if the credentials are invalid, the request proceeds regardless of the authentication error).

Having said that I believe we can safely switch our routes (login, logged_out etc.) to try since our authentication pipeline only cares if authentication is strictly required or not. So we won't need optional anymore.

@pgayvallet pgayvallet added chore Feature:Legacy Removal Issues related to removing legacy Kibana release_note:skip Skip the PR/issue when compiling release notes v7.13.0 labels Mar 1, 2021
Copy link
Contributor Author

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

self review

Comment on lines +51 to +62
resources.register(
{
path: '/app/{id}/{any*}',
validate: false,
options: {
authRequired: true,
},
},
async (context, request, response) => {
return response.renderCoreApp();
}
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • /app/{id} is registered from the CoreApp service
  • We are using renderCoreApp which ensure consistency between the 'default route' apps and the manually registered app endpoints.

Comment on lines +238 to +240
if (authRequired === 'try') {
return { mode: 'try' };
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #92784 (comment) for more context about the addition of the try auth mode.

@elastic/kibana-security tell me if you'd prefer that I replace the behavior of the optional mode instead (but imho having both makes sense)

Copy link
Member

Choose a reason for hiding this comment

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

I'm happy with the approach you have here. I don't see why we can't support both try and optional

Comment on lines +42 to +52
describe('when authRequired is `true`', () => {
it('allows authenticated access when auth returns `authenticated`', async () => {
const { http } = await root.setup();
const { registerAuth, createRouter, auth } = http;

registerAuth((req, res, toolkit) => toolkit.authenticated());

const router = createRouter('');
registerRoute(router, auth, true);

await root.start();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those can look similar to the auth tests we have in core_services.test.ts, but these added IT are testing the behavior of the interaction between the registered auth provider's response and the endpoint response directly (and more in depth)

src/core/server/http/router/response.ts Outdated Show resolved Hide resolved
Comment on lines 82 to 86
load([
{{#each styleSheetPaths}}
'{{this}}',
{{/each}}
]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Stylesheets are now directly included in the document generated by RenderingServiceSetup.render instead.

Comment on lines 46 to 53
const isAuthenticated = (request: KibanaRequest) => {
if (!auth.isEnabled()) {
return true;
}
const { status: authStatus } = auth.get(request);
// status is 'unknown' when auth is disabled. we just need to not be `unauthenticated` here.
return authStatus !== 'unauthenticated';
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isAuthenticated function could be renamed amIAuthorizedToQueryElasticsearch, as it returns true either when auth is disabled (no security), or if auth is enabled AND the user is authenticated.

(Long) note: now that we moved the stylesheet loading to RenderingServiceSetup.render, the only remaining reason to load uiSettings stored in ES when rendering bootstrap.js is to compute the __kbnThemeTag__ (themeTag below) variable (that is used as a prefix by our client-side loader for stylesheets). I tried to move that variable initialization to the rendered document instead, however, because of our CSP policy, we can't use inline scripts (and injecting the variable from the client-side core code is too late)

An improvement would be to (re-)add a nonce-XXX option to our CSP, to let the rendering service have inlined scripts, but doing that properly (aka not as it was done in legacy before we removed it) would require to generate the nonce value per-request and to have the rendering service access this value when rendering the document. If technically doable, it is not trivial and require some thinking, so imho (given that we really want that) it should be done as a follow-up (we would even be able to inline the whole bootstrap.js script in the rendered document in that case).

cc @legrego @joshdover this is the reason why I was speaking about reintroducing nonce-

Copy link
Contributor

Choose a reason for hiding this comment

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

Mind creating a follow-up issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created #93785

src/core/server/rendering/bootstrap/bootstrap_renderer.ts Outdated Show resolved Hide resolved
Comment on lines +63 to +71
const darkMode = getSettingValue('theme:darkMode', settings, Boolean);
const themeVersion = getSettingValue('theme:version', settings, String);

const stylesheetPaths = getStylesheetPaths({
darkMode,
themeVersion,
basePath: serverBasePath,
buildNum,
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving the stylesheets loading from bootstrap.js to the core/app document introduced a slight behavior change:

  • In bootstrap.js, we were checking if we were allowed to query ES to retrieve the user settings (security off OR security ON + user authenticated)
  • In the rendering service, we depends on the includeUserSettings input parameter, which is always false for unauthenticated routes (such as login)

In short: previously, an authenticated user accessing the login page would be able to use the persisted user settings, and now, the default setting values will always be used. As we are reloading the app when logging in, I think this is okay.

If not, we will have to re-use the isAuthenticated method/logic from rendering/bootstrap/bootstrap_renderer.ts instead of this includeUserSettings parameter (which could then be totally removed from the render API)

No strong opinion on that one, tell me what you think.

@@ -72,8 +71,6 @@ export default class KbnServer {
// tell the config we are done loading plugins
configCompleteMixin,

uiMixin,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

\o
o/
\o/

@pgayvallet pgayvallet marked this pull request as ready for review March 1, 2021 12:59
@pgayvallet pgayvallet requested a review from a team as a code owner March 1, 2021 12:59
@pgayvallet pgayvallet requested a review from mshustov March 1, 2021 12:59
@pgayvallet pgayvallet requested a review from mshustov March 8, 2021 07:45

// These paths should align with the bundle routes configured in
// src/optimize/bundles_route/bundles_route.ts
const publicPathMap = JSON.stringify({
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised src/optimize/bundles_route/bundles_route.ts doesn't have a similar comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My next step is to migrate the bundle_route anyway, will add the comment when moving the route to core.

etag: string;
}

export const bootstrapRendererFactory: BootstrapRendererFactory = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

optional: this file contains some logic, but I don't see bootstrapRendererFactory is tested

Comment on lines 51 to 62
let darkMode = false;
let themeVersion = 'v7';

try {
const authenticated = isAuthenticated(request);
darkMode = authenticated ? await uiSettingsClient.get('theme:darkMode') : false;
themeVersion = authenticated ? await uiSettingsClient.get('theme:version') : 'v7';
} catch (e) {
// just use the default values in case of connectivity issues with ES
}

const themeTag = `${themeVersion === 'v7' ? 'v7' : 'v8'}${darkMode ? 'dark' : 'light'}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

optional: we can keep it as is, but I'd extract themeTag calculation to a separate function to simplify reading.
Also, it's not obvious without a comment how __kbnThemeTag__ is used.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@pgayvallet pgayvallet merged commit d094912 into elastic:master Mar 10, 2021
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Mar 10, 2021
* move bootstrap endpoint to core

* some initial cleanup

* hack around the 'try' auth status

* some UT

* more UT

* add try/catch around uISettings access

* add 'auth.isEnabled'

* remove dead files

* use `try` authent mode

* adapt UT

* revert themeTag movearound

* migrate apps route to core

* some cleanup

* nit

* add integration tests

* update generated doc

* add UT for /app route

* add etag IT

* nits

* remove auth.isEnabled API

* add tests on get_apm_config

* use string template instead of handlebars for bootstrap template

* improve plugin bundle tests

* update generated doc

* remove response.etag API

* update generated doc

* update generated doc

* update generated doc again

* extract getThemeTag

* add more unit tests
pgayvallet added a commit that referenced this pull request Mar 10, 2021
* move bootstrap endpoint to core

* some initial cleanup

* hack around the 'try' auth status

* some UT

* more UT

* add try/catch around uISettings access

* add 'auth.isEnabled'

* remove dead files

* use `try` authent mode

* adapt UT

* revert themeTag movearound

* migrate apps route to core

* some cleanup

* nit

* add integration tests

* update generated doc

* add UT for /app route

* add etag IT

* nits

* remove auth.isEnabled API

* add tests on get_apm_config

* use string template instead of handlebars for bootstrap template

* improve plugin bundle tests

* update generated doc

* remove response.etag API

* update generated doc

* update generated doc

* update generated doc again

* extract getThemeTag

* add more unit tests
@pgayvallet pgayvallet mentioned this pull request Mar 10, 2021
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Feature:Legacy Removal Issues related to removing legacy Kibana release_note:skip Skip the PR/issue when compiling release notes v7.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate the bootstrap.js endpoint to Kibana Platform
5 participants