Support any dynamic credentials in VaultCredentialsProvider #21279

kdubb · 2021-11-08T15:17:39Z

This change is in preparation of support for dynamic Vault credentials for RabbitMQ. It renames the "database" dynamic credentials supports to use "dynamic" in the names in an effort to be clear about its use.

Vault dynamic credentials all have basicallly identical responses so the credentials provider should support any of them easily.

This does essentially 4 things.

Renames VaultDatabaseCredentials, VaultDBManager and supporting classes to use the naming "Dynamic"
Renames & deprecates databaseCredentialsRole to credentialsRole
Adds credentialsMount config and adds this as a parameter to VaultDynamicCredentialsManager.getDynamicCredentials
Adds an expires-at property to the returned credentials which is calculated from the Vault lease duration.

kdubb · 2021-11-08T15:21:21Z

A follow on PR for the actual RabbitMQ changes can be viewed at kdubb@276c300.

It requires a change to the smallrye-reactive-messaging-rabbitmq connector that is currently awaiting a release.

kdubb · 2021-11-08T15:29:12Z

@vsevel Feedback on this would be great. Just wanting to know if this is a good direction that is acceptable to the team.

quarkus-bot · 2021-11-08T17:15:13Z

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

Failing Jobs - Building `09b5dd4`

Status	Name	Step	Failures	Logs	Raw logs
✖	JVM Tests - JDK 11	`Build`	Failures	Logs	Raw logs
✖	JVM Tests - JDK 17	`Build`	Failures	Logs	Raw logs
✖	Native Tests - Security3	`Build`	Failures	Logs	Raw logs
✖	Native Tests - Windows - hibernate-validator	`Build`	Failures	Logs	Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: integration-tests/vault

📦 integration-tests/vault

✖ io.quarkus.vault.VaultITCase.httpclient line 211 - More details - Source on GitHub

io.quarkus.vault.runtime.client.VaultClientException code=403 body={"errors":["permission denied"]}

	at io.quarkus.vault.runtime.client.VertxVaultClient.throwVaultException(VertxVaultClient.java:235)

⚙️ JVM Tests - JDK 17 #

- Failing: integration-tests/vault

📦 integration-tests/vault

✖ io.quarkus.vault.VaultITCase.httpclient line 211 - More details - Source on GitHub

io.quarkus.vault.runtime.client.VaultClientException code=403 body={"errors":["permission denied"]}

	at io.quarkus.vault.runtime.client.VertxVaultClient.throwVaultException(VertxVaultClient.java:235)

⚙️ Native Tests - Security3 #

- Failing: integration-tests/vault

📦 integration-tests/vault

✖ io.quarkus.vault.VaultITCase.httpclient line 211 - More details - Source on GitHub

io.quarkus.vault.runtime.client.VaultClientException code=403 body={"errors":["permission denied"]}

	at io.quarkus.vault.runtime.client.VertxVaultClient.throwVaultException(VertxVaultClient.java:235)

⚙️ Native Tests - Windows - hibernate-validator #

- Failing: integration-tests/hibernate-validator

📦 integration-tests/hibernate-validator

✖ Failed to execute goal io.quarkus:quarkus-maven-plugin:999-SNAPSHOT:build (default) on project quarkus-integration-test-hibernate-validator: Failed to build quarkus application

quarkus-bot · 2021-11-08T20:39:48Z

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

Failing Jobs - Building `df3a8fa`

Status	Name	Step	Failures	Logs	Raw logs
✖	Native Tests - Windows - hibernate-validator	`Build`	Failures	Logs	Raw logs

Failures

⚙️ Native Tests - Windows - hibernate-validator #

- Failing: integration-tests/hibernate-validator

📦 integration-tests/hibernate-validator

✖ Failed to execute goal io.quarkus:quarkus-maven-plugin:999-SNAPSHOT:build (default) on project quarkus-integration-test-hibernate-validator: Failed to build quarkus application

