Merge pull request #11853 from chef/mp/11851

Signed-off-by: Tim Smith <tsmith@chef.io>

8 files changed, 117 insertions, 24 deletions

diff --git a/lib/chef/dsl/secret.rb b/lib/chef/dsl/secret.rb
index 8278ef0908..b02609c8bb 100644
--- a/lib/chef/dsl/secret.rb
+++ b/lib/chef/dsl/secret.rb
@@ -49,15 +49,15 @@ class Chef
       #
       #   value = secret(name: "test1", service: :aws_secrets_manager, version: "v1", config: { region: "us-west-1" })
       #   log "My secret is #{value}"
-      def secret(name: nil, version: nil, service: nil, config: nil)
-        Chef::Log.warn <<~EOM.gsub("\n", "")
+      def secret(name: nil, version: nil, service: nil, config: {})
+        Chef::Log.warn <<~EOM.gsub("\n", " ")
           The secrets Chef Infra language helper is currently in beta.
           This helper will most likely change over time in potentially breaking ways.
           If you have feedback or you'd like to be part of the future design of this
           helper e-mail us at secrets_management_beta@progress.com"
         EOM
         sensitive(true) if is_a?(Chef::Resource)
-        Chef::SecretFetcher.for_service(service, config).fetch(name, version)
+        Chef::SecretFetcher.for_service(service, config, run_context).fetch(name, version)
       end
     end
   end
diff --git a/lib/chef/secret_fetcher.rb b/lib/chef/secret_fetcher.rb
index 9b8acd3790..c72e693290 100644
--- a/lib/chef/secret_fetcher.rb
+++ b/lib/chef/secret_fetcher.rb
@@ -30,17 +30,18 @@ class Chef
     # @param service [Symbol] the identifier for the service that will support this request. Must be in
     #                         SECRET_FETCHERS
     # @param config [Hash] configuration that the secrets service requires
-    def self.for_service(service, config)
+    # @param run_context [Chef::RunContext] the run context this is being invoked from
+    def self.for_service(service, config, run_context)
       fetcher = case service
                 when :example
                   require_relative "secret_fetcher/example"
-                  Chef::SecretFetcher::Example.new(config)
+                  Chef::SecretFetcher::Example.new(config, run_context)
                 when :aws_secrets_manager
                   require_relative "secret_fetcher/aws_secrets_manager"
-                  Chef::SecretFetcher::AWSSecretsManager.new(config)
+                  Chef::SecretFetcher::AWSSecretsManager.new(config, run_context)
                 when :azure_key_vault
                   require_relative "secret_fetcher/azure_key_vault"
-                  Chef::SecretFetcher::AzureKeyVault.new(config)
+                  Chef::SecretFetcher::AzureKeyVault.new(config, run_context)
                 when nil, ""
                   raise Chef::Exceptions::Secret::MissingFetcher.new(SECRET_FETCHERS)
                 else
diff --git a/lib/chef/secret_fetcher/aws_secrets_manager.rb b/lib/chef/secret_fetcher/aws_secrets_manager.rb
index 3619a37dd0..c7b6b52b45 100644
--- a/lib/chef/secret_fetcher/aws_secrets_manager.rb
+++ b/lib/chef/secret_fetcher/aws_secrets_manager.rb
@@ -17,6 +17,7 @@
 #
 
 require_relative "base"
+require "aws-sdk-core"
 require "aws-sdk-secretsmanager"
 
 class Chef
@@ -26,19 +27,30 @@ class Chef
   # It is possible to pass options that configure it to use alternative credentials.
   # This implementation supports fetching with version.
   #
-  # NOTE: This does not yet support automatic retries, which the AWS client does by default.
+  # @note ':region' is required configuration.  If it is not explicitly provided,
+  # and it is not available via global AWS config, we will pull it from node ohai data by default.
+  # If this isn't correct, you will need to explicitly override it.
+  # If it is not available via ohai data either (such as if you have the AWS plugin disabled)
+  # then the converge will fail with an error.
+  #
+  # @note: This does not yet support automatic retries, which the AWS client does by default.
   #
   # For configuration options see https://docs.aws.amazon.com/sdk-for-ruby/v3/api/Aws/SecretsManager/Client.html#initialize-instance_method
   #
-  # Note that ~/.aws default and environment-based configurations are supported by default in the
-  # ruby SDK.
   #
   # Usage Example:
   #
-  # fetcher = SecretFetcher.for_service(:aws_secrets_manager, { region: "us-east-1" })
+  # fetcher = SecretFetcher.for_service(:aws_secrets_manager)
   # fetcher.fetch("secretkey1", "v1")
   class SecretFetcher
     class AWSSecretsManager < Base
