Add Vault Transit key administration

[vsevel]

Add CRUD operations on Transit Secret Engine keys.
see https://www.vaultproject.io/api-docs/secret/transit
Upgrade testcontainers to 1.15.0 to resolve macos issue testcontainers/testcontainers-java#3159

[gsmet]

Could you please rebase? The testcontainers upgrade has been merged here: #13162 .

Thanks!

[vsevel]

hello @gsmet I have rebased and squashed. I think we should be good to go.
the only thing worth noting is that I chose the option to return null on readKey if the key does not exist, as opposed to letting the 404 bubble up. apart from this, there is not much to discuss on those operations.

[sberyozkin]

@vsevel Hi Vincent, did we discuss it earlier and agree that the admin operations should be part of the dedicated admin interface such as this one ? Right, that may not be exactly relevant to the transit key management but if we do it here, then the same question would apply to other interfaces, example, we hide from the users many low -level/admin operations

Do you see some practical

[vsevel]

hello @sberyozkin
I do not remember, may be ;)
VaultSystemBackendEngine maps directly to the System Backend. it turns out that all operations on the System Backend may be considered admin
that is not the case for many of the other vault endpoints.
I do not mind mixing what could be considered as admin vs non admin to some standards. that is what SB is doing actually in VaultTransitOperations.
so I did no think we would need to make this separation. do you? would you rather introduce a VaultAdminTransitSecretEngine?

[sberyozkin]

Hi Vincent, @vsevel, well, yeah, we spent weeks debating it :-)

Actually I was thinking about it, about my last comment; so you refer to SB. The difference between what we have in Quarkus (what you created) and in SB and likely other providers is that here we have a very user centric interface which is unique enough for Quarkus; the interfaces like this one where whatever is available in a given Vault engine is mixed up in the same interface do not impress me :-), it is just the enumeration of all the operations, not the best approach IMHO.

I don't want to make this PR delayed etc, just a general consideration which I'd consider applying myself.

Re VaultAdminTransitSecretEngine - IMHO it would offer the right separation of concerns. I reckon it is fair enough to assume that no-one will for example create a secret key and then use it to sign/encrypt something in context of a single client request (executed in a server context) - as it will require 2 background remote operations - we'd have either some management service where these keys are provisioned or a user service where they are used.

The only concern I'd have about VaultAdminTransitSecretEngine is that we'd end up with twice as many interfaces as we have now, if we do it per every other interface. But the separation of the concerns seems more important. These inerfaces can sit in a admin sub-package for example and even extend the simple ones, ex, VaultAdminTransitSecretEngine extends VaultTransitSecretEngine...

Is it convincing enough :-) ? It is really a proposal to discuss as opposed to a review change request as I agree that it will work in the current PR form too

thanks

[vsevel]

yes, I guess it is a matter

• of sticking to the original api to avoid surprising the users (after all hashicorp did not create 2 interfaces admin/non-admin, and sometimes it is difficult to figure out which is which - is readSecret non-admin, and writeSecret admin? - today we both have them in VaultKVSecretEngine and that feels right),
• or creating another abstraction to provide a clearer separation of concerns that we determine was missing in the original api, and would undermine clarity for the users.

in that case, I was leaning toward the first option, because as a user, if I decided to use the transit secret engine, I would search for a class with that name and expect everything to be there. but again there is no right or wrong. it would be nice to have a third opinion. @gsmet?

no strong feeling on my side if a separation of interfaces means better respecting the quarkus philosophy, just a preference for consistency against the original api. but I will do whatever you guys believe is right for the product.

[sberyozkin]

Hi @vsevel

Yeah, they have the endpoint and it is indeed simpler for them to just have a single endpoint dealing with all the operations.

Have a look at VaultClient. We also have, for example, a super simple for the users to understand interface, VaultKVSecretEngine - I recall writeSecret was added at a user request as there was a concrete use case, etc.

At some point there was only VaultClient and I do feel the fact that the KV engine users don't see most of VaultClient operations is a good thing for them :-)

However some interfaces are less strict about it, Kubernetes one for ex.

I guess we just need to try to be conservative as to how much of the management operations we can put into the otherwise user centric interfaces.

Perhaps in the case of this specific PR it is OK.

[vsevel]

hello @sberyozkin

Have a look at VaultClient

well I think this one is a bad example ;) for one it is an implementation detail, and we should have refactored it as we were adding more and more methods. and even as an implemation detail, it is too much. I will split at some point.

I do feel the fact that the KV engine users don't see most of VaultClient operations is a good thing for them :-)

definitely

[sberyozkin]

If ever needed we can perhaps make a breaking change and move these admin operations into a sub class or a separate interface but for now it is a reasonable update

[vsevel]

If ever needed we can perhaps make a breaking change and move these admin operations into a sub class or a separate interface but for now it is a reasonable update

yes we could.
I really believe there would be value to get a third of fourth opinion. as I said multiple times there is no right or wrong, just 2 slightly different philosophies to api design. I was not trying to win the argument when I suggested to bring in some other people. I am genuinely interested in what others would have to say. and this would have the added value of facilitating future discussions. thanks for approving in the meantime. let's see if @gsmet or others have additional comments.

[sberyozkin]

@vsevel Np, thanks; see we already have configure operation in Kubernetes which is an admin one;
KV writeSecret had some application specific flavour to it as far as I recall, and these transit engine key admin operations might have similar application specific associations...which is why it is perhaps still very close in line with the idea that these interfaces should ideally remain as user centric as possible

[sberyozkin]

@vsevel Hi Vincent - are you OK for this PR to be merged now ?

[vsevel]

yes. thanks.