quarkus-bot · 2021-11-09T05:46:47Z

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

Failing Jobs - Building `5ecf0e2`

Status	Name	Step	Failures	Logs	Raw logs
✖	Gradle Tests - JDK 11 Windows	`Build`	Failures	Logs	Raw logs
✖	Native Tests - Windows - hibernate-validator	`Build`	Failures	Logs	Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ Gradle Tests - JDK 11 Windows #

- Failing: integration-tests/gradle

📦 integration-tests/gradle

✖ io.quarkus.gradle.devmode.CustomQuarkusDevModeConfigurationTest.main line 13 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.test.devmode.util.DevModeTestUtils that uses java.util.function.Supplier, java.util.function.Supplierjava.util.concurrent.atomic.AtomicReference, java.util.concurrent.atomic.AtomicReferencejava.lang.String, java.lang.Stringboolean was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:164)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

⚙️ Native Tests - Windows - hibernate-validator #

- Failing: integration-tests/hibernate-validator

📦 integration-tests/hibernate-validator

✖ Failed to execute goal io.quarkus:quarkus-maven-plugin:999-SNAPSHOT:build (default) on project quarkus-integration-test-hibernate-validator: Failed to build quarkus application

quarkus-bot · 2021-11-10T17:17:29Z

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

Failing Jobs - Building `b60e125`

Status	Name	Step	Failures	Logs	Raw logs
✖	Native Tests - Windows - hibernate-validator	`Build`	Failures	Logs	Raw logs

Failures

⚙️ Native Tests - Windows - hibernate-validator #

- Failing: integration-tests/hibernate-validator

📦 integration-tests/hibernate-validator

✖ Failed to execute goal io.quarkus:quarkus-maven-plugin:999-SNAPSHOT:build (default) on project quarkus-integration-test-hibernate-validator: Failed to build quarkus application

vsevel · 2021-11-11T13:59:53Z

@sberyozkin we have had a long chat with @kdubb yesterday about this PR. we have agreed on the direction.
@kdubb I won't be able to look at it again before next monday.

kdubb · 2021-12-02T00:29:04Z

@vsevel Can we get this reviewed and merged? I've rebased and updated the relevant docs.

The SmallRye RabbitMQ connector version I need to finish dynamic credentials support is now available. Just need to get this in and I can submit that PR.

sberyozkin · 2021-12-02T14:22:35Z

@kdubb

I'm not sure there is a strong enough reason to introduce a breaking change with the Renames VaultDatabaseCredentials, VaultDBManager and supporting classes to use the naming "Dynamic". Not 100% sure these classes can be injected into a user code but I see the properties have changed. Doc clarifications should be enough IMHO

kdubb · 2021-12-02T14:32:14Z

@sberyozkin I'm not sure these should be considered outwardly breaking changes. These classes are internal and provide configuration based dynamic credentials; I'm gonna bet that nobody injects any of those classes. If you were gonna inject any of these you'd inject the io.quarkus.credentials.CredentialsProvider directly; which has not been renamed.

Additionally, we need to add configurable values (e.g. credentials-type) to allow the classes to support non "database" dynamic credentials. Without renames you'd have "database" classes that would have a credentials type required to be set to "database". Also, all the old configuration values continue to be supported.

@vsevel and I had a long chat about this to agree on the new names and configuration value names as well.

docs/src/main/asciidoc/vault-datasource.adoc

...s/vault/runtime/src/main/java/io/quarkus/vault/runtime/config/CredentialsProviderConfig.java

sberyozkin · 2021-12-02T15:12:07Z

@kdubb Sure if @vsevel supports it is fine

I haven't been able to find VaultDynamicCredentialsType - perhaps it is no longer necessary ?

kdubb · 2021-12-02T15:21:41Z

@sberyozkin VaultCredentialsType was for credentials-type. After our chat, we decided to remove and replace it with a simple string. Using a string allows it to support any of Vault's dynamic credentials providers as well as one's with custom mount paths.

