From 98bf463a651db389fbf44ea6857ec7a2b1dffdc0 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Wed, 11 Jul 2018 15:45:09 -0400 Subject: [PATCH] Make single-lease revocation behave like expiration (#4883) This change makes it so that if a lease is revoked through user action, we set the expiration time to now and update pending, just as we do with tokens. This allows the normal retry logic to apply in these cases as well, instead of just erroring out immediately. The idea being that once you tell Vault to revoke something it should keep doing its darndest to actually make that happen. --- api/sys_leases.go | 41 ++++++++ builtin/credential/approle/backend.go | 2 +- .../credential/approle/path_tidy_user_id.go | 9 +- builtin/credential/aws/backend.go | 4 +- .../aws/path_tidy_identity_whitelist.go | 9 +- .../aws/path_tidy_roletag_blacklist.go | 9 +- builtin/logical/pki/path_tidy.go | 3 +- command/lease_revoke.go | 60 +++++++++--- command/server.go | 32 ++++--- http/handler_test.go | 25 +++++ http/logical.go | 4 +- logical/error.go | 22 +++++ logical/request.go | 21 ----- logical/response.go | 35 ++++--- vault/auth.go | 2 +- vault/expiration.go | 61 ++++++++++-- vault/expiration_test.go | 93 +++++++++++++++++++ vault/logical_system.go | 54 +++++++++-- vault/mount.go | 4 +- vault/token_store.go | 3 +- 20 files changed, 392 insertions(+), 101 deletions(-) diff --git a/api/sys_leases.go b/api/sys_leases.go index 34bd99e65..3b65ff094 100644 --- a/api/sys_leases.go +++ b/api/sys_leases.go @@ -1,5 +1,7 @@ package api +import "errors" + func (c *Sys) Renew(id string, increment int) (*Secret, error) { r := c.c.NewRequest("PUT", "/v1/sys/leases/renew") @@ -46,3 +48,42 @@ func (c *Sys) RevokeForce(id string) error { } return err } + +func (c *Sys) RevokeWithOptions(opts *RevokeOptions) error { + if opts == nil { + return errors.New("nil options provided") + } + + // Construct path + path := "/v1/sys/leases/revoke/" + switch { + case opts.Force: + path = "/v1/sys/leases/revoke-force/" + case opts.Prefix: + path = "/v1/sys/leases/revoke-prefix/" + } + path += opts.LeaseID + + r := c.c.NewRequest("PUT", path) + if !opts.Force { + body := map[string]interface{}{ + "sync": opts.Sync, + } + if err := r.SetJSONBody(body); err != nil { + return err + } + } + + resp, err := c.c.RawRequest(r) + if err == nil { + defer resp.Body.Close() + } + return err +} + +type RevokeOptions struct { + LeaseID string + Force bool + Prefix bool + Sync bool +} diff --git a/builtin/credential/approle/backend.go b/builtin/credential/approle/backend.go index d57a3ec8a..a16794e20 100644 --- a/builtin/credential/approle/backend.go +++ b/builtin/credential/approle/backend.go @@ -157,7 +157,7 @@ func (b *backend) invalidate(_ context.Context, key string) { func (b *backend) periodicFunc(ctx context.Context, req *logical.Request) error { // Initiate clean-up of expired SecretID entries if b.System().LocalMount() || !b.System().ReplicationState().HasState(consts.ReplicationPerformanceSecondary) { - b.tidySecretID(ctx, req.Storage) + b.tidySecretID(ctx, req) } return nil } diff --git a/builtin/credential/approle/path_tidy_user_id.go b/builtin/credential/approle/path_tidy_user_id.go index 6137e0594..11693a142 100644 --- a/builtin/credential/approle/path_tidy_user_id.go +++ b/builtin/credential/approle/path_tidy_user_id.go @@ -3,6 +3,7 @@ package approle import ( "context" "fmt" + "net/http" "sync/atomic" "time" @@ -26,13 +27,15 @@ func pathTidySecretID(b *backend) *framework.Path { } // tidySecretID is used to delete entries in the whitelist that are expired. -func (b *backend) tidySecretID(ctx context.Context, s logical.Storage) (*logical.Response, error) { +func (b *backend) tidySecretID(ctx context.Context, req *logical.Request) (*logical.Response, error) { if !atomic.CompareAndSwapUint32(b.tidySecretIDCASGuard, 0, 1) { resp := &logical.Response{} resp.AddWarning("Tidy operation already in progress.") return resp, nil } + s := req.Storage + go func() { defer atomic.StoreUint32(b.tidySecretIDCASGuard, 0) @@ -167,12 +170,12 @@ func (b *backend) tidySecretID(ctx context.Context, s logical.Storage) (*logical resp := &logical.Response{} resp.AddWarning("Tidy operation successfully started. Any information from the operation will be printed to Vault's server logs.") - return resp, nil + return logical.RespondWithStatusCode(resp, req, http.StatusAccepted) } // pathTidySecretIDUpdate is used to delete the expired SecretID entries func (b *backend) pathTidySecretIDUpdate(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { - return b.tidySecretID(ctx, req.Storage) + return b.tidySecretID(ctx, req) } const pathTidySecretIDSyn = "Trigger the clean-up of expired SecretID entries." diff --git a/builtin/credential/aws/backend.go b/builtin/credential/aws/backend.go index 992b29598..809beaefe 100644 --- a/builtin/credential/aws/backend.go +++ b/builtin/credential/aws/backend.go @@ -165,7 +165,7 @@ func (b *backend) periodicFunc(ctx context.Context, req *logical.Request) error } // tidy role tags if explicitly not disabled if !skipBlacklistTidy { - b.tidyBlacklistRoleTag(ctx, req.Storage, safety_buffer) + b.tidyBlacklistRoleTag(ctx, req, safety_buffer) } } @@ -189,7 +189,7 @@ func (b *backend) periodicFunc(ctx context.Context, req *logical.Request) error } // tidy identities if explicitly not disabled if !skipWhitelistTidy { - b.tidyWhitelistIdentity(ctx, req.Storage, safety_buffer) + b.tidyWhitelistIdentity(ctx, req, safety_buffer) } // Update the time at which to run the tidy functions again. diff --git a/builtin/credential/aws/path_tidy_identity_whitelist.go b/builtin/credential/aws/path_tidy_identity_whitelist.go index a8e7c98d3..c09c97de3 100644 --- a/builtin/credential/aws/path_tidy_identity_whitelist.go +++ b/builtin/credential/aws/path_tidy_identity_whitelist.go @@ -3,6 +3,7 @@ package awsauth import ( "context" "fmt" + "net/http" "sync/atomic" "time" @@ -33,13 +34,15 @@ expiration, before it is removed from the backend storage.`, } // tidyWhitelistIdentity is used to delete entries in the whitelist that are expired. -func (b *backend) tidyWhitelistIdentity(ctx context.Context, s logical.Storage, safetyBuffer int) (*logical.Response, error) { +func (b *backend) tidyWhitelistIdentity(ctx context.Context, req *logical.Request, safetyBuffer int) (*logical.Response, error) { if !atomic.CompareAndSwapUint32(b.tidyWhitelistCASGuard, 0, 1) { resp := &logical.Response{} resp.AddWarning("Tidy operation already in progress.") return resp, nil } + s := req.Storage + go func() { defer atomic.StoreUint32(b.tidyWhitelistCASGuard, 0) @@ -93,12 +96,12 @@ func (b *backend) tidyWhitelistIdentity(ctx context.Context, s logical.Storage, resp := &logical.Response{} resp.AddWarning("Tidy operation successfully started. Any information from the operation will be printed to Vault's server logs.") - return resp, nil + return logical.RespondWithStatusCode(resp, req, http.StatusAccepted) } // pathTidyIdentityWhitelistUpdate is used to delete entries in the whitelist that are expired. func (b *backend) pathTidyIdentityWhitelistUpdate(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { - return b.tidyWhitelistIdentity(ctx, req.Storage, data.Get("safety_buffer").(int)) + return b.tidyWhitelistIdentity(ctx, req, data.Get("safety_buffer").(int)) } const pathTidyIdentityWhitelistSyn = ` diff --git a/builtin/credential/aws/path_tidy_roletag_blacklist.go b/builtin/credential/aws/path_tidy_roletag_blacklist.go index e84862f48..7f9a65230 100644 --- a/builtin/credential/aws/path_tidy_roletag_blacklist.go +++ b/builtin/credential/aws/path_tidy_roletag_blacklist.go @@ -3,6 +3,7 @@ package awsauth import ( "context" "fmt" + "net/http" "sync/atomic" "time" @@ -33,13 +34,15 @@ expiration, before it is removed from the backend storage.`, } // tidyBlacklistRoleTag is used to clean-up the entries in the role tag blacklist. -func (b *backend) tidyBlacklistRoleTag(ctx context.Context, s logical.Storage, safetyBuffer int) (*logical.Response, error) { +func (b *backend) tidyBlacklistRoleTag(ctx context.Context, req *logical.Request, safetyBuffer int) (*logical.Response, error) { if !atomic.CompareAndSwapUint32(b.tidyBlacklistCASGuard, 0, 1) { resp := &logical.Response{} resp.AddWarning("Tidy operation already in progress.") return resp, nil } + s := req.Storage + go func() { defer atomic.StoreUint32(b.tidyBlacklistCASGuard, 0) @@ -93,12 +96,12 @@ func (b *backend) tidyBlacklistRoleTag(ctx context.Context, s logical.Storage, s resp := &logical.Response{} resp.AddWarning("Tidy operation successfully started. Any information from the operation will be printed to Vault's server logs.") - return resp, nil + return logical.RespondWithStatusCode(resp, req, http.StatusAccepted) } // pathTidyRoletagBlacklistUpdate is used to clean-up the entries in the role tag blacklist. func (b *backend) pathTidyRoletagBlacklistUpdate(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { - return b.tidyBlacklistRoleTag(ctx, req.Storage, data.Get("safety_buffer").(int)) + return b.tidyBlacklistRoleTag(ctx, req, data.Get("safety_buffer").(int)) } const pathTidyRoletagBlacklistSyn = ` diff --git a/builtin/logical/pki/path_tidy.go b/builtin/logical/pki/path_tidy.go index 4d9ea992d..3ae30fd4d 100644 --- a/builtin/logical/pki/path_tidy.go +++ b/builtin/logical/pki/path_tidy.go @@ -4,6 +4,7 @@ import ( "context" "crypto/x509" "fmt" + "net/http" "sync/atomic" "time" @@ -188,7 +189,7 @@ func (b *backend) pathTidyWrite(ctx context.Context, req *logical.Request, d *fr resp := &logical.Response{} resp.AddWarning("Tidy operation successfully started. Any information from the operation will be printed to Vault's server logs.") - return resp, nil + return logical.RespondWithStatusCode(resp, req, http.StatusAccepted) } const pathTidyHelpSyn = ` diff --git a/command/lease_revoke.go b/command/lease_revoke.go index 45f5dc3a7..6077f053b 100644 --- a/command/lease_revoke.go +++ b/command/lease_revoke.go @@ -4,6 +4,7 @@ import ( "fmt" "strings" + "github.com/hashicorp/vault/api" "github.com/mitchellh/cli" "github.com/posener/complete" ) @@ -16,6 +17,7 @@ type LeaseRevokeCommand struct { flagForce bool flagPrefix bool + flagSync bool } func (c *LeaseRevokeCommand) Synopsis() string { @@ -29,6 +31,12 @@ Usage: vault lease revoke [options] ID Revokes secrets by their lease ID. This command can revoke a single secret or multiple secrets based on a path-matched prefix. + The default behavior when not using -force is to revoke asynchronously; Vault + will queue the revocation and keep trying if it fails (including across + restarts). The -sync flag can be used to force a synchronous operation, but + it is then up to the caller to retry on failure. Force mode always operates + synchronously. + Revoke a single lease: $ vault lease revoke database/creds/readonly/2f6a614c... @@ -72,6 +80,14 @@ func (c *LeaseRevokeCommand) Flags() *FlagSets { "revoke multiple leases simultaneously.", }) + f.BoolVar(&BoolVar{ + Name: "sync", + Target: &c.flagSync, + Default: false, + Usage: "Force a synchronous operation; on failure it is up to the client " + + "to retry.", + }) + return set } @@ -114,29 +130,47 @@ func (c *LeaseRevokeCommand) Run(args []string) int { leaseID := strings.TrimSpace(args[0]) - switch { - case c.flagForce && c.flagPrefix: + revokeOpts := &api.RevokeOptions{ + LeaseID: leaseID, + Force: c.flagForce, + Prefix: c.flagPrefix, + Sync: c.flagSync, + } + + if c.flagForce { c.UI.Warn(wrapAtLength("Warning! Force-removing leases can cause Vault " + "to become out of sync with secret engines!")) - if err := client.Sys().RevokeForce(leaseID); err != nil { + } + + err = client.Sys().RevokeWithOptions(revokeOpts) + if err != nil { + switch { + case c.flagForce: c.UI.Error(fmt.Sprintf("Error force revoking leases with prefix %s: %s", leaseID, err)) return 2 - } - c.UI.Output(fmt.Sprintf("Success! Force revoked any leases with prefix: %s", leaseID)) - return 0 - case c.flagPrefix: - if err := client.Sys().RevokePrefix(leaseID); err != nil { + case c.flagPrefix: c.UI.Error(fmt.Sprintf("Error revoking leases with prefix %s: %s", leaseID, err)) return 2 - } - c.UI.Output(fmt.Sprintf("Success! Revoked any leases with prefix: %s", leaseID)) - return 0 - default: - if err := client.Sys().Revoke(leaseID); err != nil { + default: c.UI.Error(fmt.Sprintf("Error revoking lease %s: %s", leaseID, err)) return 2 } + } + + if c.flagForce { + c.UI.Output(fmt.Sprintf("Success! Force revoked any leases with prefix: %s", leaseID)) + return 0 + } + + if c.flagSync { + if c.flagPrefix { + c.UI.Output(fmt.Sprintf("Success! Revoked any leases with prefix: %s", leaseID)) + return 0 + } c.UI.Output(fmt.Sprintf("Success! Revoked lease: %s", leaseID)) return 0 } + + c.UI.Output("All revocation operations queued successfully!") + return 0 } diff --git a/command/server.go b/command/server.go index ede361166..32be0f197 100644 --- a/command/server.go +++ b/command/server.go @@ -1151,22 +1151,24 @@ func (c *ServerCommand) enableDev(core *vault.Core, coreConfig *vault.CoreConfig } // Upgrade the default K/V store - req := &logical.Request{ - Operation: logical.UpdateOperation, - ClientToken: init.RootToken, - Path: "sys/mounts/secret/tune", - Data: map[string]interface{}{ - "options": map[string]string{ - "version": "2", + if !c.flagDevLeasedKV { + req := &logical.Request{ + Operation: logical.UpdateOperation, + ClientToken: init.RootToken, + Path: "sys/mounts/secret/tune", + Data: map[string]interface{}{ + "options": map[string]string{ + "version": "2", + }, }, - }, - } - resp, err := core.HandleRequest(req) - if err != nil { - return nil, errwrap.Wrapf("error upgrading default K/V store: {{err}}", err) - } - if resp.IsError() { - return nil, errwrap.Wrapf("failed to upgrade default K/V store: {{err}}", resp.Error()) + } + resp, err := core.HandleRequest(req) + if err != nil { + return nil, errwrap.Wrapf("error upgrading default K/V store: {{err}}", err) + } + if resp.IsError() { + return nil, errwrap.Wrapf("failed to upgrade default K/V store: {{err}}", resp.Error()) + } } return init, nil diff --git a/http/handler_test.go b/http/handler_test.go index 3760b4c98..34ef71f3a 100644 --- a/http/handler_test.go +++ b/http/handler_test.go @@ -130,6 +130,31 @@ func TestHandler_CacheControlNoStore(t *testing.T) { } } +func TestHandler_Accepted(t *testing.T) { + core, _, token := vault.TestCoreUnsealed(t) + ln, addr := TestServer(t, core) + defer ln.Close() + + req, err := http.NewRequest("POST", addr+"/v1/auth/token/tidy", nil) + if err != nil { + t.Fatalf("err: %s", err) + } + req.Header.Set(AuthHeaderName, token) + + client := cleanhttp.DefaultClient() + resp, err := client.Do(req) + if err != nil { + t.Fatalf("err: %s", err) + } + + t.Logf("%#v", resp) + + testResponseStatus(t, resp, 202) + if resp.Body != http.NoBody { + t.Fatal("got non-empty body") + } +} + // We use this test to verify header auth func TestSysMounts_headerAuth(t *testing.T) { core, _, token := vault.TestCoreUnsealed(t) diff --git a/http/logical.go b/http/logical.go index 7b31c8751..fb60010d1 100644 --- a/http/logical.go +++ b/http/logical.go @@ -268,8 +268,7 @@ func respondRaw(w http.ResponseWriter, r *http.Request, resp *logical.Response) // Get the body bodyRaw, ok := resp.Data[logical.HTTPRawBody] if !ok { - retErr(w, "no body given") - return + goto WRITE_RESPONSE } switch bodyRaw.(type) { @@ -290,6 +289,7 @@ func respondRaw(w http.ResponseWriter, r *http.Request, resp *logical.Response) } } +WRITE_RESPONSE: // Write the response if contentType != "" { w.Header().Set("Content-Type", contentType) diff --git a/logical/error.go b/logical/error.go index 3c017bdf9..b033e0eac 100644 --- a/logical/error.go +++ b/logical/error.go @@ -1,5 +1,27 @@ package logical +import "errors" + +var ( + // ErrUnsupportedOperation is returned if the operation is not supported + // by the logical backend. + ErrUnsupportedOperation = errors.New("unsupported operation") + + // ErrUnsupportedPath is returned if the path is not supported + // by the logical backend. + ErrUnsupportedPath = errors.New("unsupported path") + + // ErrInvalidRequest is returned if the request is invalid + ErrInvalidRequest = errors.New("invalid request") + + // ErrPermissionDenied is returned if the client is not authorized + ErrPermissionDenied = errors.New("permission denied") + + // ErrMultiAuthzPending is returned if the the request needs more + // authorizations + ErrMultiAuthzPending = errors.New("request needs further approval") +) + type HTTPCodedError interface { Error() string Code() int diff --git a/logical/request.go b/logical/request.go index 4c395370d..4bc7f50a5 100644 --- a/logical/request.go +++ b/logical/request.go @@ -1,7 +1,6 @@ package logical import ( - "errors" "fmt" "strings" "time" @@ -273,23 +272,3 @@ const ( RenewOperation = "renew" RollbackOperation = "rollback" ) - -var ( - // ErrUnsupportedOperation is returned if the operation is not supported - // by the logical backend. - ErrUnsupportedOperation = errors.New("unsupported operation") - - // ErrUnsupportedPath is returned if the path is not supported - // by the logical backend. - ErrUnsupportedPath = errors.New("unsupported path") - - // ErrInvalidRequest is returned if the request is invalid - ErrInvalidRequest = errors.New("invalid request") - - // ErrPermissionDenied is returned if the client is not authorized - ErrPermissionDenied = errors.New("permission denied") - - // ErrMultiAuthzPending is returned if the the request needs more - // authorizations - ErrMultiAuthzPending = errors.New("request needs further approval") -) diff --git a/logical/response.go b/logical/response.go index 723f88e7d..8a6402df1 100644 --- a/logical/response.go +++ b/logical/response.go @@ -141,22 +141,27 @@ func ListResponseWithInfo(keys []string, keyInfo map[string]interface{}) *Respon // RespondWithStatusCode takes a response and converts it to a raw response with // the provided Status Code. func RespondWithStatusCode(resp *Response, req *Request, code int) (*Response, error) { - httpResp := LogicalResponseToHTTPResponse(resp) - httpResp.RequestID = req.ID - - body, err := json.Marshal(httpResp) - if err != nil { - return nil, err - } - - return &Response{ + ret := &Response{ Data: map[string]interface{}{ HTTPContentType: "application/json", - // We default to string here so that the value is HMAC'd via audit. - // Since this function is always marshaling to JSON, this is - // appropriate. - HTTPRawBody: string(body), - HTTPStatusCode: code, + HTTPStatusCode: code, }, - }, nil + } + + if resp != nil { + httpResp := LogicalResponseToHTTPResponse(resp) + httpResp.RequestID = req.ID + + body, err := json.Marshal(httpResp) + if err != nil { + return nil, err + } + + // We default to string here so that the value is HMAC'd via audit. + // Since this function is always marshaling to JSON, this is + // appropriate. + ret.Data[HTTPRawBody] = string(body) + } + + return ret, nil } diff --git a/vault/auth.go b/vault/auth.go index a50320dc0..a0c9d0f86 100644 --- a/vault/auth.go +++ b/vault/auth.go @@ -189,7 +189,7 @@ func (c *Core) disableCredential(ctx context.Context, path string) error { if backend != nil { // Revoke credentials from this path - if err := c.expiration.RevokePrefix(fullPath); err != nil { + if err := c.expiration.RevokePrefix(fullPath, true); err != nil { return err } diff --git a/vault/expiration.go b/vault/expiration.go index 7924a4017..a56e7fcb2 100644 --- a/vault/expiration.go +++ b/vault/expiration.go @@ -489,6 +489,38 @@ func (m *ExpirationManager) Revoke(leaseID string) error { return m.revokeCommon(leaseID, false, false) } +// LazyRevoke is used to queue revocation for a secret named by the given +// LeaseID. If the lease was not found it returns nil; if the lease was found +// it triggers a return of a 202. +func (m *ExpirationManager) LazyRevoke(leaseID string) error { + defer metrics.MeasureSince([]string{"expire", "lazy-revoke"}, time.Now()) + + // Load the entry + le, err := m.loadEntry(leaseID) + if err != nil { + return err + } + + // If there is no entry, nothing to revoke + if le == nil { + return nil + } + + le.ExpireTime = time.Now() + { + m.pendingLock.Lock() + if err := m.persistEntry(le); err != nil { + m.pendingLock.Unlock() + return err + } + + m.updatePendingInternal(le, 0) + m.pendingLock.Unlock() + } + + return nil +} + // revokeCommon does the heavy lifting. If force is true, we ignore a problem // during revocation and still remove entries/index/lease timers func (m *ExpirationManager) revokeCommon(leaseID string, force, skipToken bool) error { @@ -550,16 +582,16 @@ func (m *ExpirationManager) revokeCommon(leaseID string, force, skipToken bool) func (m *ExpirationManager) RevokeForce(prefix string) error { defer metrics.MeasureSince([]string{"expire", "revoke-force"}, time.Now()) - return m.revokePrefixCommon(prefix, true) + return m.revokePrefixCommon(prefix, true, true) } // RevokePrefix is used to revoke all secrets with a given prefix. // The prefix maps to that of the mount table to make this simpler // to reason about. -func (m *ExpirationManager) RevokePrefix(prefix string) error { +func (m *ExpirationManager) RevokePrefix(prefix string, sync bool) error { defer metrics.MeasureSince([]string{"expire", "revoke-prefix"}, time.Now()) - return m.revokePrefixCommon(prefix, false) + return m.revokePrefixCommon(prefix, false, sync) } // RevokeByToken is used to revoke all the secrets issued with a given token. @@ -623,7 +655,7 @@ func (m *ExpirationManager) RevokeByToken(te *logical.TokenEntry) error { return nil } -func (m *ExpirationManager) revokePrefixCommon(prefix string, force bool) error { +func (m *ExpirationManager) revokePrefixCommon(prefix string, force, sync bool) error { if m.inRestoreMode() { m.restoreRequestLock.Lock() defer m.restoreRequestLock.Unlock() @@ -634,10 +666,13 @@ func (m *ExpirationManager) revokePrefixCommon(prefix string, force bool) error if !strings.HasSuffix(prefix, "/") { le, err := m.loadEntry(prefix) if err == nil && le != nil { - if err := m.revokeCommon(prefix, force, false); err != nil { - return errwrap.Wrapf(fmt.Sprintf("failed to revoke %q: {{err}}", prefix), err) + if sync { + if err := m.revokeCommon(prefix, force, false); err != nil { + return errwrap.Wrapf(fmt.Sprintf("failed to revoke %q: {{err}}", prefix), err) + } + return nil } - return nil + return m.LazyRevoke(prefix) } prefix = prefix + "/" } @@ -652,10 +687,18 @@ func (m *ExpirationManager) revokePrefixCommon(prefix string, force bool) error // Revoke all the keys for idx, suffix := range existing { leaseID := prefix + suffix - if err := m.revokeCommon(leaseID, force, false); err != nil { - return errwrap.Wrapf(fmt.Sprintf("failed to revoke %q (%d / %d): {{err}}", leaseID, idx+1, len(existing)), err) + switch { + case sync: + if err := m.revokeCommon(leaseID, force, false); err != nil { + return errwrap.Wrapf(fmt.Sprintf("failed to revoke %q (%d / %d): {{err}}", leaseID, idx+1, len(existing)), err) + } + default: + if err := m.LazyRevoke(leaseID); err != nil { + return errwrap.Wrapf(fmt.Sprintf("failed to revoke %q (%d / %d): {{err}}", leaseID, idx+1, len(existing)), err) + } } } + return nil } diff --git a/vault/expiration_test.go b/vault/expiration_test.go index 6a5578a00..f9f360a07 100644 --- a/vault/expiration_test.go +++ b/vault/expiration_test.go @@ -3,11 +3,13 @@ package vault import ( "bytes" "context" + "errors" "fmt" "reflect" "sort" "strings" "sync" + "sync/atomic" "testing" "time" @@ -1438,6 +1440,97 @@ func TestExpiration_renewEntry(t *testing.T) { } } +func TestExpiration_revokeEntry_rejected(t *testing.T) { + core, _, _ := TestCoreUnsealed(t) + exp := core.expiration + + rejected := new(uint32) + + noop := &NoopBackend{ + RequestHandler: func(ctx context.Context, req *logical.Request) (*logical.Response, error) { + if req.Operation == logical.RevokeOperation { + if atomic.CompareAndSwapUint32(rejected, 0, 1) { + t.Logf("denying revocation") + return nil, errors.New("nope") + } + t.Logf("allowing revocation") + } + return nil, nil + }, + } + _, barrier, _ := mockBarrier(t) + view := NewBarrierView(barrier, "logical/") + meUUID, err := uuid.GenerateUUID() + if err != nil { + t.Fatal(err) + } + err = exp.router.Mount(noop, "foo/bar/", &MountEntry{Path: "foo/bar/", Type: "noop", UUID: meUUID, Accessor: "noop-accessor"}, view) + if err != nil { + t.Fatal(err) + } + + le := &leaseEntry{ + LeaseID: "foo/bar/1234", + Path: "foo/bar", + Data: map[string]interface{}{ + "testing": true, + }, + Secret: &logical.Secret{ + LeaseOptions: logical.LeaseOptions{ + TTL: time.Minute, + }, + }, + IssueTime: time.Now(), + ExpireTime: time.Now().Add(time.Minute), + } + + err = exp.persistEntry(le) + if err != nil { + t.Fatal(err) + } + + err = exp.Revoke(le.LeaseID) + if err != nil { + t.Fatal(err) + } + + // Give time to let the request be handled + time.Sleep(1 * time.Second) + + if atomic.LoadUint32(rejected) != 1 { + t.Fatal("unexpected val for rejected") + } + + err = exp.Stop() + if err != nil { + t.Fatal(err) + } + + err = core.setupExpiration() + if err != nil { + t.Fatal(err) + } + exp = core.expiration + + for { + if !exp.inRestoreMode() { + break + } + time.Sleep(100 * time.Millisecond) + } + + // Now let the revocation actually process + time.Sleep(1 * time.Second) + + le, err = exp.FetchLeaseTimes(le.LeaseID) + if err != nil { + t.Fatal(err) + } + if le != nil { + t.Fatal("ugh") + } +} + func TestExpiration_renewAuthEntry(t *testing.T) { exp := mockExpiration(t) diff --git a/vault/logical_system.go b/vault/logical_system.go index fd9fe69d8..913da8a34 100644 --- a/vault/logical_system.go +++ b/vault/logical_system.go @@ -536,6 +536,11 @@ func NewSystemBackend(core *Core, logger log.Logger) *SystemBackend { Type: framework.TypeString, Description: strings.TrimSpace(sysHelp["lease_id"][0]), }, + "sync": &framework.FieldSchema{ + Type: framework.TypeBool, + Default: true, + Description: strings.TrimSpace(sysHelp["revoke-sync"][0]), + }, }, Callbacks: map[logical.Operation]framework.OperationFunc{ @@ -572,6 +577,11 @@ func NewSystemBackend(core *Core, logger log.Logger) *SystemBackend { Type: framework.TypeString, Description: strings.TrimSpace(sysHelp["revoke-prefix-path"][0]), }, + "sync": &framework.FieldSchema{ + Type: framework.TypeBool, + Default: true, + Description: strings.TrimSpace(sysHelp["revoke-sync"][0]), + }, }, Callbacks: map[logical.Operation]framework.OperationFunc{ @@ -1192,7 +1202,7 @@ func (b *SystemBackend) handleTidyLeases(ctx context.Context, req *logical.Reque resp := &logical.Response{} resp.AddWarning("Tidy operation successfully started. Any information from the operation will be printed to Vault's server logs.") - return resp, nil + return logical.RespondWithStatusCode(resp, req, http.StatusAccepted) } func (b *SystemBackend) invalidate(ctx context.Context, key string) { @@ -2306,27 +2316,37 @@ func (b *SystemBackend) handleRevoke(ctx context.Context, req *logical.Request, logical.ErrInvalidRequest } - // Invoke the expiration manager directly - if err := b.Core.expiration.Revoke(leaseID); err != nil { + if data.Get("sync").(bool) { + // Invoke the expiration manager directly + if err := b.Core.expiration.Revoke(leaseID); err != nil { + b.Backend.Logger().Error("lease revocation failed", "lease_id", leaseID, "error", err) + return handleErrorNoReadOnlyForward(err) + } + + return nil, nil + } + + if err := b.Core.expiration.LazyRevoke(leaseID); err != nil { b.Backend.Logger().Error("lease revocation failed", "lease_id", leaseID, "error", err) return handleErrorNoReadOnlyForward(err) } - return nil, nil + + return logical.RespondWithStatusCode(nil, nil, http.StatusAccepted) } // handleRevokePrefix is used to revoke a prefix with many LeaseIDs func (b *SystemBackend) handleRevokePrefix(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { - return b.handleRevokePrefixCommon(req, data, false) + return b.handleRevokePrefixCommon(req, data, false, data.Get("sync").(bool)) } // handleRevokeForce is used to revoke a prefix with many LeaseIDs, ignoring errors func (b *SystemBackend) handleRevokeForce(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { - return b.handleRevokePrefixCommon(req, data, true) + return b.handleRevokePrefixCommon(req, data, true, true) } // handleRevokePrefixCommon is used to revoke a prefix with many LeaseIDs func (b *SystemBackend) handleRevokePrefixCommon( - req *logical.Request, data *framework.FieldData, force bool) (*logical.Response, error) { + req *logical.Request, data *framework.FieldData, force, sync bool) (*logical.Response, error) { // Get all the options prefix := data.Get("prefix").(string) @@ -2335,13 +2355,18 @@ func (b *SystemBackend) handleRevokePrefixCommon( if force { err = b.Core.expiration.RevokeForce(prefix) } else { - err = b.Core.expiration.RevokePrefix(prefix) + err = b.Core.expiration.RevokePrefix(prefix, sync) } if err != nil { b.Backend.Logger().Error("revoke prefix failed", "prefix", prefix, "error", err) return handleErrorNoReadOnlyForward(err) } - return nil, nil + + if sync { + return nil, nil + } + + return logical.RespondWithStatusCode(nil, nil, http.StatusAccepted) } // handleAuthTable handles the "auth" endpoint to provide the auth table @@ -3960,6 +3985,17 @@ used to revoke the secret with the given Lease ID. `, }, + "revoke-sync": { + "Whether or not to perform the revocation synchronously", + ` +If false, the call will return immediately and revocation will be queued; if it +fails, Vault will keep trying. If true, if the revocation fails, Vault will not +automatically try again and will return an error. For revoke-prefix, this +setting will apply to all leases being revoked. For revoke-force, since errors +are ignored, this setting is not supported. +`, + }, + "revoke-prefix": { "Revoke all secrets generated in a given prefix", ` diff --git a/vault/mount.go b/vault/mount.go index 5d89e1285..ff0f20150 100644 --- a/vault/mount.go +++ b/vault/mount.go @@ -408,7 +408,7 @@ func (c *Core) unmountInternal(ctx context.Context, path string) error { } // Revoke all the dynamic keys - if err := c.expiration.RevokePrefix(path); err != nil { + if err := c.expiration.RevokePrefix(path, true); err != nil { return err } @@ -555,7 +555,7 @@ func (c *Core) remount(ctx context.Context, src, dst string) error { } // Revoke all the dynamic keys - if err := c.expiration.RevokePrefix(src); err != nil { + if err := c.expiration.RevokePrefix(src, true); err != nil { return err } diff --git a/vault/token_store.go b/vault/token_store.go index 9526c72c0..aade87a05 100644 --- a/vault/token_store.go +++ b/vault/token_store.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "net/http" "sync" "sync/atomic" @@ -1507,7 +1508,7 @@ func (ts *TokenStore) handleTidy(ctx context.Context, req *logical.Request, data resp := &logical.Response{} resp.AddWarning("Tidy operation successfully started. Any information from the operation will be printed to Vault's server logs.") - return resp, nil + return logical.RespondWithStatusCode(resp, req, http.StatusAccepted) } // handleUpdateLookupAccessor handles the auth/token/lookup-accessor path for returning