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

[http] Adds route config option access flag to indicate if an API is public or internal #152404

Merged
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ export class CoreKibanaRequest<
xsrfRequired:
((request.route?.settings as RouteOptions)?.app as KibanaRouteOptions)?.xsrfRequired ??
true, // some places in LP call KibanaRequest.from(request) manually. remove fallback to true before v8
access: this.getAccess(request),
tags: request.route?.settings?.tags || [],
timeout: {
payload: payloadTimeout,
Expand All @@ -222,6 +223,13 @@ export class CoreKibanaRequest<
options,
};
}
/** infer route access from path if not declared */
private getAccess(request: RawRequest): 'internal' | 'public' {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extracted to make it easier to read

return (
((request.route?.settings as RouteOptions)?.app as KibanaRouteOptions)?.access ??
(request.path.includes('internal') ? 'internal' : 'public')
Copy link
Contributor

Choose a reason for hiding this comment

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

This algo may have unintended consequences. The following will be classified as internal and I'm not sure they should be:

post /api/monitoring/v1/elasticsearch_settings/check/internal_monitoring
post /api/monitoring/v1/setup/collection/{clusterUuid}/disable_internal_collection

imo matching more strictly would reduce the potential for surprising outcomes which is better than missing a few routes. If routes should be internal they can add the access: internal flag. It looks like 99.9% of "internal" routes follow the convention of starting with /internal/ so I would match that. Just my opinion, let me know what you think!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're totally right! I'm fixing it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Um, I thought I had changed the string match to startsWith. Not sure where that commit got lost.

);
}