We also agreed to add the credentials-request-path option which allows configuration to support literally any current or future Vault secret engines. Currently all of the Vault secret engines for dynamic credentials use the value creds; which is the default for this configuration value.

sberyozkin · 2021-12-02T15:58:27Z

@kdubb Thanks for the explanation

sberyozkin

Please update the 2.6 migration guide

gsmet · 2021-12-02T18:59:01Z

@sberyozkin it's a wiki so @kdubb can't. @kdubb, if you provide a section in Markdown, I will add it there: https://github.com/quarkusio/quarkus/wiki/Migration-Guide-2.6 .

@sberyozkin @vsevel if this one is ready, I would prefer we merge it really soon as I'd like it to be in before I move the Vault extension outside of the Core. I plan to do that either tomorrow afternoon or on Monday.

vsevel · 2021-12-02T19:01:27Z

hello @kdubb. thanks for this contribution. I will go through our chat (there may be some elements from the discussion we can share here for transparency and would help explain the rationale plus support discussion) again tomorrow and spend some time on the review.

kdubb · 2021-12-02T19:22:28Z

@gsmet The only changes worth noting in migration are the deprecation of the database-credentials-role configuration value; as it is only deprecated, the original value still works.

Suggested excerpt for the migration guide:
**** Updated with credentials-mount rename ****

Vault Datasource Credentials Provider

The configuration for Vault dynamic datasource credentials was changed as more more types of dynamic credentials are supported. Specifically the database-credentials-role key for configuring a credentials provider was deprecated. It has been replaced with two keys credentials-role and credentials-mount.

Using the example from the Dynamic Database Credentials guide. The previously configuration of:

quarkus.vault.credentials-provider.mydatabase.database-credentials-role=mydbrole

Changes to:

quarkus.vault.credentials-provider.mydatabase.credentials-role=mydbrole
quarkus.vault.credentials-provider.mydatabase.credentials-mount=database

Note that the previous configuration key of database-credentials-role has only been deprecated and will continue to work.

vsevel

Hello @kdubb I have mainly 2 comments:

I think we should have a credentials type config parameter separated from the mount point
Then we should probably have calculated defaults for the mount and forced values for the request path based on this credentials type

open to discussion.

extensions/vault/runtime/src/main/java/io/quarkus/vault/runtime/VaultCredentialsProvider.java

...ons/vault/runtime/src/main/java/io/quarkus/vault/runtime/VaultDynamicCredentialsManager.java

...s/vault/runtime/src/main/java/io/quarkus/vault/runtime/config/CredentialsProviderConfig.java

...ons/vault/runtime/src/main/java/io/quarkus/vault/runtime/VaultDynamicCredentialsManager.java

integration-tests/vault/src/test/java/io/quarkus/vault/VaultITCase.java

kdubb · 2021-12-03T15:49:31Z

@vsevel Using credentials-type (or if renamed, credentials-mount-point), credentials-role and credentials-request-path is less pedantic but it supports any conceivable secrets plugin without additional code. This includes multiple versions of the same plugin mounted at custom paths or even custom plugins.

If we were to be more pedantic and have a credentials-type with selectable credentials-mount-point and calculate it's request path then supporting a new plugin down the road becomes a code change. That's what I'm trying to avoid with this style. I don't want to have to change Quarkus to support each dynamic credentials secrets plugin. As I said above, currently credentials-request-path is always creds and might always be creds; it's only there to future proof the code in the event some new plugin shows up that doesn't use it.

So, in my opinion, the only thing I think we should be considering is a name change of the credentials-type option to credentials-mount-point.

kdubb · 2021-12-03T15:52:30Z

Actually, if we were to change credentials-type, I would probably vote for the shorter credentials-mount.

gsmet

Please coordinate with me (@gsmet) before merging this as we don't want to merge this after I moved the code to the Quarkiverse.

vsevel · 2021-12-03T18:26:22Z

