From 2144bd7fbde71c419342f80a09db659f0cc0d9ff Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" Date: Mon, 8 Apr 2019 12:05:51 -0500 Subject: [PATCH] acl: tokens can be created with an optional expiration time (#5353) --- agent/acl_endpoint.go | 30 +- agent/consul/acl.go | 11 +- agent/consul/acl_endpoint.go | 57 +- agent/consul/acl_endpoint_legacy.go | 15 +- agent/consul/acl_endpoint_test.go | 653 ++++++++++++++++++-- agent/consul/acl_replication.go | 3 +- agent/consul/acl_replication_legacy.go | 7 +- agent/consul/acl_server.go | 8 +- agent/consul/acl_token_exp.go | 144 +++++ agent/consul/acl_token_exp_test.go | 219 +++++++ agent/consul/config.go | 12 + agent/consul/fsm/commands_oss.go | 3 +- agent/consul/fsm/snapshot_oss.go | 2 + agent/consul/leader.go | 17 +- agent/consul/server.go | 6 + agent/consul/state/acl.go | 123 +++- agent/consul/state/acl_test.go | 262 ++++++-- agent/structs/acl.go | 88 ++- agent/structs/acl_test.go | 2 +- api/acl.go | 39 +- command/acl/acl_helpers.go | 38 +- command/acl/token/clone/token_clone_test.go | 10 +- command/acl/token/create/token_create.go | 23 +- command/acl/token/update/token_update.go | 6 +- 24 files changed, 1574 insertions(+), 204 deletions(-) create mode 100644 agent/consul/acl_token_exp.go create mode 100644 agent/consul/acl_token_exp_test.go diff --git a/agent/acl_endpoint.go b/agent/acl_endpoint.go index 64eaad0de831..d34690b3281b 100644 --- a/agent/acl_endpoint.go +++ b/agent/acl_endpoint.go @@ -268,15 +268,35 @@ func (s *HTTPServer) ACLPolicyCreate(resp http.ResponseWriter, req *http.Request return s.ACLPolicyWrite(resp, req, "") } -// fixCreateTimeAndHash is used to help in decoding the CreateTime and Hash +// fixTimeAndHashFields is used to help in decoding the ExpirationTTL, ExpirationTime, CreateTime, and Hash // attributes from the ACL Token/Policy create/update requests. It is needed // to help mapstructure decode things properly when decodeBody is used. -func fixCreateTimeAndHash(raw interface{}) error { +func fixTimeAndHashFields(raw interface{}) error { rawMap, ok := raw.(map[string]interface{}) if !ok { return nil } + if val, ok := rawMap["ExpirationTTL"]; ok { + if sval, ok := val.(string); ok { + d, err := time.ParseDuration(sval) + if err != nil { + return err + } + rawMap["ExpirationTTL"] = d + } + } + + if val, ok := rawMap["ExpirationTime"]; ok { + if sval, ok := val.(string); ok { + t, err := time.Parse(time.RFC3339, sval) + if err != nil { + return err + } + rawMap["ExpirationTime"] = t + } + } + if val, ok := rawMap["CreateTime"]; ok { if sval, ok := val.(string); ok { t, err := time.Parse(time.RFC3339, sval) @@ -301,7 +321,7 @@ func (s *HTTPServer) ACLPolicyWrite(resp http.ResponseWriter, req *http.Request, } s.parseToken(req, &args.Token) - if err := decodeBody(req, &args.Policy, fixCreateTimeAndHash); err != nil { + if err := decodeBody(req, &args.Policy, fixTimeAndHashFields); err != nil { return nil, BadRequestError{Reason: fmt.Sprintf("Policy decoding failed: %v", err)} } @@ -472,7 +492,7 @@ func (s *HTTPServer) ACLTokenSet(resp http.ResponseWriter, req *http.Request, to } s.parseToken(req, &args.Token) - if err := decodeBody(req, &args.ACLToken, fixCreateTimeAndHash); err != nil { + if err := decodeBody(req, &args.ACLToken, fixTimeAndHashFields); err != nil { return nil, BadRequestError{Reason: fmt.Sprintf("Token decoding failed: %v", err)} } @@ -513,7 +533,7 @@ func (s *HTTPServer) ACLTokenClone(resp http.ResponseWriter, req *http.Request, Datacenter: s.agent.config.Datacenter, } - if err := decodeBody(req, &args.ACLToken, fixCreateTimeAndHash); err != nil && err.Error() != "EOF" { + if err := decodeBody(req, &args.ACLToken, fixTimeAndHashFields); err != nil && err.Error() != "EOF" { return nil, BadRequestError{Reason: fmt.Sprintf("Token decoding failed: %v", err)} } s.parseToken(req, &args.Token) diff --git a/agent/consul/acl.go b/agent/consul/acl.go index 7553291fef96..4f1061e5d173 100644 --- a/agent/consul/acl.go +++ b/agent/consul/acl.go @@ -31,9 +31,16 @@ const ( // with all tokens in it. aclUpgradeBatchSize = 128 - // aclUpgradeRateLimit is the number of batch upgrade requests per second. + // aclUpgradeRateLimit is the number of batch upgrade requests per second allowed. aclUpgradeRateLimit rate.Limit = 1.0 + // aclTokenReapingRateLimit is the number of batch token reaping requests per second allowed. + aclTokenReapingRateLimit rate.Limit = 1.0 + + // aclTokenReapingBurst is the number of batch token reaping requests per second + // that can burst after a period of idleness. + aclTokenReapingBurst = 5 + // aclBatchDeleteSize is the number of deletions to send in a single batch operation. 4096 should produce a batch that is <150KB // in size but should be sufficiently large to handle 1 replication round in a single batch aclBatchDeleteSize = 4096 @@ -608,6 +615,8 @@ func (r *ACLResolver) resolveTokenToIdentityAndPolicies(token string) (structs.A return nil, nil, err } else if identity == nil { return nil, nil, acl.ErrNotFound + } else if identity.IsExpired(time.Now()) { + return nil, nil, acl.ErrNotFound } lastIdentity = identity diff --git a/agent/consul/acl_endpoint.go b/agent/consul/acl_endpoint.go index 5f9738c9cc42..c19ff297645a 100644 --- a/agent/consul/acl_endpoint.go +++ b/agent/consul/acl_endpoint.go @@ -221,6 +221,10 @@ func (a *ACL) TokenRead(args *structs.ACLTokenGetRequest, reply *structs.ACLToke index, token, err = state.ACLTokenGetBySecret(ws, args.TokenID) } + if token != nil && token.IsExpired(time.Now()) { + token = nil + } + if err != nil { return err } @@ -256,7 +260,7 @@ func (a *ACL) TokenClone(args *structs.ACLTokenSetRequest, reply *structs.ACLTok _, token, err := a.srv.fsm.State().ACLTokenGetByAccessor(nil, args.ACLToken.AccessorID) if err != nil { return err - } else if token == nil { + } else if token == nil || token.IsExpired(time.Now()) { return acl.ErrNotFound } else if !a.srv.InACLDatacenter() && !token.Local { // global token writes must be forwarded to the primary DC @@ -271,9 +275,10 @@ func (a *ACL) TokenClone(args *structs.ACLTokenSetRequest, reply *structs.ACLTok cloneReq := structs.ACLTokenSetRequest{ Datacenter: args.Datacenter, ACLToken: structs.ACLToken{ - Policies: token.Policies, - Local: token.Local, - Description: token.Description, + Policies: token.Policies, + Local: token.Local, + Description: token.Description, + ExpirationTime: token.ExpirationTime, }, WriteRequest: args.WriteRequest, } @@ -342,6 +347,34 @@ func (a *ACL) tokenSetInternal(args *structs.ACLTokenSetRequest, reply *structs. } token.CreateTime = time.Now() + + // Ensure an ExpirationTTL is valid if provided. + if token.ExpirationTTL != 0 { + if token.ExpirationTTL < 0 { + return fmt.Errorf("Token Expiration TTL '%s' should be > 0", token.ExpirationTTL) + } + if !token.ExpirationTime.IsZero() { + return fmt.Errorf("Token Expiration TTL and Expiration Time cannot both be set") + } + + token.ExpirationTime = token.CreateTime.Add(token.ExpirationTTL) + token.ExpirationTTL = 0 + } + + if !token.ExpirationTime.IsZero() { + if token.CreateTime.After(token.ExpirationTime) { + return fmt.Errorf("ExpirationTime cannot be before CreateTime") + } + + expiresIn := token.ExpirationTime.Sub(token.CreateTime) + if expiresIn > a.srv.config.ACLTokenMaxExpirationTTL { + return fmt.Errorf("ExpirationTime cannot be more than %s in the future (was %s)", + a.srv.config.ACLTokenMaxExpirationTTL, expiresIn) + } else if expiresIn < a.srv.config.ACLTokenMinExpirationTTL { + return fmt.Errorf("ExpirationTime cannot be less than %s in the future (was %s)", + a.srv.config.ACLTokenMinExpirationTTL, expiresIn) + } + } } else { // Token Update if _, err := uuid.ParseUUID(token.AccessorID); err != nil { @@ -365,7 +398,7 @@ func (a *ACL) tokenSetInternal(args *structs.ACLTokenSetRequest, reply *structs. if err != nil { return fmt.Errorf("Failed to lookup the acl token %q: %v", token.AccessorID, err) } - if existing == nil { + if existing == nil || existing.IsExpired(time.Now()) { return fmt.Errorf("Cannot find token %q", token.AccessorID) } if token.SecretID == "" { @@ -379,6 +412,10 @@ func (a *ACL) tokenSetInternal(args *structs.ACLTokenSetRequest, reply *structs. return fmt.Errorf("cannot toggle local mode of %s", token.AccessorID) } + if token.ExpirationTTL != 0 || !token.ExpirationTime.Equal(existing.ExpirationTime) { + return fmt.Errorf("Cannot change expiration time of %s", token.AccessorID) + } + if upgrade { token.CreateTime = time.Now() } else { @@ -440,6 +477,7 @@ func (a *ACL) tokenSetInternal(args *structs.ACLTokenSetRequest, reply *structs. return respErr } + // Don't check expiration times here as it doesn't really matter. if _, updatedToken, err := a.srv.fsm.State().ACLTokenGetByAccessor(nil, token.AccessorID); err == nil && token != nil { *reply = *updatedToken } else { @@ -490,6 +528,8 @@ func (a *ACL) TokenDelete(args *structs.ACLTokenDeleteRequest, reply *string) er return fmt.Errorf("Deletion of the request's authorization token is not permitted") } + // No need to check expiration time because it's being deleted. + if !a.srv.InACLDatacenter() && !token.Local { args.Datacenter = a.srv.config.ACLDatacenter return a.srv.forwardDC("ACL.TokenDelete", a.srv.config.ACLDatacenter, args, reply) @@ -553,8 +593,13 @@ func (a *ACL) TokenList(args *structs.ACLTokenListRequest, reply *structs.ACLTok return err } + now := time.Now() + stubs := make([]*structs.ACLTokenListStub, 0, len(tokens)) for _, token := range tokens { + if token.IsExpired(now) { + continue + } stubs = append(stubs, token.Stub()) } reply.Index, reply.Tokens = index, stubs @@ -589,6 +634,8 @@ func (a *ACL) TokenBatchRead(args *structs.ACLTokenBatchGetRequest, reply *struc return err } + // This RPC is used for replication, so don't filter out expired tokens here. + a.srv.filterACLWithAuthorizer(rule, &tokens) reply.Index, reply.Tokens = index, tokens diff --git a/agent/consul/acl_endpoint_legacy.go b/agent/consul/acl_endpoint_legacy.go index 48867fdb3af8..ad264e2dcb3b 100644 --- a/agent/consul/acl_endpoint_legacy.go +++ b/agent/consul/acl_endpoint_legacy.go @@ -93,8 +93,9 @@ func aclApplyInternal(srv *Server, args *structs.ACLRequest, reply *string) erro return fmt.Errorf("Invalid ACL Type") } + // No need to check expiration times as those did not exist in legacy tokens. _, existing, _ := srv.fsm.State().ACLTokenGetBySecret(nil, args.ACL.ID) - if existing != nil && len(existing.Policies) > 0 { + if existing != nil && existing.UsesNonLegacyFields() { return fmt.Errorf("Cannot use legacy endpoint to modify a non-legacy token") } @@ -210,8 +211,13 @@ func (a *ACL) Get(args *structs.ACLSpecificRequest, return err } - // converting an ACLToken to an ACL will return nil and an error + // Converting an ACLToken to an ACL will return nil and an error // (which we ignore) when it is unconvertible. + // + // This also means we won't have to check expiration times since + // any legacy tokens never had expiration times and no non-legacy + // tokens can be converted. + var acl *structs.ACL if token != nil { acl, _ = token.Convert() @@ -254,8 +260,13 @@ func (a *ACL) List(args *structs.DCSpecificRequest, return err } + now := time.Now() + var acls structs.ACLs for _, token := range tokens { + if token.IsExpired(now) { + continue + } if acl, err := token.Convert(); err == nil && acl != nil { acls = append(acls, acl) } diff --git a/agent/consul/acl_endpoint_test.go b/agent/consul/acl_endpoint_test.go index c88ba1563f46..0ad3c5d3bbc5 100644 --- a/agent/consul/acl_endpoint_test.go +++ b/agent/consul/acl_endpoint_test.go @@ -630,6 +630,8 @@ func TestACLEndpoint_TokenRead(t *testing.T) { c.ACLDatacenter = "dc1" c.ACLsEnabled = true c.ACLMasterToken = "root" + c.ACLTokenMinExpirationTTL = 10 * time.Millisecond + c.ACLTokenMaxExpirationTTL = 5 * time.Second }) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -638,14 +640,12 @@ func TestACLEndpoint_TokenRead(t *testing.T) { testrpc.WaitForLeader(t, s1.RPC, "dc1") - token, err := upsertTestToken(codec, "root", "dc1") - if err != nil { - t.Fatalf("err: %v", err) - } - acl := ACL{srv: s1} t.Run("exists and matches what we created", func(t *testing.T) { + token, err := upsertTestToken(codec, "root", "dc1", nil) + require.NoError(t, err) + req := structs.ACLTokenGetRequest{ Datacenter: "dc1", TokenID: token.AccessorID, @@ -655,7 +655,7 @@ func TestACLEndpoint_TokenRead(t *testing.T) { resp := structs.ACLTokenResponse{} - err := acl.TokenRead(&req, &resp) + err = acl.TokenRead(&req, &resp) require.NoError(t, err) if !reflect.DeepEqual(resp.Token, token) { @@ -663,6 +663,44 @@ func TestACLEndpoint_TokenRead(t *testing.T) { } }) + t.Run("expired tokens are filtered", func(t *testing.T) { + // insert a token that will expire + token, err := upsertTestToken(codec, "root", "dc1", func(t *structs.ACLToken) { + t.ExpirationTTL = 20 * time.Millisecond + }) + require.NoError(t, err) + + t.Run("readable until expiration", func(t *testing.T) { + req := structs.ACLTokenGetRequest{ + Datacenter: "dc1", + TokenID: token.AccessorID, + TokenIDType: structs.ACLTokenAccessor, + QueryOptions: structs.QueryOptions{Token: "root"}, + } + + resp := structs.ACLTokenResponse{} + + require.NoError(t, acl.TokenRead(&req, &resp)) + require.Equal(t, token, resp.Token) + }) + + time.Sleep(50 * time.Millisecond) + + t.Run("not returned when expired", func(t *testing.T) { + req := structs.ACLTokenGetRequest{ + Datacenter: "dc1", + TokenID: token.AccessorID, + TokenIDType: structs.ACLTokenAccessor, + QueryOptions: structs.QueryOptions{Token: "root"}, + } + + resp := structs.ACLTokenResponse{} + + require.NoError(t, acl.TokenRead(&req, &resp)) + require.Nil(t, resp.Token) + }) + }) + t.Run("nil when token does not exist", func(t *testing.T) { fakeID, err := uuid.GenerateUUID() require.NoError(t, err) @@ -704,6 +742,8 @@ func TestACLEndpoint_TokenClone(t *testing.T) { c.ACLDatacenter = "dc1" c.ACLsEnabled = true c.ACLMasterToken = "root" + c.ACLTokenMinExpirationTTL = 10 * time.Millisecond + c.ACLTokenMaxExpirationTTL = 5 * time.Second }) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -712,28 +752,52 @@ func TestACLEndpoint_TokenClone(t *testing.T) { testrpc.WaitForLeader(t, s1.RPC, "dc1") - t1, err := upsertTestToken(codec, "root", "dc1") + t1, err := upsertTestToken(codec, "root", "dc1", nil) require.NoError(t, err) - acl := ACL{srv: s1} + endpoint := ACL{srv: s1} - req := structs.ACLTokenSetRequest{ - Datacenter: "dc1", - ACLToken: structs.ACLToken{AccessorID: t1.AccessorID}, - WriteRequest: structs.WriteRequest{Token: "root"}, - } + t.Run("normal", func(t *testing.T) { + req := structs.ACLTokenSetRequest{ + Datacenter: "dc1", + ACLToken: structs.ACLToken{AccessorID: t1.AccessorID}, + WriteRequest: structs.WriteRequest{Token: "root"}, + } - t2 := structs.ACLToken{} + t2 := structs.ACLToken{} - err = acl.TokenClone(&req, &t2) - require.NoError(t, err) + err = endpoint.TokenClone(&req, &t2) + require.NoError(t, err) + + require.Equal(t, t1.Description, t2.Description) + require.Equal(t, t1.Policies, t2.Policies) + require.Equal(t, t1.Rules, t2.Rules) + require.Equal(t, t1.Local, t2.Local) + require.NotEqual(t, t1.AccessorID, t2.AccessorID) + require.NotEqual(t, t1.SecretID, t2.SecretID) + }) + + t.Run("can't clone expired token", func(t *testing.T) { + // insert a token that will expire + t1, err := upsertTestToken(codec, "root", "dc1", func(t *structs.ACLToken) { + t.ExpirationTTL = 11 * time.Millisecond + }) + require.NoError(t, err) + + time.Sleep(30 * time.Millisecond) + + req := structs.ACLTokenSetRequest{ + Datacenter: "dc1", + ACLToken: structs.ACLToken{AccessorID: t1.AccessorID}, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + + t2 := structs.ACLToken{} - require.Equal(t, t1.Description, t2.Description) - require.Equal(t, t1.Policies, t2.Policies) - require.Equal(t, t1.Rules, t2.Rules) - require.Equal(t, t1.Local, t2.Local) - require.NotEqual(t, t1.AccessorID, t2.AccessorID) - require.NotEqual(t, t1.SecretID, t2.SecretID) + err = endpoint.TokenClone(&req, &t2) + require.Error(t, err) + require.Equal(t, acl.ErrNotFound, err) + }) } func TestACLEndpoint_TokenSet(t *testing.T) { @@ -743,6 +807,8 @@ func TestACLEndpoint_TokenSet(t *testing.T) { c.ACLDatacenter = "dc1" c.ACLsEnabled = true c.ACLMasterToken = "root" + c.ACLTokenMinExpirationTTL = 10 * time.Millisecond + c.ACLTokenMaxExpirationTTL = 5 * time.Second }) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -752,6 +818,7 @@ func TestACLEndpoint_TokenSet(t *testing.T) { testrpc.WaitForLeader(t, s1.RPC, "dc1") acl := ACL{srv: s1} + var tokenID string t.Run("Create it", func(t *testing.T) { @@ -806,6 +873,262 @@ func TestACLEndpoint_TokenSet(t *testing.T) { require.Equal(t, token.Description, "new-description") require.Equal(t, token.AccessorID, resp.AccessorID) }) + + t.Run("Create it using Policies linked by id and name", func(t *testing.T) { + policy1, err := upsertTestPolicy(codec, "root", "dc1") + require.NoError(t, err) + policy2, err := upsertTestPolicy(codec, "root", "dc1") + require.NoError(t, err) + + req := structs.ACLTokenSetRequest{ + Datacenter: "dc1", + ACLToken: structs.ACLToken{ + Description: "foobar", + Policies: []structs.ACLTokenPolicyLink{ + structs.ACLTokenPolicyLink{ + ID: policy1.ID, + }, + structs.ACLTokenPolicyLink{ + Name: policy2.Name, + }, + }, + Local: false, + }, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + + resp := structs.ACLToken{} + + err = acl.TokenSet(&req, &resp) + require.NoError(t, err) + + // Delete both policies to ensure that we skip resolving ID->Name + // in the returned data. + require.NoError(t, deleteTestPolicy(codec, "root", "dc1", policy1.ID)) + require.NoError(t, deleteTestPolicy(codec, "root", "dc1", policy2.ID)) + + // Get the token directly to validate that it exists + tokenResp, err := retrieveTestToken(codec, "root", "dc1", resp.AccessorID) + require.NoError(t, err) + token := tokenResp.Token + + require.NotNil(t, token.AccessorID) + require.Equal(t, token.Description, "foobar") + require.Equal(t, token.AccessorID, resp.AccessorID) + + require.Len(t, token.Policies, 0) + }) + + for _, test := range []struct { + name string + offset time.Duration + errString string + errStringTTL string + }{ + {"before create time", -5 * time.Minute, "ExpirationTime cannot be before CreateTime", ""}, + {"too soon", 1 * time.Millisecond, "ExpirationTime cannot be less than", "ExpirationTime cannot be less than"}, + {"too distant", 25 * time.Hour, "ExpirationTime cannot be more than", "ExpirationTime cannot be more than"}, + } { + t.Run("Create it with an expiration time that is "+test.name, func(t *testing.T) { + req := structs.ACLTokenSetRequest{ + Datacenter: "dc1", + ACLToken: structs.ACLToken{ + Description: "foobar", + Policies: nil, + Local: false, + ExpirationTime: time.Now().Add(test.offset), + }, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + + resp := structs.ACLToken{} + + err := acl.TokenSet(&req, &resp) + if test.errString != "" { + requireErrorContains(t, err, test.errString) + } else { + require.NotNil(t, err) + } + }) + + t.Run("Create it with an expiration TTL that is "+test.name, func(t *testing.T) { + req := structs.ACLTokenSetRequest{ + Datacenter: "dc1", + ACLToken: structs.ACLToken{ + Description: "foobar", + Policies: nil, + Local: false, + ExpirationTTL: test.offset, + }, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + + resp := structs.ACLToken{} + + err := acl.TokenSet(&req, &resp) + if test.errString != "" { + requireErrorContains(t, err, test.errStringTTL) + } else { + require.NotNil(t, err) + } + }) + } + + t.Run("Create it with expiration time AND expiration TTL set (error)", func(t *testing.T) { + req := structs.ACLTokenSetRequest{ + Datacenter: "dc1", + ACLToken: structs.ACLToken{ + Description: "foobar", + Policies: nil, + Local: false, + ExpirationTime: time.Now().Add(4 * time.Second), + ExpirationTTL: 4 * time.Second, + }, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + + resp := structs.ACLToken{} + + err := acl.TokenSet(&req, &resp) + requireErrorContains(t, err, "Expiration TTL and Expiration Time cannot both be set") + }) + + t.Run("Create it with expiration time using TTLs", func(t *testing.T) { + req := structs.ACLTokenSetRequest{ + Datacenter: "dc1", + ACLToken: structs.ACLToken{ + Description: "foobar", + Policies: nil, + Local: false, + ExpirationTTL: 4 * time.Second, + }, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + + resp := structs.ACLToken{} + + err := acl.TokenSet(&req, &resp) + require.NoError(t, err) + + // Get the token directly to validate that it exists + tokenResp, err := retrieveTestToken(codec, "root", "dc1", resp.AccessorID) + require.NoError(t, err) + token := tokenResp.Token + + expectExpTime := resp.CreateTime.Add(4 * time.Second) + + require.NotNil(t, token.AccessorID) + require.Equal(t, token.Description, "foobar") + require.Equal(t, token.AccessorID, resp.AccessorID) + requireTimeEquals(t, expectExpTime, resp.ExpirationTime) + + tokenID = token.AccessorID + }) + + var expTime time.Time + t.Run("Create it with expiration time", func(t *testing.T) { + expTime = time.Now().Add(4 * time.Second) + req := structs.ACLTokenSetRequest{ + Datacenter: "dc1", + ACLToken: structs.ACLToken{ + Description: "foobar", + Policies: nil, + Local: false, + ExpirationTime: expTime, + }, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + + resp := structs.ACLToken{} + + err := acl.TokenSet(&req, &resp) + require.NoError(t, err) + + // Get the token directly to validate that it exists + tokenResp, err := retrieveTestToken(codec, "root", "dc1", resp.AccessorID) + require.NoError(t, err) + token := tokenResp.Token + + require.NotNil(t, token.AccessorID) + require.Equal(t, token.Description, "foobar") + require.Equal(t, token.AccessorID, resp.AccessorID) + requireTimeEquals(t, expTime, resp.ExpirationTime) + + tokenID = token.AccessorID + }) + + // do not insert another test at this point: these tests need to be serial + + t.Run("Update expiration time is not allowed", func(t *testing.T) { + req := structs.ACLTokenSetRequest{ + Datacenter: "dc1", + ACLToken: structs.ACLToken{ + Description: "new-description", + AccessorID: tokenID, + ExpirationTime: expTime.Add(-1 * time.Second), + }, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + + resp := structs.ACLToken{} + + err := acl.TokenSet(&req, &resp) + requireErrorContains(t, err, "Cannot change expiration time") + }) + + // do not insert another test at this point: these tests need to be serial + + t.Run("Update anything except expiration time is ok", func(t *testing.T) { + req := structs.ACLTokenSetRequest{ + Datacenter: "dc1", + ACLToken: structs.ACLToken{ + Description: "new-description", + AccessorID: tokenID, + ExpirationTime: expTime, + }, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + + resp := structs.ACLToken{} + + err := acl.TokenSet(&req, &resp) + require.NoError(t, err) + + // Get the token directly to validate that it exists + tokenResp, err := retrieveTestToken(codec, "root", "dc1", resp.AccessorID) + require.NoError(t, err) + token := tokenResp.Token + + require.NotNil(t, token.AccessorID) + require.Equal(t, token.Description, "new-description") + require.Equal(t, token.AccessorID, resp.AccessorID) + requireTimeEquals(t, expTime, resp.ExpirationTime) + }) + + t.Run("cannot update a token that is past its expiration time", func(t *testing.T) { + // create a token that will expire + expiringToken, err := upsertTestToken(codec, "root", "dc1", func(token *structs.ACLToken) { + token.ExpirationTTL = 11 * time.Millisecond + }) + require.NoError(t, err) + + time.Sleep(20 * time.Millisecond) // now 'expiringToken' is expired + + req := structs.ACLTokenSetRequest{ + Datacenter: "dc1", + ACLToken: structs.ACLToken{ + Description: "new-description", + AccessorID: expiringToken.AccessorID, + ExpirationTTL: 4 * time.Second, + }, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + + resp := structs.ACLToken{} + + err = acl.TokenSet(&req, &resp) + requireErrorContains(t, err, "Cannot find token") + }) } func TestACLEndpoint_TokenSet_anon(t *testing.T) { @@ -857,6 +1180,8 @@ func TestACLEndpoint_TokenDelete(t *testing.T) { c.ACLDatacenter = "dc1" c.ACLsEnabled = true c.ACLMasterToken = "root" + c.ACLTokenMinExpirationTTL = 10 * time.Millisecond + c.ACLTokenMaxExpirationTTL = 5 * time.Second }) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -869,6 +1194,8 @@ func TestACLEndpoint_TokenDelete(t *testing.T) { c.ACLDatacenter = "dc1" c.ACLsEnabled = true c.Datacenter = "dc2" + c.ACLTokenMinExpirationTTL = 10 * time.Millisecond + c.ACLTokenMaxExpirationTTL = 5 * time.Second // token replication is required to test deleting non-local tokens in secondary dc c.ACLTokenReplication = true }) @@ -885,12 +1212,74 @@ func TestACLEndpoint_TokenDelete(t *testing.T) { // Try to join joinWAN(t, s2, s1) - existingToken, err := upsertTestToken(codec, "root", "dc1") - require.NoError(t, err) - acl := ACL{srv: s1} acl2 := ACL{srv: s2} + existingToken, err := upsertTestToken(codec, "root", "dc1", nil) + require.NoError(t, err) + + t.Run("deletes a token that has an expiration time in the future", func(t *testing.T) { + // create a token that will expire + testToken, err := upsertTestToken(codec, "root", "dc1", func(token *structs.ACLToken) { + token.ExpirationTTL = 4 * time.Second + }) + require.NoError(t, err) + + // Make sure the token is listable + tokenResp, err := retrieveTestToken(codec, "root", "dc1", testToken.AccessorID) + require.NoError(t, err) + require.NotNil(t, tokenResp.Token) + + // Now try to delete it (this should work). + req := structs.ACLTokenDeleteRequest{ + Datacenter: "dc1", + TokenID: testToken.AccessorID, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + + var resp string + + err = acl.TokenDelete(&req, &resp) + require.NoError(t, err) + + // Make sure the token is gone + tokenResp, err = retrieveTestToken(codec, "root", "dc1", testToken.AccessorID) + require.NoError(t, err) + require.Nil(t, tokenResp.Token) + }) + + t.Run("deletes a token that is past its expiration time", func(t *testing.T) { + // create a token that will expire + expiringToken, err := upsertTestToken(codec, "root", "dc1", func(token *structs.ACLToken) { + token.ExpirationTTL = 11 * time.Millisecond + }) + require.NoError(t, err) + + time.Sleep(20 * time.Millisecond) // now 'expiringToken' is expired + + // Make sure the token is not listable (filtered due to expiry) + tokenResp, err := retrieveTestToken(codec, "root", "dc1", expiringToken.AccessorID) + require.NoError(t, err) + require.Nil(t, tokenResp.Token) + + // Now try to delete it (this should work). + req := structs.ACLTokenDeleteRequest{ + Datacenter: "dc1", + TokenID: expiringToken.AccessorID, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + + var resp string + + err = acl.TokenDelete(&req, &resp) + require.NoError(t, err) + + // Make sure the token is still gone (this time it's actually gone) + tokenResp, err = retrieveTestToken(codec, "root", "dc1", expiringToken.AccessorID) + require.NoError(t, err) + require.Nil(t, tokenResp.Token) + }) + t.Run("deletes a token", func(t *testing.T) { req := structs.ACLTokenDeleteRequest{ Datacenter: "dc1", @@ -919,7 +1308,7 @@ func TestACLEndpoint_TokenDelete(t *testing.T) { var out structs.ACLTokenResponse - err := msgpackrpc.CallWithCodec(codec, "ACL.TokenRead", &readReq, &out) + err := acl.TokenRead(&readReq, &out) require.NoError(t, err) @@ -1019,6 +1408,8 @@ func TestACLEndpoint_TokenList(t *testing.T) { c.ACLDatacenter = "dc1" c.ACLsEnabled = true c.ACLMasterToken = "root" + c.ACLTokenMinExpirationTTL = 10 * time.Millisecond + c.ACLTokenMaxExpirationTTL = 5 * time.Second }) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -1027,31 +1418,74 @@ func TestACLEndpoint_TokenList(t *testing.T) { testrpc.WaitForLeader(t, s1.RPC, "dc1") - t1, err := upsertTestToken(codec, "root", "dc1") + acl := ACL{srv: s1} + + t1, err := upsertTestToken(codec, "root", "dc1", nil) require.NoError(t, err) - t2, err := upsertTestToken(codec, "root", "dc1") + t2, err := upsertTestToken(codec, "root", "dc1", nil) require.NoError(t, err) - acl := ACL{srv: s1} + t3, err := upsertTestToken(codec, "root", "dc1", func(token *structs.ACLToken) { + token.ExpirationTTL = 11 * time.Millisecond + }) + require.NoError(t, err) - req := structs.ACLTokenListRequest{ - Datacenter: "dc1", - QueryOptions: structs.QueryOptions{Token: "root"}, - } + masterTokenAccessorID, err := retrieveTestTokenAccessorForSecret(codec, "root", "dc1", "root") + require.NoError(t, err) - resp := structs.ACLTokenListResponse{} + t.Run("normal", func(t *testing.T) { + req := structs.ACLTokenListRequest{ + Datacenter: "dc1", + QueryOptions: structs.QueryOptions{Token: "root"}, + } - err = acl.TokenList(&req, &resp) - require.NoError(t, err) + resp := structs.ACLTokenListResponse{} + + err = acl.TokenList(&req, &resp) + require.NoError(t, err) - tokens := []string{t1.AccessorID, t2.AccessorID} - var retrievedTokens []string + tokens := []string{ + masterTokenAccessorID, + structs.ACLTokenAnonymousID, + t1.AccessorID, + t2.AccessorID, + t3.AccessorID, + } - for _, v := range resp.Tokens { - retrievedTokens = append(retrievedTokens, v.AccessorID) - } - require.Subset(t, retrievedTokens, tokens) + var retrievedTokens []string + for _, v := range resp.Tokens { + retrievedTokens = append(retrievedTokens, v.AccessorID) + } + require.ElementsMatch(t, retrievedTokens, tokens) + }) + + time.Sleep(20 * time.Millisecond) // now 't3' is expired + + t.Run("filter expired", func(t *testing.T) { + req := structs.ACLTokenListRequest{ + Datacenter: "dc1", + QueryOptions: structs.QueryOptions{Token: "root"}, + } + + resp := structs.ACLTokenListResponse{} + + err = acl.TokenList(&req, &resp) + require.NoError(t, err) + + tokens := []string{ + masterTokenAccessorID, + structs.ACLTokenAnonymousID, + t1.AccessorID, + t2.AccessorID, + } + + var retrievedTokens []string + for _, v := range resp.Tokens { + retrievedTokens = append(retrievedTokens, v.AccessorID) + } + require.ElementsMatch(t, retrievedTokens, tokens) + }) } func TestACLEndpoint_TokenBatchRead(t *testing.T) { @@ -1061,6 +1495,8 @@ func TestACLEndpoint_TokenBatchRead(t *testing.T) { c.ACLDatacenter = "dc1" c.ACLsEnabled = true c.ACLMasterToken = "root" + c.ACLTokenMinExpirationTTL = 10 * time.Millisecond + c.ACLTokenMaxExpirationTTL = 5 * time.Second }) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -1069,32 +1505,64 @@ func TestACLEndpoint_TokenBatchRead(t *testing.T) { testrpc.WaitForLeader(t, s1.RPC, "dc1") - t1, err := upsertTestToken(codec, "root", "dc1") + acl := ACL{srv: s1} + + t1, err := upsertTestToken(codec, "root", "dc1", nil) require.NoError(t, err) - t2, err := upsertTestToken(codec, "root", "dc1") + t2, err := upsertTestToken(codec, "root", "dc1", nil) require.NoError(t, err) - acl := ACL{srv: s1} - tokens := []string{t1.AccessorID, t2.AccessorID} + t3, err := upsertTestToken(codec, "root", "dc1", func(token *structs.ACLToken) { + token.ExpirationTTL = 4 * time.Second + }) + require.NoError(t, err) - req := structs.ACLTokenBatchGetRequest{ - Datacenter: "dc1", - AccessorIDs: tokens, - QueryOptions: structs.QueryOptions{Token: "root"}, - } + t.Run("normal", func(t *testing.T) { + tokens := []string{t1.AccessorID, t2.AccessorID, t3.AccessorID} - resp := structs.ACLTokenBatchResponse{} + req := structs.ACLTokenBatchGetRequest{ + Datacenter: "dc1", + AccessorIDs: tokens, + QueryOptions: structs.QueryOptions{Token: "root"}, + } - err = acl.TokenBatchRead(&req, &resp) - require.NoError(t, err) + resp := structs.ACLTokenBatchResponse{} + + err = acl.TokenBatchRead(&req, &resp) + require.NoError(t, err) - var retrievedTokens []string + var retrievedTokens []string - for _, v := range resp.Tokens { - retrievedTokens = append(retrievedTokens, v.AccessorID) - } - require.EqualValues(t, retrievedTokens, tokens) + for _, v := range resp.Tokens { + retrievedTokens = append(retrievedTokens, v.AccessorID) + } + require.EqualValues(t, retrievedTokens, tokens) + }) + + time.Sleep(20 * time.Millisecond) // now 't3' is expired + + t.Run("returns expired tokens", func(t *testing.T) { + tokens := []string{t1.AccessorID, t2.AccessorID, t3.AccessorID} + + req := structs.ACLTokenBatchGetRequest{ + Datacenter: "dc1", + AccessorIDs: tokens, + QueryOptions: structs.QueryOptions{Token: "root"}, + } + + resp := structs.ACLTokenBatchResponse{} + + err = acl.TokenBatchRead(&req, &resp) + require.NoError(t, err) + + var retrievedTokens []string + + for _, v := range resp.Tokens { + retrievedTokens = append(retrievedTokens, v.AccessorID) + } + require.EqualValues(t, retrievedTokens, tokens) + }) } func TestACLEndpoint_PolicyRead(t *testing.T) { @@ -1419,13 +1887,17 @@ func TestACLEndpoint_PolicyList(t *testing.T) { err = acl.PolicyList(&req, &resp) require.NoError(t, err) - policies := []string{p1.ID, p2.ID} + policies := []string{ + structs.ACLPolicyGlobalManagementID, + p1.ID, + p2.ID, + } var retrievedPolicies []string for _, v := range resp.Policies { retrievedPolicies = append(retrievedPolicies, v.ID) } - require.Subset(t, retrievedPolicies, policies) + require.ElementsMatch(t, retrievedPolicies, policies) } func TestACLEndpoint_PolicyResolve(t *testing.T) { @@ -1491,7 +1963,8 @@ func TestACLEndpoint_PolicyResolve(t *testing.T) { } // upsertTestToken creates a token for testing purposes -func upsertTestToken(codec rpc.ClientCodec, masterToken string, datacenter string) (*structs.ACLToken, error) { +func upsertTestToken(codec rpc.ClientCodec, masterToken string, datacenter string, + tokenModificationFn func(token *structs.ACLToken)) (*structs.ACLToken, error) { arg := structs.ACLTokenSetRequest{ Datacenter: datacenter, ACLToken: structs.ACLToken{ @@ -1502,6 +1975,10 @@ func upsertTestToken(codec rpc.ClientCodec, masterToken string, datacenter strin WriteRequest: structs.WriteRequest{Token: masterToken}, } + if tokenModificationFn != nil { + tokenModificationFn(&arg.ACLToken) + } + var out structs.ACLToken err := msgpackrpc.CallWithCodec(codec, "ACL.TokenSet", &arg, &out) @@ -1517,6 +1994,29 @@ func upsertTestToken(codec rpc.ClientCodec, masterToken string, datacenter strin return &out, nil } +func retrieveTestTokenAccessorForSecret(codec rpc.ClientCodec, masterToken string, datacenter string, id string) (string, error) { + arg := structs.ACLTokenGetRequest{ + TokenID: "root", + TokenIDType: structs.ACLTokenSecret, + Datacenter: "dc1", + QueryOptions: structs.QueryOptions{Token: "root"}, + } + + var out structs.ACLTokenResponse + + err := msgpackrpc.CallWithCodec(codec, "ACL.TokenRead", &arg, &out) + + if err != nil { + return "", err + } + + if out.Token == nil { + return "", nil + } + + return out.Token.AccessorID, nil +} + // retrieveTestToken returns a policy for testing purposes func retrieveTestToken(codec rpc.ClientCodec, masterToken string, datacenter string, id string) (*structs.ACLTokenResponse, error) { arg := structs.ACLTokenGetRequest{ @@ -1537,6 +2037,18 @@ func retrieveTestToken(codec rpc.ClientCodec, masterToken string, datacenter str return &out, nil } +func deleteTestPolicy(codec rpc.ClientCodec, masterToken string, datacenter string, policyID string) error { + arg := structs.ACLPolicyDeleteRequest{ + Datacenter: datacenter, + PolicyID: policyID, + WriteRequest: structs.WriteRequest{Token: masterToken}, + } + + var ignored string + err := msgpackrpc.CallWithCodec(codec, "ACL.PolicyDelete", &arg, &ignored) + return err +} + // upsertTestPolicy creates a policy for testing purposes func upsertTestPolicy(codec rpc.ClientCodec, masterToken string, datacenter string) (*structs.ACLPolicy, error) { // Make sure test policies can't collide @@ -1586,3 +2098,20 @@ func retrieveTestPolicy(codec rpc.ClientCodec, masterToken string, datacenter st return &out, nil } + +func requireTimeEquals(t *testing.T, expect, got time.Time) { + t.Helper() + if !expect.Equal(got) { + t.Fatalf("expected=%q != got=%q", expect, got) + } +} + +func requireErrorContains(t *testing.T, err error, expectedErrorMessage string) { + t.Helper() + if err == nil { + t.Fatal("An error is expected but got nil.") + } + if !strings.Contains(err.Error(), expectedErrorMessage) { + t.Fatalf("unexpected error: %v", err) + } +} diff --git a/agent/consul/acl_replication.go b/agent/consul/acl_replication.go index d691895b673d..047705a60f1d 100644 --- a/agent/consul/acl_replication.go +++ b/agent/consul/acl_replication.go @@ -6,7 +6,7 @@ import ( "fmt" "time" - "github.com/armon/go-metrics" + metrics "github.com/armon/go-metrics" "github.com/hashicorp/consul/agent/structs" ) @@ -468,6 +468,7 @@ func (s *Server) replicateACLTokens(lastRemoteIndex uint64, ctx context.Context) if err != nil { return 0, false, fmt.Errorf("failed to retrieve local ACL tokens: %v", err) } + // Do not filter by expiration times. Wait until the tokens are explicitly deleted. // If the remote index ever goes backwards, it's a good indication that // the remote side was rebuilt and we should do a full sync since we diff --git a/agent/consul/acl_replication_legacy.go b/agent/consul/acl_replication_legacy.go index 182e206208e5..3fc2ce5eb363 100644 --- a/agent/consul/acl_replication_legacy.go +++ b/agent/consul/acl_replication_legacy.go @@ -6,7 +6,7 @@ import ( "sort" "time" - "github.com/armon/go-metrics" + metrics "github.com/armon/go-metrics" "github.com/hashicorp/consul/agent/structs" ) @@ -143,8 +143,13 @@ func (s *Server) fetchLocalLegacyACLs() (structs.ACLs, error) { return nil, err } + now := time.Now() + var acls structs.ACLs for _, token := range local { + if token.IsExpired(now) { + continue + } if acl, err := token.Convert(); err == nil && acl != nil { acls = append(acls, acl) } diff --git a/agent/consul/acl_server.go b/agent/consul/acl_server.go index 1eaf474c2b99..e8213af4fce9 100644 --- a/agent/consul/acl_server.go +++ b/agent/consul/acl_server.go @@ -2,6 +2,7 @@ package consul import ( "sync/atomic" + "time" "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/structs" @@ -29,6 +30,11 @@ var serverACLCacheConfig *structs.ACLCachesConfig = &structs.ACLCachesConfig{ func (s *Server) checkTokenUUID(id string) (bool, error) { state := s.fsm.State() + + // We won't check expiration times here. If we generate a UUID that matches + // a token that hasn't been reaped yet, then we won't be able to insert the + // new token due to a collision. + if _, token, err := state.ACLTokenGetByAccessor(nil, id); err != nil { return false, err } else if token != nil { @@ -145,7 +151,7 @@ func (s *Server) ResolveIdentityFromToken(token string) (bool, structs.ACLIdenti index, aclToken, err := s.fsm.State().ACLTokenGetBySecret(nil, token) if err != nil { return true, nil, err - } else if aclToken != nil { + } else if aclToken != nil && !aclToken.IsExpired(time.Now()) { return true, aclToken, nil } diff --git a/agent/consul/acl_token_exp.go b/agent/consul/acl_token_exp.go new file mode 100644 index 000000000000..f1d5cb52d5cc --- /dev/null +++ b/agent/consul/acl_token_exp.go @@ -0,0 +1,144 @@ +package consul + +import ( + "context" + "fmt" + "time" + + "github.com/hashicorp/consul/agent/structs" + "golang.org/x/time/rate" +) + +func (s *Server) startACLTokenReaping() { + s.aclTokenReapLock.Lock() + defer s.aclTokenReapLock.Unlock() + + if s.aclTokenReapEnabled { + return + } + + ctx, cancel := context.WithCancel(context.Background()) + s.aclTokenReapCancel = cancel + + // Do a quick check for config settings that would imply the goroutine + // below will just spin forever. + // + // We can only check the config settings here that cannot change without a + // restart, so we omit the check for a non-empty replication token as that + // can be changed at runtime. + if !s.InACLDatacenter() && !s.config.ACLTokenReplication { + return + } + + go func() { + limiter := rate.NewLimiter(aclTokenReapingRateLimit, aclTokenReapingBurst) + + for { + if err := limiter.Wait(ctx); err != nil { + return + } + + if s.LocalTokensEnabled() { + if _, err := s.reapExpiredLocalACLTokens(); err != nil { + s.logger.Printf("[ERR] acl: error reaping expired local ACL tokens: %v", err) + } + } + if s.InACLDatacenter() { + if _, err := s.reapExpiredGlobalACLTokens(); err != nil { + s.logger.Printf("[ERR] acl: error reaping expired global ACL tokens: %v", err) + } + } + } + }() + + s.aclTokenReapEnabled = true +} + +func (s *Server) stopACLTokenReaping() { + s.aclTokenReapLock.Lock() + defer s.aclTokenReapLock.Unlock() + + if !s.aclTokenReapEnabled { + return + } + + s.aclTokenReapCancel() + s.aclTokenReapCancel = nil + s.aclTokenReapEnabled = false +} + +func (s *Server) reapExpiredGlobalACLTokens() (int, error) { + return s.reapExpiredACLTokens(false, true) +} +func (s *Server) reapExpiredLocalACLTokens() (int, error) { + return s.reapExpiredACLTokens(true, false) +} +func (s *Server) reapExpiredACLTokens(local, global bool) (int, error) { + if !s.ACLsEnabled() { + return 0, nil + } + if s.UseLegacyACLs() { + return 0, nil + } + if local == global { + return 0, fmt.Errorf("cannot reap both local and global tokens in the same request") + } + + locality := localityName(local) + + minExpiredTime, err := s.fsm.State().ACLTokenMinExpirationTime(local) + if err != nil { + return 0, err + } + + now := time.Now() + + if minExpiredTime.After(now) { + return 0, nil // nothing to do + } + + tokens, _, err := s.fsm.State().ACLTokenListExpired(local, now, aclBatchDeleteSize) + if err != nil { + return 0, err + } + + if len(tokens) == 0 { + return 0, nil + } + + var ( + secretIDs []string + req structs.ACLTokenBatchDeleteRequest + ) + for _, token := range tokens { + if token.Local != local { + return 0, fmt.Errorf("expired index for local=%v returned a mismatched token with local=%v: %s", local, token.Local, token.AccessorID) + } + req.TokenIDs = append(req.TokenIDs, token.AccessorID) + secretIDs = append(secretIDs, token.SecretID) + } + + s.logger.Printf("[INFO] acl: deleting %d expired %s tokens", len(req.TokenIDs), locality) + resp, err := s.raftApply(structs.ACLTokenDeleteRequestType, &req) + if err != nil { + return 0, fmt.Errorf("Failed to apply token expiration deletions: %v", err) + } + + // Purge the identities from the cache + for _, secretID := range secretIDs { + s.acls.cache.RemoveIdentity(secretID) + } + + if respErr, ok := resp.(error); ok { + return 0, respErr + } + + return len(req.TokenIDs), nil +} + +func localityName(local bool) string { + if local { + return "local" + } + return "global" +} diff --git a/agent/consul/acl_token_exp_test.go b/agent/consul/acl_token_exp_test.go new file mode 100644 index 000000000000..20ae878afcb8 --- /dev/null +++ b/agent/consul/acl_token_exp_test.go @@ -0,0 +1,219 @@ +package consul + +import ( + "os" + "testing" + "time" + + "github.com/hashicorp/consul/agent/structs" + "github.com/hashicorp/consul/testrpc" + "github.com/stretchr/testify/require" +) + +func TestACLTokenReap_Primary(t *testing.T) { + t.Parallel() + + t.Run("global", func(t *testing.T) { + t.Parallel() + testACLTokenReap_Primary(t, false, true) + }) + t.Run("local", func(t *testing.T) { + t.Parallel() + testACLTokenReap_Primary(t, true, false) + }) +} + +func testACLTokenReap_Primary(t *testing.T, local, global bool) { + // ------------------------------------------- + // A word of caution when testing reapExpiredACLTokens(): + // + // The underlying memdb index used for reaping has a minimum granularity of + // 1 second as it delegates to `time.Unix()`. This test will have to be + // deliberately slow to allow for necessary sleeps. If you try to make it + // operate faster (using expiration ttls of milliseconds) it will be flaky. + // ------------------------------------------- + + t.Helper() + require.NotEqual(t, local, global) + + dir1, s1 := testServerWithConfig(t, func(c *Config) { + c.ACLDatacenter = "dc1" + c.ACLsEnabled = true + c.ACLMasterToken = "root" + c.ACLTokenMinExpirationTTL = 10 * time.Millisecond + c.ACLTokenMaxExpirationTTL = 8 * time.Second + }) + defer os.RemoveAll(dir1) + defer s1.Shutdown() + + testrpc.WaitForLeader(t, s1.RPC, "dc1") + + codec := rpcClient(t, s1) + defer codec.Close() + + acl := ACL{s1} + + masterTokenAccessorID, err := retrieveTestTokenAccessorForSecret(codec, "root", "dc1", "root") + require.NoError(t, err) + + listTokens := func() (localTokens, globalTokens []string, err error) { + req := structs.ACLTokenListRequest{ + Datacenter: "dc1", + QueryOptions: structs.QueryOptions{Token: "root"}, + } + + var res structs.ACLTokenListResponse + err = acl.TokenList(&req, &res) + if err != nil { + return nil, nil, err + } + + for _, tok := range res.Tokens { + if tok.Local { + localTokens = append(localTokens, tok.AccessorID) + } else { + globalTokens = append(globalTokens, tok.AccessorID) + } + } + + return localTokens, globalTokens, nil + } + + requireTokenMatch := func(t *testing.T, expect []string) { + t.Helper() + + var expectLocal, expectGlobal []string + // The master token and the anonymous token are always going to be + // present and global. + expectGlobal = append(expectGlobal, masterTokenAccessorID) + expectGlobal = append(expectGlobal, structs.ACLTokenAnonymousID) + + if local { + expectLocal = append(expectLocal, expect...) + } else { + expectGlobal = append(expectGlobal, expect...) + } + + localTokens, globalTokens, err := listTokens() + require.NoError(t, err) + require.ElementsMatch(t, expectLocal, localTokens) + require.ElementsMatch(t, expectGlobal, globalTokens) + } + + // initial sanity check + requireTokenMatch(t, []string{}) + + t.Run("no tokens", func(t *testing.T) { + n, err := s1.reapExpiredACLTokens(local, global) + require.NoError(t, err) + require.Equal(t, 0, n) + + requireTokenMatch(t, []string{}) + }) + + // 2 normal + token1, err := upsertTestToken(codec, "root", "dc1", func(token *structs.ACLToken) { + token.Local = local + }) + require.NoError(t, err) + token2, err := upsertTestToken(codec, "root", "dc1", func(token *structs.ACLToken) { + token.Local = local + }) + require.NoError(t, err) + + requireTokenMatch(t, []string{ + token1.AccessorID, + token2.AccessorID, + }) + + t.Run("only normal tokens", func(t *testing.T) { + n, err := s1.reapExpiredACLTokens(local, global) + require.NoError(t, err) + require.Equal(t, 0, n) + + requireTokenMatch(t, []string{ + token1.AccessorID, + token2.AccessorID, + }) + }) + + // 2 expiring + token3, err := upsertTestToken(codec, "root", "dc1", func(token *structs.ACLToken) { + token.ExpirationTTL = 1 * time.Second + token.Local = local + }) + require.NoError(t, err) + token4, err := upsertTestToken(codec, "root", "dc1", func(token *structs.ACLToken) { + token.ExpirationTTL = 5 * time.Second + token.Local = local + }) + require.NoError(t, err) + + // 2 more normal + token5, err := upsertTestToken(codec, "root", "dc1", func(token *structs.ACLToken) { + token.Local = local + }) + require.NoError(t, err) + token6, err := upsertTestToken(codec, "root", "dc1", func(token *structs.ACLToken) { + token.Local = local + }) + require.NoError(t, err) + + requireTokenMatch(t, []string{ + token1.AccessorID, + token2.AccessorID, + token3.AccessorID, + token4.AccessorID, + token5.AccessorID, + token6.AccessorID, + }) + + t.Run("mixed but nothing expired yet", func(t *testing.T) { + n, err := s1.reapExpiredACLTokens(local, global) + require.NoError(t, err) + require.Equal(t, 0, n) + + requireTokenMatch(t, []string{ + token1.AccessorID, + token2.AccessorID, + token3.AccessorID, + token4.AccessorID, + token5.AccessorID, + token6.AccessorID, + }) + }) + + time.Sleep(token3.ExpirationTime.Sub(time.Now()) + 10*time.Millisecond) + + t.Run("one should be reaped", func(t *testing.T) { + n, err := s1.reapExpiredACLTokens(local, global) + require.NoError(t, err) + require.Equal(t, 1, n) + + requireTokenMatch(t, []string{ + token1.AccessorID, + token2.AccessorID, + // token3.AccessorID, + token4.AccessorID, + token5.AccessorID, + token6.AccessorID, + }) + }) + + time.Sleep(token4.ExpirationTime.Sub(time.Now()) + 10*time.Millisecond) + + t.Run("two should be reaped", func(t *testing.T) { + n, err := s1.reapExpiredACLTokens(local, global) + require.NoError(t, err) + require.Equal(t, 1, n) + + requireTokenMatch(t, []string{ + token1.AccessorID, + token2.AccessorID, + // token3.AccessorID, + // token4.AccessorID, + token5.AccessorID, + token6.AccessorID, + }) + }) +} diff --git a/agent/consul/config.go b/agent/consul/config.go index baf9e63a29dc..cce70f392b13 100644 --- a/agent/consul/config.go +++ b/agent/consul/config.go @@ -313,6 +313,16 @@ type Config struct { // Minimum Session TTL SessionTTLMin time.Duration + // maxTokenExpirationDuration is the maximum difference allowed between + // ACLToken CreateTime and ExpirationTime values if ExpirationTime is set + // on a token. + ACLTokenMaxExpirationTTL time.Duration + + // ACLTokenMinExpirationTTL is the minimum difference allowed between + // ACLToken CreateTime and ExpirationTime values if ExpirationTime is set + // on a token. + ACLTokenMinExpirationTTL time.Duration + // ServerUp callback can be used to trigger a notification that // a Consul server is now up and known about. ServerUp func() @@ -473,6 +483,8 @@ func DefaultConfig() *Config { TombstoneTTL: 15 * time.Minute, TombstoneTTLGranularity: 30 * time.Second, SessionTTLMin: 10 * time.Second, + ACLTokenMinExpirationTTL: 1 * time.Minute, + ACLTokenMaxExpirationTTL: 24 * time.Hour, // These are tuned to provide a total throughput of 128 updates // per second. If you update these, you should update the client- diff --git a/agent/consul/fsm/commands_oss.go b/agent/consul/fsm/commands_oss.go index 1e3651cba461..f9d75e83c84a 100644 --- a/agent/consul/fsm/commands_oss.go +++ b/agent/consul/fsm/commands_oss.go @@ -4,7 +4,7 @@ import ( "fmt" "time" - "github.com/armon/go-metrics" + metrics "github.com/armon/go-metrics" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/api" ) @@ -165,6 +165,7 @@ func (c *FSM) applyACLOperation(buf []byte, index uint64) interface{} { return err } + // No need to check expiration times as those did not exist in legacy tokens. if _, token, err := c.state.ACLTokenGetBySecret(nil, req.ACL.ID); err != nil { return err } else { diff --git a/agent/consul/fsm/snapshot_oss.go b/agent/consul/fsm/snapshot_oss.go index 0c7713753e87..4195b8c422e3 100644 --- a/agent/consul/fsm/snapshot_oss.go +++ b/agent/consul/fsm/snapshot_oss.go @@ -178,6 +178,8 @@ func (s *snapshot) persistACLs(sink raft.SnapshotSink, return err } + // Don't check expiration times. Wait for explicit deletions. + for token := tokens.Next(); token != nil; token = tokens.Next() { if _, err := sink.Write([]byte{byte(structs.ACLTokenSetRequestType)}); err != nil { return err diff --git a/agent/consul/leader.go b/agent/consul/leader.go index 3264892095c5..d42fddd7983c 100644 --- a/agent/consul/leader.go +++ b/agent/consul/leader.go @@ -10,7 +10,7 @@ import ( "sync/atomic" "time" - "github.com/armon/go-metrics" + metrics "github.com/armon/go-metrics" "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/connect" ca "github.com/hashicorp/consul/agent/connect/ca" @@ -22,7 +22,7 @@ import ( "github.com/hashicorp/consul/types" memdb "github.com/hashicorp/go-memdb" uuid "github.com/hashicorp/go-uuid" - "github.com/hashicorp/go-version" + version "github.com/hashicorp/go-version" "github.com/hashicorp/raft" "github.com/hashicorp/serf/serf" "golang.org/x/time/rate" @@ -308,6 +308,8 @@ func (s *Server) revokeLeadership() error { s.setCAProvider(nil, nil) + s.stopACLTokenReaping() + s.stopACLUpgrade() s.resetConsistentReadReady() @@ -329,6 +331,7 @@ func (s *Server) initializeLegacyACL() error { if err != nil { return fmt.Errorf("failed to get anonymous token: %v", err) } + // Ignoring expiration times to avoid an insertion collision. if token == nil { req := structs.ACLRequest{ Datacenter: authDC, @@ -352,6 +355,7 @@ func (s *Server) initializeLegacyACL() error { if err != nil { return fmt.Errorf("failed to get master token: %v", err) } + // Ignoring expiration times to avoid an insertion collision. if token == nil { req := structs.ACLRequest{ Datacenter: authDC, @@ -482,6 +486,7 @@ func (s *Server) initializeACLs(upgrade bool) error { if err != nil { return fmt.Errorf("failed to get master token: %v", err) } + // Ignoring expiration times to avoid an insertion collision. if token == nil { accessor, err := lib.GenerateUUID(s.checkTokenUUID) if err != nil { @@ -543,6 +548,7 @@ func (s *Server) initializeACLs(upgrade bool) error { if err != nil { return fmt.Errorf("failed to get anonymous token: %v", err) } + // Ignoring expiration times to avoid an insertion collision. if token == nil { // DEPRECATED (ACL-Legacy-Compat) - Don't need to query for previous "anonymous" token // check for legacy token that needs an upgrade @@ -550,6 +556,7 @@ func (s *Server) initializeACLs(upgrade bool) error { if err != nil { return fmt.Errorf("failed to get anonymous token: %v", err) } + // Ignoring expiration times to avoid an insertion collision. // the token upgrade routine will take care of upgrading the token if a legacy version exists if legacyToken == nil { @@ -572,6 +579,7 @@ func (s *Server) initializeACLs(upgrade bool) error { s.logger.Printf("[INFO] consul: Created ACL anonymous token from configuration") } } + // launch the upgrade go routine to generate accessors for everything s.startACLUpgrade() } else { if s.UseLegacyACLs() && !upgrade { @@ -588,7 +596,7 @@ func (s *Server) initializeACLs(upgrade bool) error { s.startACLReplication() } - // launch the upgrade go routine to generate accessors for everything + s.startACLTokenReaping() return nil } @@ -617,6 +625,7 @@ func (s *Server) startACLUpgrade() { if err != nil { s.logger.Printf("[WARN] acl: encountered an error while searching for tokens without accessor ids: %v", err) } + // No need to check expiration time here, as that only exists for v2 tokens. if len(tokens) == 0 { ws := memdb.NewWatchSet() @@ -797,10 +806,10 @@ func (s *Server) startACLReplication() { if s.config.ACLTokenReplication { replicationType = structs.ACLReplicateTokens - go func() { var failedAttempts uint limiter := rate.NewLimiter(rate.Limit(s.config.ACLReplicationRate), s.config.ACLReplicationBurst) + var lastRemoteIndex uint64 for { if err := limiter.Wait(ctx); err != nil { diff --git a/agent/consul/server.go b/agent/consul/server.go index f19a90745542..b44b417816d0 100644 --- a/agent/consul/server.go +++ b/agent/consul/server.go @@ -109,6 +109,12 @@ type Server struct { aclReplicationLock sync.RWMutex aclReplicationEnabled bool + // aclTokenReapCancel is used to shut down the ACL Token expiration reap + // goroutine when we lose leadership. + aclTokenReapCancel context.CancelFunc + aclTokenReapLock sync.RWMutex + aclTokenReapEnabled bool + // DEPRECATED (ACL-Legacy-Compat) - only needed while we support both // useNewACLs is used to determine whether we can use new ACLs or not useNewACLs int32 diff --git a/agent/consul/state/acl.go b/agent/consul/state/acl.go index 5c2e83d57fa8..36c0a03e4692 100644 --- a/agent/consul/state/acl.go +++ b/agent/consul/state/acl.go @@ -1,10 +1,12 @@ package state import ( + "encoding/binary" "fmt" + "time" "github.com/hashicorp/consul/agent/structs" - "github.com/hashicorp/go-memdb" + memdb "github.com/hashicorp/go-memdb" ) type TokenPoliciesIndex struct { @@ -58,6 +60,54 @@ func (s *TokenPoliciesIndex) PrefixFromArgs(args ...interface{}) ([]byte, error) return val, nil } +type TokenExpirationIndex struct { + LocalFilter bool +} + +func (s *TokenExpirationIndex) encodeTime(t time.Time) []byte { + val := t.Unix() + buf := make([]byte, 8) + binary.BigEndian.PutUint64(buf, uint64(val)) + return buf +} + +func (s *TokenExpirationIndex) FromObject(obj interface{}) (bool, []byte, error) { + token, ok := obj.(*structs.ACLToken) + if !ok { + return false, nil, fmt.Errorf("object is not an ACLToken") + } + if s.LocalFilter != token.Local { + return false, nil, nil + } + if token.ExpirationTime.IsZero() { + return false, nil, nil + } + if token.ExpirationTime.Unix() < 0 { + return false, nil, fmt.Errorf("token expiration time cannot be before the unix epoch: %s", token.ExpirationTime) + } + + buf := s.encodeTime(token.ExpirationTime) + + return true, buf, nil +} + +func (s *TokenExpirationIndex) FromArgs(args ...interface{}) ([]byte, error) { + if len(args) != 1 { + return nil, fmt.Errorf("must provide only a single argument") + } + arg, ok := args[0].(time.Time) + if !ok { + return nil, fmt.Errorf("argument must be a time.Time: %#v", args[0]) + } + if arg.Unix() < 0 { + return nil, fmt.Errorf("argument must be a time.Time after the unix epoch: %s", args[0]) + } + + buf := s.encodeTime(arg) + + return buf, nil +} + func tokensTableSchema() *memdb.TableSchema { return &memdb.TableSchema{ Name: "acl-tokens", @@ -100,6 +150,18 @@ func tokensTableSchema() *memdb.TableSchema { }, }, }, + "expires-global": { + Name: "expires-global", + AllowMissing: true, + Unique: false, + Indexer: &TokenExpirationIndex{LocalFilter: false}, + }, + "expires-local": { + Name: "expires-local", + AllowMissing: true, + Unique: false, + Indexer: &TokenExpirationIndex{LocalFilter: true}, + }, //DEPRECATED (ACL-Legacy-Compat) - This index is only needed while we support upgrading v1 to v2 acls // This table indexes all the ACL tokens that do not have an AccessorID @@ -405,7 +467,7 @@ func (s *Store) aclTokenSetTxn(tx *memdb.Txn, idx uint64, token *structs.ACLToke } if legacy && original != nil { - if len(original.Policies) > 0 || original.Type == "" { + if original.UsesNonLegacyFields() { return fmt.Errorf("failed inserting acl token: cannot use legacy endpoint to modify a non-legacy token") } @@ -586,6 +648,63 @@ func (s *Store) ACLTokenListUpgradeable(max int) (structs.ACLTokens, <-chan stru return tokens, iter.WatchCh(), nil } +func (s *Store) ACLTokenMinExpirationTime(local bool) (time.Time, error) { + tx := s.db.Txn(false) + defer tx.Abort() + + item, err := tx.First("acl-tokens", s.expiresIndexName(local)) + if err != nil { + return time.Time{}, fmt.Errorf("failed acl token listing: %v", err) + } + + if item == nil { + return time.Time{}, nil + } + + token := item.(*structs.ACLToken) + + return token.ExpirationTime, nil +} + +// ACLTokenListExpires lists tokens that are expires as of the provided time. +// The returned set will be no larger than the max value provided. +func (s *Store) ACLTokenListExpired(local bool, asOf time.Time, max int) (structs.ACLTokens, <-chan struct{}, error) { + tx := s.db.Txn(false) + defer tx.Abort() + + iter, err := tx.Get("acl-tokens", s.expiresIndexName(local)) + if err != nil { + return nil, nil, fmt.Errorf("failed acl token listing: %v", err) + } + + var ( + tokens structs.ACLTokens + i int + ) + for raw := iter.Next(); raw != nil; raw = iter.Next() { + token := raw.(*structs.ACLToken) + + if !token.ExpirationTime.Before(asOf) { + return tokens, nil, nil + } + + tokens = append(tokens, token) + i += 1 + if i >= max { + return tokens, nil, nil + } + } + + return tokens, iter.WatchCh(), nil +} + +func (s *Store) expiresIndexName(local bool) string { + if local { + return "expires-local" + } + return "expires-global" +} + // ACLTokenDeleteBySecret is used to remove an existing ACL from the state store. If // the ACL does not exist this is a no-op and no error is returned. func (s *Store) ACLTokenDeleteBySecret(idx uint64, secret string) error { diff --git a/agent/consul/state/acl_test.go b/agent/consul/state/acl_test.go index b6546b5bda1e..0e01fbcfe3df 100644 --- a/agent/consul/state/acl_test.go +++ b/agent/consul/state/acl_test.go @@ -1,11 +1,16 @@ package state import ( + "math/rand" + "strconv" "testing" "time" "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/structs" + "github.com/hashicorp/consul/lib" + memdb "github.com/hashicorp/go-memdb" + "github.com/hashicorp/go-uuid" "github.com/stretchr/testify/require" ) @@ -47,6 +52,13 @@ func setupExtraPolicies(t *testing.T, s *Store) { Rules: `node_prefix "" { policy = "read" }`, Syntax: acl.SyntaxCurrent, }, + &structs.ACLPolicy{ + ID: "9386ecae-6677-4686-bcd4-5ab9d86cca1d", + Name: "agent-read", + Description: "Allows reading all node information", + Rules: `agent_prefix "" { policy = "read" }`, + Syntax: acl.SyntaxCurrent, + }, } for _, policy := range policies { @@ -94,23 +106,6 @@ func TestStateStore_ACLBootstrap(t *testing.T) { Type: structs.ACLTokenTypeManagement, } - stripIrrelevantFields := func(token *structs.ACLToken) *structs.ACLToken { - tokenCopy := token.Clone() - // When comparing the tokens disregard the policy link names. This - // data is not cleanly updated in a variety of scenarios and should not - // be relied upon. - for i, _ := range tokenCopy.Policies { - tokenCopy.Policies[i].Name = "" - } - // The raft indexes won't match either because the requester will not - // have access to that. - tokenCopy.RaftIndex = structs.RaftIndex{} - return tokenCopy - } - compareTokens := func(expected, actual *structs.ACLToken) { - require.Equal(t, stripIrrelevantFields(expected), stripIrrelevantFields(actual)) - } - s := testStateStore(t) setupGlobalManagement(t, s) @@ -143,7 +138,7 @@ func TestStateStore_ACLBootstrap(t *testing.T) { _, tokens, err := s.ACLTokenList(nil, true, true, "") require.NoError(t, err) require.Len(t, tokens, 1) - compareTokens(token1, tokens[0]) + compareTokens(t, token1, tokens[0]) // bootstrap reset err = s.ACLBootstrap(32, index-1, token2.Clone(), false) @@ -175,10 +170,10 @@ func TestStateStore_ACLToken_SetGet_Legacy(t *testing.T) { }, } - require.NoError(t, s.ACLTokenSet(2, token, false)) + require.NoError(t, s.ACLTokenSet(2, token.Clone(), false)) // legacy flag is set so it should disallow setting this token - err := s.ACLTokenSet(3, token, true) + err := s.ACLTokenSet(3, token.Clone(), true) require.Error(t, err) }) @@ -190,10 +185,10 @@ func TestStateStore_ACLToken_SetGet_Legacy(t *testing.T) { SecretID: "c0056225-5785-43b3-9b77-3954f06d6aee", } - require.NoError(t, s.ACLTokenSet(2, token, false)) + require.NoError(t, s.ACLTokenSet(2, token.Clone(), false)) // legacy flag is set so it should disallow setting this token - err := s.ACLTokenSet(3, token, true) + err := s.ACLTokenSet(3, token.Clone(), true) require.Error(t, err) }) @@ -206,7 +201,7 @@ func TestStateStore_ACLToken_SetGet_Legacy(t *testing.T) { Rules: `service "" { policy = "read" }`, } - require.NoError(t, s.ACLTokenSet(2, token, true)) + require.NoError(t, s.ACLTokenSet(2, token.Clone(), true)) idx, rtoken, err := s.ACLTokenGetBySecret(nil, token.SecretID) require.NoError(t, err) @@ -230,7 +225,7 @@ func TestStateStore_ACLToken_SetGet_Legacy(t *testing.T) { Rules: `service "" { policy = "read" }`, } - require.NoError(t, s.ACLTokenSet(2, original, true)) + require.NoError(t, s.ACLTokenSet(2, original.Clone(), true)) updatedRules := `service "" { policy = "read" } service "foo" { policy = "deny"}` update := &structs.ACLToken{ @@ -239,7 +234,7 @@ func TestStateStore_ACLToken_SetGet_Legacy(t *testing.T) { Rules: updatedRules, } - require.NoError(t, s.ACLTokenSet(3, update, true)) + require.NoError(t, s.ACLTokenSet(3, update.Clone(), true)) idx, rtoken, err := s.ACLTokenGetBySecret(nil, original.SecretID) require.NoError(t, err) @@ -265,7 +260,7 @@ func TestStateStore_ACLToken_SetGet(t *testing.T) { AccessorID: "39171632-6f34-4411-827f-9416403687f4", } - err := s.ACLTokenSet(2, token, false) + err := s.ACLTokenSet(2, token.Clone(), false) require.Error(t, err) require.Equal(t, ErrMissingACLTokenSecret, err) }) @@ -277,7 +272,7 @@ func TestStateStore_ACLToken_SetGet(t *testing.T) { SecretID: "39171632-6f34-4411-827f-9416403687f4", } - err := s.ACLTokenSet(2, token, false) + err := s.ACLTokenSet(2, token.Clone(), false) require.Error(t, err) require.Equal(t, ErrMissingACLTokenAccessor, err) }) @@ -295,7 +290,7 @@ func TestStateStore_ACLToken_SetGet(t *testing.T) { }, } - err := s.ACLTokenSet(2, token, false) + err := s.ACLTokenSet(2, token.Clone(), false) require.Error(t, err) }) @@ -312,7 +307,7 @@ func TestStateStore_ACLToken_SetGet(t *testing.T) { }, } - err := s.ACLTokenSet(2, token, false) + err := s.ACLTokenSet(2, token.Clone(), false) require.Error(t, err) }) @@ -329,13 +324,12 @@ func TestStateStore_ACLToken_SetGet(t *testing.T) { }, } - require.NoError(t, s.ACLTokenSet(2, token, false)) + require.NoError(t, s.ACLTokenSet(2, token.Clone(), false)) idx, rtoken, err := s.ACLTokenGetByAccessor(nil, "daf37c07-d04d-4fd5-9678-a8206a57d61a") require.NoError(t, err) require.Equal(t, uint64(2), idx) - // pointer equality - require.True(t, rtoken == token) + compareTokens(t, token, rtoken) require.Equal(t, uint64(2), rtoken.CreateIndex) require.Equal(t, uint64(2), rtoken.ModifyIndex) require.Len(t, rtoken.Policies, 1) @@ -355,7 +349,7 @@ func TestStateStore_ACLToken_SetGet(t *testing.T) { }, } - require.NoError(t, s.ACLTokenSet(2, token, false)) + require.NoError(t, s.ACLTokenSet(2, token.Clone(), false)) updated := &structs.ACLToken{ AccessorID: "daf37c07-d04d-4fd5-9678-a8206a57d61a", @@ -367,13 +361,12 @@ func TestStateStore_ACLToken_SetGet(t *testing.T) { }, } - require.NoError(t, s.ACLTokenSet(3, updated, false)) + require.NoError(t, s.ACLTokenSet(3, updated.Clone(), false)) idx, rtoken, err := s.ACLTokenGetByAccessor(nil, "daf37c07-d04d-4fd5-9678-a8206a57d61a") require.NoError(t, err) require.Equal(t, uint64(3), idx) - // pointer equality - require.True(t, rtoken == updated) + compareTokens(t, updated, rtoken) require.Equal(t, uint64(2), rtoken.CreateIndex) require.Equal(t, uint64(3), rtoken.ModifyIndex) require.Len(t, rtoken.Policies, 1) @@ -945,7 +938,7 @@ func TestStateStore_ACLToken_Delete(t *testing.T) { Local: true, } - require.NoError(t, s.ACLTokenSet(2, token, false)) + require.NoError(t, s.ACLTokenSet(2, token.Clone(), false)) _, rtoken, err := s.ACLTokenGetByAccessor(nil, "f1093997-b6c7-496d-bfb8-6b1b1895641b") require.NoError(t, err) @@ -973,7 +966,7 @@ func TestStateStore_ACLToken_Delete(t *testing.T) { Local: true, } - require.NoError(t, s.ACLTokenSet(2, token, false)) + require.NoError(t, s.ACLTokenSet(2, token.Clone(), false)) _, rtoken, err := s.ACLTokenGetByAccessor(nil, "f1093997-b6c7-496d-bfb8-6b1b1895641b") require.NoError(t, err) @@ -1183,7 +1176,7 @@ func TestStateStore_ACLPolicy_SetGet(t *testing.T) { // this creates the node read policy which we can update s := testACLTokensStateStore(t) - update := structs.ACLPolicy{ + update := &structs.ACLPolicy{ ID: "a0625e95-9b3e-42de-a8d6-ceef5b6f3286", Name: "node-read-modified", Description: "Modified", @@ -1192,19 +1185,29 @@ func TestStateStore_ACLPolicy_SetGet(t *testing.T) { Datacenters: []string{"dc1", "dc2"}, } - require.NoError(t, s.ACLPolicySet(3, &update)) + require.NoError(t, s.ACLPolicySet(3, update.Clone())) + + expect := update.Clone() + expect.CreateIndex = 2 + expect.ModifyIndex = 3 + // policy found via id idx, rpolicy, err := s.ACLPolicyGetByID(nil, "a0625e95-9b3e-42de-a8d6-ceef5b6f3286") + require.NoError(t, err) + require.Equal(t, uint64(3), idx) + require.Equal(t, expect, rpolicy) + + // policy no longer found via old name + idx, rpolicy, err = s.ACLPolicyGetByName(nil, "node-read") require.Equal(t, uint64(3), idx) require.NoError(t, err) - require.NotNil(t, rpolicy) - require.Equal(t, "node-read-modified", rpolicy.Name) - require.Equal(t, "Modified", rpolicy.Description) - require.Equal(t, `node_prefix "" { policy = "read" } node "secret" { policy = "deny" }`, rpolicy.Rules) - require.Equal(t, acl.SyntaxCurrent, rpolicy.Syntax) - require.ElementsMatch(t, []string{"dc1", "dc2"}, rpolicy.Datacenters) - require.Equal(t, uint64(2), rpolicy.CreateIndex) - require.Equal(t, uint64(3), rpolicy.ModifyIndex) + require.Nil(t, rpolicy) + + // policy is found via new name + idx, rpolicy, err = s.ACLPolicyGetByName(nil, "node-read-modified") + require.NoError(t, err) + require.Equal(t, uint64(3), idx) + require.Equal(t, expect, rpolicy) }) } @@ -1632,3 +1635,166 @@ func TestStateStore_ACLPolicies_Snapshot_Restore(t *testing.T) { require.Equal(t, uint64(2), s.maxIndex("acl-policies")) }() } + +func TestTokenPoliciesIndex(t *testing.T) { + lib.SeedMathRand() + + idIndex := &memdb.IndexSchema{ + Name: "id", + AllowMissing: false, + Unique: true, + Indexer: &memdb.StringFieldIndex{Field: "AccessorID", Lowercase: false}, + } + globalIndex := &memdb.IndexSchema{ + Name: "global", + AllowMissing: true, + Unique: false, + Indexer: &TokenExpirationIndex{LocalFilter: false}, + } + localIndex := &memdb.IndexSchema{ + Name: "local", + AllowMissing: true, + Unique: false, + Indexer: &TokenExpirationIndex{LocalFilter: true}, + } + schema := &memdb.DBSchema{ + Tables: map[string]*memdb.TableSchema{ + "test": &memdb.TableSchema{ + Name: "test", + Indexes: map[string]*memdb.IndexSchema{ + "id": idIndex, + "global": globalIndex, + "local": localIndex, + }, + }, + }, + } + + knownUUIDs := make(map[string]struct{}) + newUUID := func() string { + for { + ret, err := uuid.GenerateUUID() + require.NoError(t, err) + if _, ok := knownUUIDs[ret]; !ok { + knownUUIDs[ret] = struct{}{} + return ret + } + } + } + + baseTime := time.Date(2010, 12, 31, 11, 30, 7, 0, time.UTC) + + newToken := func(local bool, desc string, expTime time.Time) *structs.ACLToken { + return &structs.ACLToken{ + AccessorID: newUUID(), + SecretID: newUUID(), + Description: desc, + Local: local, + ExpirationTime: expTime, + CreateTime: baseTime, + RaftIndex: structs.RaftIndex{ + CreateIndex: 9, + ModifyIndex: 10, + }, + } + } + + db, err := memdb.NewMemDB(schema) + require.NoError(t, err) + + dumpItems := func(index string) ([]string, error) { + tx := db.Txn(false) + defer tx.Abort() + + iter, err := tx.Get("test", index) + if err != nil { + return nil, err + } + + var out []string + for raw := iter.Next(); raw != nil; raw = iter.Next() { + tok := raw.(*structs.ACLToken) + out = append(out, tok.Description) + } + + return out, nil + } + + { // insert things with no expiration time + tx := db.Txn(true) + for i := 0; i < 10; i++ { + tok := newToken(i%2 != 1, "tok["+strconv.Itoa(i)+"]", time.Time{}) + + require.NoError(t, tx.Insert("test", tok)) + } + tx.Commit() + } + + t.Run("no expiration", func(t *testing.T) { + dump, err := dumpItems("local") + require.NoError(t, err) + require.Len(t, dump, 0) + + dump, err = dumpItems("global") + require.NoError(t, err) + require.Len(t, dump, 0) + }) + + { // insert things with laddered expiration time, inserted in random order + var tokens []*structs.ACLToken + for i := 0; i < 10; i++ { + expTime := baseTime.Add(time.Duration(i+1) * time.Minute) + tok := newToken(i%2 == 0, "exp-tok["+strconv.Itoa(i)+"]", expTime) + tokens = append(tokens, tok) + } + rand.Shuffle(len(tokens), func(i, j int) { + tokens[i], tokens[j] = tokens[j], tokens[i] + }) + + tx := db.Txn(true) + for _, tok := range tokens { + require.NoError(t, tx.Insert("test", tok)) + } + tx.Commit() + } + + t.Run("mixed expiration", func(t *testing.T) { + dump, err := dumpItems("local") + require.NoError(t, err) + require.ElementsMatch(t, []string{ + "exp-tok[0]", + "exp-tok[2]", + "exp-tok[4]", + "exp-tok[6]", + "exp-tok[8]", + }, dump) + + dump, err = dumpItems("global") + require.NoError(t, err) + require.ElementsMatch(t, []string{ + "exp-tok[1]", + "exp-tok[3]", + "exp-tok[5]", + "exp-tok[7]", + "exp-tok[9]", + }, dump) + }) +} + +func stripIrrelevantTokenFields(token *structs.ACLToken) *structs.ACLToken { + tokenCopy := token.Clone() + // When comparing the tokens disregard the policy link names. This + // data is not cleanly updated in a variety of scenarios and should not + // be relied upon. + for i, _ := range tokenCopy.Policies { + tokenCopy.Policies[i].Name = "" + } + // The raft indexes won't match either because the requester will not + // have access to that. + tokenCopy.RaftIndex = structs.RaftIndex{} + return tokenCopy +} + +func compareTokens(t *testing.T, expected, actual *structs.ACLToken) { + require.Equal(t, stripIrrelevantTokenFields(expected), stripIrrelevantTokenFields(actual)) +} diff --git a/agent/structs/acl.go b/agent/structs/acl.go index 5e052b414f73..84e179366104 100644 --- a/agent/structs/acl.go +++ b/agent/structs/acl.go @@ -113,6 +113,7 @@ type ACLIdentity interface { SecretToken() string PolicyIDs() []string EmbeddedPolicy() *ACLPolicy + IsExpired(asOf time.Time) bool } type ACLTokenPolicyLink struct { @@ -150,6 +151,19 @@ type ACLToken struct { // to the ACL datacenter and replicated to others. Local bool + // ExpirationTime represents the point after which a token should be + // considered revoked and is eligible for destruction. The zero value + // represents NO expiration. + ExpirationTime time.Time `json:",omitempty"` + + // ExpirationTTL is a convenience field for helping set ExpirationTime to a + // value of CreateTime+ExpirationTTL. This can only be set during + // TokenCreate and is cleared and used to initialize the ExpirationTime + // field before being persisted to the state store or raft log. + // + // This is a string version of a time.Duration like "2m". + ExpirationTTL time.Duration `json:",omitempty"` + // The time when this token was created CreateTime time.Time `json:",omitempty"` @@ -191,6 +205,20 @@ func (t *ACLToken) PolicyIDs() []string { return ids } +func (t *ACLToken) IsExpired(asOf time.Time) bool { + if asOf.IsZero() || t.ExpirationTime.IsZero() { + return false + } + return t.ExpirationTime.Before(asOf) +} + +func (t *ACLToken) UsesNonLegacyFields() bool { + return len(t.Policies) > 0 || + t.Type == "" || + !t.ExpirationTime.IsZero() || + t.ExpirationTTL != 0 +} + func (t *ACLToken) EmbeddedPolicy() *ACLPolicy { // DEPRECATED (ACL-Legacy-Compat) // @@ -229,6 +257,14 @@ func (t *ACLToken) SetHash(force bool) []byte { panic(err) } + // Any non-immutable "content" fields should be involved with the + // overall hash. The IDs are immutable which is why they aren't here. + // The raft indices are metadata similar to the hash which is why they + // aren't incorporated. CreateTime is similarly immutable + // + // The Hash is really only used for replication to determine if a token + // has changed and should be updated locally. + // Write all the user set fields hash.Write([]byte(t.Description)) hash.Write([]byte(t.Type)) @@ -254,8 +290,8 @@ func (t *ACLToken) SetHash(force bool) []byte { } func (t *ACLToken) EstimateSize() int { - // 33 = 16 (RaftIndex) + 8 (Hash) + 8 (CreateTime) + 1 (Local) - size := 33 + len(t.AccessorID) + len(t.SecretID) + len(t.Description) + len(t.Type) + len(t.Rules) + // 41 = 16 (RaftIndex) + 8 (Hash) + 8 (ExpirationTime) + 8 (CreateTime) + 1 (Local) + size := 41 + len(t.AccessorID) + len(t.SecretID) + len(t.Description) + len(t.Type) + len(t.Rules) for _, link := range t.Policies { size += len(link.ID) + len(link.Name) } @@ -266,30 +302,32 @@ func (t *ACLToken) EstimateSize() int { type ACLTokens []*ACLToken type ACLTokenListStub struct { - AccessorID string - Description string - Policies []ACLTokenPolicyLink - Local bool - CreateTime time.Time `json:",omitempty"` - Hash []byte - CreateIndex uint64 - ModifyIndex uint64 - Legacy bool `json:",omitempty"` + AccessorID string + Description string + Policies []ACLTokenPolicyLink + Local bool + ExpirationTime time.Time `json:",omitempty"` + CreateTime time.Time `json:",omitempty"` + Hash []byte + CreateIndex uint64 + ModifyIndex uint64 + Legacy bool `json:",omitempty"` } type ACLTokenListStubs []*ACLTokenListStub func (token *ACLToken) Stub() *ACLTokenListStub { return &ACLTokenListStub{ - AccessorID: token.AccessorID, - Description: token.Description, - Policies: token.Policies, - Local: token.Local, - CreateTime: token.CreateTime, - Hash: token.Hash, - CreateIndex: token.CreateIndex, - ModifyIndex: token.ModifyIndex, - Legacy: token.Rules != "", + AccessorID: token.AccessorID, + Description: token.Description, + Policies: token.Policies, + Local: token.Local, + ExpirationTime: token.ExpirationTime, + CreateTime: token.CreateTime, + Hash: token.Hash, + CreateIndex: token.CreateIndex, + ModifyIndex: token.ModifyIndex, + Legacy: token.Rules != "", } } @@ -384,6 +422,14 @@ func (p *ACLPolicy) SetHash(force bool) []byte { panic(err) } + // Any non-immutable "content" fields should be involved with the + // overall hash. The ID is immutable which is why it isn't here. The + // raft indices are metadata similar to the hash which is why they + // aren't incorporated. CreateTime is similarly immutable + // + // The Hash is really only used for replication to determine if a policy + // has changed and should be updated locally. + // Write all the user set fields hash.Write([]byte(p.Name)) hash.Write([]byte(p.Description)) @@ -414,7 +460,7 @@ func (p *ACLPolicy) EstimateSize() int { return size } -// ACLPolicyListHash returns a consistent hash for a set of policies. +// HashKey returns a consistent hash for a set of policies. func (policies ACLPolicies) HashKey() string { cacheKeyHash, err := blake2b.New256(nil) if err != nil { diff --git a/agent/structs/acl_test.go b/agent/structs/acl_test.go index fba38545bf86..2e7e9edcdba4 100644 --- a/agent/structs/acl_test.go +++ b/agent/structs/acl_test.go @@ -208,7 +208,7 @@ func TestStructs_ACLToken_EstimateSize(t *testing.T) { // this test is very contrived. Basically just tests that the // math is okay and returns the value. - require.Equal(t, 120, token.EstimateSize()) + require.Equal(t, 128, token.EstimateSize()) } func TestStructs_ACLToken_Stub(t *testing.T) { diff --git a/api/acl.go b/api/acl.go index 53a052363e29..667d21548227 100644 --- a/api/acl.go +++ b/api/acl.go @@ -22,15 +22,17 @@ type ACLTokenPolicyLink struct { // ACLToken represents an ACL Token type ACLToken struct { - CreateIndex uint64 - ModifyIndex uint64 - AccessorID string - SecretID string - Description string - Policies []*ACLTokenPolicyLink - Local bool - CreateTime time.Time `json:",omitempty"` - Hash []byte `json:",omitempty"` + CreateIndex uint64 + ModifyIndex uint64 + AccessorID string + SecretID string + Description string + Policies []*ACLTokenPolicyLink + Local bool + ExpirationTTL time.Duration `json:",omitempty"` + ExpirationTime time.Time `json:",omitempty"` + CreateTime time.Time `json:",omitempty"` + Hash []byte `json:",omitempty"` // DEPRECATED (ACL-Legacy-Compat) // Rules will only be present for legacy tokens returned via the new APIs @@ -38,15 +40,16 @@ type ACLToken struct { } type ACLTokenListEntry struct { - CreateIndex uint64 - ModifyIndex uint64 - AccessorID string - Description string - Policies []*ACLTokenPolicyLink - Local bool - CreateTime time.Time - Hash []byte - Legacy bool + CreateIndex uint64 + ModifyIndex uint64 + AccessorID string + Description string + Policies []*ACLTokenPolicyLink + Local bool + ExpirationTime time.Time `json:",omitempty"` + CreateTime time.Time + Hash []byte + Legacy bool } // ACLEntry is used to represent a legacy ACL token diff --git a/command/acl/acl_helpers.go b/command/acl/acl_helpers.go index 96d8ec57c9c0..1b9f9cee74b8 100644 --- a/command/acl/acl_helpers.go +++ b/command/acl/acl_helpers.go @@ -10,15 +10,18 @@ import ( ) func PrintToken(token *api.ACLToken, ui cli.Ui, showMeta bool) { - ui.Info(fmt.Sprintf("AccessorID: %s", token.AccessorID)) - ui.Info(fmt.Sprintf("SecretID: %s", token.SecretID)) - ui.Info(fmt.Sprintf("Description: %s", token.Description)) - ui.Info(fmt.Sprintf("Local: %t", token.Local)) - ui.Info(fmt.Sprintf("Create Time: %v", token.CreateTime)) + ui.Info(fmt.Sprintf("AccessorID: %s", token.AccessorID)) + ui.Info(fmt.Sprintf("SecretID: %s", token.SecretID)) + ui.Info(fmt.Sprintf("Description: %s", token.Description)) + ui.Info(fmt.Sprintf("Local: %t", token.Local)) + ui.Info(fmt.Sprintf("Create Time: %v", token.CreateTime)) + if !token.ExpirationTime.IsZero() { + ui.Info(fmt.Sprintf("Expiration Time: %v", token.ExpirationTime)) + } if showMeta { - ui.Info(fmt.Sprintf("Hash: %x", token.Hash)) - ui.Info(fmt.Sprintf("Create Index: %d", token.CreateIndex)) - ui.Info(fmt.Sprintf("Modify Index: %d", token.ModifyIndex)) + ui.Info(fmt.Sprintf("Hash: %x", token.Hash)) + ui.Info(fmt.Sprintf("Create Index: %d", token.CreateIndex)) + ui.Info(fmt.Sprintf("Modify Index: %d", token.ModifyIndex)) } ui.Info(fmt.Sprintf("Policies:")) for _, policy := range token.Policies { @@ -31,15 +34,18 @@ func PrintToken(token *api.ACLToken, ui cli.Ui, showMeta bool) { } func PrintTokenListEntry(token *api.ACLTokenListEntry, ui cli.Ui, showMeta bool) { - ui.Info(fmt.Sprintf("AccessorID: %s", token.AccessorID)) - ui.Info(fmt.Sprintf("Description: %s", token.Description)) - ui.Info(fmt.Sprintf("Local: %t", token.Local)) - ui.Info(fmt.Sprintf("Create Time: %v", token.CreateTime)) - ui.Info(fmt.Sprintf("Legacy: %t", token.Legacy)) + ui.Info(fmt.Sprintf("AccessorID: %s", token.AccessorID)) + ui.Info(fmt.Sprintf("Description: %s", token.Description)) + ui.Info(fmt.Sprintf("Local: %t", token.Local)) + ui.Info(fmt.Sprintf("Create Time: %v", token.CreateTime)) + if !token.ExpirationTime.IsZero() { + ui.Info(fmt.Sprintf("Expiration Time: %v", token.ExpirationTime)) + } + ui.Info(fmt.Sprintf("Legacy: %t", token.Legacy)) if showMeta { - ui.Info(fmt.Sprintf("Hash: %x", token.Hash)) - ui.Info(fmt.Sprintf("Create Index: %d", token.CreateIndex)) - ui.Info(fmt.Sprintf("Modify Index: %d", token.ModifyIndex)) + ui.Info(fmt.Sprintf("Hash: %x", token.Hash)) + ui.Info(fmt.Sprintf("Create Index: %d", token.CreateIndex)) + ui.Info(fmt.Sprintf("Modify Index: %d", token.ModifyIndex)) } ui.Info(fmt.Sprintf("Policies:")) for _, policy := range token.Policies { diff --git a/command/acl/token/clone/token_clone_test.go b/command/acl/token/clone/token_clone_test.go index 0baa0c4f2e23..55a81a682d10 100644 --- a/command/acl/token/clone/token_clone_test.go +++ b/command/acl/token/clone/token_clone_test.go @@ -19,11 +19,11 @@ import ( func parseCloneOutput(t *testing.T, output string) *api.ACLToken { // This will only work for non-legacy tokens re := regexp.MustCompile("Token cloned successfully.\n" + - "AccessorID: ([a-zA-Z0-9\\-]{36})\n" + - "SecretID: ([a-zA-Z0-9\\-]{36})\n" + - "Description: ([^\n]*)\n" + - "Local: (true|false)\n" + - "Create Time: ([^\n]+)\n" + + "AccessorID: ([a-zA-Z0-9\\-]{36})\n" + + "SecretID: ([a-zA-Z0-9\\-]{36})\n" + + "Description: ([^\n]*)\n" + + "Local: (true|false)\n" + + "Create Time: ([^\n]+)\n" + "Policies:\n" + "( [a-zA-Z0-9\\-]{36} - [^\n]+\n)*") diff --git a/command/acl/token/create/token_create.go b/command/acl/token/create/token_create.go index 83a17cd612bf..624ec8e6477d 100644 --- a/command/acl/token/create/token_create.go +++ b/command/acl/token/create/token_create.go @@ -3,6 +3,7 @@ package tokencreate import ( "flag" "fmt" + "time" "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/command/acl" @@ -22,11 +23,12 @@ type cmd struct { http *flags.HTTPFlags help string - policyIDs []string - policyNames []string - description string - local bool - showMeta bool + policyIDs []string + policyNames []string + expirationTTL time.Duration + description string + local bool + showMeta bool } func (c *cmd) init() { @@ -39,6 +41,8 @@ func (c *cmd) init() { "policy to use for this token. May be specified multiple times") c.flags.Var((*flags.AppendSliceValue)(&c.policyNames), "policy-name", "Name of a "+ "policy to use for this token. May be specified multiple times") + c.flags.DurationVar(&c.expirationTTL, "expires-ttl", 0, "Duration of time this "+ + "token should be valid for") c.http = &flags.HTTPFlags{} flags.Merge(c.flags, c.http.ClientFlags()) flags.Merge(c.flags, c.http.ServerFlags()) @@ -65,6 +69,9 @@ func (c *cmd) Run(args []string) int { Description: c.description, Local: c.local, } + if c.expirationTTL > 0 { + newToken.ExpirationTTL = c.expirationTTL + } for _, policyName := range c.policyNames { // We could resolve names to IDs here but there isn't any reason why its would be better @@ -109,7 +116,7 @@ Usage: consul acl token create [options] Create a new token: - $ consul acl token create -description "Replication token" - -policy-id b52fc3de-5 - -policy-name "acl-replication" + $ consul acl token create -description "Replication token" \ + -policy-id b52fc3de-5 \ + -policy-name "acl-replication" ` diff --git a/command/acl/token/update/token_update.go b/command/acl/token/update/token_update.go index 09df170b5afc..08498087967d 100644 --- a/command/acl/token/update/token_update.go +++ b/command/acl/token/update/token_update.go @@ -192,7 +192,9 @@ Usage: consul acl token update [options] $ consul acl token update -id abcd -description "replication" -merge-policies - Update all editable fields of the token: + Update all editable fields of the token: - $ consul acl token update -id abcd -description "replication" -policy-name "token-replication" + $ consul acl token update -id abcd \ + -description "replication" \ + -policy-name "token-replication" `