From 8ff1f481fe6f2ea2c3f7ba28a3739bacc1c0a753 Mon Sep 17 00:00:00 2001 From: Sarah Adams Date: Mon, 12 Aug 2019 11:11:11 -0700 Subject: [PATCH] add flag to allow /operator/keyring requests to only hit local servers (#6279) Add parameter local-only to operator keyring list requests to force queries to only hit local servers (no WAN traffic). HTTP API: GET /operator/keyring?local-only=true CLI: consul keyring -list --local-only Sending the local-only flag with any non-GET/list request will result in an error. --- agent/consul/internal_endpoint.go | 19 ++- agent/consul/internal_endpoint_test.go | 119 +++++++++++++++++- agent/keyring.go | 9 ++ agent/keyring_test.go | 8 ++ agent/operator_endpoint.go | 43 +++---- agent/operator_endpoint_test.go | 46 +++++-- agent/structs/structs.go | 1 + api/api.go | 7 ++ api/api_test.go | 4 + command/keyring/keyring.go | 13 +- command/keyring/keyring_test.go | 12 ++ website/source/api/operator/keyring.html.md | 6 +- .../docs/commands/keyring.html.markdown.erb | 8 +- 13 files changed, 257 insertions(+), 38 deletions(-) diff --git a/agent/consul/internal_endpoint.go b/agent/consul/internal_endpoint.go index 9557ecbe32..166554819f 100644 --- a/agent/consul/internal_endpoint.go +++ b/agent/consul/internal_endpoint.go @@ -178,11 +178,20 @@ func (m *Internal) KeyringOperation( } } - // Only perform WAN keyring querying and RPC forwarding once - if !args.Forwarded && m.srv.serfWAN != nil { - args.Forwarded = true - m.executeKeyringOp(args, reply, true) - return m.srv.globalRPC("Internal.KeyringOperation", args, reply) + // Validate use of local-only + if args.LocalOnly && args.Operation != structs.KeyringList { + // Error aggressively to be clear about LocalOnly behavior + return fmt.Errorf("argument error: LocalOnly can only be used for List operations") + } + + // args.LocalOnly should always be false for non-GET requests + if !args.LocalOnly { + // Only perform WAN keyring querying and RPC forwarding once + if !args.Forwarded && m.srv.serfWAN != nil { + args.Forwarded = true + m.executeKeyringOp(args, reply, true) + return m.srv.globalRPC("Internal.KeyringOperation", args, reply) + } } // Query the LAN keyring of this node's DC diff --git a/agent/consul/internal_endpoint_test.go b/agent/consul/internal_endpoint_test.go index eee9569f43..a865908e59 100644 --- a/agent/consul/internal_endpoint_test.go +++ b/agent/consul/internal_endpoint_test.go @@ -3,6 +3,7 @@ package consul import ( "encoding/base64" "os" + "strings" "testing" "github.com/hashicorp/consul/acl" @@ -10,7 +11,7 @@ import ( "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/lib" "github.com/hashicorp/consul/testrpc" - "github.com/hashicorp/net-rpc-msgpackrpc" + msgpackrpc "github.com/hashicorp/net-rpc-msgpackrpc" "github.com/stretchr/testify/require" ) @@ -287,7 +288,7 @@ func TestInternal_KeyringOperation(t *testing.T) { // 3 responses (one from each DC LAN, one from WAN) in two-node cluster if len(out2.Responses) != 3 { - t.Fatalf("bad: %#v", out) + t.Fatalf("bad: %#v", out2) } wanResp, lanResp = 0, 0 for _, resp := range out2.Responses { @@ -302,6 +303,120 @@ func TestInternal_KeyringOperation(t *testing.T) { } } +func TestInternal_KeyringOperationList_LocalOnly(t *testing.T) { + t.Parallel() + key1 := "H1dfkSZOVnP/JUnaBfTzXg==" + keyBytes1, err := base64.StdEncoding.DecodeString(key1) + if err != nil { + t.Fatalf("err: %s", err) + } + dir1, s1 := testServerWithConfig(t, func(c *Config) { + c.SerfLANConfig.MemberlistConfig.SecretKey = keyBytes1 + c.SerfWANConfig.MemberlistConfig.SecretKey = keyBytes1 + }) + defer os.RemoveAll(dir1) + defer s1.Shutdown() + codec := rpcClient(t, s1) + defer codec.Close() + + testrpc.WaitForLeader(t, s1.RPC, "dc1") + + // Start a second agent to test cross-dc queries + dir2, s2 := testServerWithConfig(t, func(c *Config) { + c.SerfLANConfig.MemberlistConfig.SecretKey = keyBytes1 + c.SerfWANConfig.MemberlistConfig.SecretKey = keyBytes1 + c.Datacenter = "dc2" + }) + defer os.RemoveAll(dir2) + defer s2.Shutdown() + + // Try to join + joinWAN(t, s2, s1) + + // -- + // Try request with `LocalOnly` set to true + var out structs.KeyringResponses + req := structs.KeyringRequest{ + Operation: structs.KeyringList, + LocalOnly: true, + } + if err := msgpackrpc.CallWithCodec(codec, "Internal.KeyringOperation", &req, &out); err != nil { + t.Fatalf("err: %v", err) + } + + // 1 response (from this DC LAN) + if len(out.Responses) != 1 { + t.Fatalf("expected num responses to be 1, got %d; out is: %#v", len(out.Responses), out) + } + wanResp, lanResp := 0, 0 + for _, resp := range out.Responses { + if resp.WAN { + wanResp++ + } else { + lanResp++ + } + } + if lanResp != 1 || wanResp != 0 { + t.Fatalf("should have 1 lan and 0 wan response, got (lan=%d) (wan=%d)", lanResp, wanResp) + } + + // -- + // Try same request again but with `LocalOnly` set to false + req.LocalOnly = false + if err := msgpackrpc.CallWithCodec(codec, "Internal.KeyringOperation", &req, &out); err != nil { + t.Fatalf("err: %v", err) + } + + // 3 responses (one from each DC LAN, one from WAN) + if len(out.Responses) != 3 { + t.Fatalf("expected num responses to be 3, got %d; out is: %#v", len(out.Responses), out) + } + wanResp, lanResp = 0, 0 + for _, resp := range out.Responses { + if resp.WAN { + wanResp++ + } else { + lanResp++ + } + } + if lanResp != 2 || wanResp != 1 { + t.Fatalf("should have 2 lan and 1 wan response, got (lan=%d) (wan=%d)", lanResp, wanResp) + } +} + +func TestInternal_KeyringOperationWrite_LocalOnly(t *testing.T) { + t.Parallel() + key1 := "H1dfkSZOVnP/JUnaBfTzXg==" + keyBytes1, err := base64.StdEncoding.DecodeString(key1) + if err != nil { + t.Fatalf("err: %s", err) + } + dir1, s1 := testServerWithConfig(t, func(c *Config) { + c.SerfLANConfig.MemberlistConfig.SecretKey = keyBytes1 + c.SerfWANConfig.MemberlistConfig.SecretKey = keyBytes1 + }) + defer os.RemoveAll(dir1) + defer s1.Shutdown() + codec := rpcClient(t, s1) + defer codec.Close() + + testrpc.WaitForLeader(t, s1.RPC, "dc1") + + // Try request with `LocalOnly` set to true + var out structs.KeyringResponses + req := structs.KeyringRequest{ + Operation: structs.KeyringRemove, + LocalOnly: true, + } + err = msgpackrpc.CallWithCodec(codec, "Internal.KeyringOperation", &req, &out) + if err == nil { + t.Fatalf("expected an error") + } + if !strings.Contains(err.Error(), "LocalOnly") { + t.Fatalf("expected error to contain string 'LocalOnly'. Got: %v", err) + } +} + func TestInternal_NodeInfo_FilterACL(t *testing.T) { t.Parallel() dir, token, srv, codec := testACLFilterServer(t) diff --git a/agent/keyring.go b/agent/keyring.go index 8f00853dd4..6b0c48a5a9 100644 --- a/agent/keyring.go +++ b/agent/keyring.go @@ -130,6 +130,15 @@ func ParseRelayFactor(n int) (uint8, error) { return uint8(n), nil } +// ValidateLocalOnly validates the local-only flag, requiring that it only be +// set for list requests. +func ValidateLocalOnly(local bool, list bool) error { + if local && !list { + return fmt.Errorf("local-only can only be set for list requests") + } + return nil +} + // ListKeys lists out all keys installed on the collective Consul cluster. This // includes both servers and clients in all DC's. func (a *Agent) ListKeys(token string, relayFactor uint8) (*structs.KeyringResponses, error) { diff --git a/agent/keyring_test.go b/agent/keyring_test.go index 9812577944..ee8a065752 100644 --- a/agent/keyring_test.go +++ b/agent/keyring_test.go @@ -12,6 +12,7 @@ import ( "github.com/hashicorp/consul/sdk/testutil" "github.com/hashicorp/memberlist" + "github.com/stretchr/testify/require" ) func checkForKey(key string, keyring *memberlist.Keyring) error { @@ -332,3 +333,10 @@ func TestAgentKeyring_ACL(t *testing.T) { t.Fatalf("err: %s", err) } } + +func TestValidateLocalOnly(t *testing.T) { + require.NoError(t, ValidateLocalOnly(false, false)) + require.NoError(t, ValidateLocalOnly(true, true)) + + require.Error(t, ValidateLocalOnly(true, false)) +} diff --git a/agent/operator_endpoint.go b/agent/operator_endpoint.go index 34aa90d935..e46685e793 100644 --- a/agent/operator_endpoint.go +++ b/agent/operator_endpoint.go @@ -47,14 +47,10 @@ func (s *HTTPServer) OperatorRaftPeer(resp http.ResponseWriter, req *http.Reques } if !hasID && !hasAddress { - resp.WriteHeader(http.StatusBadRequest) - fmt.Fprint(resp, "Must specify either ?id with the server's ID or ?address with IP:port of peer to remove") - return nil, nil + return nil, BadRequestError{Reason: "Must specify either ?id with the server's ID or ?address with IP:port of peer to remove"} } if hasID && hasAddress { - resp.WriteHeader(http.StatusBadRequest) - fmt.Fprint(resp, "Must specify only one of ?id or ?address") - return nil, nil + return nil, BadRequestError{Reason: "Must specify only one of ?id or ?address"} } var reply struct{} @@ -73,6 +69,7 @@ type keyringArgs struct { Key string Token string RelayFactor uint8 + LocalOnly bool // ?local-only; only used for GET requests } // OperatorKeyringEndpoint handles keyring operations (install, list, use, remove) @@ -80,9 +77,7 @@ func (s *HTTPServer) OperatorKeyringEndpoint(resp http.ResponseWriter, req *http var args keyringArgs if req.Method == "POST" || req.Method == "PUT" || req.Method == "DELETE" { if err := decodeBody(req, &args, nil); err != nil { - resp.WriteHeader(http.StatusBadRequest) - fmt.Fprintf(resp, "Request decode failed: %v", err) - return nil, nil + return nil, BadRequestError{Reason: fmt.Sprintf("Request decode failed: %v", err)} } } s.parseToken(req, &args.Token) @@ -91,16 +86,26 @@ func (s *HTTPServer) OperatorKeyringEndpoint(resp http.ResponseWriter, req *http if relayFactor := req.URL.Query().Get("relay-factor"); relayFactor != "" { n, err := strconv.Atoi(relayFactor) if err != nil { - resp.WriteHeader(http.StatusBadRequest) - fmt.Fprintf(resp, "Error parsing relay factor: %v", err) - return nil, nil + return nil, BadRequestError{Reason: fmt.Sprintf("Error parsing relay factor: %v", err)} } args.RelayFactor, err = ParseRelayFactor(n) if err != nil { - resp.WriteHeader(http.StatusBadRequest) - fmt.Fprintf(resp, "Invalid relay factor: %v", err) - return nil, nil + return nil, BadRequestError{Reason: fmt.Sprintf("Invalid relay-factor: %v", err)} + } + } + + // Parse local-only. local-only can only be used in GET requests. + if localOnly := req.URL.Query().Get("local-only"); localOnly != "" { + var err error + args.LocalOnly, err = strconv.ParseBool(localOnly) + if err != nil { + return nil, BadRequestError{Reason: fmt.Sprintf("Error parsing local-only: %v", err)} + } + + err = ValidateLocalOnly(args.LocalOnly, req.Method == "GET") + if err != nil { + return nil, BadRequestError{Reason: fmt.Sprintf("Invalid use of local-only: %v", err)} } } @@ -214,9 +219,7 @@ func (s *HTTPServer) OperatorAutopilotConfiguration(resp http.ResponseWriter, re var conf api.AutopilotConfiguration durations := NewDurationFixer("lastcontactthreshold", "serverstabilizationtime") if err := decodeBody(req, &conf, durations.FixupDurations); err != nil { - resp.WriteHeader(http.StatusBadRequest) - fmt.Fprintf(resp, "Error parsing autopilot config: %v", err) - return nil, nil + return nil, BadRequestError{Reason: fmt.Sprintf("Error parsing autopilot config: %v", err)} } args.Config = autopilot.Config{ @@ -234,9 +237,7 @@ func (s *HTTPServer) OperatorAutopilotConfiguration(resp http.ResponseWriter, re if _, ok := params["cas"]; ok { casVal, err := strconv.ParseUint(params.Get("cas"), 10, 64) if err != nil { - resp.WriteHeader(http.StatusBadRequest) - fmt.Fprintf(resp, "Error parsing cas value: %v", err) - return nil, nil + return nil, BadRequestError{Reason: fmt.Sprintf("Error parsing cas value: %v", err)} } args.Config.ModifyIndex = casVal args.CAS = true diff --git a/agent/operator_endpoint_test.go b/agent/operator_endpoint_test.go index bf6c838f90..4ffab2bcd1 100644 --- a/agent/operator_endpoint_test.go +++ b/agent/operator_endpoint_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/hashicorp/consul/testrpc" + "github.com/stretchr/testify/require" "github.com/hashicorp/consul/agent/consul/autopilot" "github.com/hashicorp/consul/agent/structs" @@ -276,15 +277,46 @@ func TestOperator_Keyring_InvalidRelayFactor(t *testing.T) { "asdf": "Error parsing relay factor", } for relayFactor, errString := range cases { - req, _ := http.NewRequest("GET", "/v1/operator/keyring?relay-factor="+relayFactor, nil) + req, err := http.NewRequest("GET", "/v1/operator/keyring?relay-factor="+relayFactor, nil) + require.NoError(t, err) resp := httptest.NewRecorder() - _, err := a.srv.OperatorKeyringEndpoint(resp, req) - if err != nil { - t.Fatalf("err: %v", err) + _, err = a.srv.OperatorKeyringEndpoint(resp, req) + require.Error(t, err, "tc: "+relayFactor) + require.Contains(t, err.Error(), errString, "tc: "+relayFactor) + } +} + +func TestOperator_Keyring_LocalOnly(t *testing.T) { + t.Parallel() + key := "H3/9gBxcKKRf45CaI2DlRg==" + a := NewTestAgent(t, t.Name(), ` + encrypt = "`+key+`" + `) + defer a.Shutdown() + + cases := []struct { + description string + method string + local interface{} + ok bool + }{ + {"all ok", "GET", true, true}, + {"garbage local-only value", "GET", "garbage", false}, + {"wrong method (DELETE)", "DELETE", true, false}, + } + + for _, tc := range cases { + url := fmt.Sprintf("/v1/operator/keyring?local-only=%v", tc.local) + req, err := http.NewRequest(tc.method, url, nil) + require.NoError(t, err, "tc: "+tc.description) + + resp := httptest.NewRecorder() + _, err = a.srv.OperatorKeyringEndpoint(resp, req) + if tc.ok { + require.NoError(t, err, "tc: "+tc.description) } - body := resp.Body.String() - if !strings.Contains(body, errString) { - t.Fatalf("bad: %v", body) + if !tc.ok { + require.Error(t, err, "tc: "+tc.description) } } } diff --git a/agent/structs/structs.go b/agent/structs/structs.go index 8fc6c3281a..80e2a63c88 100644 --- a/agent/structs/structs.go +++ b/agent/structs/structs.go @@ -1579,6 +1579,7 @@ type KeyringRequest struct { Datacenter string Forwarded bool RelayFactor uint8 + LocalOnly bool QueryOptions } diff --git a/api/api.go b/api/api.go index 11c3a2cd7c..b624b3cce3 100644 --- a/api/api.go +++ b/api/api.go @@ -143,6 +143,10 @@ type QueryOptions struct { // a value from 0 to 5 (inclusive). RelayFactor uint8 + // LocalOnly is used in keyring list operation to force the keyring + // query to only hit local servers (no WAN traffic). + LocalOnly bool + // Connect filters prepared query execution to only include Connect-capable // services. This currently affects prepared query execution. Connect bool @@ -655,6 +659,9 @@ func (r *request) setQueryOptions(q *QueryOptions) { if q.RelayFactor != 0 { r.params.Set("relay-factor", strconv.Itoa(int(q.RelayFactor))) } + if q.LocalOnly { + r.params.Set("local-only", fmt.Sprintf("%t", q.LocalOnly)) + } if q.Connect { r.params.Set("connect", "true") } diff --git a/api/api_test.go b/api/api_test.go index 9b6c935f27..8de3a0805f 100644 --- a/api/api_test.go +++ b/api/api_test.go @@ -702,6 +702,7 @@ func TestAPI_SetQueryOptions(t *testing.T) { WaitTime: 100 * time.Second, Token: "12345", Near: "nodex", + LocalOnly: true, } r.setQueryOptions(q) @@ -726,6 +727,9 @@ func TestAPI_SetQueryOptions(t *testing.T) { if r.params.Get("near") != "nodex" { t.Fatalf("bad: %v", r.params) } + if r.params.Get("local-only") != "true" { + t.Fatalf("bad: %v", r.params) + } assert.Equal("", r.header.Get("Cache-Control")) r = c.newRequest("GET", "/v1/kv/foo") diff --git a/command/keyring/keyring.go b/command/keyring/keyring.go index b9a12732a7..6ef618f2a8 100644 --- a/command/keyring/keyring.go +++ b/command/keyring/keyring.go @@ -28,6 +28,7 @@ type cmd struct { removeKey string listKeys bool relay int + local bool } func (c *cmd) init() { @@ -48,6 +49,9 @@ func (c *cmd) init() { "Setting this to a non-zero value will cause nodes to relay their response "+ "to the operation through this many randomly-chosen other nodes in the "+ "cluster. The maximum allowed value is 5.") + c.flags.BoolVar(&c.local, "local-only", false, + "Setting this to true will force the keyring query to only hit local servers "+ + "(no WAN traffic). This flag can only be set for list queries.") c.http = &flags.HTTPFlags{} flags.Merge(c.flags, c.http.ClientFlags()) @@ -89,6 +93,13 @@ func (c *cmd) Run(args []string) int { return 1 } + // Validate local-only + err = agent.ValidateLocalOnly(c.local, c.listKeys) + if err != nil { + c.UI.Error(fmt.Sprintf("Error validating local-only: %s", err)) + return 1 + } + // All other operations will require a client connection client, err := c.http.APIClient() if err != nil { @@ -98,7 +109,7 @@ func (c *cmd) Run(args []string) int { if c.listKeys { c.UI.Info("Gathering installed encryption keys...") - responses, err := client.Operator().KeyringList(&consulapi.QueryOptions{RelayFactor: relayFactor}) + responses, err := client.Operator().KeyringList(&consulapi.QueryOptions{RelayFactor: relayFactor, LocalOnly: c.local}) if err != nil { c.UI.Error(fmt.Sprintf("error: %s", err)) return 1 diff --git a/command/keyring/keyring_test.go b/command/keyring/keyring_test.go index 7f4fd0a96a..051b852c9e 100644 --- a/command/keyring/keyring_test.go +++ b/command/keyring/keyring_test.go @@ -95,6 +95,18 @@ func TestKeyringCommand_failedConnection(t *testing.T) { } } +func TestKeyringCommand_invalidLocalOnly(t *testing.T) { + t.Parallel() + ui := cli.NewMockUi() + c := New(ui) + + args := []string{"-install=blah", "-local-only=true"} + code := c.Run(args) + if code != 1 { + t.Fatalf("bad: %d. %#v", code, ui.ErrorWriter.String()) + } +} + func TestKeyringCommand_invalidRelayFactor(t *testing.T) { t.Parallel() ui := cli.NewMockUi() diff --git a/website/source/api/operator/keyring.html.md b/website/source/api/operator/keyring.html.md index 26401744b0..2f2067cee7 100644 --- a/website/source/api/operator/keyring.html.md +++ b/website/source/api/operator/keyring.html.md @@ -16,7 +16,8 @@ more details on the gossip protocol and its use. ## List Gossip Encryption Keys This endpoint lists the gossip encryption keys installed on both the WAN and LAN -rings of every known datacenter. +rings of every known datacenter, unless otherwise specified with the `local-only` +query parameter (see below). If ACLs are enabled, the client will need to supply an ACL Token with `keyring` read privileges. @@ -41,6 +42,9 @@ The table below shows this endpoint's support for non-zero value will cause nodes to relay their responses through this many randomly-chosen other nodes in the cluster. The maximum allowed value is `5`. This is specified as part of the URL as a query parameter. +- `local-only` `(bool: false)` - Setting `local-only` to true will force keyring + list queries to only hit local servers (no WAN traffic). This flag can only be set + for list queries. It is specified as part of the URL as a query parameter. ### Sample Request diff --git a/website/source/docs/commands/keyring.html.markdown.erb b/website/source/docs/commands/keyring.html.markdown.erb index 6aa3ec2dbc..80bdde3e00 100644 --- a/website/source/docs/commands/keyring.html.markdown.erb +++ b/website/source/docs/commands/keyring.html.markdown.erb @@ -20,7 +20,9 @@ are installed on the cluster. You can review the installed keys using the `-list` argument, and remove unneeded keys with `-remove`. All operations performed by this command can only be run against server nodes, -and affect both the LAN and WAN keyrings in lock-step. +and affect both the LAN and WAN keyrings in lock-step. The only exception to this +is for the `-list` operation, you may set the `-local-only` flag to only query +against local server nodes. All variations of the `keyring` command return 0 if all nodes reply and there are no errors. If any node fails to reply or reports failure, the exit code @@ -54,6 +56,10 @@ Only one actionable argument may be specified per run, including `-list`, cause nodes to relay their response to the operation through this many randomly-chosen other nodes in the cluster. The maximum allowed value is 5. +* `-local-only` - Setting this to true will force a keyring list query to only hit + local servers (no WAN traffic). Setting `local-only` is only allowed for list + queries. + ## Output The output of the `consul keyring -list` command consolidates information from