[http] Adds route config option access flag to indicate if an API is public or internal by TinaHeiligers · Pull Request #152404 · elastic/kibana

TinaHeiligers · 2023-02-28T23:28:45Z

Summary

Part of #151940

The current naming convention used to indicate if an API is intended for internal or public use has proven to be insufficient. This limits what we can change in internal APIs, since we risk breaking external integrations. As such, we want to formalize the distinction, both to clarify an API's intended use and reduce the number of public APIs we have to maintain for a long time.

This PR adds an optional access option to Core's RouteConfigOptions that allows consumers to mark their API's intended consumption.
ATM, the option is only a flag, but we intend to develop the formalization further at some later stage.

If access is not declared, core's HTTP service infers it from a route path:
/internal/my-foo is taken to be access: 'internal', whereas any other path string is interpreted as 'public'.

This PR also makes the access parameter required for Versioned routers.

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately
Note: the new config option is optional in the core implementation specifically to avoid breaking changes.

TinaHeiligers

draft self-review

TinaHeiligers · 2023-02-28T23:40:47Z

packages/core/http/core-http-router-server-internal/src/request.ts

    };
  }
+  /** infer route access from path if not declared */
+  private getAccess(request: RawRequest): 'internal' | 'public' {


Extracted to make it easier to read

packages/core/http/core-http-router-server-internal/src/route.ts

packages/core/http/core-http-router-server-mocks/src/router.mock.ts

TinaHeiligers · 2023-02-28T23:44:44Z

packages/core/http/core-http-server-internal/src/http_server.test.ts

+  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) =>


'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.

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

/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.

packages/core/http/core-http-server-internal/src/http_server.ts

packages/core/http/core-http-server-internal/src/lifecycle_handlers.ts

TinaHeiligers · 2023-02-28T23:47:08Z

packages/core/versioning/core-version-http-server/src/version_http_toolkit.ts

  ): VersionedRouter<Ctx>;
 }

