From 2c829a482408cff4b55f1bc5731def98a0e3f31b Mon Sep 17 00:00:00 2001 From: Donal McBreen Date: Thu, 19 Sep 2024 14:58:13 +0100 Subject: [PATCH] Avoid setting env via SSHKit SSHKit puts the env in the command, so leaks them in process listings. --- lib/kamal/cli/base.rb | 15 ++++++++++++-- lib/kamal/cli/build.rb | 38 ++++++++++++++++++----------------- lib/kamal/cli/main.rb | 6 +++--- lib/kamal/commands/hook.rb | 11 ++++++---- test/cli/build_test.rb | 4 ++-- test/cli/cli_test_case.rb | 22 +------------------- test/commands/hook_test.rb | 41 ++++++++++++++++---------------------- 7 files changed, 63 insertions(+), 74 deletions(-) diff --git a/lib/kamal/cli/base.rb b/lib/kamal/cli/base.rb index f594c54e..4aebfd90 100644 --- a/lib/kamal/cli/base.rb +++ b/lib/kamal/cli/base.rb @@ -135,8 +135,10 @@ module Kamal::Cli details = { hosts: KAMAL.hosts.join(","), command: command, subcommand: subcommand } say "Running the #{hook} hook...", :magenta - run_locally do - execute *KAMAL.hook.run(hook, **details, **extra_details) + with_env KAMAL.hook.env(**details, **extra_details) do + run_locally do + execute *KAMAL.hook.run(hook) + end rescue SSHKit::Command::Failed => e raise HookError.new("Hook `#{hook}` failed:\n#{e.message}") end @@ -183,5 +185,14 @@ module Kamal::Cli execute(*KAMAL.server.ensure_run_directory) end end + + def with_env(env) + current_env = ENV.to_h.dup + ENV.update(env) + yield + ensure + ENV.clear + ENV.update(current_env) + end end end diff --git a/lib/kamal/cli/build.rb b/lib/kamal/cli/build.rb index 0347d18c..53ecb0bb 100644 --- a/lib/kamal/cli/build.rb +++ b/lib/kamal/cli/build.rb @@ -30,28 +30,30 @@ class Kamal::Cli::Build < Kamal::Cli::Base say "Building with uncommitted changes:\n #{uncommitted_changes}", :yellow end - run_locally do - begin - execute *KAMAL.builder.inspect_builder - rescue SSHKit::Command::Failed => e - if e.message =~ /(context not found|no builder|no compatible builder|does not exist)/ - warn "Missing compatible builder, so creating a new one first" - begin - cli.remove - rescue SSHKit::Command::Failed - raise unless e.message =~ /(context not found|no builder|does not exist)/ + with_env(KAMAL.config.builder.secrets) do + run_locally do + begin + execute *KAMAL.builder.inspect_builder + rescue SSHKit::Command::Failed => e + if e.message =~ /(context not found|no builder|no compatible builder|does not exist)/ + warn "Missing compatible builder, so creating a new one first" + begin + cli.remove + rescue SSHKit::Command::Failed + raise unless e.message =~ /(context not found|no builder|does not exist)/ + end + cli.create + else + raise end - cli.create - else - raise end - end - # Get the command here to ensure the Dir.chdir doesn't interfere with it - push = KAMAL.builder.push + # Get the command here to ensure the Dir.chdir doesn't interfere with it + push = KAMAL.builder.push - KAMAL.with_verbosity(:debug) do - Dir.chdir(KAMAL.config.builder.build_directory) { execute *push, env: KAMAL.config.builder.secrets } + KAMAL.with_verbosity(:debug) do + Dir.chdir(KAMAL.config.builder.build_directory) { execute *push } + end end end end diff --git a/lib/kamal/cli/main.rb b/lib/kamal/cli/main.rb index a47d8592..69735f2a 100644 --- a/lib/kamal/cli/main.rb +++ b/lib/kamal/cli/main.rb @@ -48,7 +48,7 @@ class Kamal::Cli::Main < Kamal::Cli::Base end end - run_hook "post-deploy", secrets: true, runtime: runtime.round + run_hook "post-deploy", secrets: true, runtime: runtime.round.to_s end desc "redeploy", "Deploy app to servers without bootstrapping servers, starting kamal-proxy, pruning, and registry login" @@ -75,7 +75,7 @@ class Kamal::Cli::Main < Kamal::Cli::Base end end - run_hook "post-deploy", secrets: true, runtime: runtime.round + run_hook "post-deploy", secrets: true, runtime: runtime.round.to_s end desc "rollback [VERSION]", "Rollback app to VERSION" @@ -99,7 +99,7 @@ class Kamal::Cli::Main < Kamal::Cli::Base end end - run_hook "post-deploy", secrets: true, runtime: runtime.round if rolled_back + run_hook "post-deploy", secrets: true, runtime: runtime.round.to_s if rolled_back end desc "details", "Show details about all containers" diff --git a/lib/kamal/commands/hook.rb b/lib/kamal/commands/hook.rb index eb710d5e..cb622d3b 100644 --- a/lib/kamal/commands/hook.rb +++ b/lib/kamal/commands/hook.rb @@ -1,9 +1,12 @@ class Kamal::Commands::Hook < Kamal::Commands::Base - def run(hook, secrets: false, **details) - env = tags(**details).env - env.merge!(config.secrets.to_h) if secrets + def run(hook) + [ hook_file(hook) ] + end - [ hook_file(hook), env: env ] + def env(secrets: false, **details) + tags(**details).env.tap do |env| + env.merge!(config.secrets.to_h) if secrets + end end def hook_exists?(hook) diff --git a/test/cli/build_test.rb b/test/cli/build_test.rb index 2d5d1051..4259fa5b 100644 --- a/test/cli/build_test.rb +++ b/test/cli/build_test.rb @@ -49,7 +49,7 @@ class CliBuildTest < CliTestCase SSHKit::Backend::Abstract.any_instance.expects(:execute).with(:git, "-C", build_directory, :submodule, :update, "--init") SSHKit::Backend::Abstract.any_instance.expects(:execute) - .with(:docker, :buildx, :build, "--push", "--platform", "linux/amd64", "--builder", "kamal-local-docker-container", "-t", "dhh/app:999", "-t", "dhh/app:latest", "--label", "service=\"app\"", "--file", "Dockerfile", ".", env: {}) + .with(:docker, :buildx, :build, "--push", "--platform", "linux/amd64", "--builder", "kamal-local-docker-container", "-t", "dhh/app:999", "-t", "dhh/app:latest", "--label", "service=\"app\"", "--file", "Dockerfile", ".") SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info) .with(:git, "-C", anything, :"rev-parse", :HEAD) @@ -140,7 +140,7 @@ class CliBuildTest < CliTestCase .returns("") SSHKit::Backend::Abstract.any_instance.expects(:execute) - .with(:docker, :buildx, :build, "--push", "--platform", "linux/amd64", "--builder", "kamal-local-docker-container", "-t", "dhh/app:999", "-t", "dhh/app:latest", "--label", "service=\"app\"", "--file", "Dockerfile", ".", env: {}) + .with(:docker, :buildx, :build, "--push", "--platform", "linux/amd64", "--builder", "kamal-local-docker-container", "-t", "dhh/app:999", "-t", "dhh/app:latest", "--label", "service=\"app\"", "--file", "Dockerfile", ".") run_command("push").tap do |output| assert_match /WARN Missing compatible builder, so creating a new one first/, output diff --git a/test/cli/cli_test_case.rb b/test/cli/cli_test_case.rb index 5a2bb76f..27bf7b69 100644 --- a/test/cli/cli_test_case.rb +++ b/test/cli/cli_test_case.rb @@ -41,27 +41,7 @@ class CliTestCase < ActiveSupport::TestCase end def assert_hook_ran(hook, output, version:, service_version:, hosts:, command:, subcommand: nil, runtime: false, secrets: false) - whoami = `whoami`.chomp - performer = Kamal::Git.email.presence || whoami - service = service_version.split("@").first - - assert_match "Running the #{hook} hook...\n", output - - expected = %r{Running\s/usr/bin/env\s\.kamal/hooks/#{hook}\sas\s#{whoami}@localhost\n\s - DEBUG\s\[[0-9a-f]*\]\sCommand:\s\(\sexport\s - KAMAL_RECORDED_AT=\"\d\d\d\d-\d\d-\d\dT\d\d:\d\d:\d\dZ\"\s - KAMAL_PERFORMER=\"#{performer}\"\s - KAMAL_VERSION=\"#{version}\"\s - KAMAL_SERVICE_VERSION=\"#{service_version}\"\s - KAMAL_SERVICE=\"#{service}\"\s - KAMAL_HOSTS=\"#{hosts}\"\s - KAMAL_COMMAND=\"#{command}\"\s - #{"KAMAL_SUBCOMMAND=\\\"#{subcommand}\\\"\\s" if subcommand} - #{"KAMAL_RUNTIME=\\\"\\d+\\\"\\s" if runtime} - #{"DB_PASSWORD=\"secret\"\\s" if secrets} - ;\s/usr/bin/env\s\.kamal/hooks/#{hook} }x - - assert_match expected, output + assert_match %r{usr/bin/env\s\.kamal/hooks/#{hook}}, output end def with_argv(*argv) diff --git a/test/commands/hook_test.rb b/test/commands/hook_test.rb index dc2afc5a..fe1cdc3f 100644 --- a/test/commands/hook_test.rb +++ b/test/commands/hook_test.rb @@ -16,41 +16,34 @@ class CommandsHookTest < ActiveSupport::TestCase end test "run" do - assert_equal [ - ".kamal/hooks/foo", - { env: { - "KAMAL_RECORDED_AT" => @recorded_at, - "KAMAL_PERFORMER" => @performer, - "KAMAL_VERSION" => "123", - "KAMAL_SERVICE_VERSION" => "app@123", - "KAMAL_SERVICE" => "app" } } - ], new_command.run("foo") + assert_equal [ ".kamal/hooks/foo" ], new_command.run("foo") + end + + test "env" do + assert_equal ({ + "KAMAL_RECORDED_AT" => @recorded_at, + "KAMAL_PERFORMER" => @performer, + "KAMAL_VERSION" => "123", + "KAMAL_SERVICE_VERSION" => "app@123", + "KAMAL_SERVICE" => "app" + }), new_command.env end test "run with custom hooks_path" do - assert_equal [ - "custom/hooks/path/foo", - { env: { - "KAMAL_RECORDED_AT" => @recorded_at, - "KAMAL_PERFORMER" => @performer, - "KAMAL_VERSION" => "123", - "KAMAL_SERVICE_VERSION" => "app@123", - "KAMAL_SERVICE" => "app" } } - ], new_command(hooks_path: "custom/hooks/path").run("foo") + assert_equal [ "custom/hooks/path/foo" ], new_command(hooks_path: "custom/hooks/path").run("foo") end - test "hook with secrets" do + test "env with secrets" do with_test_secrets("secrets" => "DB_PASSWORD=secret") do - assert_equal [ - ".kamal/hooks/foo", - { env: { + assert_equal ( + { "KAMAL_RECORDED_AT" => @recorded_at, "KAMAL_PERFORMER" => @performer, "KAMAL_VERSION" => "123", "KAMAL_SERVICE_VERSION" => "app@123", "KAMAL_SERVICE" => "app", - "DB_PASSWORD" => "secret" } } - ], new_command(env: { "secret" => [ "DB_PASSWORD" ] }).run("foo", secrets: true) + "DB_PASSWORD" => "secret" } + ), new_command.env(secrets: true) end end