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.

Sign up for GitHub

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

Add the ability to view and list of leases metadata #2650

Merged
merged 18 commits into from
May 4, 2017
Merged

Add the ability to view and list of leases metadata #2650

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
7ba95c8
Adding viewing and listing of leases
Apr 27, 2017
cc209b1
Merge branch 'master-oss' into read-lease
Apr 27, 2017
e0ba238
cleanup
Apr 27, 2017
68681b6
reorganizing
Apr 27, 2017
093766d
Nesting "lease" related urls under /sys/lease
Apr 27, 2017
2e15776
refactoring shared lease times methods
Apr 27, 2017
65cedd9
reverting path parameter updates
Apr 27, 2017
342bebe
fixing token lookup tests
Apr 27, 2017
ebf24ab
cleaning up method names and tests
Apr 28, 2017
7694708
fixing root path test
Apr 28, 2017
c3bbe55
Updates based on feedback
Apr 29, 2017
585464a
Updating docs, updating and adding tests
Apr 30, 2017
9ac7d8d
Merge branch 'master-oss' into read-lease
Apr 30, 2017
54b6bbc
fixing tests
Apr 30, 2017
426354b
Root protecting the "LIST" operation on /sys/leases/lookup
May 1, 2017
0223fc8
docs updates
May 3, 2017
619cb2e
minor docs updates
May 3, 2017
5577099
root protecting leases/revoke-force, updating docs
May 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 112 additions & 0 deletions vault/logical_system.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ func NewSystemBackend(core *Core, config *logical.BackendConfig) (logical.Backen
"replication/reindex",
"rotate",
"config/auditing/*",
"lease*",
Copy link
Contributor

Choose a reason for hiding this comment

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

Also you'll need to update the root paths test after changing this list

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the head up. I wasn't aware of that test.

},

