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

Jump to bottom

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 5 commits
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
138 changes: 116 additions & 22 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 @@ -230,7 +231,18 @@ func NewSystemBackend(core *Core, config *logical.BackendConfig) (logical.Backen
},

&framework.Path{
Pattern: "mounts/(?P<path>.+?)",
Pattern: "mounts$",

Callbacks: map[logical.Operation]framework.OperationFunc{
logical.ReadOperation: b.handleMountTable,
},

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

&framework.Path{
Pattern: "mounts" + framework.OptionalParamRegex("path"),

Fields: map[string]*framework.FieldSchema{
"path": &framework.FieldSchema{
Expand Down Expand Up @@ -265,17 +277,6 @@ func NewSystemBackend(core *Core, config *logical.BackendConfig) (logical.Backen
HelpDescription: strings.TrimSpace(sysHelp["mount"][1]),
},

&framework.Path{
Pattern: "mounts$",

Callbacks: map[logical.Operation]framework.OperationFunc{
logical.ReadOperation: b.handleMountTable,
},

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

&framework.Path{
Pattern: "remount",

Expand All @@ -299,7 +300,30 @@ func NewSystemBackend(core *Core, config *logical.BackendConfig) (logical.Backen
},

&framework.Path{
Pattern: "renew" + framework.OptionalParamRegex("url_lease_id"),
Pattern: "lease/lookup" + framework.OptionalParamRegex("prefix"),

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

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

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

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

Fields: map[string]*framework.FieldSchema{
"url_lease_id": &framework.FieldSchema{
Expand All @@ -325,7 +349,7 @@ func NewSystemBackend(core *Core, config *logical.BackendConfig) (logical.Backen
},

&framework.Path{
Pattern: "revoke" + framework.OptionalParamRegex("url_lease_id"),
Pattern: "(lease/)?revoke" + framework.OptionalParamRegex("url_lease_id"),

Fields: map[string]*framework.FieldSchema{
"url_lease_id": &framework.FieldSchema{
Expand All @@ -347,7 +371,7 @@ func NewSystemBackend(core *Core, config *logical.BackendConfig) (logical.Backen
},

&framework.Path{
Pattern: "revoke-force/(?P<prefix>.+)",
Pattern: "(lease/)?revoke-force" + framework.OptionalParamRegex("prefix"),
Copy link
Member

Choose a reason for hiding this comment

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

Before this change, I think the prefix parameter was mandatory and would always be set. If it was not supplied, parsing the path pattern would error out. Now that it is an optional param, missing prefix I think will not error out. We might want to check explicitly that the required parameters (but optionally set in the URL) are in fact set. This comment applies to all the similar changes below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. I got a little ambitious in my refactoring. I'll revert out these changes.


Fields: map[string]*framework.FieldSchema{
"prefix": &framework.FieldSchema{
Expand All @@ -365,7 +389,7 @@ func NewSystemBackend(core *Core, config *logical.BackendConfig) (logical.Backen
},

&framework.Path{
Pattern: "revoke-prefix/(?P<prefix>.+)",
Pattern: "(lease/)?revoke-prefix" + framework.OptionalParamRegex("prefix"),

Fields: map[string]*framework.FieldSchema{
"prefix": &framework.FieldSchema{
Expand Down Expand Up @@ -394,7 +418,7 @@ func NewSystemBackend(core *Core, config *logical.BackendConfig) (logical.Backen
},

&framework.Path{
Pattern: "auth/(?P<path>.+)",
Pattern: "auth" + framework.OptionalParamRegex("path"),

Fields: map[string]*framework.FieldSchema{
"path": &framework.FieldSchema{
Expand Down Expand Up @@ -438,7 +462,7 @@ func NewSystemBackend(core *Core, config *logical.BackendConfig) (logical.Backen
},

&framework.Path{
Pattern: "policy/(?P<name>.+)",
Pattern: "policy" + framework.OptionalParamRegex("name"),

Fields: map[string]*framework.FieldSchema{
"name": &framework.FieldSchema{
Expand Down Expand Up @@ -480,7 +504,7 @@ func NewSystemBackend(core *Core, config *logical.BackendConfig) (logical.Backen
},

&framework.Path{
Pattern: "audit-hash/(?P<path>.+)",
Pattern: "audit-hash" + framework.OptionalParamRegex("path"),

Fields: map[string]*framework.FieldSchema{
"path": &framework.FieldSchema{
Expand Down Expand Up @@ -513,7 +537,7 @@ func NewSystemBackend(core *Core, config *logical.BackendConfig) (logical.Backen
},

&framework.Path{
Pattern: "audit/(?P<path>.+)",
Pattern: "audit" + framework.OptionalParamRegex("path"),

Fields: map[string]*framework.FieldSchema{
"path": &framework.FieldSchema{
Expand Down Expand Up @@ -549,7 +573,7 @@ func NewSystemBackend(core *Core, config *logical.BackendConfig) (logical.Backen
},

&framework.Path{
Pattern: "raw/(?P<path>.+)",
Pattern: "raw" + framework.OptionalParamRegex("path"),

Fields: map[string]*framework.FieldSchema{
"path": &framework.FieldSchema{
Expand Down Expand Up @@ -666,7 +690,7 @@ func NewSystemBackend(core *Core, config *logical.BackendConfig) (logical.Backen
},

&framework.Path{
Pattern: "config/auditing/request-headers/(?P<header>.+)",
Pattern: "config/auditing/request-headers" + framework.OptionalParamRegex("header"),

Fields: map[string]*framework.FieldSchema{
"header": &framework.FieldSchema{
Expand All @@ -686,6 +710,7 @@ func NewSystemBackend(core *Core, config *logical.BackendConfig) (logical.Backen
HelpSynopsis: strings.TrimSpace(sysHelp["audited-headers-name"][0]),
HelpDescription: strings.TrimSpace(sysHelp["audited-headers-name"][1]),
},

&framework.Path{
Pattern: "config/auditing/request-headers$",

Expand Down Expand Up @@ -1274,6 +1299,60 @@ 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{}{
"id": leaseID,
"creation_time": leaseTimes.IssueTime,
Copy link
Member

Choose a reason for hiding this comment

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

This has a different type than creation_time in the token store; there, you set this to issue_time.

Copy link
Member

Choose a reason for hiding this comment

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

There's a type discrepancy with last_renewal_time too.

},
}
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.ListResponse(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 @@ -2429,4 +2508,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.`,
},
}
Loading