Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite vault core, issue AppRoles to minions #62684

Merged
merged 104 commits into from
Dec 16, 2023

Conversation

lkubb
Copy link
Contributor

@lkubb lkubb commented Sep 14, 2022

What does this PR do?

  • Fundamentally rewrites the Vault integration core and provides higher-level abstractions to interact with Vault.
  • Issues AppRoles to minions and manages their metadata, allowing much simpler ACL policies.
  • Makes use of response wrapping to distribute secrets.
  • Allows to configure token lifecycle, including renewals and revocations.
  • Uses Salt cache classes instead of raw file operations.
  • Changes the configuration format, the old format is translated automatically.
  • Fixes some bugs with the current implementation (see below).
  • Adds some low-effort miscellaneous new behaviors like being able to configure audit log metadata.
  • Properly splits policy management into execution and state module parts.
  • Corrects some docs, adds configuration examples and required policies.
  • Removes __utils__ use.
  • Migrates tests to pytest.
  • Adds (a lot of) new (unit/integration) tests (accounts for ~75% of additions!).
  • Tries to achieve all this while staying backwards-compatible: Keeps the previous runner endpoint as well as public utils module functions. Translates old vault configuration. Compatible with legacy peer_run config, but it should be updated to avoid unnecessary roundtrips/reduce network overhead.

Background

While working on #62674, I noticed the better approach would be to issue AppRoles to minions. Implementing that, I was a bit frustrated with the abstraction level and found myself in a yak shaving situation.
The missing abstraction causes issues such as #62651.

Builds on #62674.
See also: https://discuss.hashicorp.com/t/saltstack-vault-and-host-role-policies/19214

What issues does this PR fix or reference?

Fixes #62380
Fixes #58174
Fixes #62823
Fixes #62825
Fixes #60779
Fixes #57561
Fixes #62828
Fixes #58580
Fixes #43287
Fixes #51986
Fixes #63406
Fixes #63440
Fixes #64096
Fixes #64128
Fixes #64379

Included:
#62552
#59827

Previous Behavior

  • Adding new Vault behavior is cumbersome.
  • The master fetches tokens in plain text and sends them to minions, which are unknown to Vault (see Anti-Patterns).
  • To securely assign ACL policies to issued tokens, you need to create a separate policy for each minion. This would be has been relieved a bit by the mentioned pillar templating PR, so that a separate policy is only necessary for each defined role.
  • Tokens are always issued with the same minimum and maximum ttl and never renewed or revoked.
  • Lease management is left to the user/author of custom modules.

New Behavior

  • Adding new Vault behavior has a much lower entry barrier and potential for mistakes.
  • Secrets are distributed via response wrapping tokens, ensuring integrity and secrecy. The master can be configured to issue AppRoles and entities to minions. Vault knows about them and associated pillars.
  • In theory, a single policy for all salt minions is sufficient for most behaviors:
path "salt/data/minions/{{identity.entity.metadata.minion-id}}" {
    capabilities = ["read"]  # change capabilities as necessary
}

path "salt/data/roles/{{identity.entity.metadata.role}}" {
    capabilities = ["read"]
}

# ... match other custom metadata that is managed by Salt
  • Tokens (and AppRole-issued tokens) can be configured with most parameters supported by Vault. Tokens are renewed when necessary and revoked when not in use anymore.
  • Leases can be stored and retrieved easily by modules and are renewed automatically (when requested).

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes

@lkubb lkubb changed the title Approle minions vault Rewrite vault core, issue AppRoles to minions Sep 14, 2022
@lukasraska
Copy link
Contributor

From the looks of it this is breaking change right? Meaning that people that currently use Vault integration will need to adjust at least their configuration (or worse) to get it working again.

If that's the case, I wonder whether it wouldn't make sense deprecating the current modules and provide this as "v2" or similar.

@lkubb
Copy link
Contributor Author

lkubb commented Sep 18, 2022

The intention is for this to be backwards-compatible. The current configuration scheme is translated and in case the master is upgraded before minions, the current runner endpoint is still available, same as with the current public utility functions in case custom modules are using those. They are marked as deprecated though.

@lkubb lkubb force-pushed the approle-minions-vault branch 4 times, most recently from 54a3a1f to 7c8bb50 Compare September 22, 2022 20:51
@lkubb lkubb force-pushed the approle-minions-vault branch 6 times, most recently from b4f7c5c to 407c82e Compare October 1, 2022 08:26
This commit represents a fundamental rewrite in how Salt interacts with
Vault. The master should still be compatible with minions running the
old code. There should be no breaking changes to public interfaces and
the old configuration format should still apply.

Core:
- Issue AppRoles to minions
- Manage entities with templatable metadata for minions
- Use inbuilt Salt cache
- Separate config cache from token cache
- Cache: introduce connection-scope vs global scope

Utility module:
- Support being imported (__utils__ deprecation)
- Raise exceptions on queries to simplify response handling
- Add classes to wrap complexity, especially regarding KV v2
- Lay some groundwork for renewing tokens

Execution module:
- Add patch_secret
- Add version support to delete_secret
- Allow returning listed keys only in list_secret
- Add policy_[fetch/write/delete] and policies_list
- Add query for arbitrary API queries

State module:
- Make use of execution module
- Change output format

Docs:
- Update for new configuration format
- Correct examples
- Add configuration examples
- Add required policies
In some cases, the `spec` calls were failing because the underlying
object was already patched
@lkubb
Copy link
Contributor Author

lkubb commented Dec 13, 2023

@dwoz I resolved the conflicts and fixed the tests, remaining failures are flaky ones imo. Can you restart the failed runs only? Not sure why it's not done automatically, this was the first run.

@lkubb
Copy link
Contributor Author

lkubb commented Dec 13, 2023

@dwoz Today's test run is all green (and merge conflicts are resolved).

I have been using this code heavily since its inception and think it has at least a few less rough edges than the current one. Either way, the saltext will be published soon as well, so issues can be dealt with there. Sorry for pinging twice, wanted to give this a chance of landing in core before the final saltext migration.

@dwoz dwoz merged commit f2121e5 into saltstack:master Dec 16, 2023
531 checks passed
@lkubb
Copy link
Contributor Author

lkubb commented Apr 23, 2024

Update for anyone that followed this: The first release of saltext.vault has just been published to PyPi: https://pypi.org/project/saltext.vault/

You can migrate to the extension at your discretion, it's nearly identical* to what's found in 3007.0. The vault modules (together with many others) have been removed from the master branch, so from 3008.0 onward, the only option to use the Vault integration will be the extension.

* with some minor deprecations, a bugfix that's still pending for 3007 and a small QoL improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment