[http] Add log for route path access #152621

TinaHeiligers · 2023-03-02T21:10:54Z

part of 151940
follow up to 152404

As part of implementing API versioning, we need an easy way to identify which HTTP APIs are intended for internal vs public consumption and added an access route option param laying the foundation for future work.

The access setting is configurable and falls back to a naïve interpretation of the path string.

Given that we now have some internal/public classification, we can use logs to generate a list of all HTTP APIs registered with core's HTTP server.

This PR adds a debug logger (http.server.kibana.options) that will log the route path and the access value on startup.

Usage

Configure logging as follows:

// kibana.yml
logging:
  appenders:
    http-console:
      type: console
      layout:
        type: pattern
        highlight: true
        pattern: "HTTP--[%date][%level][%logger]---%message"
  root:
    appenders: [default, console]
    level: info
  loggers:
    - name: http.server
      level: debug
      appenders: [http-console]
    - name: http.server.kibana.options
      level: debug
      appenders: [http-console]

On server startup, you should see the logs similar to:

HTTP--[2023-03-02T13:45:44.600-07:00][DEBUG][http.server.Kibana.options]---kibanaRouteOptions [public] for path [/api/apm/fleet/apm_server_schema]
HTTP--[2023-03-02T13:45:44.601-07:00][DEBUG][http.server.Kibana]---registering route handler for [/internal/apm/fleet/apm_server_schema/unsupported]
HTTP--[2023-03-02T13:45:44.601-07:00][DEBUG][http.server.Kibana.options]---kibanaRouteOptions [internal] for path [/internal/apm/fleet/apm_server_schema/unsupported]

TinaHeiligers

Comments to reviewers.

TinaHeiligers · 2023-03-02T21:57:45Z

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

@@ -526,6 +527,10 @@ export class HttpServer {
      xsrfRequired: route.options.xsrfRequired ?? !isSafeMethod(route.method),
      access: route.options.access ?? (route.path.startsWith('/internal') ? 'internal' : 'public'),
    };
+    // Log HTTP API target consumer. Warning: may log sensitive information on paths including secrets


I added a warning to remind ourselves (and API owners) where debug logs could cause issues.

IIUC, something like POST /api/my-secret/is/:mySecret would log ... for path [/api/my-secret/is/:mySecret], and not the actual value in the secret when the user performs the request. Am I missing anything?

You're not missing anything, the warning is to remind folks that anything in the path will log as is and to think very carefully about what they are fine with exposing is.

Sorry... I'm a bit slow today... 😞

AFAIK, this comment is only read by Kibana Core devs. I don't get how this comment would warn a plugin developer to design their API so that they don't register an HTTP route like POST /api/my/plugin/${config.secret}/:id. Bear in mind we are logging the registered path, not the final path that is requested with the variables replaced by the values (:id in the example)

@afharo I believe what you are describing has already been introduced with

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

Line 519 in 8fdcf09

this.log.debug(`registering route handler for [${route.path}]`);

Not a reason skip reviewing the decision, just saying it's not being added with this PR :)

I don't want to block this PR 😇. These are nits rather than anything. Also, I noticed @TinaHeiligers removed the comment.

@afharo I believe what you are describing has already been introduced with

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

Line 519 in 8fdcf09

this.log.debug(`registering route handler for [${route.path}]`);

IIUC, I'm describing both lines. This PR adds a new entry in the logs that rephrases and adds info to the already existing entry. The new log entry is also logging the registered path, not the accessed path.

TinaHeiligers · 2023-03-02T21:58:47Z

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

