From c986ec91a3b12340761c19cf767c009abbf64f2c Mon Sep 17 00:00:00 2001 From: Michael Fyffe <6224270+TraGicCode@users.noreply.github.com> Date: Sun, 30 Mar 2025 21:59:18 -0500 Subject: [PATCH] (GH-146) Add ability to configure prefix hierarchy Closes #146 --- README.md | 31 ++++++ .../functions/azure_key_vault/lookup.rb | 50 +++++++-- spec/functions/azure_key_vault_lookup_spec.rb | 100 +++++++++++++++++- spec/functions/tragic_code/azure_spec.rb | 4 +- 4 files changed, 169 insertions(+), 16 deletions(-) diff --git a/README.md b/README.md index d674117..dadd8b9 100644 --- a/README.md +++ b/README.md @@ -295,6 +295,37 @@ How flexible can this get? Below shows an example of how you "could" remove pro A lookup to 'profile::windows::sqlserver::azure_sql_user_password' or 'profile::linux::blah::blah_again::some_secret' would end up searching for secrets named azure_sql_user_password and some_secret in your Azure Key Vault named "prod-key-vault". +### What is prefixes? + +Prefixes are an optional feature that allow you to create a "hierarchy" similar to the YAML backend built into Puppet. This enables you to configure node-specific secrets and/or create a customized lookup hierarchy. It is also useful if you are migrating from HashiCorp Hiera Vault to Azure Key Vault, as this behavior is similar to that of the [HashiCorp hiera_lookup puppet module](https://forge.puppet.com/modules/petems/hiera_vault/readme). + + +Let's walk through an example to see how this works. If you wanted the ability to configure node-specific secrets but fall back on a "common/shared" secret when none are configured, you can set up the prefixes as shown below: + +```yaml + - name: 'Azure Key Vault Secrets' + lookup_key: azure_key_vault::lookup + options: + vault_name: secrets-vault + vault_api_version: '2016-10-01' + metadata_api_version: '2018-04-02' + key_replacement_token: '-' + prefixes: + - nodes--%{trusted.hostname}-- + - common-- + confine_to_keys: + - '^azure_.*' +``` + +If a Puppet catalog is being compiled for a node with the `trusted.hostname` fact set to `WIN-SQL01.domain.com`, the following lookups would occur in the azure vault named `secrets-vault`: + +1. Check for a secret named `nodes--WIN-SQL-domain-com--profile--windows--sqlserver--sensitive-azure-sql-user-password`. If it exists, cache and return the secret. If not, continue to the next level in the hierarchy. + +2. Check for a secret named `common--profile--windows--sqlserver--sensitive-azure-sql-user-password`. If it exists, cache and return the secret. If it does not exist, return nothing and proceed to the next configured lookup if one exists in the hiera.yaml file. + +**NOTE: The prefixes are always normalized using the [key_replacement_token](#what-is-key_replacement_token). This is because certain facts (such as a machine's hostname) may contain characters that are not supported as part of a secret's name. This normalization prevents failures and avoids forcing users to make difficult decisions, such as changing a node's hostname, which is not ideal.** + + ## How it's secure by default In order to prevent accidental leakage of your secrets throughout all of the locations puppet stores information the returned value of the `azure_key_vault::secret` function & Hiera backend return a string wrapped in a Sensitive data type. Lets look at an example of what this means and why it's important. Below is an example of pulling a secret and trying to output the value in a notice function. diff --git a/lib/puppet/functions/azure_key_vault/lookup.rb b/lib/puppet/functions/azure_key_vault/lookup.rb index 7dae637..31156cf 100644 --- a/lib/puppet/functions/azure_key_vault/lookup.rb +++ b/lib/puppet/functions/azure_key_vault/lookup.rb @@ -11,7 +11,8 @@ Optional[strip_from_keys] => Array[String], Optional[key_replacement_token] => String, Optional[service_principal_credentials] => String, - Optional[use_azure_arc_authentication] => Boolean + Optional[use_azure_arc_authentication] => Boolean, + Optional[prefixes] => Array[String], }]', :options param 'Puppet::LookupContext', :context return_type 'Variant[Sensitive, Undef]' @@ -39,6 +40,11 @@ def lookup_key(secret_name, options, context) end end + prefixes = options['prefixes'] + if prefixes + raise ArgumentError, 'prefixes must be an array' unless prefixes.is_a?(Array) + end + strip_from_keys = options['strip_from_keys'] if strip_from_keys raise ArgumentError, 'strip_from_keys must be an array' unless strip_from_keys.is_a?(Array) @@ -53,9 +59,17 @@ def lookup_key(secret_name, options, context) end end - normalized_secret_name = TragicCode::Azure.normalize_object_name(secret_name, options['key_replacement_token'] || '-') - context.explain { "Using normalized KeyVault secret key for lookup: #{normalized_secret_name}" } - return Puppet::Pops::Types::PSensitiveType::Sensitive.new(context.cached_value(normalized_secret_name)) if context.cache_has_key(normalized_secret_name) + key_replacement_token = options['key_replacement_token'] || '-' + if prefixes + normalized_prefixed_keys = prefixes.map { |prefix| TragicCode::Azure.normalize_object_name(prefix + secret_name, key_replacement_token) } + normalized_prefixed_keys.each do |normalized_prefixed_key| + return Puppet::Pops::Types::PSensitiveType::Sensitive.new(context.cached_value(normalized_prefixed_key)) if context.cache_has_key(normalized_prefixed_key) + end + else + normalized_secret_name = TragicCode::Azure.normalize_object_name(secret_name, key_replacement_token) + context.explain { "Using normalized KeyVault secret key for lookup: #{normalized_secret_name}" } + return Puppet::Pops::Types::PSensitiveType::Sensitive.new(context.cached_value(normalized_secret_name)) if context.cache_has_key(normalized_secret_name) + end access_token = context.cached_value('access_token') if access_token.nil? metadata_api_version = options['metadata_api_version'] @@ -79,14 +93,28 @@ def lookup_key(secret_name, options, context) end context.cache('access_token', access_token) end + secret_value = nil begin - secret_value = TragicCode::Azure.get_secret( - options['vault_name'], - normalized_secret_name, - options['vault_api_version'], - access_token, - '', - ) + if normalized_prefixed_keys + normalized_prefixed_keys.each do |normalized_prefixed_secret_key| + secret_value = TragicCode::Azure.get_secret( + options['vault_name'], + normalized_prefixed_secret_key, + options['vault_api_version'], + access_token, + '', + ) + break unless secret_value.nil? + end + else + secret_value = TragicCode::Azure.get_secret( + options['vault_name'], + normalized_secret_name, + options['vault_api_version'], + access_token, + '', + ) + end rescue RuntimeError => e Puppet.warning(e.message) secret_value = nil diff --git a/spec/functions/azure_key_vault_lookup_spec.rb b/spec/functions/azure_key_vault_lookup_spec.rb index 5fc7d66..c264a31 100644 --- a/spec/functions/azure_key_vault_lookup_spec.rb +++ b/spec/functions/azure_key_vault_lookup_spec.rb @@ -60,7 +60,7 @@ end # rubocop:enable RSpec/NamedSubject - it 'call context.not_found for the lookup_options key' do + it 'calls context.not_found for the lookup_options key' do expect(lookup_context).to receive(:not_found) is_expected.to run.with_params( 'lookup_options', options, lookup_context @@ -80,13 +80,13 @@ end # rubocop:enable RSpec/NamedSubject - it 'errors when confine_to_keys is no array' do + it 'errors when `confine_to_keys` is no array' do is_expected.to run.with_params( 'profile::windows::sqlserver::sensitive_azure_sql_user_password', options.merge({ 'confine_to_keys' => '^vault.*$' }), lookup_context ).and_raise_error(ArgumentError, %r{'confine_to_keys' expects an Array value}i) end - it 'errors when strip_from_keys is no array' do + it 'errors when `strip_from_keys` is no array' do is_expected.to run.with_params( 'profile::windows::sqlserver::sensitive_azure_sql_user_password', options.merge({ 'strip_from_keys' => '^vault.*$' }), lookup_context ).and_raise_error(ArgumentError, %r{'strip_from_keys' expects an Array value}i) @@ -98,6 +98,12 @@ ).and_raise_error(ArgumentError, %r{metadata_api_version and service_principal_credentials cannot be used together}i) end + it 'errors when `prefixes` is no array' do + is_expected.to run.with_params( + 'profile::windows::sqlserver::sensitive_azure_sql_user_password', options.merge({ 'prefixes' => 'common--' }), lookup_context + ).and_raise_error(ArgumentError, %r{'prefixes' expects an Array value}i) + end + it "errors when missing both 'metadata_api_version' and 'service_principal_credentials'" do bad_options = options bad_options.delete('metadata_api_version') @@ -204,4 +210,92 @@ .to be_an_instance_of(Puppet::Pops::Types::PSensitiveType::Sensitive) end # rubocop:enable RSpec/NamedSubject + + describe 'prefixes' do + it 'calls context.not_found when secret is not found in vault' do + # Arrange + access_token_value = 'access_value' + expect(lookup_context).to receive(:not_found) + allow(TragicCode::Azure).to receive(:get_access_token).and_return(access_token_value) + allow(TragicCode::Azure).to receive(:get_secret).and_return(nil) + # This works but not sure if it's a good practice + # expect(TragicCode::Azure).to receive(:get_secret).exactly(2).times.and_return(nil) + + expect(TragicCode::Azure).to receive(:get_secret).exactly(2).times + is_expected.to run.with_params( + 'profile::windows::sqlserver::sensitive_azure_sql_user_password', options.merge({ 'prefixes' => ['nodes--%{trusted.hostname}--', 'common--'] }), lookup_context + ) + end + + # rubocop:disable RSpec/NamedSubject + it 'returns the key if found using the last prefix' do + access_token_value = 'access_value' + secret_value = 'secret_value' + allow(TragicCode::Azure).to receive(:get_access_token).and_return(access_token_value) + allow(TragicCode::Azure).to receive(:get_secret).and_return(nil) + allow(TragicCode::Azure).to receive(:get_secret).with( + options['vault_name'], + 'common--profile--windows--sqlserver--sensitive-azure-sql-user-password', + options['vault_api_version'], + access_token_value, + '', + ).and_return('secret_value') + + expect(subject.execute('profile::windows::sqlserver::sensitive_azure_sql_user_password', options.merge({ 'prefixes' => ['nodes--%{trusted.hostname}--', 'common--'] }), lookup_context).unwrap) + .to eq secret_value + end + # rubocop:enable RSpec/NamedSubject + + # rubocop:disable RSpec/NamedSubject + it 'returns the key if found using the first prefix' do + access_token_value = 'access_value' + secret_value = 'secret_value' + allow(TragicCode::Azure).to receive(:get_access_token).and_return(access_token_value) + allow(TragicCode::Azure).to receive(:get_secret).and_return(nil) + allow(TragicCode::Azure).to receive(:get_secret).with( + options['vault_name'], + 'nodes--WIN-P-01-domain-com--profile--windows--sqlserver--sensitive-azure-sql-user-password', + options['vault_api_version'], + access_token_value, + '', + ).and_return('secret_value') + + expect(subject.execute('profile::windows::sqlserver::sensitive_azure_sql_user_password', options.merge({ 'prefixes' => ['nodes--WIN_P-01.domain.com--', 'common--'] }), lookup_context).unwrap) + .to eq secret_value + end + # rubocop:enable RSpec/NamedSubject + + # rubocop:disable RSpec/NamedSubject + # If someone uses a puppet fact in their one of their prefixes (EX: 'nodes--%{trusted.hostname}--') instead of erroring out + # and forcing them to update the fact ( change the node name ) lets just normalize all prefixes + it 'normalizes prefixes to prevent issues for users' do + access_token_value = 'access_value' + secret_value = 'secret_value' + allow(TragicCode::Azure).to receive(:get_access_token).and_return(access_token_value) + expect(TragicCode::Azure).to receive(:get_secret).with( + options['vault_name'], + 'nodes--WIN-P-01-domain-com--profile--windows--sqlserver--sensitive-azure-sql-user-password', + options['vault_api_version'], + access_token_value, + '', + ).and_return('secret_value') + + expect(subject.execute('profile::windows::sqlserver::sensitive_azure_sql_user_password', options.merge({ 'prefixes' => ['nodes--WIN_P-01.domain.com--'] }), lookup_context).unwrap) + .to eq secret_value + end + # rubocop:enable RSpec/NamedSubject + + # rubocop:disable RSpec/NamedSubject + it 'returns the key from the cache if it exists with the prefix' do + secret_value = 'secret_value' + + allow(lookup_context).to receive(:cache_has_key).and_return(false) + + expect(lookup_context).to receive(:cache_has_key).with('common--profile--windows--sqlserver--sensitive-azure-sql-user-password').and_return(true) + expect(lookup_context).to receive(:cached_value).with('common--profile--windows--sqlserver--sensitive-azure-sql-user-password').and_return(secret_value) + expect(subject.execute('profile::windows::sqlserver::sensitive_azure_sql_user_password', options.merge({ 'prefixes' => ['nodes--%{trusted.hostname}--', 'common--'] }), +lookup_context).unwrap).to eq secret_value + end + # rubocop:enable RSpec/NamedSubject + end end diff --git a/spec/functions/tragic_code/azure_spec.rb b/spec/functions/tragic_code/azure_spec.rb index e269c5c..07faf8c 100644 --- a/spec/functions/tragic_code/azure_spec.rb +++ b/spec/functions/tragic_code/azure_spec.rb @@ -27,7 +27,7 @@ context '.get_access_token_azure_arc' do it 'returns a bearer token' do - File.stub(:read).and_return('magical-token-from-file') + allow(File).to receive(:read).and_return('magical-token-from-file') stub_request(:get, %r{127.0.0.1}) .to_return( @@ -56,7 +56,7 @@ end it 'throws error with response body when response is not 2xx when getting the auth token' do - File.stub(:read).and_return('magical-token-from-file') + allow(File).to receive(:read).and_return('magical-token-from-file') # rubocop:disable Layout/LineLength stub_request(:get, %r{127.0.0.1}) .to_return(body: '{"access_token": "token"}', status: 401, headers: { 'Www-Authenticate' => 'Basic realm=C:\\ProgramData\\AzureConnectedMachineAgent\\Tokens\\f1da0584-97f4-42fd-a671-879ad3de86fa.key' })