@kdubb I vote for replacing the type by credentials-mount. it is closer to the real semantic, and makes sense when used along the requestn path attribute. but as you said, usually in vault the mount path is the type of the engine. I suppose circumstances are rare when you need to use another mount path (an example is having multiple kv engines with different versions and/or config).
anyway, I could go either way as well. I will leave you with the final word on this.
please do not forget to squash. thanks.

kdubb · 2021-12-03T20:52:21Z

@vsevel Renamed credentials-type to credentials-mount. Ultimately the decision rested with the fact that all the internal code was already referring to it as "mount". So now its uniform and it was ultimately a small change that only affected config key.

…alsProvider This change is in preparation of support for dynamic Vault credentials for RabbitMQ. It renames the "database" dynamic credentials supports to use "dynamic" in the names in an effort to be clear about its use. Vault dynamic credentials all have basicallly identical responses so the credentials provider should support any of them easily. This does essentially 4 things. 1. Renames `VaultDatabaseCredentials`, `VaultDBManager` and supporting classes to use the naming "Dynamic" 2. Renames & deprecates `databaseCredentialsRole` to `credentialsRole` 3. `credentialsMount` config and adds this as a parameter to `VaultDynamicCredentialsManager.getDynamicCredentials` 4. Adds an `expires-at` property to the returned credentials which is calculated from the Vault lease duration.

kdubb · 2021-12-03T21:06:15Z

@gsmet Squished and rebased and I updated the migration blurb above.

Let's merge

gsmet · 2021-12-06T11:06:03Z

Let's merge this, thanks!

quarkus-bot bot added the area/vault label Nov 8, 2021

kdubb force-pushed the feature/vault_generic_dyn_creds branch from 09b5dd4 to df3a8fa Compare November 8, 2021 18:26

kdubb force-pushed the feature/vault_generic_dyn_creds branch from df3a8fa to 5ecf0e2 Compare November 9, 2021 03:38

sberyozkin requested a review from vsevel November 9, 2021 09:20

kdubb force-pushed the feature/vault_generic_dyn_creds branch from 5ecf0e2 to b60e125 Compare November 10, 2021 15:03

kdubb force-pushed the feature/vault_generic_dyn_creds branch from fd857ae to 24c4558 Compare November 28, 2021 21:00

quarkus-bot bot added the area/documentation label Nov 28, 2021

kdubb force-pushed the feature/vault_generic_dyn_creds branch 2 times, most recently from 874cd34 to e164aac Compare December 2, 2021 00:26

sberyozkin reviewed Dec 2, 2021

View reviewed changes

docs/src/main/asciidoc/vault-datasource.adoc Outdated Show resolved Hide resolved

sberyozkin reviewed Dec 2, 2021

View reviewed changes

...s/vault/runtime/src/main/java/io/quarkus/vault/runtime/config/CredentialsProviderConfig.java Show resolved Hide resolved

kdubb force-pushed the feature/vault_generic_dyn_creds branch from e164aac to d77f8dd Compare December 2, 2021 15:28

sberyozkin self-requested a review December 2, 2021 15:58

sberyozkin approved these changes Dec 2, 2021

View reviewed changes

kdubb mentioned this pull request Dec 2, 2021

RabbitMQ dynamic credentials via credentials provider #21897

Merged

vsevel requested changes Dec 3, 2021

View reviewed changes

gsmet previously requested changes Dec 3, 2021

View reviewed changes

kdubb force-pushed the feature/vault_generic_dyn_creds branch from d77f8dd to 58e7523 Compare December 3, 2021 16:55

vsevel self-requested a review December 3, 2021 18:27

vsevel approved these changes Dec 3, 2021

View reviewed changes

kdubb force-pushed the feature/vault_generic_dyn_creds branch from eb734d8 to 43eac92 Compare December 3, 2021 20:48

kdubb force-pushed the feature/vault_generic_dyn_creds branch from 43eac92 to cd23d5d Compare December 3, 2021 20:58

