Skip to content

Commit

Permalink
Revert "always enable tracing header injection for AWS requests (#4717)…
Browse files Browse the repository at this point in the history
…" (#4867)

- this reverts commit 1d2543c.
- reverts a change that would automatically inject tracing headers into AWS requests
- this appears to break S3 requests (and DynamoDB?) when using AWS SDK v2
  - we don't have any reports of other services or of AWS SDK v3 breaking
- for follow up work we need to make this a configurable environment variable instead of just an init setting
  - this is because folks using the lambda layer need to configure the tracer via env vars
  - alternatively we only block s3 and dynamo? however there could be other services that fail...
  - alternatively we only block aws sdk v2? however it seems that a bunch of the services are fine...
- internal stuff: APMS-13694, APMS-13713
- more discussion in #4717
  • Loading branch information
tlhunter authored and rochdev committed Nov 19, 2024
1 parent 2ba82f7 commit d724f21
Show file tree
Hide file tree
Showing 8 changed files with 454 additions and 24 deletions.
3 changes: 3 additions & 0 deletions docs/test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,9 @@ tracer.use('http', {
tracer.use('http', {
client: httpClientOptions
});
tracer.use('http', {
enablePropagationWithAmazonHeaders: true
});
tracer.use('http2');
tracer.use('http2', {
server: http2ServerOptions
Expand Down
8 changes: 8 additions & 0 deletions index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1043,6 +1043,14 @@ declare namespace tracer {
* @default code => code < 500
*/
validateStatus?: (code: number) => boolean;

/**
* Enable injection of tracing headers into requests signed with AWS IAM headers.
* Disable this if you get AWS signature errors (HTTP 403).
*
* @default false
*/
enablePropagationWithAmazonHeaders?: boolean;
}

/** @hidden */
Expand Down
22 changes: 0 additions & 22 deletions packages/datadog-plugin-aws-sdk/test/aws-sdk.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,28 +114,6 @@ describe('Plugin', () => {
s3.listBuckets({}, e => e && done(e))
})

// different versions of aws-sdk use different casings and different AWS headers
it('should include tracing headers and not cause a 403 error', (done) => {
const HttpClientPlugin = require('../../datadog-plugin-http/src/client.js')
const spy = sinon.spy(HttpClientPlugin.prototype, 'bindStart')
agent.use(traces => {
const headers = new Set(
Object.keys(spy.firstCall.firstArg.args.options.headers)
.map(x => x.toLowerCase())
)
spy.restore()

expect(headers).to.include('authorization')
expect(headers).to.include('x-amz-date')
expect(headers).to.include('x-datadog-trace-id')
expect(headers).to.include('x-datadog-parent-id')
expect(headers).to.include('x-datadog-sampling-priority')
expect(headers).to.include('x-datadog-tags')
}).then(done, done)

s3.listBuckets({}, e => e && done(e))
})

it('should mark error responses', (done) => {
let error

Expand Down
96 changes: 96 additions & 0 deletions packages/datadog-plugin-fetch/test/index.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,102 @@ describe('Plugin', () => {
})
})

it('should skip injecting if the Authorization header contains an AWS signature', done => {
const app = express()

app.get('/', (req, res) => {
try {
expect(req.get('x-datadog-trace-id')).to.be.undefined
expect(req.get('x-datadog-parent-id')).to.be.undefined

res.status(200).send()

done()
} catch (e) {
done(e)
}
})

appListener = server(app, port => {
fetch(`http://localhost:${port}/`, {
headers: {
Authorization: 'AWS4-HMAC-SHA256 ...'
}
})
})
})

it('should skip injecting if one of the Authorization headers contains an AWS signature', done => {
const app = express()

app.get('/', (req, res) => {
try {
expect(req.get('x-datadog-trace-id')).to.be.undefined
expect(req.get('x-datadog-parent-id')).to.be.undefined

res.status(200).send()

done()
} catch (e) {
done(e)
}
})

appListener = server(app, port => {
fetch(`http://localhost:${port}/`, {
headers: {
Authorization: ['AWS4-HMAC-SHA256 ...']
}
})
})
})

it('should skip injecting if the X-Amz-Signature header is set', done => {
const app = express()

app.get('/', (req, res) => {
try {
expect(req.get('x-datadog-trace-id')).to.be.undefined
expect(req.get('x-datadog-parent-id')).to.be.undefined

res.status(200).send()

done()
} catch (e) {
done(e)
}
})

appListener = server(app, port => {
fetch(`http://localhost:${port}/`, {
headers: {
'X-Amz-Signature': 'abc123'
}
})
})
})

it('should skip injecting if the X-Amz-Signature query param is set', done => {
const app = express()

app.get('/', (req, res) => {
try {
expect(req.get('x-datadog-trace-id')).to.be.undefined
expect(req.get('x-datadog-parent-id')).to.be.undefined

res.status(200).send()

done()
} catch (e) {
done(e)
}
})

appListener = server(app, port => {
fetch(`http://localhost:${port}/?X-Amz-Signature=abc123`)
})
})

it('should handle connection errors', done => {
let error

Expand Down
43 changes: 42 additions & 1 deletion packages/datadog-plugin-http/src/client.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ class HttpClientPlugin extends ClientPlugin {
span._spanContext._trace.record = false
}

if (this.config.propagationFilter(uri)) {
if (this.shouldInjectTraceHeaders(options, uri)) {
this.tracer.inject(span, HTTP_HEADERS, options.headers)
}

Expand All @@ -71,6 +71,18 @@ class HttpClientPlugin extends ClientPlugin {
return message.currentStore
}

shouldInjectTraceHeaders (options, uri) {
if (hasAmazonSignature(options) && !this.config.enablePropagationWithAmazonHeaders) {
return false
}

if (!this.config.propagationFilter(uri)) {
return false
}

return true
}

bindAsyncStart ({ parentStore }) {
return parentStore
}
Expand Down Expand Up @@ -200,6 +212,31 @@ function getHooks (config) {
return { request }
}

function hasAmazonSignature (options) {
if (!options) {
return false
}

if (options.headers) {
const headers = Object.keys(options.headers)
.reduce((prev, next) => Object.assign(prev, {
[next.toLowerCase()]: options.headers[next]
}), {})

if (headers['x-amz-signature']) {
return true
}

if ([].concat(headers.authorization).some(startsWith('AWS4-HMAC-SHA256'))) {
return true
}
}

const search = options.search || options.path

return search && search.toLowerCase().indexOf('x-amz-signature=') !== -1
}

function extractSessionDetails (options) {
if (typeof options === 'string') {
return new URL(options).host
Expand All @@ -211,4 +248,8 @@ function extractSessionDetails (options) {
return { host, port }
}

function startsWith (searchString) {
return value => String(value).startsWith(searchString)
}

module.exports = HttpClientPlugin
154 changes: 154 additions & 0 deletions packages/datadog-plugin-http/test/client.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,116 @@ describe('Plugin', () => {
})
})

it('should skip injecting if the Authorization header contains an AWS signature', done => {
const app = express()

app.get('/', (req, res) => {
try {
expect(req.get('x-datadog-trace-id')).to.be.undefined
expect(req.get('x-datadog-parent-id')).to.be.undefined

res.status(200).send()

done()
} catch (e) {
done(e)
}
})

appListener = server(app, port => {
const req = http.request({
port,
headers: {
Authorization: 'AWS4-HMAC-SHA256 ...'
}
})

req.end()
})
})

it('should skip injecting if one of the Authorization headers contains an AWS signature', done => {
const app = express()

app.get('/', (req, res) => {
try {
expect(req.get('x-datadog-trace-id')).to.be.undefined
expect(req.get('x-datadog-parent-id')).to.be.undefined

res.status(200).send()

done()
} catch (e) {
done(e)
}
})

appListener = server(app, port => {
const req = http.request({
port,
headers: {
Authorization: ['AWS4-HMAC-SHA256 ...']
}
})

req.end()
})
})

it('should skip injecting if the X-Amz-Signature header is set', done => {
const app = express()

app.get('/', (req, res) => {
try {
expect(req.get('x-datadog-trace-id')).to.be.undefined
expect(req.get('x-datadog-parent-id')).to.be.undefined

res.status(200).send()

done()
} catch (e) {
done(e)
}
})

appListener = server(app, port => {
const req = http.request({
port,
headers: {
'X-Amz-Signature': 'abc123'
}
})

req.end()
})
})

it('should skip injecting if the X-Amz-Signature query param is set', done => {
const app = express()

app.get('/', (req, res) => {
try {
expect(req.get('x-datadog-trace-id')).to.be.undefined
expect(req.get('x-datadog-parent-id')).to.be.undefined

res.status(200).send()

done()
} catch (e) {
done(e)
}
})

appListener = server(app, port => {
const req = http.request({
port,
path: '/?X-Amz-Signature=abc123'
})

req.end()
})
})

it('should run the callback in the parent context', done => {
const app = express()

Expand Down Expand Up @@ -983,6 +1093,50 @@ describe('Plugin', () => {
})
})

describe('with config enablePropagationWithAmazonHeaders enabled', () => {
let config

beforeEach(() => {
config = {
enablePropagationWithAmazonHeaders: true
}

return agent.load('http', config)
.then(() => {
http = require(pluginToBeLoaded)
express = require('express')
})
})

it('should inject tracing header into AWS signed request', done => {
const app = express()

app.get('/', (req, res) => {
try {
expect(req.get('x-datadog-trace-id')).to.be.a('string')
expect(req.get('x-datadog-parent-id')).to.be.a('string')

res.status(200).send()

done()
} catch (e) {
done(e)
}
})

appListener = server(app, port => {
const req = http.request({
port,
headers: {
Authorization: 'AWS4-HMAC-SHA256 ...'
}
})

req.end()
})
})
})

describe('with validateStatus configuration', () => {
let config

Expand Down
Loading

0 comments on commit d724f21

Please sign in to comment.