diff options
author | tyler-ball <tyleraball@gmail.com> | 2014-09-16 09:37:17 -0700 |
---|---|---|
committer | tyler-ball <tyleraball@gmail.com> | 2014-09-29 08:31:08 -0700 |
commit | 32310137000ca0983f3952eb3febb08c93c84bdc (patch) | |
tree | 4e7dc15e572bd94eb740d500da64b488826586e0 | |
parent | e121b47679cb38fe8a1571405d9f8f8969266122 (diff) | |
download | chef-32310137000ca0983f3952eb3febb08c93c84bdc.tar.gz |
Finishing UX spec - storing CL options in different config values so we can correctly validate, updating bootstrap to use shared module
-rw-r--r-- | lib/chef/knife/bootstrap.rb | 16 | ||||
-rw-r--r-- | lib/chef/knife/core/bootstrap_context.rb | 13 | ||||
-rw-r--r-- | lib/chef/knife/data_bag_secret_options.rb | 64 | ||||
-rw-r--r-- | lib/chef/knife/data_bag_show.rb | 3 | ||||
-rw-r--r-- | spec/unit/knife/bootstrap_spec.rb | 64 | ||||
-rw-r--r-- | spec/unit/knife/core/bootstrap_context_spec.rb | 41 | ||||
-rw-r--r-- | spec/unit/knife/data_bag_secret_options_spec.rb | 57 | ||||
-rw-r--r-- | spec/unit/knife/data_bag_show_spec.rb | 4 |
8 files changed, 110 insertions, 152 deletions
diff --git a/lib/chef/knife/bootstrap.rb b/lib/chef/knife/bootstrap.rb index 36a0fc1e47..6d628f0224 100644 --- a/lib/chef/knife/bootstrap.rb +++ b/lib/chef/knife/bootstrap.rb @@ -17,11 +17,13 @@ # require 'chef/knife' +require 'chef/knife/data_bag_secret_options' require 'erubis' class Chef class Knife class Bootstrap < Knife + include DataBagSecretOptions deps do require 'chef/knife/core/bootstrap_context' @@ -157,17 +159,6 @@ class Chef Chef::Config[:knife][:hints][name] = path ? Chef::JSONCompat.parse(::File.read(path)) : Hash.new } - option :secret, - :short => "-s SECRET", - :long => "--secret ", - :description => "The secret key to use to encrypt data bag item values", - :proc => Proc.new { |s| Chef::Config[:knife][:secret] = s } - - option :secret_file, - :long => "--secret-file SECRET_FILE", - :description => "A file containing the secret key to use to encrypt data bag item values", - :proc => Proc.new { |sf| Chef::Config[:knife][:secret_file] = sf } - option :bootstrap_url, :long => "--bootstrap-url URL", :description => "URL to a custom installation script", @@ -248,7 +239,8 @@ class Chef def render_template template_file = find_template template = IO.read(template_file).chomp - context = Knife::Core::BootstrapContext.new(config, config[:run_list], Chef::Config) + secret = encryption_secret_provided?(false) ? read_secret : nil + context = Knife::Core::BootstrapContext.new(config, config[:run_list], Chef::Config, secret) Erubis::Eruby.new(template).evaluate(context) end diff --git a/lib/chef/knife/core/bootstrap_context.rb b/lib/chef/knife/core/bootstrap_context.rb index 03e3a42e4a..e681d7a49b 100644 --- a/lib/chef/knife/core/bootstrap_context.rb +++ b/lib/chef/knife/core/bootstrap_context.rb @@ -30,10 +30,11 @@ class Chef # class BootstrapContext - def initialize(config, run_list, chef_config) + def initialize(config, run_list, chef_config, secret) @config = config @run_list = run_list @chef_config = chef_config + @secret = secret end def bootstrap_environment @@ -45,15 +46,7 @@ class Chef end def encrypted_data_bag_secret - knife_config[:secret] || begin - secret_file_path = knife_config[:secret_file] - expanded_secret_file_path = File.expand_path(secret_file_path.to_s) - if secret_file_path && File.exist?(expanded_secret_file_path) - IO.read(expanded_secret_file_path) - else - nil - end - end + @secret end def trusted_certs diff --git a/lib/chef/knife/data_bag_secret_options.rb b/lib/chef/knife/data_bag_secret_options.rb index b692fc767c..238d09667c 100644 --- a/lib/chef/knife/data_bag_secret_options.rb +++ b/lib/chef/knife/data_bag_secret_options.rb @@ -26,17 +26,26 @@ class Chef include Mixlib::CLI include Chef::EncryptedDataBagItem::CheckEncrypted + # The config object is populated by knife#merge_configs with knife.rb `knife[:*]` config values, but they do + # not overwrite the command line properties. It does mean, however, that `knife[:secret]` and `--secret-file` + # passed at the same time populate both `config[:secret]` and `config[:secret_file]`. We cannot differentiate + # the valid case (`knife[:secret]` in config file and `--secret-file` on CL) and the invalid case (`--secret` + # and `--secret-file` on the CL) - thats why I'm storing the CL options in a different config key if they + # are provided. + def self.included(base) base.option :secret, :short => "-s SECRET", :long => "--secret ", :description => "The secret key to use to encrypt data bag item values. Can also be defaulted in your config with the key 'secret'", - :proc => Proc.new { |s| Chef::Config[:knife][:secret] = s } + # Need to store value from command line in separate variable - knife#merge_configs populates same keys + # on config object from + :proc => Proc.new { |s| set_cl_secret(s) } base.option :secret_file, :long => "--secret-file SECRET_FILE", :description => "A file containing the secret key to use to encrypt data bag item values. Can also be defaulted in your config with the key 'secret_file'", - :proc => Proc.new { |sf| Chef::Config[:knife][:secret_file] = sf } + :proc => Proc.new { |sf| set_cl_secret_file(sf) } base.option :encrypt, :long => "--encrypt", @@ -48,18 +57,23 @@ class Chef ## # Determine if the user has specified an appropriate secret for encrypting data bag items. # @returns boolean - def encryption_secret_provided? + def encryption_secret_provided?(need_encrypt_flag = true) validate_secrets - return true if config[:secret] || config[:secret_file] + return true if has_cl_secret? || has_cl_secret_file? - if config[:encrypt] - unless has_secret? || has_secret_file? - ui.fatal("No secret or secret_file specified in config, unable to encrypt item.") - exit(1) - else + if need_encrypt_flag + if config[:encrypt] + unless knife_config[:secret] || knife_config[:secret_file] + ui.fatal("No secret or secret_file specified in config, unable to encrypt item.") + exit(1) + end return true end + return false + elsif knife_config[:secret] || knife_config[:secret_file] + # Certain situations (show and bootstrap) don't need a --encrypt flag to use the config file secret + return true end return false end @@ -69,39 +83,47 @@ class Chef # IE, if we are not running 'knife data bag *' we don't need to load 'chef/encrypted_data_bag_item' require 'chef/encrypted_data_bag_item' - if config[:secret] + if has_cl_secret? config[:secret] - elsif config[:secret_file] + elsif has_cl_secret_file? Chef::EncryptedDataBagItem.load_secret(config[:secret_file]) - elsif secret = knife_config[:secret] || Chef::Config[:secret] + elsif secret = knife_config[:secret] secret else - secret_file = knife_config[:secret_file] || Chef::Config[:secret_file] + secret_file = knife_config[:secret_file] Chef::EncryptedDataBagItem.load_secret(secret_file) end end def validate_secrets - if config[:secret] && config[:secret_file] + if has_cl_secret? && has_cl_secret_file? ui.fatal("Please specify only one of --secret, --secret-file") exit(1) end - if has_secret? && has_secret_file? - ui.fatal("Please specify only one of 'secret' or 'secret_file' in your config") + if knife_config[:secret] && knife_config[:secret_file] + ui.fatal("Please specify only one of 'secret' or 'secret_file' in your config file") exit(1) end end - def has_secret? - knife_config[:secret] || Chef::Config[:secret] + private + + def has_cl_secret? + Chef::Config[:knife].has_key?(:cl_secret) end - def has_secret_file? - knife_config[:secret_file] || Chef::Config[:secret_file] + def self.set_cl_secret(s) + Chef::Config[:knife][:cl_secret] = s end - private + def has_cl_secret_file? + Chef::Config[:knife].has_key?(:cl_secret_file) + end + + def self.set_cl_secret_file(sf) + Chef::Config[:knife][:cl_secret_file] = sf + end def knife_config Chef::Config.key?(:knife) ? Chef::Config[:knife] : {} diff --git a/lib/chef/knife/data_bag_show.rb b/lib/chef/knife/data_bag_show.rb index 05f831a62d..2f97d36ca3 100644 --- a/lib/chef/knife/data_bag_show.rb +++ b/lib/chef/knife/data_bag_show.rb @@ -36,12 +36,11 @@ class Chef def run display = case @name_args.length when 2 # Bag and Item names provided - secret = encryption_secret_provided? ? read_secret : nil + secret = encryption_secret_provided?(false) ? read_secret : nil raw_data = Chef::DataBagItem.load(@name_args[0], @name_args[1]).raw_data encrypted = encrypted?(raw_data) if encrypted && secret - # TODO If the decryption fails with provided secret, what does that look like? # Users do not need to pass --encrypt to read data, we simply try to use the provided secret ui.info("Encrypted data bag detected, decrypting with provided secret.") raw = Chef::EncryptedDataBagItem.load(@name_args[0], diff --git a/spec/unit/knife/bootstrap_spec.rb b/spec/unit/knife/bootstrap_spec.rb index a1c5d168dd..1b1bf3a699 100644 --- a/spec/unit/knife/bootstrap_spec.rb +++ b/spec/unit/knife/bootstrap_spec.rb @@ -30,6 +30,7 @@ describe Chef::Knife::Bootstrap do k.merge_configs k.ui.stub(:stderr).and_return(stderr) + allow(k).to receive(:encryption_secret_provided?).with(false).and_return(false) k end @@ -221,10 +222,6 @@ describe Chef::Knife::Bootstrap do k end - # Include a data bag secret in the options to prevent Bootstrap from - # attempting to access /etc/chef/encrypted_data_bag_secret, which - # can fail when the file exists but can't be accessed by the user - # running the tests. let(:options){ ["--bootstrap-no-proxy", setting, "-s", "foo"] } let(:template_file) { File.expand_path(File.join(CHEF_SPEC_DATA, "bootstrap", "no_proxy.erb")) } let(:rendered_template) do @@ -290,7 +287,6 @@ describe Chef::Knife::Bootstrap do describe "specifying the encrypted data bag secret key" do let(:secret) { "supersekret" } - let(:secret_file) { File.join(CHEF_SPEC_DATA, 'bootstrap', 'encrypted_data_bag_secret') } let(:options) { [] } let(:bootstrap_template) { File.expand_path(File.join(CHEF_SPEC_DATA, "bootstrap", "secret.erb")) } let(:rendered_template) do @@ -299,60 +295,18 @@ describe Chef::Knife::Bootstrap do knife.render_template end - context "via --secret" do - let(:options){ ["--secret", secret] } - - it "creates a secret file" do - rendered_template.should match(%r{#{secret}}) - end - - it "renders the client.rb with an encrypted_data_bag_secret entry" do - rendered_template.should match(%r{encrypted_data_bag_secret\s*"/etc/chef/encrypted_data_bag_secret"}) - end - end - - context "via --secret-file" do - let(:options) { ["--secret-file", secret_file] } - let(:secret) { IO.read(secret_file) } - - it "creates a secret file" do - rendered_template.should match(%r{#{secret}}) - end - - it "renders the client.rb with an encrypted_data_bag_secret entry" do - rendered_template.should match(%r{encrypted_data_bag_secret\s*"/etc/chef/encrypted_data_bag_secret"}) - end + it "creates a secret file" do + expect(knife).to receive(:encryption_secret_provided?).with(false).and_return(true) + expect(knife).to receive(:read_secret).and_return(secret) + rendered_template.should match(%r{#{secret}}) end - context "secret via config" do - before do - Chef::Config[:knife][:secret] = secret - end - - it "creates a secret file" do - rendered_template.should match(%r{#{secret}}) - end - - it "renders the client.rb with an encrypted_data_bag_secret entry" do - rendered_template.should match(%r{encrypted_data_bag_secret\s*"/etc/chef/encrypted_data_bag_secret"}) - end + it "renders the client.rb with an encrypted_data_bag_secret entry" do + expect(knife).to receive(:encryption_secret_provided?).with(false).and_return(true) + expect(knife).to receive(:read_secret).and_return(secret) + rendered_template.should match(%r{encrypted_data_bag_secret\s*"/etc/chef/encrypted_data_bag_secret"}) end - context "secret-file via config" do - let(:secret) { IO.read(secret_file) } - - before do - Chef::Config[:knife][:secret_file] = secret_file - end - - it "creates a secret file" do - rendered_template.should match(%r{#{secret}}) - end - - it "renders the client.rb with an encrypted_data_bag_secret entry" do - rendered_template.should match(%r{encrypted_data_bag_secret\s*"/etc/chef/encrypted_data_bag_secret"}) - end - end end describe "when transferring trusted certificates" do diff --git a/spec/unit/knife/core/bootstrap_context_spec.rb b/spec/unit/knife/core/bootstrap_context_spec.rb index 6427071a6b..266991a7dd 100644 --- a/spec/unit/knife/core/bootstrap_context_spec.rb +++ b/spec/unit/knife/core/bootstrap_context_spec.rb @@ -29,9 +29,10 @@ describe Chef::Knife::Core::BootstrapContext do :validation_client_name => 'chef-validator-testing' } end - let(:secret_file) { File.join(CHEF_SPEC_DATA, 'bootstrap', 'encrypted_data_bag_secret') } - subject(:bootstrap_context) { described_class.new(config, run_list, chef_config) } + let(:secret) { nil } + + subject(:bootstrap_context) { described_class.new(config, run_list, chef_config, secret) } it "runs chef with the first-boot.json in the _default environment" do bootstrap_context.start_chef.should eq "chef-client -j /etc/chef/first-boot.json -E _default" @@ -105,39 +106,9 @@ EXPECTED end describe "when an encrypted_data_bag_secret is provided" do - context "via config[:secret]" do - let(:chef_config) do - { - :knife => {:secret => "supersekret" } - } - end - it "reads the encrypted_data_bag_secret" do - bootstrap_context.encrypted_data_bag_secret.should eq "supersekret" - end - end - - context "via config[:secret_file]" do - let(:chef_config) do - { - :knife => {:secret_file => secret_file} - } - end - it "reads the encrypted_data_bag_secret" do - bootstrap_context.encrypted_data_bag_secret.should eq IO.read(secret_file) - end - end - - context "via config[:secret_file] with short home path" do - let(:chef_config) do - home_path = File.expand_path("~") - shorted_secret_file_path = secret_file.gsub(home_path, "~") - { - :knife => {:secret_file => shorted_secret_file_path} - } - end - it "reads the encrypted_data_bag_secret" do - bootstrap_context.encrypted_data_bag_secret.should eq IO.read(secret_file) - end + let(:secret) { "supersekret" } + it "reads the encrypted_data_bag_secret" do + bootstrap_context.encrypted_data_bag_secret.should eq "supersekret" end end diff --git a/spec/unit/knife/data_bag_secret_options_spec.rb b/spec/unit/knife/data_bag_secret_options_spec.rb index 98e876a5db..b45a95b73a 100644 --- a/spec/unit/knife/data_bag_secret_options_spec.rb +++ b/spec/unit/knife/data_bag_secret_options_spec.rb @@ -50,7 +50,8 @@ describe Chef::Knife::DataBagSecretOptions do describe "#validate_secrets" do it "throws an error when provided with both --secret and --secret-file on the CL" do - expect(example_db).to receive(:config).exactly(2).times.and_return({ :secret_file => secret_file.path, :secret => secret }) + Chef::Config[:knife][:cl_secret_file] = secret_file.path + Chef::Config[:knife][:cl_secret] = secret expect(example_db).to receive(:exit).with(1) expect(example_db.ui).to receive(:fatal).with("Please specify only one of --secret, --secret-file") @@ -61,7 +62,7 @@ describe Chef::Knife::DataBagSecretOptions do Chef::Config[:knife][:secret_file] = secret_file.path Chef::Config[:knife][:secret] = secret expect(example_db).to receive(:exit).with(1) - expect(example_db.ui).to receive(:fatal).with("Please specify only one of 'secret' or 'secret_file' in your config") + expect(example_db.ui).to receive(:fatal).with("Please specify only one of 'secret' or 'secret_file' in your config file") example_db.validate_secrets end @@ -71,25 +72,25 @@ describe Chef::Knife::DataBagSecretOptions do describe "#read_secret" do it "returns the secret first" do - expect(example_db).to receive(:config).exactly(2).times.and_return({ :secret_file => secret_file.path, :secret => secret }) + Chef::Config[:knife][:cl_secret] = secret + expect(example_db).to receive(:config).and_return({ :secret => secret }) expect(example_db.read_secret).to eq(secret) end it "returns the secret_file only if secret does not exist" do - expect(example_db).to receive(:config).exactly(3).times.and_return({ :secret_file => secret_file.path }) + Chef::Config[:knife][:cl_secret_file] = secret_file.path + expect(example_db).to receive(:config).and_return({ :secret_file => secret_file.path }) expect(Chef::EncryptedDataBagItem).to receive(:load_secret).with(secret_file.path).and_return("secret file contents") expect(example_db.read_secret).to eq("secret file contents") end it "returns the secret from the knife.rb config" do - expect(example_db).to receive(:config).exactly(2).times.and_return({}) Chef::Config[:knife][:secret_file] = secret_file.path Chef::Config[:knife][:secret] = secret expect(example_db.read_secret).to eq(secret) end it "returns the secret_file from the knife.rb config only if the secret does not exist" do - expect(example_db).to receive(:config).exactly(2).times.and_return({}) Chef::Config[:knife][:secret_file] = secret_file.path expect(Chef::EncryptedDataBagItem).to receive(:load_secret).with(secret_file.path).and_return("secret file contents") expect(example_db.read_secret).to eq("secret file contents") @@ -100,39 +101,65 @@ describe Chef::Knife::DataBagSecretOptions do describe "#encryption_secret_provided?" do it "returns true if the secret is passed on the CL" do - expect(example_db).to receive(:config).exactly(3).times.and_return({ :secret => secret }) + Chef::Config[:knife][:cl_secret] = secret expect(example_db.encryption_secret_provided?).to eq(true) end it "returns true if the secret_file is passed on the CL" do - expect(example_db).to receive(:config).exactly(3).times.and_return({ :secret_file => secret_file.path }) + Chef::Config[:knife][:cl_secret_file] = secret_file.path expect(example_db.encryption_secret_provided?).to eq(true) end - it "returns true if --encrypt is passed on the CL and :secret is in knife.rb" do - expect(example_db).to receive(:config).exactly(4).times.and_return({ :encrypt => true }) + it "returns true if --encrypt is passed on the CL and :secret is in config" do + expect(example_db).to receive(:config).and_return({ :encrypt => true }) Chef::Config[:knife][:secret] = secret expect(example_db.encryption_secret_provided?).to eq(true) end - it "returns true if --encrypt is passed on the CL and :secret_file is in knife.rb" do - expect(example_db).to receive(:config).exactly(4).times.and_return({ :encrypt => true }) + it "returns true if --encrypt is passed on the CL and :secret_file is in config" do + expect(example_db).to receive(:config).and_return({ :encrypt => true }) Chef::Config[:knife][:secret_file] = secret_file.path expect(example_db.encryption_secret_provided?).to eq(true) end - it "throws an error if --encrypt is passed and there is not :secret or :secret_file in the knife.rb" do - expect(example_db).to receive(:config).exactly(4).times.and_return({ :encrypt => true }) + it "throws an error if --encrypt is passed and there is not :secret or :secret_file in the config" do + expect(example_db).to receive(:config).and_return({ :encrypt => true }) expect(example_db).to receive(:exit).with(1) expect(example_db.ui).to receive(:fatal).with("No secret or secret_file specified in config, unable to encrypt item.") example_db.encryption_secret_provided? end it "returns false if no secret is passed" do - expect(example_db).to receive(:config).exactly(4).times.and_return({}) + expect(example_db).to receive(:config).and_return({}) expect(example_db.encryption_secret_provided?).to eq(false) end + it "returns false if --encrypt is not provided and :secret is in the config" do + expect(example_db).to receive(:config).and_return({}) + Chef::Config[:knife][:secret] = secret + expect(example_db.encryption_secret_provided?).to eq(false) + end + + it "returns false if --encrypt is not provided and :secret_file is in the config" do + expect(example_db).to receive(:config).and_return({}) + Chef::Config[:knife][:secret_file] = secret_file.path + expect(example_db.encryption_secret_provided?).to eq(false) + end + + it "returns true if --encrypt is not provided, :secret is in the config and need_encrypt_flag is false" do + Chef::Config[:knife][:secret] = secret + expect(example_db.encryption_secret_provided?(false)).to eq(true) + end + + it "returns true if --encrypt is not provided, :secret_file is in the config and need_encrypt_flag is false" do + Chef::Config[:knife][:secret_file] = secret_file.path + expect(example_db.encryption_secret_provided?(false)).to eq(true) + end + + it "returns false if --encrypt is not provided and need_encrypt_flag is false" do + expect(example_db.encryption_secret_provided?(false)).to eq(false) + end + end end diff --git a/spec/unit/knife/data_bag_show_spec.rb b/spec/unit/knife/data_bag_show_spec.rb index 49632a2911..47778bdf15 100644 --- a/spec/unit/knife/data_bag_show_spec.rb +++ b/spec/unit/knife/data_bag_show_spec.rb @@ -64,7 +64,7 @@ describe Chef::Knife::DataBagShow do end it "decrypts and displays the encrypted data bag when the secret is provided" do - expect(knife).to receive(:encryption_secret_provided?).and_return(true) + expect(knife).to receive(:encryption_secret_provided?).with(false).and_return(true) expect(knife).to receive(:read_secret).and_return(secret) expect(Chef::DataBagItem).to receive(:load).with(bag_name, item_name).and_return(data_bag_with_encoded_hash) expect(knife.ui).to receive(:info).with("Encrypted data bag detected, decrypting with provided secret.") @@ -78,7 +78,7 @@ qux: http://localhost:4000/data/bag_o_data/qux| end it "displays the encrypted data bag when the secret is not provided" do - expect(knife).to receive(:encryption_secret_provided?).and_return(false) + expect(knife).to receive(:encryption_secret_provided?).with(false).and_return(false) expect(Chef::DataBagItem).to receive(:load).with(bag_name, item_name).and_return(data_bag_with_encoded_hash) expect(knife.ui).to receive(:warn).with("Encrypted data bag detected, but no secret provided for decoding. Displaying encrypted data.") |