Add support for reading request ID from X-Opaque-Id header #71019

joshdover · 2020-07-07T21:42:12Z

Summary

Fixes #62018

This PR essentially does 3 things:

Adds new requestId field to KibanaRequest that is sourced from the X-Opaque-Id header
- The user can configure which IP addresses this header should be trusted from
Forwards the requestId to all requests to Elasticsearch that are made using the LegacyScopedClusterClient
Adds requestId to AuditEvents in the audit_trail plugin

TODO:

If Add new elasticsearch client #69905 is merged first, the changes to the Elasticsearch service need to be ported to the new client implementation
Add docs for new config settings

Checklist

Delete any items that are not applicable to this PR.

Documentation was added for features that require explanation or tutorials
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

src/core/server/http/http_config.ts

src/core/server/http/http_server.test.ts

src/core/server/http/router/request.test.ts

joshdover · 2020-07-07T22:18:32Z

x-pack/plugins/audit_trail/server/types.ts

@@ -13,4 +13,5 @@ export interface AuditEvent {
  scope?: string;
  user?: string;
  space?: string;
+  requestId?: string;


This is probably not the right name since this identifier may not come from a request (eg. background task).

In ECS audit log this will be logged as trace.id but I'd say don't worry about updating the schema for now. I am happy to implement this as part of audit log PR as it's just going to create merge conflicts.

pgayvallet · 2020-07-08T11:10:42Z

If #69905 is merged first, the changes to the Elasticsearch service need to be ported to the new client implementation

I added a task to add X-Opaque-Id support for the new client in #35508. It doesn't (necessarily) need to be done in this PR if #69905 is merged first though, or for the internal changes in core (as we are not using a scoped client) but it would be necessary before we expose the new client to the plugins.

src/core/server/http/http_config.test.ts

src/core/server/http/http_config.ts

src/core/server/http/http_server.test.ts

src/core/server/http/integration_tests/request.test.ts

packages/kbn-utility-types/index.ts

src/core/server/http/router/request.ts

joshdover · 2020-07-08T22:57:53Z

This PR is still in draft and WIP. Just wanted to push up what I had at the end of my day to run CI. There's definitely some still some rough edges. I will get back to this tomorrow and address the feedback. Also big +1 on using Joi here, I had forgotten that it included very specific validations like IP.

delvedor

Just a heads up, if you need it, the client does have first-class support for the X-Opaque-Id header :)

joshdover · 2020-07-13T17:40:16Z

Just a heads up, if you need it, the client does have first-class support for the X-Opaque-Id header :)

It could be useful to include the audittrail's current scope name as a prefix in the X-Opaque-Id. I will explore in a follow up.

pgayvallet · 2020-07-13T18:46:27Z

Just a heads up, if you need it, the client does have first-class support for the X-Opaque-Id header :)

Not sure how to leverage that however, as we decided to get rid of our facade. We can't create a child client with preconfigured opaqueId, can we?

delvedor · 2020-07-14T08:16:49Z

Not sure how to leverage that however, as we decided to get rid of our facade. We can't create a child client with preconfigured opaqueId, can we?

@pgayvallet correct, the global configuration is used as a prefix for the local configuration, and it should be handwritten each time. Anyhow, the client does not complain at all if you directly configure it in the headers, the only catch is that if you configure both headers and the opaqueId option, the option will take precedence (see here).

src/core/server/http/http_config.ts

joshdover · 2020-07-14T23:51:38Z

packages/kbn-config-schema/src/types/ip_type.ts

