diff --git a/command/agent/acl_endpoint.go b/command/agent/acl_endpoint.go index 487130799..72ae4c20a 100644 --- a/command/agent/acl_endpoint.go +++ b/command/agent/acl_endpoint.go @@ -2,9 +2,10 @@ package agent import ( "fmt" - "github.com/hashicorp/consul/consul/structs" "net/http" "strings" + + "github.com/hashicorp/consul/consul/structs" ) // aclCreateResponse is used to wrap the ACL ID @@ -80,14 +81,8 @@ func (s *HTTPServer) aclSet(resp http.ResponseWriter, req *http.Request, update } } - // Ensure there is no ID set for create - if !update && args.ACL.ID != "" { - resp.WriteHeader(400) - resp.Write([]byte(fmt.Sprintf("ACL ID cannot be set"))) - return nil, nil - } - - // Ensure there is an ID set for update + // Ensure there is an ID set for update. ID is optional for + // create, as one will be generated if not provided. if update && args.ACL.ID == "" { resp.WriteHeader(400) resp.Write([]byte(fmt.Sprintf("ACL ID must be set"))) diff --git a/command/agent/acl_endpoint_test.go b/command/agent/acl_endpoint_test.go index 84af0604a..b0dea0c9b 100644 --- a/command/agent/acl_endpoint_test.go +++ b/command/agent/acl_endpoint_test.go @@ -3,10 +3,11 @@ package agent import ( "bytes" "encoding/json" - "github.com/hashicorp/consul/consul/structs" "net/http" "net/http/httptest" "testing" + + "github.com/hashicorp/consul/consul/structs" ) func makeTestACL(t *testing.T, srv *HTTPServer) string { @@ -62,6 +63,34 @@ func TestACLUpdate(t *testing.T) { }) } +func TestACLUpdate_Upsert(t *testing.T) { + httpTest(t, func(srv *HTTPServer) { + body := bytes.NewBuffer(nil) + enc := json.NewEncoder(body) + raw := map[string]interface{}{ + "ID": "my-old-id", + "Name": "User Token 2", + "Type": "client", + "Rules": "", + } + enc.Encode(raw) + + req, err := http.NewRequest("PUT", "/v1/acl/update?token=root", body) + if err != nil { + t.Fatalf("err: %v", err) + } + resp := httptest.NewRecorder() + obj, err := srv.ACLUpdate(resp, req) + if err != nil { + t.Fatalf("err: %v", err) + } + aclResp := obj.(aclCreateResponse) + if aclResp.ID != "my-old-id" { + t.Fatalf("bad: %v", aclResp) + } + }) +} + func TestACLDestroy(t *testing.T) { httpTest(t, func(srv *HTTPServer) { id := makeTestACL(t, srv) diff --git a/consul/acl_endpoint.go b/consul/acl_endpoint.go index 6a7cfc472..f3c162b98 100644 --- a/consul/acl_endpoint.go +++ b/consul/acl_endpoint.go @@ -55,22 +55,12 @@ func (a *ACL) Apply(args *structs.ACLRequest, reply *string) error { return fmt.Errorf("ACL rule compilation failed: %v", err) } - // Check if this is an update - state := a.srv.fsm.State() - var existing *structs.ACL - if args.ACL.ID != "" { - _, existing, err = state.ACLGet(args.ACL.ID) - if err != nil { - a.srv.logger.Printf("[ERR] consul.acl: ACL lookup failed: %v", err) - return err - } - } - - // If this is a create, generate a new ID. This must + // 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 existing == nil { + if args.ACL.ID == "" { + state := a.srv.fsm.State() for { args.ACL.ID = generateUUID() _, acl, err := state.ACLGet(args.ACL.ID) diff --git a/consul/acl_endpoint_test.go b/consul/acl_endpoint_test.go index 4c2e4410a..11a305a94 100644 --- a/consul/acl_endpoint_test.go +++ b/consul/acl_endpoint_test.go @@ -148,6 +148,53 @@ func TestACLEndpoint_Update_PurgeCache(t *testing.T) { } } +func TestACLEndpoint_Apply_CustomID(t *testing.T) { + dir1, s1 := testServerWithConfig(t, func(c *Config) { + c.ACLDatacenter = "dc1" + c.ACLMasterToken = "root" + }) + defer os.RemoveAll(dir1) + defer s1.Shutdown() + client := rpcClient(t, s1) + defer client.Close() + + testutil.WaitForLeader(t, client.Call, "dc1") + + arg := structs.ACLRequest{ + Datacenter: "dc1", + Op: structs.ACLSet, + ACL: structs.ACL{ + ID: "foobarbaz", // Specify custom ID, does not exist + Name: "User token", + Type: structs.ACLTypeClient, + }, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + var out string + if err := client.Call("ACL.Apply", &arg, &out); err != nil { + t.Fatalf("err: %v", err) + } + if out != "foobarbaz" { + t.Fatalf("bad token ID: %s", out) + } + + // Verify + state := s1.fsm.State() + _, s, err := state.ACLGet(out) + if err != nil { + t.Fatalf("err: %v", err) + } + if s == nil { + t.Fatalf("should not be nil") + } + if s.ID != out { + t.Fatalf("bad: %v", s) + } + if s.Name != "User token" { + t.Fatalf("bad: %v", s) + } +} + func TestACLEndpoint_Apply_Denied(t *testing.T) { dir1, s1 := testServerWithConfig(t, func(c *Config) { c.ACLDatacenter = "dc1" diff --git a/website/source/docs/agent/http/acl.html.markdown b/website/source/docs/agent/http/acl.html.markdown index b3b50abbb..b6b30c575 100644 --- a/website/source/docs/agent/http/acl.html.markdown +++ b/website/source/docs/agent/http/acl.html.markdown @@ -54,6 +54,10 @@ None of the fields are mandatory. In fact, no body needs to be PUT if the defaults are to be used. The `Name` and `Rules` fields default to being blank, and the `Type` defaults to "client". +The `ID` field may be provided, and if omitted a random UUID will be generated. +The security of the ACL system depends on the difficulty of guessing the token. +Tokens should not be generated in a predictable manner or with too little entropy. + The format of the `Rules` property is [documented here](/docs/internals/acl.html). A successful response body will return the `ID` of the newly created ACL, like so: @@ -69,8 +73,8 @@ A successful response body will return the `ID` of the newly created ACL, like s The update endpoint is used to modify the policy for a given ACL token. It is very similar to the create endpoint; however, instead of generating a new token ID, the `ID` field must be provided. As with [`/v1/acl/create`](#acl_create), -requests to this endpoint must be made with a management -token. +requests to this endpoint must be made with a management token. If the ID does not +exist, the ACL will be upserted. In this sense, create and update are identical. In any Consul cluster, only a single datacenter is authoritative for ACLs, so all requests are automatically routed to that datacenter regardless