feat(vault): add fine-grained app policies and preserve hub-role #87
Conversation
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 <minzhang@redhat.com>
mlorenzofr
left a comment
mlorenzofr
left a comment
There was a problem hiding this comment.
Good work, but I would implement some additional changes before merging it
| register: _hub_role_result | ||
| failed_when: false | ||
|
|
||
| # Define the default policies that VP framework requires |
There was a problem hiding this comment.
I'd move this to the role's defaults file (defaults/main.yml). The fact name reflects it's a default value and it makes more sense if this values, which may be modified in future, to reside in the defaults file rather than a task file.
There was a problem hiding this comment.
moved the default policies to defaults/main.yml
| # - hub/infra/keycloak | ||
| # app_update_hub_role: true | ||
|
|
||
| - name: Debug - Show app prefixes to process |
There was a problem hiding this comment.
I understand that these debug messages are useful during the development phase, do we need them during the execution phase?
There was a problem hiding this comment.
Good point! Added verbosity: 1 to the debug tasks so they only appear when running with -v flag. This keeps normal execution outoput clean while still providing troubleshooting info when needed.
| @@ -0,0 +1,152 @@ | |||
| --- | |||
There was a problem hiding this comment.
When are these tasks executed? I haven't been able to find the include
There was a problem hiding this comment.
My bad. I only included this file in my local testing ansible tasks which is outside of this repo. Now I have this file integrated into the push_parsed_secrets.yaml flow. The task file is automatically included when processing secrets with custom vaultPrefix values.
| # 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']) }}" |
There was a problem hiding this comment.
Instead of hardcoding the default values manually on this line, it's better to put them in a variable in defaults/main.yaml and retrieve them from there.
There was a problem hiding this comment.
Good catch! Moved the default policies to defaults/main.yml as vault_hub_role_default_policies and updated the task to reference it.
|
|
||
| - 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'] }}" |
There was a problem hiding this comment.
Let's move this to a variable in defaults/main.yml
There was a problem hiding this comment.
Done: both instances now reference vault_hub_role_default_policies from defautls/main.yml
| # Each application gets its own policy that only allows access to its | ||
| # specific path, enabling application-level secret isolation. | ||
| # | ||
| # Required variables: |
There was a problem hiding this comment.
Let's add these variables and their descriptions to defaults/main.yml
There was a problem hiding this comment.
Added all app policy variables to defaults/main.yml.
| label: "Creating JWT role for {{ _app_name }}" | ||
| vars: | ||
| _app_name: "{{ item | basename }}" | ||
| _role_config: "{{ app_jwt_role_config[_app_name] | default({}) }}" |
There was a problem hiding this comment.
Instead of using an alternative dictionary with role configuration values and a key that matches the app name, wouldn't it be better to have app_prefixes be a list with dictionaries and their related properties in the same object?
There was a problem hiding this comment.
Refactored app_prefixes to use a unified dict structure with prefix and optional jwt_role in the same object.
| - app_prefixes is defined and app_prefixes | length > 0 | ||
| - app_update_hub_role | default(true) | bool | ||
|
|
||
| - name: Display updated hub-role policies |
There was a problem hiding this comment.
Debug messages are useful during development, but we should remove it in execution
There was a problem hiding this comment.
Debug messages are now gated with verbosity: 1.
| - app_prefixes is defined and app_prefixes | length > 0 | ||
| - app_update_hub_role | default(true) | bool | ||
|
|
||
| # Update hub-role with merged policies |
There was a problem hiding this comment.
We execute this unconditionally, whether the policies are already configured or not?
There was a problem hiding this comment.
Added idempotency check. The hub-role is now only updated whent he merged policies differ from the current policies. Sorted comparison is used to handle order differences.
| command: > | ||
| vault read -format=json auth/{{ vault_hub }}/role/{{ vault_hub }}-role | ||
| register: _hub_role_result | ||
| failed_when: false |
There was a problem hiding this comment.
It's a command that doesn't change anything (it only reads), we might want to add changed_when: false to it.
There was a problem hiding this comment.
added changed_when: false in this task.
minmzzhang
force-pushed
the
app-vault-policies
branch
3 times, most recently
from
e6457b2 to
0dcac48
Compare
mlorenzofr
left a comment
mlorenzofr
left a comment
There was a problem hiding this comment.
Just a small modification to prevent a read task from showing like a change
| command: > | ||
| vault read -format=json auth/{{ vault_hub }}/role/{{ vault_hub }}-role | ||
| register: _hub_role_result | ||
| failed_when: false |
There was a problem hiding this comment.
| failed_when: false | |
| failed_when: false | |
| changed_when: false |
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 <minzhang@redhat.com>
minmzzhang
force-pushed
the
app-vault-policies
branch
from
0dcac48 to
f97ac86
Compare
2 participants
This PR adds support for application-level Vault policy segmentation:
vault_secrets_init.yaml: Preserve existing hub-role policies
vault_app_policies.yaml: Create K8s auth policies from vaultPrefixes
vault_jwt.yaml: Create JWT policies for SPIFFE workloads
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.