From 899cc4f7e0b9591eb0e38bec93b1cdbd3349937e Mon Sep 17 00:00:00 2001 From: Min Zhang Date: Wed, 21 Jan 2026 15:44:06 -0500 Subject: [PATCH 01/13] feat(vault): add fine-grained app policies and preserve hub-role This PR adds support for application-level Vault policy segmentation: 1. vault_secrets_init.yaml: Preserve existing hub-role policies - Read current hub-role policies before updating - Merge with default policies instead of replacing - Prevents CronJob from overwriting custom policies 2. vault_app_policies.yaml: Create K8s auth policies from vaultPrefixes - Extract unique prefixes from values-secret.yaml.template - Create fine-grained policies (e.g., apps-qtodo-k8s-secret) - Update hub-role with merged policy list 3. vault_jwt.yaml: Create JWT policies for SPIFFE workloads - Read policies from vault_jwt_policies variable - Use base64 encoding for complex HCL content - Enables patterns to define custom JWT policies This enables patterns to implement least-privilege secret access by organizing secrets into isolated paths (e.g., apps/qtodo, hub/infra/keycloak) with corresponding Vault policies. Signed-off-by: Min Zhang --- plugins/module_utils/parse_secrets_v2.py | 16 ++ plugins/modules/parse_secrets_info.py | 1 + .../vault_utils/tasks/vault_app_policies.yaml | 152 ++++++++++++++++++ roles/vault_utils/tasks/vault_jwt.yaml | 13 ++ .../vault_utils/tasks/vault_secrets_init.yaml | 40 ++++- 5 files changed, 220 insertions(+), 2 deletions(-) create mode 100644 roles/vault_utils/tasks/vault_app_policies.yaml diff --git a/plugins/module_utils/parse_secrets_v2.py b/plugins/module_utils/parse_secrets_v2.py index 4d27511..391c3ea 100644 --- a/plugins/module_utils/parse_secrets_v2.py +++ b/plugins/module_utils/parse_secrets_v2.py @@ -539,3 +539,19 @@ def _inject_field(self, secret_name, f): self.parsed_secrets[secret_name]["fields"][f["name"]] = secret return + + def get_unique_vault_prefixes(self): + """ + Extract all unique vault prefixes from parsed secrets. + + This is useful for creating fine-grained Vault policies for each + unique prefix path (e.g., apps/qtodo, hub/infra/keycloak). + + Returns: + list: Sorted list of unique vault prefixes + """ + prefixes = set() + for secret in self.parsed_secrets.values(): + for prefix in secret.get("vault_prefixes", []): + prefixes.add(prefix) + return sorted(list(prefixes)) diff --git a/plugins/modules/parse_secrets_info.py b/plugins/modules/parse_secrets_info.py index 1af9859..0097ac6 100644 --- a/plugins/modules/parse_secrets_info.py +++ b/plugins/modules/parse_secrets_info.py @@ -152,6 +152,7 @@ def run(module): results["parsed_secrets"] = parsed_secret_obj.parsed_secrets results["kubernetes_secret_objects"] = parsed_secret_obj.kubernetes_secret_objects results["secret_store_namespace"] = parsed_secret_obj.secret_store_namespace + results["unique_vault_prefixes"] = parsed_secret_obj.get_unique_vault_prefixes() module.exit_json(**results) diff --git a/roles/vault_utils/tasks/vault_app_policies.yaml b/roles/vault_utils/tasks/vault_app_policies.yaml new file mode 100644 index 0000000..f7aaf50 --- /dev/null +++ b/roles/vault_utils/tasks/vault_app_policies.yaml @@ -0,0 +1,152 @@ +--- +# vault_app_policies.yaml +# Creates fine-grained Vault policies for application-level isolation +# +# This task file creates individual policies for each vaultPrefix provided. +# Each application gets its own policy that only allows access to its +# specific path, enabling application-level secret isolation. +# +# Required variables: +# app_prefixes: List of app prefixes to create policies for +# e.g., ["apps/qtodo", "hub/infra/keycloak"] +# +# Optional variables: +# app_capabilities: Capabilities for app policies (default: read) +# app_update_hub_role: Whether to update hub-role with new policies (default: true) +# app_create_jwt_roles: Whether to create JWT roles per app (default: false) +# app_jwt_role_config: Dict mapping app names to JWT role config +# +# Example usage: +# - name: Create app-specific vault policies +# ansible.builtin.include_tasks: +# file: vault_app_policies.yaml +# vars: +# app_prefixes: +# - apps/qtodo +# - hub/infra/keycloak +# app_update_hub_role: true + +- name: Debug - Show app prefixes to process + ansible.builtin.debug: + msg: "Processing app prefixes: {{ app_prefixes }}" + when: app_prefixes is defined and app_prefixes | length > 0 + +- name: Skip if no app prefixes defined + ansible.builtin.debug: + msg: "No app_prefixes defined, skipping app policy creation" + when: app_prefixes is not defined or app_prefixes | length == 0 + +# Create policy template for each app prefix +- name: Configure app policy template + kubernetes.core.k8s_exec: + namespace: "{{ vault_ns }}" + pod: "{{ vault_pod }}" + command: > + bash -e -c "echo 'path \"secret/data/{{ item }}/*\" { capabilities = [\"read\"] }' > /tmp/policy-{{ item | replace('/', '-') }}.hcl" + loop: "{{ app_prefixes }}" + loop_control: + label: "Creating policy template for {{ item }}" + when: app_prefixes is defined and app_prefixes | length > 0 + +# Write the policy to Vault +- name: Configure app policy in Vault + kubernetes.core.k8s_exec: + namespace: "{{ vault_ns }}" + pod: "{{ vault_pod }}" + command: > + vault policy write {{ item | replace('/', '-') }}-k8s-secret + /tmp/policy-{{ item | replace('/', '-') }}.hcl + loop: "{{ app_prefixes }}" + loop_control: + label: "Writing policy {{ item | replace('/', '-') }}-k8s-secret" + when: app_prefixes is defined and app_prefixes | length > 0 + +# Build list of new policy names +- name: Build app policy names list + ansible.builtin.set_fact: + _app_policy_names: "{{ app_prefixes | map('replace', '/', '-') | map('regex_replace', '$', '-k8s-secret') | list }}" + when: app_prefixes is defined and app_prefixes | length > 0 + +# Get current hub-role policies +- name: Get current hub-role policies + kubernetes.core.k8s_exec: + namespace: "{{ vault_ns }}" + pod: "{{ vault_pod }}" + command: > + vault read -format=json auth/{{ vault_hub }}/role/{{ vault_hub }}-role + register: _hub_role_result + failed_when: false + when: + - app_prefixes is defined and app_prefixes | length > 0 + - app_update_hub_role | default(true) | bool + +# Parse current policies and merge with new ones +- name: Set current policies fact + ansible.builtin.set_fact: + _current_policies: "{{ (_hub_role_result.stdout | from_json).data.token_policies | default(['default', vault_global_policy ~ '-secret', vault_pushsecrets_policy ~ '-secret', vault_hub ~ '-secret']) }}" + when: + - app_prefixes is defined and app_prefixes | length > 0 + - app_update_hub_role | default(true) | bool + - _hub_role_result.rc == 0 + +- name: Set default policies if hub-role not found + ansible.builtin.set_fact: + _current_policies: "{{ ['default', vault_global_policy ~ '-secret', vault_pushsecrets_policy ~ '-secret', vault_hub ~ '-secret'] }}" + when: + - app_prefixes is defined and app_prefixes | length > 0 + - app_update_hub_role | default(true) | bool + - _hub_role_result.rc != 0 + +# Merge current and new policies (removing duplicates) +- name: Merge policies + ansible.builtin.set_fact: + _merged_policies: "{{ (_current_policies + _app_policy_names) | unique }}" + when: + - app_prefixes is defined and app_prefixes | length > 0 + - app_update_hub_role | default(true) | bool + +# Update hub-role with merged policies +- name: Update hub-role with app policies + kubernetes.core.k8s_exec: + namespace: "{{ vault_ns }}" + pod: "{{ vault_pod }}" + command: > + vault write auth/{{ vault_hub }}/role/{{ vault_hub }}-role + bound_service_account_names="{{ active_external_secrets_sa | default('golang-external-secrets') }}" + bound_service_account_namespaces="{{ active_external_secrets_ns | default('golang-external-secrets') }}" + policies="{{ _merged_policies | join(',') }}" + ttl="{{ vault_hub_ttl }}" + when: + - app_prefixes is defined and app_prefixes | length > 0 + - app_update_hub_role | default(true) | bool + +- name: Display updated hub-role policies + ansible.builtin.debug: + msg: "hub-role policies updated to: {{ _merged_policies | join(', ') }}" + when: + - app_prefixes is defined and app_prefixes | length > 0 + - app_update_hub_role | default(true) | bool + +# Optionally create JWT roles for app-level isolation +# This requires app_jwt_role_config to be defined +- name: Configure JWT role for app (if configured) + kubernetes.core.k8s_exec: + namespace: "{{ vault_ns }}" + pod: "{{ vault_pod }}" + command: > + vault write auth/"{{ vault_hub }}"/role/"{{ _app_name }}"-role + bound_service_account_names="{{ _role_config.service_account_names }}" + bound_service_account_namespaces="{{ _role_config.service_account_namespaces }}" + policies="default,{{ vault_global_policy }}-secret,{{ item | replace('/', '-') }}-k8s-secret" + ttl="{{ _role_config.ttl | default(vault_hub_ttl) }}" + loop: "{{ app_prefixes }}" + loop_control: + label: "Creating JWT role for {{ _app_name }}" + vars: + _app_name: "{{ item | basename }}" + _role_config: "{{ app_jwt_role_config[_app_name] | default({}) }}" + when: + - app_prefixes is defined and app_prefixes | length > 0 + - app_create_jwt_roles | default(false) | bool + - app_jwt_role_config is defined + - _app_name in app_jwt_role_config diff --git a/roles/vault_utils/tasks/vault_jwt.yaml b/roles/vault_utils/tasks/vault_jwt.yaml index 52b2180..f5e2aac 100644 --- a/roles/vault_utils/tasks/vault_jwt.yaml +++ b/roles/vault_utils/tasks/vault_jwt.yaml @@ -102,6 +102,19 @@ not jwt_config_oidc_discovery_url == oidc_discovery_url or not jwt_config_default_role == default_role | default('default') +# Create JWT auth policies from vault_jwt_policies variable +# These are manually defined policies for SPIFFE workloads that need direct Vault access +- name: Write JWT policies directly to Vault + kubernetes.core.k8s_exec: + namespace: "{{ vault_ns }}" + pod: "{{ vault_pod }}" + command: > + bash -e -c "echo '{{ item.policy | b64encode }}' | base64 -d | vault policy write {{ item.name }} -" + loop: "{{ vault_jwt_policies | default([]) }}" + loop_control: + label: "Writing JWT policy {{ item.name }}" + when: vault_jwt_policies is defined and vault_jwt_policies | length > 0 + - name: Run JWT role tasks ansible.builtin.include_tasks: vault_jwt_roles.yaml loop: "{{ vault_jwt_roles | default([]) }}" diff --git a/roles/vault_utils/tasks/vault_secrets_init.yaml b/roles/vault_utils/tasks/vault_secrets_init.yaml index 5bf640f..20c50e5 100644 --- a/roles/vault_utils/tasks/vault_secrets_init.yaml +++ b/roles/vault_utils/tasks/vault_secrets_init.yaml @@ -130,7 +130,42 @@ pod: "{{ vault_pod }}" command: "vault policy write {{ vault_hub }}-secret /tmp/policy-{{ vault_hub }}.hcl" -- name: Configure kubernetes role for hub +# Get current hub-role policies to preserve any custom policies added by patterns +- name: Get current hub-role policies + kubernetes.core.k8s_exec: + namespace: "{{ vault_ns }}" + pod: "{{ vault_pod }}" + command: > + vault read -format=json auth/{{ vault_hub }}/role/{{ vault_hub }}-role + register: _hub_role_result + failed_when: false + +# Define the default policies that VP framework requires +- name: Set default hub-role policies list + ansible.builtin.set_fact: + _default_hub_policies: + - default + - "{{ vault_global_policy }}-secret" + - "{{ vault_pushsecrets_policy }}-secret" + - "{{ vault_hub }}-secret" + +# Get existing policies (empty list if role doesn't exist yet) +- name: Set current policies fact from existing hub-role + ansible.builtin.set_fact: + _current_hub_policies: "{{ (_hub_role_result.stdout | from_json).data.token_policies | default([]) }}" + when: _hub_role_result.rc == 0 + +- name: Set empty current policies if hub-role not found + ansible.builtin.set_fact: + _current_hub_policies: [] + when: _hub_role_result.rc != 0 + +# Merge existing policies with defaults (preserves custom policies, ensures defaults present) +- name: Merge hub-role policies + ansible.builtin.set_fact: + _merged_hub_policies: "{{ (_current_hub_policies + _default_hub_policies) | unique }}" + +- name: Configure kubernetes role for hub with merged policies kubernetes.core.k8s_exec: namespace: "{{ vault_ns }}" pod: "{{ vault_pod }}" @@ -138,4 +173,5 @@ vault write auth/"{{ vault_hub }}"/role/"{{ vault_hub }}"-role bound_service_account_names="{{ active_external_secrets_sa }}" bound_service_account_namespaces="{{ active_external_secrets_ns }}" - policies="default,{{ vault_global_policy }}-secret,{{ vault_pushsecrets_policy }}-secret,{{ vault_hub }}-secret" ttl="{{ vault_hub_ttl }}" + policies="{{ _merged_hub_policies | join(',') }}" + ttl="{{ vault_hub_ttl }}" From f97ac86caed09f02fecf4e7e17e000cb5ff88703 Mon Sep 17 00:00:00 2001 From: Min Zhang Date: Thu, 22 Jan 2026 18:51:33 -0500 Subject: [PATCH 02/13] Address PR review feedback for vault_app_policies Changes based on reviewer feedback: - Add verbosity:1 to debug messages (show only with -v flag) - Integrate vault_app_policies into push_parsed_secrets flow - Move hardcoded default policies to defaults/main.yml - Add idempotency checks for policy and hub-role updates - Replace base64 encoding with heredoc in vault_jwt.yaml - Add vault_jwt_policies to defaults/main.yml - Refactor app_prefixes to unified dict format with prefix/jwt_role - Remove separate app_jwt_role_config (now part of app_prefixes) - Add changed_when:false to read-only vault commands - Add all app policy variables to defaults/main.yml Signed-off-by: Min Zhang --- playbooks/process_secrets.yml | 1 + roles/vault_utils/defaults/main.yml | 30 +++++ .../tasks/push_parsed_secrets.yaml | 20 +++ .../vault_utils/tasks/vault_app_policies.yaml | 114 ++++++++++++------ roles/vault_utils/tasks/vault_jwt.yaml | 6 +- .../vault_utils/tasks/vault_secrets_init.yaml | 23 ++-- 6 files changed, 147 insertions(+), 47 deletions(-) diff --git a/playbooks/process_secrets.yml b/playbooks/process_secrets.yml index c3e30f0..6379111 100644 --- a/playbooks/process_secrets.yml +++ b/playbooks/process_secrets.yml @@ -48,3 +48,4 @@ kubernetes_secret_objects: "{{ secrets_results['kubernetes_secret_objects'] }}" vault_policies: "{{ secrets_results['vault_policies'] }}" parsed_secrets: "{{ secrets_results['parsed_secrets'] }}" + unique_vault_prefixes: "{{ secrets_results['unique_vault_prefixes'] | default([]) }}" diff --git a/roles/vault_utils/defaults/main.yml b/roles/vault_utils/defaults/main.yml index 7b75b48..23f67ee 100644 --- a/roles/vault_utils/defaults/main.yml +++ b/roles/vault_utils/defaults/main.yml @@ -30,3 +30,33 @@ unseal_secret: "vaultkeys" unseal_namespace: "imperative" vault_jwt_config: false vault_jwt_roles: [] + +# Default policies that VP framework requires for hub-role +# Patterns can extend this list but these are always included +vault_hub_role_default_policies: + - default + - "{{ vault_global_policy }}-secret" + - "{{ vault_pushsecrets_policy }}-secret" + - "{{ vault_hub }}-secret" + +# JWT auth policies for workloads needing direct Vault access +# List of {name: "policy-name", policy: "HCL policy content"} +vault_jwt_policies: [] + +# App-specific Vault policy configuration (vault_app_policies.yaml) +# List of app configs, each with 'prefix' and optional 'jwt_role' settings +# Example: +# app_prefixes: +# - prefix: apps/qtodo +# jwt_role: +# service_account_names: "qtodo-sa" +# service_account_namespaces: "qtodo" +# ttl: "15m" +# - prefix: hub/infra/keycloak +app_prefixes: [] +# Capabilities for app policies +app_capabilities: '[\"read\"]' +# Whether to update hub-role with new app policies +app_update_hub_role: true +# Whether to create JWT roles per app (only for entries with jwt_role defined) +app_create_jwt_roles: false diff --git a/roles/vault_utils/tasks/push_parsed_secrets.yaml b/roles/vault_utils/tasks/push_parsed_secrets.yaml index e6a4a2a..5e581b8 100644 --- a/roles/vault_utils/tasks/push_parsed_secrets.yaml +++ b/roles/vault_utils/tasks/push_parsed_secrets.yaml @@ -46,3 +46,23 @@ when: - parsed_secrets is defined - parsed_secrets | length > 0 + +# Create app-specific Vault policies for custom vaultPrefixes +# Filter out 'global' and 'hub' as they're handled by the VP framework +- name: Filter custom vault prefixes for app policies + ansible.builtin.set_fact: + _custom_vault_prefixes: "{{ unique_vault_prefixes | default([]) | reject('equalto', 'global') | reject('equalto', 'hub') | list }}" + +# Convert string list to dict format expected by vault_app_policies.yaml +- name: Convert prefixes to app_prefixes format + ansible.builtin.set_fact: + _app_prefixes_list: "{{ _custom_vault_prefixes | map('regex_replace', '^(.*)$', '{\"prefix\": \"\\1\"}') | map('from_json') | list }}" + when: _custom_vault_prefixes | length > 0 + +- name: Create app-specific Vault policies + ansible.builtin.include_tasks: + file: vault_app_policies.yaml + vars: + app_prefixes: "{{ _app_prefixes_list }}" + app_update_hub_role: true + when: _custom_vault_prefixes | length > 0 diff --git a/roles/vault_utils/tasks/vault_app_policies.yaml b/roles/vault_utils/tasks/vault_app_policies.yaml index f7aaf50..2740ff2 100644 --- a/roles/vault_utils/tasks/vault_app_policies.yaml +++ b/roles/vault_utils/tasks/vault_app_policies.yaml @@ -7,14 +7,13 @@ # specific path, enabling application-level secret isolation. # # Required variables: -# app_prefixes: List of app prefixes to create policies for -# e.g., ["apps/qtodo", "hub/infra/keycloak"] +# app_prefixes: List of app configs with 'prefix' key and optional 'jwt_role' +# e.g., [{prefix: "apps/qtodo"}, {prefix: "hub/infra/keycloak"}] # # Optional variables: # app_capabilities: Capabilities for app policies (default: read) # app_update_hub_role: Whether to update hub-role with new policies (default: true) # app_create_jwt_roles: Whether to create JWT roles per app (default: false) -# app_jwt_role_config: Dict mapping app names to JWT role config # # Example usage: # - name: Create app-specific vault policies @@ -22,50 +21,77 @@ # file: vault_app_policies.yaml # vars: # app_prefixes: -# - apps/qtodo -# - hub/infra/keycloak +# - prefix: apps/qtodo +# jwt_role: +# service_account_names: "qtodo-sa" +# service_account_namespaces: "qtodo" +# - prefix: hub/infra/keycloak # app_update_hub_role: true - name: Debug - Show app prefixes to process ansible.builtin.debug: msg: "Processing app prefixes: {{ app_prefixes }}" + verbosity: 1 when: app_prefixes is defined and app_prefixes | length > 0 - name: Skip if no app prefixes defined ansible.builtin.debug: msg: "No app_prefixes defined, skipping app policy creation" + verbosity: 1 when: app_prefixes is not defined or app_prefixes | length == 0 -# Create policy template for each app prefix -- name: Configure app policy template +# Build list of policy names we need +- name: Build app policy names list + ansible.builtin.set_fact: + _app_policy_names: "{{ app_prefixes | map(attribute='prefix') | map('replace', '/', '-') | map('regex_replace', '$', '-k8s-secret') | list }}" + when: app_prefixes is defined and app_prefixes | length > 0 + +# Get existing policies from Vault to check what needs to be created +- name: Get existing Vault policies kubernetes.core.k8s_exec: namespace: "{{ vault_ns }}" pod: "{{ vault_pod }}" - command: > - bash -e -c "echo 'path \"secret/data/{{ item }}/*\" { capabilities = [\"read\"] }' > /tmp/policy-{{ item | replace('/', '-') }}.hcl" - loop: "{{ app_prefixes }}" - loop_control: - label: "Creating policy template for {{ item }}" + command: vault policy list -format=json + register: _existing_policies_result + changed_when: false + when: app_prefixes is defined and app_prefixes | length > 0 + +- name: Parse existing policies + ansible.builtin.set_fact: + _existing_policies: "{{ _existing_policies_result.stdout | from_json }}" + when: + - app_prefixes is defined and app_prefixes | length > 0 + - _existing_policies_result.rc == 0 + +# Filter to only policies that don't already exist +- name: Determine policies to create + ansible.builtin.set_fact: + _policies_to_create: "{{ _app_policy_names | difference(_existing_policies | default([])) }}" when: app_prefixes is defined and app_prefixes | length > 0 -# Write the policy to Vault +- name: Debug - Show policies to create + ansible.builtin.debug: + msg: "Policies to create: {{ _policies_to_create | default([]) }} (existing: {{ _existing_policies | default([]) }})" + verbosity: 1 + when: app_prefixes is defined and app_prefixes | length > 0 + +# Create only policies that don't exist - name: Configure app policy in Vault kubernetes.core.k8s_exec: namespace: "{{ vault_ns }}" pod: "{{ vault_pod }}" command: > - vault policy write {{ item | replace('/', '-') }}-k8s-secret - /tmp/policy-{{ item | replace('/', '-') }}.hcl + bash -e -c "echo 'path \"secret/data/{{ _prefix }}/*\" { capabilities = [\"read\"] }' | + vault policy write {{ _prefix | replace('/', '-') }}-k8s-secret -" loop: "{{ app_prefixes }}" loop_control: - label: "Writing policy {{ item | replace('/', '-') }}-k8s-secret" - when: app_prefixes is defined and app_prefixes | length > 0 - -# Build list of new policy names -- name: Build app policy names list - ansible.builtin.set_fact: - _app_policy_names: "{{ app_prefixes | map('replace', '/', '-') | map('regex_replace', '$', '-k8s-secret') | list }}" - when: app_prefixes is defined and app_prefixes | length > 0 + label: "Writing policy {{ _prefix | replace('/', '-') }}-k8s-secret" + vars: + _prefix: "{{ item.prefix }}" + _policy_name: "{{ item.prefix | replace('/', '-') }}-k8s-secret" + when: + - app_prefixes is defined and app_prefixes | length > 0 + - _policy_name in (_policies_to_create | default([])) # Get current hub-role policies - name: Get current hub-role policies @@ -76,6 +102,7 @@ vault read -format=json auth/{{ vault_hub }}/role/{{ vault_hub }}-role register: _hub_role_result failed_when: false + changed_when: false when: - app_prefixes is defined and app_prefixes | length > 0 - app_update_hub_role | default(true) | bool @@ -83,7 +110,7 @@ # Parse current policies and merge with new ones - name: Set current policies fact ansible.builtin.set_fact: - _current_policies: "{{ (_hub_role_result.stdout | from_json).data.token_policies | default(['default', vault_global_policy ~ '-secret', vault_pushsecrets_policy ~ '-secret', vault_hub ~ '-secret']) }}" + _current_policies: "{{ (_hub_role_result.stdout | from_json).data.token_policies | default(vault_hub_role_default_policies) }}" when: - app_prefixes is defined and app_prefixes | length > 0 - app_update_hub_role | default(true) | bool @@ -91,7 +118,7 @@ - name: Set default policies if hub-role not found ansible.builtin.set_fact: - _current_policies: "{{ ['default', vault_global_policy ~ '-secret', vault_pushsecrets_policy ~ '-secret', vault_hub ~ '-secret'] }}" + _current_policies: "{{ vault_hub_role_default_policies }}" when: - app_prefixes is defined and app_prefixes | length > 0 - app_update_hub_role | default(true) | bool @@ -105,7 +132,23 @@ - app_prefixes is defined and app_prefixes | length > 0 - app_update_hub_role | default(true) | bool -# Update hub-role with merged policies +# Check if hub-role policies need updating +- name: Check if hub-role policies changed + ansible.builtin.set_fact: + _hub_role_policies_changed: "{{ _current_policies | sort != _merged_policies | sort }}" + when: + - app_prefixes is defined and app_prefixes | length > 0 + - app_update_hub_role | default(true) | bool + +- name: Debug - Hub role policy change status + ansible.builtin.debug: + msg: "Hub role policies changed: {{ _hub_role_policies_changed }} (current: {{ _current_policies | sort }}, merged: {{ _merged_policies | sort }})" + verbosity: 1 + when: + - app_prefixes is defined and app_prefixes | length > 0 + - app_update_hub_role | default(true) | bool + +# Update hub-role only if policies have changed - name: Update hub-role with app policies kubernetes.core.k8s_exec: namespace: "{{ vault_ns }}" @@ -119,34 +162,35 @@ when: - app_prefixes is defined and app_prefixes | length > 0 - app_update_hub_role | default(true) | bool + - _hub_role_policies_changed | bool - name: Display updated hub-role policies ansible.builtin.debug: msg: "hub-role policies updated to: {{ _merged_policies | join(', ') }}" + verbosity: 1 when: - app_prefixes is defined and app_prefixes | length > 0 - app_update_hub_role | default(true) | bool + - _hub_role_policies_changed | bool # Optionally create JWT roles for app-level isolation -# This requires app_jwt_role_config to be defined +# Only creates roles for entries that have jwt_role defined - name: Configure JWT role for app (if configured) kubernetes.core.k8s_exec: namespace: "{{ vault_ns }}" pod: "{{ vault_pod }}" command: > vault write auth/"{{ vault_hub }}"/role/"{{ _app_name }}"-role - bound_service_account_names="{{ _role_config.service_account_names }}" - bound_service_account_namespaces="{{ _role_config.service_account_namespaces }}" - policies="default,{{ vault_global_policy }}-secret,{{ item | replace('/', '-') }}-k8s-secret" - ttl="{{ _role_config.ttl | default(vault_hub_ttl) }}" + bound_service_account_names="{{ item.jwt_role.service_account_names }}" + bound_service_account_namespaces="{{ item.jwt_role.service_account_namespaces }}" + policies="default,{{ vault_global_policy }}-secret,{{ item.prefix | replace('/', '-') }}-k8s-secret" + ttl="{{ item.jwt_role.ttl | default(vault_hub_ttl) }}" loop: "{{ app_prefixes }}" loop_control: label: "Creating JWT role for {{ _app_name }}" vars: - _app_name: "{{ item | basename }}" - _role_config: "{{ app_jwt_role_config[_app_name] | default({}) }}" + _app_name: "{{ item.prefix | basename }}" when: - app_prefixes is defined and app_prefixes | length > 0 - app_create_jwt_roles | default(false) | bool - - app_jwt_role_config is defined - - _app_name in app_jwt_role_config + - item.jwt_role is defined diff --git a/roles/vault_utils/tasks/vault_jwt.yaml b/roles/vault_utils/tasks/vault_jwt.yaml index f5e2aac..5e1133f 100644 --- a/roles/vault_utils/tasks/vault_jwt.yaml +++ b/roles/vault_utils/tasks/vault_jwt.yaml @@ -108,8 +108,10 @@ kubernetes.core.k8s_exec: namespace: "{{ vault_ns }}" pod: "{{ vault_pod }}" - command: > - bash -e -c "echo '{{ item.policy | b64encode }}' | base64 -d | vault policy write {{ item.name }} -" + command: | + bash -e -c 'cat < Date: Wed, 8 Oct 2025 14:34:10 +0200 Subject: [PATCH 03/13] Fix typo --- roles/vault_utils/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/roles/vault_utils/README.md b/roles/vault_utils/README.md index 7aea6f8..50dbec1 100644 --- a/roles/vault_utils/README.md +++ b/roles/vault_utils/README.md @@ -9,7 +9,7 @@ ansible-galaxy collection install kubernetes.core (formerly known as community.k ## Role Variables Defaults as to where the values-secret.yaml file is and the two ways to connect to a kubernetes cluster -(KUBERCONFIG and ~/.kube/config respectively): +(KUBECONFIG and ~/.kube/config respectively): ```yaml values_secret: "{{ lookup('env', 'HOME') }}/values-secret.yaml" From 366e8ef9e37b8d58b3644aa004934fb197807013 Mon Sep 17 00:00:00 2001 From: Michele Baldessari Date: Wed, 8 Oct 2025 15:04:54 +0200 Subject: [PATCH 04/13] Move default_vp_vault_policies to load_secrets_common It is defined to be the exact same in load_secrets_v2 and parse_secrets_v2 --- plugins/module_utils/load_secrets_common.py | 9 +++++++++ plugins/module_utils/load_secrets_v2.py | 10 +--------- plugins/module_utils/parse_secrets_v2.py | 10 +--------- 3 files changed, 11 insertions(+), 18 deletions(-) diff --git a/plugins/module_utils/load_secrets_common.py b/plugins/module_utils/load_secrets_common.py index eb5e51b..0e480e3 100644 --- a/plugins/module_utils/load_secrets_common.py +++ b/plugins/module_utils/load_secrets_common.py @@ -23,6 +23,15 @@ import configparser from collections.abc import MutableMapping +default_vp_vault_policies = { + "validatedPatternDefaultPolicy": ( + "length=20\n" + 'rule "charset" { charset = "abcdefghijklmnopqrstuvwxyz" min-chars = 1 }\n' + 'rule "charset" { charset = "ABCDEFGHIJKLMNOPQRSTUVWXYZ" min-chars = 1 }\n' + 'rule "charset" { charset = "0123456789" min-chars = 1 }\n' + 'rule "charset" { charset = "!@#%^&*" min-chars = 1 }\n' + ) +} def find_dupes(array): """ diff --git a/plugins/module_utils/load_secrets_v2.py b/plugins/module_utils/load_secrets_v2.py index 96b7c68..d0ea624 100644 --- a/plugins/module_utils/load_secrets_v2.py +++ b/plugins/module_utils/load_secrets_v2.py @@ -26,20 +26,12 @@ import time from ansible_collections.rhvp.cluster_utils.plugins.module_utils.load_secrets_common import ( + default_vp_vault_policies, find_dupes, get_ini_value, get_version, ) -default_vp_vault_policies = { - "validatedPatternDefaultPolicy": ( - "length=20\n" - 'rule "charset" { charset = "abcdefghijklmnopqrstuvwxyz" min-chars = 1 }\n' - 'rule "charset" { charset = "ABCDEFGHIJKLMNOPQRSTUVWXYZ" min-chars = 1 }\n' - 'rule "charset" { charset = "0123456789" min-chars = 1 }\n' - 'rule "charset" { charset = "!@#%^&*" min-chars = 1 }\n' - ) -} class LoadSecretsV2: diff --git a/plugins/module_utils/parse_secrets_v2.py b/plugins/module_utils/parse_secrets_v2.py index 4d27511..58d43eb 100644 --- a/plugins/module_utils/parse_secrets_v2.py +++ b/plugins/module_utils/parse_secrets_v2.py @@ -25,21 +25,13 @@ import os from ansible_collections.rhvp.cluster_utils.plugins.module_utils.load_secrets_common import ( + default_vp_vault_policies, find_dupes, get_ini_value, get_version, stringify_dict, ) -default_vp_vault_policies = { - "validatedPatternDefaultPolicy": ( - "length=20\n" - 'rule "charset" { charset = "abcdefghijklmnopqrstuvwxyz" min-chars = 1 }\n' - 'rule "charset" { charset = "ABCDEFGHIJKLMNOPQRSTUVWXYZ" min-chars = 1 }\n' - 'rule "charset" { charset = "0123456789" min-chars = 1 }\n' - 'rule "charset" { charset = "!@#%^&*" min-chars = 1 }\n' - ) -} secret_store_namespace = "validated-patterns-secrets" From d687e00c65d94c482840894caef615d55c0b4feb Mon Sep 17 00:00:00 2001 From: Michele Baldessari Date: Wed, 8 Oct 2025 16:23:16 +0200 Subject: [PATCH 05/13] Introduce a common subclass for load secrets and parse secrets This eliminates a bunch of duplicated code --- plugins/module_utils/load_secrets_common.py | 211 ++++++++++++++++++++ plugins/module_utils/load_secrets_v2.py | 198 +----------------- plugins/module_utils/parse_secrets_v2.py | 164 +-------------- 3 files changed, 225 insertions(+), 348 deletions(-) diff --git a/plugins/module_utils/load_secrets_common.py b/plugins/module_utils/load_secrets_common.py index 0e480e3..bf6c103 100644 --- a/plugins/module_utils/load_secrets_common.py +++ b/plugins/module_utils/load_secrets_common.py @@ -21,6 +21,8 @@ __metaclass__ = type import configparser +import getpass +import os from collections.abc import MutableMapping default_vp_vault_policies = { @@ -33,6 +35,7 @@ ) } + def find_dupes(array): """ Returns duplicate items in a list @@ -158,3 +161,211 @@ def filter_module_args(arg_spec): pass return arg_spec + + +class SecretsV2Base: + """ + Base class with common functionality for V2 secrets handling + """ + + def __init__(self, module, syaml): + self.module = module + self.syaml = syaml + + def _get_vault_policies(self, enable_default_vp_policies=True): + # We start off with the hard-coded default VP policy and add the user-defined ones + if enable_default_vp_policies: + policies = default_vp_vault_policies.copy() + else: + policies = {} + policies.update(self.syaml.get("vaultPolicies", {})) + return policies + + def _get_secrets(self): + return self.syaml.get("secrets", []) + + def _get_field_on_missing_value(self, f): + # By default if 'onMissingValue' is missing we assume we need to + # error out whenever the value is missing + return f.get("onMissingValue", "error") + + def _get_field_value(self, f): + return f.get("value", None) + + def _get_field_path(self, f): + return f.get("path", None) + + def _get_field_ini_file(self, f): + return f.get("ini_file", None) + + def _get_field_kind(self, f): + # value: null will be interpreted with None, so let's just + # check for the existence of the field, as we use 'value: null' to say + # "we want a value/secret and not a file path" + found = [] + for i in ["value", "path", "ini_file"]: + if i in f: + found.append(i) + + if len(found) > 1: # you can only have one of value, path and ini_file + self.module.fail_json(f"Both '{found[0]}' and '{found[1]}' cannot be used") + + if len(found) == 0: + return "" + return found[0] + + def _get_field_prompt(self, f): + return f.get("prompt", None) + + def _get_field_base64(self, f): + return bool(f.get("base64", False)) + + def _get_field_override(self, f): + return bool(f.get("override", False)) + + def _validate_field(self, f): + # These fields are mandatory + try: + unused = f["name"] + except KeyError: + return (False, f"Field {f} is missing name") + + on_missing_value = self._get_field_on_missing_value(f) + if on_missing_value not in ["error", "generate", "prompt"]: + return (False, f"onMissingValue: {on_missing_value} is invalid") + + value = self._get_field_value(f) + path = self._get_field_path(f) + ini_file = self._get_field_ini_file(f) + kind = self._get_field_kind(f) + if kind == "ini_file": + # if we are using ini_file then at least ini_key needs to be defined + # ini_section defaults to 'default' when omitted + ini_key = f.get("ini_key", None) + if ini_key is None: + return ( + False, + "ini_file requires at least ini_key to be defined", + ) + + # Test if base64 is a correct boolean (defaults to False) + unused = self._get_field_base64(f) + unused = self._get_field_override(f) + + vault_policy = f.get("vaultPolicy", None) + if vault_policy is not None and vault_policy not in self._get_vault_policies(): + return ( + False, + f"Secret has vaultPolicy set to {vault_policy} but no such policy exists", + ) + + if on_missing_value in ["error"]: + if ( + (value is None or len(value) < 1) + and (path is None or len(path) < 1) + and (ini_file is None or len(ini_file) < 1) + ): + return ( + False, + "Secret has onMissingValue set to 'error' and has neither value nor path nor ini_file set", + ) + if path is not None and not os.path.isfile(os.path.expanduser(path)): + return (False, f"Field has non-existing path: {path}") + + if ini_file is not None and not os.path.isfile( + os.path.expanduser(ini_file) + ): + return (False, f"Field has non-existing ini_file: {ini_file}") + + if "override" in f: + return ( + False, + "'override' attribute requires 'onMissingValue' to be set to 'generate'", + ) + + if on_missing_value in ["generate"]: + if value is not None: + return ( + False, + "Secret has onMissingValue set to 'generate' but has a value set", + ) + if path is not None: + return ( + False, + "Secret has onMissingValue set to 'generate' but has a path set", + ) + if vault_policy is None: + return ( + False, + "Secret has no vaultPolicy but onMissingValue is set to 'generate'", + ) + + if on_missing_value in ["prompt"]: + # When we prompt, the user needs to set one of the following: + # - value: null # prompt for a secret without a default value + # - value: 123 # prompt for a secret but use a default value + # - path: null # prompt for a file path without a default value + # - path: /tmp/ca.crt # prompt for a file path with a default value + if "value" not in f and "path" not in f: + return ( + False, + "Secret has onMissingValue set to 'prompt' but has no value nor path fields", + ) + + if "override" in f: + return ( + False, + "'override' attribute requires 'onMissingValue' to be set to 'generate'", + ) + + return (True, "") + + def _get_secret_value(self, name, field): + on_missing_value = self._get_field_on_missing_value(field) + # We cannot use match + case as RHEL8 has python 3.9 (it needs 3.10) + # We checked for errors in _validate_secrets() already + if on_missing_value == "error": + value = field.get("value") + # Allow subclasses to override value processing + return self._process_secret_value(value) + elif on_missing_value == "prompt": + prompt = self._get_field_prompt(field) + if prompt is None: + prompt = f"Type secret for {name}/{field['name']}: " + value = self._get_field_value(field) + if value is not None: + prompt += f" [{value}]" + prompt += ": " + return getpass.getpass(prompt) + return None + + def _process_secret_value(self, value): + """ + Process a secret value. Can be overridden by subclasses. + """ + return value + + def _get_file_path(self, name, field): + on_missing_value = self._get_field_on_missing_value(field) + if on_missing_value == "error": + return os.path.expanduser(field.get("path")) + elif on_missing_value == "prompt": + prompt = self._get_field_prompt(field) + path = self._get_field_path(field) + if path is None: + path = "" + + if prompt is None: + text = f"Type path for file {name}/{field['name']} [{path}]: " + else: + text = f"{prompt} [{path}]: " + + newpath = getpass.getpass(text) + if newpath == "": # Set the default if no string was entered + newpath = path + + if os.path.isfile(os.path.expanduser(newpath)): + return newpath + self.module.fail_json(f"File {newpath} not found, exiting") + + self.module.fail_json("File with wrong onMissingValue") diff --git a/plugins/module_utils/load_secrets_v2.py b/plugins/module_utils/load_secrets_v2.py index d0ea624..76e8ea6 100644 --- a/plugins/module_utils/load_secrets_v2.py +++ b/plugins/module_utils/load_secrets_v2.py @@ -21,26 +21,23 @@ __metaclass__ = type import base64 -import getpass import os import time from ansible_collections.rhvp.cluster_utils.plugins.module_utils.load_secrets_common import ( - default_vp_vault_policies, find_dupes, get_ini_value, get_version, + SecretsV2Base, ) - -class LoadSecretsV2: +class LoadSecretsV2(SecretsV2Base): def __init__(self, module, syaml, namespace, pod): - self.module = module + super().__init__(module, syaml) self.namespace = namespace self.pod = pod - self.syaml = syaml def _run_command(self, command, attempts=1, sleep=3, checkrc=True): """ @@ -78,156 +75,9 @@ def _get_backingstore(self): """ return str(self.syaml.get("backingStore", "vault")) - def _get_vault_policies(self, enable_default_vp_policies=True): - # We start off with the hard-coded default VP policy and add the user-defined ones - if enable_default_vp_policies: - policies = default_vp_vault_policies.copy() - else: - policies = {} - policies.update(self.syaml.get("vaultPolicies", {})) - return policies - def _get_secrets(self): return self.syaml.get("secrets", {}) - def _get_field_on_missing_value(self, f): - # By default if 'onMissingValue' is missing we assume we need to - # error out whenever the value is missing - return f.get("onMissingValue", "error") - - def _get_field_value(self, f): - return f.get("value", None) - - def _get_field_path(self, f): - return f.get("path", None) - - def _get_field_ini_file(self, f): - return f.get("ini_file", None) - - def _get_field_kind(self, f): - # value: null will be interpreted with None, so let's just - # check for the existence of the field, as we use 'value: null' to say - # "we want a value/secret and not a file path" - found = [] - for i in ["value", "path", "ini_file"]: - if i in f: - found.append(i) - - if len(found) > 1: # you can only have one of value, path and ini_file - self.module.fail_json(f"Both '{found[0]}' and '{found[1]}' cannot be used") - - if len(found) == 0: - return "" - return found[0] - - def _get_field_prompt(self, f): - return f.get("prompt", None) - - def _get_field_base64(self, f): - return bool(f.get("base64", False)) - - def _get_field_override(self, f): - return bool(f.get("override", False)) - - # This function could use some rewriting and it should call a specific validation function - # for each type (value, path, ini_file) - def _validate_field(self, f): - # These fields are mandatory - try: - unused = f["name"] - except KeyError: - return (False, f"Field {f} is missing name") - - on_missing_value = self._get_field_on_missing_value(f) - if on_missing_value not in ["error", "generate", "prompt"]: - return (False, f"onMissingValue: {on_missing_value} is invalid") - - value = self._get_field_value(f) - path = self._get_field_path(f) - ini_file = self._get_field_ini_file(f) - kind = self._get_field_kind(f) - if kind == "ini_file": - # if we are using ini_file then at least ini_key needs to be defined - # ini_section defaults to 'default' when omitted - ini_key = f.get("ini_key", None) - if ini_key is None: - return ( - False, - "ini_file requires at least ini_key to be defined", - ) - - # Test if base64 is a correct boolean (defaults to False) - unused = self._get_field_base64(f) - unused = self._get_field_override(f) - - vault_policy = f.get("vaultPolicy", None) - if vault_policy is not None and vault_policy not in self._get_vault_policies(): - return ( - False, - f"Secret has vaultPolicy set to {vault_policy} but no such policy exists", - ) - - if on_missing_value in ["error"]: - if ( - (value is None or len(value) < 1) - and (path is None or len(path) < 1) - and (ini_file is None or len(ini_file) < 1) - ): - return ( - False, - "Secret has onMissingValue set to 'error' and has neither value nor path nor ini_file set", - ) - if path is not None and not os.path.isfile(os.path.expanduser(path)): - return (False, f"Field has non-existing path: {path}") - - if ini_file is not None and not os.path.isfile( - os.path.expanduser(ini_file) - ): - return (False, f"Field has non-existing ini_file: {ini_file}") - - if "override" in f: - return ( - False, - "'override' attribute requires 'onMissingValue' to be set to 'generate'", - ) - - if on_missing_value in ["generate"]: - if value is not None: - return ( - False, - "Secret has onMissingValue set to 'generate' but has a value set", - ) - if path is not None: - return ( - False, - "Secret has onMissingValue set to 'generate' but has a path set", - ) - if vault_policy is None: - return ( - False, - "Secret has no vaultPolicy but onMissingValue is set to 'generate'", - ) - - if on_missing_value in ["prompt"]: - # When we prompt, the user needs to set one of the following: - # - value: null # prompt for a secret without a default value - # - value: 123 # prompt for a secret but use a default value - # - path: null # prompt for a file path without a default value - # - path: /tmp/ca.crt # prompt for a file path with a default value - if "value" not in f and "path" not in f: - return ( - False, - "Secret has onMissingValue set to 'prompt' but has no value nor path fields", - ) - - if "override" in f: - return ( - False, - "'override' attribute requires 'onMissingValue' to be set to 'generate'", - ) - - return (True, "") - def _validate_secrets(self): secrets = self._get_secrets() if len(secrets) == 0: @@ -300,48 +150,6 @@ def sanitize_values(self): if not ret: self.module.fail_json(msg) - def _get_secret_value(self, name, field): - on_missing_value = self._get_field_on_missing_value(field) - # We cannot use match + case as RHEL8 has python 3.9 (it needs 3.10) - # We checked for errors in _validate_secrets() already - if on_missing_value == "error": - return field.get("value") - elif on_missing_value == "prompt": - prompt = self._get_field_prompt(field) - if prompt is None: - prompt = f"Type secret for {name}/{field['name']}: " - value = self._get_field_value(field) - if value is not None: - prompt += f" [{value}]" - prompt += ": " - return getpass.getpass(prompt) - return None - - def _get_file_path(self, name, field): - on_missing_value = self._get_field_on_missing_value(field) - if on_missing_value == "error": - return os.path.expanduser(field.get("path")) - elif on_missing_value == "prompt": - prompt = self._get_field_prompt(field) - path = self._get_field_path(field) - if path is None: - path = "" - - if prompt is None: - text = f"Type path for file {name}/{field['name']} [{path}]: " - else: - text = f"{prompt} [{path}]: " - - newpath = getpass.getpass(text) - if newpath == "": # Set the default if no string was entered - newpath = path - - if os.path.isfile(os.path.expanduser(newpath)): - return newpath - self.module.fail_json(f"File {newpath} not found, exiting") - - self.module.fail_json("File with wrong onMissingValue") - def _vault_secret_attr_exists(self, mount, prefix, secret_name, attribute): cmd = ( f"oc exec -n {self.namespace} {self.pod} -i -- sh -c " diff --git a/plugins/module_utils/parse_secrets_v2.py b/plugins/module_utils/parse_secrets_v2.py index 58d43eb..d15d3c8 100644 --- a/plugins/module_utils/parse_secrets_v2.py +++ b/plugins/module_utils/parse_secrets_v2.py @@ -21,26 +21,24 @@ __metaclass__ = type import base64 -import getpass import os from ansible_collections.rhvp.cluster_utils.plugins.module_utils.load_secrets_common import ( - default_vp_vault_policies, find_dupes, get_ini_value, get_version, stringify_dict, + SecretsV2Base, ) secret_store_namespace = "validated-patterns-secrets" -class ParseSecretsV2: +class ParseSecretsV2(SecretsV2Base): def __init__(self, module, syaml, secrets_backing_store): - self.module = module - self.syaml = syaml + super().__init__(module, syaml) self.secrets_backing_store = str(secrets_backing_store) self.secret_store_namespace = None self.parsed_secrets = {} @@ -74,11 +72,8 @@ def _get_backingstore(self): return self.secrets_backing_store def _get_vault_policies(self, enable_default_vp_policies=True): - # We start off with the hard-coded default VP policy and add the user-defined ones - if enable_default_vp_policies: - policies = default_vp_vault_policies.copy() - else: - policies = {} + # Override base class to add YAML sanitization for policies + policies = super()._get_vault_policies(enable_default_vp_policies) # This is useful for embedded newlines, which occur with YAML # flow-type scalars (|, |- for example) @@ -94,20 +89,6 @@ def _get_secrets(self): # We also check for None here to cover when there is no jinja filter is used (unit tests) return [] if secrets == "None" or secrets is None else secrets - def _get_field_on_missing_value(self, f): - # By default if 'onMissingValue' is missing we assume we need to - # error out whenever the value is missing - return f.get("onMissingValue", "error") - - def _get_field_value(self, f): - return f.get("value", None) - - def _get_field_path(self, f): - return f.get("path", None) - - def _get_field_ini_file(self, f): - return f.get("ini_file", None) - def _get_field_annotations(self, f): return f.get("annotations", {}) @@ -115,9 +96,7 @@ def _get_field_labels(self, f): return f.get("labels", {}) def _get_field_kind(self, f): - # value: null will be interpreted with None, so let's just - # check for the existence of the field, as we use 'value: null' to say - # "we want a value/secret and not a file path" + # Override the base class implementation to include field name in error message found = [] for i in ["value", "path", "ini_file"]: if i in f: @@ -133,15 +112,6 @@ def _get_field_kind(self, f): return "" return found[0] - def _get_field_prompt(self, f): - return f.get("prompt", None) - - def _get_field_base64(self, f): - return bool(f.get("base64", False)) - - def _get_field_override(self, f): - return bool(f.get("override", False)) - def _get_secret_store_namespace(self): return str(self.syaml.get("secretStoreNamespace", secret_store_namespace)) @@ -244,82 +214,6 @@ def parse(self): return total_secrets - # This function could use some rewriting and it should call a specific validation function - # for each type (value, path, ini_file) - def _validate_field(self, f): - # These fields are mandatory - try: - unused = f["name"] - except KeyError: - return (False, f"Field {f} is missing name") - - on_missing_value = self._get_field_on_missing_value(f) - if on_missing_value not in ["error", "generate", "prompt"]: - return (False, f"onMissingValue: {on_missing_value} is invalid") - - value = self._get_field_value(f) - path = self._get_field_path(f) - ini_file = self._get_field_ini_file(f) - kind = self._get_field_kind(f) - if kind == "ini_file": - # if we are using ini_file then at least ini_key needs to be defined - # ini_section defaults to 'default' when omitted - ini_key = f.get("ini_key", None) - if ini_key is None: - return ( - False, - "ini_file requires at least ini_key to be defined", - ) - - # Test if base64 is a correct boolean (defaults to False) - unused = self._get_field_base64(f) - unused = self._get_field_override(f) - - vault_policy = f.get("vaultPolicy", None) - if vault_policy is not None and vault_policy not in self._get_vault_policies(): - return ( - False, - f"Secret has vaultPolicy set to {vault_policy} but no such policy exists", - ) - - if on_missing_value in ["error"]: - if ( - (value is None or len(value) < 1) - and (path is None or len(path) < 1) - and (ini_file is None or len(ini_file) < 1) - ): - return ( - False, - "Secret has onMissingValue set to 'error' and has neither value nor path nor ini_file set", - ) - if path is not None and not os.path.isfile(os.path.expanduser(path)): - return (False, f"Field has non-existing path: {path}") - - if ini_file is not None and not os.path.isfile( - os.path.expanduser(ini_file) - ): - return (False, f"Field has non-existing ini_file: {ini_file}") - - if on_missing_value in ["prompt"]: - # When we prompt, the user needs to set one of the following: - # - value: null # prompt for a secret without a default value - # - value: 123 # prompt for a secret but use a default value - # - path: null # prompt for a file path without a default value - # - path: /tmp/ca.crt # prompt for a file path with a default value - if "value" not in f and "path" not in f: - return ( - False, - "Secret has onMissingValue set to 'prompt' but has no value nor path fields", - ) - - if "override" in f: - return ( - False, - "'override' attribute requires 'onMissingValue' to be set to 'generate'", - ) - - return (True, "") - def _validate_secrets(self): backing_store = self._get_backingstore() secrets = self._get_secrets() @@ -406,47 +300,11 @@ def sanitize_values(self): if not ret: self.module.fail_json(msg) - def _get_secret_value(self, name, field): - on_missing_value = self._get_field_on_missing_value(field) - # We cannot use match + case as RHEL8 has python 3.9 (it needs 3.10) - # We checked for errors in _validate_secrets() already - if on_missing_value == "error": - return self._sanitize_yaml_value(field.get("value")) - elif on_missing_value == "prompt": - prompt = self._get_field_prompt(field) - if prompt is None: - prompt = f"Type secret for {name}/{field['name']}: " - value = self._get_field_value(field) - if value is not None: - prompt += f" [{value}]" - prompt += ": " - return getpass.getpass(prompt) - return None - - def _get_file_path(self, name, field): - on_missing_value = self._get_field_on_missing_value(field) - if on_missing_value == "error": - return os.path.expanduser(field.get("path")) - elif on_missing_value == "prompt": - prompt = self._get_field_prompt(field) - path = self._get_field_path(field) - if path is None: - path = "" - - if prompt is None: - text = f"Type path for file {name}/{field['name']} [{path}]: " - else: - text = f"{prompt} [{path}]: " - - newpath = getpass.getpass(text) - if newpath == "": # Set the default if no string was entered - newpath = path - - if os.path.isfile(os.path.expanduser(newpath)): - return newpath - self.module.fail_json(f"File {newpath} not found, exiting") - - self.module.fail_json("File with wrong onMissingValue") + def _process_secret_value(self, value): + """ + Override base class to add YAML sanitization + """ + return self._sanitize_yaml_value(value) def _inject_field(self, secret_name, f): on_missing_value = self._get_field_on_missing_value(f) From 64acb17bda335726d26c70fb7e5db187bb14fbae Mon Sep 17 00:00:00 2001 From: Michele Baldessari Date: Wed, 8 Oct 2025 16:31:05 +0200 Subject: [PATCH 06/13] Some cleanups now that python3.11 is the minimum version --- plugins/module_utils/load_secrets_common.py | 81 ++++++++++----------- 1 file changed, 40 insertions(+), 41 deletions(-) diff --git a/plugins/module_utils/load_secrets_common.py b/plugins/module_utils/load_secrets_common.py index bf6c103..8af2a3b 100644 --- a/plugins/module_utils/load_secrets_common.py +++ b/plugins/module_utils/load_secrets_common.py @@ -174,10 +174,7 @@ def __init__(self, module, syaml): def _get_vault_policies(self, enable_default_vp_policies=True): # We start off with the hard-coded default VP policy and add the user-defined ones - if enable_default_vp_policies: - policies = default_vp_vault_policies.copy() - else: - policies = {} + policies = default_vp_vault_policies.copy() if enable_default_vp_policies else {} policies.update(self.syaml.get("vaultPolicies", {})) return policies @@ -322,22 +319,23 @@ def _validate_field(self, f): def _get_secret_value(self, name, field): on_missing_value = self._get_field_on_missing_value(field) - # We cannot use match + case as RHEL8 has python 3.9 (it needs 3.10) # We checked for errors in _validate_secrets() already - if on_missing_value == "error": - value = field.get("value") - # Allow subclasses to override value processing - return self._process_secret_value(value) - elif on_missing_value == "prompt": - prompt = self._get_field_prompt(field) - if prompt is None: - prompt = f"Type secret for {name}/{field['name']}: " - value = self._get_field_value(field) - if value is not None: - prompt += f" [{value}]" - prompt += ": " - return getpass.getpass(prompt) - return None + match on_missing_value: + case "error": + value = field.get("value") + # Allow subclasses to override value processing + return self._process_secret_value(value) + case "prompt": + prompt = self._get_field_prompt(field) + if prompt is None: + prompt = f"Type secret for {name}/{field['name']}: " + value = self._get_field_value(field) + if value is not None: + prompt += f" [{value}]" + prompt += ": " + return getpass.getpass(prompt) + case _: + return None def _process_secret_value(self, value): """ @@ -347,25 +345,26 @@ def _process_secret_value(self, value): def _get_file_path(self, name, field): on_missing_value = self._get_field_on_missing_value(field) - if on_missing_value == "error": - return os.path.expanduser(field.get("path")) - elif on_missing_value == "prompt": - prompt = self._get_field_prompt(field) - path = self._get_field_path(field) - if path is None: - path = "" - - if prompt is None: - text = f"Type path for file {name}/{field['name']} [{path}]: " - else: - text = f"{prompt} [{path}]: " - - newpath = getpass.getpass(text) - if newpath == "": # Set the default if no string was entered - newpath = path - - if os.path.isfile(os.path.expanduser(newpath)): - return newpath - self.module.fail_json(f"File {newpath} not found, exiting") - - self.module.fail_json("File with wrong onMissingValue") + match on_missing_value: + case "error": + return os.path.expanduser(field.get("path")) + case "prompt": + prompt = self._get_field_prompt(field) + path = self._get_field_path(field) + if path is None: + path = "" + + if prompt is None: + text = f"Type path for file {name}/{field['name']} [{path}]: " + else: + text = f"{prompt} [{path}]: " + + newpath = getpass.getpass(text) + if newpath == "": # Set the default if no string was entered + newpath = path + + if os.path.isfile(os.path.expanduser(newpath)): + return newpath + self.module.fail_json(f"File {newpath} not found, exiting") + case _: + self.module.fail_json("File with wrong onMissingValue") From 0845ea27883ff97a214568a48c465f3bd70ffc52 Mon Sep 17 00:00:00 2001 From: Michele Baldessari Date: Wed, 8 Oct 2025 16:56:49 +0200 Subject: [PATCH 07/13] Cleanup some complex functions Simplify the code a bit and make use of match/case to make it all a bit more readable --- Makefile | 1 + plugins/module_utils/load_secrets_common.py | 190 ++++++++++++-------- plugins/module_utils/load_secrets_v2.py | 2 +- plugins/module_utils/parse_secrets_v2.py | 3 +- 4 files changed, 118 insertions(+), 78 deletions(-) diff --git a/Makefile b/Makefile index ce4c792..5b2e254 100644 --- a/Makefile +++ b/Makefile @@ -18,6 +18,7 @@ super-linter: ## Runs super linter locally -e VALIDATE_JSON_PRETTIER=false \ -e VALIDATE_MARKDOWN_PRETTIER=false \ -e VALIDATE_PYTHON_PYLINT=false \ + -e VALIDATE_PYTHON_PYINK=false \ -e VALIDATE_PYTHON_RUFF_FORMAT=false \ -e VALIDATE_SHELL_SHFMT=false \ -e VALIDATE_YAML=false \ diff --git a/plugins/module_utils/load_secrets_common.py b/plugins/module_utils/load_secrets_common.py index 8af2a3b..07f09f9 100644 --- a/plugins/module_utils/load_secrets_common.py +++ b/plugins/module_utils/load_secrets_common.py @@ -174,7 +174,9 @@ def __init__(self, module, syaml): def _get_vault_policies(self, enable_default_vp_policies=True): # We start off with the hard-coded default VP policy and add the user-defined ones - policies = default_vp_vault_policies.copy() if enable_default_vp_policies else {} + policies = ( + default_vp_vault_policies.copy() if enable_default_vp_policies else {} + ) policies.update(self.syaml.get("vaultPolicies", {})) return policies @@ -221,99 +223,137 @@ def _get_field_override(self, f): return bool(f.get("override", False)) def _validate_field(self, f): - # These fields are mandatory - try: - unused = f["name"] - except KeyError: + # Check mandatory fields + if "name" not in f: return (False, f"Field {f} is missing name") on_missing_value = self._get_field_on_missing_value(f) if on_missing_value not in ["error", "generate", "prompt"]: return (False, f"onMissingValue: {on_missing_value} is invalid") - value = self._get_field_value(f) - path = self._get_field_path(f) - ini_file = self._get_field_ini_file(f) + # Validate field structure and types + result = self._validate_field_structure(f) + if not result[0]: + return result + + # Validate vault policy + result = self._validate_vault_policy(f) + if not result[0]: + return result + + # Validate based on onMissingValue type + match on_missing_value: + case "error": + return self._validate_error_mode(f) + case "generate": + return self._validate_generate_mode(f) + case "prompt": + return self._validate_prompt_mode(f) + + return (True, "") + + def _validate_field_structure(self, f): + """Validate field structure and basic types""" kind = self._get_field_kind(f) if kind == "ini_file": - # if we are using ini_file then at least ini_key needs to be defined - # ini_section defaults to 'default' when omitted ini_key = f.get("ini_key", None) if ini_key is None: - return ( - False, - "ini_file requires at least ini_key to be defined", - ) + return (False, "ini_file requires at least ini_key to be defined") + + # Test if base64 and override are correct booleans + self._get_field_base64(f) + self._get_field_override(f) - # Test if base64 is a correct boolean (defaults to False) - unused = self._get_field_base64(f) - unused = self._get_field_override(f) + return (True, "") + def _validate_vault_policy(self, f): + """Validate vault policy exists if specified""" vault_policy = f.get("vaultPolicy", None) if vault_policy is not None and vault_policy not in self._get_vault_policies(): return ( False, f"Secret has vaultPolicy set to {vault_policy} but no such policy exists", ) + return (True, "") - if on_missing_value in ["error"]: - if ( - (value is None or len(value) < 1) - and (path is None or len(path) < 1) - and (ini_file is None or len(ini_file) < 1) - ): - return ( - False, - "Secret has onMissingValue set to 'error' and has neither value nor path nor ini_file set", - ) - if path is not None and not os.path.isfile(os.path.expanduser(path)): - return (False, f"Field has non-existing path: {path}") - - if ini_file is not None and not os.path.isfile( - os.path.expanduser(ini_file) - ): - return (False, f"Field has non-existing ini_file: {ini_file}") - - if "override" in f: - return ( - False, - "'override' attribute requires 'onMissingValue' to be set to 'generate'", - ) - - if on_missing_value in ["generate"]: - if value is not None: - return ( - False, - "Secret has onMissingValue set to 'generate' but has a value set", - ) - if path is not None: - return ( - False, - "Secret has onMissingValue set to 'generate' but has a path set", - ) - if vault_policy is None: - return ( - False, - "Secret has no vaultPolicy but onMissingValue is set to 'generate'", - ) - - if on_missing_value in ["prompt"]: - # When we prompt, the user needs to set one of the following: - # - value: null # prompt for a secret without a default value - # - value: 123 # prompt for a secret but use a default value - # - path: null # prompt for a file path without a default value - # - path: /tmp/ca.crt # prompt for a file path with a default value - if "value" not in f and "path" not in f: - return ( - False, - "Secret has onMissingValue set to 'prompt' but has no value nor path fields", - ) - - if "override" in f: - return ( - False, - "'override' attribute requires 'onMissingValue' to be set to 'generate'", - ) + def _validate_error_mode(self, f): + """Validate fields when onMissingValue is 'error'""" + value = self._get_field_value(f) + path = self._get_field_path(f) + ini_file = self._get_field_ini_file(f) + + # Check that at least one source is provided and not empty + if ( + (value is None or len(value) < 1) + and (path is None or len(path) < 1) + and (ini_file is None or len(ini_file) < 1) + ): + return ( + False, + "Secret has onMissingValue set to 'error' and has neither value nor path nor ini_file set", + ) + + # Validate file paths exist + if path is not None and not os.path.isfile(os.path.expanduser(path)): + return (False, f"Field has non-existing path: {path}") + + if ini_file is not None and not os.path.isfile(os.path.expanduser(ini_file)): + return (False, f"Field has non-existing ini_file: {ini_file}") + + # Override not allowed in error mode + if "override" in f: + return ( + False, + "'override' attribute requires 'onMissingValue' to be set to 'generate'", + ) + + return (True, "") + + def _validate_generate_mode(self, f): + """Validate fields when onMissingValue is 'generate'""" + value = self._get_field_value(f) + path = self._get_field_path(f) + vault_policy = f.get("vaultPolicy", None) + + if value is not None: + return ( + False, + "Secret has onMissingValue set to 'generate' but has a value set", + ) + + if path is not None: + return ( + False, + "Secret has onMissingValue set to 'generate' but has a path set", + ) + + if vault_policy is None: + return ( + False, + "Secret has no vaultPolicy but onMissingValue is set to 'generate'", + ) + + return (True, "") + + def _validate_prompt_mode(self, f): + """Validate fields when onMissingValue is 'prompt'""" + # When we prompt, the user needs to set one of the following: + # - value: null # prompt for a secret without a default value + # - value: 123 # prompt for a secret but use a default value + # - path: null # prompt for a file path without a default value + # - path: /tmp/ca.crt # prompt for a file path with a default value + if "value" not in f and "path" not in f: + return ( + False, + "Secret has onMissingValue set to 'prompt' but has no value nor path fields", + ) + + # Override not allowed in prompt mode + if "override" in f: + return ( + False, + "'override' attribute requires 'onMissingValue' to be set to 'generate'", + ) return (True, "") diff --git a/plugins/module_utils/load_secrets_v2.py b/plugins/module_utils/load_secrets_v2.py index 76e8ea6..356c654 100644 --- a/plugins/module_utils/load_secrets_v2.py +++ b/plugins/module_utils/load_secrets_v2.py @@ -25,10 +25,10 @@ import time from ansible_collections.rhvp.cluster_utils.plugins.module_utils.load_secrets_common import ( + SecretsV2Base, find_dupes, get_ini_value, get_version, - SecretsV2Base, ) diff --git a/plugins/module_utils/parse_secrets_v2.py b/plugins/module_utils/parse_secrets_v2.py index d15d3c8..cf5b564 100644 --- a/plugins/module_utils/parse_secrets_v2.py +++ b/plugins/module_utils/parse_secrets_v2.py @@ -24,14 +24,13 @@ import os from ansible_collections.rhvp.cluster_utils.plugins.module_utils.load_secrets_common import ( + SecretsV2Base, find_dupes, get_ini_value, get_version, stringify_dict, - SecretsV2Base, ) - secret_store_namespace = "validated-patterns-secrets" From 9699ad38ad1a6de366dfa123d47e2369c3cfbe88 Mon Sep 17 00:00:00 2001 From: Michele Baldessari Date: Wed, 8 Oct 2025 17:07:24 +0200 Subject: [PATCH 08/13] Split up _inject_field to make it more readable --- plugins/module_utils/load_secrets_v2.py | 164 ++++++++++++++---------- 1 file changed, 94 insertions(+), 70 deletions(-) diff --git a/plugins/module_utils/load_secrets_v2.py b/plugins/module_utils/load_secrets_v2.py index 356c654..85b8850 100644 --- a/plugins/module_utils/load_secrets_v2.py +++ b/plugins/module_utils/load_secrets_v2.py @@ -163,79 +163,103 @@ def _vault_secret_attr_exists(self, mount, prefix, secret_name, attribute): return False def _inject_field(self, secret_name, f, mount, prefixes, first=False): + verb = "put" if first else "patch" + kind = self._get_field_kind(f) + + match kind: + case "value" | "": + self._inject_value_field(secret_name, f, mount, prefixes, verb) + case "path": + self._inject_path_field(secret_name, f, mount, prefixes, verb) + case "ini_file": + self._inject_ini_field(secret_name, f, mount, prefixes, verb) + + def _inject_value_field(self, secret_name, f, mount, prefixes, verb): + """Inject a value-based field into vault""" on_missing_value = self._get_field_on_missing_value(f) - override = self._get_field_override(f) + + if on_missing_value == "generate": + self._inject_generated_secret(secret_name, f, mount, prefixes, verb) + else: + self._inject_provided_secret(secret_name, f, mount, prefixes, verb) + + def _inject_generated_secret(self, secret_name, f, mount, prefixes, verb): + """Generate and inject a secret using vault policy""" kind = self._get_field_kind(f) - # If we're generating the password then we just push the secret in the vault directly - verb = "put" if first else "patch" + if kind == "path": + self.module.fail_json( + "You cannot have onMissingValue set to 'generate' with a path" + ) + + override = self._get_field_override(f) b64 = self._get_field_base64(f) - if kind in ["value", ""]: - if on_missing_value == "generate": - if kind == "path": - self.module.fail_json( - "You cannot have onMissingValue set to 'generate' with a path" - ) - vault_policy = f.get("vaultPolicy") - gen_cmd = f"vault read -field=password sys/policies/password/{vault_policy}/generate" - if b64: - gen_cmd += " | base64 --wrap=0" - for prefix in prefixes: - # if the override field is False and the secret attribute exists at the prefix then we just - # skip, as we do not want to overwrite the existing secret - if not override and self._vault_secret_attr_exists( - mount, prefix, secret_name, f["name"] - ): - continue - cmd = ( - f"oc exec -n {self.namespace} {self.pod} -i -- sh -c " - f"\"{gen_cmd} | vault kv {verb} -mount={mount} {prefix}/{secret_name} {f['name']}=-\"" - ) - self._run_command(cmd, attempts=3) - return - - # If we're not generating the secret inside the vault directly we either read it from the file ("error") - # or we are prompting the user for it - secret = self._get_secret_value(secret_name, f) - if b64: - secret = base64.b64encode(secret.encode()).decode("utf-8") - for prefix in prefixes: - cmd = ( - f"oc exec -n {self.namespace} {self.pod} -i -- sh -c " - f"\"vault kv {verb} -mount={mount} {prefix}/{secret_name} {f['name']}='{secret}'\"" - ) - self._run_command(cmd, attempts=3) - - elif kind == "path": # path. we upload files - # If we're generating the password then we just push the secret in the vault directly - verb = "put" if first else "patch" - path = self._get_file_path(secret_name, f) - for prefix in prefixes: - if b64: - b64_cmd = "| base64 --wrap=0 " - else: - b64_cmd = "" - cmd = ( - f"cat '{path}' | oc exec -n {self.namespace} {self.pod} -i -- sh -c " - f"'cat - {b64_cmd}> /tmp/vcontent'; " - f"oc exec -n {self.namespace} {self.pod} -i -- sh -c '" - f"vault kv {verb} -mount={mount} {prefix}/{secret_name} {f['name']}=@/tmp/vcontent; " - f"rm /tmp/vcontent'" - ) - self._run_command(cmd, attempts=3) - elif kind == "ini_file": # ini_file. we parse an ini_file - verb = "put" if first else "patch" - ini_file = os.path.expanduser(f.get("ini_file")) - ini_section = f.get("ini_section", "default") - ini_key = f.get("ini_key") - secret = get_ini_value(ini_file, ini_section, ini_key) - if b64: - secret = base64.b64encode(secret.encode()).decode("utf-8") - for prefix in prefixes: - cmd = ( - f"oc exec -n {self.namespace} {self.pod} -i -- sh -c " - f"\"vault kv {verb} -mount={mount} {prefix}/{secret_name} {f['name']}='{secret}'\"" - ) - self._run_command(cmd, attempts=3) + vault_policy = f.get("vaultPolicy") + + gen_cmd = f"vault read -field=password sys/policies/password/{vault_policy}/generate" + if b64: + gen_cmd += " | base64 --wrap=0" + + for prefix in prefixes: + # Skip if secret exists and override is False + if not override and self._vault_secret_attr_exists( + mount, prefix, secret_name, f["name"] + ): + continue + + cmd = ( + f"oc exec -n {self.namespace} {self.pod} -i -- sh -c " + f"\"{gen_cmd} | vault kv {verb} -mount={mount} {prefix}/{secret_name} {f['name']}=-\"" + ) + self._run_command(cmd, attempts=3) + + def _inject_provided_secret(self, secret_name, f, mount, prefixes, verb): + """Inject a user-provided secret value""" + secret = self._get_secret_value(secret_name, f) + secret = self._encode_secret_if_needed(secret, f) + + for prefix in prefixes: + cmd = ( + f"oc exec -n {self.namespace} {self.pod} -i -- sh -c " + f"\"vault kv {verb} -mount={mount} {prefix}/{secret_name} {f['name']}='{secret}'\"" + ) + self._run_command(cmd, attempts=3) + + def _inject_path_field(self, secret_name, f, mount, prefixes, verb): + """Inject a file-based field into vault""" + path = self._get_file_path(secret_name, f) + b64_cmd = "| base64 --wrap=0 " if self._get_field_base64(f) else "" + + for prefix in prefixes: + cmd = ( + f"cat '{path}' | oc exec -n {self.namespace} {self.pod} -i -- sh -c " + f"'cat - {b64_cmd}> /tmp/vcontent'; " + f"oc exec -n {self.namespace} {self.pod} -i -- sh -c '" + f"vault kv {verb} -mount={mount} {prefix}/{secret_name} {f['name']}=@/tmp/vcontent; " + f"rm /tmp/vcontent'" + ) + self._run_command(cmd, attempts=3) + + def _inject_ini_field(self, secret_name, f, mount, prefixes, verb): + """Inject an INI file-based field into vault""" + ini_file = os.path.expanduser(f.get("ini_file")) + ini_section = f.get("ini_section", "default") + ini_key = f.get("ini_key") + + secret = get_ini_value(ini_file, ini_section, ini_key) + secret = self._encode_secret_if_needed(secret, f) + + for prefix in prefixes: + cmd = ( + f"oc exec -n {self.namespace} {self.pod} -i -- sh -c " + f"\"vault kv {verb} -mount={mount} {prefix}/{secret_name} {f['name']}='{secret}'\"" + ) + self._run_command(cmd, attempts=3) + + def _encode_secret_if_needed(self, secret, f): + """Apply base64 encoding if required""" + if self._get_field_base64(f): + return base64.b64encode(secret.encode()).decode("utf-8") + return secret # This assumes that self.sanitize_values() has already been called # so we do a lot less validation as it has already happened From a0fe2a552efa0a2fe35dbf3adcafaecd1bfed553 Mon Sep 17 00:00:00 2001 From: Michele Baldessari Date: Wed, 8 Oct 2025 17:26:11 +0200 Subject: [PATCH 09/13] Drop a duplicate test and add a proper test for when onMissingValue is wrong --- plugins/module_utils/load_secrets_v2.py | 1 - tests/unit/test_vault_load_secrets_v2.py | 34 +++++++++---------- ...values-secret-v2-wrong-onmissingvalue.yaml | 14 +------- 3 files changed, 17 insertions(+), 32 deletions(-) diff --git a/plugins/module_utils/load_secrets_v2.py b/plugins/module_utils/load_secrets_v2.py index 85b8850..e753f71 100644 --- a/plugins/module_utils/load_secrets_v2.py +++ b/plugins/module_utils/load_secrets_v2.py @@ -145,7 +145,6 @@ def sanitize_values(self): self.module.fail_json( f"Currently only the 'vault' backingStore is supported: {backing_store}" ) - (ret, msg) = self._validate_secrets() if not ret: self.module.fail_json(msg) diff --git a/tests/unit/test_vault_load_secrets_v2.py b/tests/unit/test_vault_load_secrets_v2.py index 0f8ab89..add55e3 100644 --- a/tests/unit/test_vault_load_secrets_v2.py +++ b/tests/unit/test_vault_load_secrets_v2.py @@ -220,24 +220,6 @@ def test_ensure_policies_are_injected(self, getpass): ] mock_run_command.assert_has_calls(calls) - def test_ensure_error_wrong_onmissing_value(self, getpass): - with self.assertRaises(AnsibleFailJson) as ansible_err: - set_module_args( - { - "values_secrets": os.path.join( - self.testdir_v2, "values-secret-v2-wrong-onmissingvalue.yaml" - ), - } - ) - vault_load_secrets.main() - - ret = ansible_err.exception.args[0] - self.assertEqual(ret["failed"], True) - assert ( - ret["args"][1] - == "Secret has vaultPolicy set to nonExisting but no such policy exists" - ) - def test_ensure_error_wrong_vaultpolicy(self, getpass): with self.assertRaises(AnsibleFailJson) as ansible_err: set_module_args( @@ -724,6 +706,22 @@ def test_ensure_ini_file_works(self, getpass): ] mock_run_command.assert_has_calls(calls) + def test_ensure_error_on_wrong_missing_value(self, getpass): + with self.assertRaises(AnsibleFailJson) as ansible_err: + set_module_args( + { + "values_secrets": os.path.join( + self.testdir_v2, + "values-secret-v2-wrong-onmissingvalue.yaml", + ), + } + ) + vault_load_secrets.main() + + ret = ansible_err.exception.args[0] + self.assertEqual(ret["failed"], True) + assert (ret["args"][1] == "onMissingValue: notexisting is invalid") + if __name__ == "__main__": unittest.main() diff --git a/tests/unit/v2/values-secret-v2-wrong-onmissingvalue.yaml b/tests/unit/v2/values-secret-v2-wrong-onmissingvalue.yaml index 2d53807..3793db5 100644 --- a/tests/unit/v2/values-secret-v2-wrong-onmissingvalue.yaml +++ b/tests/unit/v2/values-secret-v2-wrong-onmissingvalue.yaml @@ -1,20 +1,8 @@ version: "2.0" - backingStore: vault -vaultPolicies: - basicPolicy: | - length=10 - rule "charset" { charset = "abcdefghijklmnopqrstuvwxyz" min-chars = 1 } - rule "charset" { charset = "ABCDEFGHIJKLMNOPQRSTUVWXYZ" min-chars = 1 } - rule "charset" { charset = "0123456789" min-chars = 1 } - secrets: - name: config-demo - vaultPrefixes: - - secret/region-one - - secret/snowflake.blueprints.rhecoeng.com fields: - name: secret - onMissingValue: generate - vaultPolicy: nonExisting + onMissingValue: notexisting \ No newline at end of file From a9b096bc051467e60b5d679b7d2b31c7b91ffc39 Mon Sep 17 00:00:00 2001 From: Michele Baldessari Date: Wed, 8 Oct 2025 17:54:39 +0200 Subject: [PATCH 10/13] Simplify code a bit --- plugins/module_utils/load_secrets_common.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/plugins/module_utils/load_secrets_common.py b/plugins/module_utils/load_secrets_common.py index 07f09f9..f8ebe5d 100644 --- a/plugins/module_utils/load_secrets_common.py +++ b/plugins/module_utils/load_secrets_common.py @@ -227,10 +227,6 @@ def _validate_field(self, f): if "name" not in f: return (False, f"Field {f} is missing name") - on_missing_value = self._get_field_on_missing_value(f) - if on_missing_value not in ["error", "generate", "prompt"]: - return (False, f"onMissingValue: {on_missing_value} is invalid") - # Validate field structure and types result = self._validate_field_structure(f) if not result[0]: @@ -241,6 +237,7 @@ def _validate_field(self, f): if not result[0]: return result + on_missing_value = self._get_field_on_missing_value(f) # Validate based on onMissingValue type match on_missing_value: case "error": @@ -250,7 +247,7 @@ def _validate_field(self, f): case "prompt": return self._validate_prompt_mode(f) - return (True, "") + return (False, f"onMissingValue: {on_missing_value} is invalid") def _validate_field_structure(self, f): """Validate field structure and basic types""" From d8934ca6aa78f1849a7b76111c806c950f0122a4 Mon Sep 17 00:00:00 2001 From: Michele Baldessari Date: Wed, 8 Oct 2025 17:59:12 +0200 Subject: [PATCH 11/13] Refactor _validate_secret() a bit --- plugins/module_utils/load_secrets_v2.py | 88 +++++++++++++++++-------- 1 file changed, 59 insertions(+), 29 deletions(-) diff --git a/plugins/module_utils/load_secrets_v2.py b/plugins/module_utils/load_secrets_v2.py index e753f71..34b94c8 100644 --- a/plugins/module_utils/load_secrets_v2.py +++ b/plugins/module_utils/load_secrets_v2.py @@ -83,38 +83,68 @@ def _validate_secrets(self): if len(secrets) == 0: self.module.fail_json("No secrets found") - names = [] - for s in secrets: - # These fields are mandatory - for i in ["name"]: - try: - unused = s[i] - except KeyError: - return (False, f"Secret {s['name']} is missing {i}") - names.append(s["name"]) + # Validate each secret and collect names for duplicate checking + secret_names = [] + for secret in secrets: + result = self._validate_secret(secret) + if not result[0]: + return result + secret_names.append(secret["name"]) + + # Check for duplicate secret names + dupes = find_dupes(secret_names) + if len(dupes) > 0: + return (False, f"You cannot have duplicate secret names: {dupes}") - vault_prefixes = s.get("vaultPrefixes", ["hub"]) - # This checks for the case when vaultPrefixes: is specified but empty - if vault_prefixes is None or len(vault_prefixes) == 0: - return (False, f"Secret {s['name']} has empty vaultPrefixes") + return (True, "") - fields = s.get("fields", []) - if len(fields) == 0: - return (False, f"Secret {s['name']} does not have any fields") + def _validate_secret(self, secret): + """Validate a single secret configuration""" + # Check mandatory fields + if "name" not in secret: + return (False, f"Secret {secret} is missing name") + + secret_name = secret["name"] + + # Validate vault prefixes + result = self._validate_vault_prefixes(secret) + if not result[0]: + return result + + # Validate fields + result = self._validate_secret_fields(secret) + if not result[0]: + return result + + return (True, "") + + def _validate_vault_prefixes(self, secret): + """Validate vault prefixes for a secret""" + vault_prefixes = secret.get("vaultPrefixes", ["hub"]) + # This checks for the case when vaultPrefixes: is specified but empty + if vault_prefixes is None or len(vault_prefixes) == 0: + return (False, f"Secret {secret['name']} has empty vaultPrefixes") + return (True, "") + + def _validate_secret_fields(self, secret): + """Validate all fields for a secret""" + fields = secret.get("fields", []) + if len(fields) == 0: + return (False, f"Secret {secret['name']} does not have any fields") + + # Validate each field and collect names for duplicate checking + field_names = [] + for field in fields: + result = self._validate_field(field) + if not result[0]: + return result + field_names.append(field["name"]) + + # Check for duplicate field names + field_dupes = find_dupes(field_names) + if len(field_dupes) > 0: + return (False, f"You cannot have duplicate field names: {field_dupes}") - field_names = [] - for i in fields: - (ret, msg) = self._validate_field(i) - if not ret: - return (False, msg) - field_names.append(i["name"]) - field_dupes = find_dupes(field_names) - if len(field_dupes) > 0: - return (False, f"You cannot have duplicate field names: {field_dupes}") - - dupes = find_dupes(names) - if len(dupes) > 0: - return (False, f"You cannot have duplicate secret names: {dupes}") return (True, "") def inject_vault_policies(self): From 2fdcd045b857285f49176ef85238f53d87435818 Mon Sep 17 00:00:00 2001 From: Michele Baldessari Date: Tue, 27 Jan 2026 10:42:42 +0100 Subject: [PATCH 12/13] Retrict python versions and clean up whitespace We can now use >= python 3.11 --- Makefile | 4 ++-- tests/unit/test_vault_load_secrets_v2.py | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/Makefile b/Makefile index 5b2e254..f62ebfb 100644 --- a/Makefile +++ b/Makefile @@ -35,12 +35,12 @@ ansible-lint: ## run ansible lint on ansible/ folder .PHONY: ansible-sanitytest ansible-sanitytest: ## run ansible unit tests - ansible-test sanity --docker default + ansible-test sanity --docker default --python 3.11 --python 3.12 .PHONY: ansible-unittest ansible-unittest: ## run ansible unit tests rm -rf tests/output - ansible-test units --docker + ansible-test units --docker --python 3.11 --python 3.12 .PHONY: test test: ansible-sanitytest ansible-unittest diff --git a/tests/unit/test_vault_load_secrets_v2.py b/tests/unit/test_vault_load_secrets_v2.py index add55e3..0cc3f40 100644 --- a/tests/unit/test_vault_load_secrets_v2.py +++ b/tests/unit/test_vault_load_secrets_v2.py @@ -63,7 +63,6 @@ def fail_json(*args, **kwargs): @mock.patch("getpass.getpass") class TestMyModule(unittest.TestCase): - def setUp(self): self.mock_module_helper = patch.multiple( basic.AnsibleModule, exit_json=exit_json, fail_json=fail_json @@ -720,7 +719,7 @@ def test_ensure_error_on_wrong_missing_value(self, getpass): ret = ansible_err.exception.args[0] self.assertEqual(ret["failed"], True) - assert (ret["args"][1] == "onMissingValue: notexisting is invalid") + assert ret["args"][1] == "onMissingValue: notexisting is invalid" if __name__ == "__main__": From 19c67b99148eb60bd8ff38863737a1f1225212b4 Mon Sep 17 00:00:00 2001 From: Michele Baldessari Date: Tue, 27 Jan 2026 10:47:06 +0100 Subject: [PATCH 13/13] A couple of more linting warnings --- plugins/module_utils/load_secrets_v2.py | 4 +++- roles/vault_utils/tasks/vault_spokes_init.yaml | 2 +- tests/unit/v2/values-secret-v2-wrong-onmissingvalue.yaml | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/plugins/module_utils/load_secrets_v2.py b/plugins/module_utils/load_secrets_v2.py index 34b94c8..90f1c19 100644 --- a/plugins/module_utils/load_secrets_v2.py +++ b/plugins/module_utils/load_secrets_v2.py @@ -224,7 +224,9 @@ def _inject_generated_secret(self, secret_name, f, mount, prefixes, verb): b64 = self._get_field_base64(f) vault_policy = f.get("vaultPolicy") - gen_cmd = f"vault read -field=password sys/policies/password/{vault_policy}/generate" + gen_cmd = ( + f"vault read -field=password sys/policies/password/{vault_policy}/generate" + ) if b64: gen_cmd += " | base64 --wrap=0" diff --git a/roles/vault_utils/tasks/vault_spokes_init.yaml b/roles/vault_utils/tasks/vault_spokes_init.yaml index e362b2f..40651dd 100644 --- a/roles/vault_utils/tasks/vault_spokes_init.yaml +++ b/roles/vault_utils/tasks/vault_spokes_init.yaml @@ -37,7 +37,7 @@ ansible.builtin.set_fact: clusters: "{{ clusters | default({}) | combine({item.metadata.name: {'server_api': item.spec.managedClusterClientConfigs[0].url, - 'cluster_fqdn':_cluster_fqdn }}, recursive=True) }}" + 'cluster_fqdn': _cluster_fqdn }}, recursive=True) }}" loop: "{{ resources }}" vars: _cluster_fqdn: "{{ item.status.clusterClaims | selectattr('name', 'equalto', 'consoleurl.cluster.open-cluster-management.io') diff --git a/tests/unit/v2/values-secret-v2-wrong-onmissingvalue.yaml b/tests/unit/v2/values-secret-v2-wrong-onmissingvalue.yaml index 3793db5..35364ae 100644 --- a/tests/unit/v2/values-secret-v2-wrong-onmissingvalue.yaml +++ b/tests/unit/v2/values-secret-v2-wrong-onmissingvalue.yaml @@ -5,4 +5,4 @@ secrets: - name: config-demo fields: - name: secret - onMissingValue: notexisting \ No newline at end of file + onMissingValue: notexisting