Add vault_pki modules

[voyvodov]

What does this PR do?

Add execution and state modules for managing and using Vault's PKI secret store.

Currently the following states are added:

• certificate_managed - Used to manage and renew certificates issued by Vault. Pretty much replicate the x509.certificate_managed module.
• role_managed - Used to manage PKI roles in Vault and keep them according to the state.
• role_absent - Used to ensure PKI role is absent from Vault.

In terms of execution module, there are several modules providing possibility to manage issuers, roles, certificates, keys.

Some updates in utils which will provide helpers for PKI modules functionality.

What issues does this PR fix or reference?

Fixes: #56

Previous Behavior

Remove this section if not relevant

New Behavior

Remove this section if not relevant

Merge requirements satisfied?

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

• Docs
• Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
• Tests written/updated

Commits signed with GPG?

Yes/No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

[lkubb]

This looks nice so far. Some things I noticed while skimming (just trying to help + persist it for a future review, you're probably aware of many of them since this is a draft):

General:

• issue: We will need tests, preferably functional ones + some unit tests for more specific stuff. You can take inspiration from https://github.com/saltstack/salt/blob/master/tests/pytests/functional/states/test_x509_v2.py and e.g. https://github.com/lkubb/saltext-vault/blob/vault-db/tests/functional/modules/test_vault_db.py / https://github.com/lkubb/saltext-vault/blob/vault-db/tests/functional/states/test_vault_db.py.
• todo: Importing salt.utils.x509 requires Salt >=3006 (that's fine, we just have to remember to raise the requirement and account for it in the central-artifacts repo)
• thought: Maybe we should just require x509_v2 to be enabled instead of duplicating encode_certificate (the old modules are deprecated and x509_v2 will be the default in 3008)
• nitpick: I would prefer if the whole cert comparison was done in the utils module (we could make a Salt-SSH wrapper more easily then)

More specific:

• question: Is there a need for a separate timestring_to_seconds function or can we just use int(timestring_map(x)) (since it's probably only relevant when POSTing is-not-None values)? If it's more ergonomic to have it, we should do the mentioned conversion in timestring_to_seconds (or introduce a parameter like timestring_map(value, typ=float)), not duplicate timestring_map.
• issue: vault_pki.certificate_managed will not work for binary formats (they are not supported by file.managed and need to be written separately from managing the file's metadata, hence replace is set to False)
• thought: Not sure if vault_pki.certificate_managed will misbehave or not when follow_symlinks=False is passed and name is a symlink
• nitpick: vault_pki.write_role can misbehave (return None instead of raising an exception) if both requests return HTTP 405 for some reason
• issue: vault_pki.role_managed needs to catch exceptions to avoid a hard crash during state application
• issue: _file_managed should use __salt__["state.single"] (the workaround for sticky test=true is not required anymore and discouraged by the core team)
• issue: Certificate timestamps should try to use the _utc variants, e.g. cert.not_valid_after_utc and fallback to cert.not_valid_after.replace(tzinfo=timezone.utc) on AttributeErrors to avoid deprecation warnings
• chore: The docs often mention DB backend, where it should be PKI backend
• nitpick: Passing is_unauthd=False to vault.query is unnecessary (when setting it to true, it must be accurate though)
• nitpick: pre-commit complaints :) (Edit: Just saw you fixed most of them while I wrote this, the remaining ones can be fixed if you rebase on current main and run pre-commit locally again)

Thanks a lot for working on this! Sorry for the mess, I didn't want to focus too much on specific lines in the current state.

[voyvodov]

Thanks for the comment!

Really appreciated.

To the points:

• tests are written, will add them. Wanted first to push the initial structure of what the modules will look like.

• X509 - still thinking how to proceed here. Probably require x509_v2 to avoid duplications and so on.

• will move the comparison in utils. Still doing cleanup there and optimizing things.

• I needed timestamp_to_seconds to ensure I get int. Probably will cleanup and just convert what I get from Vault to float to make correct comparison. Still not sure why we need float at all, as everything in Vault is seconds based.

• I know that it DER format is not supported (yet). Will add it before opening for review.

• It should work correctly with symlinks too (at least with some manual tests)

• will fix write_role. It was a temp workaround for one of the func tests

• For the rest - will fix, still not fully reworked from our custom modules.

Once again, thank you for spending time and efforts looking at this!

[lkubb]

I needed timestamp_to_seconds to ensure I get int. Probably will cleanup and just convert what I get from Vault to float to make correct comparison. Still not sure why we need float at all, as everything in Vault is seconds based.

It's a float because in theory, Vault handles values such as 1.5s: https://developer.hashicorp.com/vault/docs/concepts/duration-format. Not sure in which contexts it's allowed and how that's reported, but decided to account for it at the time.

It might be sufficient to just add a cast=float default parameter to timestring_map, replace all float with cast and override it with cast=int when needed.

Once again, thank you for spending time and efforts looking at this!

Likewise!

[lkubb]

salt-extensions/central-artifacts#4 should help with (some of) the failures by

a) dropping 3005 from the tests and
b) testing against 3006.8 (it seems 3006.4 did not have cryptography in the base requirements for Linux only - not certain that's the reason for it being absent though)

[voyvodov]

salt-extensions/central-artifacts#4 should help with (some of) the failures by

a) dropping 3005 from the tests and b) testing against 3006.8 (it seems 3006.4 did not have cryptography in the base requirements for Linux only - not certain that's the reason for it being absent though)

Thanks for that!

Meanwhile I'm adding more tests and fixing few minor issues here and there.

[voyvodov]

So... Tests are added and passing (locally).
Once salt-extensions/central-artifacts#4 is merged they should pass here too.

Opening the PR ready for review.

[voyvodov]

Will add more functionality once this is approved and merged (just to be sure logic and code style are acceptable).

[voyvodov]

Probably I should also update pyproject.toml from

dependencies = [
    "salt>=3005",
]

to

dependencies = [
    "salt>=3006",
    "cryptography>=41.0.3"
]

but we can do it a little bit later.

[lkubb]

This looks pretty good. Thanks a lot for taking the time to write the tests!

I'll play with the modules a bit until you had a chance to consider the suggestions and we get the central-artifacts PR merged.

At some point in the future, I might try to improve the docs a bit further (you can review the PR then), but they are mostly fine for now.

Note: We should point out that minions need to be able to read their role configuration for certificate_managed. It could be a possible future addition to not hard-require this and rely on expiration only in case it's not accessible.

[lkubb]

Probably I should also update pyproject.toml from

Yup, I'll do that in a follow-up PR since that touches the copier template variables. What's the reason for cryptography>=41.0.3 specifically? I suspect in the code's current state it would be >=42, but I'd prefer it to be lower (as mentioned in the review).

[voyvodov]

Probably I should also update pyproject.toml from

Yup, I'll do that in a follow-up PR since that touches the copier template variables. What's the reason for cryptography>=41.0.3 specifically? I suspect in the code's current state it would be >=42, but I'd prefer it to be lower (as mentioned in the review).

Not big one. Is the one installed with Salt 3006.3 (any versions before that have major issues with x509 module).

But >=42 is more than fine!

[voyvodov]

@lkubb, first I want to thank you for the quick and detailed review!

I think I fixed/changed everything mentioned.

[lkubb]

Thanks for taking care of the suggestions so quickly! Just some additional nits and the few outstanding ones from the last review.

Edit: Also we'll need a changelog file, maybe something like:

echo "Added vault_pki modules for interfacing with the PKI backend and managing X.509 certificates" > changelog/58.added.md

[voyvodov]

All should be fixed. Added the changelog.

Also, decide to rename issuer param here and there to issuer_ref as more correct one and for consistency.

[lkubb]

Thanks for your patience in bearing with me and sorry for the iterative approach [big PR :)]. We're nearly there!

