Add support for Vault’s PKI secret engine

[kdubb]

• Supports all endpoints of PKI secret engine, except prvileged endpoints.
• Has complete test coverage for all endpoints and options.

[kdubb]

@vsevel Tests need to be added. I am having trouble getting a simple test to work.

I added VaultPKIITCase but attempting to inject the VaultPKISecretEngine into the test class causes an unsatisfied dependency deployment exception, even though VaultPKIManager is an application scoped bean that implements VaultPKISecretEngine. Any thoughts on solving this? I'm assuming I'm missing some required test configuration.

Sovled: Beans must be added to VaultProcessor.registerAdditionalBeans.

[quarkus-bot]

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 91659ee

Status	Name	Step	Test failures	Logs	Raw logs
✖	JVM Tests - JDK 11	Build	Test failures	Logs	Raw logs
✖	JVM Tests - JDK 16	Build	Test failures	Logs	Raw logs
✖	Native Tests - Security3	Build	Test failures	Logs	Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ JVM Tests - JDK 11 #

📦 integration-tests/vault

✖ io.quarkus.vault.VaultPKIITCase. - More details - Source on GitHub

---

⚙️ JVM Tests - JDK 16 #

📦 integration-tests/vault

✖ io.quarkus.vault.VaultPKIITCase. - More details - Source on GitHub

---

⚙️ Native Tests - Security3 #

📦 integration-tests/vault

✖ io.quarkus.vault.VaultPKIITCase. - More details - Source on GitHub

[kdubb]

@vsevel @sberyozkin This is complete support for Vault's PKI secret engine with complete test coverage. Can I get this reviewed and your thoughts on it?

One point to note is that supporting "dynamic" engine's was a requirement, as opposed to listing them in the configuration. To facilitate this we added a default VaultPKISecretEngine bean (via VaultPKIManager) that uses the default mount of pki. We also added a VaultPKISecretEngineFactory (via VaultPKIManagerFactory) that allows getting a VaultPKISecretEngine for any mounted engine point.

[sberyozkin]

This is a quality contribution IMHO - I don't understand all the details but it looks like it naturally fits into the existing Quarkus Vault ecosystem of services.
I'm curious how does it align with Lets Encrypt, I've found https://developer.epages.com/blog/tech-stories/managing-lets-encrypt-certificates-in-vault/.
IMHO the doc introducing this engine integration and explaining when to use it is needed for this PR.
Also CC @stuartwdouglas

[vsevel]

I agree with @sberyozkin. I will take a look in the next few days.

[vsevel]

Hello @kdubb
This looks very good already.

2 main points:

• I am wondering if there is a better way to implement VaultPKIManager, which is used sometimes as a CDI bean (when injected directly), and sometimes as a simple java object in VaultPKIManagerFactory.engine(String). what do you think @sberyozkin
• I agree with @sberyozkin there should be a dedicated guide either following https://www.vaultproject.io/docs/secrets/pki, https://learn.hashicorp.com/tutorials/vault/pki-engine or https://blog.revolve.team/2019/09/12/vault-pki/ (in french)

[kdubb]

@vsevel Updated with undisputed changes.

[vsevel]

hello @kdubb
I am not going to be able to follow this for 2 weeks.

• as far as the defaults are concerned (i.e. pem vs der), what I meant was that documentation in either VaultPKISecretEngine or one of the public classes/interfaces should be clear about the choice we are making in terms of default, in particular if the default is quarkus is different than the one in vault's api.
• with respect to checkEngineEnabled, I still wanted to do some tests to see if there was something we could do better. I have not had time so far.
• as discussed earlier, I think this feature deserves a dedicated guide.

if making progress on this PR is important to you, and if @sberyozkin has any spare time, may be you two can work together while I am away.

[kdubb]

as far as the defaults are concerned (i.e. pem vs der)

As I said, I've added the format to all the methods that use an API that are format selectable. I will also add a blurb about it, along with the reasoning, to the VaultPKISecretEngine interface.

as discussed earlier, I think this feature deserves a dedicated guide.

I am working on a guide for the documentation and am gonna try and add it in the next few days.

[vsevel]

hi @kdubb
on the checkEngineEnabled question, why do you not use the Read Mount Configuration?

if the pki engine is not enabled we get a 400:

