Skip to content
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

Merged
merged 14 commits into from
Oct 17, 2018
1 change: 1 addition & 0 deletions builtin/logical/transit/backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ func Backend(conf *logical.BackendConfig) *backend {
b.pathVerify(),
b.pathBackup(),
b.pathRestore(),
b.pathTrim(),
},

Secrets: []*framework.Secret{},
Expand Down
27 changes: 25 additions & 2 deletions builtin/logical/transit/path_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ the latest version of the key is allowed.`,
}
}

func (b *backend) pathConfigWrite(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
func (b *backend) pathConfigWrite(ctx context.Context, req *logical.Request, d *framework.FieldData) (resp *logical.Response, retErr error) {
name := d.Get("name").(string)

// Check if the policy already exists before we lock everything
Expand All @@ -79,7 +79,23 @@ func (b *backend) pathConfigWrite(ctx context.Context, req *logical.Request, d *
}
defer p.Unlock()

resp := &logical.Response{}
originalMinDecryptionVersion := p.MinDecryptionVersion
originalMinEncryptionVersion := p.MinEncryptionVersion
originalDeletionAllowed := p.DeletionAllowed
originalExportable := p.Exportable
originalAllowPlaintextBackup := p.AllowPlaintextBackup

defer func() {
Copy link
Contributor

@kalafut kalafut Sep 25, 2018

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 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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

if retErr != nil || (resp != nil && resp.IsError()) {
p.MinDecryptionVersion = originalMinDecryptionVersion
p.MinEncryptionVersion = originalMinEncryptionVersion
p.DeletionAllowed = originalDeletionAllowed
p.Exportable = originalExportable
p.AllowPlaintextBackup = originalAllowPlaintextBackup
}
}()

resp = &logical.Response{}

persistNeeded := false

Expand Down Expand Up @@ -173,6 +189,13 @@ func (b *backend) pathConfigWrite(ctx context.Context, req *logical.Request, d *
return nil, nil
}

switch {
case p.MinVersion > p.MinEncryptionVersion:
return logical.ErrorResponse("min encryption version should not be less than min version"), nil
case p.MinVersion > p.MinDecryptionVersion:
return logical.ErrorResponse("min decryption version should not be less then min version"), nil
}

if len(resp.Warnings) == 0 {
return nil, p.Persist(ctx, req.Storage)
}
Expand Down
2 changes: 1 addition & 1 deletion builtin/logical/transit/path_config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ func TestTransit_ConfigSettings(t *testing.T) {

doReq := func(req *logical.Request) *logical.Response {
resp, err := b.HandleRequest(context.Background(), req)
if err != nil {
if err != nil || (resp != nil && resp.IsError()) {
t.Fatalf("got err:\n%#v\nreq:\n%#v\n", err, *req)
}
return resp
Expand Down
1 change: 1 addition & 0 deletions builtin/logical/transit/path_keys.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

min_available_version is better I suppose. first_available_version is slightly confusing.

"min_decryption_version": p.MinDecryptionVersion,
"min_encryption_version": p.MinEncryptionVersion,
"latest_version": p.LatestVersion,
Expand Down
101 changes: 101 additions & 0 deletions builtin/logical/transit/path_trim.go
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)
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this will ever be true

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
`
Loading