Transit: Key Trim #5388
Transit: Key Trim #5388
Conversation
| originalExportable := p.Exportable | ||
| originalAllowPlaintextBackup := p.AllowPlaintextBackup | ||
|
|
||
| defer func() { |
There was a problem hiding this comment.
I guess the intent is that p.Persist in the return restores the old values. Does this defer approach work? Won't the updates to p take place after the Persist call runs? Or did you intend to call Persist in this function? Maybe one of the test case demonstrates this but it wasn't obvious which one.
There was a problem hiding this comment.
The policy is responded from the cache. The pointer to the policy is being modified in this function. Even if this function errors out, the policy in the cache gets modified accidentally. This is even before the p.Persist is being called. p.Persist however is already doing the right thing. So, this function is only trying to restore values in policy that it is touching.
There was a problem hiding this comment.
Ah, didn't notice that p is pointing into the cache. Makes sense then.
There was a problem hiding this comment.
I think this needs to be rethought a bit. It doesn't make sense to me to put a trimmed version in config, because the UX feels pretty weird, like, set this config flag which passively takes action, and then the config flag sticks around but doesn't really have meaning anymore.
I think the right approach is likely to create a trim path, maybe keys/<name>/trim that performs the trim in real time, and that a property is added to the key policy indicating the last trimmed version or min available version or so so that logic for archiving and such can know when to stop.
|
@jefferai The functionality is moved to its own endpoint now. |
builtin/logical/transit/path_trim.go
Outdated
| return logical.ErrorResponse("minimum version cannot be negative"), nil | ||
| case p.MinVersion == 0: | ||
| return logical.ErrorResponse("minimum version should be positive"), nil | ||
| case p.MinVersion < p.MinVersion: |
There was a problem hiding this comment.
I don't think this will ever be true
There was a problem hiding this comment.
Ah, right. This was a refactoring issue. Fixed.
builtin/logical/transit/path_trim.go
Outdated
| } | ||
| }() | ||
|
|
||
| p.MinVersion = minVersionRaw.(int) |
There was a problem hiding this comment.
Why not set this after all the validation logic below? We could avoid the defer above if we do it that way and call Persist outside of the return function. e.g.
originalMinVersion := p.MinVersion
p.MinVersion = minVersionRaw.(int)
if err := p.Persist(ctx, req.Storage); err != nil {
p.MinVersion = originalMinVersion
return nil, err
}
return nil, nil
builtin/logical/transit/path_keys.go
Outdated
| @@ -206,6 +206,7 @@ func (b *backend) pathPolicyRead(ctx context.Context, req *logical.Request, d *f | |||
| "type": p.Type.String(), | |||
| "derived": p.Derived, | |||
| "deletion_allowed": p.DeletionAllowed, | |||
| "min_version": p.MinVersion, | |||
There was a problem hiding this comment.
I don't really like the name, I think it's confusing next to min_decryption_version and min_encryption_version. Slightly better might be min_available_version, although that mostly has the same problem. Possibly first_available_version?
There was a problem hiding this comment.
min_available_version is better I suppose. first_available_version is slightly confusing.
| @@ -326,6 +326,13 @@ type Policy struct { | |||
| // a max. | |||
| ArchiveVersion int `json:"archive_version"` | |||
|
|
|||
| // ArchiveMinVersion is the minimum version of the key in the archive. | |||
| ArchiveMinVersion int `json:"archive_min_version"` | |||
There was a problem hiding this comment.
This seems redundant. Archiving is simply a factor of the min decryption version. So if the min decryption version is the same as the min version (or, first available version), nothing is in the archive; otherwise, the first version in the archive is the same as min version.
There was a problem hiding this comment.
I don't feel that this is redundant (unless I am failing to see your point). The policy that invokes handleArchiving might already have MinAvailableVersion modified. So, we can't rely on that. We want the last used MinAvailableVersion, which is held by ArchiveMinVersion.
|
Looking good! Will defer to Brian or Chris for final 👍 |
No description provided.