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
Member 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.MinAvailableVersion > p.MinEncryptionVersion:
return logical.ErrorResponse("min encryption version should not be less than min available version"), nil
case p.MinAvailableVersion > p.MinDecryptionVersion:
return logical.ErrorResponse("min decryption version should not be less then min available 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_available_version": p.MinAvailableVersion,
"min_decryption_version": p.MinDecryptionVersion,
"min_encryption_version": p.MinEncryptionVersion,
"latest_version": p.LatestVersion,
Expand Down
99 changes: 99 additions & 0 deletions builtin/logical/transit/path_trim.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
package transit

import (
"context"

"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_available_version": &framework.FieldSchema{
Type: framework.TypeInt,
Description: `
The minimum available 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()

minAvailableVersionRaw, ok := d.GetOk("min_available_version")
if !ok {
return logical.ErrorResponse("missing min_available_version"), nil
}
minAvailableVersion := minAvailableVersionRaw.(int)

originalMinAvailableVersion := p.MinAvailableVersion

switch {
case minAvailableVersion < originalMinAvailableVersion:
return logical.ErrorResponse("minimum available version cannot be decremented"), nil
case p.MinEncryptionVersion == 0:
return logical.ErrorResponse("minimum available version cannot be set when minimum encryption version is not set"), nil
case p.MinDecryptionVersion == 0:
return logical.ErrorResponse("minimum available version cannot be set when minimum decryption version is not set"), nil
case minAvailableVersion > p.MinEncryptionVersion:
return logical.ErrorResponse("minimum available version cannot be greater than minmum encryption version"), nil
case minAvailableVersion > p.MinDecryptionVersion:
return logical.ErrorResponse("minimum available version cannot be greater than minimum decryption version"), nil
case minAvailableVersion < 0:
return logical.ErrorResponse("minimum available version cannot be negative"), nil
case minAvailableVersion == 0:
return logical.ErrorResponse("minimum available version should be positive"), nil
}

// Ensure that cache doesn't get corrupted in error cases
p.MinAvailableVersion = minAvailableVersion
if err := p.Persist(ctx, req.Storage); err != nil {
p.MinAvailableVersion = originalMinAvailableVersion
return nil, err
}

return nil, nil
}
}

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