Merge pull request #1870 from spotify/rugvip/taint-config

config-loader: taint secret paths to treat them as secret in later configs as well

packages/config-loader/src/lib/reader.test.ts

@@ -26,6 +26,13 @@ function memoryFiles(files: { [path: string]: string }) {
   };
 }
 
+const mockContext: ReaderContext = {
+  env: {},
+  skip: () => false,
+  readFile: jest.fn(),
+  readSecret: jest.fn(),
+};
+
 describe('readConfigFile', () => {
   it('should read a plain config file', async () => {
     const readFile = memoryFiles({
@@ -34,8 +41,9 @@ describe('readConfigFile', () => {
     });
 
     const config = readConfigFile('./app-config.yaml', {
+      ...mockContext,
       readFile,
-    } as ReaderContext);
+    });
 
     await expect(config).resolves.toEqual({
       data: {
@@ -55,8 +63,9 @@ describe('readConfigFile', () => {
     });
 
     const config = readConfigFile('./app-config.yaml', {
+      ...mockContext,
       readFile,
-    } as ReaderContext);
+    });
 
     await expect(config).rejects.toThrow('Flow map contains an unexpected ]');
   });
@@ -67,8 +76,9 @@ describe('readConfigFile', () => {
     });
 
     const config = readConfigFile('./app-config.yaml', {
+      ...mockContext,
       readFile,
-    } as ReaderContext);
+    });
 
     await expect(config).rejects.toThrow('Expected object at config root');
   });