+type WithRequiredProperty<Type, Key extends keyof Type> = Type & {


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

TinaHeiligers · 2023-02-28T23:47:30Z

packages/core/versioning/core-version-http-server/src/version_http_toolkit.ts

-  'validate'
->;
+  'validate' | 'options'
+> & { options: VersionedRouteConfigOptions };


makes access required for versioned routes

jloleysens

Great work @TinaHeiligers ! Overall this is looking great and I think this will be a really nice improvement. I left a few comments I'd like to get your thoughts on before approving.

One other thought: I see we are determining internal/public at the CoreKibanaRequest level. However, we know this at the Router level (i.e. we know this when the route is registered, not only when a request is made). We could calculate it once and pass it in to CoreKibanaRequest.from factory as an additional arg. I get this is more of a refactor and maybe not worth it for this work though.

jloleysens · 2023-03-01T10:02:52Z

packages/core/http/core-http-router-server-internal/src/request.ts

+  private getAccess(request: RawRequest): 'internal' | 'public' {
+    return (
+      ((request.route?.settings as RouteOptions)?.app as KibanaRouteOptions)?.access ??
+      (request.path.includes('internal') ? 'internal' : 'public')


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!

you're totally right! I'm fixing it

jloleysens · 2023-03-01T10:13:38Z

packages/core/http/core-http-server/src/router/route.ts

  xsrfRequired?: Method extends 'get' ? never : boolean;

+  /**
+   * Defines access permission for a route


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.

jloleysens · 2023-03-01T10:14:19Z

packages/core/http/core-http-server/src/router/route.ts

+   *
+   * If not declared, infers access from route path:
+   * - access =`internal` for '/internal' route path prefix
+   * - access = `public` for '/api' route path prefix


Suggested change

* - access = `public` for '/api' route path prefix

* - access = `public` for everything else

jloleysens · 2023-03-01T10:17:27Z

packages/core/http/core-http-server-internal/src/http_server.test.ts

+  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) =>


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

jloleysens · 2023-03-01T10:26:53Z

packages/core/versioning/core-version-http-server/src/version_http_toolkit.ts

+ * - '/internal/my-foo'  is 'internal'
+ * Required
+ */
+type VersionedRouteConfigOptions = WithRequiredProperty<RouteConfigOptions<RouteMethod>, 'access'>;


One alternative:

Suggested change

type VersionedRouteConfigOptions = WithRequiredProperty<RouteConfigOptions<RouteMethod>, 'access'>;

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

Using Pick doesn't allow us to specify timeout.

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

TinaHeiligers · 2023-03-01T14:02:36Z

packages/core/http/core-http-router-server-internal/src/request.ts

+  private getAccess(request: RawRequest): 'internal' | 'public' {
+    return (
+      ((request.route?.settings as RouteOptions)?.app as KibanaRouteOptions)?.access ??
+      (request.path.includes('internal') ? 'internal' : 'public')


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

TinaHeiligers · 2023-03-01T17:34:33Z

We could calculate it once and pass it in to CoreKibanaRequest.from factory as an additional arg. I get this is more of a refactor and maybe not worth it for this work though.

It's an interesting approach and may even be more optimal, depending on how the rest of the work progresses.
However, wouldn't consumers who don't use CoreKibanaRequest.from still need an implementation from KibanaRequest?

kibana-ci · 2023-03-01T20:44:36Z

💚 Build Succeeded

Buildkite Build
Commit: ce86192

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/core-http-server`	160	161	+1

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-http-server`	411	413	+2

ESLint disabled line counts

id	before	after	diff
`securitySolution`	428	430	+2

Total ESLint disabled count

id	before	after	diff
`securitySolution`	506	508	+2

History

💔 Build #111451 failed 30e903b
💚 Build #111242 succeeded ac5eb63
💔 Build #111238 failed 53d2577

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

TinaHeiligers · 2023-03-01T20:47:16Z

@jloleysens I've addressed your comments. Could you TAL please?

jloleysens

Good work @TinaHeiligers! I left a couple comments based on your responses. Otherwise this looks good to me.

consumers who don't use CoreKibanaRequest.from

That's a good point. I hadn't considered the instances of fake raw requests or wrapping raw requests (mainly like what happens in security plugin) outside the context of our Router.

jloleysens · 2023-03-02T14:09:56Z

packages/core/http/core-http-router-server-internal/src/request.ts

+  private getAccess(request: RawRequest): 'internal' | 'public' {
+    return (
+      ((request.route?.settings as RouteOptions)?.app as KibanaRouteOptions)?.access ??
+      (request.path.startsWith('/internal') ? 'internal' : 'public')


We can be even stricter and add a trailing slash /internal/ or use RegExp: new RegExp('^(\/internal$|\/internal\/)')

However, a route that starts with /internal... will almost certainly be intended for access: internal unless we ever have a route /internalize or something 😅

jloleysens · 2023-03-02T14:22:30Z

packages/core/versioning/core-version-http-server/src/version_http_toolkit.ts

+ * - '/internal/my-foo'  is 'internal'
+ * Required
+ */
+type VersionedRouteConfigOptions = WithRequiredProperty<RouteConfigOptions<RouteMethod>, 'access'>;


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

TinaHeiligers · 2023-03-02T15:14:45Z

WDYT about moving your type helper to the type utilities package?

It's not fully tested and will have to be if we move that to a general type-utils package. Once we need to use it more widely, we can put the work in to move that.

…public or internal (elastic#152404)

TinaHeiligers added 2 commits February 28, 2023 14:56

Adds option API access flag to indicate if an API is public or internal

28972a4

Versioned routes must declare access

53d2577

TinaHeiligers added Feature:http release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting Epic:VersionedAPIs Kibana Versioned APIs labels Feb 28, 2023

TinaHeiligers added 2 commits February 28, 2023 16:38

Fix lifecycle_handlers test

0ddbfc7

Remove unused log and helper

ac5eb63

TinaHeiligers commented Feb 28, 2023

View reviewed changes

TinaHeiligers added the v8.8.0 label Mar 1, 2023

TinaHeiligers marked this pull request as ready for review March 1, 2023 00:52

TinaHeiligers requested a review from a team as a code owner March 1, 2023 00:52

jloleysens reviewed Mar 1, 2023

View reviewed changes

TinaHeiligers commented Mar 1, 2023

View reviewed changes

Match path correctly, PR enhancements

30e903b

TinaHeiligers added 2 commits March 1, 2023 12:20

revert utility type change

e0c356d

Adds test specifically for route path containing both api and internal

ce86192

jloleysens approved these changes Mar 2, 2023

View reviewed changes

TinaHeiligers merged commit 41f7e63 into elastic:main Mar 2, 2023

TinaHeiligers deleted the kbn-152269-RouteConfigOption-allows-declaring-access branch March 2, 2023 15:31

TinaHeiligers mentioned this pull request Mar 2, 2023

[http] Add log for route path access #152621

Merged

bmorelli25 pushed a commit to bmorelli25/kibana that referenced this pull request Mar 10, 2023

[http] Adds route config option access flag to indicate if an API is …

1db57bc

…public or internal (elastic#152404)

TinaHeiligers mentioned this pull request May 5, 2023

Enables preventing access to internal APIs #156935

Merged

5 tasks

ElenaStoeva mentioned this pull request Jun 14, 2023

Disable Advanced Settings for serverless #159590

Closed

3 tasks

azasypkin mentioned this pull request Jul 6, 2023

Prepare the Security, Spaces, and Encrypted Saved Objects HTTP APIs for the Serverless offering #161337

Closed

34 tasks

This was referenced Jul 25, 2023

Enable API protection in serverless #162149

Merged

enable API protection in serverless #162749

Closed

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

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

Conversation

TinaHeiligers commented Feb 28, 2023 • edited by lukeelmers Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

For maintainers

Uh oh!

TinaHeiligers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TinaHeiligers Feb 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TinaHeiligers Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jloleysens left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TinaHeiligers commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kibana-ci commented Mar 1, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

API count

ESLint disabled line counts

Total ESLint disabled count

History

Uh oh!

TinaHeiligers commented Mar 1, 2023

Uh oh!

jloleysens left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TinaHeiligers commented Mar 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

TinaHeiligers commented Feb 28, 2023 •

edited by lukeelmers

Loading

TinaHeiligers Feb 28, 2023 •

edited

Loading

TinaHeiligers Mar 1, 2023 •

edited

Loading

TinaHeiligers commented Mar 1, 2023 •

edited

Loading