Enhance the programmatic access to the KV secret engine #184

bmscomp · 2023-10-15T22:07:05Z

The purpose of this pull request is to enhance the programmatic access to the KV secret engine, to give it the ability to access a multi mount KV secret engine, it comes that in some production environnement secrets are stored on different mounts for different domains, and this is not allowed in the current version of Quarkus vault extension, even when using the programatic way it comes always to retrieve the secrets only the configured mount set up in the properties file

In the current pull request we overloaded some methods to give the extension this ability, means if we have three mounts for example we can read from them programmatically even if we configured a default mount to read from

kdubb · 2023-10-15T22:10:43Z

The PKI engine uses a different method. It simply allows creating an "engine" for a given "mount"; instead of overloading every conceivable method.

I think that's the model to follow.

bmscomp · 2023-10-15T22:13:10Z

@kdubb Yes it's a good idea I need a name for this engine, if you can help on finding a name for it it will be great for me, yes I agree it's better to define a new engine rather then overload the existing engine and method, notice that the other engine will always be of kind KV , what do you think about VaultKVSecretMultiMountEngine ? as name

kdubb · 2023-10-15T22:24:50Z

You don't need a new engine. You just need to allowing creating VaultKVManager with a specific mount point (like PKI does).

bmscomp · 2023-10-15T22:39:44Z

@kdubb I can get it now, I'll give it a try

bmscomp · 2023-10-17T13:25:30Z

@kdubb I made some changes for the pull request now the Oliver loaded methods are still here, it may be a kind of shortcut, in the next tastemaker I'll remove it

bmscomp · 2023-10-28T20:11:49Z

@kdubb I updated the pull request and I also rebased it please can you give it a check, Thanks in avance

bmscomp · 2023-11-08T08:37:09Z

@kdubb @vsevel Please provide me with a feedback after the latest changes to enhance my pull request

Thank you

kdubb · 2023-11-19T00:42:26Z

@bmscomp Not ignoring you or your contribution. I am swamped with current work and it's going to be a while before I can weigh in on this.

bmscomp · 2023-11-19T10:48:04Z

@kdubb Thank you su much for the feedback

kdubb

Structure is good and similar to PKI thus providing a consistent method for programmatic access.

Just need to minimize unnecessary changes.

kdubb · 2023-12-07T15:23:37Z

runtime/src/main/java/io/quarkus/vault/VaultKVSecretEngine.java

@@ -18,7 +18,8 @@
 @ApplicationScoped
 public class VaultKVSecretEngine {

-    private final VaultKVSecretReactiveEngine engine;
+    @Inject


Why this change? It was already using constructor injection into a final field?

@kdubb I agree that constructor based injection is better , I will fix that

@kdubb Done

runtime/src/main/java/io/quarkus/vault/runtime/VaultKvManager.java

model/src/main/java/io/quarkus/vault/runtime/client/dto/kv/VaultKvSecretV2WriteBody.java

kdubb · 2023-12-07T15:28:59Z

runtime/src/main/java/io/quarkus/vault/runtime/VaultKvManager.java


+    private Uni<Map<String, Object>> readValues(String mount, String path) {


Can't you name this readSecret as well? It has a mount path to handle the overload.

@kdubb I just followed the existing naming for the private methods for reading and writing secrects or values

For example the existing private method private Uni<Void> writeValues(String mount, String path, Map<String, String> secret) that is used on another public method

kdubb · 2023-12-07T15:29:23Z

runtime/src/main/java/io/quarkus/vault/runtime/VaultKvManager.java

@@ -77,27 +111,30 @@ public Uni<Map<String, Object>> readSecretJson(String path) {
        });
    }

-    @Override
-    public Uni<Void> writeSecret(String path, Map<String, String> secret) {
+    private Uni<Void> writeValues(String mount, String path, Map<String, String> secret) {


writeSecret? No need to change all the names.

writeSecret method did not change

@Override public Uni<Void> writeSecret(String path, Map<String, String> secret) { return writeValues(mount, path, secret); }

It is just using the private method writeValues https://github.com/quarkiverse/quarkus-vault/pull/184/files#diff-6e7c8fa98f912471ec483ca0bc3ca258d768dd009f2f974f39c76065e6932e04R127

kdubb · 2023-12-07T15:30:06Z

runtime/src/main/java/io/quarkus/vault/runtime/VaultKvManager.java

@@ -107,16 +144,15 @@ public Uni<Void> deleteSecret(String path) {
        });
    }

