aws / aws-xray-sdk-node

The official AWS X-Ray SDK for Node.js.
Apache License 2.0
271 stars 155 forks source link

Remove `X-Amzn-Trace-Id` from outbound http/s requests to third parties #465

Closed mattfysh closed 3 years ago

mattfysh commented 3 years ago

After globally capturing the http and https native modules, all outbound requests are being instrumented with an additional X-Amzn-Trace-Id header

This is useful in some circumstances, e.g. when calling other AWS services where tracing is enabled so that the trace can continue through multiple services.

However when calling a third party external service, I would like to have this header removed, as I want to prevent this value from being delivered to the third party, but I would still like to trace the outbound call to trace e.g. errors and durations.

Here is the code I am using to setup the global http/s instrumentation:

// capture HTTP(s)
const subsegmentCallback = (subsegment, req) => {
  if (typeof req.reusedSocket === 'boolean') {
    subsegment.addAnnotation('reusedSocket', req.reusedSocket)
  }
}

const capture = lib =>
  AWSXRay.captureHTTPsGlobal(lib, false, subsegmentCallback)

capture(http)
capture(https)
AWSXRay.capturePromise()
mattfysh commented 3 years ago

I've got a workaround but it's not pretty:

const capture = lib => {
  AWSXRay.captureHTTPsGlobal(lib, false, subsegmentCallback)
  // wrap the original method stored and used by xray
  // remove the x-amzn-trace-id header if requested
  const origRequest = lib.__request
  Object.assign(lib, {
    __request: (...args) => {
      const [, opts] = args
      if (!opts?.suppressXrayHeader) {
        return origRequest(...args)
      }
      const removeHeader = Object.keys(opts.headers).find(
        h => h.toLowerCase() === 'x-amzn-trace-id'
      )
      const newOpts = {
        ...opts,
        headers: { ...opts.headers },
      }
      delete newOpts.suppressXrayHeader
      delete newOpts.headers[removeHeader]
      const newArgs = [args[0], newOpts, ...args.slice(2)]
      return origRequest(...newArgs)
    },
  })
}

And then elsewhere in the app I'm using, eg:

lib.request(url, {
  method: 'POST',
  headers: { /* ... */ },
  suppressXrayHeader: true
})
srprash commented 3 years ago

Hi @mattfysh The X-Amzn-Trace-Id is used to propagate the trace information to downstream remote services so that they can continue the current trace to provide end-to-end tracing. It is not possible for the SDK to determine if the remote service is third-party or not, so I don't know how we can implement this request. The X-Amzn-Trace-Id header contains the trace-id, parent-id, and the sampling decision, all of which are scoped to a single trace and are fairly harmless if exposed to a third party entity. That being said, is there a specific reason why exposing this information to third-party may not be a good idea?

I'm glad that you found a workaround at the moment. :)

mattfysh commented 3 years ago

It is not possible for the SDK to determine if the remote service is third-party or not

That's okay! I'm happy to add a flag in the spots where my application calls out to a third party service to say "Please trace this request (capture duration, return status code, etc) but do not send X-Amzn-Trace-Id header"

is there a specific reason why exposing this information to third-party may not be a good idea

I'm building a web data extraction service, and a lot of work is being done to ensure the outgoing request faithfully recreates a browser request, which wouldn't typically include tracing headers

I think it's also a good idea from a security perspective. Including this header partially reveals implementation details that are usually better kept under wraps.

willarmiros commented 3 years ago

@mattfysh here's where we set the header, if you'd like to make a PR to add an option to not include trace header we'd be happy to review: https://github.com/aws/aws-xray-sdk-node/blob/043be345d24dc537463a58999dd8651fefa12349/packages/core/lib/patchers/http_p.js#L127

mattfysh commented 3 years ago

Thanks @willarmiros :) will send through a PR in the next week or so. Cheers