+      def validate!
+        config[:region] = config[:region] || Aws.config[:region] || run_context.node.dig("ec2", "region")
+        if config[:region].nil?
+          raise Chef::Exceptions::Secret::ConfigurationInvalid.new("Missing required config for AWS secret fetcher: :region")
+        end
+      end
+
       # @param identifier [String] the secret_id
       # @param version [String] the secret version. Not usd at this time
       # @return Aws::SecretsManager::Types::GetSecretValueResponse
diff --git a/lib/chef/secret_fetcher/base.rb b/lib/chef/secret_fetcher/base.rb
index 36b63990c0..2eee1df38b 100644
--- a/lib/chef/secret_fetcher/base.rb
+++ b/lib/chef/secret_fetcher/base.rb
@@ -25,13 +25,17 @@ class Chef
   class SecretFetcher
     class Base
       attr_reader :config
+      # Note that this is only available in the context of a recipe.
+      # Since that's the only place it's intended to be used, that's probably OK.
+      attr_reader :run_context
 
       # Initialize a new SecretFetcher::Base
       #
       # @param config [Hash] Configuration hash.  Expected configuration keys and values
       # will vary based on implementation, and are validated in `validate!`.
-      def initialize(config)
+      def initialize(config, run_context)
         @config = config
+        @run_context = run_context
       end
 
       # Fetch the named secret by invoking implementation-specific [Chef::SecretFetcher::Base#do_fetch]
diff --git a/spec/unit/dsl/secret_spec.rb b/spec/unit/dsl/secret_spec.rb
index ee25c511ee..96a915c43d 100644
--- a/spec/unit/dsl/secret_spec.rb
+++ b/spec/unit/dsl/secret_spec.rb
@@ -21,6 +21,12 @@ require "chef/dsl/secret"
 require "chef/secret_fetcher/base"
 class SecretDSLTester
   include Chef::DSL::Secret
+  # Because DSL is invoked in the context of a recipe,
+  # we expect run_context to always be available when SecretFetcher::Base
+  # requests it - making it safe to mock here
+  def run_context
+    nil
+  end
 end
 
 class SecretFetcherImpl < Chef::SecretFetcher::Base
@@ -36,8 +42,8 @@ describe Chef::DSL::Secret do
   end
 
   it "uses SecretFetcher.for_service to find the fetcher" do
-    substitute_fetcher = SecretFetcherImpl.new({})
-    expect(Chef::SecretFetcher).to receive(:for_service).with(:example, {}).and_return(substitute_fetcher)
+    substitute_fetcher = SecretFetcherImpl.new({}, nil)
+    expect(Chef::SecretFetcher).to receive(:for_service).with(:example, {}, nil).and_return(substitute_fetcher)
     expect(substitute_fetcher).to receive(:fetch).with("key1", nil)
     dsl.secret(name: "key1", service: :example, config: {})
   end
diff --git a/spec/unit/secret_fetcher/aws_secrets_manager_spec.rb b/spec/unit/secret_fetcher/aws_secrets_manager_spec.rb
new file mode 100644
index 0000000000..a98813a7bc
--- /dev/null
+++ b/spec/unit/secret_fetcher/aws_secrets_manager_spec.rb
@@ -0,0 +1,70 @@
+#
+# Author:: Marc Paradise <marc@chef.io>
+# Copyright:: Copyright (c) Chef Software Inc.
+# License:: Apache License, Version 2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+#
+
+require_relative "../../spec_helper"
+require "chef/secret_fetcher/aws_secrets_manager"
+
+describe Chef::SecretFetcher::AWSSecretsManager do
+  let(:node) { {} }
+  let(:aws_global_config) { {} }
+  let(:fetcher_config) { {} }
+  let(:run_context) { double("run_context", node: node) }
+  let(:fetcher) {
+    Chef::SecretFetcher::AWSSecretsManager.new( fetcher_config, run_context )
+  }
+
+  before do
+    allow(Aws).to receive(:config).and_return(aws_global_config)
+  end
+
+  context "when region is provided" do
+    let(:fetcher_config) { { region: "region-from-caller" } }
+    it "uses the provided region" do
+      fetcher.validate!
+      expect(fetcher.config[:region]).to eq "region-from-caller"
+    end
+  end
+
+  context "when region is not provided" do
+    context "and no region exists in AWS config or node attributes" do
+      it "raises a ConfigurationInvalid error" do
+        expect { fetcher.validate! }.to raise_error Chef::Exceptions::Secret::ConfigurationInvalid
+      end
+    end
+
+    context "and region exists in AWS config and node attributes" do
+      let(:aws_global_config)  { { region: "region-from-aws-global-config" } }
+      let(:node) { { "ec2" => { "region" => "region-from-ohai-data" } } }
+      it "uses the region from AWS config" do
+        fetcher.validate!
+        expect(fetcher.config[:region]).to eq "region-from-aws-global-config"
+      end
+    end
+
+    context "and region exists only in node attributes" do
+      let(:node) { { "ec2" => { "region" => "region-from-ohai-data" } } }
+      it "uses the region from AWS config" do
+        fetcher.validate!
+        expect(fetcher.config[:region]).to eq "region-from-ohai-data"
+      end
+
+    end
+
+  end
+end
diff --git a/spec/unit/secret_fetcher/azure_key_vault_spec.rb b/spec/unit/secret_fetcher/azure_key_vault_spec.rb
index cf8c5733c9..d41973992b 100644
--- a/spec/unit/secret_fetcher/azure_key_vault_spec.rb
+++ b/spec/unit/secret_fetcher/azure_key_vault_spec.rb
@@ -23,7 +23,7 @@ require "chef/secret_fetcher/azure_key_vault"
 
 describe Chef::SecretFetcher::AzureKeyVault do
   let(:config) { { vault: "myvault" } }
