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

chrishoffman · 2017-04-27T14:45:51Z

This adds the ability to view and list lease metadata. There are currently no methods to view lease metadata or to list leases by a given prefix. Since there are methods that exist to revoke individual or a prefix of leases and renew individual leases, it makes sense to have endpoints that allow you to view this data as well. The data that will be returned will only be metadata about the lease and will not return any internal lease data.

jefferai · 2017-04-27T17:59:38Z

vault/logical_system.go

+		return handleError(err)
+	}
+
+	return &logical.Response{


Check logical.ListResponse, which does this for you!

vishalnayak · 2017-04-27T18:05:27Z

vault/logical_system.go

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


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

vishalnayak · 2017-04-27T18:07:52Z

vault/logical_system.go

+		Data: map[string]interface{}{
+			"lease_id":          leaseID,
+			"issue_time":        leaseTimes.IssueTime,
+			"expire_time":       nil,


Is there a reason for setting these 2 fields explicitly to nil?

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

vishalnayak · 2017-04-27T18:13:23Z

vault/logical_system_test.go

+	}
+
+	// Read a key with a LeaseID
+	req = logical.TestRequest(t, logical.ReadOperation, "secret/foo")


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

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.

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!

vishalnayak · 2017-04-27T18:14:10Z

vault/logical_system_test.go

+	}
+
+	// Read a key with a LeaseID
+	req = logical.TestRequest(t, logical.ReadOperation, "secret/foo")


Same with this test as well.

briankassouf · 2017-04-27T17:51:19Z

vault/logical_system.go

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


Should this be Read operation?

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.

briankassouf · 2017-04-27T18:29:59Z

vault/logical_system.go

