Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redact potentially sensitive information from error objects #76

Merged
merged 8 commits into from
Dec 8, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
"@types/sinonjs__fake-timers": "^8.1.2",
"@types/stoppable": "^1.1.1",
"@types/tap": "^15.0.7",
"@types/traverse": "^0.6.35",
"into-stream": "^6.0.0",
"license-checker": "^25.0.1",
"node-abort-controller": "^3.0.1",
Expand All @@ -61,6 +62,7 @@
"hpagent": "^1.0.0",
"ms": "^2.1.3",
"secure-json-parse": "^2.4.0",
"traverse": "^0.6.7",
"tslib": "^2.4.0",
"undici": "^5.22.1"
},
Expand Down
18 changes: 11 additions & 7 deletions src/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import * as http from 'http'
import { DiagnosticResult } from './types'
import { redactObject } from './security'

export class ElasticsearchClientError extends Error {
constructor (message: string) {
Expand All @@ -34,7 +35,7 @@ export class TimeoutError extends ElasticsearchClientError {
Error.captureStackTrace(this, TimeoutError)
this.name = 'TimeoutError'
this.message = message ?? 'Timeout Error'
this.meta = meta
this.meta = isObject(meta) ? redactObject(meta) as DiagnosticResult : meta
}
}

Expand All @@ -45,7 +46,7 @@ export class ConnectionError extends ElasticsearchClientError {
Error.captureStackTrace(this, ConnectionError)
this.name = 'ConnectionError'
this.message = message ?? 'Connection Error'
this.meta = meta
this.meta = isObject(meta) ? redactObject(meta) as DiagnosticResult : meta
}
}

Expand All @@ -56,7 +57,7 @@ export class NoLivingConnectionsError extends ElasticsearchClientError {
Error.captureStackTrace(this, NoLivingConnectionsError)
this.name = 'NoLivingConnectionsError'
this.message = message ?? 'Given the configuration, the ConnectionPool was not able to find a usable Connection for this request.'
this.meta = meta
this.meta = redactObject(meta) as DiagnosticResult
}
}

Expand All @@ -68,6 +69,7 @@ export class SerializationError extends ElasticsearchClientError {
this.name = 'SerializationError'
this.message = message ?? 'Serialization Error'
this.data = data
this.data = isObject(data) ? redactObject(data) : data
}
}

Expand Down Expand Up @@ -97,6 +99,7 @@ export class ResponseError extends ElasticsearchClientError {
super('Response Error')
Error.captureStackTrace(this, ResponseError)
this.name = 'ResponseError'

// TODO: this is for Elasticsearch
if (isObject(meta.body) && meta.body.error != null && meta.body.error.type != null) {
this.message = meta.body.error.type
Expand Down Expand Up @@ -126,7 +129,8 @@ export class ResponseError extends ElasticsearchClientError {
} else {
this.message = meta.body as string ?? 'Response Error'
}
this.meta = meta

this.meta = redactObject(meta) as DiagnosticResult
}

get body (): any | undefined {
Expand All @@ -152,7 +156,7 @@ export class RequestAbortedError extends ElasticsearchClientError {
Error.captureStackTrace(this, RequestAbortedError)
this.name = 'RequestAbortedError'
this.message = message ?? 'Request aborted'
this.meta = meta
this.meta = isObject(meta) ? redactObject(meta) as DiagnosticResult : meta
}
}

Expand All @@ -163,10 +167,10 @@ export class ProductNotSupportedError extends ElasticsearchClientError {
Error.captureStackTrace(this, ProductNotSupportedError)
this.name = 'ProductNotSupportedError'
this.message = `The client noticed that the server is not ${product} and we do not support this unknown product.`
this.meta = meta
this.meta = isObject(meta) ? redactObject(meta) as DiagnosticResult : meta
}
}

function isObject (obj: any): obj is Record<string, any> {
return typeof obj === 'object'
return typeof obj === 'object' && obj !== null
}
53 changes: 53 additions & 0 deletions src/security.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import traverse from 'traverse'

const secretKeys = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can a user customize this list?

'authorization',
'password',
'apikey',
'x-elastic-app-auth'
]

/**
* Clones an object and recursively loops through all keys, redacting their values if the key matches any of a list of strings.
* @param obj: Object to clone and redact
* @param additionalKeys: Extra keys that can be matched for redaction. Does not overwrite the default set.
*/
export function redactObject (obj: Record<string, any>, additionalKeys: string[] = []): Record<string, any> {
const toRedact = [...secretKeys, ...additionalKeys].map(key => key.toLowerCase())
return traverse(obj)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

traverse loops recursively through every key/value in any deeply-nested object, and can easily account for circular references (see line 47). I wanted both of these things so that this function could be generic enough to be used elsewhere later.

It was proposed that we could use fast-redact to implement this redaction feature. However, it uses wildcards to match a key string at multiple levels. I benchmarked it against my solution and, when using it to match keys at 5+ levels of nesting—DiagnosticResult includes HTTP headers nested 5 levels deep—it was substantially slower than using traverse, and did not account for circular references.

// ts-standard thinks this is Array.prototype.map but it isn't
.map(function (node) { // eslint-disable-line array-callback-return
// represent URLs as strings with no username/password
// @ts-expect-error current typedef for `pre` is inaccurate
this.pre(function (childNode, key) {
if (childNode instanceof URL) {
node[key] = `${childNode.origin}${childNode.pathname}${childNode.search}`
}
})

if (this.circular !== null && this.circular !== undefined) {
this.remove()
} else if (this.key !== undefined && toRedact.includes(this.key.toLowerCase())) {
this.update('[redacted]')
}
})
}
173 changes: 173 additions & 0 deletions test/unit/errors.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,173 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { test } from 'tap'
import { inspect } from 'node:util'
import { errors, DiagnosticResult } from '../../'
import { HttpConnection, UndiciConnection } from '../../'

