From 19f2056dbee7b526356377bab10273e14dec3283 Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Tue, 16 Jul 2024 17:22:43 +0200 Subject: [PATCH 01/11] feat(container): first-class BuildKit secrets support First-class BuildKit secrets support for BuildKit in-cluster building, Garden Cloud Builder and building locally. Kaniko is not supported due to lack of support (See also https://github.com/GoogleContainerTools/kaniko/issues/3028) --- core/src/plugins/container/build.ts | 38 ++++++++- core/src/plugins/container/config.ts | 28 ++++++- core/src/plugins/container/helpers.ts | 4 +- .../kubernetes/container/build/buildkit.ts | 11 ++- .../kubernetes/container/build/kaniko.ts | 18 ++++- core/src/plugins/kubernetes/jib-container.ts | 1 + core/src/plugins/kubernetes/run.ts | 10 +-- core/src/plugins/kubernetes/sync.ts | 6 +- core/src/util/escape.ts | 50 +++++++++--- core/test/unit/src/plugins/container/build.ts | 72 ++++++++++++++++- core/test/unit/src/util/escape.ts | 77 ++++++++++++++++--- .../reference/action-types/Build/container.md | 28 +++++++ .../action-types/Build/jib-container.md | 28 +++++++ docs/reference/module-types/container.md | 35 +++++++++ docs/reference/module-types/jib-container.md | 35 +++++++++ 15 files changed, 402 insertions(+), 39 deletions(-) diff --git a/core/src/plugins/container/build.ts b/core/src/plugins/container/build.ts index 50b0c2149b..049b7252ed 100644 --- a/core/src/plugins/container/build.ts +++ b/core/src/plugins/container/build.ts @@ -11,7 +11,7 @@ import { ConfigurationError, toGardenError } from "../../exceptions.js" import type { PrimitiveMap } from "../../config/common.js" import split2 from "split2" import type { BuildActionHandler } from "../../plugin/action-types.js" -import type { ContainerBuildAction, ContainerBuildOutputs } from "./config.js" +import type { ContainerBuildAction, ContainerBuildActionSpec, ContainerBuildOutputs } from "./config.js" import { defaultDockerfileName } from "./config.js" import { joinWithPosix } from "../../util/fs.js" import type { Resolved } from "../../actions/types.js" @@ -153,6 +153,9 @@ async function buildContainerLocally({ const dockerFlags = [...getDockerBuildFlags(action, ctx.provider.config), ...extraDockerOpts] + const { secretArgs, secretEnvVars } = getDockerSecrets(action.getSpec()) + dockerFlags.push(...secretArgs) + // If there already is a --tag flag, another plugin like the Kubernetes plugin already decided how to tag the image. // In this case, we don't want to add another local tag. // TODO: it would be nice to find a better way to become aware of the parent plugin's concerns in the container plugin. @@ -175,6 +178,7 @@ async function buildContainerLocally({ stderr: outputStream, timeout, ctx, + env: secretEnvVars, }) } catch (e) { const error = toGardenError(e) @@ -262,6 +266,38 @@ export function getContainerBuildActionOutputs(action: Resolved +} { + const args: string[] = [] + const env: Record = {} + + for (const [secretKey, secretValue] of Object.entries(actionSpec.secrets || {})) { + if (!secretKey.match(/^[a-zA-Z0-9\._-]+$/)) { + throw new ConfigurationError({ + message: `Invalid secret ID '${secretKey}'. Only alphanumeric characters (a-z, A-Z, 0-9), underscores (_), dashes (-) and dots (.) are allowed.`, + }) + } + + // determine env var names. There can be name collisions due to the fact that we replace special characters with underscores. + let envVarname: string + let i = 1 + do { + envVarname = `GARDEN_BUILD_SECRET_${secretKey.toUpperCase().replaceAll(/[-\.]/g, "_")}${i > 1 ? `_${i}` : ""}` + i += 1 + } while (env[envVarname]) + + env[envVarname] = secretValue + args.push("--secret", `id=${secretKey},env=${envVarname}`) + } + + return { + secretArgs: args, + secretEnvVars: env, + } +} + export function getDockerBuildFlags( action: Resolved, containerProviderConfig: ContainerProviderConfig diff --git a/core/src/plugins/container/config.ts b/core/src/plugins/container/config.ts index 6f0c4486f1..adc395d1bc 100644 --- a/core/src/plugins/container/config.ts +++ b/core/src/plugins/container/config.ts @@ -1013,6 +1013,7 @@ export interface ContainerBuildActionSpec { buildArgs: PrimitiveMap dockerfile: string extraFlags: string[] + secrets?: Record localId?: string publishId?: string targetStage?: string @@ -1061,9 +1062,30 @@ export const containerCommonBuildSpecKeys = memoize(() => ({ Specify extra flags to use when building the container image. Note that arguments may not be portable across implementations.`), platforms: joi.sparseArray().items(joi.string()).description(dedent` - Specify the platforms to build the image for. This is useful when building multi-platform images. - The format is \`os/arch\`, e.g. \`linux/amd64\`, \`linux/arm64\`, etc. - `), + Specify the platforms to build the image for. This is useful when building multi-platform images. + The format is \`os/arch\`, e.g. \`linux/amd64\`, \`linux/arm64\`, etc. + `), + secrets: joi + .object() + .pattern(/.+/, joi.string()) + .description( + dedent` + Specify secret values that can be mounted during the build process but become part of the resulting image filesystem or image manifest, for example private registry auth tokens. + + Build arguments and environment variables are inappropriate for secrets, as they persist in the final image. + + The secret can later be consumed in the Dockerfile like so: + \`\`\` + RUN --mount=type=secret,id=mytoken \ + TOKEN=$(cat /run/secrets/mytoken) ... + \`\`\` + + See also https://docs.docker.com/build/building/secrets/ + ` + ) + .example({ + mytoken: "supersecret", + }), })) export const containerBuildSpecSchema = createSchema({ diff --git a/core/src/plugins/container/helpers.ts b/core/src/plugins/container/helpers.ts index 528b3b4d70..115f04ef4c 100644 --- a/core/src/plugins/container/helpers.ts +++ b/core/src/plugins/container/helpers.ts @@ -382,6 +382,7 @@ const helpers = { stdout, stderr, timeout, + env, }: { cwd: string args: string[] @@ -391,6 +392,7 @@ const helpers = { stdout?: Writable stderr?: Writable timeout?: number + env?: { [key: string]: string } }) { const docker = ctx.tools["container.docker"] @@ -398,7 +400,7 @@ const helpers = { const res = await docker.spawnAndWait({ args, cwd, - env: { ...process.env, DOCKER_CLI_EXPERIMENTAL: "enabled" }, + env, ignoreError, log, stdout, diff --git a/core/src/plugins/kubernetes/container/build/buildkit.ts b/core/src/plugins/kubernetes/container/build/buildkit.ts index a6e90c3d66..2357f6061d 100644 --- a/core/src/plugins/kubernetes/container/build/buildkit.ts +++ b/core/src/plugins/kubernetes/container/build/buildkit.ts @@ -36,7 +36,7 @@ import { import { getNamespaceStatus } from "../../namespace.js" import { sleep } from "../../../../util/util.js" import type { ContainerBuildAction, ContainerModuleOutputs } from "../../../container/moduleConfig.js" -import { getDockerBuildArgs } from "../../../container/build.js" +import { getDockerBuildArgs, getDockerSecrets } from "../../../container/build.js" import type { Resolved } from "../../../../actions/types.js" import { PodRunner } from "../../run.js" import { prepareSecrets } from "../../secrets.js" @@ -280,6 +280,8 @@ export function makeBuildkitBuildCommand({ contextPath: string dockerfile: string }): string[] { + const { secretArgs, secretEnvVars } = getDockerSecrets(action.getSpec()) + const buildctlCommand = [ "buildctl", "build", @@ -290,6 +292,7 @@ export function makeBuildkitBuildCommand({ "dockerfile=" + contextPath, "--opt", "filename=" + dockerfile, + ...secretArgs, ...getBuildkitImageFlags( provider.config.clusterBuildkit!.cache, outputs, @@ -298,7 +301,11 @@ export function makeBuildkitBuildCommand({ ...getBuildkitFlags(action), ] - return ["sh", "-c", `cd ${contextPath} && ${commandListToShellScript(buildctlCommand)}`] + return [ + "sh", + "-c", + `cd ${contextPath} && ${commandListToShellScript({ command: buildctlCommand, env: secretEnvVars })}`, + ] } export function getBuildkitFlags(action: Resolved) { diff --git a/core/src/plugins/kubernetes/container/build/kaniko.ts b/core/src/plugins/kubernetes/container/build/kaniko.ts index ee4b2d7a6a..961d49f673 100644 --- a/core/src/plugins/kubernetes/container/build/kaniko.ts +++ b/core/src/plugins/kubernetes/container/build/kaniko.ts @@ -91,6 +91,18 @@ export const kanikoBuild: BuildHandler = async (params) => { const projectNamespace = (await getNamespaceStatus({ log, ctx: k8sCtx, provider })).namespaceName const spec = action.getSpec() + + if (spec.secrets) { + throw new ConfigurationError({ + message: dedent` + Unfortunately Kaniko does not support secret build arguments. + Garden Cloud Builder and the Kubernetes BuildKit in-cluster builder both support secrets. + + See also https://github.com/GoogleContainerTools/kaniko/issues/3028 + `, + }) + } + const outputs = k8sGetContainerBuildActionOutputs({ provider, action }) const localId = outputs.localImageId @@ -318,7 +330,7 @@ export function getKanikoBuilderPodManifest({ n=0 until [ "$n" -ge 30 ] do - rsync ${commandListToShellScript(syncArgs)} && break + rsync ${commandListToShellScript({ command: syncArgs })} && break n=$((n+1)) sleep 1 done @@ -352,9 +364,9 @@ export function getKanikoBuilderPodManifest({ "/bin/sh", "-c", dedent` - ${commandListToShellScript(kanikoCommand)}; + ${commandListToShellScript({ command: kanikoCommand })}; export exitcode=$?; - ${commandListToShellScript(["touch", `${sharedMountPath}/done`])}; + ${commandListToShellScript({ command: ["touch", `${sharedMountPath}/done`] })}; exit $exitcode; `, ], diff --git a/core/src/plugins/kubernetes/jib-container.ts b/core/src/plugins/kubernetes/jib-container.ts index a20e354cfe..0f426d5438 100644 --- a/core/src/plugins/kubernetes/jib-container.ts +++ b/core/src/plugins/kubernetes/jib-container.ts @@ -74,6 +74,7 @@ async function buildAndPushViaRemote(params: BuildActionParams<"build", Containe // Build the tarball with the base handler const spec: any = action.getSpec() + spec.tarOnly = true spec.tarFormat = "oci" diff --git a/core/src/plugins/kubernetes/run.ts b/core/src/plugins/kubernetes/run.ts index 10abb8ade0..bbfc6a2e4d 100644 --- a/core/src/plugins/kubernetes/run.ts +++ b/core/src/plugins/kubernetes/run.ts @@ -455,7 +455,7 @@ exec 2<&- exec 1<>/tmp/output exec 2>&1 -${commandListToShellScript(cmd)} +${commandListToShellScript({ command: cmd })} ` } @@ -469,20 +469,20 @@ ${commandListToShellScript(cmd)} */ function getArtifactsTarScript(artifacts: ArtifactSpec[]) { const directoriesToCreate = artifacts.map((a) => a.target).filter((target) => !!target && target !== ".") - const tmpPath = commandListToShellScript(["/tmp/.garden-artifacts-" + randomString(8)]) + const tmpPath = commandListToShellScript({ command: ["/tmp/.garden-artifacts-" + randomString(8)] }) const createDirectoriesCommands = directoriesToCreate.map((target) => - commandListToShellScript(["mkdir", "-p", target]) + commandListToShellScript({ command: ["mkdir", "-p", target] }) ) const copyArtifactsCommands = artifacts.map(({ source, target }) => { - const escapedTarget = commandListToShellScript([target || "."]) + const escapedTarget = commandListToShellScript({ command: [target || "."] }) // Allow globs (*) in the source path // Note: This works because `commandListToShellScript` wraps every parameter in single quotes, escaping contained single quotes. // The string `bin/*` will be transformed to `'bin/*'` by `commandListToShellScript`. The shell would treat `*` as literal and not expand it. // `replaceAll` transforms that string then to `'bin/'*''`, which allows the shell to expand the glob, everything else is treated as literal. - const escapedSource = commandListToShellScript([source]).replaceAll("*", "'*'") + const escapedSource = commandListToShellScript({ command: [source] }).replaceAll("*", "'*'") return `cp -r ${escapedSource} ${escapedTarget} >/dev/null || true` }) diff --git a/core/src/plugins/kubernetes/sync.ts b/core/src/plugins/kubernetes/sync.ts index 62448b4255..5021bff8ce 100644 --- a/core/src/plugins/kubernetes/sync.ts +++ b/core/src/plugins/kubernetes/sync.ts @@ -464,7 +464,11 @@ export async function configureSyncMode({ const initContainer = { name: "garden-dev-init", image: k8sSyncUtilImageName, - command: ["/bin/sh", "-c", commandListToShellScript(["cp", "/usr/local/bin/mutagen-agent", mutagenAgentPath])], + command: [ + "/bin/sh", + "-c", + commandListToShellScript({ command: ["cp", "/usr/local/bin/mutagen-agent", mutagenAgentPath] }), + ], imagePullPolicy: "IfNotPresent", volumeMounts: [gardenVolumeMount], } diff --git a/core/src/util/escape.ts b/core/src/util/escape.ts index 77afccc1ce..7879b3b217 100644 --- a/core/src/util/escape.ts +++ b/core/src/util/escape.ts @@ -6,47 +6,77 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +import { InternalError } from "../exceptions.js" + /** * Wraps every parameter in single quotes, escaping contained single quotes (for use in bash scripts). Joins the elements with a space character. * * Examples: * * // returns `echo 'hello world'` - * commandListToShellScript(["echo", "hello world"]) + * commandListToShellScript({ command: ["echo", "hello world"] }) * * // returns `echo 'hello'"'"'world'` - * commandListToShellScript(["echo", "hello'world"]) + * commandListToShellScript({ command: ["echo", "hello'world"] }) * * // returns `echo ''"'"'; exec ls /'` - * commandListToShellScript(["echo", "'; exec ls /"]) + * commandListToShellScript({ command: ["echo", "'; exec ls /"] }) * * Caveat: This is only safe if the command is directly executed. It is not safe, if you wrap the output of this in double quotes, for instance. * * // SAFE - * exec(["sh", "-c", ${commandListToShellScript(["some", "command", "--with" untrustedInput])}]) + * exec(["sh", "-c", ${commandListToShellScript({ command: ["some", "command", "--with" untrustedInput] })}]) * exec(["sh", "-c", dedent` * set -e * echo "running command..." - * ${commandListToShellScript(["some", "command", "--with" untrustedInput])} + * ${commandListToShellScript({ command: ["some", "command", "--with" untrustedInput] })} * echo "done" * `]) * * // UNSAFE! don't do this * - * const commandWithUntrustedInput = commandListToShellScript(["some", "command", "--with" untrustedInput]) - * exec(["sh", "-c", `some_var="${commandWithUntrustedInput}"; echo "$some_var"`]) + * const UNSAFE_commandWithUntrustedInput = commandListToShellScript({ command: ["some", "UNSAFE", "command", "--with" untrustedInput] }) + * exec(["sh", "-c", `UNSAFE_some_var="${UNSAFE_commandWithUntrustedInput}"; echo "$UNSAFE_some_var"`]) * * The second is UNSAFE, because we can't know that the /double quotes/ need to be escaped here. * * If you can, use environment variables instead of this, to pass untrusted values to shell scripts, e.g. if you do not need to construct a command with untrusted input. * - * // SAFE + * // SAFE (preferred, if possible) * * exec(["sh", "-c", `some_var="$UNTRUSTED_INPUT"; echo "$some_var"`], { env: { UNTRUSTED_INPUT: untrustedInput } }) * + * // ALSO SAFE + * + * exec([ + * "sh", + * "-c", + * commandListToShellScript({ + * command: ["some", "command", "--with" untrustedInput], + * env: { UNTRUSTED_ENV_VAR: "moreUntrustedInput" }, + * }), + * ]) + * * @param command array of command line arguments * @returns string to be used as shell script statement to execute the given command. */ -export function commandListToShellScript(command: string[]) { - return command.map((c) => `'${c.replaceAll("'", `'"'"'`)}'`).join(" ") +export function commandListToShellScript({ command, env }: { command: string[]; env?: Record }) { + const wrapInSingleQuotes = (s: string) => `'${s.replaceAll("'", `'"'"'`)}'` + + const escapedCommand = command.map(wrapInSingleQuotes).join(" ") + + const envVars = Object.entries(env || {}) + const escapedEnv = + envVars.length > 0 + ? envVars.map(([k, v]) => { + if (!k.match(/^[0-9a-zA-Z_]+$/)) { + throw new InternalError({ + message: `Invalid environment variable name ${k}. Alphanumeric letters and underscores are allowed.`, + }) + } + return `${k}=${wrapInSingleQuotes(v)}` + }) + : undefined + + return `${escapedEnv ? `${escapedEnv} ` : ""}${escapedCommand}` } diff --git a/core/test/unit/src/plugins/container/build.ts b/core/test/unit/src/plugins/container/build.ts index 324ed7c194..6c21a09b52 100644 --- a/core/test/unit/src/plugins/container/build.ts +++ b/core/test/unit/src/plugins/container/build.ts @@ -14,7 +14,11 @@ import type { ConfigGraph } from "../../../../../src/graph/config-graph.js" import type { ActionLog, Log } from "../../../../../src/logger/log-entry.js" import { createActionLog } from "../../../../../src/logger/log-entry.js" import type { PluginContext } from "../../../../../src/plugin-context.js" -import { buildContainer, getContainerBuildStatus } from "../../../../../src/plugins/container/build.js" +import { + buildContainer, + getContainerBuildStatus, + getDockerSecrets, +} from "../../../../../src/plugins/container/build.js" import type { ContainerProvider } from "../../../../../src/plugins/container/container.js" import { gardenPlugin } from "../../../../../src/plugins/container/container.js" import { containerHelpers } from "../../../../../src/plugins/container/helpers.js" @@ -23,6 +27,7 @@ import type { TestGarden } from "../../../../helpers.js" import { getDataDir, makeTestGarden } from "../../../../helpers.js" import fsExtra from "fs-extra" const { createFile } = fsExtra +import { type ContainerBuildActionSpec } from "../../../../../src/plugins/container/config.js" context("build.ts", () => { const projectRoot = getDataDir("test-project-container") @@ -64,6 +69,71 @@ context("build.ts", () => { }) }) + describe("getDockerSecrets", () => { + const baseSpec: ContainerBuildActionSpec = { + buildArgs: {}, + extraFlags: [], + dockerfile: "Dockerfile", + secrets: undefined, + } + + it("returns empty list of args when no secrets are declared", () => { + const { secretArgs, secretEnvVars } = getDockerSecrets(baseSpec) + expect(secretArgs).to.eql([]) + expect(secretEnvVars).to.eql({}) + }) + + it("returns correct args and env vars when secrets have been declared", () => { + const { secretArgs, secretEnvVars } = getDockerSecrets({ + ...baseSpec, + secrets: { + "api-key.fruit-ninja.company.com": "banana", + }, + }) + expect(secretArgs).to.eql([ + "--secret", + "id=api-key.fruit-ninja.company.com,env=GARDEN_BUILD_SECRET_API_KEY_FRUIT_NINJA_COMPANY_COM", + ]) + expect(secretEnvVars).to.eql({ + GARDEN_BUILD_SECRET_API_KEY_FRUIT_NINJA_COMPANY_COM: "banana", + }) + }) + + it("handles ambiguous env var names", () => { + const { secretArgs, secretEnvVars } = getDockerSecrets({ + ...baseSpec, + secrets: { + "api-key": "banana", + "api_key": "apple", + }, + }) + expect(secretArgs).to.eql([ + "--secret", + "id=api-key,env=GARDEN_BUILD_SECRET_API_KEY", + "--secret", + "id=api_key,env=GARDEN_BUILD_SECRET_API_KEY_2", + ]) + expect(secretEnvVars).to.eql({ + GARDEN_BUILD_SECRET_API_KEY: "banana", + GARDEN_BUILD_SECRET_API_KEY_2: "apple", + }) + }) + + it("validates secret key names", () => { + expect(() => + getDockerSecrets({ + ...baseSpec, + secrets: { + "not allowed": "banana", + "not-safe$(exec ls /)": "apple", + }, + }) + ).throws( + "Invalid secret ID 'not allowed'. Only alphanumeric characters (a-z, A-Z, 0-9), underscores (_), dashes (-) and dots (.) are allowed." + ) + }) + }) + describe("buildContainer", () => { beforeEach(() => { sinon.replace(containerHelpers, "checkDockerServerVersion", () => null) diff --git a/core/test/unit/src/util/escape.ts b/core/test/unit/src/util/escape.ts index 45ac719c15..05dc5429c4 100644 --- a/core/test/unit/src/util/escape.ts +++ b/core/test/unit/src/util/escape.ts @@ -11,26 +11,79 @@ import { commandListToShellScript } from "../../../../src/util/escape.js" describe("commandListToShellScript", () => { it("transforms a list of command line arguments to a shell script", () => { - const commandList = ["echo", "hello", "world"] - const commandString = commandListToShellScript(commandList) - expect(commandString).to.equal("'echo' 'hello' 'world'") + const command = ["echo", "hello", "world"] + const script = commandListToShellScript({ command }) + expect(script).to.equal("'echo' 'hello' 'world'") + }) + + it("allows adding environment variables to shell script", () => { + const command = ["docker", "build", "--secret", "id=foo,env=FRUIT"] + const env = { FRUIT: "banana" } + + const script = commandListToShellScript({ command, env }) + expect(script).to.equal(`FRUIT='banana' 'docker' 'build' '--secret' 'id=foo,env=FRUIT'`) }) it("escapes single quotes in command line arguments", () => { - const commandList = ["echo", "hello", "world's"] - const commandString = commandListToShellScript(commandList) - expect(commandString).to.equal(`'echo' 'hello' 'world'"'"'s'`) + const command = ["echo", "hello", "world's"] + const env = { FRUIT: "banana's" } + + const script = commandListToShellScript({ command, env }) + expect(script).to.equal(`FRUIT='banana'"'"'s' 'echo' 'hello' 'world'"'"'s'`) }) it("replaces all single quotes", () => { - const commandList = ["echo", "'''"] - const commandString = commandListToShellScript(commandList) - expect(commandString).to.equal(`'echo' ''"'"''"'"''"'"''`) + const command = ["echo", "'''"] + const env = { FRUIT: "'''" } + const script = commandListToShellScript({ command, env }) + expect(script).to.equal(`FRUIT=''"'"''"'"''"'"'' 'echo' ''"'"''"'"''"'"''`) }) it("avoids shell injection attacks if used properly", () => { - const commandList = ["echo", "'; exec ls /"] - const commandString = commandListToShellScript(commandList) - expect(commandString).to.equal(`'echo' ''"'"'; exec ls /'`) + const command = ["echo", "'; exec ls /"] + const env = { FRUIT: "$(exec ls /)" } + const script = commandListToShellScript({ command, env }) + expect(script).to.equal(`FRUIT='$(exec ls /)' 'echo' ''"'"'; exec ls /'`) + }) + + it("allows multiline input", () => { + const command = ["echo", "hello\nmultiline\nworld"] + const env = { FRUIT: "multiline\nbanana" } + const script = commandListToShellScript({ command, env }) + expect(script).to.equal(`FRUIT='multiline\nbanana' 'echo' 'hello\nmultiline\nworld'`) + }) + + it("allows underscores in variable names", () => { + const command = ["echo", "hello world"] + const env = { FRUIT_NAME: "banana" } + const script = commandListToShellScript({ command, env }) + expect(script).to.equal(`FRUIT_NAME='banana' 'echo' 'hello world'`) + }) + + it("validates environment variable names", () => { + const command = ["echo", "hello\nmultiline\nworld"] + const env = { "INVALID_FRUIT${exec ls /}": "banana" } + + expect(() => commandListToShellScript({ command, env })).throws( + "Invalid environment variable name INVALID_FRUIT${exec ls /}. Alphanumeric letters and underscores are allowed." + ) + }) + + it("it can handle empty command list", () => { + const command = [] + const script = commandListToShellScript({ command }) + expect(script).to.equal("") + }) + + it("it can handle empty env list", () => { + const command = [] + const script = commandListToShellScript({ command, env: {} }) + expect(script).to.equal("") + }) + + it("empty env vars do not result in unnecessary whitespace", () => { + const command = ["echo", "hello"] + const script = commandListToShellScript({ command, env: {} }) + expect(script).to.equal("'echo' 'hello'") }) }) diff --git a/docs/reference/action-types/Build/container.md b/docs/reference/action-types/Build/container.md index 925d5de2a2..375202fd57 100644 --- a/docs/reference/action-types/Build/container.md +++ b/docs/reference/action-types/Build/container.md @@ -385,6 +385,34 @@ The format is `os/arch`, e.g. `linux/amd64`, `linux/arm64`, etc. | --------------- | -------- | | `array[string]` | No | +### `spec.secrets` + +[spec](#spec) > secrets + +Specify secret values that can be mounted during the build process but become part of the resulting image filesystem or image manifest, for example private registry auth tokens. + +Build arguments and environment variables are inappropriate for secrets, as they persist in the final image. + +The secret can later be consumed in the Dockerfile like so: +``` + RUN --mount=type=secret,id=mytoken TOKEN=$(cat /run/secrets/mytoken) ... +``` + +See also https://docs.docker.com/build/building/secrets/ + +| Type | Required | +| -------- | -------- | +| `object` | No | + +Example: + +```yaml +spec: + ... + secrets: + mytoken: supersecret +``` + ### `spec.dockerfile` [spec](#spec) > dockerfile diff --git a/docs/reference/action-types/Build/jib-container.md b/docs/reference/action-types/Build/jib-container.md index cba3d8d157..2c68815b89 100644 --- a/docs/reference/action-types/Build/jib-container.md +++ b/docs/reference/action-types/Build/jib-container.md @@ -387,6 +387,34 @@ The format is `os/arch`, e.g. `linux/amd64`, `linux/arm64`, etc. | --------------- | -------- | | `array[string]` | No | +### `spec.secrets` + +[spec](#spec) > secrets + +Specify secret values that can be mounted during the build process but become part of the resulting image filesystem or image manifest, for example private registry auth tokens. + +Build arguments and environment variables are inappropriate for secrets, as they persist in the final image. + +The secret can later be consumed in the Dockerfile like so: +``` + RUN --mount=type=secret,id=mytoken TOKEN=$(cat /run/secrets/mytoken) ... +``` + +See also https://docs.docker.com/build/building/secrets/ + +| Type | Required | +| -------- | -------- | +| `object` | No | + +Example: + +```yaml +spec: + ... + secrets: + mytoken: supersecret +``` + ### `spec.dockerfile` [spec](#spec) > dockerfile diff --git a/docs/reference/module-types/container.md b/docs/reference/module-types/container.md index e1f9eb6a40..e1ed239967 100644 --- a/docs/reference/module-types/container.md +++ b/docs/reference/module-types/container.md @@ -196,6 +196,17 @@ extraFlags: # The format is `os/arch`, e.g. `linux/amd64`, `linux/arm64`, etc. platforms: +# Specify secret values that can be mounted during the build process but become part of the resulting image filesystem +# or image manifest, for example private registry auth tokens. +# +# Build arguments and environment variables are inappropriate for secrets, as they persist in the final image. +# +# The secret can later be consumed in the Dockerfile like so: +# RUN --mount=type=secret,id=mytoken TOKEN=$(cat /run/secrets/mytoken) ... +# +# See also https://docs.docker.com/build/building/secrets/ +secrets: + # Specify the image name for the container. Should be a valid Docker image identifier. If specified and the module # does not contain a Dockerfile, this image will be used to deploy services for this module. If specified and the # module does contain a Dockerfile, this identifier is used when pushing the built image. @@ -1088,6 +1099,30 @@ The format is `os/arch`, e.g. `linux/amd64`, `linux/arm64`, etc. | --------------- | -------- | | `array[string]` | No | +### `secrets` + +Specify secret values that can be mounted during the build process but become part of the resulting image filesystem or image manifest, for example private registry auth tokens. + +Build arguments and environment variables are inappropriate for secrets, as they persist in the final image. + +The secret can later be consumed in the Dockerfile like so: +``` + RUN --mount=type=secret,id=mytoken TOKEN=$(cat /run/secrets/mytoken) ... +``` + +See also https://docs.docker.com/build/building/secrets/ + +| Type | Required | +| -------- | -------- | +| `object` | No | + +Example: + +```yaml +secrets: + mytoken: supersecret +``` + ### `image` Specify the image name for the container. Should be a valid Docker image identifier. If specified and the module does not contain a Dockerfile, this image will be used to deploy services for this module. If specified and the module does contain a Dockerfile, this identifier is used when pushing the built image. diff --git a/docs/reference/module-types/jib-container.md b/docs/reference/module-types/jib-container.md index 8c7fa2cb77..b538d23042 100644 --- a/docs/reference/module-types/jib-container.md +++ b/docs/reference/module-types/jib-container.md @@ -264,6 +264,17 @@ extraFlags: # The format is `os/arch`, e.g. `linux/amd64`, `linux/arm64`, etc. platforms: +# Specify secret values that can be mounted during the build process but become part of the resulting image filesystem +# or image manifest, for example private registry auth tokens. +# +# Build arguments and environment variables are inappropriate for secrets, as they persist in the final image. +# +# The secret can later be consumed in the Dockerfile like so: +# RUN --mount=type=secret,id=mytoken TOKEN=$(cat /run/secrets/mytoken) ... +# +# See also https://docs.docker.com/build/building/secrets/ +secrets: + # Specify the image name for the container. Should be a valid Docker image identifier. If specified and the module # does not contain a Dockerfile, this image will be used to deploy services for this module. If specified and the # module does contain a Dockerfile, this identifier is used when pushing the built image. @@ -1303,6 +1314,30 @@ The format is `os/arch`, e.g. `linux/amd64`, `linux/arm64`, etc. | --------------- | -------- | | `array[string]` | No | +### `secrets` + +Specify secret values that can be mounted during the build process but become part of the resulting image filesystem or image manifest, for example private registry auth tokens. + +Build arguments and environment variables are inappropriate for secrets, as they persist in the final image. + +The secret can later be consumed in the Dockerfile like so: +``` + RUN --mount=type=secret,id=mytoken TOKEN=$(cat /run/secrets/mytoken) ... +``` + +See also https://docs.docker.com/build/building/secrets/ + +| Type | Required | +| -------- | -------- | +| `object` | No | + +Example: + +```yaml +secrets: + mytoken: supersecret +``` + ### `image` Specify the image name for the container. Should be a valid Docker image identifier. If specified and the module does not contain a Dockerfile, this image will be used to deploy services for this module. If specified and the module does contain a Dockerfile, this identifier is used when pushing the built image. From 261baf2b46f78de6586a5db1a73ac72cb60dc14e Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Tue, 16 Jul 2024 18:46:06 +0200 Subject: [PATCH 02/11] Update core/src/plugins/container/config.ts --- core/src/plugins/container/config.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/plugins/container/config.ts b/core/src/plugins/container/config.ts index adc395d1bc..f2717ca62b 100644 --- a/core/src/plugins/container/config.ts +++ b/core/src/plugins/container/config.ts @@ -1070,7 +1070,7 @@ export const containerCommonBuildSpecKeys = memoize(() => ({ .pattern(/.+/, joi.string()) .description( dedent` - Specify secret values that can be mounted during the build process but become part of the resulting image filesystem or image manifest, for example private registry auth tokens. + Secret values that can be mounted in the Dockerfile, but do not become part of the image filesystem or image manifest. This is useful e.g. for private registry auth tokens. Build arguments and environment variables are inappropriate for secrets, as they persist in the final image. From faab6c11c127c5d928086fdaaae3016ee7675645 Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Wed, 17 Jul 2024 16:43:08 +0200 Subject: [PATCH 03/11] improvement: protect leaking secrets in logs --- cli/src/build-pkg.ts | 2 +- core/src/commands/util/mutagen.ts | 2 +- core/src/plugins/container/build.ts | 5 +- core/src/plugins/container/config.ts | 4 +- core/src/plugins/container/helpers.ts | 3 +- core/src/plugins/exec/common.ts | 2 +- core/src/plugins/exec/deploy.ts | 2 +- core/src/plugins/kubernetes/api.ts | 5 +- core/src/plugins/kubernetes/run.ts | 15 +- core/src/plugins/kubernetes/sync.ts | 4 +- core/src/util/escape.ts | 35 ++-- core/src/util/ext-tools.ts | 9 +- core/src/util/secrets.ts | 197 ++++++++++++++++++ core/src/util/util.ts | 31 +-- core/src/vcs/git.ts | 2 +- core/test/unit/src/plugins/container/build.ts | 15 +- core/test/unit/src/util/escape.ts | 29 +++ core/test/unit/src/util/secrets.ts | 67 ++++++ 18 files changed, 373 insertions(+), 56 deletions(-) create mode 100644 core/src/util/secrets.ts create mode 100644 core/test/unit/src/util/secrets.ts diff --git a/cli/src/build-pkg.ts b/cli/src/build-pkg.ts index 95ab1807b7..5d4acfedbb 100644 --- a/cli/src/build-pkg.ts +++ b/cli/src/build-pkg.ts @@ -329,7 +329,7 @@ async function buildBinaries(args: string[]) { cwd: repoRoot, stdio: "inherit", // We have to pass the garden version explicitly to rollup due to an issue with the json() plugin loading the wrong package.json files - env: { GARDEN_CORE_VERSION: versionInBinary }, + environment: { GARDEN_CORE_VERSION: versionInBinary }, }) await zipAndHash({ diff --git a/core/src/commands/util/mutagen.ts b/core/src/commands/util/mutagen.ts index df8a6df8d4..bcdbea1077 100644 --- a/core/src/commands/util/mutagen.ts +++ b/core/src/commands/util/mutagen.ts @@ -58,7 +58,7 @@ export class MutagenCommand extends Command<{}, {}> { const result = await exec(mutagenPath, args["$all"]?.slice(2) || [], { cwd: mutagenDir, stdio: "inherit", - env: { + environment: { MUTAGEN_DATA_DIRECTORY: mutagenDir, }, reject: false, diff --git a/core/src/plugins/container/build.ts b/core/src/plugins/container/build.ts index 049b7252ed..dfdf6defb2 100644 --- a/core/src/plugins/container/build.ts +++ b/core/src/plugins/container/build.ts @@ -29,6 +29,7 @@ import { cloudBuilder } from "./cloudbuilder.js" import { styles } from "../../logger/styles.js" import type { CloudBuilderAvailableV2 } from "../../cloud/api.js" import type { SpawnOutput } from "../../util/util.js" +import { type Secret } from "../../util/secrets.js" export const validateContainerBuild: BuildActionHandler<"validate", ContainerBuildAction> = async ({ action }) => { // configure concurrency limit for build status task nodes. @@ -268,10 +269,10 @@ export function getContainerBuildActionOutputs(action: Resolved + secretEnvVars: Record } { const args: string[] = [] - const env: Record = {} + const env: Record = {} for (const [secretKey, secretValue] of Object.entries(actionSpec.secrets || {})) { if (!secretKey.match(/^[a-zA-Z0-9\._-]+$/)) { diff --git a/core/src/plugins/container/config.ts b/core/src/plugins/container/config.ts index f2717ca62b..1c2332e416 100644 --- a/core/src/plugins/container/config.ts +++ b/core/src/plugins/container/config.ts @@ -32,6 +32,7 @@ import type Joi from "@hapi/joi" import type { OctalPermissionMask } from "../kubernetes/types.js" import { templateStringLiteral } from "../../docs/common.js" import { syncGuideLink } from "../kubernetes/constants.js" +import { makeSecret, type Secret } from "../../util/secrets.js" export const defaultDockerfileName = "Dockerfile" @@ -1013,7 +1014,7 @@ export interface ContainerBuildActionSpec { buildArgs: PrimitiveMap dockerfile: string extraFlags: string[] - secrets?: Record + secrets?: Record localId?: string publishId?: string targetStage?: string @@ -1083,6 +1084,7 @@ export const containerCommonBuildSpecKeys = memoize(() => ({ See also https://docs.docker.com/build/building/secrets/ ` ) + .custom(makeSecret) .example({ mytoken: "supersecret", }), diff --git a/core/src/plugins/container/helpers.ts b/core/src/plugins/container/helpers.ts index 115f04ef4c..d9711f3925 100644 --- a/core/src/plugins/container/helpers.ts +++ b/core/src/plugins/container/helpers.ts @@ -33,6 +33,7 @@ import type { Resolved } from "../../actions/types.js" import pMemoize from "../../lib/p-memoize.js" import { styles } from "../../logger/styles.js" import type { ContainerProviderConfig } from "./container.js" +import { type MaybeSecret } from "../../util/secrets.js" const { readFile, pathExists, lstat } = fsExtra @@ -392,7 +393,7 @@ const helpers = { stdout?: Writable stderr?: Writable timeout?: number - env?: { [key: string]: string } + env?: { [key: string]: MaybeSecret } }) { const docker = ctx.tools["container.docker"] diff --git a/core/src/plugins/exec/common.ts b/core/src/plugins/exec/common.ts index dd565cbda7..583feeb914 100644 --- a/core/src/plugins/exec/common.ts +++ b/core/src/plugins/exec/common.ts @@ -77,7 +77,7 @@ export async function execRunCommand({ ...opts, shell, cwd: action.getBuildPath(), - env: envVars, + environment: envVars, stdout: outputStream, stderr: outputStream, }) diff --git a/core/src/plugins/exec/deploy.ts b/core/src/plugins/exec/deploy.ts index 810ca52d7d..e37a753214 100644 --- a/core/src/plugins/exec/deploy.ts +++ b/core/src/plugins/exec/deploy.ts @@ -440,7 +440,7 @@ function runPersistent({ ...getDefaultEnvVars(action), ...(env ? mapValues(env, (v) => v + "") : {}), ...getTracePropagationEnvVars(), - }, + } as Record, shell, cleanup: true, ...opts, diff --git a/core/src/plugins/kubernetes/api.ts b/core/src/plugins/kubernetes/api.ts index 216303384c..4ed5c47597 100644 --- a/core/src/plugins/kubernetes/api.ts +++ b/core/src/plugins/kubernetes/api.ts @@ -71,6 +71,7 @@ import type { RequestOptions } from "http" import https from "node:https" import http from "node:http" import { ProxyAgent } from "proxy-agent" +import { type MaybeSecret, toClearText } from "../../util/secrets.js" interface ApiGroupMap { [groupVersion: string]: V1APIGroup @@ -876,7 +877,7 @@ export class KubeApi { namespace: string podName: string containerName: string - command: string[] + command: MaybeSecret[] stdout?: Writable stderr?: Writable stdin?: Readable @@ -956,7 +957,7 @@ export class KubeApi { namespace, podName, containerName, - command, + command.map(toClearText), _stdout, _stderr, stdin || null, diff --git a/core/src/plugins/kubernetes/run.ts b/core/src/plugins/kubernetes/run.ts index bbfc6a2e4d..c5691fe483 100644 --- a/core/src/plugins/kubernetes/run.ts +++ b/core/src/plugins/kubernetes/run.ts @@ -45,6 +45,7 @@ import { LogLevel } from "../../logger/logger.js" import { getResourceEvents } from "./status/events.js" import stringify from "json-stringify-safe" import { commandListToShellScript } from "../../util/escape.js" +import { maybeSecret, type MaybeSecret, transformSecret } from "../../util/secrets.js" // ref: https://kubernetes.io/docs/reference/labels-annotations-taints/#kubectl-kubernetes-io-default-container export const K8_POD_DEFAULT_CONTAINER_ANNOTATION_KEY = "kubectl.kubernetes.io/default-container" @@ -448,8 +449,8 @@ async function runWithoutArtifacts({ * See https://stackoverflow.com/a/20564208 * @param cmd the command to wrap */ -function getCommandExecutionScript(cmd: string[]) { - return ` +function getCommandExecutionScript(cmd: MaybeSecret[]) { + return maybeSecret` exec 1<&- exec 2<&- exec 1<>/tmp/output @@ -482,12 +483,14 @@ function getArtifactsTarScript(artifacts: ArtifactSpec[]) { // Note: This works because `commandListToShellScript` wraps every parameter in single quotes, escaping contained single quotes. // The string `bin/*` will be transformed to `'bin/*'` by `commandListToShellScript`. The shell would treat `*` as literal and not expand it. // `replaceAll` transforms that string then to `'bin/'*''`, which allows the shell to expand the glob, everything else is treated as literal. - const escapedSource = commandListToShellScript({ command: [source] }).replaceAll("*", "'*'") + const escapedSource = transformSecret(commandListToShellScript({ command: [source] }), (s) => + s.replaceAll("*", "'*'") + ) - return `cp -r ${escapedSource} ${escapedTarget} >/dev/null || true` + return maybeSecret`cp -r ${escapedSource} ${escapedTarget} >/dev/null || true` }) - return ` + return maybeSecret` rm -rf ${tmpPath} >/dev/null || true mkdir -p ${tmpPath} cd ${tmpPath} @@ -710,7 +713,7 @@ interface StartParams { } export type PodRunnerExecParams = StartParams & { - command: string[] + command: MaybeSecret[] containerName?: string stdout?: Writable stderr?: Writable diff --git a/core/src/plugins/kubernetes/sync.ts b/core/src/plugins/kubernetes/sync.ts index 5021bff8ce..81ae03917e 100644 --- a/core/src/plugins/kubernetes/sync.ts +++ b/core/src/plugins/kubernetes/sync.ts @@ -71,6 +71,7 @@ import { ConfigurationError } from "../../exceptions.js" import { gardenEnv } from "../../constants.js" import { styles } from "../../logger/styles.js" import { commandListToShellScript } from "../../util/escape.js" +import { toClearText } from "../../util/secrets.js" export const builtInExcludes = ["/**/*.git", "**/*.garden"] @@ -467,7 +468,8 @@ export async function configureSyncMode({ command: [ "/bin/sh", "-c", - commandListToShellScript({ command: ["cp", "/usr/local/bin/mutagen-agent", mutagenAgentPath] }), + // toClearText: The mutagen agent path isn't secret. + toClearText(commandListToShellScript({ command: ["cp", "/usr/local/bin/mutagen-agent", mutagenAgentPath] })), ], imagePullPolicy: "IfNotPresent", volumeMounts: [gardenVolumeMount], diff --git a/core/src/util/escape.ts b/core/src/util/escape.ts index 7879b3b217..60be0f8db3 100644 --- a/core/src/util/escape.ts +++ b/core/src/util/escape.ts @@ -7,6 +7,7 @@ */ import { InternalError } from "../exceptions.js" +import { joinSecrets, maybeSecret, type MaybeSecret, transformSecret } from "./secrets.js" /** * Wraps every parameter in single quotes, escaping contained single quotes (for use in bash scripts). Joins the elements with a space character. @@ -60,23 +61,33 @@ import { InternalError } from "../exceptions.js" * @param command array of command line arguments * @returns string to be used as shell script statement to execute the given command. */ -export function commandListToShellScript({ command, env }: { command: string[]; env?: Record }) { - const wrapInSingleQuotes = (s: string) => `'${s.replaceAll("'", `'"'"'`)}'` +export function commandListToShellScript>({ + command, + env, +}: { + command: C + env?: E +}) { + const wrapInSingleQuotes = (s: MaybeSecret) => + maybeSecret`'${transformSecret(s, (clearText) => clearText.replaceAll("'", `'"'"'`))}'` - const escapedCommand = command.map(wrapInSingleQuotes).join(" ") + const escapedCommand: MaybeSecret = joinSecrets(command.map(wrapInSingleQuotes), " ") const envVars = Object.entries(env || {}) const escapedEnv = envVars.length > 0 - ? envVars.map(([k, v]) => { - if (!k.match(/^[0-9a-zA-Z_]+$/)) { - throw new InternalError({ - message: `Invalid environment variable name ${k}. Alphanumeric letters and underscores are allowed.`, - }) - } - return `${k}=${wrapInSingleQuotes(v)}` - }) + ? joinSecrets( + envVars.map(([k, v]) => { + if (!k.match(/^[0-9a-zA-Z_]+$/)) { + throw new InternalError({ + message: `Invalid environment variable name ${k}. Alphanumeric letters and underscores are allowed.`, + }) + } + return maybeSecret`${k}=${wrapInSingleQuotes(v)}` + }), + " " + ) : undefined - return `${escapedEnv ? `${escapedEnv} ` : ""}${escapedCommand}` + return maybeSecret`${escapedEnv ? `${escapedEnv} ` : ""}${escapedCommand}` } diff --git a/core/src/util/ext-tools.ts b/core/src/util/ext-tools.ts index 8e41e7bc88..5e9700dfb8 100644 --- a/core/src/util/ext-tools.ts +++ b/core/src/util/ext-tools.ts @@ -11,7 +11,7 @@ const { pathExists, createWriteStream, ensureDir, chmod, remove, move, createRea import { InternalError } from "../exceptions.js" import { join, dirname, basename, posix } from "path" import { getArchitecture, getPlatform, isDarwinARM } from "./arch-platform.js" -import { hashString, exec } from "./util.js" +import { hashString, exec, prepareClearTextEnv } from "./util.js" import tar from "tar" import { GARDEN_GLOBAL_PATH } from "../constants.js" import type { Log } from "../logger/log-entry.js" @@ -28,6 +28,7 @@ import { LogLevel } from "../logger/logger.js" import { uuidv4 } from "./random.js" import { streamLogs, waitForProcess } from "./process.js" import { pipeline } from "node:stream/promises" +import { type MaybeSecret } from "./secrets.js" const toolsPath = join(GARDEN_GLOBAL_PATH, "tools") const lock = new AsyncLock() @@ -35,7 +36,7 @@ const lock = new AsyncLock() export interface ExecParams { args?: string[] cwd?: string - env?: { [key: string]: string } + env?: { [key: string]: MaybeSecret } log: Log timeoutSec?: number input?: Buffer | string @@ -80,7 +81,7 @@ export abstract class CliWrapper { return exec(path, args, { cwd, timeout: timeoutSec ? timeoutSec * 1000 : undefined, - env, + environment: env, input, reject: !ignoreError, stdout, @@ -117,7 +118,7 @@ export abstract class CliWrapper { } log.debug(`Spawning '${path} ${args.join(" ")}' in ${cwd}`) - return crossSpawn(path, args, { cwd, env, windowsHide: true }) + return crossSpawn(path, args, { cwd, env: prepareClearTextEnv(env), windowsHide: true }) } /** diff --git a/core/src/util/secrets.ts b/core/src/util/secrets.ts new file mode 100644 index 0000000000..222ac1ad06 --- /dev/null +++ b/core/src/util/secrets.ts @@ -0,0 +1,197 @@ +/* + * Copyright (C) 2018-2024 Garden Technologies, Inc. + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. + */ + +import { isArray, zip } from "lodash-es" +import { inspect } from "node:util" + +/////////// Public interface + +export function isSecret(s: unknown): s is Secret { + return s !== null && typeof s === "object" && s["isSecretString"] === true +} + +export type UnwrapSecret = + T extends Record + ? Record> + : T extends Array + ? UnwrapSecret + : T extends MaybeSecret + ? string + : T + +type OptionalMaybeSecret = MaybeSecret | undefined +export type DeepMaybeSecret = OptionalMaybeSecret | OptionalMaybeSecret[] | { [key: string]: OptionalMaybeSecret } + +export function toClearText(s: T): UnwrapSecret { + if (isSecret(s)) { + return s.unwrapSecretValue() as UnwrapSecret + } + + // lodash isPlainObject implementation causes a type error + if (!!s && typeof s === "object" && s.constructor === Object) { + return Object.fromEntries(Object.entries(s).map(([k, v]) => [k, toClearText(v)])) as UnwrapSecret + } + + if (isArray(s)) { + return s.map(toClearText) as UnwrapSecret + } + + // it's a string or another type that doesn't need to be unwrapped + return s as UnwrapSecret +} + +export function makeSecret(s: string): Secret { + return new SecretValue(s) +} + +export function transformSecret(s: T, transformFn: (s: string) => string): T { + if (isSecret(s)) { + return s.transformSecretValue(transformFn) as T + } + return transformFn(s) as T +} + +export interface Secret { + /** + * Redacts secrets with three asterisks (***) + */ + toString(): string + /** + * Gives access to the clear text. + * Use toClearText if you are dealing with MaybeSecret values. + */ + unwrapSecretValue(): string + + /** + * Transform a secret value, returning a new instance of Secret + */ + transformSecretValue(transformFn: (secretValue: string) => string): Secret +} +export type MaybeSecret = string | Secret + +/** + * To be used as tagged string. + * + * If none of the template expressions evaluate to a secret value, calling .toString() + * will redact secrets to prevent accidentally leaking them e.g. in logs. + * + * Otherwise it will return a special instance of Secret that only blanks out the secrets + * and leaves the rest as clear text; if we blank out too much then it will be harder to make sense of logs. + * + * @example + * + * const secretBanana = maybeSecret`FRUIT=${makeSecret("banana")}` // Secret + * const regularBanana = maybeSecret`FRUIT=${"banana"}` // string + * + * console.log(secretBanana) // => MY_ENV_VAR=*** + * console.log(regularBanana) // => MY_ENV_VAR=banana + * + * console.log(secretBanana.unwrapSecretValue()) // => MY_ENV_VAR=banana + */ +export function maybeSecret( + nonSecrets: ReadonlyArray, + ...maybeSecrets: ReadonlyArray +): MaybeSecret { + const components = zip(nonSecrets, maybeSecrets) + .flat() + .filter((s): s is MaybeSecret => s !== undefined) + + if (!maybeSecrets.some((s) => isSecret(s))) { + // None of the expressions evaluated to secrets. Let's call toString on all the components. + // if we were wrong for some reason, the only risk is that our secret value gets lost. + return components.join("") + } + + return new CompoundSecret(components) +} + +export function joinSecrets(s: Array, separator: string): MaybeSecret { + return s.reduce((previous, currentValue) => { + return maybeSecret`${previous ? maybeSecret`${previous}${separator}${currentValue}` : currentValue}` + }, undefined) as MaybeSecret +} + +/////////// Private implementation details + +abstract class BaseSecret implements Secret { + public readonly isSecretString = true as const + + // We are using a private class field. + // This prevents most types of accidental leaks due to deep object serialisation. + // See also https://github.com/tc39/proposal-class-fields/blob/main/PRIVATE_SYNTAX_FAQ.md#what-do-you-mean-by-encapsulation--hard-private + readonly #secretValue: SecretValueType + + constructor(secretValue: SecretValueType) { + this.#secretValue = secretValue + } + + // This allows accessing the secret value by means of calling a function, which + // serialization libraries usually don't do. + protected getSecretValue(): SecretValueType { + return this.#secretValue + } + + // Make sure this is serialized as string. + // toString is expected to redact the secret. + public toJSON(): string { + return this.toString() + } + + // Make inspect useful and readable; This is what console.log will use to generate a string representation + public [inspect.custom]() { + return { + secretValue: this.toString(), + } + } + + /** + * Protect accidentally leaking the secret + * + * Replaces secrets with three asterisks (***) + */ + public abstract toString(): string + + /** + * Allow reading the clear text value + */ + public abstract unwrapSecretValue(): string + + public transformSecretValue(transformFn: (secretValue: string) => string): Secret { + const secretValue = this.unwrapSecretValue() + return new SecretValue(transformFn(secretValue)) + } +} + +class SecretValue extends BaseSecret { + constructor(secretValue: string) { + super(secretValue) + } + + public toString(): string { + return "***" + } + public unwrapSecretValue(): string { + return this.getSecretValue() + } +} + +class CompoundSecret extends BaseSecret { + constructor(components: MaybeSecret[]) { + super(components) + } + + override toString(): string { + // calls toString on each of the components + // toString implementation of secret components will turn into three asterisks (***) automatically. + return this.getSecretValue().join("") + } + + override unwrapSecretValue(): string { + return this.getSecretValue().map(toClearText).join("") + } +} diff --git a/core/src/util/util.ts b/core/src/util/util.ts index 8bd7a9c948..d4fbbd93a5 100644 --- a/core/src/util/util.ts +++ b/core/src/util/util.ts @@ -49,6 +49,7 @@ import { execa } from "execa" import corePackageJson from "../../package.json" with { type: "json" } import { makeDocsLinkStyled } from "../docs/common.js" import { getPlatform } from "./arch-platform.js" +import { toClearText, type MaybeSecret } from "./secrets.js" export { apply as jsonMerge } from "json-merge-patch" @@ -166,7 +167,7 @@ export function createOutputStream(log: Log, origin?: string) { return outputStream } -function prepareEnv(env: NodeJS.ProcessEnv | undefined): NodeJS.ProcessEnv { +export function prepareClearTextEnv(env: Record | undefined): NodeJS.ProcessEnv { const envOverride = getPlatform() === "windows" ? { @@ -175,15 +176,16 @@ function prepareEnv(env: NodeJS.ProcessEnv | undefined): NodeJS.ProcessEnv { } : {} - return { + return toClearText({ ...(env || process.env), ...envOverride, - } + }) } -export interface ExecOpts extends ExecaOptions { +export type ExecOpts = Omit & { stdout?: Writable stderr?: Writable + environment?: Record } /** @@ -195,19 +197,17 @@ export interface ExecOpts extends ExecaOptions { * @throws RuntimeError on EMFILE (Too many open files) * @throws ChildProcessError on any other error condition */ -export async function exec(cmd: string, args: string[], opts: ExecOpts = {}) { - opts = { +export async function exec(cmd: string, args: MaybeSecret[], opts: ExecOpts = {}) { + const proc = execa(cmd, args.map(toClearText), { cwd: process.cwd(), windowsHide: true, - ...opts, - env: prepareEnv(opts.env), + ...omit(opts, "stdout", "stderr"), + env: prepareClearTextEnv(opts.environment), // Ensure buffer is always set to true so that we can read the error output // Defaulting cwd to process.cwd() to avoid defaulting to a virtual path after packaging with pkg buffer: true, all: true, - } - - const proc = execa(cmd, args, omit(opts, ["stdout", "stderr"])) + }) opts.stdout && proc.stdout && proc.stdout.pipe(opts.stdout) opts.stderr && proc.stderr && proc.stderr.pipe(opts.stderr) @@ -236,7 +236,8 @@ export async function exec(cmd: string, args: string[], opts: ExecOpts = {}) { if (isExecaError(err)) { throw new ChildProcessError({ cmd, - args, + // toString redacts secret values, if args happens to contain any. + args: args.map((a) => a.toString()), code: err.exitCode, output: err.all || err.stdout || err.stderr || "", stderr: err.stderr || "", @@ -254,7 +255,7 @@ export interface SpawnOpts { cwd?: string data?: Buffer ignoreError?: boolean - env?: { [key: string]: string | undefined } + env?: { [key: string]: MaybeSecret | undefined } rawMode?: boolean // Only used if tty = true. See also: https://nodejs.org/api/tty.html#tty_readstream_setrawmode_mode stdout?: Writable stderr?: Writable @@ -294,7 +295,7 @@ export function spawn(cmd: string, args: string[], opts: SpawnOpts = {}) { } = opts const stdio = tty ? "inherit" : "pipe" - const proc = _spawn(cmd, args, { cwd, env: prepareEnv(env), stdio, windowsHide: true }) + const proc = _spawn(cmd, args, { cwd, env: prepareClearTextEnv(env), stdio, windowsHide: true }) const result: SpawnOutput = { code: 0, @@ -644,7 +645,7 @@ export async function runScript({ const result = await exec(script, [], { shell: true, cwd, - env, + environment: env, stdout: outputStream, stderr: errorStream, }) diff --git a/core/src/vcs/git.ts b/core/src/vcs/git.ts index 4eab654ce2..8b74128d25 100644 --- a/core/src/vcs/git.ts +++ b/core/src/vcs/git.ts @@ -63,7 +63,7 @@ function gitCliExecutor({ log, cwd, failOnPrompt = false }: GitCliParams): GitCl const { stdout } = await exec("git", args, { cwd, maxBuffer: 100 * 1024 * 1024, - env: failOnPrompt ? { GIT_TERMINAL_PROMPT: "0", GIT_ASKPASS: "true" } : undefined, + environment: failOnPrompt ? { GIT_TERMINAL_PROMPT: "0", GIT_ASKPASS: "true" } : undefined, }) return stdout.split("\n").filter((line) => line.length > 0) } diff --git a/core/test/unit/src/plugins/container/build.ts b/core/test/unit/src/plugins/container/build.ts index 6c21a09b52..6ec1f9c9c8 100644 --- a/core/test/unit/src/plugins/container/build.ts +++ b/core/test/unit/src/plugins/container/build.ts @@ -28,6 +28,7 @@ import { getDataDir, makeTestGarden } from "../../../../helpers.js" import fsExtra from "fs-extra" const { createFile } = fsExtra import { type ContainerBuildActionSpec } from "../../../../../src/plugins/container/config.js" +import { makeSecret, toClearText } from "../../../../../src/util/secrets.js" context("build.ts", () => { const projectRoot = getDataDir("test-project-container") @@ -87,14 +88,14 @@ context("build.ts", () => { const { secretArgs, secretEnvVars } = getDockerSecrets({ ...baseSpec, secrets: { - "api-key.fruit-ninja.company.com": "banana", + "api-key.fruit-ninja.company.com": makeSecret("banana"), }, }) expect(secretArgs).to.eql([ "--secret", "id=api-key.fruit-ninja.company.com,env=GARDEN_BUILD_SECRET_API_KEY_FRUIT_NINJA_COMPANY_COM", ]) - expect(secretEnvVars).to.eql({ + expect(toClearText(secretEnvVars)).to.eql({ GARDEN_BUILD_SECRET_API_KEY_FRUIT_NINJA_COMPANY_COM: "banana", }) }) @@ -103,8 +104,8 @@ context("build.ts", () => { const { secretArgs, secretEnvVars } = getDockerSecrets({ ...baseSpec, secrets: { - "api-key": "banana", - "api_key": "apple", + "api-key": makeSecret("banana"), + "api_key": makeSecret("apple"), }, }) expect(secretArgs).to.eql([ @@ -113,7 +114,7 @@ context("build.ts", () => { "--secret", "id=api_key,env=GARDEN_BUILD_SECRET_API_KEY_2", ]) - expect(secretEnvVars).to.eql({ + expect(toClearText(secretEnvVars)).to.eql({ GARDEN_BUILD_SECRET_API_KEY: "banana", GARDEN_BUILD_SECRET_API_KEY_2: "apple", }) @@ -124,8 +125,8 @@ context("build.ts", () => { getDockerSecrets({ ...baseSpec, secrets: { - "not allowed": "banana", - "not-safe$(exec ls /)": "apple", + "not allowed": makeSecret("banana"), + "not-safe$(exec ls /)": makeSecret("apple"), }, }) ).throws( diff --git a/core/test/unit/src/util/escape.ts b/core/test/unit/src/util/escape.ts index 05dc5429c4..c7cfe650e5 100644 --- a/core/test/unit/src/util/escape.ts +++ b/core/test/unit/src/util/escape.ts @@ -8,6 +8,7 @@ import { expect } from "chai" import { commandListToShellScript } from "../../../../src/util/escape.js" +import { makeSecret, Secret, toClearText } from "../../../../src/util/secrets.js" describe("commandListToShellScript", () => { it("transforms a list of command line arguments to a shell script", () => { @@ -16,6 +17,13 @@ describe("commandListToShellScript", () => { expect(script).to.equal("'echo' 'hello' 'world'") }) + it("protects secrets", () => { + const command = ["echo", "hello", makeSecret("secret")] + const script = commandListToShellScript({ command }) + expect(script.toString()).to.equal("'echo' 'hello' '***'") + expect(toClearText(script)).to.equal("'echo' 'hello' 'secret'") + }) + it("allows adding environment variables to shell script", () => { const command = ["docker", "build", "--secret", "id=foo,env=FRUIT"] const env = { FRUIT: "banana" } @@ -69,6 +77,27 @@ describe("commandListToShellScript", () => { ) }) + it("it can handle multiple env vars", () => { + const command = ["wake", "up", makeSecret("neo")] + const script = commandListToShellScript({ + command, + env: { + VAR_1: "hello", + VAR_2: makeSecret("world"), + VAR_3: "where", + VAR_4: "am", + VAR_5: "I", + }, + }) + expect(script.toString()).to.equal("VAR_1='hello' VAR_2='***' VAR_3='where' VAR_4='am' VAR_5='I' 'wake' 'up' '***'") + expect((script).unwrapSecretValue()).to.equal( + "VAR_1='hello' VAR_2='world' VAR_3='where' VAR_4='am' VAR_5='I' 'wake' 'up' 'neo'" + ) + expect(toClearText(script)).to.equal( + "VAR_1='hello' VAR_2='world' VAR_3='where' VAR_4='am' VAR_5='I' 'wake' 'up' 'neo'" + ) + }) + it("it can handle empty command list", () => { const command = [] const script = commandListToShellScript({ command }) diff --git a/core/test/unit/src/util/secrets.ts b/core/test/unit/src/util/secrets.ts new file mode 100644 index 0000000000..b0aa97710e --- /dev/null +++ b/core/test/unit/src/util/secrets.ts @@ -0,0 +1,67 @@ +/* + * Copyright (C) 2018-2024 Garden Technologies, Inc. + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. + */ + +import { expect } from "chai" +import { makeSecret, maybeSecret } from "../../../../src/util/secrets.js" +import { inspect } from "node:util" + +describe("Secret values", () => { + const secretBanana = makeSecret("banana") + const secretApple = makeSecret("apple") + const regularKiwi = "kiwi" + const fruitBasket = maybeSecret`This fruit basket contains an ${secretApple}, a ${secretBanana} as well as a ${regularKiwi}.` + + const clearTextSecrets = ["banana", "apple", "This fruit basket contains an apple, a banana as well as a kiwi."] + + const protectionTestCases = [ + [secretBanana, "***", "banana"], + [secretApple, "***", "apple"], + [fruitBasket, "This fruit basket contains an ***, a *** as well as a kiwi.", "fruit basket"], + ] as const + + for (const [secret, redacted, shortDescription] of protectionTestCases) { + describe(`protects accidentally leaking secret values (${shortDescription})`, () => { + it("converts a plain text secret to an object that protects it against accidental leaks", () => { + expect(secret.toString()).to.eql(redacted) + expect(secret + "").to.eql(redacted) + expect(`${secret}`).to.eql(redacted) + expect(`Hello ${secret}`).to.eql(`Hello ${redacted}`) + expect(JSON.stringify(secret)).to.eql(`"${redacted}"`) + }) + + it("secrets are protected from property enumeration algorithms", () => { + // enumerates all non-symbol properties, including inherited properties. + let count = 0 + for (const p in secret as object) { + for (const c of clearTextSecrets) { + expect(inspect(secret[p]), `Property ${p} leaks the secret value`).not.to.include(c) + count += 1 + } + } + expect(count).not.eql(0) + }) + + it("protects against printing the secret using util.inspect", () => { + const res = inspect(secret, { + showHidden: true, + showProxy: true, + getters: true, + customInspect: false, + depth: Infinity, + colors: false, + }) + let count = 0 + for (const c of clearTextSecrets) { + expect(res).not.to.contain(c) + count += 1 + } + expect(count).not.to.eql(0) + }) + }) + } +}) From 2afd2f7eecccb4089b19b87edfafb2e62d7eb3c0 Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Wed, 17 Jul 2024 16:44:47 +0200 Subject: [PATCH 04/11] improvement: update docs --- docs/reference/action-types/Build/container.md | 2 +- docs/reference/action-types/Build/jib-container.md | 2 +- docs/reference/module-types/container.md | 6 +++--- docs/reference/module-types/jib-container.md | 6 +++--- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/docs/reference/action-types/Build/container.md b/docs/reference/action-types/Build/container.md index 375202fd57..0f975a0054 100644 --- a/docs/reference/action-types/Build/container.md +++ b/docs/reference/action-types/Build/container.md @@ -389,7 +389,7 @@ The format is `os/arch`, e.g. `linux/amd64`, `linux/arm64`, etc. [spec](#spec) > secrets -Specify secret values that can be mounted during the build process but become part of the resulting image filesystem or image manifest, for example private registry auth tokens. +Secret values that can be mounted in the Dockerfile, but do not become part of the image filesystem or image manifest. This is useful e.g. for private registry auth tokens. Build arguments and environment variables are inappropriate for secrets, as they persist in the final image. diff --git a/docs/reference/action-types/Build/jib-container.md b/docs/reference/action-types/Build/jib-container.md index 2c68815b89..e91406b165 100644 --- a/docs/reference/action-types/Build/jib-container.md +++ b/docs/reference/action-types/Build/jib-container.md @@ -391,7 +391,7 @@ The format is `os/arch`, e.g. `linux/amd64`, `linux/arm64`, etc. [spec](#spec) > secrets -Specify secret values that can be mounted during the build process but become part of the resulting image filesystem or image manifest, for example private registry auth tokens. +Secret values that can be mounted in the Dockerfile, but do not become part of the image filesystem or image manifest. This is useful e.g. for private registry auth tokens. Build arguments and environment variables are inappropriate for secrets, as they persist in the final image. diff --git a/docs/reference/module-types/container.md b/docs/reference/module-types/container.md index e1ed239967..d0cbd32861 100644 --- a/docs/reference/module-types/container.md +++ b/docs/reference/module-types/container.md @@ -196,8 +196,8 @@ extraFlags: # The format is `os/arch`, e.g. `linux/amd64`, `linux/arm64`, etc. platforms: -# Specify secret values that can be mounted during the build process but become part of the resulting image filesystem -# or image manifest, for example private registry auth tokens. +# Secret values that can be mounted in the Dockerfile, but do not become part of the image filesystem or image +# manifest. This is useful e.g. for private registry auth tokens. # # Build arguments and environment variables are inappropriate for secrets, as they persist in the final image. # @@ -1101,7 +1101,7 @@ The format is `os/arch`, e.g. `linux/amd64`, `linux/arm64`, etc. ### `secrets` -Specify secret values that can be mounted during the build process but become part of the resulting image filesystem or image manifest, for example private registry auth tokens. +Secret values that can be mounted in the Dockerfile, but do not become part of the image filesystem or image manifest. This is useful e.g. for private registry auth tokens. Build arguments and environment variables are inappropriate for secrets, as they persist in the final image. diff --git a/docs/reference/module-types/jib-container.md b/docs/reference/module-types/jib-container.md index b538d23042..a0017a1855 100644 --- a/docs/reference/module-types/jib-container.md +++ b/docs/reference/module-types/jib-container.md @@ -264,8 +264,8 @@ extraFlags: # The format is `os/arch`, e.g. `linux/amd64`, `linux/arm64`, etc. platforms: -# Specify secret values that can be mounted during the build process but become part of the resulting image filesystem -# or image manifest, for example private registry auth tokens. +# Secret values that can be mounted in the Dockerfile, but do not become part of the image filesystem or image +# manifest. This is useful e.g. for private registry auth tokens. # # Build arguments and environment variables are inappropriate for secrets, as they persist in the final image. # @@ -1316,7 +1316,7 @@ The format is `os/arch`, e.g. `linux/amd64`, `linux/arm64`, etc. ### `secrets` -Specify secret values that can be mounted during the build process but become part of the resulting image filesystem or image manifest, for example private registry auth tokens. +Secret values that can be mounted in the Dockerfile, but do not become part of the image filesystem or image manifest. This is useful e.g. for private registry auth tokens. Build arguments and environment variables are inappropriate for secrets, as they persist in the final image. From 630a822802e162228194f1d1df6fc6c7ea3ab4be Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Wed, 17 Jul 2024 18:03:45 +0200 Subject: [PATCH 05/11] test: extensive test coverage for secret values and maybeSecret Co-authored-by: Vova --- core/src/util/escape.ts | 6 +- core/src/util/secrets.ts | 20 +++- core/test/unit/src/util/secrets.ts | 165 ++++++++++++++++++++++------- 3 files changed, 146 insertions(+), 45 deletions(-) diff --git a/core/src/util/escape.ts b/core/src/util/escape.ts index 60be0f8db3..ae79d633f1 100644 --- a/core/src/util/escape.ts +++ b/core/src/util/escape.ts @@ -89,5 +89,9 @@ export function commandListToShellScript string): Secret } + export type MaybeSecret = string | Secret /** @@ -99,7 +101,7 @@ export function maybeSecret( ): MaybeSecret { const components = zip(nonSecrets, maybeSecrets) .flat() - .filter((s): s is MaybeSecret => s !== undefined) + .filter((s): s is MaybeSecret => s !== undefined || s !== "") if (!maybeSecrets.some((s) => isSecret(s))) { // None of the expressions evaluated to secrets. Let's call toString on all the components. @@ -110,10 +112,17 @@ export function maybeSecret( return new CompoundSecret(components) } -export function joinSecrets(s: Array, separator: string): MaybeSecret { - return s.reduce((previous, currentValue) => { - return maybeSecret`${previous ? maybeSecret`${previous}${separator}${currentValue}` : currentValue}` - }, undefined) as MaybeSecret +export function joinSecrets(s: ReadonlyArray, separator: string): MaybeSecret { + const result = s.reduce((previous, currentValue) => { + if (previous !== undefined) { + return maybeSecret`${previous}${separator}${currentValue}` + } else { + return maybeSecret`${currentValue}` + } + }, undefined) as MaybeSecret | undefined + + // join must return empty string in case of zero elements. + return result || "" } /////////// Private implementation details @@ -175,6 +184,7 @@ class SecretValue extends BaseSecret { public toString(): string { return "***" } + public unwrapSecretValue(): string { return this.getSecretValue() } diff --git a/core/test/unit/src/util/secrets.ts b/core/test/unit/src/util/secrets.ts index b0aa97710e..1c0da9a818 100644 --- a/core/test/unit/src/util/secrets.ts +++ b/core/test/unit/src/util/secrets.ts @@ -7,7 +7,14 @@ */ import { expect } from "chai" -import { makeSecret, maybeSecret } from "../../../../src/util/secrets.js" +import { + isSecret, + joinSecrets, + makeSecret, + maybeSecret, + toClearText, + transformSecret, +} from "../../../../src/util/secrets.js" import { inspect } from "node:util" describe("Secret values", () => { @@ -16,52 +23,132 @@ describe("Secret values", () => { const regularKiwi = "kiwi" const fruitBasket = maybeSecret`This fruit basket contains an ${secretApple}, a ${secretBanana} as well as a ${regularKiwi}.` - const clearTextSecrets = ["banana", "apple", "This fruit basket contains an apple, a banana as well as a kiwi."] + describe("maybeSecret", () => { + it("allows easy templating of secret values without losing the secret value", () => { + const redacted = `${secretBanana} and ${secretApple}` + expect(redacted).eql("*** and ***") + expect(toClearText(redacted)).eql("*** and ***") - const protectionTestCases = [ - [secretBanana, "***", "banana"], - [secretApple, "***", "apple"], - [fruitBasket, "This fruit basket contains an ***, a *** as well as a kiwi.", "fruit basket"], - ] as const + const secret = maybeSecret`${secretBanana} and ${secretApple}` + expect(secret.toString()).eql("*** and ***") + expect(toClearText(secret)).eql("banana and apple") + }) + it("allows mixing secrets and non-secrets", () => { + const secret = maybeSecret`${secretApple} and ${regularKiwi}` + expect(secret.toString()).eql("*** and kiwi") + expect(toClearText(secret)).eql("apple and kiwi") + }) + }) - for (const [secret, redacted, shortDescription] of protectionTestCases) { - describe(`protects accidentally leaking secret values (${shortDescription})`, () => { - it("converts a plain text secret to an object that protects it against accidental leaks", () => { - expect(secret.toString()).to.eql(redacted) - expect(secret + "").to.eql(redacted) - expect(`${secret}`).to.eql(redacted) - expect(`Hello ${secret}`).to.eql(`Hello ${redacted}`) - expect(JSON.stringify(secret)).to.eql(`"${redacted}"`) - }) + describe("Secret protection", () => { + const clearTextSecrets = [toClearText(secretBanana), toClearText(secretApple), toClearText(fruitBasket)] + + const protectionTestCases = [ + [secretBanana, "***", "banana"], + [secretApple, "***", "apple"], + [fruitBasket, "This fruit basket contains an ***, a *** as well as a kiwi.", "fruit basket"], + ] as const + + for (const [secret, redacted, shortDescription] of protectionTestCases) { + describe(`protects accidentally leaking secret values (${shortDescription})`, () => { + it("converts a plain text secret to an object that protects it against accidental leaks", () => { + expect(secret.toString()).to.eql(redacted) + expect(secret + "").to.eql(redacted) + expect(`${secret}`).to.eql(redacted) + expect(`Hello ${secret}`).to.eql(`Hello ${redacted}`) + expect(JSON.stringify(secret)).to.eql(`"${redacted}"`) + }) - it("secrets are protected from property enumeration algorithms", () => { - // enumerates all non-symbol properties, including inherited properties. - let count = 0 - for (const p in secret as object) { + it("secrets are protected from property enumeration algorithms", () => { + // enumerates all non-symbol properties, including inherited properties. + let count = 0 + for (const p in secret as object) { + for (const c of clearTextSecrets) { + expect(inspect(secret[p]), `Property ${p} leaks the secret value`).not.to.include(c) + count += 1 + } + } + expect(count).not.eql(0) + }) + + it("protects against printing the secret using util.inspect", () => { + const res = inspect(secret, { + showHidden: true, + showProxy: true, + getters: true, + customInspect: false, + depth: Infinity, + colors: false, + }) + let count = 0 for (const c of clearTextSecrets) { - expect(inspect(secret[p]), `Property ${p} leaks the secret value`).not.to.include(c) + expect(res).not.to.contain(c) count += 1 } - } - expect(count).not.eql(0) + expect(count).not.to.eql(0) + }) }) + } + }) - it("protects against printing the secret using util.inspect", () => { - const res = inspect(secret, { - showHidden: true, - showProxy: true, - getters: true, - customInspect: false, - depth: Infinity, - colors: false, - }) - let count = 0 - for (const c of clearTextSecrets) { - expect(res).not.to.contain(c) - count += 1 - } - expect(count).not.to.eql(0) + describe("joinSecrets", () => { + const testCases = [ + [["a", "b"], " ", "a b", "a b"], + [[makeSecret("secret1"), makeSecret("secret2")], " ", "*** ***", "secret1 secret2"], + [["a", makeSecret("secret1")], " ", "a ***", "a secret1"], + [[], "", "", ""], + [["one"], "+", "one", "one"], + [["one", ""], "+", "one+", "one+"], + [[makeSecret("oneSecret")], "_", "***", "oneSecret"], + [[makeSecret("oneSecret"), ""], "_", "***_", "oneSecret_"], + [["", "", "", ""], "", "", ""], + [["", "", "", ""], "!", "!!!", "!!!"], + ] as const + + for (const [elements, separator, redacted, clearText] of testCases) { + it(`it joins secrets correctly (${clearText || "empty"})`, () => { + const result = joinSecrets(elements, separator) + expect(result.toString()).to.eql(redacted) + expect(toClearText(result)).to.eql(clearText) }) + } + }) + + describe("transformSecret", () => { + it("allows transforming secrets", () => { + const longBanana = transformSecret(secretBanana, (s) => s.replaceAll("a", "aaa")) + expect(longBanana.toString()).to.eql("***") + expect(toClearText(longBanana)).to.eql("baaanaaanaaa") + }) + it("allows transforming non-secret values", () => { + const longKiwi = transformSecret(regularKiwi, (s) => s.replaceAll("i", "iii")) + expect(longKiwi.toString()).to.eql("kiiiwiii") + expect(toClearText(longKiwi)).to.eql("kiiiwiii") + }) + it("can handle compound secrets", () => { + const fruitBasketWithLongBanana = transformSecret(fruitBasket, (s) => s.replaceAll("a", "aaa")) + expect(fruitBasketWithLongBanana.toString()).to.eql("***") + expect(toClearText(fruitBasketWithLongBanana)).to.eql( + "This fruit baaasket contaaains aaan aaapple, aaa baaanaaanaaa aaas well aaas aaa kiwi." + ) + }) + }) + + describe("isSecret", () => { + it("can identify secrets", () => { + expect(isSecret(secretApple)).to.be.true + expect(isSecret(secretBanana)).to.be.true + expect(isSecret(fruitBasket)).to.be.true + }) + it("returns false for other values", () => { + expect(isSecret(regularKiwi)).to.be.false + expect( + isSecret({ + fakeSecretValue: "hello", + }) + ).to.be.false + expect(isSecret(undefined)).to.be.false + expect(isSecret(null)).to.be.false }) - } + }) }) From 6f144dc134950f783a8e86a031a2cb1de1c69bc2 Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Wed, 17 Jul 2024 18:52:53 +0200 Subject: [PATCH 06/11] fix: fixes after manual testing Co-authored-by: Vova --- core/src/plugins/container/build.ts | 11 ++++++++--- core/src/plugins/container/config.ts | 3 +-- .../plugins/kubernetes/container/build/buildkit.ts | 5 +++-- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/core/src/plugins/container/build.ts b/core/src/plugins/container/build.ts index dfdf6defb2..79e1e5c27b 100644 --- a/core/src/plugins/container/build.ts +++ b/core/src/plugins/container/build.ts @@ -7,7 +7,7 @@ */ import { containerHelpers } from "./helpers.js" -import { ConfigurationError, toGardenError } from "../../exceptions.js" +import { ConfigurationError, InternalError, toGardenError } from "../../exceptions.js" import type { PrimitiveMap } from "../../config/common.js" import split2 from "split2" import type { BuildActionHandler } from "../../plugin/action-types.js" @@ -29,7 +29,7 @@ import { cloudBuilder } from "./cloudbuilder.js" import { styles } from "../../logger/styles.js" import type { CloudBuilderAvailableV2 } from "../../cloud/api.js" import type { SpawnOutput } from "../../util/util.js" -import { type Secret } from "../../util/secrets.js" +import { isSecret, type Secret } from "../../util/secrets.js" export const validateContainerBuild: BuildActionHandler<"validate", ContainerBuildAction> = async ({ action }) => { // configure concurrency limit for build status task nodes. @@ -169,7 +169,7 @@ async function buildContainerLocally({ } } - const cmdOpts = ["build", ...dockerFlags, "--file", dockerfilePath] + const cmdOpts = ["buildx", "build", ...dockerFlags, "--file", dockerfilePath] try { return await containerHelpers.dockerCli({ cwd: buildPath, @@ -280,6 +280,11 @@ export function getDockerSecrets(actionSpec: ContainerBuildActionSpec): { message: `Invalid secret ID '${secretKey}'. Only alphanumeric characters (a-z, A-Z, 0-9), underscores (_), dashes (-) and dots (.) are allowed.`, }) } + if (!isSecret(secretValue)) { + throw new InternalError({ + message: "joi schema did not call makeSecret for every secret value." + }) + } // determine env var names. There can be name collisions due to the fact that we replace special characters with underscores. let envVarname: string diff --git a/core/src/plugins/container/config.ts b/core/src/plugins/container/config.ts index 1c2332e416..ad644150da 100644 --- a/core/src/plugins/container/config.ts +++ b/core/src/plugins/container/config.ts @@ -1068,7 +1068,7 @@ export const containerCommonBuildSpecKeys = memoize(() => ({ `), secrets: joi .object() - .pattern(/.+/, joi.string()) + .pattern(/.+/, joi.string().custom(makeSecret)) .description( dedent` Secret values that can be mounted in the Dockerfile, but do not become part of the image filesystem or image manifest. This is useful e.g. for private registry auth tokens. @@ -1084,7 +1084,6 @@ export const containerCommonBuildSpecKeys = memoize(() => ({ See also https://docs.docker.com/build/building/secrets/ ` ) - .custom(makeSecret) .example({ mytoken: "supersecret", }), diff --git a/core/src/plugins/kubernetes/container/build/buildkit.ts b/core/src/plugins/kubernetes/container/build/buildkit.ts index 2357f6061d..5dff65246d 100644 --- a/core/src/plugins/kubernetes/container/build/buildkit.ts +++ b/core/src/plugins/kubernetes/container/build/buildkit.ts @@ -47,6 +47,7 @@ import { stringifyResources } from "../util.js" import { styles } from "../../../../logger/styles.js" import type { ResolvedBuildAction } from "../../../../actions/build.js" import { commandListToShellScript } from "../../../../util/escape.js" +import { type MaybeSecret, maybeSecret } from "../../../../util/secrets.js" const AWS_ECR_REGEX = /^([^\.]+\.)?dkr\.ecr\.([^\.]+\.)amazonaws\.com\//i // AWS Elastic Container Registry @@ -279,7 +280,7 @@ export function makeBuildkitBuildCommand({ action: ResolvedBuildAction contextPath: string dockerfile: string -}): string[] { +}): MaybeSecret[] { const { secretArgs, secretEnvVars } = getDockerSecrets(action.getSpec()) const buildctlCommand = [ @@ -304,7 +305,7 @@ export function makeBuildkitBuildCommand({ return [ "sh", "-c", - `cd ${contextPath} && ${commandListToShellScript({ command: buildctlCommand, env: secretEnvVars })}`, + maybeSecret`cd ${contextPath} && ${commandListToShellScript({ command: buildctlCommand, env: secretEnvVars })}`, ] } From 27f1fa367da12e071e28207419e9173f64b6fef9 Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Wed, 17 Jul 2024 18:54:28 +0200 Subject: [PATCH 07/11] fix: format --- core/src/plugins/container/build.ts | 2 +- core/test/unit/src/util/escape.ts | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/core/src/plugins/container/build.ts b/core/src/plugins/container/build.ts index 79e1e5c27b..f03598fe68 100644 --- a/core/src/plugins/container/build.ts +++ b/core/src/plugins/container/build.ts @@ -282,7 +282,7 @@ export function getDockerSecrets(actionSpec: ContainerBuildActionSpec): { } if (!isSecret(secretValue)) { throw new InternalError({ - message: "joi schema did not call makeSecret for every secret value." + message: "joi schema did not call makeSecret for every secret value.", }) } diff --git a/core/test/unit/src/util/escape.ts b/core/test/unit/src/util/escape.ts index c7cfe650e5..fdbfec3744 100644 --- a/core/test/unit/src/util/escape.ts +++ b/core/test/unit/src/util/escape.ts @@ -8,7 +8,8 @@ import { expect } from "chai" import { commandListToShellScript } from "../../../../src/util/escape.js" -import { makeSecret, Secret, toClearText } from "../../../../src/util/secrets.js" +import type { Secret } from "../../../../src/util/secrets.js" +import { makeSecret, toClearText } from "../../../../src/util/secrets.js" describe("commandListToShellScript", () => { it("transforms a list of command line arguments to a shell script", () => { From f79cb3f02c018fc93288bd4cb51dbf982d6db5a1 Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Wed, 17 Jul 2024 18:59:02 +0200 Subject: [PATCH 08/11] improvement: undo unnecessary change --- core/src/plugins/exec/deploy.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/plugins/exec/deploy.ts b/core/src/plugins/exec/deploy.ts index e37a753214..810ca52d7d 100644 --- a/core/src/plugins/exec/deploy.ts +++ b/core/src/plugins/exec/deploy.ts @@ -440,7 +440,7 @@ function runPersistent({ ...getDefaultEnvVars(action), ...(env ? mapValues(env, (v) => v + "") : {}), ...getTracePropagationEnvVars(), - } as Record, + }, shell, cleanup: true, ...opts, From 9b709144b2bd7b241525f58ce1f3d31b371b6d66 Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Wed, 17 Jul 2024 19:33:49 +0200 Subject: [PATCH 09/11] fix: framework test --- core/test/unit/src/plugins/container/build.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/core/test/unit/src/plugins/container/build.ts b/core/test/unit/src/plugins/container/build.ts index 6ec1f9c9c8..c8e02ab195 100644 --- a/core/test/unit/src/plugins/container/build.ts +++ b/core/test/unit/src/plugins/container/build.ts @@ -142,6 +142,7 @@ context("build.ts", () => { function getCmdArgs(action: ResolvedBuildAction, any>, buildPath: string) { return [ + "buildx", "build", "--build-arg", `GARDEN_MODULE_VERSION=${action.versionString()}`, From 6350347547e19b2cda43c12388c47e722e617763 Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Wed, 17 Jul 2024 22:16:01 +0200 Subject: [PATCH 10/11] improvement: remove unnecessary maybeSecret --- core/src/util/secrets.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/util/secrets.ts b/core/src/util/secrets.ts index 24283601d2..991a8b223c 100644 --- a/core/src/util/secrets.ts +++ b/core/src/util/secrets.ts @@ -117,7 +117,7 @@ export function joinSecrets(s: ReadonlyArray, separator: string): M if (previous !== undefined) { return maybeSecret`${previous}${separator}${currentValue}` } else { - return maybeSecret`${currentValue}` + return currentValue } }, undefined) as MaybeSecret | undefined From 22880e851b3f65d106989a184aa7e5c1157c6f57 Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Thu, 18 Jul 2024 10:58:23 +0200 Subject: [PATCH 11/11] improvement: clearer doc strings and minor changes --- core/src/util/secrets.ts | 94 +++++++++++++++++++++------------------- 1 file changed, 49 insertions(+), 45 deletions(-) diff --git a/core/src/util/secrets.ts b/core/src/util/secrets.ts index 991a8b223c..5639a374e8 100644 --- a/core/src/util/secrets.ts +++ b/core/src/util/secrets.ts @@ -15,47 +15,18 @@ export function isSecret(s: unknown): s is Secret { return s !== null && typeof s === "object" && s["isSecretString"] === true } -export type UnwrapSecret = - T extends Record - ? Record> - : T extends Array - ? UnwrapSecret - : T extends MaybeSecret - ? string - : T - -type OptionalMaybeSecret = MaybeSecret | undefined -export type DeepMaybeSecret = OptionalMaybeSecret | OptionalMaybeSecret[] | { [key: string]: OptionalMaybeSecret } - -export function toClearText(s: T): UnwrapSecret { - if (isSecret(s)) { - return s.unwrapSecretValue() as UnwrapSecret - } - - // lodash isPlainObject implementation causes a type error - if (!!s && typeof s === "object" && s.constructor === Object) { - return Object.fromEntries(Object.entries(s).map(([k, v]) => [k, toClearText(v)])) as UnwrapSecret - } - - if (isArray(s)) { - return s.map(toClearText) as UnwrapSecret - } - - // it's a string or another type that doesn't need to be unwrapped - return s as UnwrapSecret -} - +/** + * Create an instance of Secret + * @example + * + * const secret = makeSecret("foo") + * console.log(secret) // => *** + * toClearText(secret) // => foo + */ export function makeSecret(s: string): Secret { return new SecretValue(s) } -export function transformSecret(s: T, transformFn: (s: string) => string): T { - if (isSecret(s)) { - return s.transformSecretValue(transformFn) as T - } - return transformFn(s) as T -} - export interface Secret { /** * Redacts secrets with three asterisks (***) @@ -64,7 +35,7 @@ export interface Secret { /** * Gives access to the clear text. - * Use toClearText if you are dealing with MaybeSecret values. + * Use {@link toClearText} if you are dealing with {@link MaybeSecret} values. */ unwrapSecretValue(): string @@ -77,13 +48,9 @@ export interface Secret { export type MaybeSecret = string | Secret /** - * To be used as tagged string. - * - * If none of the template expressions evaluate to a secret value, calling .toString() - * will redact secrets to prevent accidentally leaking them e.g. in logs. + * To be used as tagged string, to concatenate secret and non-secret strings, protecting the secrets from leaking them accidentally. * - * Otherwise it will return a special instance of Secret that only blanks out the secrets - * and leaves the rest as clear text; if we blank out too much then it will be harder to make sense of logs. + * Returns a {@link Secret} if any of the template expressions evaluate to a secret; Otherwise returns string. * * @example * @@ -93,7 +60,7 @@ export type MaybeSecret = string | Secret * console.log(secretBanana) // => MY_ENV_VAR=*** * console.log(regularBanana) // => MY_ENV_VAR=banana * - * console.log(secretBanana.unwrapSecretValue()) // => MY_ENV_VAR=banana + * console.log(toClearText(secretBanana)) // => MY_ENV_VAR=banana */ export function maybeSecret( nonSecrets: ReadonlyArray, @@ -125,6 +92,43 @@ export function joinSecrets(s: ReadonlyArray, separator: string): M return result || "" } +type UnwrapSecret = + T extends Record + ? Record> + : T extends Array + ? UnwrapSecret + : T extends MaybeSecret + ? string + : T + +type OptionalMaybeSecret = MaybeSecret | undefined +type DeepOptionalMaybeSecret = OptionalMaybeSecret | OptionalMaybeSecret[] | { [key: string]: OptionalMaybeSecret } + +export function toClearText(s: T): UnwrapSecret { + if (isSecret(s)) { + return s.unwrapSecretValue() as UnwrapSecret + } + + // lodash isPlainObject implementation causes a type error + if (!!s && typeof s === "object" && s.constructor === Object) { + return Object.fromEntries(Object.entries(s).map(([k, v]) => [k, toClearText(v)])) as UnwrapSecret + } + + if (isArray(s)) { + return s.map(toClearText) as UnwrapSecret + } + + // it's a string or another type that doesn't need to be unwrapped + return s as UnwrapSecret +} + +export function transformSecret(s: T, transformFn: (s: string) => string): T { + if (isSecret(s)) { + return s.transformSecretValue(transformFn) as T + } + return transformFn(s) as T +} + /////////// Private implementation details abstract class BaseSecret implements Secret {