+			},
+
+			&framework.Path{
+				Pattern: "lease" + framework.OptionalParamRegex("prefix"),


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

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

briankassouf · 2017-04-27T18:30:42Z

vault/logical_system.go

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


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

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

briankassouf · 2017-04-27T18:33:26Z

vault/logical_system.go

@@ -63,6 +63,7 @@ func NewSystemBackend(core *Core, config *logical.BackendConfig) (logical.Backen
 				"replication/reindex",
 				"rotate",
 				"config/auditing/*",
+				"lease*",


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

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

vishalnayak · 2017-04-27T20:53:42Z

vault/logical_system.go

@@ -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"),


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.

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

vishalnayak · 2017-04-28T11:58:02Z

vault/logical_system.go

 				"audit",
 				"audit/*",
 				"raw/*",
 				"replication/primary/secondary-token",
 				"replication/reindex",
 				"rotate",
 				"config/auditing/*",
+				"lease/lookup*",
+				"lease/revoke-prefix/*",


Can we also add revoke-force/* and lease/revoke-force/* to this list?

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.

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.

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.

vishalnayak · 2017-04-28T11:59:01Z

vault/logical_system.go

@@ -1274,6 +1300,56 @@ func (b *SystemBackend) handleTuneWriteCommon(
 	return nil, nil
 }

+// handleLeasse is use to view the metadata for a given LeaseID


s/handleLeasse/handleLeaseLookup

vishalnayak · 2017-04-28T12:06:37Z

vault/logical_system.go

+			"renewable":     leaseTimes.renewable(),
+		},
+	}
+	if !leaseTimes.LastRenewalTime.IsZero() {


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.

vishalnayak · 2017-04-28T12:08:18Z

vault/logical_system.go

@@ -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": {
+		``,


Reminder to fill these out.

jefferai · 2017-04-28T12:46:15Z

vault/logical_system.go

 				"audit",
 				"audit/*",
 				"raw/*",
 				"replication/primary/secondary-token",
 				"replication/reindex",
 				"rotate",
 				"config/auditing/*",
+				"lease/lookup*",


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

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.

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.

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

jefferai · 2017-04-28T12:47:34Z

vault/expiration.go

+	return false
+}
+
+func (le *leaseEntry) renewableErr() error {


What is the point of doing this?

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.

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.

jefferai · 2017-04-28T12:52:04Z

vault/logical_system.go

+	resp := &logical.Response{
+		Data: map[string]interface{}{
+			"id":            leaseID,
+			"creation_time": leaseTimes.IssueTime,


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

There's a type discrepancy with last_renewal_time too.

chrishoffman · 2017-05-01T13:52:10Z

This should be ready for review again.

Notable changes since last full review:

Added documentation
Nested "lease" related paths under /sys/leases. Backwards compatibility is maintained.
Token lookup and lease lookup fields merged

I ended up moving a lot of documentation around. Should we provide landing pages for the old docs locations that point to the new locations?

jefferai

Looks good! Please just make sys/leases/revoke-force a sudo operation (and document that) and it's good to go IMHO.

briankassouf · 2017-05-03T22:08:22Z

vault/logical_system.go

@@ -325,7 +363,7 @@ func NewSystemBackend(core *Core, config *logical.BackendConfig) (logical.Backen
 			},

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


vishalnayak · 2017-05-03T22:20:25Z

vault/logical_system.go

@@ -1274,6 +1314,61 @@ func (b *SystemBackend) handleTuneWriteCommon(
 	return nil, nil
 }

+// handleLease is use to view the metadata for a given LeaseID


s/handleLease/handleLeaseLookup

vishalnayak · 2017-05-03T22:21:45Z

vault/logical_system.go

+	if leaseTimes == nil {
+		return logical.ErrorResponse("invalid lease"), logical.ErrInvalidRequest
+	}
+


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

briankassouf · 2017-05-03T22:32:04Z

vault/logical_system.go

+			},
+
+			&framework.Path{
+				Pattern: "leases/lookup",


Do we want this to be a root protected path?

@briankassouf Since lease/lookup doesn't return sensitive information (e.g. the contents of internal data) and you have to know the lease ID ahead of time I don't think it needs to be root. In fact, I am actually thinking we may want to add it to the default policy. Listing on the other hand does divulge lease IDs which can be used to e.g. renew them, so that should be root protected.

briankassouf

Just that one comment about leases/lookup being a root path but other that that LGTM!!

vishalnayak

LGTM! You might want to update the upgrade notes with the revoke-force change. I think its okay to have a breaking change in 0.7.1 considering that its a very low impact break.

jefferai · 2017-05-04T00:09:57Z

@vishalnayak it's not a breaking change; sys/revoke-force remains non-root (for now), but sys/leases/revoke-force is root. Since that's what documentation will focus on we'll get people used to using that, then eventually can change over the older path.

Adding viewing and listing of leases

7ba95c8

chrishoffman requested review from jefferai and briankassouf April 27, 2017 14:45

jefferai added this to the 0.7.1 milestone Apr 27, 2017

jefferai reviewed Apr 27, 2017

View reviewed changes

vishalnayak reviewed Apr 27, 2017

View reviewed changes

briankassouf requested changes Apr 27, 2017

View reviewed changes

briankassouf reviewed Apr 27, 2017

View reviewed changes

Chris Hoffman added 4 commits April 27, 2017 14:49

Merge branch 'master-oss' into read-lease

cc209b1

cleanup

e0ba238

reorganizing

68681b6

Nesting "lease" related urls under /sys/lease

093766d

vishalnayak reviewed Apr 27, 2017

View reviewed changes

Chris Hoffman added 5 commits April 27, 2017 17:41

refactoring shared lease times methods

2e15776

reverting path parameter updates

65cedd9

fixing token lookup tests

342bebe

cleaning up method names and tests

ebf24ab

fixing root path test

7694708

vishalnayak reviewed Apr 28, 2017

View reviewed changes

jefferai reviewed Apr 28, 2017

View reviewed changes

Chris Hoffman added 4 commits April 29, 2017 09:12

Updates based on feedback

c3bbe55

Updating docs, updating and adding tests

585464a

Merge branch 'master-oss' into read-lease

9ac7d8d

fixing tests

54b6bbc

chrishoffman force-pushed the read-lease branch from cd314fa to 54b6bbc Compare April 30, 2017 20:57

Root protecting the "LIST" operation on /sys/leases/lookup

426354b

jefferai modified the milestones: 0.7.1, 0.7.2 May 2, 2017

Chris Hoffman added 2 commits May 3, 2017 16:55

docs updates

0223fc8

minor docs updates

619cb2e

jefferai approved these changes May 3, 2017

View reviewed changes

root protecting leases/revoke-force, updating docs

5577099

briankassouf reviewed May 3, 2017

View reviewed changes

vishalnayak reviewed May 3, 2017

View reviewed changes

briankassouf reviewed May 3, 2017

View reviewed changes

briankassouf approved these changes May 3, 2017

View reviewed changes

vishalnayak approved these changes May 3, 2017

View reviewed changes

chrishoffman merged commit 4490e93 into master May 4, 2017

chrishoffman deleted the read-lease branch May 4, 2017 02:09

chrishoffman mentioned this pull request May 4, 2017

MySQL secret backend: Ability to list generated credentials #2585

Closed

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

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

Conversation

chrishoffman commented Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrishoffman commented May 1, 2017

jefferai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briankassouf left a comment

Choose a reason for hiding this comment

vishalnayak left a comment

Choose a reason for hiding this comment

jefferai commented May 4, 2017

chrishoffman commented Apr 27, 2017 •

edited

Loading