Add SDK helpers and Core stubs for plugins to communicate with Enterprise Rotation Manager #29273
Conversation
github-actions
bot
added
the
hashicorp-contributed-pr
|
CI Results: |
|
Build Results: |
| return logical.ErrorResponse("error registering rotation job: %s", err), nil | ||
| } | ||
|
|
||
| rc.RotationID = rotationID |
There was a problem hiding this comment.
When the backend is reloaded, will this value be overwritten with a new value? Should we check if the mount's root rotation job has already been registered before registering?
There was a problem hiding this comment.
Good catch, don't think we accounted for this yet.
There was a problem hiding this comment.
This is a good question. Like Robert said, we haven't worked this out yet, but one thought I had was the following:
We can have the RM keep track of each Rotation ID in the form of a list. The structure of Rotation IDs encodes the Request Path as well. Before registering a root credential, we can check the incoming Rotation Job's ReqPath, and if that Path has already been stored in our Rotation IDs list, we can avoid registering and log a warning — stating the user needs to delete a rotation job before re-registering another for the same mount.
Can sketch out some ideas to enable tracking IDs in the case where the plugin crashes/reloads, and will put that up as part of the Enterprise PR 👍🏼
There was a problem hiding this comment.
The structure of Rotation IDs encodes the Request Path as well
Sorry to keep coming back to this, but if we are already doing this in Vault Core, why are we requiring plugins to store this mapping as well?
| RotationTTL int `json:"rotation_ttl"` | ||
|
|
||
| // RotationID is the unique ID of the registered rotation job. | ||
| // Used by the plugin to track the rotation. |
There was a problem hiding this comment.
I think we should add more guidance on the care plugin authors must take with this value.
"For static roles, plugin authors must maintain and store a mapping of role path to rotation ID. This should then be used to..."
There was a problem hiding this comment.
Good callout! We can note this down as an item to address as part of our Plugin Developer Facing Docs
| } | ||
|
|
||
| b.Logger().Debug("Registering rotation job", "mount", req.MountPoint+req.Path) | ||
| rotationID, err := b.System().RegisterRotationJob(ctx, rotationJob) |
There was a problem hiding this comment.
How and when does a rotation job get deregistered?
There was a problem hiding this comment.
Good call out, not sure if we have yet discussed the idea of deregistering rotation. Is it something we should allow? I wonder if there's any differences between allowing automated rotation to be disabled versus a pause mechanism meant only for emergencies?
There was a problem hiding this comment.
De-registration / off-boarding should be allowed IMO, if only as a disabling escape hatch. Added a method on the SystemView to Deregister a rotation job. This will be triggered if a boolean disable_automated_rotation (subject to change) is set to true on the config. Example of this in AWS is linked here
There was a problem hiding this comment.
I was thinking about the disable and delete actions. Without deregistration, how do we stop rotations when a mount is disabled or a static role is deleted that have been registered with the Rotation Manager?
There was a problem hiding this comment.
Ah I see, that's a good question! So this won't be visible in this PR, but the Enterprise Rotation Manager implementation does store the request path that a credential comes from. The RM also has access to core.Router, and I believe we should be able to use MatchingMountEntry to determine if a given mount exists.
This will be performed on the Enterprise Rotation Manager code (before it routes the request to the plugin backend, it can confirm that a matching mount entry exists for handling the request). I can make sure to test that flow, and call it out in the Enterprise PR once it is opened (after this PR is merged in so that CI in Enterprise is happy)
There was a problem hiding this comment.
it can confirm that a matching mount entry exists for handling the request
I think we will want to deregister a job when the backend/role is disabled or deleted, right?
Co-authored-by: John-Michael Faircloth <[email protected]>
Co-authored-by: John-Michael Faircloth <[email protected]>
Description
Adds helpers for all plugins to communicate with the upcoming Rotation Manager (Enterprise-only) feature. This PR adds the helper methods and RPC interfaces that will allow all built-in and external plugins to talk to the Rotation Manager once it exists.
This PR also adds helpers to create Rotation Job structures easily with the help of methods in
sdk/logical/rotation_job.goand parse Rotation Schedules with a common helper library insdk/logical/schedule.go.Note: This PR will not implement the Rotation Manager. Attempting to use the feature will result in a
not implemented in CEerror (from CE Stubs)An example of how these helpers can be used by plugins is provided with the builtin AWS Secrets plugin. A test with AWS secrets also confirms that the feature is not implemented on CE.