Those two were missed by GitHub in the last review for some reason, but at least the potential crash should be addressed:

#57 (comment)
#57 (comment)

[voyvodov]

issuer = salt["vault_pki.read_role"](role_name, mount=mount)["issuer_ref"]

Yeah. I was expecting issues as it's really big one and I still didn't have the enough experience with Salt's codebase.

I also want to thank you for the patience and all the checks! I think I fixed those two and the rest mentioned.

[lkubb]

We're all learning on some level, the Salt code base is a behemoth. 😅

Perfect, I think that addresses everything I noticed code-wise so far. Will wait with approval/merge until CI is passing. If anything comes up in the meantime, I might leave another review.

Thanks a lot for your significant contribution, much appreciated! And feel free to expand on the modules whenever you like.

TODO for myself:

• increase Salt requirement to >=3006
• add cryptography to requirements (version?)
• document the requirement for read access to <mount>/roles/<role_name> in certificate_managed

[voyvodov]

Thanks once again!

Will do, probably in another PR to avoid messing with this one :).

[lkubb]

@voyvodov Sorry for the massive delay, I really tried to push though... Happy to report the necessary change is finally in.

Are you still interested in finishing up this PR?

If so, I'd suggest to:

• get this branch up to date (+ squash the fixup commits, otherwise I can squash-merge the PR into a single commit)
• add .. versionadded:: 1.1.0 to the new modules
• add "cryptography>=36", to

  saltext-vault/pyproject.toml

  Lines 39 to 41 in f6f35f7

  	dependencies = [
  	"salt>=3006",
  	]

• add a note that X509 certificates can be managed as well to

  saltext-vault/docs/index.rst

  Lines 8 to 14 in 9c327dd

  	Currently, you can
  	------------------
  	* manage and dynamically retrieve secrets from the KV v1 and v2 secret backends
  	* manage Vault policies
  	* manage the Database secret engine
  	* request, renew and monitor short-lived database credentials
  	* write your own modules on top of the provided utilities

[voyvodov]

Sure.

Changes are done, fixup commits are squashed and branch is re-based.

Also did some final small fixes (for python3.8 which requires typing classes).

Should be okey for merging.

[lkubb]

Thanks again for persevering and creating this valuable contribution! :)

I'm going to merge this now and create a new release. Once again, feel free to keep improving this extension, very much appreciated!