Make plugin-specific env take precedence over sys env (#25128)

* Make plugin-specific env take precedence over sys env
* Expand the existing plugin env integration test

---------

Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com>

changelog/25128.txt

@@ -0,0 +1,6 @@
+```release-note:change
+plugins: By default, environment variables provided during plugin registration will now take precedence over system environment variables.
+Use the environment variable `VAULT_PLUGIN_USE_LEGACY_ENV_LAYERING=true` to opt out and keep higher preference for system environment
+variables. When this flag is set, Vault will check during unseal for conflicts and print warnings for any plugins with environment
+variables that conflict with system environment variables.
+```

sdk/helper/pluginutil/env.go

@@ -38,6 +38,12 @@ const (
 	// PluginMultiplexingOptOut is an ENV name used to define a comma separated list of plugin names
 	// opted-out of the multiplexing feature; for emergencies if multiplexing ever causes issues
 	PluginMultiplexingOptOut = "VAULT_PLUGIN_MULTIPLEXING_OPT_OUT"
+
+	// PluginUseLegacyEnvLayering opts out of new environment variable precedence.
+	// If set to true, Vault process environment variables take precedence over any
+	// colliding plugin-specific environment variables. Otherwise, plugin-specific
+	// environment variables take precedence over Vault process environment variables.
+	PluginUseLegacyEnvLayering = "VAULT_PLUGIN_USE_LEGACY_ENV_LAYERING"
 )
 
 // OptionallyEnableMlock determines if mlock should be called, and if so enables

sdk/helper/pluginutil/run_config.go

@@ -65,26 +65,26 @@ func (rc runConfig) mlockEnabled() bool {
 
 func (rc runConfig) generateCmd(ctx context.Context) (cmd *exec.Cmd, clientTLSConfig *tls.Config, err error) {
 	cmd = exec.Command(rc.command, rc.args...)
-	cmd.Env = append(cmd.Env, rc.env...)
+	env := rc.env
 
 	// Add the mlock setting to the ENV of the plugin
 	if rc.mlockEnabled() {
-		cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s", PluginMlockEnabled, "true"))
+		env = append(env, fmt.Sprintf("%s=%s", PluginMlockEnabled, "true"))
 	}
 	version, err := rc.Wrapper.VaultVersion(ctx)
 	if err != nil {
 		return nil, nil, err
 	}
-	cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s", PluginVaultVersionEnv, version))
+	env = append(env, fmt.Sprintf("%s=%s", PluginVaultVersionEnv, version))
 
 	if rc.IsMetadataMode {
 		rc.Logger = rc.Logger.With("metadata", "true")
 	}
 	metadataEnv := fmt.Sprintf("%s=%t", PluginMetadataModeEnv, rc.IsMetadataMode)
-	cmd.Env = append(cmd.Env, metadataEnv)
+	env = append(env, metadataEnv)
 
 	automtlsEnv := fmt.Sprintf("%s=%t", PluginAutoMTLSEnv, rc.AutoMTLS)
-	cmd.Env = append(cmd.Env, automtlsEnv)
+	env = append(env, automtlsEnv)
 
 	if !rc.AutoMTLS && !rc.IsMetadataMode {
 		// Get a CA TLS Certificate
@@ -107,7 +107,35 @@ func (rc runConfig) generateCmd(ctx context.Context) (cmd *exec.Cmd, clientTLSCo
 		}
 
 		// Add the response wrap token to the ENV of the plugin
-		cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s", PluginUnwrapTokenEnv, wrapToken))
+		env = append(env, fmt.Sprintf("%s=%s", PluginUnwrapTokenEnv, wrapToken))
+	}
+
+	if rc.image == "" {
+		// go-plugin has always overridden user-provided env vars with the OS
+		// (Vault process) env vars, but we want plugins to be able to override
+		// the Vault process env. We don't want to make a breaking change in
+		// go-plugin so always set SkipHostEnv and replicate the legacy behavior
+		// ourselves if user opts in.
+		if legacy, _ := strconv.ParseBool(os.Getenv(PluginUseLegacyEnvLayering)); legacy {
+			// Env vars are layered as follows, with later entries overriding
+			// earlier entries if there are duplicate keys:
+			// 1. Env specified at plugin registration
+			// 2. Env from Vault SDK
+			// 3. Env from Vault process (OS)
+			// 4. Env from go-plugin
+			cmd.Env = append(env, os.Environ()...)
+		} else {
+			// Env vars are layered as follows, with later entries overriding
+			// earlier entries if there are duplicate keys:
+			// 1. Env from Vault process (OS)
+			// 2. Env specified at plugin registration
+			// 3. Env from Vault SDK
+			// 4. Env from go-plugin
+			cmd.Env = append(os.Environ(), env...)
+		}
+	} else {
+		// Containerized plugins do not inherit any env vars from Vault.
+		cmd.Env = env
 	}
 
 	return cmd, clientTLSConfig, nil
@@ -128,7 +156,8 @@ func (rc runConfig) makeConfig(ctx context.Context) (*plugin.ClientConfig, error
 			plugin.ProtocolNetRPC,
 			plugin.ProtocolGRPC,
 		},
-		AutoMTLS: rc.AutoMTLS,
+		AutoMTLS:    rc.AutoMTLS,
+		SkipHostEnv: true,
 	}
 	if rc.image == "" {
 		clientConfig.Cmd = cmd
@@ -141,7 +170,6 @@ func (rc runConfig) makeConfig(ctx context.Context) (*plugin.ClientConfig, error
 		if err != nil {
 			return nil, err
 		}
-		clientConfig.SkipHostEnv = true
 		clientConfig.RunnerFunc = containerCfg.NewContainerRunner
 		clientConfig.UnixSocketConfig = &plugin.UnixSocketConfig{
 			Group:   strconv.Itoa(containerCfg.GroupAdd),

sdk/helper/pluginutil/run_config_test.go

@@ -34,10 +34,11 @@ func TestMakeConfig(t *testing.T) {
 		mlockEnabled      bool
 		mlockEnabledTimes int
 
-		expectedConfig   *plugin.ClientConfig
-		expectTLSConfig  bool
-		expectRunnerFunc bool
-		skipSecureConfig bool
+		expectedConfig       *plugin.ClientConfig
+		expectTLSConfig      bool
+		expectRunnerFunc     bool
+		skipSecureConfig     bool
+		useLegacyEnvLayering bool
 	}
 
 	tests := map[string]testCase{
@@ -66,8 +67,9 @@ func TestMakeConfig(t *testing.T) {
 
 			responseWrapInfoTimes: 0,
 
-			mlockEnabled:      false,
-			mlockEnabledTimes: 1,
+			mlockEnabled:         false,
+			mlockEnabledTimes:    1,
+			useLegacyEnvLayering: true,
 
 			expectedConfig: &plugin.ClientConfig{
 				HandshakeConfig: plugin.HandshakeConfig{
@@ -83,12 +85,12 @@ func TestMakeConfig(t *testing.T) {
 				Cmd: commandWithEnv(
 					"echo",
 					[]string{"foo", "bar"},
-					[]string{
+					append(append([]string{
 						"initial=true",
 						fmt.Sprintf("%s=%s", PluginVaultVersionEnv, "dummyversion"),
 						fmt.Sprintf("%s=%t", PluginMetadataModeEnv, true),
 						fmt.Sprintf("%s=%t", PluginAutoMTLSEnv, false),
-					},
+					}, os.Environ()...), PluginUseLegacyEnvLayering+"=true"),
 				),
 				SecureConfig: &plugin.SecureConfig{
 					Checksum: []byte("some_sha256"),
@@ -98,8 +100,9 @@ func TestMakeConfig(t *testing.T) {
 					plugin.ProtocolNetRPC,
 					plugin.ProtocolGRPC,
 				},
-				Logger:   hclog.NewNullLogger(),
-				AutoMTLS: false,
+				Logger:      hclog.NewNullLogger(),
+				AutoMTLS:    false,
+				SkipHostEnv: true,
 			},
 			expectTLSConfig: false,
 		},
@@ -148,14 +151,14 @@ func TestMakeConfig(t *testing.T) {
 				Cmd: commandWithEnv(
 					"echo",
 					[]string{"foo", "bar"},
-					[]string{
+					append(os.Environ(), []string{
 						"initial=true",
 						fmt.Sprintf("%s=%t", PluginMlockEnabled, true),
 						fmt.Sprintf("%s=%s", PluginVaultVersionEnv, "dummyversion"),
 						fmt.Sprintf("%s=%t", PluginMetadataModeEnv, false),
 						fmt.Sprintf("%s=%t", PluginAutoMTLSEnv, false),
 						fmt.Sprintf("%s=%s", PluginUnwrapTokenEnv, "testtoken"),
-					},
+					}...),
 				),
 				SecureConfig: &plugin.SecureConfig{
 					Checksum: []byte("some_sha256"),
@@ -165,8 +168,9 @@ func TestMakeConfig(t *testing.T) {
 					plugin.ProtocolNetRPC,
 					plugin.ProtocolGRPC,
 				},
-				Logger:   hclog.NewNullLogger(),
-				AutoMTLS: false,
+				Logger:      hclog.NewNullLogger(),
+				AutoMTLS:    false,
+				SkipHostEnv: true,
 			},
 			expectTLSConfig: true,
 		},
@@ -212,12 +216,12 @@ func TestMakeConfig(t *testing.T) {
 				Cmd: commandWithEnv(
 					"echo",
 					[]string{"foo", "bar"},
-					[]string{
+					append(os.Environ(), []string{
 						"initial=true",
 						fmt.Sprintf("%s=%s", PluginVaultVersionEnv, "dummyversion"),
 						fmt.Sprintf("%s=%t", PluginMetadataModeEnv, true),
 						fmt.Sprintf("%s=%t", PluginAutoMTLSEnv, true),
-					},
+					}...),
 				),
 				SecureConfig: &plugin.SecureConfig{
 					Checksum: []byte("some_sha256"),
@@ -227,8 +231,9 @@ func TestMakeConfig(t *testing.T) {
 					plugin.ProtocolNetRPC,
 					plugin.ProtocolGRPC,
 				},
-				Logger:   hclog.NewNullLogger(),
-				AutoMTLS: true,
+				Logger:      hclog.NewNullLogger(),
+				AutoMTLS:    true,
+				SkipHostEnv: true,
 			},
 			expectTLSConfig: false,
 		},
@@ -274,12 +279,12 @@ func TestMakeConfig(t *testing.T) {
 				Cmd: commandWithEnv(
 					"echo",
 					[]string{"foo", "bar"},
-					[]string{
+					append(os.Environ(), []string{
 						"initial=true",
 						fmt.Sprintf("%s=%s", PluginVaultVersionEnv, "dummyversion"),
 						fmt.Sprintf("%s=%t", PluginMetadataModeEnv, false),
 						fmt.Sprintf("%s=%t", PluginAutoMTLSEnv, true),
-					},
+					}...),
 				),
 				SecureConfig: &plugin.SecureConfig{
 					Checksum: []byte("some_sha256"),
@@ -289,8 +294,9 @@ func TestMakeConfig(t *testing.T) {
 					plugin.ProtocolNetRPC,
 					plugin.ProtocolGRPC,
 				},
-				Logger:   hclog.NewNullLogger(),
-				AutoMTLS: true,
+				Logger:      hclog.NewNullLogger(),
+				AutoMTLS:    true,
+				SkipHostEnv: true,
 			},
 			expectTLSConfig: false,
 		},
@@ -369,6 +375,10 @@ func TestMakeConfig(t *testing.T) {
 			ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
 			defer cancel()
 
+			if test.useLegacyEnvLayering {
+				t.Setenv(PluginUseLegacyEnvLayering, "true")
+			}
+
 			config, err := test.rc.makeConfig(ctx)
 			if err != nil {
 				t.Fatalf("no error expected, got: %s", err)

sdk/plugin/mock/backend.go

@@ -59,6 +59,7 @@ func Backend() *backend {
 				pathInternal(&b),
 				pathSpecial(&b),
 				pathRaw(&b),
+				pathEnv(&b),
 			},
 		),
 		PathsSpecial: &logical.Paths{

sdk/plugin/mock/path_env.go

@@ -0,0 +1,38 @@
+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: MPL-2.0
+
+package mock
+
+import (
+	"context"
+	"os"
+
+	"github.com/hashicorp/vault/sdk/framework"
+	"github.com/hashicorp/vault/sdk/logical"
+)
+
+// pathEnv is used to interrogate plugin env vars.
+func pathEnv(b *backend) *framework.Path {
+	return &framework.Path{
+		Pattern: "env/" + framework.GenericNameRegex("key"),
+		Fields: map[string]*framework.FieldSchema{
+			"key": {
+				Type:        framework.TypeString,
+				Required:    true,
+				Description: "The name of the environment variable to read.",
+			},
+		},
+		Callbacks: map[logical.Operation]framework.OperationFunc{
+			logical.ReadOperation: b.pathEnvRead,
+		},
+	}
+}
+
+func (b *backend) pathEnvRead(_ context.Context, _ *logical.Request, data *framework.FieldData) (*logical.Response, error) {
+	// Return the secret
+	return &logical.Response{
+		Data: map[string]interface{}{
+			"key": os.Getenv(data.Get("key").(string)),
+		},
+	}, nil
+}