Unauthenticated: []string{
Expand Down Expand Up @@ -298,6 +299,42 @@ func NewSystemBackend(core *Core, config *logical.BackendConfig) (logical.Backen
HelpDescription: strings.TrimSpace(sysHelp["remount"][1]),
},

&framework.Path{
Pattern: "lease",

Fields: map[string]*framework.FieldSchema{
"lease_id": &framework.FieldSchema{
Type: framework.TypeString,
Description: strings.TrimSpace(sysHelp["lease_id"][0]),
},
},

Callbacks: map[logical.Operation]framework.OperationFunc{
logical.UpdateOperation: b.handleLease,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be Read operation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since lease_id is sensitive, we determined it made sense to pass lease_id as a parameter with an update operation. This is currently the preferred method to use in /sys/renew and /sys/revoke.

},

HelpSynopsis: strings.TrimSpace(sysHelp["lease"][0]),
HelpDescription: strings.TrimSpace(sysHelp["lease"][1]),
},

&framework.Path{
Pattern: "lease" + framework.OptionalParamRegex("prefix"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of making this optional what if the two framework.Paths were merged with both a read op and a list op?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My only concern here was some arbitrary path would not be an "unsupported operation" on update operations because of the optional prefix.


Fields: map[string]*framework.FieldSchema{
"prefix": &framework.FieldSchema{
Type: framework.TypeString,
Description: strings.TrimSpace(sysHelp["lease-list-prefix"][0]),
},
},

Callbacks: map[logical.Operation]framework.OperationFunc{
logical.ListOperation: b.handleLeaseList,
},

HelpSynopsis: strings.TrimSpace(sysHelp["lease-list"][0]),
HelpDescription: strings.TrimSpace(sysHelp["lease-list"][1]),
},

&framework.Path{
Pattern: "renew" + framework.OptionalParamRegex("url_lease_id"),

Expand Down Expand Up @@ -1270,6 +1307,66 @@ func (b *SystemBackend) handleTuneWriteCommon(
return nil, nil
}

// handleLeasse is use to view the metadata for a given LeaseID
Copy link
Member

Choose a reason for hiding this comment

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

s/handleLeasse/handleLeaseLookup

func (b *SystemBackend) handleLease(
req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
leaseID := data.Get("lease_id").(string)

Copy link
Member

Choose a reason for hiding this comment

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

Can we have a check for leaseID == "" here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

leaseTimes, err := b.Core.expiration.FetchLeaseTimes(leaseID)
if err != nil {
b.Backend.Logger().Error("sys: error retrieving lease", "lease_id", leaseID, "error", err)
return handleError(err)
}
if leaseTimes == nil {
return logical.ErrorResponse("invalid lease"), logical.ErrInvalidRequest
}

Copy link
Member

Choose a reason for hiding this comment

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

Can we add a comment here explaining why we are setting values to nil beforehand. This will avoid people removing it in future.

resp := &logical.Response{
Data: map[string]interface{}{
"lease_id": leaseID,
"issue_time": leaseTimes.IssueTime,
"expire_time": nil,
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason for setting these 2 fields explicitly to 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.

This was really just to serialize to null in the serializer. I guess omitting would probably be fine.

"last_renewal_time": nil,
},
}
if !leaseTimes.LastRenewalTime.IsZero() {
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for circling around on this. On a second thought, the idea of having fields with default values (like you were doing earlier) does seem like a sensible thing to do too. What might be of concern is that this API will not have deterministic number of response fields. That is okay I guess but I am not sure if that is a problem. Bringing this up so we can all agree on this and move forward.

resp.Data["last_renewal_time"] = leaseTimes.LastRenewalTime
}
if !leaseTimes.ExpireTime.IsZero() {
resp.Data["expire_time"] = leaseTimes.ExpireTime
}
if leaseTimes.Auth != nil {
resp.Data["renewable"] = leaseTimes.Auth.Renewable
resp.Data["ttl"] = leaseTimes.Auth.TTL
}
if leaseTimes.Secret != nil {
resp.Data["renewable"] = leaseTimes.Secret.Renewable
resp.Data["ttl"] = leaseTimes.Secret.TTL
}

return resp, nil
}

func (b *SystemBackend) handleLeaseList(
req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
prefix := data.Get("prefix").(string)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a check to make sure prefix is not ""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Empty prefix is valid and will list the top level keys.

if !strings.HasSuffix(prefix, "/") {
prefix = prefix + "/"
}

keys, err := b.Core.expiration.idView.List(prefix)
if err != nil {
b.Backend.Logger().Error("sys: error listing leases", "prefix", prefix, "error", err)
return handleError(err)
}

return &logical.Response{
Copy link
Member

Choose a reason for hiding this comment

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

Check logical.ListResponse, which does this for you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Magic :).

Data: map[string]interface{}{
"keys": keys,
},
}, nil
}

// handleRenew is used to renew a lease with a given LeaseID
func (b *SystemBackend) handleRenew(
req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
Expand Down Expand Up @@ -2414,4 +2511,19 @@ This path responds to the following HTTP methods.
"Lists the headers configured to be audited.",
`Returns a list of headers that have been configured to be audited.`,
},

"lease": {
``,
Copy link
Member

Choose a reason for hiding this comment

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

Reminder to fill these out.

``,
},

"lease-list": {
``,
``,
},

"lease-list-prefix": {
`The path to list leases under. Example: "prod/aws/ops"`,
`Returns a list of lease ids.`,
},
}
175 changes: 175 additions & 0 deletions vault/logical_system_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package vault
import (
"crypto/sha256"
"reflect"
"sort"
"strings"
"testing"
"time"
Expand All @@ -11,6 +12,7 @@ import (
"github.com/hashicorp/vault/audit"
"github.com/hashicorp/vault/helper/salt"
"github.com/hashicorp/vault/logical"
"github.com/mitchellh/mapstructure"
)

func TestSystemBackend_RootPaths(t *testing.T) {
Expand Down Expand Up @@ -315,6 +317,179 @@ func TestSystemBackend_remount_system(t *testing.T) {
}
}

func TestSystemBackend_lease(t *testing.T) {
core, b, root := testCoreSystemBackend(t)

// Create a key with a lease
req := logical.TestRequest(t, logical.UpdateOperation, "secret/foo")
req.Data["foo"] = "bar"
req.ClientToken = root
resp, err := core.HandleRequest(req)
if err != nil {
t.Fatalf("err: %v", err)
}
if resp != nil {
t.Fatalf("bad: %#v", resp)
}

// Read a key with a LeaseID
req = logical.TestRequest(t, logical.ReadOperation, "secret/foo")
Copy link
Member

Choose a reason for hiding this comment

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

Generic secret backend does not attach a lease to secrets. I'm not sure if this test would work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does and I was surprised too and this how all the renew and revoke functions are written. I think it might be something in the test harness. Otherwise it isn't straightforward creating a non-token lease without some backing dynamic backend. Maybe pki with generate_lease set true.

Copy link
Member

Choose a reason for hiding this comment

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

When we removed leases from the generic backend in the normal case we still kept the ability to have it issue leases specifically to make testing easier!

req.ClientToken = root
resp, err = core.HandleRequest(req)
if err != nil {
t.Fatalf("err: %v", err)
}
if resp == nil || resp.Secret == nil || resp.Secret.LeaseID == "" {
t.Fatalf("bad: %#v", resp)
}

// Read lease
req = logical.TestRequest(t, logical.UpdateOperation, "lease")
req.Data["lease_id"] = resp.Secret.LeaseID
resp, err = b.HandleRequest(req)
if err != nil {
t.Fatalf("err: %v", err)
}
if resp.Data["renewable"] == nil || resp.Data["renewable"].(bool) {
t.Fatal("generic leases are not renewable")
}

// Invalid lease
req = logical.TestRequest(t, logical.UpdateOperation, "lease")
req.Data["lease_id"] = "invalid"
resp, err = b.HandleRequest(req)
if err != logical.ErrInvalidRequest {
t.Fatalf("expected invalid request, got err: %v", err)
}
}

func TestSystemBackend_lease_list(t *testing.T) {
core, b, root := testCoreSystemBackend(t)

// Create a key with a lease
req := logical.TestRequest(t, logical.UpdateOperation, "secret/foo")
req.Data["foo"] = "bar"
req.ClientToken = root
resp, err := core.HandleRequest(req)
if err != nil {
t.Fatalf("err: %v", err)
}
if resp != nil {
t.Fatalf("bad: %#v", resp)
}

// Read a key with a LeaseID
req = logical.TestRequest(t, logical.ReadOperation, "secret/foo")
Copy link
Member

Choose a reason for hiding this comment

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

Same with this test as well.

req.ClientToken = root
resp, err = core.HandleRequest(req)
if err != nil {
t.Fatalf("err: %v", err)
}
if resp == nil || resp.Secret == nil || resp.Secret.LeaseID == "" {
t.Fatalf("bad: %#v", resp)
}

// List lease
req = logical.TestRequest(t, logical.ListOperation, "lease/secret/foo")
resp, err = b.HandleRequest(req)
if err != nil {
t.Fatalf("err: %v", err)
}
if resp == nil || resp.Data == nil {
t.Fatalf("bad: %#v", resp)
}

var keys []string
if err := mapstructure.WeakDecode(resp.Data["keys"], &keys); err != nil {
t.Fatalf("err: %v", err)
}
if len(keys) != 1 {
t.Fatalf("Expected 1 secret lease, got %d: %#v", len(keys), keys)
}

// Generate multiple leases
req = logical.TestRequest(t, logical.ReadOperation, "secret/foo")
req.ClientToken = root
resp, err = core.HandleRequest(req)
if err != nil {
t.Fatalf("err: %v", err)
}
if resp == nil || resp.Secret == nil || resp.Secret.LeaseID == "" {
t.Fatalf("bad: %#v", resp)
}

req = logical.TestRequest(t, logical.ReadOperation, "secret/foo")
req.ClientToken = root
resp, err = core.HandleRequest(req)
if err != nil {
t.Fatalf("err: %v", err)
}
if resp == nil || resp.Secret == nil || resp.Secret.LeaseID == "" {
t.Fatalf("bad: %#v", resp)
}

req = logical.TestRequest(t, logical.ListOperation, "lease/secret/foo")
resp, err = b.HandleRequest(req)
if err != nil {
t.Fatalf("err: %v", err)
}
if resp == nil || resp.Data == nil {
t.Fatalf("bad: %#v", resp)
}

if err := mapstructure.WeakDecode(resp.Data["keys"], &keys); err != nil {
t.Fatalf("err: %v", err)
}
if len(keys) != 3 {
t.Fatalf("Expected 3 secret lease, got %d: %#v", len(keys), keys)
}

// Listing subkeys
req = logical.TestRequest(t, logical.UpdateOperation, "secret/bar")
req.Data["foo"] = "bar"
req.ClientToken = root
resp, err = core.HandleRequest(req)
if err != nil {
t.Fatalf("err: %v", err)
}
if resp != nil {
t.Fatalf("bad: %#v", resp)
}

// Read a key with a LeaseID
req = logical.TestRequest(t, logical.ReadOperation, "secret/bar")
req.ClientToken = root
resp, err = core.HandleRequest(req)
if err != nil {
t.Fatalf("err: %v", err)
}
if resp == nil || resp.Secret == nil || resp.Secret.LeaseID == "" {
t.Fatalf("bad: %#v", resp)
}

req = logical.TestRequest(t, logical.ListOperation, "lease/secret")
resp, err = b.HandleRequest(req)
if err != nil {
t.Fatalf("err: %v", err)
}
if resp == nil || resp.Data == nil {
t.Fatalf("bad: %#v", resp)
}

var keys2 []string
if err := mapstructure.WeakDecode(resp.Data["keys"], &keys2); err != nil {
t.Fatalf("err: %v", err)
}
if len(keys2) != 2 {
t.Fatalf("Expected 2 secret lease, got %d: %#v", len(keys), keys)
}
expected := []string{"bar/", "foo/"}
sort.Strings(keys2)
if !reflect.DeepEqual(expected, keys2) {
t.Fatalf("exp: %#v, act: %#v", expected, keys2)
}
}

func TestSystemBackend_renew(t *testing.T) {
core, b, root := testCoreSystemBackend(t)

Expand Down