@@ -516,6 +516,7 @@ export class HttpServer {
  }

  private configureRoute(route: RouterRoute) {
+    const optionsLogger = this.logger.get('http', 'server', this.name, 'options');


I chose to add a new logger to make it easier to filter for these specific log lines

If we want to avoid repeating the http and server part, I think we can define the logger asthis.log.get(this.name, 'options);.

We'd lose the full context around where the log comes from if we remove http and server, which makes it very hard to analyze the debug-level logs.
Personally, I'd prefer being able to filter logs by a full logger name that keeps some sort of record of where it's coming from.

We'd lose the full context around where the log comes from if we remove http and server, which makes it very hard to analyze the debug-level logs.

Mind that I'm using this.log (instead of this.logger). AFAIK, this.log is already scoped to http.server, calling this.log.get calls a nested logger. If we ever change the parent, it all changes together (as opposed to using the root logger this.logger). I hope it makes sense.

Personally, I'd prefer being able to filter logs by a full logger name that keeps some sort of record of where it's coming from.

++ I agree.

using this.log (instead of this.logger)

I did a quick test with this.log.get(this.name, 'options') with the same http-console` appender. We end up duplicating the server name:

HTTP--[2023-03-06T10:05:25.632-07:00][DEBUG][http.server.Kibana.Kibana.options]---access [internal] for path [/internal/apm/service-map/service/{serviceName}]

oh! cool! I didn't notice the existing entry is logged as http.server.Kibana. In that case, this.log.get('options') is enough :)

done: commit fb86412 (#152621)

TinaHeiligers · 2023-03-02T22:26:09Z

@elasticmachine merge upstream

elasticmachine · 2023-03-02T23:48:54Z

Pinging @elastic/kibana-core (Team:Core)

TinaHeiligers · 2023-03-05T22:45:12Z

@elasticmachine merge upstream

jloleysens · 2023-03-06T09:08:01Z

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

@@ -526,6 +527,10 @@ export class HttpServer {
      xsrfRequired: route.options.xsrfRequired ?? !isSafeMethod(route.method),
      access: route.options.access ?? (route.path.startsWith('/internal') ? 'internal' : 'public'),
    };
+    // Log HTTP API target consumer. Warning: may log sensitive information on paths including secrets
+    optionsLogger.debug(
+      `kibanaRouteOptions [${kibanaRouteOptions.access}] for path [${route.path}]`


Shall we log the method too? IMO "access" is a clearer prefix than "kibanaRouteOptions":

Suggested change

`kibanaRouteOptions [${kibanaRouteOptions.access}] for path [${route.path}]`

`access [${kibanaRouteOptions.access}] for path [${route.method} ${route.path}]`

Approved!
I've also removed the code comment's warning.
Please TAL.

afharo

Having this is awesome! Thanks for making it happen, Tina!

I added a few comments that I'd like to clarify before LGTM :)

afharo · 2023-03-06T13:16:05Z

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

@@ -516,6 +516,7 @@ export class HttpServer {
  }

  private configureRoute(route: RouterRoute) {
+    const optionsLogger = this.logger.get('http', 'server', this.name, 'options');
    this.log.debug(`registering route handler for [${route.path}]`);


I wonder if extending this log entry with the values suggested in https://github.com/elastic/kibana/pull/152621/files/bf7a3c36cb4394b06a3b3fa09a0553c3f958bfed#r1126120651 would suffice. Both entries look too similar and are logged at the same time. If the only reason is so that we can use a new logger, I wonder if we can use it here (as long as it's nested, it should still log with the same config)

I thought about that but went a new logger route to be able to silence the extra info or to later remove the logger completely without losing the original log

afharo · 2023-03-06T13:16:46Z

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

@@ -516,6 +516,7 @@ export class HttpServer {
  }

  private configureRoute(route: RouterRoute) {
+    const optionsLogger = this.logger.get('http', 'server', this.name, 'options');


If we want to avoid repeating the http and server part, I think we can define the logger asthis.log.get(this.name, 'options);.

afharo · 2023-03-06T13:21:37Z

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

@@ -526,6 +527,10 @@ export class HttpServer {
      xsrfRequired: route.options.xsrfRequired ?? !isSafeMethod(route.method),
      access: route.options.access ?? (route.path.startsWith('/internal') ? 'internal' : 'public'),
    };
+    // Log HTTP API target consumer. Warning: may log sensitive information on paths including secrets


IIUC, something like POST /api/my-secret/is/:mySecret would log ... for path [/api/my-secret/is/:mySecret], and not the actual value in the secret when the user performs the request. Am I missing anything?

afharo · 2023-03-06T18:58:46Z

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

@@ -526,6 +527,8 @@ export class HttpServer {
      xsrfRequired: route.options.xsrfRequired ?? !isSafeMethod(route.method),
      access: route.options.access ?? (route.path.startsWith('/internal') ? 'internal' : 'public'),
    };
+    // Log HTTP API target consumer.
+    optionsLogger.debug(`access [${kibanaRouteOptions.access}] for path [${route.path}]`);


mind that @jloleysens's comment also suggested adding the method

Apologies, adding that now.

done: commit fb86412

kibana-ci · 2023-03-06T21:48:29Z

💚 Build Succeeded

Buildkite Build
Commit: fb86412

Metrics [docs]

Unknown metric groups

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 #112160 succeeded 4ccb36c
💚 Build #111926 succeeded ed281fe
💚 Build #111843 succeeded 2962355
💔 Build #111831 failed bf7a3c3

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

cc @TinaHeiligers

Co-authored-by: Kibana Machine <[email protected]>

Logs route path access

bf7a3c3

TinaHeiligers self-assigned this Mar 2, 2023

TinaHeiligers commented Mar 2, 2023

View reviewed changes

Merge branch 'main' into kbn-152269-followup-log-route-access

2962355

TinaHeiligers marked this pull request as ready for review March 2, 2023 23:48

TinaHeiligers requested a review from a team as a code owner March 2, 2023 23:48

Merge branch 'main' into kbn-152269-followup-log-route-access

ed281fe

jloleysens reviewed Mar 6, 2023

View reviewed changes

afharo reviewed Mar 6, 2023

View reviewed changes

address PR comments

4ccb36c

afharo reviewed Mar 6, 2023

View reviewed changes

Adds route method to options logger logs, implement PR nit

fb86412

afharo approved these changes Mar 6, 2023

View reviewed changes

TinaHeiligers merged commit cf1fbc7 into elastic:main Mar 6, 2023

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

[http] Add log for route path access (elastic#152621)

7a89263

Co-authored-by: Kibana Machine <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[http] Add log for route path access #152621

[http] Add log for route path access #152621

TinaHeiligers commented Mar 2, 2023 •

edited

Loading

TinaHeiligers left a comment

TinaHeiligers Mar 2, 2023

afharo Mar 6, 2023

TinaHeiligers Mar 6, 2023

afharo Mar 6, 2023

jloleysens Mar 6, 2023

jloleysens Mar 6, 2023 •

edited

Loading

afharo Mar 6, 2023

TinaHeiligers Mar 2, 2023

afharo Mar 6, 2023

TinaHeiligers Mar 6, 2023

afharo Mar 6, 2023

TinaHeiligers Mar 6, 2023 •

edited

Loading

afharo Mar 6, 2023 •

edited

Loading

TinaHeiligers Mar 6, 2023

TinaHeiligers commented Mar 2, 2023

elasticmachine commented Mar 2, 2023

TinaHeiligers commented Mar 5, 2023

jloleysens Mar 6, 2023

TinaHeiligers Mar 6, 2023 •

edited

Loading

afharo left a comment

afharo Mar 6, 2023

TinaHeiligers Mar 6, 2023

afharo Mar 6, 2023

afharo Mar 6, 2023

afharo Mar 6, 2023

TinaHeiligers Mar 6, 2023

TinaHeiligers Mar 6, 2023

kibana-ci commented Mar 6, 2023

ESLint disabled line counts

Total ESLint disabled count

	`kibanaRouteOptions [${kibanaRouteOptions.access}] for path [${route.path}]`
	`access [${kibanaRouteOptions.access}] for path [${route.method} ${route.path}]`

[http] Add log for route path access #152621

[http] Add log for route path access #152621

Conversation

TinaHeiligers commented Mar 2, 2023 • edited Loading

Usage

TinaHeiligers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jloleysens Mar 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinaHeiligers Mar 6, 2023 • edited Loading

Choose a reason for hiding this comment

afharo Mar 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinaHeiligers commented Mar 2, 2023

elasticmachine commented Mar 2, 2023

TinaHeiligers commented Mar 5, 2023

Choose a reason for hiding this comment

TinaHeiligers Mar 6, 2023 • edited Loading

Choose a reason for hiding this comment

afharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Mar 6, 2023

💚 Build Succeeded

Metrics [docs]

ESLint disabled line counts

Total ESLint disabled count

History

TinaHeiligers commented Mar 2, 2023 •

edited

Loading

jloleysens Mar 6, 2023 •

edited

Loading

TinaHeiligers Mar 6, 2023 •

edited

Loading

afharo Mar 6, 2023 •

edited

Loading

TinaHeiligers Mar 6, 2023 •

edited

Loading