Cleaning of stale leases #2452

vishalnayak · 2017-03-07T20:53:42Z

No description provided.

jefferai · 2017-04-04T16:08:15Z

I'm not sure what else was earmarked to be done here; one definite comment is there needs to be a guard so that you don't run the tidy operation if it's already running.

jefferai · 2017-04-26T18:01:03Z

vault/expiration.go

+
+		lock := locksutil.LockForKey(m.tokenStore.tokenLocks, le.ClientToken)
+		lock.RLock()
+		defer lock.RUnlock()


This needs to be given up after the lookupSalted call or if we are trying to revoke a token it'll deadlock.

jefferai · 2017-04-26T18:22:17Z

vault/expiration.go

+func (m *ExpirationManager) Tidy() error {
+	var tidyErrors *multierror.Error
+
+	tidyFunc := func(leaseID string) {


Please add some progress logging so people can be aware things are still happening.

jefferai · 2017-04-26T18:22:57Z

vault/expiration.go

+
+	tidyFunc := func(leaseID string) {
+		le, err := m.loadEntry(leaseID)
+		if err != nil {


I think it's worth adding a memoizing cache (a map would work, as a set) so that if we already know a token does still exist we don't have to look it up again.

jefferai · 2017-04-26T18:23:14Z

vault/expiration.go

+			return
+		}
+
+		if le.ClientToken == "" {


In this case we should probably revoke the lease as well.

jefferai · 2017-04-27T14:34:47Z

vault/expiration.go

+	tokenCache := make(map[string]string)
+
+	tidyFunc := func(leaseID string) {
+		m.logger.Trace("expiration: checking if lease is valid", "lease_id", leaseID)


This is likely to be way too verbose, since this will print out for every single lease -- imagine if you have 200k of them. What would be better is a log line before this happens, then a running count, e.g. if the current count is divisible evenly by 500, print out an update line.

jefferai · 2017-04-27T14:39:28Z

vault/expiration.go

+	var tidyErrors *multierror.Error
+
+	// Create a cache to keep track of looked up tokens
+	tokenCache := make(map[string]string)


You don't need to store all tokens you have seen; you only need to store a list of valid tokens so that you don't check for validity again. So treat it as a set via map[string]struct{}.

The reason for doing this way is that, if we looked up a token and if that had turned out to be invalid, we need that information too. In comparison to the valid tokens, I assume that the number of invalid ones will be fairly less in number. While cleaning up, it is likely that we hit the same invalid token more than once. Caching information about a looked up invalid token might be of help.

I am okay to do the suggested changes. Just sharing my thoughts.

The hit for an invalid entry isn't as high, because there won't be any value to decode/de-serialize. But memoizing both statues is fine -- however, you don't need a string, you just need a bool. Presence of the entry in the map means it's been seen; the bool indicates whether it was valid or not.

jefferai · 2017-04-27T14:40:30Z

vault/expiration.go

+			revokeLease = true
+		}
+
+		switch tokenCache[le.ClientToken] {


Don't do this; simply do an if _, ok := validTokenCache; ok { return

jefferai · 2017-04-27T14:41:59Z

vault/token_store.go

@@ -102,14 +107,11 @@ func NewTokenStore(c *Core, config *logical.BackendConfig) (*TokenStore, error)
 	t := &TokenStore{
 		view:               view,
 		cubbyholeDestroyer: destroyCubbyhole,
+		logger:             c.logger,
+		policyLookupFunc:   c.policyStore.GetPolicy,


By doing this here you are skipping the check for nil; IIRC this check is there on purpose for some test reasons.

jefferai · 2017-04-27T15:26:58Z

vault/expiration.go

@@ -124,7 +124,7 @@ func (m *ExpirationManager) Tidy() error {
 	var tidyErrors *multierror.Error

 	// Create a cache to keep track of looked up tokens
-	tokenCache := make(map[string]string)
+	validTokenCache := make(map[string]struct{})


Why not make this a bool and cache discovery of both valid and invalid?

Done. Had seen your previous comment after making this struct{} :-)

jefferai · 2017-04-27T16:50:14Z

vault/expiration.go

+
+	// If no errors were encountered, return a normal error instead of a
+	// multierror
+	if tidyErrors == nil {


Rather than this, just use https://godoc.org/github.com/hashicorp/go-multierror#Error.ErrorOrNil !

jefferai · 2017-04-27T16:50:59Z

vault/expiration.go

+	tidyFunc := func(leaseID string) {
+		i++
+		if i%500 == 0 {
+			m.logger.Debug("expiration: tidying of leases", "progress", i)


s/tidying of leases/tidying leases/

jefferai · 2017-04-27T16:54:45Z

vault/token_store.go

-	// with the token's salt ID at the end, remove it
-	for _, parent := range parentList {
-		children, err := ts.view.List(parentPrefix + parent + "/")
+	if atomic.CompareAndSwapInt64(&ts.tidyLock, 0, 1) {


Why wrap the entire function in an if as opposed to having a block that checks at the beginning whether it's running and simply returns if so (as it does in the expiration manager tidy function)?

Done. In expiration manager, the whole thing is in fact happening inside in if condition. Its just that the tidy tasks are all inside a func.

Actually I was going to ask you to move that check in the expiration manager to the top of that function but then decided to leave it alone :-)

To make it consistent with the changes in the token store, I just did that. Also, added debug statements that indicate that the operation is complete.

1) Ensure that if we fail to generate a lease for a secret we attempt to revoke it 2) Ensure that any lease that is registered should never have a blank token In theory, number 2 will let us a) find places where this *is* the case, and b) if errors are encountered when revoking tokens due to a blank client token, it suggests that the client token values are being stripped somewhere along the way, which is also instructive.

…gisterauth fails

vishalnayak · 2017-05-04T16:25:53Z

vault/expiration.go

+		if retErr != nil {
+			revResp, err := m.router.Route(logical.RevokeRequest(req.Path, resp.Secret, resp.Data))
+			if err != nil {
+				retErr = multierror.Append(retErr, errwrap.Wrapf("an additional error was encountered revoking the newly-generated secret: {{err}}", err))


Can we distinguish the 4 error messages here so that if we hit them, we can know where exactly the error was caused.

vishalnayak · 2017-05-04T16:43:42Z

vault/expiration.go

+	}()
+
+	if req.ClientToken == "" {
+		return "", fmt.Errorf("expiration: cannot register a lease with an empty client token")


Can we move the defer function to be after this check? There is no point in revoking a lease which didn't even get through the input validations.

vishalnayak

LGTM!

briankassouf

Just two optional comments, LGTM!

briankassouf · 2017-05-04T21:36:45Z

vault/expiration.go

+				m.logger.Debug("expiration: revoking lease which holds an invalid token", "lease_id", leaseID)
+				revokeLease = true
+				deletedCountInvalidToken++
+				tokenCache[le.ClientToken] = false


Not necessary, but I think you might as well add a goto REVOKE_CHECK statement here incase more branches are added in the future. Makes it slightly more extendable.

briankassouf · 2017-05-04T21:37:01Z

vault/expiration.go

+			} else {
+				m.logger.Debug("expiration: revoking lease which contains an invalid token", "lease_id", leaseID)
+				revokeLease = true
+				deletedCountInvalidToken++


Same comment here.

juliangamble · 2017-05-13T05:53:33Z

Hi Everyone,
Thanks for all your work on this - where is the best place to talk through ongoing blank token issues even after running the two new tidy operations on 0.7.2?
Cheers
Julian

vishalnayak added 2 commits March 7, 2017 15:50

Added sys/tidy-leases endpoint

1e5d6e3

Add locking where possible while doing auth/token/tidy

239bd1c

jefferai reviewed Apr 26, 2017

View reviewed changes

vault/expiration.go Outdated

return

}

if le.ClientToken == "" {

Copy link

Member

jefferai Apr 26, 2017

Choose a reason for hiding this comment

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

In this case we should probably revoke the lease as well.

vishalnayak added 7 commits April 26, 2017 15:07

Merge branch 'oss' into clean-stale-leases

14aaa0a

Added atomic lock to ensure a single tidy operation is in progress

3477038

Revoke lease that has empty token; added logs

e52625d

Added logger to token store and logs to tidy function

dca0d70

Added caching of looked up tokens

de1a2a0

Fix the log statements

65c63b4

Fix logging levels

b036478

vishalnayak changed the title ~~[WIP] Cleaning of stale leases~~ Cleaning of stale leases Apr 27, 2017

jefferai requested changes Apr 27, 2017

View reviewed changes

vishalnayak added 4 commits April 27, 2017 10:56

Fix logging suggestions; put the policyStore nil check back in

711153d

Cache only valid tokens

0d629ff

Some more logging updates

0c65cd4

Merge branch 'oss' into sys-tidy-leases

785177a

jefferai reviewed Apr 27, 2017

View reviewed changes

vishalnayak added 2 commits April 27, 2017 11:31

Distinguish valid and invalid tokens using bool value in cache

3fdf38a

Use an atomic lock for tidy operation in token store

98cdb68

jefferai reviewed Apr 27, 2017

View reviewed changes

jefferai mentioned this pull request Apr 27, 2017

Corrupt Token Lease Cleanup #2458

Closed

vishalnayak added 3 commits April 27, 2017 13:48

refactor lock handling in token tidy function

2ef62fe

Refactor locking code in lease tidy; add ending debug statements

a8ef2c0

Merge branch 'oss' into sys-tidy-leases

0892102

vishalnayak and others added 16 commits May 3, 2017 10:54

Added summary logs to help better understand the consequence

b6843ec

Less scary debugging

0c02540

change some logging output

b3c6a56

Adhere to tainted status in salted accessor lookup

0bda5a7

consistent logging

8d35f92

Added logs when deletion fails so we can rely on server logs

a2e431b

logging updates

2d21bf6

Add taint flag for looking up by accessor

5bc47b0

Add more cleanup if a lease fails to register and revoke tokens if re…

b0c4a7e

…gisterauth fails

Merge remote-tracking branch 'oss/master' into sys-tidy-leases

265b4cd

Fix substitution of index/child in delete call

2f6e924

Merge branch 'master-oss' into sys-tidy-leases

f1d2fc3

Move tidy-leases to leases/tidy

5320da0

Update commenting

cacf072

Update Tidy function comment

4de09fb

vishalnayak commented May 4, 2017

View reviewed changes

Fix up the tests

106f08a

vishalnayak commented May 4, 2017

View reviewed changes

jefferai added 2 commits May 4, 2017 12:44

Update comments

7829107

Move client token check in exp register to top

1378dd5

vishalnayak commented May 4, 2017

View reviewed changes

briankassouf approved these changes May 4, 2017

View reviewed changes

jefferai added 3 commits May 5, 2017 10:26

Address feedback

5dde45d

Update debugging around tidy

e61298e

Merge branch 'master-oss' into sys-tidy-leases

0e10477

jefferai approved these changes May 5, 2017

View reviewed changes

jefferai merged commit 2f1f4cc into master May 5, 2017

jefferai deleted the sys-tidy-leases branch May 5, 2017 14:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleaning of stale leases #2452

Cleaning of stale leases #2452

vishalnayak commented Mar 7, 2017 •

edited

Loading

jefferai commented Apr 4, 2017

jefferai Apr 26, 2017

jefferai Apr 26, 2017

jefferai Apr 26, 2017

jefferai Apr 26, 2017

jefferai Apr 27, 2017

jefferai Apr 27, 2017

vishalnayak Apr 27, 2017

jefferai Apr 27, 2017

jefferai Apr 27, 2017

jefferai Apr 27, 2017

jefferai Apr 27, 2017

vishalnayak Apr 27, 2017

jefferai Apr 27, 2017

jefferai Apr 27, 2017

jefferai Apr 27, 2017

vishalnayak Apr 27, 2017

jefferai Apr 27, 2017

vishalnayak Apr 27, 2017

vishalnayak May 4, 2017

jefferai May 4, 2017

vishalnayak May 4, 2017

jefferai May 4, 2017

vishalnayak left a comment

briankassouf left a comment

briankassouf May 4, 2017

briankassouf May 4, 2017

juliangamble commented May 13, 2017

Cleaning of stale leases #2452

Cleaning of stale leases #2452

Conversation

vishalnayak commented Mar 7, 2017 • edited Loading

jefferai commented Apr 4, 2017

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

vishalnayak left a comment

Choose a reason for hiding this comment

briankassouf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliangamble commented May 13, 2017

vishalnayak commented Mar 7, 2017 •

edited

Loading