private getAuthRequired(request: RawRequest): boolean | 'optional' {
if (isFakeRawRequest(request)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ function createKibanaRequestMock<P = any, Q = any, B = any>({
routeTags,
routeAuthRequired,
validation = {},
kibanaRouteOptions = { xsrfRequired: true },
kibanaRouteOptions = { xsrfRequired: true, access: 'public' },
TinaHeiligers marked this conversation as resolved.
Show resolved Hide resolved
kibanaRequestState = {
requestId: '123',
requestUuid: '123e4567-e89b-12d3-a456-426614174000',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -817,6 +817,49 @@ test('allows attaching metadata to attach meta-data tag strings to a route', asy
await supertest(innerServer.listener).get('/without-tags').expect(200, { tags: [] });
});

test('allows declaring route access to flag a route as public or internal', async () => {
const access = 'internal';
const { registerRouter, server: innerServer } = await server.setup(config);

const router = new Router('', logger, enhanceWithContext);
router.get({ path: '/with-access', validate: false, options: { access } }, (context, req, res) =>
res.ok({ body: { access: req.route.options.access } })
);
router.get({ path: '/without-access', validate: false }, (context, req, res) =>
res.ok({ body: { access: req.route.options.access } })
);
registerRouter(router);

await server.start();
await supertest(innerServer.listener).get('/with-access').expect(200, { access });

await supertest(innerServer.listener).get('/without-access').expect(200, { access: 'public' });
});

test('infers access flag from path if not defined', async () => {
const { registerRouter, server: innerServer } = await server.setup(config);

const router = new Router('', logger, enhanceWithContext);
router.get({ path: '/internal/foo', validate: false }, (context, req, res) =>
res.ok({ body: { access: req.route.options.access } })
);
router.get({ path: '/random/foo', validate: false }, (context, req, res) =>
res.ok({ body: { access: req.route.options.access } })
);
router.get({ path: '/random/internal/foo', validate: false }, (context, req, res) =>
Copy link
Contributor Author

@TinaHeiligers TinaHeiligers Feb 28, 2023

Choose a reason for hiding this comment

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

'includes' would match any string that contains /internal, so paths such as /api/foo/internal/my-foo would have been inferred as public. This is a simple test to verify a 'mixed' path string.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like just /api/foo/internal/my-foo would be inferred to internal?

Copy link
Contributor Author

@TinaHeiligers TinaHeiligers Mar 1, 2023

Choose a reason for hiding this comment

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

/api/foo/internal/my-foo should be inferred as public actually but the commit where I changed the string matching didn't make it to the PR 🤕 .
I'm ok with this though, if folks want to use a "mixed" path string they can declare access upfront.

res.ok({ body: { access: req.route.options.access } })
);
registerRouter(router);

await server.start();
await supertest(innerServer.listener).get('/internal/foo').expect(200, { access: 'internal' });

await supertest(innerServer.listener).get('/random/foo').expect(200, { access: 'public' });
await supertest(innerServer.listener)
.get('/random/internal/foo')
.expect(200, { access: 'public' });
});

test('exposes route details of incoming request to a route handler', async () => {
const { registerRouter, server: innerServer } = await server.setup(config);

Expand All @@ -833,6 +876,7 @@ test('exposes route details of incoming request to a route handler', async () =>
options: {
authRequired: true,
xsrfRequired: false,
access: 'public',
tags: [],
timeout: {},
},
Expand Down Expand Up @@ -1010,6 +1054,7 @@ test('exposes route details of incoming request to a route handler (POST + paylo
options: {
authRequired: true,
xsrfRequired: true,
access: 'public',
tags: [],
timeout: {
payload: 10000,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -524,6 +524,7 @@ export class HttpServer {

const kibanaRouteOptions: KibanaRouteOptions = {
xsrfRequired: route.options.xsrfRequired ?? !isSafeMethod(route.method),
access: route.options.access ?? (route.path.startsWith('/internal') ? 'internal' : 'public'),
};

this.server!.route({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ describe('xsrf post-auth handler', () => {
path: '/some-path',
kibanaRouteOptions: {
xsrfRequired: false,
access: 'public',
},
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ export const createVersionCheckPostAuthHandler = (kibanaVersion: string): OnPost
};
};

// TODO: implement header required for accessing internal routes. See https://github.com/elastic/kibana/issues/151940
TinaHeiligers marked this conversation as resolved.
Show resolved Hide resolved
export const createCustomHeadersPreResponseHandler = (config: HttpConfig): OnPreResponseHandler => {
const {
name: serverName,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import type { Headers } from './headers';
*/
export interface KibanaRouteOptions extends RouteOptionsApp {
xsrfRequired: boolean;
access: 'internal' | 'public';
}

/**
Expand Down
12 changes: 12 additions & 0 deletions packages/core/http/core-http-server/src/router/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,18 @@ export interface RouteConfigOptions<Method extends RouteMethod> {
*/
xsrfRequired?: Method extends 'get' ? never : boolean;

/**
* Defines access permission for a route
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe: "Intended request origin of the route". I think it is OK to call the property access but I think we should make it clear in the doc comment this is not a security feature.

* - public. The route is public, declared stable and intended for external access.
* In the future, may require an incomming request to contain a specified header.
* - internal. The route is internal and intended for internal access only.
*
* If not declared, infers access from route path:
* - access =`internal` for '/internal' route path prefix
* - access = `public` for '/api' route path prefix
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* - access = `public` for '/api' route path prefix
* - access = `public` for everything else

*/
access?: 'public' | 'internal';

/**
* Additional metadata tag strings to attach to the route.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ const versionedRouter = vtk.createVersionedRouter({ router });
const versionedRoute = versionedRouter
.post({
path: '/api/my-app/foo/{id?}',
options: { timeout: { payload: 60000 } },
options: { timeout: { payload: 60000 }, access: 'public' },
})
.addVersion(
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import type {
RequestHandler,
RouteValidatorFullConfig,
RequestHandlerContextBase,
RouteConfigOptions,
} from '@kbn/core-http-server';

type RqCtx = RequestHandlerContextBase;
Expand Down Expand Up @@ -45,7 +46,7 @@ export interface CreateVersionedRouterArgs<Ctx extends RqCtx = RqCtx> {
* const versionedRoute = versionedRouter
* .post({
* path: '/api/my-app/foo/{id?}',
* options: { timeout: { payload: 60000 } },
* options: { timeout: { payload: 60000 }, access: 'public' },
* })
* .addVersion(
* {
Expand Down Expand Up @@ -99,14 +100,25 @@ export interface VersionHTTPToolkit {
): VersionedRouter<Ctx>;
}

type WithRequiredProperty<Type, Key extends keyof Type> = Type & {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm pretty sure we already have a util that does this but I couldn't find it.

[Property in Key]-?: Type[Property];
};

/**
* Versioned route access flag, required
* - '/api/foo' is 'public'
* - '/internal/my-foo' is 'internal'
* Required
*/
type VersionedRouteConfigOptions = WithRequiredProperty<RouteConfigOptions<RouteMethod>, 'access'>;
Copy link
Contributor

Choose a reason for hiding this comment

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

One alternative:

Suggested change
type VersionedRouteConfigOptions = WithRequiredProperty<RouteConfigOptions<RouteMethod>, 'access'>;
type VersionedRouteConfigOptions = Required<Pick<RouteConfigOptions<RouteMethod>, 'access'>>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using Pick doesn't allow us to specify timeout.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻 . WDYT about moving your type helper to the type utilities package?

/**
* Configuration for a versioned route
* @experimental
*/
export type VersionedRouteConfig<Method extends RouteMethod> = Omit<
RouteConfig<unknown, unknown, unknown, Method>,
'validate'
>;
'validate' | 'options'
> & { options: VersionedRouteConfigOptions };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes access required for versioned routes


/**
* Create an {@link VersionedRoute | versioned route}.
Expand Down