Patch update of consul/config/access parameters

[danieleva]

Is your feature request related to a problem? Please describe. The config/access endpoint for consul secret backend overwrites all the fields in the storage, regardless of which parameters are passed in the request. That works perfectly when the backend configuration is a once off operation, and it's performed by a single operator. In my case I have 2 different processes that takes care of setting up/rotating the consul token and TLS certificates, and that breaks the config as one process will reset the other. Having a single process taking care of both TLS and consul token rotation will impact the threat model of my system and I'd like to avoid that.

Describe the solution you'd like A modified config/access endpoint supporting PATCH style updates would be preferable. Since vault API makes no difference between post/patch/put I can see 2 ways to do this: 1- modify pathConfigAccessWrite to overwrite only the fields provided in the request (similar to what is done client-side in the kv patch command), and add a delete operation to cleanup the config. 2 - add new endpoints to independently overwrite TLS cert/key, address/scheme and token fields respectively.

Option 1 looks more elegant to me, as it won't require changes if the access data structure gets modified in the future. Again, I can architect a system where the process responsible for TLS updates grabs the ACL bootstrap token from a secure location but that will increase the attack surface and I think the feature proposed here can be useful to other users with similar security concerns and restrictions. I'm happy to provide a PR for it if you are interested in the feature

[ncabatoff]

Hi @danieleva,

The pattern we usually follow for this sort of thing is to define an ExistenceCheck in the backend, and then distinguish between Create and Update requests, where the latter may provide only a subset of the fields and only those will be modified. This is similar to your option 1, but only in the update case. Many of our secrets engines and auth methods support this behaviour for their config endpoints, though it's not always documented.

Unfortunately I don't see a good path forward to add this behaviour to the Consul secrets engine without breaking backwards compatibility. Your option 2 doesn't suffer from this issue, but it's inconsistent with our other APIs, so I'm not fond of that option either. I know we do something similar with approle role settings, and we're not entirely happy with the resulting more complex code and inconsistency.

I will continue thinking about this because I would like to make our engine APIs more consistent with one another, and I want a path forward for existing config APIs which don't support ExistenceCheck. One idea I kind of like is a mount option to toggle the behaviour for engines that currently don't have it. I think we'd have to make it opt-in, initially at least. If you have any other ideas for how to address backwards compatibility while adding patch-style updates to the Consul secrets engine, I'm all ears.

[danieleva]

Hi @ncabatoff,

I agree with you there is no good way to add the feature I need without breaking the current API. The mount option toggle could be a good compromise, it could be even added to all backends to let users decide whether they prefer a put or patch behaviour on the config APIs, with different defaults in order not to break APIs. I'd need some guidance on how to approach the implementation of that. From what I understood so far on the codebase I'd need to:

• add a way to pass a mount options to the backend.
• have the backend to check for said option and set a sensible default.
• in the update function of a config path, check the option and perform the update or fallback to the create function. The mount option should affect only configuration paths, not every path in the backend that implements an ExistenceCheck,so that got me thinking, would it be better to explicit define configuration paths as such, by adding a new SpecialPath to logical.Paths or an extra attribute to framework.Path? In that case a backend developers will be able to explicit declare paths responsible for configuration thus subject to the mount option. The mount option can be parsed into a dedicated type/enum in case there will be more that 2 options in the future, you never know :) This implementation is backward compatible, unless a path is marked as config it won't be affected, giving backend owners time to uptake the feature as they see fit. Please let me know if the above makes any sense :)

[ncabatoff]

Hi @danieleva,

It does make sense, but I haven't got agreement on the team for this approach. I'm going to write a design document articulating our options to try to build a consensus. I will get back to you in the not too distant future, though I'm afraid I can't commit to a time.

[danieleva]

I understand, the proposed changes are not trivial and will span across multiple backends. Hopefully the team will reach an agreement soon, in the meantime I'll put together some workaround on my side so I can continue with my project :)