Fix env.LookupEnv treating values that start with "env" as references

env/env_test.go

@@ -18,10 +18,16 @@
 package env
 
 import (
+	"crypto/x509"
+	"errors"
 	"fmt"
+	"net"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"os"
+	"strings"
+	"sync/atomic"
 	"testing"
 	"time"
 
@@ -165,3 +171,110 @@ func TestGetEnv(t *testing.T) {
 		t.Fatalf("Expected 'value-new', but got %s", v)
 	}
 }
+
+func TestLookupEnvReferenceScheme(t *testing.T) {
+	ts := startTestServer(t)
+	tlsTS := httptest.NewTLSServer(ts.Config.Handler)
+	t.Cleanup(tlsTS.Close)
+	pool := x509.NewCertPool()
+	pool.AddCert(tlsTS.Certificate())
+	prevCAs := globalRootCAs
+	RegisterGlobalCAs(pool)
+	t.Cleanup(func() { RegisterGlobalCAs(prevCAs) })
+	unparsableLn, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { unparsableLn.Close() })
+	var unparsableConns atomic.Int32
+	go func() {
+		for {
+			c, err := unparsableLn.Accept()
+			if err != nil {
+				return
+			}
+			unparsableConns.Add(1)
+			c.Close()
+		}
+	}()
+
+	host := func(s *httptest.Server) string {
+		u, err := url.Parse(s.URL)
+		if err != nil {
+			t.Fatal(err)
+		}
+		return u.Host
+	}
+	const path = "/webhook/v1/getenv/default/minio"
+	const fetched = "http://127.0.0.{1..4}:9000/data{1...4}"
+
+	testCases := []struct {
+		name, value, cached      string
+		want, wantUser, wantPass string
+		wantParseErr             bool
+	}{
+		{name: "plain envroot", value: "envroot", want: "envroot"},
+		{name: "plain value with stale cache", value: "envroot", cached: "stale", want: "envroot"},
+		{name: "plain env", value: "env", want: "env"},
+		{name: "plain environment", value: "environment", want: "environment"},
+		{name: "plain env+tlsx", value: "env+tlsx", want: "env+tlsx"},
+		{name: "opaque env colon", value: "env:foo", want: "env:foo"},
+		{name: "single slash", value: "env:/x", want: "env:/x"},
+		{name: "other scheme envs", value: "envs://h/x", want: "envs://h/x"},
+		{name: "other scheme env-tls", value: "env-tls://h/x", want: "env-tls://h/x"},
+		{name: "other scheme env+tlsx", value: "env+tlsx://h/x", want: "env+tlsx://h/x"},
+		{name: "leading space", value: " env://x", want: " env://x"},
+		{name: "unicode fold of tls", value: "ENV+TL\u017f://x", want: "ENV+TL\u017f://x"},
+		{name: "env reference", value: "env://minio:minio123@" + host(ts) + path, want: fetched, wantUser: "minio", wantPass: "minio123"},
+		{name: "upper case env reference", value: "ENV://minio:minio123@" + host(ts) + path, want: fetched, wantUser: "minio", wantPass: "minio123"},
+		{name: "env+tls reference", value: "env+tls://minio:minio123@" + host(tlsTS) + path, want: fetched, wantUser: "minio", wantPass: "minio123"},
+		{name: "mixed case env+tls reference", value: "env+TLS://minio:minio123@" + host(tlsTS) + path, want: fetched, wantUser: "minio", wantPass: "minio123"},
+		{name: "unparsable reference", value: "env://minio:minio123@" + unparsableLn.Addr().String() + "/%zz", wantParseErr: true},
+		{name: "unreachable reference uses cache", value: "env://minio:minio123@127.0.0.1:1" + path, cached: "cached", want: "cached"},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Setenv("MINIO_ARGS", tc.value)
+			t.Setenv("_MINIO_ARGS", tc.cached)
+			if tc.cached == "" {
+				if err := os.Unsetenv("_MINIO_ARGS"); err != nil {
+					t.Fatal(err)
+				}
+			}
+			got, user, pass, err := LookupEnv("MINIO_ARGS")
+			if (err != nil) != tc.wantParseErr {
+				t.Fatalf("got error %v, want error %v", err, tc.wantParseErr)
+			}
+			var escapeErr url.EscapeError
+			if tc.wantParseErr && !errors.As(err, &escapeErr) {
+				t.Fatalf("got error %v, want a url.Parse escape error", err)
+			}
+			if got != tc.want || user != tc.wantUser || pass != tc.wantPass {
+				t.Fatalf("got (%q, %q, %q), want (%q, %q, %q)", got, user, pass, tc.want, tc.wantUser, tc.wantPass)
+			}
+			cache, cached := os.LookupEnv("_MINIO_ARGS")
+			switch {
+			case tc.wantUser != "":
+				if cache != tc.want {
+					t.Fatalf("cache holds %q, want %q", cache, tc.want)
+				}
+			case tc.cached == "":
+				if cached {
+					t.Fatalf("cache set to %q, want unset", cache)
+				}
+			case cache != tc.cached:
+				t.Fatalf("cache holds %q, want %q", cache, tc.cached)
+			}
+			want := strings.TrimSpace(tc.want)
+			if tc.wantParseErr {
+				want = "default"
+			}
+			if v := Get("MINIO_ARGS", "default"); v != want {
+				t.Fatalf("Get returned %q, want %q", v, want)
+			}
+			if n := unparsableConns.Load(); tc.wantParseErr && n != 0 {
+				t.Fatalf("unparsable reference opened %d connection(s), want 0", n)
+			}
+		})
+	}
+}

env/web_env.go

@@ -167,19 +167,24 @@ func Environ() []string {
 
 // LookupEnv retrieves the value of the environment variable named by `key`.
 //
-// If the value of the variable starts with "env://" or "env+tls://" it is
-// fetched from the referenced remote server. The fetched value is cached in a
-// separate environment variable prefixed with an underscore for subsequent
-// lookups should the remote server be unreachable. When fetching from a remote
-// server the username and password used are also returned.
+// If the value of the variable starts with "env://" or "env+tls://", in any
+// letter case, it is fetched from the referenced remote server. The fetched
+// value is cached in a separate environment variable prefixed with an
+// underscore for subsequent lookups should the remote server be unreachable.
+// When fetching from a remote server the username and password used are also
+// returned.
 //
 // For regular environment variables the value is returned as-is with empty
 // credentials.
 func LookupEnv(key string) (string, string, string, error) {
-	v, ok := os.LookupEnv(key)
-	if ok && strings.HasPrefix(v, webEnvScheme) {
-		// If env value starts with `env*://`
-		// continue to parse and fetch from remote
+	v := os.Getenv(key)
+	scheme, _, found := strings.Cut(v, "://")
+	if found {
+		scheme = strings.ToLower(scheme)
+	}
+	if found && (scheme == webEnvScheme || scheme == webEnvSchemeSecure) {
+		// Not strings.EqualFold: it also folds U+017F to "s", and
+		// url.Parse rejects such a scheme.
 		var err error
 		v, user, pwd, err := getEnvValueFromHTTP(strings.TrimSpace(v), key)
 		if err != nil {