fix(redis-4): omit credentials from db.connection_string span attribu…

…te (#1562) * fix(redis-4): omit credentials from db.connection_string span attribute * Reconstruct non-sensitive db.connection_string using built-in URL.href * style(redis-4): Fix code style according to community standards * fix(redis-4): Log error on connection string sanitization failure --------- Co-authored-by: Amir Blum <[email protected]> Co-authored-by: Haddas Bronfman <[email protected]>
open-telemetry · Aug 10, 2023 · ccf1efe · ccf1efe
1 parent f4df836
commit ccf1efe
Show file tree

Hide file tree

Showing 3 changed files with 95 additions and 5 deletions.
diff --git a/plugins/node/opentelemetry-instrumentation-redis-4/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-redis-4/src/instrumentation.ts
@@ -354,7 +354,7 @@ export class RedisInstrumentation extends InstrumentationBase<any> {
       return function patchedConnect(this: any): Promise<void> {
         const options = this.options;
 
-        const attributes = getClientAttributes(options);
+        const attributes = getClientAttributes(this._diag, options);
 
         const span = plugin.tracer.startSpan(
           `${RedisInstrumentation.COMPONENT}-connect`,
@@ -405,7 +405,7 @@ export class RedisInstrumentation extends InstrumentationBase<any> {
     const dbStatementSerializer =
       this._config?.dbStatementSerializer || defaultDbStatementSerializer;
 
-    const attributes = getClientAttributes(clientOptions);
+    const attributes = getClientAttributes(this._diag, clientOptions);
 
     try {
       const dbStatement = dbStatementSerializer(commandName, commandArgs);

diff --git a/plugins/node/opentelemetry-instrumentation-redis-4/src/utils.ts b/plugins/node/opentelemetry-instrumentation-redis-4/src/utils.ts
@@ -13,16 +13,45 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+import { DiagLogger } from '@opentelemetry/api';
 import {
   DbSystemValues,
   SemanticAttributes,
 } from '@opentelemetry/semantic-conventions';
 
-export function getClientAttributes(options: any) {
+export function getClientAttributes(diag: DiagLogger, options: any) {
   return {
     [SemanticAttributes.DB_SYSTEM]: DbSystemValues.REDIS,
     [SemanticAttributes.NET_PEER_NAME]: options?.socket?.host,
     [SemanticAttributes.NET_PEER_PORT]: options?.socket?.port,
-    [SemanticAttributes.DB_CONNECTION_STRING]: options?.url,
+    [SemanticAttributes.DB_CONNECTION_STRING]:
+      removeCredentialsFromDBConnectionStringAttribute(diag, options?.url),
   };
 }
+
+/**
+ * removeCredentialsFromDBConnectionStringAttribute removes basic auth from url and user_pwd from query string
+ *
+ * Examples:
+ *   redis://user:pass@localhost:6379/mydb => redis://localhost:6379/mydb
+ *   redis://localhost:6379?db=mydb&user_pwd=pass => redis://localhost:6379?db=mydb
+ */
+function removeCredentialsFromDBConnectionStringAttribute(
+  diag: DiagLogger,
+  url?: unknown
+): string | undefined {
+  if (typeof url !== 'string') {
+    return;
+  }
+
+  try {
+    const u = new URL(url);
+    u.searchParams.delete('user_pwd');
+    u.username = '';
+    u.password = '';
+    return u.href;
+  } catch (err) {
+    diag.error('failed to sanitize redis connection url', err);
+  }
+  return;
+}
diff --git a/plugins/node/opentelemetry-instrumentation-redis-4/test/redis.test.ts b/plugins/node/opentelemetry-instrumentation-redis-4/test/redis.test.ts
@@ -220,8 +220,11 @@ describe('redis@^4.0.0', () => {
     });
 
     it('sets error status on connection failure', async () => {
+      const redisURL = `redis://${redisTestConfig.host}:${
+        redisTestConfig.port + 1
+      }`;
       const newClient = createClient({
-        url: `redis://${redisTestConfig.host}:${redisTestConfig.port + 1}`,
+        url: redisURL,
       });
 
       await assert.rejects(newClient.connect());
@@ -230,6 +233,64 @@ describe('redis@^4.0.0', () => {
 
       assert.strictEqual(span.name, 'redis-connect');
       assert.strictEqual(span.status.code, SpanStatusCode.ERROR);
+      assert.strictEqual(
+        span.attributes[SemanticAttributes.DB_CONNECTION_STRING],
+        redisURL
+      );
+    });
+
+    it('omits basic auth from DB_CONNECTION_STRING span attribute', async () => {
+      const redisURL = `redis://myuser:mypassword@${redisTestConfig.host}:${
+        redisTestConfig.port + 1
+      }`;
+      const expectAttributeConnString = `redis://${redisTestConfig.host}:${
+        redisTestConfig.port + 1
+      }`;
+      const newClient = createClient({
+        url: redisURL,
+      });
+
+      await assert.rejects(newClient.connect());
+
+      const [span] = getTestSpans();
+
+      assert.strictEqual(span.name, 'redis-connect');
+      assert.strictEqual(span.status.code, SpanStatusCode.ERROR);
+      assert.strictEqual(
+        span.attributes[SemanticAttributes.NET_PEER_NAME],
+        redisTestConfig.host
+      );
+      assert.strictEqual(
+        span.attributes[SemanticAttributes.DB_CONNECTION_STRING],
+        expectAttributeConnString
+      );
+    });
+
+    it('omits user_pwd query parameter from DB_CONNECTION_STRING span attribute', async () => {
+      const redisURL = `redis://${redisTestConfig.host}:${
+        redisTestConfig.port + 1
+      }?db=mydb&user_pwd=mypassword`;
+      const expectAttributeConnString = `redis://${redisTestConfig.host}:${
+        redisTestConfig.port + 1
+      }?db=mydb`;
+      const newClient = createClient({
+        url: redisURL,
+      });
+
+      await assert.rejects(newClient.connect());
+
+      const [span] = getTestSpans();
+
+      assert.strictEqual(span.name, 'redis-connect');
+      assert.strictEqual(span.status.code, SpanStatusCode.ERROR);
+      assert.strictEqual(
+        span.attributes[SemanticAttributes.NET_PEER_NAME],
+        redisTestConfig.host
+      );
+      assert.strictEqual(
+        span.attributes[SemanticAttributes.DB_CONNECTION_STRING],
+        expectAttributeConnString
+      );
     });
   });