-  let(:fetcher) { Chef::SecretFetcher::AzureKeyVault.new(config) }
+  let(:fetcher) { Chef::SecretFetcher::AzureKeyVault.new(config, nil) }
 
   context "when validating configuration and configuration is missing :vault" do
     context "and configuration does not have a 'vault'" do
diff --git a/spec/unit/secret_fetcher_spec.rb b/spec/unit/secret_fetcher_spec.rb
index 545176f65a..f6fe2a90b5 100644
--- a/spec/unit/secret_fetcher_spec.rb
+++ b/spec/unit/secret_fetcher_spec.rb
@@ -28,7 +28,7 @@ class SecretFetcherImpl < Chef::SecretFetcher::Base
 end
 
 describe Chef::SecretFetcher do
-  let(:fetcher_impl) { SecretFetcherImpl.new({}) }
+  let(:fetcher_impl) { SecretFetcherImpl.new({}, nil) }
 
   before do
     allow(Chef::SecretFetcher::Example).to receive(:new).and_return fetcher_impl
@@ -36,38 +36,38 @@ describe Chef::SecretFetcher do
 
   context ".for_service" do
     it "resolves the example fetcher without error" do
-      Chef::SecretFetcher.for_service(:example, {})
+      Chef::SecretFetcher.for_service(:example, {}, nil)
     end
 
     it "resolves the Azure Key Vault fetcher without error" do
-      Chef::SecretFetcher.for_service(:azure_key_vault, vault: "invalid")
+      Chef::SecretFetcher.for_service(:azure_key_vault, { vault: "invalid" }, nil)
     end
 
     it "resolves the AWS fetcher without error" do
-      Chef::SecretFetcher.for_service(:aws_secrets_manager, region: "invalid")
+      Chef::SecretFetcher.for_service(:aws_secrets_manager, { region: "invalid" }, nil)
     end
 
     it "raises Chef::Exceptions::Secret::MissingFetcher when service is blank" do
-      expect { Chef::SecretFetcher.for_service(nil, {}) }.to raise_error(Chef::Exceptions::Secret::MissingFetcher)
+      expect { Chef::SecretFetcher.for_service(nil, {}, nil) }.to raise_error(Chef::Exceptions::Secret::MissingFetcher)
     end
 
     it "raises Chef::Exceptions::Secret::MissingFetcher when service is nil" do
-      expect { Chef::SecretFetcher.for_service("", {}) }.to raise_error(Chef::Exceptions::Secret::MissingFetcher)
+      expect { Chef::SecretFetcher.for_service("", {}, nil) }.to raise_error(Chef::Exceptions::Secret::MissingFetcher)
     end
 
     it "raises Chef::Exceptions::Secret::InvalidFetcher for an unknown fetcher" do
-      expect { Chef::SecretFetcher.for_service(:bad_example, {}) }.to raise_error(Chef::Exceptions::Secret::InvalidFetcherService)
+      expect { Chef::SecretFetcher.for_service(:bad_example, {}, nil) }.to raise_error(Chef::Exceptions::Secret::InvalidFetcherService)
     end
 
     it "ensures fetcher configuration is valid by invoking validate!" do
       expect(fetcher_impl).to receive(:validate!)
-      Chef::SecretFetcher.for_service(:example, {})
+      Chef::SecretFetcher.for_service(:example, {}, nil)
     end
   end
 
   context "#fetch" do
     let(:fetcher) {
-      Chef::SecretFetcher.for_service(:example, { "key1" => "value1" })
+      Chef::SecretFetcher.for_service(:example, { "key1" => "value1" }, nil)
     }
 
     it "fetches from the underlying service when secret name is provided " do