From 9718079a49e6afcbf4d35e71dcb05ca9f2743e42 Mon Sep 17 00:00:00 2001 From: skpratt Date: Wed, 8 Feb 2023 17:49:44 -0600 Subject: [PATCH] ACL error improvements: incomplete bootstrapping and non-existent token (#16105) * add bootstrapping detail for acl errors * error detail improvements * update acl bootstrapping test coverage * update namespace errors * update test coverage * add changelog * update message for unbootstrapped error * consolidate error message code and update changelog * logout message change --- .changelog/16105.txt | 11 ++++ acl/errors.go | 7 ++ agent/acl_endpoint.go | 54 ++++++++++++--- agent/acl_endpoint_test.go | 4 +- agent/consul/acl_endpoint.go | 62 +++++++++++------ agent/consul/acl_endpoint_test.go | 66 ++++++++++++------- agent/consul/acl_replication_test.go | 4 +- agent/consul/acl_server.go | 8 ++- api/acl_test.go | 22 +++++++ api/api_test.go | 13 ++++ command/acl/acl_helpers.go | 2 +- .../delete/authmethod_delete_test.go | 14 ++-- .../delete/bindingrule_delete_test.go | 23 ++++++- .../acl/policy/delete/policy_delete_test.go | 7 +- command/acl/token/delete/token_delete_test.go | 3 +- command/logout/logout_test.go | 4 +- 16 files changed, 230 insertions(+), 74 deletions(-) create mode 100644 .changelog/16105.txt diff --git a/.changelog/16105.txt b/.changelog/16105.txt new file mode 100644 index 000000000..ac3ae0e92 --- /dev/null +++ b/.changelog/16105.txt @@ -0,0 +1,11 @@ +```release-note:breaking-change +acl errors: Delete and get requests now return descriptive errors when the specified resource cannot be found. Other ACL request errors provide more information about when a resource is missing. Add error for when the ACL system has not been bootstrapped. +1. Delete Token/Policy/AuthMethod/Role/BindingRule endpoints now return 404 when the resource cannot be found. + - New error formats: "Requested * does not exist: ACL not found", "* not found in namespace $NAMESPACE: ACL not found" +3. Read Token/Policy/Role endpoints now return 404 when the resource cannot be found. + - New error format: "Cannot find * to delete" +4. Logout now returns a 401 error when the supplied token cannot be found + - New error format: "Supplied token does not exist" +5. Token Self endpoint now returns 404 when the token cannot be found. + - New error format: "Supplied token does not exist" +``` \ No newline at end of file diff --git a/acl/errors.go b/acl/errors.go index 5241494de..9938957c1 100644 --- a/acl/errors.go +++ b/acl/errors.go @@ -140,3 +140,10 @@ func extractAccessorID(authz interface{}) string { } return accessor } + +func ACLResourceNotExistError(resourceType string, entMeta EnterpriseMeta) error { + if ns := entMeta.NamespaceOrEmpty(); ns != "" { + return fmt.Errorf("Requested %s not found in namespace %s: %w", resourceType, ns, ErrNotFound) + } + return fmt.Errorf("Requested %s does not exist: %w", resourceType, ErrNotFound) +} diff --git a/agent/acl_endpoint.go b/agent/acl_endpoint.go index 3c5f6c7d6..af9f3a15d 100644 --- a/agent/acl_endpoint.go +++ b/agent/acl_endpoint.go @@ -162,12 +162,15 @@ func (s *HTTPHandlers) ACLPolicyRead(resp http.ResponseWriter, req *http.Request var out structs.ACLPolicyResponse defer setMeta(resp, &out.QueryMeta) if err := s.agent.RPC(req.Context(), "ACL.PolicyRead", &args, &out); err != nil { + // should return permission denied error if missing permissions return nil, err } if out.Policy == nil { - // TODO(rb): should this return a normal 404? - return nil, acl.ErrNotFound + // if no error was returned above, the policy does not exist + resp.WriteHeader(http.StatusNotFound) + msg := acl.ACLResourceNotExistError("policy", args.EnterpriseMeta) + return nil, HTTPError{StatusCode: http.StatusNotFound, Reason: msg.Error()} } return out.Policy, nil @@ -247,6 +250,10 @@ func (s *HTTPHandlers) ACLPolicyDelete(resp http.ResponseWriter, req *http.Reque var ignored string if err := s.agent.RPC(req.Context(), "ACL.PolicyDelete", args, &ignored); err != nil { + if strings.Contains(err.Error(), acl.ErrNotFound.Error()) { + resp.WriteHeader(http.StatusNotFound) + return nil, HTTPError{StatusCode: http.StatusNotFound, Reason: "Cannot find policy to delete"} + } return nil, err } @@ -346,11 +353,14 @@ func (s *HTTPHandlers) ACLTokenSelf(resp http.ResponseWriter, req *http.Request) var out structs.ACLTokenResponse defer setMeta(resp, &out.QueryMeta) if err := s.agent.RPC(req.Context(), "ACL.TokenRead", &args, &out); err != nil { + // should return permission denied error if missing permissions return nil, err } if out.Token == nil { - return nil, acl.ErrNotFound + // if no error was returned above, the token does not exist + resp.WriteHeader(http.StatusNotFound) + return nil, HTTPError{StatusCode: http.StatusNotFound, Reason: "Supplied token does not exist"} } return out.Token, nil @@ -393,7 +403,10 @@ func (s *HTTPHandlers) ACLTokenGet(resp http.ResponseWriter, req *http.Request, } if out.Token == nil { - return nil, acl.ErrNotFound + // if no error was returned above, the token does not exist + resp.WriteHeader(http.StatusNotFound) + msg := acl.ACLResourceNotExistError("token", args.EnterpriseMeta) + return nil, HTTPError{StatusCode: http.StatusNotFound, Reason: msg.Error()} } if args.Expanded { @@ -449,6 +462,10 @@ func (s *HTTPHandlers) ACLTokenDelete(resp http.ResponseWriter, req *http.Reques var ignored string if err := s.agent.RPC(req.Context(), "ACL.TokenDelete", args, &ignored); err != nil { + if strings.Contains(err.Error(), acl.ErrNotFound.Error()) { + resp.WriteHeader(http.StatusNotFound) + return nil, HTTPError{StatusCode: http.StatusNotFound, Reason: "Cannot find token to delete"} + } return nil, err } return true, nil @@ -582,12 +599,15 @@ func (s *HTTPHandlers) ACLRoleRead(resp http.ResponseWriter, req *http.Request, var out structs.ACLRoleResponse defer setMeta(resp, &out.QueryMeta) if err := s.agent.RPC(req.Context(), "ACL.RoleRead", &args, &out); err != nil { + // should return permission denied error if missing permissions return nil, err } if out.Role == nil { + // if not permission denied error is returned above, role does not exist resp.WriteHeader(http.StatusNotFound) - return nil, nil + msg := acl.ACLResourceNotExistError("role", args.EnterpriseMeta) + return nil, HTTPError{StatusCode: http.StatusNotFound, Reason: msg.Error()} } return out.Role, nil @@ -640,6 +660,10 @@ func (s *HTTPHandlers) ACLRoleDelete(resp http.ResponseWriter, req *http.Request var ignored string if err := s.agent.RPC(req.Context(), "ACL.RoleDelete", args, &ignored); err != nil { + if strings.Contains(err.Error(), acl.ErrNotFound.Error()) { + resp.WriteHeader(http.StatusNotFound) + return nil, HTTPError{StatusCode: http.StatusNotFound, Reason: "Cannot find role to delete"} + } return nil, err } @@ -729,12 +753,15 @@ func (s *HTTPHandlers) ACLBindingRuleRead(resp http.ResponseWriter, req *http.Re var out structs.ACLBindingRuleResponse defer setMeta(resp, &out.QueryMeta) if err := s.agent.RPC(req.Context(), "ACL.BindingRuleRead", &args, &out); err != nil { + // should return permission denied error if missing permissions return nil, err } if out.BindingRule == nil { + // if no error was returned above, the binding rule does not exist resp.WriteHeader(http.StatusNotFound) - return nil, nil + msg := acl.ACLResourceNotExistError("binding rule", args.EnterpriseMeta) + return nil, HTTPError{StatusCode: http.StatusNotFound, Reason: msg.Error()} } return out.BindingRule, nil @@ -786,6 +813,10 @@ func (s *HTTPHandlers) ACLBindingRuleDelete(resp http.ResponseWriter, req *http. var ignored bool if err := s.agent.RPC(req.Context(), "ACL.BindingRuleDelete", args, &ignored); err != nil { + if strings.Contains(err.Error(), acl.ErrNotFound.Error()) { + resp.WriteHeader(http.StatusNotFound) + return nil, HTTPError{StatusCode: http.StatusNotFound, Reason: "Cannot find binding rule to delete"} + } return nil, err } @@ -871,12 +902,15 @@ func (s *HTTPHandlers) ACLAuthMethodRead(resp http.ResponseWriter, req *http.Req var out structs.ACLAuthMethodResponse defer setMeta(resp, &out.QueryMeta) if err := s.agent.RPC(req.Context(), "ACL.AuthMethodRead", &args, &out); err != nil { + // should return permission denied if missing permissions return nil, err } if out.AuthMethod == nil { + // if no error was returned above, the auth method does not exist resp.WriteHeader(http.StatusNotFound) - return nil, nil + msg := acl.ACLResourceNotExistError("auth method", args.EnterpriseMeta) + return nil, HTTPError{StatusCode: http.StatusNotFound, Reason: msg.Error()} } fixupAuthMethodConfig(out.AuthMethod) @@ -932,6 +966,10 @@ func (s *HTTPHandlers) ACLAuthMethodDelete(resp http.ResponseWriter, req *http.R var ignored bool if err := s.agent.RPC(req.Context(), "ACL.AuthMethodDelete", args, &ignored); err != nil { + if strings.Contains(err.Error(), acl.ErrNotFound.Error()) { + resp.WriteHeader(http.StatusNotFound) + return nil, HTTPError{StatusCode: http.StatusNotFound, Reason: "Cannot find auth method to delete"} + } return nil, err } @@ -976,7 +1014,7 @@ func (s *HTTPHandlers) ACLLogout(resp http.ResponseWriter, req *http.Request) (i s.parseToken(req, &args.Token) if args.Token == "" { - return nil, acl.ErrNotFound + return nil, HTTPError{StatusCode: http.StatusUnauthorized, Reason: "Supplied token does not exist"} } var ignored bool diff --git a/agent/acl_endpoint_test.go b/agent/acl_endpoint_test.go index 0708d2196..d2822e39e 100644 --- a/agent/acl_endpoint_test.go +++ b/agent/acl_endpoint_test.go @@ -1845,7 +1845,7 @@ func TestACL_LoginProcedure_HTTP(t *testing.T) { resp := httptest.NewRecorder() _, err := a.srv.ACLTokenCRUD(resp, req) require.Error(t, err) - require.True(t, acl.IsErrNotFound(err), err.Error()) + require.ErrorContains(t, err, acl.ErrNotFound.Error()) }) }) } @@ -2010,7 +2010,7 @@ func TestACLEndpoint_LoginLogout_jwt(t *testing.T) { // make the request _, err = a.srv.ACLTokenCRUD(resp, req) require.Error(t, err) - require.Equal(t, acl.ErrNotFound, err) + require.ErrorContains(t, err, acl.ErrNotFound.Error()) }) }) } diff --git a/agent/consul/acl_endpoint.go b/agent/consul/acl_endpoint.go index 1cbea7779..790bd71a5 100644 --- a/agent/consul/acl_endpoint.go +++ b/agent/consul/acl_endpoint.go @@ -314,19 +314,22 @@ func (a *ACL) TokenRead(args *structs.ACLTokenGetRequest, reply *structs.ACLToke // no extra validation is needed here. If you have the secret ID you can read it. } - if token != nil && token.IsExpired(time.Now()) { - token = nil - } - if err != nil { return err } + if token != nil && token.IsExpired(time.Now()) { + return fmt.Errorf("token has expired: %w", acl.ErrNotFound) + } else if token == nil { + // token does not exist + if ns := args.EnterpriseMeta.NamespaceOrEmpty(); ns != "" { + return fmt.Errorf("token not found in namespace %s: %w", ns, acl.ErrNotFound) + } + return fmt.Errorf("token does not exist: %w", acl.ErrNotFound) + } + reply.Index, reply.Token = index, token reply.SourceDatacenter = args.Datacenter - if token == nil { - return errNotFound - } if args.Expanded { info, err := a.lookupExpandedTokenInfo(ws, state, token) @@ -458,8 +461,13 @@ func (a *ACL) TokenClone(args *structs.ACLTokenSetRequest, reply *structs.ACLTok _, token, err := a.srv.fsm.State().ACLTokenGetByAccessor(nil, args.ACLToken.AccessorID, &args.ACLToken.EnterpriseMeta) if err != nil { return err - } else if token == nil || token.IsExpired(time.Now()) { - return acl.ErrNotFound + } else if token == nil { + if ns := args.ACLToken.EnterpriseMeta.NamespaceOrEmpty(); ns != "" { + return fmt.Errorf("token not found in namespace %s: %w", ns, acl.ErrNotFound) + } + return fmt.Errorf("token does not exist: %w", acl.ErrNotFound) + } else if token.IsExpired(time.Now()) { + return fmt.Errorf("token is expired: %w", acl.ErrNotFound) } else if !a.srv.InPrimaryDatacenter() && !token.Local { // global token writes must be forwarded to the primary DC args.Datacenter = a.srv.config.PrimaryDatacenter @@ -597,8 +605,11 @@ func (a *ACL) TokenDelete(args *structs.ACLTokenDeleteRequest, reply *string) er args.Datacenter = a.srv.config.PrimaryDatacenter return a.srv.forwardDC("ACL.TokenDelete", a.srv.config.PrimaryDatacenter, args, reply) } else { - // in Primary Datacenter but the token does not exist - return early as there is nothing to do. - return nil + // in Primary Datacenter but the token does not exist - return early indicating it wasn't found. + if ns := args.EnterpriseMeta.NamespaceOrEmpty(); ns != "" { + return fmt.Errorf("token not found in namespace %s: %w", ns, acl.ErrNotFound) + } + return fmt.Errorf("token does not exist: %w", acl.ErrNotFound) } req := &structs.ACLTokenBatchDeleteRequest{ @@ -979,7 +990,10 @@ func (a *ACL) PolicyDelete(args *structs.ACLPolicyDeleteRequest, reply *string) } if policy == nil { - return nil + if ns := args.EnterpriseMeta.NamespaceOrEmpty(); ns != "" { + return fmt.Errorf("policy not found in namespace %s: %w", ns, acl.ErrNotFound) + } + return fmt.Errorf("policy does not exist: %w", acl.ErrNotFound) } if policy.ID == structs.ACLPolicyGlobalManagementID { @@ -1393,7 +1407,10 @@ func (a *ACL) RoleDelete(args *structs.ACLRoleDeleteRequest, reply *string) erro } if role == nil { - return nil + if ns := args.EnterpriseMeta.NamespaceOrEmpty(); ns != "" { + return fmt.Errorf("role not found in namespace %s: %w", ns, acl.ErrNotFound) + } + return fmt.Errorf("role does not exist: %w", acl.ErrNotFound) } req := structs.ACLRoleBatchDeleteRequest{ @@ -1706,11 +1723,15 @@ func (a *ACL) BindingRuleDelete(args *structs.ACLBindingRuleDeleteRequest, reply } _, rule, err := a.srv.fsm.State().ACLBindingRuleGetByID(nil, args.BindingRuleID, &args.EnterpriseMeta) - switch { - case err != nil: + if err != nil { return err - case rule == nil: - return nil + } + + if rule == nil { + if ns := args.EnterpriseMeta.NamespaceOrEmpty(); ns != "" { + return fmt.Errorf("binding rule not found in namespace %s: %w", ns, acl.ErrNotFound) + } + return fmt.Errorf("binding rule does not exist: %w", acl.ErrNotFound) } req := structs.ACLBindingRuleBatchDeleteRequest{ @@ -1955,7 +1976,10 @@ func (a *ACL) AuthMethodDelete(args *structs.ACLAuthMethodDeleteRequest, reply * } if method == nil { - return nil + if ns := args.EnterpriseMeta.NamespaceOrEmpty(); ns != "" { + return fmt.Errorf("auth method not found in namespace %s: %w", ns, acl.ErrNotFound) + } + return fmt.Errorf("auth method does not exist: %w", acl.ErrNotFound) } if err := a.srv.enterpriseAuthMethodTypeValidation(method.Type); err != nil { @@ -2082,7 +2106,7 @@ func (a *ACL) Logout(args *structs.ACLLogoutRequest, reply *bool) error { } if args.Token == "" { - return acl.ErrNotFound + return fmt.Errorf("no valid token ID provided: %w", acl.ErrNotFound) } if done, err := a.srv.ForwardRPC("ACL.Logout", args, reply); done { diff --git a/agent/consul/acl_endpoint_test.go b/agent/consul/acl_endpoint_test.go index 0b5fddb6b..8bbf8379b 100644 --- a/agent/consul/acl_endpoint_test.go +++ b/agent/consul/acl_endpoint_test.go @@ -214,13 +214,16 @@ func TestACLEndpoint_TokenRead(t *testing.T) { resp := structs.ACLTokenResponse{} retry.Run(t, func(r *retry.R) { - require.NoError(r, aclEp.TokenRead(&req, &resp)) + time.Sleep(200 * time.Millisecond) + err := aclEp.TokenRead(&req, &resp) + require.Error(r, err) + require.ErrorContains(t, err, "ACL not found") require.Nil(r, resp.Token) }) }) }) - t.Run("nil when token does not exist", func(t *testing.T) { + t.Run("error when token does not exist", func(t *testing.T) { fakeID, err := uuid.GenerateUUID() require.NoError(t, err) @@ -235,7 +238,8 @@ func TestACLEndpoint_TokenRead(t *testing.T) { err = aclEp.TokenRead(&req, &resp) require.Nil(t, resp.Token) - require.NoError(t, err) + require.Error(t, err) + require.ErrorContains(t, err, acl.ErrNotFound.Error()) }) t.Run("validates ID format", func(t *testing.T) { @@ -507,7 +511,7 @@ func TestACLEndpoint_TokenClone(t *testing.T) { err = endpoint.TokenClone(&req, &t2) require.Error(t, err) - require.Equal(t, acl.ErrNotFound, err) + require.ErrorContains(t, err, "token is expired") }) } @@ -1647,7 +1651,8 @@ func TestACLEndpoint_TokenDelete(t *testing.T) { // Make sure the token is gone tokenResp, err = retrieveTestToken(codec, TestDefaultInitialManagementToken, "dc1", testToken.AccessorID) - require.NoError(t, err) + require.Error(t, err) + require.ErrorContains(t, err, acl.ErrNotFound.Error()) require.Nil(t, tokenResp.Token) }) @@ -1662,7 +1667,8 @@ func TestACLEndpoint_TokenDelete(t *testing.T) { // Make sure the token is not listable (filtered due to expiry) tokenResp, err := retrieveTestToken(codec, TestDefaultInitialManagementToken, "dc1", expiringToken.AccessorID) - require.NoError(t, err) + require.Error(t, err) + require.ErrorContains(t, err, "token has expired") require.Nil(t, tokenResp.Token) // Now try to delete it (this should work). @@ -1679,7 +1685,8 @@ func TestACLEndpoint_TokenDelete(t *testing.T) { // Make sure the token is still gone (this time it's actually gone) tokenResp, err = retrieveTestToken(codec, TestDefaultInitialManagementToken, "dc1", expiringToken.AccessorID) - require.NoError(t, err) + require.Error(t, err) + require.ErrorContains(t, err, acl.ErrNotFound.Error()) require.Nil(t, tokenResp.Token) }) @@ -1697,8 +1704,9 @@ func TestACLEndpoint_TokenDelete(t *testing.T) { // Make sure the token is gone tokenResp, err := retrieveTestToken(codec, TestDefaultInitialManagementToken, "dc1", existingToken.AccessorID) + require.Error(t, err) + require.ErrorContains(t, err, acl.ErrNotFound.Error()) require.Nil(t, tokenResp.Token) - require.NoError(t, err) }) t.Run("can't delete itself", func(t *testing.T) { @@ -1739,12 +1747,14 @@ func TestACLEndpoint_TokenDelete(t *testing.T) { var resp string err = acl1.TokenDelete(&req, &resp) - require.NoError(t, err) + require.Error(t, err) + require.ErrorIs(t, err, acl.ErrNotFound) // token should be nil tokenResp, err := retrieveTestToken(codec, TestDefaultInitialManagementToken, "dc1", existingToken.AccessorID) require.Nil(t, tokenResp.Token) - require.NoError(t, err) + require.Error(t, err) + require.ErrorContains(t, err, acl.ErrNotFound.Error()) }) t.Run("don't segfault when attempting to delete non existent token in secondary dc", func(t *testing.T) { @@ -1760,12 +1770,14 @@ func TestACLEndpoint_TokenDelete(t *testing.T) { var resp string err = acl2.TokenDelete(&req, &resp) - require.NoError(t, err) + require.Error(t, err) + require.ErrorContains(t, err, acl.ErrNotFound.Error()) // token should be nil tokenResp, err := retrieveTestToken(codec2, TestDefaultInitialManagementToken, "dc1", existingToken.AccessorID) require.Nil(t, tokenResp.Token) - require.NoError(t, err) + require.Error(t, err) + require.ErrorContains(t, err, acl.ErrNotFound.Error()) }) } @@ -3307,7 +3319,8 @@ func TestACLEndpoint_AuthMethodDelete(t *testing.T) { var ignored bool err = aclEp.AuthMethodDelete(&req, &ignored) - require.NoError(t, err) + require.Error(t, err) + require.ErrorContains(t, err, acl.ErrNotFound.Error()) }) } @@ -3413,7 +3426,8 @@ func TestACLEndpoint_AuthMethodDelete_RuleAndTokenCascade(t *testing.T) { } for _, id := range []string{i1_t1.AccessorID, i1_t2.AccessorID} { tokResp, err := retrieveTestToken(codec, TestDefaultInitialManagementToken, "dc1", id) - require.NoError(t, err) + require.Error(t, err) + require.ErrorContains(t, err, acl.ErrNotFound.Error()) require.Nil(t, tokResp.Token) } @@ -3774,7 +3788,8 @@ func TestACLEndpoint_BindingRuleDelete(t *testing.T) { var ignored bool err = aclEp.BindingRuleDelete(&req, &ignored) - require.NoError(t, err) + require.Error(t, err) + require.ErrorContains(t, err, acl.ErrNotFound.Error()) }) } @@ -4197,7 +4212,8 @@ func TestACLEndpoint_SecureIntroEndpoints_OnlyCreateLocalData(t *testing.T) { require.Equal(t, "web2", resp2.Token.ServiceIdentities[0].ServiceName) // absent in dc1 resp2, err = retrieveTestToken(codec1, TestDefaultInitialManagementToken, "dc1", remoteToken.AccessorID) - require.NoError(t, err) + require.Error(t, err) + require.ErrorContains(t, err, acl.ErrNotFound.Error()) require.Nil(t, resp2.Token) }) @@ -4256,7 +4272,8 @@ func TestACLEndpoint_SecureIntroEndpoints_OnlyCreateLocalData(t *testing.T) { require.Equal(t, "web1", resp2.Token.ServiceIdentities[0].ServiceName) // absent in dc2 resp2, err = retrieveTestToken(codec2, TestDefaultInitialManagementToken, "dc2", primaryToken.AccessorID) - require.NoError(t, err) + require.Error(t, err) + require.ErrorContains(t, err, acl.ErrNotFound.Error()) require.Nil(t, resp2.Token) }) @@ -4274,11 +4291,13 @@ func TestACLEndpoint_SecureIntroEndpoints_OnlyCreateLocalData(t *testing.T) { // absent in dc2 resp2, err := retrieveTestToken(codec2, TestDefaultInitialManagementToken, "dc2", remoteToken.AccessorID) - require.NoError(t, err) + require.Error(t, err) + require.ErrorContains(t, err, acl.ErrNotFound.Error()) require.Nil(t, resp2.Token) // absent in dc1 resp2, err = retrieveTestToken(codec1, TestDefaultInitialManagementToken, "dc1", remoteToken.AccessorID) - require.NoError(t, err) + require.Error(t, err) + require.ErrorContains(t, err, acl.ErrNotFound.Error()) require.Nil(t, resp2.Token) }) @@ -4299,7 +4318,8 @@ func TestACLEndpoint_SecureIntroEndpoints_OnlyCreateLocalData(t *testing.T) { require.Equal(t, "web1", resp2.Token.ServiceIdentities[0].ServiceName) // absent in dc2 resp2, err = retrieveTestToken(codec2, TestDefaultInitialManagementToken, "dc2", primaryToken.AccessorID) - require.NoError(t, err) + require.Error(t, err) + require.ErrorContains(t, err, acl.ErrNotFound.Error()) require.Nil(t, resp2.Token) }) @@ -5458,11 +5478,7 @@ func retrieveTestToken(codec rpc.ClientCodec, initialManagementToken string, dat err := msgpackrpc.CallWithCodec(codec, "ACL.TokenRead", &arg, &out) - if err != nil { - return nil, err - } - - return &out, nil + return &out, err } func deleteTestToken(codec rpc.ClientCodec, initialManagementToken string, datacenter string, tokenAccessor string) error { diff --git a/agent/consul/acl_replication_test.go b/agent/consul/acl_replication_test.go index a0c2576cf..c3c564fed 100644 --- a/agent/consul/acl_replication_test.go +++ b/agent/consul/acl_replication_test.go @@ -754,8 +754,8 @@ func TestACLReplication_TokensRedacted(t *testing.T) { QueryOptions: structs.QueryOptions{Token: aclfilter.RedactedToken}, } err := s2.RPC(context.Background(), "ACL.TokenRead", &req, &tokenResp) - // its not an error for the secret to not be found. - require.NoError(r, err) + require.Error(r, err) + require.ErrorContains(r, err, "token does not exist") require.Nil(r, tokenResp.Token) var status structs.ACLReplicationStatus diff --git a/agent/consul/acl_server.go b/agent/consul/acl_server.go index cc7e3a766..9ee36448f 100644 --- a/agent/consul/acl_server.go +++ b/agent/consul/acl_server.go @@ -142,7 +142,6 @@ func (s *Server) ResolveIdentityFromToken(token string) (bool, structs.ACLIdenti if !s.InPrimaryDatacenter() && !s.config.ACLTokenReplication { return false, nil, nil } - index, aclToken, err := s.fsm.State().ACLTokenGetBySecret(nil, token, nil) if err != nil { return true, nil, err @@ -150,7 +149,12 @@ func (s *Server) ResolveIdentityFromToken(token string) (bool, structs.ACLIdenti return true, aclToken, nil } - return s.InPrimaryDatacenter() || index > 0, nil, acl.ErrNotFound + defaultErr := acl.ErrNotFound + canBootstrap, _, _ := s.fsm.State().CanBootstrapACLToken() + if canBootstrap { + defaultErr = fmt.Errorf("ACL system must be bootstrapped before making any requests that require authorization: %w", defaultErr) + } + return s.InPrimaryDatacenter() || index > 0, nil, defaultErr } func (s *serverACLResolverBackend) ResolvePolicyFromID(policyID string) (bool, *structs.ACLPolicy, error) { diff --git a/api/acl_test.go b/api/acl_test.go index 7f73e5b73..1cf8708d2 100644 --- a/api/acl_test.go +++ b/api/acl_test.go @@ -289,6 +289,28 @@ func prepTokenPoliciesInPartition(t *testing.T, acl *ACL, partition string) (pol return } +func TestAPI_ACLBootstrap(t *testing.T) { + t.Parallel() + c, s := makeNonBootstrappedACLClient(t) + defer s.Stop() + + acl := c.ACL() + s.WaitForLeader(t) + + // not bootstrapped + _, _, err := acl.TokenList(nil) + require.EqualError(t, err, "Unexpected response code: 403 (ACL system must be bootstrapped before making any requests that require authorization: ACL not found)") + // bootstrap + mgmtTok, _, err := acl.Bootstrap() + require.NoError(t, err) + // bootstrapped + acl.c.config.Token = mgmtTok.SecretID + toks, _, err := acl.TokenList(nil) + require.NoError(t, err) + // management and anonymous should be only tokens + require.Len(t, toks, 2) +} + func TestAPI_ACLToken_CreateReadDelete(t *testing.T) { t.Parallel() c, s := makeACLClient(t) diff --git a/api/api_test.go b/api/api_test.go index ea1578c49..94eadd959 100644 --- a/api/api_test.go +++ b/api/api_test.go @@ -50,6 +50,19 @@ func makeACLClient(t *testing.T) (*Client, *testutil.TestServer) { }) } +func makeNonBootstrappedACLClient(t *testing.T) (*Client, *testutil.TestServer) { + return makeClientWithConfig(t, + func(clientConfig *Config) { + clientConfig.Token = "root" + }, + func(serverConfig *testutil.TestServerConfig) { + serverConfig.PrimaryDatacenter = "dc1" + serverConfig.ACL.Enabled = true + serverConfig.ACL.DefaultPolicy = "deny" + serverConfig.Bootstrap = true + }) +} + func makeClientWithCA(t *testing.T) (*Client, *testutil.TestServer) { return makeClientWithConfig(t, func(c *Config) { diff --git a/command/acl/acl_helpers.go b/command/acl/acl_helpers.go index 39c6f975e..c571934ed 100644 --- a/command/acl/acl_helpers.go +++ b/command/acl/acl_helpers.go @@ -163,7 +163,7 @@ func GetBindingRuleIDFromPartial(client *api.Client, partialID string) (string, } if ruleID == "" { - return "", fmt.Errorf("No such rule ID with prefix: %s", partialID) + return "", fmt.Errorf("no such rule ID with prefix: %s: %w", partialID, acl.ErrNotFound) } return ruleID, nil diff --git a/command/acl/authmethod/delete/authmethod_delete_test.go b/command/acl/authmethod/delete/authmethod_delete_test.go index 61ee7b169..f64f4d01f 100644 --- a/command/acl/authmethod/delete/authmethod_delete_test.go +++ b/command/acl/authmethod/delete/authmethod_delete_test.go @@ -5,13 +5,14 @@ import ( "strings" "testing" - "github.com/hashicorp/consul/agent" - "github.com/hashicorp/consul/api" - "github.com/hashicorp/consul/testrpc" "github.com/hashicorp/go-uuid" "github.com/mitchellh/cli" "github.com/stretchr/testify/require" + "github.com/hashicorp/consul/agent" + "github.com/hashicorp/consul/api" + "github.com/hashicorp/consul/testrpc" + // activate testing auth method _ "github.com/hashicorp/consul/agent/consul/authmethod/testauth" ) @@ -70,12 +71,11 @@ func TestAuthMethodDeleteCommand(t *testing.T) { } code := cmd.Run(args) - require.Equal(t, code, 0) - require.Empty(t, ui.ErrorWriter.String()) + require.Equal(t, 1, code) + require.Contains(t, ui.ErrorWriter.String(), "404 (Cannot find auth method to delete)") output := ui.OutputWriter.String() - require.Contains(t, output, fmt.Sprintf("deleted successfully")) - require.Contains(t, output, "notfound") + require.Empty(t, output) }) createAuthMethod := func(t *testing.T) string { diff --git a/command/acl/bindingrule/delete/bindingrule_delete_test.go b/command/acl/bindingrule/delete/bindingrule_delete_test.go index 1c05cd610..c84ba38c4 100644 --- a/command/acl/bindingrule/delete/bindingrule_delete_test.go +++ b/command/acl/bindingrule/delete/bindingrule_delete_test.go @@ -5,11 +5,12 @@ import ( "strings" "testing" + "github.com/mitchellh/cli" + "github.com/stretchr/testify/require" + "github.com/hashicorp/consul/agent" "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/testrpc" - "github.com/mitchellh/cli" - "github.com/stretchr/testify/require" // activate testing auth method _ "github.com/hashicorp/consul/agent/consul/authmethod/testauth" @@ -180,4 +181,22 @@ func TestBindingRuleDeleteCommand(t *testing.T) { require.Equal(t, code, 1) require.Contains(t, ui.ErrorWriter.String(), "Error determining binding rule ID") }) + + t.Run("delete notfound", func(t *testing.T) { + ui := cli.NewMockUi() + cmd := New(ui) + + args := []string{ + "-http-addr=" + a.HTTPAddr(), + "-token=root", + "-id=notfound", + } + + code := cmd.Run(args) + require.Equal(t, 1, code) + require.Contains(t, ui.ErrorWriter.String(), "Error determining binding rule ID: no such rule ID with prefix: notfound") + + output := ui.OutputWriter.String() + require.Empty(t, output) + }) } diff --git a/command/acl/policy/delete/policy_delete_test.go b/command/acl/policy/delete/policy_delete_test.go index 3a7660dbc..3df6c2053 100644 --- a/command/acl/policy/delete/policy_delete_test.go +++ b/command/acl/policy/delete/policy_delete_test.go @@ -5,11 +5,12 @@ import ( "strings" "testing" + "github.com/mitchellh/cli" + "github.com/stretchr/testify/assert" + "github.com/hashicorp/consul/agent" "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/testrpc" - "github.com/mitchellh/cli" - "github.com/stretchr/testify/assert" ) func TestPolicyDeleteCommand_noTabs(t *testing.T) { @@ -69,5 +70,5 @@ func TestPolicyDeleteCommand(t *testing.T) { policy.ID, &api.QueryOptions{Token: "root"}, ) - assert.EqualError(t, err, "Unexpected response code: 403 (ACL not found)") + assert.EqualError(t, err, "Unexpected response code: 404 (Requested policy does not exist: ACL not found)") } diff --git a/command/acl/token/delete/token_delete_test.go b/command/acl/token/delete/token_delete_test.go index be01fdf22..4cf74c0f3 100644 --- a/command/acl/token/delete/token_delete_test.go +++ b/command/acl/token/delete/token_delete_test.go @@ -70,5 +70,6 @@ func TestTokenDeleteCommand(t *testing.T) { token.AccessorID, &api.QueryOptions{Token: "root"}, ) - assert.EqualError(t, err, "Unexpected response code: 403 (ACL not found)") + assert.ErrorContains(t, err, "Unexpected response code: 403") + assert.ErrorContains(t, err, "ACL not found") } diff --git a/command/logout/logout_test.go b/command/logout/logout_test.go index e41a33ef4..4ec8bf1d2 100644 --- a/command/logout/logout_test.go +++ b/command/logout/logout_test.go @@ -55,7 +55,7 @@ func TestLogoutCommand(t *testing.T) { code := cmd.Run(args) require.Equal(t, code, 1, "err: %s", ui.ErrorWriter.String()) - require.Contains(t, ui.ErrorWriter.String(), "403 (ACL not found)") + require.Contains(t, ui.ErrorWriter.String(), "401 (Supplied token does not exist)") }) t.Run("logout of deleted token", func(t *testing.T) { @@ -185,7 +185,7 @@ func TestLogoutCommand_k8s(t *testing.T) { code := cmd.Run(args) require.Equal(t, code, 1, "err: %s", ui.ErrorWriter.String()) - require.Contains(t, ui.ErrorWriter.String(), "403 (ACL not found)") + require.Contains(t, ui.ErrorWriter.String(), "401 (Supplied token does not exist)") }) t.Run("logout of deleted token", func(t *testing.T) {