+    private Uni<List<String>> listValues(String mount, String path) {


listSecrets?

In VaultKvManager.java class when it comes to private methods that are used for reading writing and listing values of secrets we use the word Value for example :

private Uni<Void> deleteValues(String mount, String path) {

private Uni<Void> writeValues(String mount, String path, Map<String, String> secret) {

bmscomp · 2023-12-14T10:21:55Z

@kdubb I'll rebase the brach and solve conflicts and then take care of the comment's reviews by the end of day

bmscomp · 2023-12-18T13:37:36Z

@kdubb I updated the pull request, answered the questions, please can you take a look to this last update of the pull request

kdubb · 2024-01-03T06:54:01Z

@bmscomp Please look at #214 & #215. It does this and a whole lot more.

If you can please build it locally and comment on #214 with ideas and/or #215 with specific suggestions for changes.

bmscomp · 2024-01-04T23:26:52Z

@kdubb Thanks for your answer, I checked the great work done to bring up a new client to vault extension for Quarkus, and I think the content of this pull request still remains important as an evolution to the programmatic way to support multi without any breaking changes to the existing way to use a programatic way to get vault secrets, we can merge this pull request then after try to enhance the api with your implementation of vault client, since we still need in some cases something strongly coupled with Quarkus, this will also enrich the existing API, Please would you consider the updated review

Thanks in advance

bmscomp · 2024-01-08T14:08:21Z

@kdubb @vsevel please any feedback about the current pull request

kdubb · 2024-01-08T15:30:16Z

@bmscomp The new client is still deeply integrated into Quarkus. It has the added ability to be run standalone without using Quarkus static configuration. Additionally, all of the apis that support a dynamic mount path (e.g., any "secret" api like kv) can be easily accessed.

For example, to access a kv engine mounted at a specific path using the new client you would do this:

@Inject VaultClient client; // Inject Quarkus configured client

Map<String, Object> getSomeSecret(String mount) {
   return client.secrets().kv2(mount).read("something").getData();
}

If you want a "preconfigured" mount you can create a CDI @Produces for it.

@Produces
VaultSecretsKV2 getSecretAPI(VaultClient client, @ConfigProperty("app.kv.mount") String mount) {
   return client.secrets.kv2(mount);
}

And use it like so:

@Inject VaultSecretsKV2 kv2;

Map<String, Object> getSomeSecret() {
  return kv2.read("something").getData();
}

Given the advantages of the new client. The current thinking is to deprecate and remove the current "engines" because having and maintaining two separate implementations is a burden.

Currently PR #215 re-implements the "engines" using the new client simply to allow using it in its current form and validate that the new client is solid and performing correctly.

You are advocating for enhancing the current KV engine at the same time. This would keep users using the "old" system when we want them to be replacing it by using the new client approach.

kdubb · 2024-01-08T15:37:58Z

@bmscomp I understand that this new client PR undermines the work you've done in this PR, but it was all the work that you were required to do that inspired me to get to work on the new client.

Transforming each engine into one that can be both statically configured and dynamically configured, as you now know, is quite a bit of work. Additionally, the CDI model is a bit complicated to ensure both a reactive and non-reactive api exists for everything.

I wanted to get away from this model... quickly. It slows down development and introduces a maintenance headache in the long run. The new client solves all these problems and more, all the while making development, whether adding new APIs or maintaining old ones, easier.

bmscomp · 2024-01-08T15:46:48Z

@kdubb Then would you merge your pull request

kdubb · 2024-01-08T15:49:02Z

Waiting for review. I've been in contact with @vsevelv and he's going to look at it early this week.

bmscomp · 2024-01-09T01:20:05Z

@kdubb Can you please make public my answers to your reviews, at least other developers or reviews or developers can suggest more

kdubb · 2024-01-09T01:22:47Z

@bmscomp I have no idea what you're requesting. I believe everything in Quarkiverse projects is public (and we team members do not control the project settings, that's done by the Quarkus admins).

bmscomp · 2024-01-09T01:31:46Z

@kdubb it is totally my mistake apologies, I just could not submit them I think, it is totally mist mistake, I was really on this case https://github.com/orgs/community/discussions/10369

Answers to your reviews are submited, sorry for delay

bmscomp · 2024-01-17T22:57:55Z

Please @gsmet @kdubb Can you take a look at this pull request, it's updated regarding the last comments and we really need this feature

bmscomp · 2024-02-06T11:11:19Z

Any news about this pull request ?

kdubb · 2024-03-02T00:52:53Z

4.0.0-alpha.2 has been released. Injecting the VaultClient (via @Inject VaultClient client) and using client.secrets().kv1(mountPath) or client.secrets().kv2(mountPath) allows you to supply any mount path.

I'm closing this now that dynamic mounts should use the client directly instead.

bmscomp changed the title ~~Enhance programmatic access to the KV secret engine~~ Enhance the programmatic access to the KV secret engine Oct 15, 2023

bmscomp force-pushed the feature/configurable_mount_for_vault branch from d114e53 to 40161bc Compare November 11, 2023 10:16

kdubb requested changes Dec 7, 2023

View reviewed changes

kdubb mentioned this pull request Dec 7, 2023

Make transit engine mount path configurable #208

Closed

bmscomp added 8 commits December 14, 2023 11:48

enable multi mount sercrets readings

9813bf3

fixe serialization issue

e47f304

remove commented instruction

1c8962d

format vault json files configurations

fe159d4

add some test and adjust documentation

ddfb376

add end of files for json configuration vault files

b7ad719

introduce Vault kv manager factory

de58867

remove method overriding for VaultKVSecretReactiveEngine interface

bc6d3b1

bmscomp force-pushed the feature/configurable_mount_for_vault branch from ac73ed4 to bc6d3b1 Compare December 14, 2023 10:55

bmscomp added 2 commits December 14, 2023 12:18

use constructor based injection

4cc5a3f

format json configuration

1908799

bmscomp requested a review from kdubb December 14, 2023 13:02

kdubb closed this Mar 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance the programmatic access to the KV secret engine #184

Enhance the programmatic access to the KV secret engine #184

bmscomp commented Oct 15, 2023 •

edited

Loading

kdubb commented Oct 15, 2023

bmscomp commented Oct 15, 2023 •

edited

Loading

kdubb commented Oct 15, 2023

bmscomp commented Oct 15, 2023

bmscomp commented Oct 17, 2023

bmscomp commented Oct 28, 2023

bmscomp commented Nov 8, 2023 •

edited

Loading

kdubb commented Nov 19, 2023

bmscomp commented Nov 19, 2023

kdubb left a comment

kdubb Dec 7, 2023

bmscomp Dec 14, 2023

bmscomp Dec 14, 2023

kdubb Dec 7, 2023

bmscomp Dec 14, 2023

kdubb Dec 7, 2023

bmscomp Dec 14, 2023

kdubb Dec 7, 2023

bmscomp Dec 14, 2023

bmscomp commented Dec 14, 2023

bmscomp commented Dec 18, 2023

kdubb commented Jan 3, 2024

bmscomp commented Jan 4, 2024

bmscomp commented Jan 8, 2024

kdubb commented Jan 8, 2024 •

edited

Loading

kdubb commented Jan 8, 2024 •

edited

Loading

bmscomp commented Jan 8, 2024

kdubb commented Jan 8, 2024

bmscomp commented Jan 9, 2024

kdubb commented Jan 9, 2024

bmscomp commented Jan 9, 2024 •

edited

Loading

bmscomp commented Jan 17, 2024

bmscomp commented Feb 6, 2024

kdubb commented Mar 2, 2024


		private Uni<Map<String, Object>> readValues(String mount, String path) {

Enhance the programmatic access to the KV secret engine #184

Enhance the programmatic access to the KV secret engine #184

Conversation

bmscomp commented Oct 15, 2023 • edited Loading

kdubb commented Oct 15, 2023

bmscomp commented Oct 15, 2023 • edited Loading

kdubb commented Oct 15, 2023

bmscomp commented Oct 15, 2023

bmscomp commented Oct 17, 2023

bmscomp commented Oct 28, 2023

bmscomp commented Nov 8, 2023 • edited Loading

kdubb commented Nov 19, 2023

bmscomp commented Nov 19, 2023

kdubb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmscomp commented Dec 14, 2023

bmscomp commented Dec 18, 2023

kdubb commented Jan 3, 2024

bmscomp commented Jan 4, 2024

bmscomp commented Jan 8, 2024

kdubb commented Jan 8, 2024 • edited Loading

kdubb commented Jan 8, 2024 • edited Loading

bmscomp commented Jan 8, 2024

kdubb commented Jan 8, 2024

bmscomp commented Jan 9, 2024

kdubb commented Jan 9, 2024

bmscomp commented Jan 9, 2024 • edited Loading

bmscomp commented Jan 17, 2024

bmscomp commented Feb 6, 2024

kdubb commented Mar 2, 2024

bmscomp commented Oct 15, 2023 •

edited

Loading

bmscomp commented Oct 15, 2023 •

edited

Loading

bmscomp commented Nov 8, 2023 •

edited

Loading

kdubb commented Jan 8, 2024 •

edited

Loading

kdubb commented Jan 8, 2024 •

edited

Loading

bmscomp commented Jan 9, 2024 •

edited

Loading