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 10 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
21 changes: 16 additions & 5 deletions vault/expiration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,7 @@ func (m *ExpirationManager) Renew(leaseID string, increment time.Duration) (*log
}

// Check if the lease is renewable
if err := le.renewable(); err != nil {
if err := le.renewableErr(); err != nil {
return nil, err
}

Expand Down Expand Up @@ -450,7 +450,7 @@ func (m *ExpirationManager) RenewToken(req *logical.Request, source string, toke

// Check if the lease is renewable. Note that this also checks for a nil
// lease and errors in that case as well.
if err := le.renewable(); err != nil {
if err := le.renewableErr(); err != nil {
return logical.ErrorResponse(err.Error()), logical.ErrInvalidRequest
}

Expand Down Expand Up @@ -841,11 +841,18 @@ type leaseEntry struct {
}

// encode is used to JSON encode the lease entry
func (l *leaseEntry) encode() ([]byte, error) {
return json.Marshal(l)
func (le *leaseEntry) encode() ([]byte, error) {
return json.Marshal(le)
}

func (le *leaseEntry) renewable() error {
func (le *leaseEntry) renewable() bool {
if err := le.renewableErr(); err == nil {
return true
}
return false
}

func (le *leaseEntry) renewableErr() error {
Copy link
Member

Choose a reason for hiding this comment

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

What is the point of doing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought this was a bit confusing and was just consolidating the logic for dealing with the error, which the expiration manager uses, and returning the value of renewable.

Copy link
Member

Choose a reason for hiding this comment

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

I'm concerned about people defaulting to use this function when in fact the exact error returned can be useful, e.g. in a debugging/logs context.

// If there is no entry, cannot review
if le == nil || le.ExpireTime.IsZero() {
return fmt.Errorf("lease not found or lease is not renewable")
Expand All @@ -866,6 +873,10 @@ func (le *leaseEntry) renewable() error {
return nil
}

func (le *leaseEntry) ttl() int64 {
return int64(le.ExpireTime.Sub(time.Now().Round(time.Second)).Seconds())
}

// decodeLeaseEntry is used to reverse encode and return a new entry
func decodeLeaseEntry(buf []byte) (*leaseEntry, error) {
out := new(leaseEntry)
Expand Down
101 changes: 96 additions & 5 deletions vault/logical_system.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,16 @@ func NewSystemBackend(core *Core, config *logical.BackendConfig) (logical.Backen
Root: []string{
"auth/*",
"remount",
"revoke-prefix/*",
"audit",
"audit/*",
"raw/*",
"replication/primary/secondary-token",
"replication/reindex",
"rotate",
"config/auditing/*",
"lease/lookup*",
Copy link
Member

Choose a reason for hiding this comment

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

Can this be leases not lease? It's a collection so plural makes sense. (As opposed to a singular system like replication, e.g. if it was lease-management, but that's too wordy.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I debated this but wanted to match the other paths that had similar functions, like token and policy. I am not strongly either way and was just trying for consistency.

Copy link
Member

Choose a reason for hiding this comment

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

The difference with policy is that it's supposed to be you acting on a single one, e.g. PUT (write) a policy, GET a policy, and so on. If there were a collection of policy-related APIs under a prefix, having that prefix be policies instead of policy would be the right move IMHO.

Copy link
Member

Choose a reason for hiding this comment

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

Also, lease lookup should definitely not be a root path.

"lease/revoke-prefix/*",
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 also add revoke-force/* and lease/revoke-force/* to 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.

It is probably the right thing to do but this would be a change in behavior. I just want to make sure that what we want to do. I will note it in the changelog if we decide to go that route.

Copy link
Member

Choose a reason for hiding this comment

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

If revoke-prefix is a privileged operation, revoke-force must be I suppose. If we decide to do this, we should document it in CL as you said, in the docs and as a breaking behavior in the upgrade notes.

Copy link
Member

Choose a reason for hiding this comment

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

Generally we reserve root paths for functions that can be super destructive or super sensitive from a security standpoint. revoke-force does fit that bill and probably should be privileged, but I wonder if we should reserve that change for 0.8; people will be more likely to expect breaking changes for that release and I don't think this is super high priority.

"revoke-prefix/*",
},

Unauthenticated: []string{
Expand Down Expand Up @@ -299,7 +301,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.handleLeaseLookup,
logical.ListOperation: b.handleLeaseLookupList,
},

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 +350,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 +372,7 @@ func NewSystemBackend(core *Core, config *logical.BackendConfig) (logical.Backen
},

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

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

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

Fields: map[string]*framework.FieldSchema{
"prefix": &framework.FieldSchema{
Expand Down Expand Up @@ -686,6 +711,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 +1300,56 @@ 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) handleLeaseLookup(
req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
leaseID := data.Get("lease_id").(string)
if leaseID == "" {
return logical.ErrorResponse("lease_id must be specified"),
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 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.

"renewable": leaseTimes.renewable(),
},
}
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
resp.Data["ttl"] = leaseTimes.ttl()
}
return resp, nil
}

func (b *SystemBackend) handleLeaseLookupList(
req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
prefix := data.Get("prefix").(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 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 +2505,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