-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Transit: Key Trim #5388
Transit: Key Trim #5388
Changes from 10 commits
d820d1f
c041bb0
07689b1
e9147fb
0bfd5fc
fb43329
e472dff
f3195a2
4dae783
341ad43
24589c0
6687dcb
f09c5fc
8cbbc09
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
"min_decryption_version": p.MinDecryptionVersion, | ||
"min_encryption_version": p.MinEncryptionVersion, | ||
"latest_version": p.LatestVersion, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,101 @@ | ||
package transit | ||
|
||
import ( | ||
"context" | ||
"fmt" | ||
|
||
"github.com/hashicorp/vault/helper/keysutil" | ||
"github.com/hashicorp/vault/logical" | ||
"github.com/hashicorp/vault/logical/framework" | ||
) | ||
|
||
func (b *backend) pathTrim() *framework.Path { | ||
return &framework.Path{ | ||
Pattern: "keys/" + framework.GenericNameRegex("name") + "/trim", | ||
Fields: map[string]*framework.FieldSchema{ | ||
"name": &framework.FieldSchema{ | ||
Type: framework.TypeString, | ||
Description: "Name of the key", | ||
}, | ||
"min_version": &framework.FieldSchema{ | ||
Type: framework.TypeInt, | ||
Description: `The minimum version for the key ring. All versions before this version will be | ||
permanently deleted. This value can at most be equal to the lesser of | ||
'min_decryption_version' and 'min_encryption_version'. This is not allowed to | ||
be set when either 'min_encryption_version' or 'min_decryption_version' is set | ||
to zero.`, | ||
}, | ||
}, | ||
|
||
Callbacks: map[logical.Operation]framework.OperationFunc{ | ||
logical.UpdateOperation: b.pathTrimUpdate(), | ||
}, | ||
|
||
HelpSynopsis: pathTrimHelpSyn, | ||
HelpDescription: pathTrimHelpDesc, | ||
} | ||
} | ||
|
||
func (b *backend) pathTrimUpdate() framework.OperationFunc { | ||
return func(ctx context.Context, req *logical.Request, d *framework.FieldData) (resp *logical.Response, retErr error) { | ||
name := d.Get("name").(string) | ||
|
||
p, _, err := b.lm.GetPolicy(ctx, keysutil.PolicyRequest{ | ||
Storage: req.Storage, | ||
Name: name, | ||
}) | ||
if err != nil { | ||
return nil, err | ||
} | ||
if p == nil { | ||
return logical.ErrorResponse("invalid key name"), logical.ErrInvalidRequest | ||
} | ||
if !b.System().CachingDisabled() { | ||
p.Lock(true) | ||
} | ||
defer p.Unlock() | ||
|
||
minVersionRaw, ok := d.GetOk("min_version") | ||
if !ok { | ||
return logical.ErrorResponse("missing min_version"), nil | ||
} | ||
|
||
// Ensure that cache doesn't get corrupted in error cases | ||
originalMinVersion := p.MinVersion | ||
defer func() { | ||
if retErr != nil || (resp != nil && resp.IsError()) { | ||
p.MinVersion = originalMinVersion | ||
} | ||
}() | ||
|
||
p.MinVersion = minVersionRaw.(int) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
|
||
switch { | ||
case p.MinVersion < originalMinVersion: | ||
return logical.ErrorResponse("minimum version cannot be decremented"), nil | ||
case p.MinEncryptionVersion == 0: | ||
return logical.ErrorResponse("minimum version cannot be set when minimum encryption version is not set"), nil | ||
case p.MinDecryptionVersion == 0: | ||
return logical.ErrorResponse("minimum version cannot be set when minimum decryption version is not set"), nil | ||
case p.MinVersion > p.MinEncryptionVersion: | ||
return logical.ErrorResponse("minimum version cannot be greater than minmum encryption version"), nil | ||
case p.MinVersion > p.MinDecryptionVersion: | ||
return logical.ErrorResponse("minimum version cannot be greater than minimum decryption version"), nil | ||
case p.MinVersion < 0: | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this will ever be true There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, right. This was a refactoring issue. Fixed. |
||
return logical.ErrorResponse(fmt.Sprintf("minimum version cannot be less than the already set value of %d", p.MinVersion)), nil | ||
} | ||
|
||
return nil, p.Persist(ctx, req.Storage) | ||
} | ||
} | ||
|
||
const pathTrimHelpSyn = `Trim key versions of a named key` | ||
|
||
const pathTrimHelpDesc = ` | ||
This path is used to trim key versions of a named key. Trimming only happens | ||
from the lower end of version numbers. | ||
` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the intent is that
p.Persist
in thereturn
restores the old values. Does thisdefer
approach work? Won't the updates top
take place after thePersist
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, didn't notice that
p
is pointing into the cache. Makes sense then.