From 6f663edad0e73c07227c8f6df74277200cc1471e Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Thu, 14 Dec 2017 15:35:15 -0800 Subject: [PATCH] Scrub sensitive information prior to message output This provides a simple wrapper around all output to scrub any strings that have been registered as sensitive before being output. Also included is a small change to the initial debug output to only show vagrant specific environment variables and not the full user environment. --- lib/vagrant.rb | 12 ++- lib/vagrant/ui.rb | 4 +- lib/vagrant/util.rb | 1 + lib/vagrant/util/credential_scrubber.rb | 39 ++++++++ lib/vagrant/util/logging_formatter.rb | 28 ++++++ test/unit/vagrant/ui_test.rb | 14 +++ .../vagrant/util/credential_scrubber_test.rb | 88 +++++++++++++++++++ 7 files changed, 181 insertions(+), 5 deletions(-) create mode 100644 lib/vagrant/util/logging_formatter.rb create mode 100644 test/unit/vagrant/util/credential_scrubber_test.rb diff --git a/lib/vagrant.rb b/lib/vagrant.rb index 3b6694947..5324c8950 100644 --- a/lib/vagrant.rb +++ b/lib/vagrant.rb @@ -1,7 +1,8 @@ require "vagrant/shared_helpers" -require 'rubygems' -require 'log4r' +require "rubygems" +require "log4r" +require "vagrant/util" # Enable logging if it is requested. We do this before # anything else so that we can setup the output before @@ -41,12 +42,14 @@ if ENV["VAGRANT_LOG"] && ENV["VAGRANT_LOG"] != "" logger = Log4r::Logger.new("vagrant") logger.outputters = Log4r::Outputter.stderr logger.level = level + base_formatter = Log4r::BasicFormatter.new if ENV["VAGRANT_LOG_TIMESTAMP"] - Log4r::Outputter.stderr.formatter = Log4r::PatternFormatter.new( + base_formatter = Log4r::PatternFormatter.new( pattern: "%d [%5l] %m", date_pattern: "%F %T" ) end + Log4r::Outputter.stderr.formatter = Vagrant::Util::LoggingFormatter.new(base_formatter) logger = nil end end @@ -69,7 +72,8 @@ global_logger.info("Vagrant version: #{Vagrant::VERSION}") global_logger.info("Ruby version: #{RUBY_VERSION}") global_logger.info("RubyGems version: #{Gem::VERSION}") ENV.each do |k, v| - global_logger.info("#{k}=#{v.inspect}") if k =~ /^VAGRANT_/ + next if k.start_with?("VAGRANT_OLD") + global_logger.info("#{k}=#{v.inspect}") if k.start_with?("VAGRANT_") end # We need these components always so instead of an autoload we diff --git a/lib/vagrant/ui.rb b/lib/vagrant/ui.rb index 2a52c900e..70474d111 100644 --- a/lib/vagrant/ui.rb +++ b/lib/vagrant/ui.rb @@ -248,7 +248,7 @@ module Vagrant end def format_message(type, message, **opts) - message + Util::CredentialScrubber.desensitize(message) end end @@ -320,6 +320,8 @@ module Vagrant type == :detail || type == :ask || opts[:prefix_spaces] end + message = Util::CredentialScrubber.desensitize(message) + # Fast-path if there is no prefix return message if prefix.empty? diff --git a/lib/vagrant/util.rb b/lib/vagrant/util.rb index 313e1bd19..cef24e8ad 100644 --- a/lib/vagrant/util.rb +++ b/lib/vagrant/util.rb @@ -8,6 +8,7 @@ module Vagrant autoload :Env, 'vagrant/util/env' autoload :HashWithIndifferentAccess, 'vagrant/util/hash_with_indifferent_access' autoload :GuestInspection, 'vagrant/util/guest_inspection' + autoload :LoggingFormatter, 'vagrant/util/logging_formatter' autoload :Platform, 'vagrant/util/platform' autoload :Retryable, 'vagrant/util/retryable' autoload :SafeExec, 'vagrant/util/safe_exec' diff --git a/lib/vagrant/util/credential_scrubber.rb b/lib/vagrant/util/credential_scrubber.rb index f9498ddf4..08fc7c563 100644 --- a/lib/vagrant/util/credential_scrubber.rb +++ b/lib/vagrant/util/credential_scrubber.rb @@ -24,6 +24,45 @@ module Vagrant uri.to_s end end + + # Remove sensitive information from string + # + # @param [String] string + # @return [String] + def self.desensitize(string) + string = string.dup + sensitive_strings.each do |remove| + string.gsub!(remove, REPLACEMENT_TEXT) + end + string + end + + # Register a sensitive string to be scrubbed + def self.sensitive(string) + sensitive_strings.push(string).uniq! + nil + end + + # Deregister a sensitive string and allow output + def self.unsensitive(string) + sensitive_strings.delete(string) + nil + end + + # @return [Array] + def self.sensitive_strings + if !defined?(@_sensitive_strings) + @_sensitive_strings = [] + end + @_sensitive_strings + end + + # @private + # Reset the cached values for scrubber. This is not considered a public + # API and should only be used for testing. + def self.reset! + instance_variables.each(&method(:remove_instance_variable)) + end end end end diff --git a/lib/vagrant/util/logging_formatter.rb b/lib/vagrant/util/logging_formatter.rb new file mode 100644 index 000000000..4007f06b5 --- /dev/null +++ b/lib/vagrant/util/logging_formatter.rb @@ -0,0 +1,28 @@ +require "vagrant/util/credential_scrubber" +require "log4r/formatter/formatter" + +module Vagrant + module Util + # Wrapper for logging formatting to provide + # information scrubbing prior to being written + # to output target + class LoggingFormatter < Log4r::BasicFormatter + + # @return [Log4r::PatternFormatter] + attr_reader :formatter + + # Creates a new formatter wrapper instance. + # + # @param [Log4r::Formatter] + def initialize(formatter) + @formatter = formatter + end + + # Format event and scrub output + def format(event) + msg = formatter.format(event) + CredentialScrubber.desensitize(msg) + end + end + end +end diff --git a/test/unit/vagrant/ui_test.rb b/test/unit/vagrant/ui_test.rb index 856449b58..94d7740c1 100644 --- a/test/unit/vagrant/ui_test.rb +++ b/test/unit/vagrant/ui_test.rb @@ -96,6 +96,20 @@ describe Vagrant::UI::Basic do subject.detail("foo") end end + + context "with sensitive data" do + let(:password){ "my-birthday" } + let(:output){ "You're password is: #{password}" } + + before{ Vagrant::Util::CredentialScrubber.sensitive(password) } + + it "should remove sensitive information from the output" do + expect(subject).to receive(:safe_puts).with(any_args) do |message, **opts| + expect(message).not_to include(password) + end + subject.detail(output) + end + end end describe Vagrant::UI::Colored do diff --git a/test/unit/vagrant/util/credential_scrubber_test.rb b/test/unit/vagrant/util/credential_scrubber_test.rb new file mode 100644 index 000000000..c1815d964 --- /dev/null +++ b/test/unit/vagrant/util/credential_scrubber_test.rb @@ -0,0 +1,88 @@ +require File.expand_path("../../../base", __FILE__) + +require "vagrant/util/credential_scrubber" + +describe Vagrant::Util::CredentialScrubber do + subject{ Vagrant::Util::CredentialScrubber } + + after{ subject.reset! } + + describe ".url_scrubber" do + let(:user){ "vagrant-user" } + let(:password){ "vagrant-pass" } + let(:url){ "http://#{user}:#{password}@example.com" } + + it "should remove user credentials from URL" do + result = subject.url_scrubber(url) + expect(result).not_to include(user) + expect(result).not_to include(password) + end + end + + describe ".sensitive" do + it "should return a nil value" do + expect(subject.sensitive("value")).to be_nil + end + + it "should add value to list of strings" do + subject.sensitive("value") + expect(subject.sensitive_strings).to include("value") + end + + it "should remove duplicates" do + subject.sensitive("value") + subject.sensitive("value") + expect(subject.sensitive_strings.count("value")).to eq(1) + end + end + + describe ".unsensitive" do + it "should return a nil value" do + expect(subject.unsensitive("value")).to be_nil + end + + it "should remove value from list" do + subject.sensitive("value") + expect(subject.sensitive_strings).to include("value") + subject.unsensitive("value") + expect(subject.sensitive_strings).not_to include("value") + end + end + + describe ".sensitive_strings" do + it "should always return the same array" do + expect(subject.sensitive_strings).to be(subject.sensitive_strings) + end + end + + describe ".desensitize" do + let(:to_scrub){ [] } + let(:string){ "a line of text with my-birthday and my-cats-birthday embedded" } + before{ to_scrub.each{|s| subject.sensitive(s) }} + + context "with no sensitive strings registered" do + it "should not modify the string" do + expect(subject.desensitize(string)).to eq(string) + end + end + + context "with single value registered" do + let(:to_scrub){ ["my-birthday"] } + + it "should remove the registered value" do + expect(subject.desensitize(string)).not_to include(to_scrub.first) + end + end + + context "with multiple values registered" do + let(:to_scrub){ ["my-birthday", "my-cats-birthday"] } + + it "should remove all registered values" do + result = subject.desensitize(string) + to_scrub.each do |registered_value| + expect(result).not_to include(registered_value) + end + end + end + end +end