$curl -v -H "X-Vault-Token: s.IwKJKdNvhgwl5CSBk1TwNqvZ" -X GET http://127.0.0.1:8200/v1/sys/mounts/pki/tune
< HTTP/1.1 400 Bad Request

and a 200 if i is enabled:

$ vault secrets enable pki
Success! Enabled the pki secrets engine at: pki/
$ curl -v -H "X-Vault-Token: s.IwKJKdNvhgwl5CSBk1TwNqvZ" -X GET http://127.0.0.1:8200/v1/sys/mounts/pki/tune
Note: Unnecessary use of -X or --request, GET is already inferred.
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to 127.0.0.1 (127.0.0.1) port 8200 (#0)
> GET /v1/sys/mounts/pki/tune HTTP/1.1
> Host: 127.0.0.1:8200
> User-Agent: curl/7.64.1
> Accept: */*
> X-Vault-Token: s.IwKJKdNvhgwl5CSBk1TwNqvZ
> 
< HTTP/1.1 200 OK
< Cache-Control: no-store
< Content-Type: application/json
< Date: Wed, 18 Aug 2021 17:22:43 GMT
< Content-Length: 343
< 
{"force_no_cache":false,"description":"","default_lease_ttl":2764800,"max_lease_ttl":2764800,"request_id":"5d82dbaf-ea45-b251-541b-8b5f1b9ac0bb","lease_id":"","renewable":false,"lease_duration":0,"data":{"default_lease_ttl":2764800,"description":"","force_no_cache":false,"max_lease_ttl":2764800},"wrap_info":null,"warnings":null,"auth":null}
* Connection #0 to host 127.0.0.1 left intact
* Closing connection 0

is that because you think that the client would not have the right to access this endpoint?

I did some tests with the endpoint you are using pki/config/urls. what attracted my attention initially is that it is a POST, which looks award when doing a check.
that said, it works for your use case only because of what is to me an implementation detail: if you pass an empty structure, it will not override values that may have been provisioned before.

In other words, if I enable the pki engine, and provision 1 url:

$ curl -v -H "X-Vault-Token: s.IwKJKdNvhgwl5CSBk1TwNqvZ" --data '{"ocsp_servers":["https://hello"]}' -X POST http://127.0.0.1:8200/v1/pki/config/urls
< HTTP/1.1 204 No Content

then I execute the same call with no data I get a 204:

$ curl -v -H "X-Vault-Token: s.IwKJKdNvhgwl5CSBk1TwNqvZ" --data '{}' -X POST http://127.0.0.1:8200/v1/pki/config/urls
< HTTP/1.1 204 No Content

and if I query again the urls, the ocsp_servers has remained:

curl -v -H "X-Vault-Token: s.IwKJKdNvhgwl5CSBk1TwNqvZ" -X GET http://127.0.0.1:8200/v1/pki/config/urls
< HTTP/1.1 200 OK
{"request_id":"7cf51447-43f7-98c8-b1a5-877e09a3a78a","lease_id":"","renewable":false,"lease_duration":0,"data":{"crl_distribution_points":[],"issuing_certificates":[],"ocsp_servers":["https://hello"]},"wrap_info":null,"warnings":null,"auth":null}

we are kind of lucky that {} was not interpreted as {"ocsp_servers": null, ...}

it works but that is very fragile. so if the tune operation works, I think it is a much better choice.

[kdubb]

@vsevel The problem is only that none of Vault's methods (including tune) tell you which engine is mounted at a specific location. In other words, if we implemented it using tune the checkEngineEnabled would return true for the kv mount point.

Using the pki/config/urls endpoint is a workaround. It will return 404 for any engine except the pki engine. So if we get a 204 we know specifically that a pki engine is mounted at the queried path.

I guess there's an argument to be made that we only need to see if "something" is mounted and then the querying app can assume it's pki.

[vsevel]

understood. but yes, I agree that checking that a named engine exists should be enough for what you are doing. if the client wrongly specified another engine such as kv, he would not go very far with further operations using the pki rest api.
it is unfortunate that the tune operation does not return a type attribute. I have created hashicorp/vault#12349.

actually if you are not using the tune operation internally, then may be we should expose the tune operation on VaultSystemBackendEngine and be done with it. and remove your operation isEnabled from VaultPKISecretEngineFactory.

actually I am now thinking that the VaultPKISecretEngineFactory should be dropped in favor of VaultSystemBackendEngine.

[kdubb]

@vsevel VaultPKISecretEngineFactory.isEnabled has been removed. Support for the tune endpoint has been added to VaultSystemBackendEngine via getTuneInfo(String) and updateTuneInfo(String,VaultTuneInfo). I've also added a convenience method isEngineMounted(String) to VaultSystemBackendEngine to easily check if any engine is mounted; it basically wraps getTuneInfo(String) with try/catch and proper checks.

I'm thinking all that's left is the docs we've discussed.

[kdubb]

@vsevel @sberyozkin I've added a PKI guide modeled after the Transit guide. Let me know what you think. I'm hoping to get this PR wrapped up soon.

[vsevel]

@kdubb I will take a look before the next 2 days.

[vsevel]

hello @kdubb I like a lot your documentation. it really adds value. I also like the added services around the tuneInfo.
I reviewed carefully the format issue (beyond the default value itself). I think there is a bit more to discuss here.
There is also a point of discussion on where to locate the enable/disable operations.

overall I think we are closing on. I am very excited with your work.

[quarkus-bot]

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 526d235

Status	Name	Step	Failures	Logs	Raw logs
✖	Initial JDK 11 Build	Build	⚠️ Check →	Logs	Raw logs

[vsevel]

I like how you modeled the CertificateData.
I reviewed all methods that treated PEM encoded arguments. there are different situations.

• sometimes vault's api does not leave you the choice. may be we should use CertificateData.PEM for the type of the arg
  it has to be pem.
• sometimes it gives you the choice. in that case may be there should be an extra argument of type DataFormat. e.g. getCertificateRevocationList(DataFormat)

we can still keep your methods such as getCertificateRevocationList(), and treat them as opiniated in the sense that they decide that it will be PEM (because it is the usual case).

[kdubb]

@vsevel I think all of your issues that can be addressed have been.

[quarkus-bot]

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building a00711a

Status	Name	Step	Failures	Logs	Raw logs
✖	Native Tests - Security3	Build	Failures	Logs	Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ Native Tests - Security3 #

- Failing: integration-tests/vault-agroal 

📦 integration-tests/vault-agroal

✖ io.quarkus.vault.AgroalVaultITCase. - More details - Source on GitHub

java.lang.RuntimeException: Error waiting for test resource future to finish.
	at io.quarkus.test.common.TestResourceManager.waitForAllFutures(TestResourceManager.java:151)
	at io.quarkus.test.common.TestResourceManager.start(TestResourceManager.java:136)

✖ io.quarkus.vault.AgroalVaultKv1ITCase. - More details - Source on GitHub

java.lang.RuntimeException: Error waiting for test resource future to finish.
	at io.quarkus.test.common.TestResourceManager.waitForAllFutures(TestResourceManager.java:151)
	at io.quarkus.test.common.TestResourceManager.start(TestResourceManager.java:136)

✖ io.quarkus.vault.VaultKv1ITCase. - More details - Source on GitHub

java.lang.RuntimeException: Error waiting for test resource future to finish.
	at io.quarkus.test.common.TestResourceManager.waitForAllFutures(TestResourceManager.java:151)
	at io.quarkus.test.common.TestResourceManager.start(TestResourceManager.java:136)

[vsevel]

hello @kdubb did not have time today to look at your previous changes and answers.
I had checked vault's api, and did not grab the difference between raw /pki/ca(/pem) and /pki/cert at first.
I will have a last look tomorrow.

[vsevel]

I love it! it is a go for me. have you checked the generated documentation?
@kdubb please squash your commits and rebase.
@sberyozkin and @gsmet do you want to do a last review before merge?
note: as this is a new feature, it may be worth a reference in the release notes.

[kdubb]

@vsevel Squashed and rebased!

have you checked the generated documentation?

I've generated the docs locally and all seems to be good.

[kdubb]

@vsevel @sberyozkin @gsmet Any chance of merging this before next release?

[vsevel]

@sberyozkin @gsmet hope you are well. I feel confident about this PR. I will merge it early next week (which should allow it to make 2.3), unless you want to take a look, and need more time.

[vsevel]

@kdubb hello, can you rebase one last time, and I will merge it afterward. thanks.

[kdubb]

@vsevel Done!

[vsevel]

thank you for this new feature @kdubb!
@gsmet it would be nice to underline this addition in the release notes.