const theSecret = '**foo-bar-baz-bat**'

function makeDiagnostics(): DiagnosticResult[] {
const diagnosticBase: DiagnosticResult = {
headers: {
'accept': 'text/plain',
'authorization': theSecret,
},
warnings: null,
meta: {
context: '',
name: 'foo',
request: {
params: {
method: 'get',
path: '/',
headers: {
authorization: theSecret,
}
},
options: {
headers: {
authorization: theSecret,
}
},
id: 'foo',
},
connection: null,
attempts: 1,
aborted: false
}
}

const diagnostics: DiagnosticResult[] = []
diagnostics.push(diagnosticBase)

const classes = [HttpConnection, UndiciConnection]
const auths = [
{ username: 'elastic', password: theSecret },
{ apiKey: theSecret },
{ apiKey: { id: theSecret, api_key: theSecret }},
{ bearer: theSecret },
]

classes.forEach(Conn => {
auths.forEach(auth => {
const diag = structuredClone(diagnosticBase)
diag.meta.connection = new Conn({
url: new URL(`http://user:${theSecret}@www.foo.com`),
auth,
})
diagnostics.push(diag)
})
})

return diagnostics

// TODO: figure out which of these could also contain secrets
// auth?: BasicAuth | ApiKeyAuth | BearerAuth;
// diagnostic?: Diagnostic;
// timeout?: number;
// agent?: HttpAgentOptions | UndiciAgentOptions | agentFn | boolean;
// proxy?: string | URL;
// caFingerprint?: string;
}

test('redact sensitive data when logging a TimeoutError', t => {
const diags = makeDiagnostics()

diags.forEach(diag => {
const err = new errors.TimeoutError('err', diag)
t.notMatch(inspect(err), theSecret, 'TimeoutError should redact sensitive data')
t.notMatch(inspect(err.meta), theSecret, 'TimeoutError should redact sensitive data')
t.notMatch(JSON.stringify(err.meta ?? ''), theSecret)
t.notMatch(err.meta?.toString(), theSecret)
})

t.end()
})

test('redact sensitive data when logging a ConnectionError', t => {
const diags = makeDiagnostics()

diags.forEach(diag => {
const err = new errors.ConnectionError('err', diag)
t.notMatch(inspect(err), theSecret, 'ConnectionError should redact sensitive data')
t.notMatch(inspect(err.meta), theSecret, 'ConnectionError should redact sensitive data')
t.notMatch(JSON.stringify(err.meta ?? ''), theSecret)
t.notMatch(err.meta?.toString(), theSecret)
})

t.end()
})

test('redact sensitive data when logging a NoLivingConnectionsError', t => {
const diags = makeDiagnostics()

diags.forEach(diag => {
const err = new errors.NoLivingConnectionsError('err', diag)
t.notMatch(inspect(err), theSecret, 'NoLivingConnectionsError should redact sensitive data')
t.notMatch(inspect(err.meta), theSecret, 'NoLivingConnectionsError should redact sensitive data')
t.notMatch(JSON.stringify(err.meta ?? ''), theSecret)
t.notMatch(err.meta?.toString(), theSecret)
})

t.end()
})

test('redact sensitive data when logging a ResponseError', t => {
const diags = makeDiagnostics()

diags.forEach(diag => {
const err = new errors.ResponseError(diag)
t.notMatch(inspect(err), theSecret, 'ResponseError should redact sensitive data')
t.notMatch(inspect(err.meta), theSecret, 'ResponseError should redact sensitive data')
t.notMatch(JSON.stringify(err.meta ?? ''), theSecret)
t.notMatch(err.meta?.toString(), theSecret)
})

t.end()
})

test('redact sensitive data when logging a RequestAbortedError', t => {
const diags = makeDiagnostics()

diags.forEach(diag => {
const err = new errors.RequestAbortedError('err', diag)
t.notMatch(inspect(err), theSecret, 'RequestAbortedError should redact sensitive data')
t.notMatch(inspect(err.meta), theSecret, 'RequestAbortedError should redact sensitive data')
t.notMatch(JSON.stringify(err.meta ?? ''), theSecret)
t.notMatch(err.meta?.toString(), theSecret)
})

t.end()
})

test('redact sensitive data when logging a ProductNotSupportedError', t => {
const diags = makeDiagnostics()

diags.forEach(diag => {
const err = new errors.ProductNotSupportedError('err', diag)
t.notMatch(inspect(err), theSecret, 'ProductNotSupportedError should redact sensitive data')
t.notMatch(inspect(err.meta), theSecret, 'ProductNotSupportedError should redact sensitive data')
t.notMatch(JSON.stringify(err.meta ?? ''), theSecret)
t.notMatch(err.meta?.toString(), theSecret)
})

t.end()
})
Loading