From 9232ca4508184505f140b3810823872299b480df Mon Sep 17 00:00:00 2001 From: Cheng XU Date: Fri, 28 Jun 2019 14:50:38 +0800 Subject: [PATCH 1/5] system_command: allow redacting secrets in the log Add a new argument `secrets` to specify secret tokens, so we can redact them in the log. --- Library/Homebrew/exceptions.rb | 6 ++++-- Library/Homebrew/system_command.rb | 9 ++++----- Library/Homebrew/test/system_command_spec.rb | 12 ++++++++++++ Library/Homebrew/utils.rb | 4 ++++ 4 files changed, 24 insertions(+), 7 deletions(-) diff --git a/Library/Homebrew/exceptions.rb b/Library/Homebrew/exceptions.rb index 5d6ca67cda..51ad3898fa 100644 --- a/Library/Homebrew/exceptions.rb +++ b/Library/Homebrew/exceptions.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require "shellwords" +require "utils" class UsageError < RuntimeError attr_reader :reason @@ -520,7 +521,7 @@ class ErrorDuringExecution < RuntimeError attr_reader :status attr_reader :output - def initialize(cmd, status:, output: nil) + def initialize(cmd, status:, output: nil, secrets: []) @cmd = cmd @status = status @output = output @@ -531,7 +532,8 @@ class ErrorDuringExecution < RuntimeError status end - s = +"Failure while executing; `#{cmd.shelljoin.gsub(/\\=/, "=")}` exited with #{exitstatus}." + redacted_cmd = redact_secrets(cmd.shelljoin.gsub('\=', "="), secrets) + s = +"Failure while executing; `#{redacted_cmd}` exited with #{exitstatus}." unless [*output].empty? format_output_line = lambda do |type_line| diff --git a/Library/Homebrew/system_command.rb b/Library/Homebrew/system_command.rb index e365ec1ab2..838661732f 100644 --- a/Library/Homebrew/system_command.rb +++ b/Library/Homebrew/system_command.rb @@ -34,7 +34,7 @@ class SystemCommand end def run! - puts command.shelljoin.gsub(/\\=/, "=") if verbose? || ARGV.debug? + puts redact_secrets(command.shelljoin.gsub('\=', "="), @secrets) if verbose? || ARGV.debug? @output = [] @@ -54,7 +54,7 @@ class SystemCommand end def initialize(executable, args: [], sudo: false, env: {}, input: [], must_succeed: false, - print_stdout: false, print_stderr: true, verbose: false, **options) + print_stdout: false, print_stderr: true, verbose: false, secrets: [], **options) @executable = executable @args = args @@ -63,6 +63,7 @@ class SystemCommand @print_stdout = print_stdout @print_stderr = print_stderr @verbose = verbose + @secrets = Array(secrets) @must_succeed = must_succeed options.assert_valid_keys!(:chdir) @options = options @@ -106,9 +107,7 @@ class SystemCommand def assert_success return if @status.success? - raise ErrorDuringExecution.new(command, - status: @status, - output: @output) + raise ErrorDuringExecution.new(command, status: @status, output: @output, secrets: @secrets) end def expanded_args diff --git a/Library/Homebrew/test/system_command_spec.rb b/Library/Homebrew/test/system_command_spec.rb index 56241ac33f..d1d5a79eed 100644 --- a/Library/Homebrew/test/system_command_spec.rb +++ b/Library/Homebrew/test/system_command_spec.rb @@ -252,5 +252,17 @@ describe SystemCommand do expect(system_command(executable)).to be_a_success end end + + context "when given arguments with secrets" do + it "does not leak the secrets" do + redacted_msg = /#{Regexp.escape("username:******")}/ + expect do + described_class.run! "curl", + args: %w[--user username:hunter2], + verbose: true, + secrets: %w[hunter2] + end.to raise_error.with_message(redacted_msg).and output(redacted_msg).to_stdout + end + end end end diff --git a/Library/Homebrew/utils.rb b/Library/Homebrew/utils.rb index be96044804..57499f2e3b 100644 --- a/Library/Homebrew/utils.rb +++ b/Library/Homebrew/utils.rb @@ -500,3 +500,7 @@ end def command_help_lines(path) path.read.lines.grep(/^#:/).map { |line| line.slice(2..-1) } end + +def redact_secrets(input, secrets) + secrets.reduce(input) { |str, secret| str.gsub secret, "******" }.freeze +end From 66697d429046550904adfa3ac8e689b37c964b6a Mon Sep 17 00:00:00 2001 From: Cheng XU Date: Sat, 13 Jul 2019 22:48:22 +0800 Subject: [PATCH 2/5] ENV: add sensitive_environment function ENV#sensitive_environment is used to list all sensitive environments. Also refactor the code on determining whether an environment is sensitive. --- Library/Homebrew/extend/ENV.rb | 14 +++++++++----- Library/Homebrew/system_config.rb | 3 ++- Library/Homebrew/test/ENV_spec.rb | 7 +++++++ 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/Library/Homebrew/extend/ENV.rb b/Library/Homebrew/extend/ENV.rb index 4d8bae1024..8b7ca32219 100644 --- a/Library/Homebrew/extend/ENV.rb +++ b/Library/Homebrew/extend/ENV.rb @@ -29,12 +29,16 @@ module EnvActivation replace(old_env) end - def clear_sensitive_environment! - each_key do |key| - next unless /(cookie|key|token|password)/i =~ key + def sensitive?(key) + /(cookie|key|token|password)/i =~ key + end - delete key - end + def sensitive_environment + select { |key, _| sensitive?(key) } + end + + def clear_sensitive_environment! + each_key { |key| delete key if sensitive?(key) } end end diff --git a/Library/Homebrew/system_config.rb b/Library/Homebrew/system_config.rb index 2f8924b771..cb5a0884a2 100644 --- a/Library/Homebrew/system_config.rb +++ b/Library/Homebrew/system_config.rb @@ -4,6 +4,7 @@ require "hardware" require "software_spec" require "rexml/document" require "development_tools" +require "extend/ENV" class SystemConfig class << self @@ -173,7 +174,7 @@ class SystemConfig next if boring_keys.include?(key) next if defaults_hash[key.to_sym] - value = "set" if key =~ /(cookie|key|token|password)/i + value = "set" if ENV.sensitive?(key) f.puts "#{key}: #{value}" end end diff --git a/Library/Homebrew/test/ENV_spec.rb b/Library/Homebrew/test/ENV_spec.rb index 726479c648..e7bac787fa 100644 --- a/Library/Homebrew/test/ENV_spec.rb +++ b/Library/Homebrew/test/ENV_spec.rb @@ -143,6 +143,13 @@ shared_examples EnvActivation do expect(subject["MAKEFLAGS"]).to eq("-j4") end + describe "#sensitive_environment" do + it "list sensitive environment" do + subject["SECRET_TOKEN"] = "password" + expect(subject.sensitive_environment).to include("SECRET_TOKEN") + end + end + describe "#clear_sensitive_environment!" do it "removes sensitive environment variables" do subject["SECRET_TOKEN"] = "password" From 1d957c202981a38f6e5f36b25d0efe54944474ee Mon Sep 17 00:00:00 2001 From: Cheng XU Date: Sat, 13 Jul 2019 23:22:18 +0800 Subject: [PATCH 3/5] system_command: automatically find secrets from ENV --- Library/Homebrew/system_command.rb | 3 ++- Library/Homebrew/test/system_command_spec.rb | 14 ++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/Library/Homebrew/system_command.rb b/Library/Homebrew/system_command.rb index 838661732f..5129720c5e 100644 --- a/Library/Homebrew/system_command.rb +++ b/Library/Homebrew/system_command.rb @@ -56,6 +56,7 @@ class SystemCommand def initialize(executable, args: [], sudo: false, env: {}, input: [], must_succeed: false, print_stdout: false, print_stderr: true, verbose: false, secrets: [], **options) + require "extend/ENV" @executable = executable @args = args @sudo = sudo @@ -63,7 +64,7 @@ class SystemCommand @print_stdout = print_stdout @print_stderr = print_stderr @verbose = verbose - @secrets = Array(secrets) + @secrets = (Array(secrets) + ENV.sensitive_environment.values).uniq @must_succeed = must_succeed options.assert_valid_keys!(:chdir) @options = options diff --git a/Library/Homebrew/test/system_command_spec.rb b/Library/Homebrew/test/system_command_spec.rb index d1d5a79eed..63675661a8 100644 --- a/Library/Homebrew/test/system_command_spec.rb +++ b/Library/Homebrew/test/system_command_spec.rb @@ -263,6 +263,20 @@ describe SystemCommand do secrets: %w[hunter2] end.to raise_error.with_message(redacted_msg).and output(redacted_msg).to_stdout end + + it "does not leak the secrets set by environment" do + redacted_msg = /#{Regexp.escape("username:******")}/ + expect do + begin + ENV["PASSWORD"] = "hunter2" + described_class.run! "curl", + args: %w[--user username:hunter2], + verbose: true + ensure + ENV.delete "PASSWORD" + end + end.to raise_error.with_message(redacted_msg).and output(redacted_msg).to_stdout + end end end end From 739c06229dc04861d225f19480b8169e965b9727 Mon Sep 17 00:00:00 2001 From: Cheng XU Date: Sat, 13 Jul 2019 23:31:56 +0800 Subject: [PATCH 4/5] curl/curl_output: allow redacting secrets in the log Add a new argument `secrets` to specify secret tokens, so we can redact them in the log. --- Library/Homebrew/utils/curl.rb | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/Library/Homebrew/utils/curl.rb b/Library/Homebrew/utils/curl.rb index 6bd2333bd3..59be98dd49 100644 --- a/Library/Homebrew/utils/curl.rb +++ b/Library/Homebrew/utils/curl.rb @@ -42,13 +42,14 @@ def curl_args(*extra_args, show_output: false, user_agent: :default) args + extra_args end -def curl(*args) +def curl(*args, secrets: [], **options) # SSL_CERT_FILE can be incorrectly set by users or portable-ruby and screw # with SSL downloads so unset it here. system_command! curl_executable, - args: curl_args(*args), + args: curl_args(*args, **options), print_stdout: true, - env: { "SSL_CERT_FILE" => nil } + env: { "SSL_CERT_FILE" => nil }, + secrets: secrets end def curl_download(*args, to: nil, **options) @@ -77,10 +78,11 @@ def curl_download(*args, to: nil, **options) curl("--location", "--remote-time", "--continue-at", continue_at.to_s, "--output", destination, *args, **options) end -def curl_output(*args, **options) - system_command(curl_executable, +def curl_output(*args, secrets: [], **options) + system_command curl_executable, args: curl_args(*args, show_output: true, **options), - print_stderr: false) + print_stderr: false, + secrets: secrets end def curl_check_http_content(url, user_agents: [:default], check_content: false, strict: false) From 4f29af08f825f2ee5d302b3aa7ff15205a78cc50 Mon Sep 17 00:00:00 2001 From: Cheng XU Date: Sat, 13 Jul 2019 23:35:07 +0800 Subject: [PATCH 5/5] github: redact token in the log --- Library/Homebrew/utils/github.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Library/Homebrew/utils/github.rb b/Library/Homebrew/utils/github.rb index d3237346c4..0c813ac366 100644 --- a/Library/Homebrew/utils/github.rb +++ b/Library/Homebrew/utils/github.rb @@ -195,7 +195,7 @@ module GitHub args += ["--dump-header", headers_tmpfile.path] - output, errors, status = curl_output("--location", url.to_s, *args) + output, errors, status = curl_output("--location", url.to_s, *args, secrets: [token]) output, _, http_code = output.rpartition("\n") output, _, http_code = output.rpartition("\n") if http_code == "000" headers = headers_tmpfile.read