@@ -80,7 +90,7 @@ describe('readConfigFile', () => {
     const readSecret = jest.fn().mockResolvedValue('secret');
 
     const config = readConfigFile('./app-config.yaml', {
-      env: {},
+      ...mockContext,
       readFile,
       readSecret: readSecret as ReadSecretFunc,
     });
@@ -91,7 +101,9 @@ describe('readConfigFile', () => {
       },
       context: 'app-config.yaml',
     });
-    expect(readSecret).toHaveBeenCalledWith({ file: './my-secret' });
+    expect(readSecret).toHaveBeenCalledWith('.app', {
+      file: './my-secret',
+    });
   });
 
   it('should require secrets to be objects', async () => {
@@ -101,7 +113,7 @@ describe('readConfigFile', () => {
     const readSecret = jest.fn().mockResolvedValue('secret');
 
     const config = readConfigFile('./app-config.yaml', {
-      env: {},
+      ...mockContext,
       readFile,
       readSecret: readSecret as ReadSecretFunc,
     });
@@ -119,11 +131,29 @@ describe('readConfigFile', () => {
     const readSecret = jest.fn().mockRejectedValue(new Error('NOPE'));
 
     const config = readConfigFile('./app-config.yaml', {
-      env: {},
+      ...mockContext,
       readFile,
       readSecret: readSecret as ReadSecretFunc,
     });
 
     await expect(config).rejects.toThrow('Invalid secret at .app: NOPE');
   });
+
+  it('should omit skipped values', async () => {
+    const readFile = memoryFiles({
+      './app-config.yaml': 'app: { title: skip, name: include }',
+    });
+
+    const config = readConfigFile('./app-config.yaml', {
+      ...mockContext,
+      readFile,
+      skip: (path: string) => path === '.app.title',
+      readSecret: jest.fn() as ReadSecretFunc,
+    });
+
+    await expect(config).resolves.toEqual({
+      context: 'app-config.yaml',
+      data: { app: { name: 'include' } },
+    });
+  });
 });

packages/config-loader/src/lib/reader.ts

@@ -35,6 +35,10 @@ export async function readConfigFile(
     obj: JsonValue,
     path: string,
   ): Promise<JsonValue | undefined> {
+    if (ctx.skip(path)) {
+      return undefined;
+    }
+
     if (typeof obj !== 'object') {
       return obj;
     } else if (obj === null) {
@@ -58,7 +62,7 @@ export async function readConfigFile(
       }
 
       try {
-        return await ctx.readSecret(obj.$secret);
+        return await ctx.readSecret(path, obj.$secret);
       } catch (error) {
         throw new Error(`Invalid secret at ${path}: ${error.message}`);
       }

packages/config-loader/src/lib/secrets.test.ts

@@ -21,6 +21,7 @@ const ctx: ReaderContext = {
   env: {
     SECRET: 'my-secret',
   },
+  skip: () => false,
   readSecret: jest.fn(),
   async readFile(path) {
     const content = ({

packages/config-loader/src/lib/types.ts

@@ -17,13 +17,18 @@
 import { JsonObject } from '@backstage/config';
 
 export type ReadFileFunc = (path: string) => Promise<string>;
-export type ReadSecretFunc = (desc: JsonObject) => Promise<string | undefined>;
+export type ReadSecretFunc = (
+  path: string,
+  desc: JsonObject,
+) => Promise<string | undefined>;
+export type SkipFunc = (path: string) => boolean;
 
 /**
  * Common context that provides all the necessary hooks for reading configuration files.
  */
 export type ReaderContext = {
   env: { [name in string]?: string };
+  skip: SkipFunc;
   readFile: ReadFileFunc;
   readSecret: ReadSecretFunc;
 };

packages/config-loader/src/loader.test.ts

@@ -0,0 +1,140 @@
+/*
+ * Copyright 2020 Spotify AB
+ *
+ * Licensed 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 { loadConfig } from './loader';
+
+jest.mock('fs-extra', () => {
+  const mockFiles: { [path in string]: string } = {
+    '/root/app-config.yaml': `
+      app:
+        title: Example App
+        sessionKey:
+          $secret:
+            file: secrets/session-key.txt
+    `,
+    '/root/app-config.development.yaml': `
+      app:
+        sessionKey: development-key
+    `,
+    '/root/secrets/session-key.txt': 'abc123',
+  };
+
+  return {
+    async readFile(path: string) {
+      if (path in mockFiles) {
+        return mockFiles[path];
+      }
+      throw new Error(`File not found, ${path}`);
+    },
+    async pathExists(path: string) {
+      return path in mockFiles;
+    },
+  };
+});
+
+describe('loadConfig', () => {
+  it('loads config without secrets', async () => {
+    await expect(
+      loadConfig({
+        rootPaths: ['/root'],
+        env: 'production',
+        shouldReadSecrets: false,
+      }),
+    ).resolves.toEqual([
+      {
+        context: 'app-config.yaml',
+        data: {
+          app: {
+            title: 'Example App',
+          },
+        },
+      },
+    ]);
+  });
+
+  it('loads config with secrets', async () => {
+    await expect(
+      loadConfig({
+        rootPaths: ['/root'],
+        env: 'production',
+        shouldReadSecrets: true,
+      }),
+    ).resolves.toEqual([
+      {
+        context: 'app-config.yaml',
+        data: {
+          app: {
+            title: 'Example App',
+            sessionKey: 'abc123',
+          },
+        },
+      },
+    ]);
+  });
+
+  it('loads development config without secrets', async () => {
+    await expect(
+      loadConfig({
+        rootPaths: ['/root'],
+        env: 'development',
+        shouldReadSecrets: false,
+      }),
+    ).resolves.toEqual([
+      {
+        context: 'app-config.yaml',
+        data: {
+          app: {
+            title: 'Example App',
+          },
+        },
+      },
+      {
+        context: 'app-config.development.yaml',
+        data: {
+          app: {},
+        },
+      },
+    ]);
+  });
+
+  it('loads development config with secrets', async () => {
+    await expect(
+      loadConfig({
+        rootPaths: ['/root'],
+        env: 'development',
+        shouldReadSecrets: true,
+      }),
+    ).resolves.toEqual([
+      {
+        context: 'app-config.yaml',
+        data: {
+          app: {
+            title: 'Example App',
+            sessionKey: 'abc123',
+          },
+        },
+      },
+      {
+        context: 'app-config.development.yaml',
+        data: {
+          app: {
+            sessionKey: 'development-key',
+          },
+        },
+      },
+    ]);
+  });
+});

packages/config-loader/src/loader.ts

@@ -38,6 +38,7 @@ export type LoadConfigOptions = {
 class Context {
   constructor(
     private readonly options: {
+      secretPaths: Set<string>;
       env: { [name in string]?: string };
       rootPath: string;
       shouldReadSecrets: boolean;
@@ -48,11 +49,22 @@ class Context {
     return this.options.env;
   }
 
+  skip(path: string): boolean {
+    if (this.options.shouldReadSecrets) {
+      return false;
+    }
+    return this.options.secretPaths.has(path);
+  }
+
   async readFile(path: string): Promise<string> {
     return fs.readFile(resolvePath(this.options.rootPath, path), 'utf8');
   }
 
-  async readSecret(desc: JsonObject): Promise<string | undefined> {
+  async readSecret(
+    path: string,
+    desc: JsonObject,
+  ): Promise<string | undefined> {
+    this.options.secretPaths.add(path);
     if (!this.options.shouldReadSecrets) {
       return undefined;
     }
@@ -69,10 +81,13 @@ export async function loadConfig(
   const configPaths = await resolveStaticConfig(options);
 
   try {
+    const secretPaths = new Set<string>();
+
     for (const configPath of configPaths) {
       const config = await readConfigFile(
         configPath,
         new Context({
+          secretPaths,
           env: process.env,
           rootPath: dirname(configPath),
           shouldReadSecrets: Boolean(options.shouldReadSecrets),