cli: avoid passing envoy bootstrap configuration as arguments (#4747)

Play a trick with CLOEXEC to pass the envoy bootstrap configuration as
an open file descriptor to the exec'd envoy process. The file only
briefly touches disk before being unlinked.

We convince envoy to read from this open file descriptor by using the
/dev/fd/$FDNUMBER mechanism to read the open file descriptor as a file.

Because the filename no longer has an extension envoy's sniffing logic
falls back on JSON instead of YAML, so the bootstrap configuration must
be generated as JSON instead.

api/agent.go

@@ -191,6 +191,8 @@ type AgentServiceCheck struct {
 	TLSSkipVerify     bool                `json:",omitempty"`
 	GRPC              string              `json:",omitempty"`
 	GRPCUseTLS        bool                `json:",omitempty"`
+	AliasNode         string              `json:",omitempty"`
+	AliasService      string              `json:",omitempty"`
 
 	// In Consul 0.7 and later, checks that are associated with a service
 	// may also contain this optional DeregisterCriticalServiceAfter field,

command/connect/envoy/bootstrap_tpl.go

@@ -13,43 +13,66 @@ type templateArgs struct {
 }
 
 const bootstrapTemplate = `
-# Bootstrap Config for Consul Connect
-# Generated by consul connect envoy
-admin:
-  access_log_path: /dev/null
-  address:
-    socket_address:
-      address: "{{ .AdminBindAddress }}"
-      port_value: {{ .AdminBindPort }}
-node:
-  cluster: "{{ .ProxyCluster }}"
-  id: "{{ .ProxyID }}"
-static_resources:
-  clusters:
-  - name: "{{ .LocalAgentClusterName }}"
-    connect_timeout: 1s
-    type: STATIC
-    {{ if .AgentTLS -}}
-    tls_context:
-      common_tls_context:
-        validation_context:
-          trusted_ca:
-            filename: {{ .AgentCAFile }}
-    {{- end }}
-    http2_protocol_options: {}
-    hosts:
-    - socket_address:
-        address: "{{ .AgentHTTPAddress }}"
-        port_value: {{ .AgentHTTPPort }}
-dynamic_resources:
-  lds_config: {ads: {}}
-  cds_config: {ads: {}}
-  ads_config:
-    api_type: GRPC
-    grpc_services:
-      initial_metadata:
-        - key: x-consul-token
-          value: "{{ .Token }}"
-      envoy_grpc:
-        cluster_name: "{{ .LocalAgentClusterName }}"
+{
+  "admin": {
+    "access_log_path": "/dev/null",
+    "address": {
+      "socket_address": {
+        "address": "{{ .AdminBindAddress }}",
+        "port_value": {{ .AdminBindPort }}
+      }
+    }
+  },
+  "node": {
+    "cluster": "{{ .ProxyCluster }}",
+    "id": "{{ .ProxyID }}"
+  },
+  "static_resources": {
+    "clusters": [
+      {
+        "name": "{{ .LocalAgentClusterName }}",
+        "connect_timeout": "1s",
+        "type": "STATIC",
+        {{ if .AgentTLS -}}
+        "tls_context": {
+          "common_tls_context": {
+            "validation_context": {
+              "trusted_ca": {
+                "filename": "{{ .AgentCAFile }}"
+              }
+            }
+          }
+        },
+        {{- end }}
+        "http2_protocol_options": {},
+        "hosts": [
+          {
+            "socket_address": {
+              "address": "{{ .AgentHTTPAddress }}",
+              "port_value": {{ .AgentHTTPPort }}
+            }
+          }
+        ]
+      }
+    ]
+  },
+  "dynamic_resources": {
+    "lds_config": { "ads": {} },
+    "cds_config": { "ads": {} },
+    "ads_config": {
+      "api_type": "GRPC",
+      "grpc_services": {
+        "initial_metadata": [
+          {
+            "key": "x-consul-token",
+            "value": "{{ .Token }}"
+          }
+        ],
+        "envoy_grpc": {
+          "cluster_name": "{{ .LocalAgentClusterName }}"
+        }
+      }
+    }
+  }
+}
 `

command/connect/envoy/envoy.go

@@ -2,6 +2,7 @@ package envoy
 
 import (
 	"bytes"
+	"errors"
 	"flag"
 	"fmt"
 	"html/template"
@@ -10,7 +11,6 @@ import (
 	"os/exec"
 	"strconv"
 	"strings"
-	"syscall"
 
 	proxyAgent "github.com/hashicorp/consul/agent/proxyprocess"
 	"github.com/hashicorp/consul/agent/xds"
@@ -72,7 +72,7 @@ func (c *cmd) init() {
 			"as it has full control over the secrets and config of the proxy.")
 
 	c.flags.BoolVar(&c.bootstrap, "bootstrap", false,
-		"Generate the bootstrap.yaml but don't exec envoy")
+		"Generate the bootstrap.json but don't exec envoy")
 
 	c.flags.StringVar(&c.grpcAddr, "grpc-addr", "",
 		"Set the agent's gRPC address and port (in http(s)://host:port format). "+
@@ -135,7 +135,7 @@ func (c *cmd) Run(args []string) int {
 	}
 
 	// Generate config
-	bootstrapYaml, err := c.generateConfig()
+	bootstrapJson, err := c.generateConfig()
 	if err != nil {
 		c.UI.Error(err.Error())
 		return 1
@@ -143,7 +143,7 @@ func (c *cmd) Run(args []string) int {
 
 	if c.bootstrap {
 		// Just output it and we are done
-		fmt.Println(bootstrapYaml)
+		os.Stdout.Write(bootstrapJson)
 		return 0
 	}
 
@@ -154,26 +154,23 @@ func (c *cmd) Run(args []string) int {
 		return 1
 	}
 
-	// First argument needs to be the executable name.
-
-	// TODO(banks): passing config including an ACL token on command line is jank
-	// - this is world readable. It's easiest thing for now. Temp files are kinda
-	// gross in a different way - we can limit to same-user access which is much
-	// better but we are leaving the ACL secret on disk unencrypted for an
-	// uncontrolled amount of time and in a location the operator doesn't even
-	// know about. Envoy doesn't support reading bootstrap from stdin or ENV
-	envoyArgs := []string{binary, "--config-yaml", bootstrapYaml}
-	envoyArgs = append(envoyArgs, passThroughArgs...)
-
-	// Exec
-	err = syscall.Exec(binary, envoyArgs, os.Environ())
-	if err != nil {
-		c.UI.Error("Failed to exec envoy: " + err.Error())
+	err = execEnvoy(binary, nil, passThroughArgs, bootstrapJson)
+	if err == errUnsupportedOS {
+		c.UI.Error("Directly running Envoy is only supported on linux and macOS " +
+			"since envoy itself doesn't build on other platforms currently.")
+		c.UI.Error("Use the -bootstrap option to generate the JSON to use when running envoy " +
+			"on a supported OS or via a container or VM.")
+		return 1
+	} else if err != nil {
+		c.UI.Error(err.Error())
 		return 1
 	}
+
 	return 0
 }
 
+var errUnsupportedOS = errors.New("envoy: not implemented on this operating system")
+
 func (c *cmd) findBinary() (string, error) {
 	if c.envoyBin != "" {
 		return c.envoyBin, nil
@@ -183,7 +180,7 @@ func (c *cmd) findBinary() (string, error) {
 
 // TODO(banks) this method ended up with a few subtleties that should be unit
 // tested.
-func (c *cmd) generateConfig() (string, error) {
+func (c *cmd) generateConfig() ([]byte, error) {
 	var t = template.Must(template.New("bootstrap").Parse(bootstrapTemplate))
 
 	httpCfg := api.DefaultConfig()
@@ -212,7 +209,7 @@ func (c *cmd) generateConfig() (string, error) {
 
 	agentAddr, agentPort, err := net.SplitHostPort(addrPort)
 	if err != nil {
-		return "", fmt.Errorf("Invalid Consul HTTP address: %s", err)
+		return nil, fmt.Errorf("Invalid Consul HTTP address: %s", err)
 	}
 	if agentAddr == "" {
 		agentAddr = "127.0.0.1"
@@ -226,19 +223,19 @@ func (c *cmd) generateConfig() (string, error) {
 	// can't connect.
 	agentIP, err := net.ResolveIPAddr("ip", agentAddr)
 	if err != nil {
-		return "", fmt.Errorf("Failed to resolve agent address: %s", err)
+		return nil, fmt.Errorf("Failed to resolve agent address: %s", err)
 	}
 
 	adminAddr, adminPort, err := net.SplitHostPort(c.adminBind)
 	if err != nil {
-		return "", fmt.Errorf("Invalid Consul HTTP address: %s", err)
+		return nil, fmt.Errorf("Invalid Consul HTTP address: %s", err)
 	}
 
 	// Envoy requires IP addresses to bind too when using static so resolve DNS or
 	// localhost here.
 	adminBindIP, err := net.ResolveIPAddr("ip", adminAddr)
 	if err != nil {
-		return "", fmt.Errorf("Failed to resolve admin bind address: %s", err)
+		return nil, fmt.Errorf("Failed to resolve admin bind address: %s", err)
 	}
 
 	args := templateArgs{
@@ -257,9 +254,9 @@ func (c *cmd) generateConfig() (string, error) {
 	var buf bytes.Buffer
 	err = t.Execute(&buf, args)
 	if err != nil {
-		return "", err
+		return nil, err
 	}
-	return buf.String(), nil
+	return buf.Bytes(), nil
 }
 
 func (c *cmd) lookupProxyIDForSidecar() (string, error) {
@@ -285,7 +282,7 @@ Usage: consul connect envoy [options]
   It will search $PATH for the envoy binary but this can be overridden with
   -envoy-binary.
 
-  It can instead only generate the bootstrap.yaml based on the current ENV and
+  It can instead only generate the bootstrap.json based on the current ENV and
   arguments using -bootstrap.
 
   The proxy requires service:write permissions for the service it represents.

command/connect/envoy/exec_test.go

@@ -0,0 +1,173 @@
+// +build linux darwin
+
+package envoy
+
+import (
+	"encoding/json"
+	"fmt"
+	"io/ioutil"
+	"os"
+	"os/exec"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestExecEnvoy(t *testing.T) {
+	require := require.New(t)
+
+	cmd, destroy := helperProcess("exec-fake-envoy")
+	defer destroy()
+
+	cmd.Stderr = os.Stderr
+	outBytes, err := cmd.Output()
+	if err != nil {
+		t.Fatalf("error launching child process: %v", err)
+	}
+
+	var got FakeEnvoyExecData
+	require.NoError(json.Unmarshal(outBytes, &got))
+
+	expectArgs := []string{
+		"--v2-config-only",
+		"--disable-hot-restart",
+		"--config-path",
+		"/dev/fd/3",
+		"--fake-envoy-arg",
+	}
+	expectConfigPath := "/dev/fd/3"
+	expectConfigData := fakeEnvoyTestData
+
+	require.Equal(expectArgs, got.Args)
+	require.Equal(expectConfigPath, got.ConfigPath)
+	require.Equal(expectConfigData, got.ConfigData)
+}
+
+type FakeEnvoyExecData struct {
+	Args       []string `json:"args"`
+	ConfigPath string   `json:"configPath"`
+	ConfigData string   `json:"configData"`
+}
+
+// helperProcessSentinel is a sentinel value that is put as the first
+// argument following "--" and is used to determine if TestHelperProcess
+// should run.
+const helperProcessSentinel = "GO_WANT_HELPER_PROCESS"
+
+// helperProcess returns an *exec.Cmd that can be used to execute the
+// TestHelperProcess function below. This can be used to test multi-process
+// interactions.
+func helperProcess(s ...string) (*exec.Cmd, func()) {
+	cs := []string{"-test.run=TestHelperProcess", "--", helperProcessSentinel}
+	cs = append(cs, s...)
+
+	cmd := exec.Command(os.Args[0], cs...)
+	destroy := func() {
+		if p := cmd.Process; p != nil {
+			p.Kill()
+		}
+	}
+
+	return cmd, destroy
+}
+
+const fakeEnvoyTestData = "pahx9eiPoogheb4haeb2abeem1QuireWahtah1Udi5ae4fuD0c"
+
+// This is not a real test. This is just a helper process kicked off by tests
+// using the helperProcess helper function.
+func TestHelperProcess(t *testing.T) {
+	args := os.Args
+	for len(args) > 0 {
+		if args[0] == "--" {
+			args = args[1:]
+			break
+		}
+
+		args = args[1:]
+	}
+
+	if len(args) == 0 || args[0] != helperProcessSentinel {
+		return
+	}
+
+	defer os.Exit(0)
+	args = args[1:] // strip sentinel value
+	cmd, args := args[0], args[1:]
+
+	switch cmd {
+	case "exec-fake-envoy":
+		// this will just exec the "fake-envoy" flavor below
+
+		limitProcessLifetime(2 * time.Minute)
+
+		err := execEnvoy(
+			os.Args[0],
+			[]string{
+				"-test.run=TestHelperProcess",
+				"--",
+				helperProcessSentinel,
+				"fake-envoy",
+			},
+			[]string{"--fake-envoy-arg"},
+			[]byte(fakeEnvoyTestData),
+		)
+		if err != nil {
+			fmt.Fprintf(os.Stderr, "fake envoy process failed to exec: %v\n", err)
+			os.Exit(1)
+		}
+
+	case "fake-envoy":
+		// This subcommand is instrumented to verify some settings
+		// survived an exec.
+
+		limitProcessLifetime(2 * time.Minute)
+
+		data := FakeEnvoyExecData{
+			Args: args,
+		}
+
+		// Dump all of the args.
+		var captureNext bool
+		for _, arg := range args {
+			if arg == "--config-path" {
+				captureNext = true
+			} else if captureNext {
+				data.ConfigPath = arg
+				captureNext = false
+			}
+		}
+
+		if data.ConfigPath == "" {
+			fmt.Fprintf(os.Stderr, "did not detect a --config-path argument passed through\n")
+			os.Exit(1)
+		}
+
+		d, err := ioutil.ReadFile(data.ConfigPath)
+		if err != nil {
+			fmt.Fprintf(os.Stderr, "could not read provided --config-path file %q: %v\n", data.ConfigPath, err)
+			os.Exit(1)
+		}
+		data.ConfigData = string(d)
+
+		enc := json.NewEncoder(os.Stdout)
+		if err := enc.Encode(&data); err != nil {
+			fmt.Fprintf(os.Stderr, "could not dump results to stdout: %v", err)
+			os.Exit(1)
+
+		}
+
+	default:
+		fmt.Fprintf(os.Stderr, "Unknown command: %q\n", cmd)
+		os.Exit(2)
+	}
+}
+
+// limitProcessLifetime installs a background goroutine that self-exits after
+// the specified duration elapses to prevent leaking processes from tests that
+// may spawn them.
+func limitProcessLifetime(dur time.Duration) {
+	go time.AfterFunc(dur, func() {
+		os.Exit(99)
+	})
+}

command/connect/envoy/exec_unix.go

@@ -0,0 +1,129 @@
+// +build linux darwin
+
+package envoy
+
+import (
+	"errors"
+	"io"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"strconv"
+
+	"golang.org/x/sys/unix"
+)
+
+func execEnvoy(binary string, prefixArgs, suffixArgs []string, bootstrapJson []byte) error {
+	// Write the Envoy bootstrap config file out to disk in a pocket universe
+	// visible only to the current process (and exec'd future selves).
+	fd, err := writeEphemeralEnvoyTempFile(bootstrapJson)
+	if err != nil {
+		return errors.New("Could not write envoy bootstrap config to a temp file: " + err.Error())
+	}
+
+	// On unix systems after exec the file descriptors that we should see:
+	//
+	//   0: stdin
+	//   1: stdout
+	//   2: stderr
+	//   ... any open file descriptors from the parent without CLOEXEC set
+	//
+	// Above we explicitly disabled CLOEXEC for our temp file, so assuming
+	// FD numbers survive across execs, it should just be the value of
+	// `fd`. This is accessible as a file itself (trippy!) under
+	// /dev/fd/$FDNUMBER.
+	magicPath := filepath.Join("/dev/fd", strconv.Itoa(int(fd)))
+
+	// First argument needs to be the executable name.
+	envoyArgs := []string{binary}
+	envoyArgs = append(envoyArgs, prefixArgs...)
+	envoyArgs = append(envoyArgs, "--v2-config-only",
+		"--disable-hot-restart",
+		"--config-path",
+		magicPath,
+	)
+	envoyArgs = append(envoyArgs, suffixArgs...)
+
+	// Exec
+	if err = unix.Exec(binary, envoyArgs, os.Environ()); err != nil {
+		return errors.New("Failed to exec envoy: " + err.Error())
+	}
+
+	return nil
+}
+
+func writeEphemeralEnvoyTempFile(b []byte) (uintptr, error) {
+	f, err := ioutil.TempFile("", "envoy-ephemeral-config")
+	if err != nil {
+		return 0, err
+	}
+
+	errFn := func(err error) (uintptr, error) {
+		_ = f.Close()
+		return 0, err
+	}
+
+	// TempFile already does this, but it's cheap to reinforce that we
+	// WANT the default behavior.
+	if err := f.Chmod(0600); err != nil {
+		return errFn(err)
+	}
+
+	// Immediately unlink the file as we are going to just pass the
+	// file descriptor, not the path.
+	if err = os.Remove(f.Name()); err != nil {
+		return errFn(err)
+	}
+	if _, err = f.Write(b); err != nil {
+		return errFn(err)
+	}
+	// Rewind the file descriptor so Envoy can read it.
+	if _, err = f.Seek(0, io.SeekStart); err != nil {
+		return errFn(err)
+	}
+
+	// Disable CLOEXEC so that this file descriptor is available
+	// to the exec'd Envoy.
+	if err := setCloseOnExec(f.Fd(), false); err != nil {
+		return errFn(err)
+	}
+
+	return f.Fd(), nil
+}
+
+// isCloseOnExec checks the provided file descriptor to see if the CLOEXEC flag
+// is set.
+func isCloseOnExec(fd uintptr) (bool, error) {
+	flags, err := getFdFlags(fd)
+	if err != nil {
+		return false, err
+	}
+	return flags&unix.FD_CLOEXEC != 0, nil
+}
+
+// setCloseOnExec sets or unsets the CLOEXEC flag on the provided file descriptor
+// depending upon the value of the enabled arg.
+func setCloseOnExec(fd uintptr, enabled bool) error {
+	flags, err := getFdFlags(fd)
+	if err != nil {
+		return err
+	}
+
+	newFlags := flags
+	if enabled {
+		newFlags |= unix.FD_CLOEXEC
+	} else {
+		newFlags &= ^unix.FD_CLOEXEC
+	}
+
+	if newFlags == flags {
+		return nil // noop
+	}
+
+	_, err = unix.FcntlInt(fd, unix.F_SETFD, newFlags)
+	return err
+}
+
+func getFdFlags(fd uintptr) (int, error) {
+	return unix.FcntlInt(fd, unix.F_GETFD, 0)
+}

command/connect/envoy/exec_unsupported.go

@@ -0,0 +1,7 @@
+// +build !linux,!darwin
+
+package envoy
+
+func execEnvoy(binary string, prefixArgs, suffixArgs []string, bootstrapJson []byte) error {
+	return errUnsupportedOS
+}