+
+export class IpType extends Type<string> {
+  constructor(options: IpOptions = { versions: ['ipv4', 'ipv6'] }) {
+    const schema = internals.string().ip({ version: options.versions, cidr: 'forbidden' });


cidr not needed right now, so I just set to forbidden. Can be exposed in the future if needed.

src/core/server/http/http_server.ts

joshdover · 2020-07-15T17:17:10Z

This will be on hold while I'm on PTO. It should not block any other Audit Logging work, it just needs to be completed for 7.10 FF

src/core/server/http/router/request.ts

elasticmachine · 2020-08-04T13:56:22Z

Pinging @elastic/kibana-platform (Team:Platform)

mikecote

Alerting code LGTM

thomheymann

Looks great, can't wait to use this in audit log 🥳

thomheymann · 2020-08-05T20:26:18Z

x-pack/plugins/audit_trail/server/types.ts

@@ -13,4 +13,5 @@ export interface AuditEvent {
  scope?: string;
  user?: string;
  space?: string;
+  requestId?: string;


In ECS audit log this will be logged as trace.id but I'd say don't worry about updating the schema for now. I am happy to implement this as part of audit log PR as it's just going to create merge conflicts.

src/core/server/http/http_server.test.ts

joshdover · 2020-08-14T20:57:36Z

@elasticmachine merge upstream

joshdover · 2020-08-17T18:38:46Z

@elasticmachine merge upstream

pgayvallet

Few nits and a question, but overall LGTM.

src/core/server/elasticsearch/client/cluster_client.ts

src/core/server/elasticsearch/legacy/cluster_client.ts

src/core/server/http/http_server.ts

pgayvallet · 2020-08-19T07:55:06Z

src/core/server/http/router/request.ts

+  /**
+   * A identifier to identify this request.
+   *
+   * @remarks
+   * Depending on the user's configuration, this value may be sourced from the
+   * incoming request's `X-Opaque-Id` header which is not guaranteed to be unique
+   * per request.
+   */
+  public readonly id: string;


This comment makes me wonder if we really have the correct approach by having a single property for the request 'id' and the 'x-opaque-id' that we forward to ES.

The risk I see here is that it would make our audit logging virtually 'useless' to debug a specific user scenario in a environment where the kibana instance is configured to allow x-opaque-id from requests and where the user would send a constant value for multiple requests, as all requests/log entries would have the same id in the logs.

This feels like an edge case though, so it's probably not worth changing everything just for that (or maybe this is even what we want?)

The risk I see here is that it would make our audit logging virtually 'useless' to debug a specific user scenario in a environment where the kibana instance is configured to allow x-opaque-id from requests and where the user would send a constant value for multiple requests, as all requests/log entries would have the same id in the logs.

I think we should treat this as an invalid configuration. I'm not sure how else we could properly support forwarding the header from the incoming request to ES without introducing this possibility. I think this case is kind of an edge case but it's hard to know for sure since it is so dependent on the infrastructure Kibana is running behind. Operators will have to explicitly turn on request ID forwarding from the incoming request, so I would hope that they also validate that the configuration is correct end-to-end with their proxy.

We could help the operator experience here by logging a warning message if we see the duplicate x-opaque-id headers being sent in the last N requests. @thomheymann do you have any thoughts? Should we add a warning like this for the MVP?

Merging as-is for now, but we can address this in a follow up PR if needed.

kibanamachine · 2020-08-19T21:24:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: ec9a96d

Build metrics

✅ unchanged

History

💔 Build #69192 failed 275b58c
💚 Build #68545 succeeded c5e7ce4
💔 Build #68243 failed 1d2c017
💚 Build #67317 succeeded d4815c0
💚 Build #67057 succeeded 1144ebe

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

…1019) Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

mshustov · 2020-08-20T10:02:45Z

src/core/server/http/http_tools.ts

+    (request.raw.req.socket?.remoteAddress &&
+      options.ipAllowlist.includes(request.raw.req.socket.remoteAddress))
+    ? request.headers['x-opaque-id'] ?? uuid.v4()
+    : uuid.v4();


Can we extract the id generation logic in a common place to use it here and in KibanaRequest contructor?

export function generateId() { return uuid.v4(); }

…75502) Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

These broke due to the recently merged code for reading request ID from the X-Opaque-Id header in #71019.

joshdover force-pushed the al/x-opaque-id branch from 7984ba3 to 18fe8bb Compare July 7, 2020 22:01

joshdover commented Jul 7, 2020

View reviewed changes

epixa mentioned this pull request Jul 8, 2020

Use new elasticsearch-js client in Elasticsearch service #35508

Closed

14 tasks

pgayvallet reviewed Jul 8, 2020

View reviewed changes

mshustov reviewed Jul 8, 2020

View reviewed changes

packages/kbn-utility-types/index.ts Outdated Show resolved Hide resolved

mshustov reviewed Jul 8, 2020

View reviewed changes

src/core/server/http/router/request.ts Outdated Show resolved Hide resolved

src/core/server/http/router/request.ts Outdated Show resolved Hide resolved

src/core/server/http/router/request.ts Outdated Show resolved Hide resolved

mshustov mentioned this pull request Jul 8, 2020

Beats Management plugin: migrate server-side code #70930

Merged

2 tasks

delvedor reviewed Jul 9, 2020

View reviewed changes

joshdover mentioned this pull request Jul 14, 2020

Use updated onPreAuth from Platform #71552

Merged

2 tasks

kobelb reviewed Jul 14, 2020

View reviewed changes

src/core/server/http/http_config.ts Outdated Show resolved Hide resolved

Add IP type to kbn/config-schema

6c20c08

joshdover force-pushed the al/x-opaque-id branch from 18fe8bb to cffd076 Compare July 14, 2020 23:50

joshdover commented Jul 14, 2020

View reviewed changes

src/core/server/http/http_server.ts Show resolved Hide resolved

Add support for reading request ID from X-Opaque-Id header

b638bcd

joshdover force-pushed the al/x-opaque-id branch from cffd076 to b638bcd Compare July 14, 2020 23:55

joshdover added 4 commits July 30, 2020 11:13

Merge remote-tracking branch 'upstream/master' into al/x-opaque-id

a6b970e

Default allowFromAnyIp to false

891f3a7

Fix fake requests in tests

7ee769f

Default to internal for fake requests

4eb455d

joshdover commented Jul 31, 2020

View reviewed changes

src/core/server/http/router/request.ts Outdated Show resolved Hide resolved

Merge remote-tracking branch 'upstream/master' into al/x-opaque-id

3f5db53

joshdover requested a review from thomheymann August 4, 2020 13:54

joshdover added release_note:enhancement Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.10.0 labels Aug 4, 2020

mikecote approved these changes Aug 5, 2020

View reviewed changes

thomheymann approved these changes Aug 5, 2020

View reviewed changes

joshdover added 3 commits August 6, 2020 14:43

Merge remote-tracking branch 'upstream/master' into al/x-opaque-id

a33b18f

Rename server.requestOpaqueId to server.requestId

1144ebe

Merge remote-tracking branch 'upstream/master' into al/x-opaque-id

d4815c0

kobelb mentioned this pull request Aug 13, 2020

Audit logging: phase 1 #54836

Closed

11 tasks

Merge branch 'master' into al/x-opaque-id

1d2c017

Merge branch 'master' into al/x-opaque-id

c5e7ce4

pgayvallet approved these changes Aug 19, 2020

View reviewed changes

joshdover added 3 commits August 19, 2020 13:04

Merge remote-tracking branch 'upstream/master' into al/x-opaque-id

12f35d8

Review nits

275b58c

rm unused import

ec9a96d

joshdover merged commit 51a80c6 into elastic:master Aug 19, 2020

joshdover deleted the al/x-opaque-id branch August 19, 2020 21:41

joshdover mentioned this pull request Aug 19, 2020

[7.x] Add support for reading request ID from X-Opaque-Id header (#71019) #75502

Merged

joshdover added a commit to joshdover/kibana that referenced this pull request Aug 19, 2020

Add support for reading request ID from X-Opaque-Id header (elastic#7…

2902564

…1019) Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

mshustov reviewed Aug 20, 2020

View reviewed changes

jportner added a commit that referenced this pull request Aug 22, 2020

Fix jest integration tests for import and resolve import errors

1e8a3bb

These broke due to the recently merged code for reading request ID from the X-Opaque-Id header in #71019.

mshustov mentioned this pull request Jul 1, 2021

Add execution context service #102039

Merged

2 tasks

pgomulka mentioned this pull request Dec 1, 2021

X-Opaque-ID contains UUID causing ES deduplication to fail #120124

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for reading request ID from X-Opaque-Id header #71019

Add support for reading request ID from X-Opaque-Id header #71019

joshdover commented Jul 7, 2020 •

edited

Loading

joshdover Jul 7, 2020

thomheymann Aug 5, 2020

pgayvallet commented Jul 8, 2020 •

edited

Loading

joshdover commented Jul 8, 2020

delvedor left a comment

joshdover commented Jul 13, 2020

pgayvallet commented Jul 13, 2020

delvedor commented Jul 14, 2020

joshdover Jul 14, 2020

joshdover commented Jul 15, 2020

elasticmachine commented Aug 4, 2020

mikecote left a comment

thomheymann left a comment

thomheymann Aug 5, 2020

joshdover commented Aug 14, 2020

joshdover commented Aug 17, 2020

pgayvallet left a comment

pgayvallet Aug 19, 2020

joshdover Aug 19, 2020

joshdover Aug 19, 2020

kibanamachine commented Aug 19, 2020

mshustov Aug 20, 2020 •

edited

Loading

Add support for reading request ID from X-Opaque-Id header #71019

Add support for reading request ID from X-Opaque-Id header #71019

Conversation

joshdover commented Jul 7, 2020 • edited Loading

Summary

Checklist

For maintainers

joshdover Jul 7, 2020

Choose a reason for hiding this comment

thomheymann Aug 5, 2020

Choose a reason for hiding this comment

pgayvallet commented Jul 8, 2020 • edited Loading

joshdover commented Jul 8, 2020

delvedor left a comment

Choose a reason for hiding this comment

joshdover commented Jul 13, 2020

pgayvallet commented Jul 13, 2020

delvedor commented Jul 14, 2020

joshdover Jul 14, 2020

Choose a reason for hiding this comment

joshdover commented Jul 15, 2020

elasticmachine commented Aug 4, 2020

mikecote left a comment

Choose a reason for hiding this comment

thomheymann left a comment

Choose a reason for hiding this comment

thomheymann Aug 5, 2020

Choose a reason for hiding this comment

joshdover commented Aug 14, 2020

joshdover commented Aug 17, 2020

pgayvallet left a comment

Choose a reason for hiding this comment

pgayvallet Aug 19, 2020

Choose a reason for hiding this comment

joshdover Aug 19, 2020

Choose a reason for hiding this comment

joshdover Aug 19, 2020

Choose a reason for hiding this comment

kibanamachine commented Aug 19, 2020

💚 Build Succeeded

Build metrics

History

mshustov Aug 20, 2020 • edited Loading

Choose a reason for hiding this comment

joshdover commented Jul 7, 2020 •

edited

Loading

pgayvallet commented Jul 8, 2020 •

edited

Loading

mshustov Aug 20, 2020 •

edited

Loading