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

@@ -178,11 +178,20 @@ func (m *Internal) KeyringOperation(
 		}
 	}
 
-	// Only perform WAN keyring querying and RPC forwarding once
+	// Validate use of local-only
-	if !args.Forwarded && m.srv.serfWAN != nil {
+	if args.LocalOnly && args.Operation != structs.KeyringList {
-		args.Forwarded = true
+		// Error aggressively to be clear about LocalOnly behavior
-		m.executeKeyringOp(args, reply, true)
+		return fmt.Errorf("argument error: LocalOnly can only be used for List operations")
-		return m.srv.globalRPC("Internal.KeyringOperation", args, reply)
+	}
+
+	// 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

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)

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) {

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))
+}

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)
+		return nil, BadRequestError{Reason: "Must specify either ?id with the server's ID or ?address with IP:port of peer to remove"}
-		fmt.Fprint(resp, "Must specify either ?id with the server's ID or ?address with IP:port of peer to remove")
-		return nil, nil
 	}
 	if hasID && hasAddress {
-		resp.WriteHeader(http.StatusBadRequest)
+		return nil, BadRequestError{Reason: "Must specify only one of ?id or ?address"}
-		fmt.Fprint(resp, "Must specify only one of ?id or ?address")
-		return nil, nil
 	}
 
 	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)
+			return nil, BadRequestError{Reason: fmt.Sprintf("Request decode failed: %v", err)}
-			fmt.Fprintf(resp, "Request decode failed: %v", err)
-			return nil, nil
 		}
 	}
 	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)
+			return nil, BadRequestError{Reason: fmt.Sprintf("Error parsing relay factor: %v", err)}
-			fmt.Fprintf(resp, "Error parsing relay factor: %v", err)
-			return nil, nil
 		}
 
 		args.RelayFactor, err = ParseRelayFactor(n)
 		if err != nil {
-			resp.WriteHeader(http.StatusBadRequest)
+			return nil, BadRequestError{Reason: fmt.Sprintf("Invalid relay-factor: %v", err)}
-			fmt.Fprintf(resp, "Invalid relay factor: %v", err)
+		}
-			return nil, nil
+	}
+
+	// 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)
+			return nil, BadRequestError{Reason: fmt.Sprintf("Error parsing autopilot config: %v", err)}
-			fmt.Fprintf(resp, "Error parsing autopilot config: %v", err)
-			return nil, nil
 		}
 
 		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)
+				return nil, BadRequestError{Reason: fmt.Sprintf("Error parsing cas value: %v", err)}
-				fmt.Fprintf(resp, "Error parsing cas value: %v", err)
-				return nil, nil
 			}
 			args.Config.ModifyIndex = casVal
 			args.CAS = true

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)
+		_, err = a.srv.OperatorKeyringEndpoint(resp, req)
-		if err != nil {
+		require.Error(t, err, "tc: "+relayFactor)
-			t.Fatalf("err: %v", err)
+		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 !tc.ok {
-		if !strings.Contains(body, errString) {
+			require.Error(t, err, "tc: "+tc.description)
-			t.Fatalf("bad: %v", body)
 		}
 	}
 }

agent/structs/structs.go

@@ -1579,6 +1579,7 @@ type KeyringRequest struct {
 	Datacenter  string
 	Forwarded   bool
 	RelayFactor uint8
+	LocalOnly   bool
 	QueryOptions
 }
 

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")
 	}

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")

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

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()

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
 

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