From 72f2199ea118a1355b56ad320c34d025021009c6 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 21 Sep 2021 18:48:50 -0400 Subject: [PATCH 1/2] acl: remove remaining tests that use ACL.Apply In preparation for removing ACL.Apply. Tests for ACL.Apply, ACL.GetPolicy, and ACL upgrades were removed because all 3 of those will be removed shortly. The forth test appears to be for the ACLResolver cache, so the test was moved to the correct test file, and the name was updated to make it obvious what is being tested. --- agent/consul/acl_endpoint_test.go | 246 ------------------------------ agent/consul/acl_test.go | 79 ++++++++++ agent/consul/leader_test.go | 67 -------- 3 files changed, 79 insertions(+), 313 deletions(-) diff --git a/agent/consul/acl_endpoint_test.go b/agent/consul/acl_endpoint_test.go index bd31232c1b..10574dc0f7 100644 --- a/agent/consul/acl_endpoint_test.go +++ b/agent/consul/acl_endpoint_test.go @@ -74,252 +74,6 @@ func TestACLEndpoint_BootstrapTokens(t *testing.T) { require.Equal(t, out.CreateIndex, out.ModifyIndex) } -func TestACLEndpoint_Apply(t *testing.T) { - if testing.Short() { - t.Skip("too slow for testing.Short") - } - - t.Parallel() - _, srv, codec := testACLServerWithConfig(t, nil, false) - waitForLeaderEstablishment(t, srv) - - arg := structs.ACLRequest{ - Datacenter: "dc1", - Op: structs.ACLSet, - ACL: structs.ACL{ - Name: "User token", - Type: structs.ACLTokenTypeClient, - }, - WriteRequest: structs.WriteRequest{Token: TestDefaultMasterToken}, - } - var out string - err := msgpackrpc.CallWithCodec(codec, "ACL.Apply", &arg, &out) - require.NoError(t, err) - id := out - - // Verify - state := srv.fsm.State() - _, s, err := state.ACLTokenGetBySecret(nil, out, nil) - require.NoError(t, err) - require.NotNil(t, s) - require.Equal(t, out, s.SecretID) - require.Equal(t, "User token", s.Description) - - // Do a delete - arg.Op = structs.ACLDelete - arg.ACL.ID = out - err = msgpackrpc.CallWithCodec(codec, "ACL.Apply", &arg, &out) - require.NoError(t, err) - - // Verify - _, s, err = state.ACLTokenGetBySecret(nil, id, nil) - require.NoError(t, err) - require.Nil(t, s) -} - -func TestACLEndpoint_Update_PurgeCache(t *testing.T) { - if testing.Short() { - t.Skip("too slow for testing.Short") - } - - t.Parallel() - _, srv, codec := testACLServerWithConfig(t, nil, false) - waitForLeaderEstablishment(t, srv) - - arg := structs.ACLRequest{ - Datacenter: "dc1", - Op: structs.ACLSet, - ACL: structs.ACL{ - Name: "User token", - Type: structs.ACLTokenTypeClient, - Rules: `key "" { policy = "read"}`, - }, - WriteRequest: structs.WriteRequest{Token: TestDefaultMasterToken}, - } - var out string - err := msgpackrpc.CallWithCodec(codec, "ACL.Apply", &arg, &out) - require.NoError(t, err) - id := out - - // Resolve - acl1, err := srv.ResolveToken(id) - require.NoError(t, err) - require.NotNil(t, acl1) - require.Equal(t, acl.Allow, acl1.KeyRead("foo", nil)) - - // Do an update - arg.ACL.ID = out - arg.ACL.Rules = `{"key": {"": {"policy": "deny"}}}` - err = msgpackrpc.CallWithCodec(codec, "ACL.Apply", &arg, &out) - require.NoError(t, err) - - // Resolve again - acl2, err := srv.ResolveToken(id) - require.NoError(t, err) - require.NotNil(t, acl2) - require.NotSame(t, acl2, acl1) - require.NotEqual(t, acl.Allow, acl2.KeyRead("foo", nil)) - - // Do a delete - arg.Op = structs.ACLDelete - arg.ACL.Rules = "" - err = msgpackrpc.CallWithCodec(codec, "ACL.Apply", &arg, &out) - require.NoError(t, err) - - // Resolve again - acl3, err := srv.ResolveToken(id) - require.True(t, acl.IsErrNotFound(err), "Error %v is not acl.ErrNotFound", err) - require.Nil(t, acl3) -} - -func TestACLEndpoint_Apply_CustomID(t *testing.T) { - if testing.Short() { - t.Skip("too slow for testing.Short") - } - - t.Parallel() - _, srv, codec := testACLServerWithConfig(t, nil, false) - waitForLeaderEstablishment(t, srv) - - arg := structs.ACLRequest{ - Datacenter: "dc1", - Op: structs.ACLSet, - ACL: structs.ACL{ - ID: "foobarbaz", // Specify custom ID, does not exist - Name: "User token", - Type: structs.ACLTokenTypeClient, - }, - WriteRequest: structs.WriteRequest{Token: TestDefaultMasterToken}, - } - var out string - err := msgpackrpc.CallWithCodec(codec, "ACL.Apply", &arg, &out) - require.NoError(t, err) - require.Equal(t, "foobarbaz", out) - - // Verify - state := srv.fsm.State() - _, s, err := state.ACLTokenGetBySecret(nil, out, nil) - require.NoError(t, err) - require.NotNil(t, s) - require.Equal(t, out, s.SecretID) - require.Equal(t, "User token", s.Description) -} - -func TestACLEndpoint_Apply_Denied(t *testing.T) { - if testing.Short() { - t.Skip("too slow for testing.Short") - } - - t.Parallel() - _, srv, codec := testACLServerWithConfig(t, nil, false) - waitForLeaderEstablishment(t, srv) - - arg := structs.ACLRequest{ - Datacenter: "dc1", - Op: structs.ACLSet, - ACL: structs.ACL{ - Name: "User token", - Type: structs.ACLTokenTypeClient, - }, - } - var out string - err := msgpackrpc.CallWithCodec(codec, "ACL.Apply", &arg, &out) - require.True(t, acl.IsErrPermissionDenied(err), "Err %v is not acl.PermissionDenied", err) -} - -func TestACLEndpoint_Apply_DeleteAnon(t *testing.T) { - if testing.Short() { - t.Skip("too slow for testing.Short") - } - - t.Parallel() - _, srv, codec := testACLServerWithConfig(t, nil, false) - waitForLeaderEstablishment(t, srv) - - arg := structs.ACLRequest{ - Datacenter: "dc1", - Op: structs.ACLDelete, - ACL: structs.ACL{ - ID: anonymousToken, - Name: "User token", - Type: structs.ACLTokenTypeClient, - }, - WriteRequest: structs.WriteRequest{Token: TestDefaultMasterToken}, - } - var out string - err := msgpackrpc.CallWithCodec(codec, "ACL.Apply", &arg, &out) - testutil.RequireErrorContains(t, err, "delete anonymous") -} - -func TestACLEndpoint_Apply_RootChange(t *testing.T) { - if testing.Short() { - t.Skip("too slow for testing.Short") - } - - t.Parallel() - _, srv, codec := testACLServerWithConfig(t, nil, false) - waitForLeaderEstablishment(t, srv) - - arg := structs.ACLRequest{ - Datacenter: "dc1", - Op: structs.ACLSet, - ACL: structs.ACL{ - ID: "manage", - Name: "User token", - Type: structs.ACLTokenTypeClient, - }, - WriteRequest: structs.WriteRequest{Token: TestDefaultMasterToken}, - } - var out string - err := msgpackrpc.CallWithCodec(codec, "ACL.Apply", &arg, &out) - testutil.RequireErrorContains(t, err, "root ACL") -} - -func TestACLEndpoint_GetPolicy(t *testing.T) { - if testing.Short() { - t.Skip("too slow for testing.Short") - } - - t.Parallel() - _, srv, codec := testACLServerWithConfig(t, nil, false) - waitForLeaderEstablishment(t, srv) - - arg := structs.ACLRequest{ - Datacenter: "dc1", - Op: structs.ACLSet, - ACL: structs.ACL{ - Name: "User token", - Type: structs.ACLTokenTypeClient, - }, - WriteRequest: structs.WriteRequest{Token: TestDefaultMasterToken}, - } - var out string - err := msgpackrpc.CallWithCodec(codec, "ACL.Apply", &arg, &out) - require.NoError(t, err) - - getR := structs.ACLPolicyResolveLegacyRequest{ - Datacenter: "dc1", - ACL: out, - } - - var acls structs.ACLPolicyResolveLegacyResponse - retry.Run(t, func(r *retry.R) { - err := msgpackrpc.CallWithCodec(codec, "ACL.GetPolicy", &getR, &acls) - - require.NoError(r, err) - require.NotNil(t, acls.Policy) - require.Equal(t, 30*time.Second, acls.TTL) - }) - - // Do a conditional lookup with etag - getR.ETag = acls.ETag - var out2 structs.ACLPolicyResolveLegacyResponse - require.NoError(t, msgpackrpc.CallWithCodec(codec, "ACL.GetPolicy", &getR, &out2)) - - require.Nil(t, out2.Policy) - require.Equal(t, 30*time.Second, out2.TTL) -} - func TestACLEndpoint_GetPolicy_Management(t *testing.T) { if testing.Short() { t.Skip("too slow for testing.Short") diff --git a/agent/consul/acl_test.go b/agent/consul/acl_test.go index c449355d2c..720fdd5f6b 100644 --- a/agent/consul/acl_test.go +++ b/agent/consul/acl_test.go @@ -9,6 +9,8 @@ import ( "testing" "time" + "github.com/hashicorp/go-uuid" + msgpackrpc "github.com/hashicorp/net-rpc-msgpackrpc" "github.com/mitchellh/copystructure" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -3771,3 +3773,80 @@ func TestACLResolver_ACLsEnabled(t *testing.T) { } } + +func TestACLResolver_ResolveTokenToIdentityAndAuthorizer_UpdatesPurgeTheCache(t *testing.T) { + if testing.Short() { + t.Skip("too slow for testing.Short") + } + + t.Parallel() + _, srv, codec := testACLServerWithConfig(t, nil, false) + waitForLeaderEstablishment(t, srv) + + reqPolicy := structs.ACLPolicySetRequest{ + Datacenter: "dc1", + Policy: structs.ACLPolicy{ + Name: "the-policy", + Rules: `key_prefix "" { policy = "read"}`, + }, + WriteRequest: structs.WriteRequest{Token: TestDefaultMasterToken}, + } + var respPolicy = structs.ACLPolicy{} + err := msgpackrpc.CallWithCodec(codec, "ACL.PolicySet", &reqPolicy, &respPolicy) + require.NoError(t, err) + + token, err := uuid.GenerateUUID() + require.NoError(t, err) + + reqToken := structs.ACLTokenSetRequest{ + Datacenter: "dc1", + ACLToken: structs.ACLToken{ + SecretID: token, + Policies: []structs.ACLTokenPolicyLink{{Name: "the-policy"}}, + }, + WriteRequest: structs.WriteRequest{Token: TestDefaultMasterToken}, + } + var respToken structs.ACLToken + err = msgpackrpc.CallWithCodec(codec, "ACL.TokenSet", &reqToken, &respToken) + require.NoError(t, err) + + runStep(t, "first resolve", func(t *testing.T) { + _, authz, err := srv.acls.ResolveTokenToIdentityAndAuthorizer(token) + require.NoError(t, err) + require.NotNil(t, authz) + require.Equal(t, acl.Allow, authz.KeyRead("foo", nil)) + }) + + runStep(t, "update the policy and resolve again", func(t *testing.T) { + reqPolicy := structs.ACLPolicySetRequest{ + Datacenter: "dc1", + Policy: structs.ACLPolicy{ + ID: respPolicy.ID, + Name: "the-policy", + Rules: `{"key_prefix": {"": {"policy": "deny"}}}`, + }, + WriteRequest: structs.WriteRequest{Token: TestDefaultMasterToken}, + } + err := msgpackrpc.CallWithCodec(codec, "ACL.PolicySet", &reqPolicy, &structs.ACLPolicy{}) + require.NoError(t, err) + + _, authz, err := srv.acls.ResolveTokenToIdentityAndAuthorizer(token) + require.NoError(t, err) + require.NotNil(t, authz) + require.Equal(t, acl.Deny, authz.KeyRead("foo", nil)) + }) + + runStep(t, "delete the token", func(t *testing.T) { + req := structs.ACLTokenDeleteRequest{ + Datacenter: "dc1", + TokenID: respToken.AccessorID, + WriteRequest: structs.WriteRequest{Token: TestDefaultMasterToken}, + } + var resp string + err := msgpackrpc.CallWithCodec(codec, "ACL.TokenDelete", &req, &resp) + require.NoError(t, err) + + _, _, err = srv.acls.ResolveTokenToIdentityAndAuthorizer(token) + require.True(t, acl.IsErrNotFound(err), "Error %v is not acl.ErrNotFound", err) + }) +} diff --git a/agent/consul/leader_test.go b/agent/consul/leader_test.go index 8d1a3d1afe..1dcaeda86e 100644 --- a/agent/consul/leader_test.go +++ b/agent/consul/leader_test.go @@ -1208,73 +1208,6 @@ func TestLeader_ACL_Initialization(t *testing.T) { } } -func TestLeader_ACLUpgrade(t *testing.T) { - if testing.Short() { - t.Skip("too slow for testing.Short") - } - - t.Parallel() - dir1, s1 := testServerWithConfig(t, func(c *Config) { - c.ACLsEnabled = true - c.PrimaryDatacenter = "dc1" - c.ACLMasterToken = "root" - }) - defer os.RemoveAll(dir1) - defer s1.Shutdown() - testrpc.WaitForTestAgent(t, s1.RPC, "dc1") - codec := rpcClient(t, s1) - defer codec.Close() - - // create a legacy management ACL - mgmt := structs.ACLRequest{ - Datacenter: "dc1", - Op: structs.ACLSet, - ACL: structs.ACL{ - Name: "Management token", - Type: structs.ACLTokenTypeManagement, - }, - WriteRequest: structs.WriteRequest{Token: "root"}, - } - var mgmt_id string - require.NoError(t, msgpackrpc.CallWithCodec(codec, "ACL.Apply", &mgmt, &mgmt_id)) - - // wait for it to be upgraded - retry.Run(t, func(t *retry.R) { - _, token, err := s1.fsm.State().ACLTokenGetBySecret(nil, mgmt_id, nil) - require.NoError(t, err) - require.NotNil(t, token) - require.NotEqual(t, "", token.AccessorID) - require.Equal(t, structs.ACLTokenTypeManagement, token.Type) - require.Len(t, token.Policies, 1) - require.Equal(t, structs.ACLPolicyGlobalManagementID, token.Policies[0].ID) - }) - - // create a legacy management ACL - client := structs.ACLRequest{ - Datacenter: "dc1", - Op: structs.ACLSet, - ACL: structs.ACL{ - Name: "Management token", - Type: structs.ACLTokenTypeClient, - Rules: `node "" { policy = "read"}`, - }, - WriteRequest: structs.WriteRequest{Token: "root"}, - } - var client_id string - require.NoError(t, msgpackrpc.CallWithCodec(codec, "ACL.Apply", &client, &client_id)) - - // wait for it to be upgraded - retry.Run(t, func(t *retry.R) { - _, token, err := s1.fsm.State().ACLTokenGetBySecret(nil, client_id, nil) - require.NoError(t, err) - require.NotNil(t, token) - require.NotEqual(t, "", token.AccessorID) - require.Len(t, token.Policies, 0) - require.Equal(t, structs.ACLTokenTypeClient, token.Type) - require.Equal(t, client.ACL.Rules, token.Rules) - }) -} - func TestLeader_ACLUpgrade_IsStickyEvenIfSerfTagsRegress(t *testing.T) { if testing.Short() { t.Skip("too slow for testing.Short") From c84867feda0da73289125c90578033976d12b1e4 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 21 Sep 2021 19:43:05 -0400 Subject: [PATCH 2/2] acl: remove ACL.Apply As part of removing the legacy ACL system. --- agent/consul/acl_endpoint_legacy.go | 129 +---------------------- agent/setup.go | 1 - website/content/docs/agent/telemetry.mdx | 1 - 3 files changed, 2 insertions(+), 129 deletions(-) diff --git a/agent/consul/acl_endpoint_legacy.go b/agent/consul/acl_endpoint_legacy.go index a5e463e556..e851631479 100644 --- a/agent/consul/acl_endpoint_legacy.go +++ b/agent/consul/acl_endpoint_legacy.go @@ -2,141 +2,16 @@ package consul import ( "fmt" - "time" - "github.com/armon/go-metrics" - "github.com/armon/go-metrics/prometheus" - - "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/structs" - "github.com/hashicorp/consul/lib" ) -var ACLEndpointLegacySummaries = []prometheus.SummaryDefinition{ - { - Name: []string{"acl", "apply"}, - Help: "Measures the time it takes to complete an update to the ACL store.", - }, -} - func (a *ACL) Bootstrap(*structs.DCSpecificRequest, *structs.ACL) error { return fmt.Errorf("ACL.Bootstrap: the legacy ACL system has been removed") } -// aclApplyInternal is used to apply an ACL request after it has been vetted that -// this is a valid operation. It is used when users are updating ACLs, in which -// case we check their token to make sure they have management privileges. It is -// also used for ACL replication. We want to run the replicated ACLs through the -// same checks on the change itself. -func aclApplyInternal(srv *Server, args *structs.ACLRequest, reply *string) error { - // All ACLs must have an ID by this point. - if args.ACL.ID == "" { - return fmt.Errorf("Missing ACL ID") - } - - switch args.Op { - case structs.ACLSet: - // Verify the ACL type - switch args.ACL.Type { - case structs.ACLTokenTypeClient: - case structs.ACLTokenTypeManagement: - default: - 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, nil) - if existing != nil && existing.UsesNonLegacyFields() { - return fmt.Errorf("Cannot use legacy endpoint to modify a non-legacy token") - } - - // Verify this is not a root ACL - if acl.RootAuthorizer(args.ACL.ID) != nil { - return acl.PermissionDeniedError{Cause: "Cannot modify root ACL"} - } - - // Ensure that we allow more permissive rule formats for legacy tokens, - // but that we correct them on the way into the system. - // - // DEPRECATED (ACL-Legacy-Compat) - correctedRules := structs.SanitizeLegacyACLTokenRules(args.ACL.Rules) - if correctedRules != "" { - args.ACL.Rules = correctedRules - } - - // Validate the rules compile - _, err := acl.NewPolicyFromSource("", 0, args.ACL.Rules, acl.SyntaxLegacy, srv.aclConfig, nil) - if err != nil { - return fmt.Errorf("ACL rule compilation failed: %v", err) - } - - case structs.ACLDelete: - if args.ACL.ID == anonymousToken { - return acl.PermissionDeniedError{Cause: "Cannot delete anonymous token"} - } - - default: - return fmt.Errorf("Invalid ACL Operation") - } - - // Apply the update - resp, err := srv.raftApply(structs.ACLRequestType, args) - if err != nil { - return fmt.Errorf("raft apply failed: %w", err) - } - - // Check if the return type is a string - if respString, ok := resp.(string); ok { - *reply = respString - } - - return nil -} - -// Apply is used to apply a modifying request to the data store. This should -// only be used for operations that modify the data -func (a *ACL) Apply(args *structs.ACLRequest, reply *string) error { - if done, err := a.srv.ForwardRPC("ACL.Apply", args, reply); done { - return err - } - defer metrics.MeasureSince([]string{"acl", "apply"}, time.Now()) - - // Verify we are allowed to serve this request - if !a.srv.config.ACLsEnabled { - return acl.ErrDisabled - } - - // Verify token is permitted to modify ACLs - // NOTE: We will not support enterprise authorizer contexts with legacy ACLs - if authz, err := a.srv.ResolveToken(args.Token); err != nil { - return err - } else if authz.ACLWrite(nil) != acl.Allow { - return acl.ErrPermissionDenied - } - - // If no ID is provided, generate a new ID. This must be done prior to - // appending to the Raft log, because the ID is not deterministic. Once - // the entry is in the log, the state update MUST be deterministic or - // the followers will not converge. - if args.Op == structs.ACLSet && args.ACL.ID == "" { - var err error - args.ACL.ID, err = lib.GenerateUUID(a.srv.checkTokenUUID) - if err != nil { - return err - } - } - - // Do the apply now that this update is vetted. - if err := aclApplyInternal(a.srv, args, reply); err != nil { - return err - } - - // Clear the cache if applicable - if args.ACL.ID != "" { - a.srv.acls.cache.RemoveIdentity(tokenSecretCacheID(args.ACL.ID)) - } - - return nil +func (a *ACL) Apply(*structs.ACLRequest, *string) error { + return fmt.Errorf("ACL.Apply: the legacy ACL system has been removed") } func (a *ACL) Get(*structs.ACLSpecificRequest, *structs.IndexedACLs) error { diff --git a/agent/setup.go b/agent/setup.go index c10a5e34f6..02991d94ff 100644 --- a/agent/setup.go +++ b/agent/setup.go @@ -296,7 +296,6 @@ func getPrometheusDefs(cfg lib.TelemetryConfig) ([]prometheus.GaugeDefinition, [ HTTPSummaries, consul.ACLSummaries, consul.ACLEndpointSummaries, - consul.ACLEndpointLegacySummaries, consul.CatalogSummaries, consul.FederationStateSummaries, consul.IntentionSummaries, diff --git a/website/content/docs/agent/telemetry.mdx b/website/content/docs/agent/telemetry.mdx index 9d4e0c4b4a..15bfcafeb5 100644 --- a/website/content/docs/agent/telemetry.mdx +++ b/website/content/docs/agent/telemetry.mdx @@ -328,7 +328,6 @@ These metrics are used to monitor the health of the Consul servers. | Metric | Description | Unit | Type | | --------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------- | ------- | -| `consul.acl.apply` | Measures the time it takes to complete an update to the ACL store. | ms | timer | | `consul.acl.resolveTokenLegacy` | Measures the time it takes to resolve an ACL token using the legacy ACL system. | ms | timer | | `consul.acl.ResolveToken` | Measures the time it takes to resolve an ACL token. | ms | timer | | `consul.acl.ResolveTokenToIdentity` | Measures the time it takes to resolve an ACL token to an Identity. | ms | timer |