gsmet merged commit 1110076 into quarkusio:main Dec 6, 2021

quarkus-bot bot added this to the 2.6 - main milestone Dec 6, 2021

kdubb deleted the feature/vault_generic_dyn_creds branch December 6, 2021 21:39

Support any dynamic credentials in VaultCredentialsProvider #21279

Support any dynamic credentials in VaultCredentialsProvider #21279

Conversation

kdubb commented Nov 8, 2021 • edited Loading

kdubb commented Nov 8, 2021 • edited Loading

kdubb commented Nov 8, 2021

quarkus-bot bot commented Nov 8, 2021 • edited Loading

Failing Jobs - Building 09b5dd4

Failures

⚙️ JVM Tests - JDK 11 #

📦 integration-tests/vault

⚙️ JVM Tests - JDK 17 #

📦 integration-tests/vault

⚙️ Native Tests - Security3 #

📦 integration-tests/vault

⚙️ Native Tests - Windows - hibernate-validator #

📦 integration-tests/hibernate-validator

quarkus-bot bot commented Nov 8, 2021 • edited Loading

Failing Jobs - Building df3a8fa

Failures

⚙️ Native Tests - Windows - hibernate-validator #

📦 integration-tests/hibernate-validator

quarkus-bot bot commented Nov 9, 2021 • edited Loading

Failing Jobs - Building 5ecf0e2

Failures

⚙️ Gradle Tests - JDK 11 Windows #

📦 integration-tests/gradle

⚙️ Native Tests - Windows - hibernate-validator #

📦 integration-tests/hibernate-validator

quarkus-bot bot commented Nov 10, 2021 • edited Loading

Failing Jobs - Building b60e125

Failures

⚙️ Native Tests - Windows - hibernate-validator #

📦 integration-tests/hibernate-validator

vsevel commented Nov 11, 2021

kdubb commented Dec 2, 2021 • edited Loading

sberyozkin commented Dec 2, 2021 • edited Loading

kdubb commented Dec 2, 2021 • edited Loading

sberyozkin commented Dec 2, 2021

kdubb commented Dec 2, 2021 • edited Loading

sberyozkin commented Dec 2, 2021

sberyozkin left a comment

Choose a reason for hiding this comment

gsmet commented Dec 2, 2021

vsevel commented Dec 2, 2021

kdubb commented Dec 2, 2021 • edited Loading

Vault Datasource Credentials Provider

vsevel left a comment

Choose a reason for hiding this comment

kdubb commented Dec 3, 2021 • edited Loading

kdubb commented Dec 3, 2021

gsmet left a comment

Choose a reason for hiding this comment

vsevel commented Dec 3, 2021

kdubb commented Dec 3, 2021

kdubb commented Dec 3, 2021 • edited Loading

gsmet commented Dec 6, 2021

kdubb commented Nov 8, 2021 •

edited

Loading

kdubb commented Nov 8, 2021 •

edited

Loading

quarkus-bot bot commented Nov 8, 2021 •

edited

Loading

Failing Jobs - Building `09b5dd4`

quarkus-bot bot commented Nov 8, 2021 •

edited

Loading

Failing Jobs - Building `df3a8fa`

quarkus-bot bot commented Nov 9, 2021 •

edited

Loading

Failing Jobs - Building `5ecf0e2`

quarkus-bot bot commented Nov 10, 2021 •

edited

Loading

Failing Jobs - Building `b60e125`

kdubb commented Dec 2, 2021 •

edited

Loading

sberyozkin commented Dec 2, 2021 •

edited

Loading

kdubb commented Dec 2, 2021 •

edited

Loading

kdubb commented Dec 2, 2021 •

edited

Loading

kdubb commented Dec 2, 2021 •

edited

Loading

kdubb commented Dec 3, 2021 •

edited

Loading

kdubb commented Dec 3, 2021 •

edited

Loading