rename --experimental-peer-skip-client-san-verification to --peer-ski…

…p-client-san-verification
etcd-io · Jan 27, 2025 · 4378dbe · 4378dbe
1 parent 532c601
commit 4378dbe
Show file tree

Hide file tree

Showing 5 changed files with 147 additions and 12 deletions.
diff --git a/server/embed/config.go b/server/embed/config.go
@@ -415,6 +415,13 @@ type Config struct {
 	// TODO: Delete in v3.7
 	ExperimentalBootstrapDefragThresholdMegabytes uint `json:"experimental-bootstrap-defrag-threshold-megabytes"`
 	BootstrapDefragThresholdMegabytes             uint `json:"bootstrap-defrag-threshold-megabytes"`
+
+	// ExperimentalPeerSkipClientSanVerification determines whether to skip verification of SAN field
+	// in client certificate for peer connections.
+	// Deprecated in v3.6 and will be decommissioned in v3.7.
+	// TODO: Delete in v3.7
+	ExperimentalPeerSkipClientSanVerification bool `json:"experimental-peer-skip-client-san-verification"`
+
 	// WarningUnaryRequestDuration is the time duration after which a warning is generated if applying
 	// unary request takes more time than this value.
 	WarningUnaryRequestDuration time.Duration `json:"warning-unary-request-duration"`
@@ -533,15 +540,16 @@ type configJSON struct {
 }
 
 type securityConfig struct {
-	CertFile         string   `json:"cert-file"`
-	KeyFile          string   `json:"key-file"`
-	ClientCertFile   string   `json:"client-cert-file"`
-	ClientKeyFile    string   `json:"client-key-file"`
-	CertAuth         bool     `json:"client-cert-auth"`
-	TrustedCAFile    string   `json:"trusted-ca-file"`
-	AutoTLS          bool     `json:"auto-tls"`
-	AllowedCNs       []string `json:"allowed-cn"`
-	AllowedHostnames []string `json:"allowed-hostname"`
+	CertFile                      string   `json:"cert-file"`
+	KeyFile                       string   `json:"key-file"`
+	ClientCertFile                string   `json:"client-cert-file"`
+	ClientKeyFile                 string   `json:"client-key-file"`
+	CertAuth                      bool     `json:"client-cert-auth"`
+	TrustedCAFile                 string   `json:"trusted-ca-file"`
+	AutoTLS                       bool     `json:"auto-tls"`
+	AllowedCNs                    []string `json:"allowed-cn"`
+	AllowedHostnames              []string `json:"allowed-hostname"`
+	PeerSkipClientSanVerification bool     `json:"peer-skip-client-san-verification"`
 }
 
 // NewConfig creates a new Config populated with default values.
@@ -749,7 +757,10 @@ func (cfg *Config) AddFlags(fs *flag.FlagSet) {
 	fs.Var(flags.NewStringsValue(""), "peer-cert-allowed-cn", "Comma-separated list of allowed CNs for inter-peer TLS authentication.")
 	fs.Var(flags.NewStringsValue(""), "peer-cert-allowed-hostname", "Comma-separated list of allowed SAN hostnames for inter-peer TLS authentication.")
 	fs.Var(flags.NewStringsValue(""), "cipher-suites", "Comma-separated list of supported TLS cipher suites between client/server and peers (empty will be auto-populated by Go).")
-	fs.BoolVar(&cfg.PeerTLSInfo.SkipClientSANVerify, "experimental-peer-skip-client-san-verification", false, "Skip verification of SAN field in client certificate for peer connections.")
+
+	fs.BoolVar(&cfg.ExperimentalPeerSkipClientSanVerification, "experimental-peer-skip-client-san-verification", false, "Skip verification of SAN field in client certificate for peer connections.Deprecated in v3.6 and will be decommissioned in v3.7. Use peer-skip-client-san-verification instead")
+	fs.BoolVar(&cfg.PeerTLSInfo.SkipClientSANVerify, "peer-skip-client-san-verification", false, "Skip verification of SAN field in client certificate for peer connections.")
+
 	fs.StringVar(&cfg.TlsMinVersion, "tls-min-version", string(tlsutil.TLSVersion12), "Minimum TLS version supported by etcd. Possible values: TLS1.2, TLS1.3.")
 	fs.StringVar(&cfg.TlsMaxVersion, "tls-max-version", string(tlsutil.TLSVersionDefault), "Maximum TLS version supported by etcd. Possible values: TLS1.2, TLS1.3 (empty defers to Go).")
 
@@ -969,6 +980,7 @@ func (cfg *configYAML) configFromFile(path string) error {
 		tls.TrustedCAFile = ysc.TrustedCAFile
 		tls.AllowedCNs = ysc.AllowedCNs
 		tls.AllowedHostnames = ysc.AllowedHostnames
+		tls.SkipClientSANVerify = ysc.PeerSkipClientSanVerification
 	}
 	copySecurityDetails(&cfg.ClientTLSInfo, &cfg.ClientSecurityJSON)
 	copySecurityDetails(&cfg.PeerTLSInfo, &cfg.PeerSecurityJSON)

diff --git a/server/embed/config_test.go b/server/embed/config_test.go
@@ -48,7 +48,7 @@ func notFoundErr(service, domain string) error {
 func TestConfigFileOtherFields(t *testing.T) {
 	ctls := securityConfig{TrustedCAFile: "cca", CertFile: "ccert", KeyFile: "ckey"}
 	// Note AllowedCN and AllowedHostname are mutually exclusive, this test is just to verify the fields can be correctly marshalled & unmarshalled.
-	ptls := securityConfig{TrustedCAFile: "pca", CertFile: "pcert", KeyFile: "pkey", AllowedCNs: []string{"etcd"}, AllowedHostnames: []string{"whatever.example.com"}}
+	ptls := securityConfig{TrustedCAFile: "pca", CertFile: "pcert", KeyFile: "pkey", AllowedCNs: []string{"etcd"}, AllowedHostnames: []string{"whatever.example.com"}, PeerSkipClientSanVerification: true}
 	yc := struct {
 		ClientSecurityCfgFile securityConfig       `json:"client-transport-security"`
 		PeerSecurityCfgFile   securityConfig       `json:"peer-transport-security"`

diff --git a/server/etcdmain/config.go b/server/etcdmain/config.go
@@ -199,6 +199,10 @@ func (cfg *config) parse(arguments []string) error {
 		cfg.ec.BootstrapDefragThresholdMegabytes = cfg.ec.ExperimentalBootstrapDefragThresholdMegabytes
 	}
 
+	if cfg.ec.FlagsExplicitlySet["experimental-peer-skip-client-san-verification"] {
+		cfg.ec.PeerTLSInfo.SkipClientSANVerify = cfg.ec.ExperimentalPeerSkipClientSanVerification
+	}
+
 	// `V2Deprecation` (--v2-deprecation) is deprecated and scheduled for removal in v3.8. The default value is enforced, ignoring user input.
 	cfg.ec.V2Deprecation = cconfig.V2DeprDefault
 

diff --git a/server/etcdmain/config_test.go b/server/etcdmain/config_test.go
@@ -21,6 +21,7 @@ import (
 	"net/url"
 	"os"
 	"reflect"
+	"strconv"
 	"strings"
 	"testing"
 	"time"
@@ -1085,3 +1086,121 @@ func TestConfigFileDeprecatedOptions(t *testing.T) {
 		})
 	}
 }
+
+// TestPeerSkipClientSanVerificationFlagMigration tests the migration from
+// --experimental-peer-skip-client-san-verification to --peer-skip-client-san-verification
+// TODO: delete in v3.7
+func TestPeerSkipClientSanVerificationFlagMigration(t *testing.T) {
+	testCases := []struct {
+		name                                      string
+		peerSkipClientSanVerification             string
+		experimentalPeerSkipClientSanVerification string
+		useConfigFile                             bool
+		expectErr                                 bool
+		expectedPeerSkipClientSanVerification     bool
+	}{
+		{
+			name:                          "set both experimental flag and non experimental flag,experimental = false,non experimental = true",
+			peerSkipClientSanVerification: "true",
+			experimentalPeerSkipClientSanVerification: "false",
+			expectedPeerSkipClientSanVerification:     false,
+		},
+		{
+			name:                          "set both experimental flag and non experimental flag,experimental = true,non experimental = false",
+			peerSkipClientSanVerification: "false",
+			experimentalPeerSkipClientSanVerification: "true",
+			expectedPeerSkipClientSanVerification:     true,
+		},
+		{
+			name: "can set experimental flag to true",
+			experimentalPeerSkipClientSanVerification: "true",
+			expectedPeerSkipClientSanVerification:     true,
+		},
+		{
+			name: "can set experimental flag to false",
+			experimentalPeerSkipClientSanVerification: "false",
+			expectedPeerSkipClientSanVerification:     false,
+		},
+		{
+			name:                                  "can set non experimental flag to true",
+			peerSkipClientSanVerification:         "true",
+			expectedPeerSkipClientSanVerification: true,
+		},
+		{
+			name:                                  "can set non experimental flag to false",
+			peerSkipClientSanVerification:         "false",
+			expectedPeerSkipClientSanVerification: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			cmdLineArgs := []string{}
+			type securityConfig struct {
+				PeerSkipClientSanVerification bool `json:"peer-skip-client-san-verification"`
+			}
+			yc := struct {
+				ExperimentalPeerSkipClientSanVerification bool           `json:"experimental-peer-skip-client-san-verification,omitempty"`
+				PeerSkipClientSanVerification             bool           `json:"peer-skip-client-san-verification,omitempty"`
+				PeerSecurityCfgFile                       securityConfig `json:"peer-transport-security"`
+			}{}
+
+			if tc.peerSkipClientSanVerification != "" {
+				cmdLineArgs = append(cmdLineArgs, fmt.Sprintf("--peer-skip-client-san-verification=%s", tc.peerSkipClientSanVerification))
+				val, err := strconv.ParseBool(tc.peerSkipClientSanVerification)
+				if err != nil {
+					t.Fatal(err)
+				}
+				yc.PeerSkipClientSanVerification = val
+				yc.PeerSecurityCfgFile.PeerSkipClientSanVerification = val
+			}
+
+			if tc.experimentalPeerSkipClientSanVerification != "" {
+				cmdLineArgs = append(cmdLineArgs, fmt.Sprintf("--experimental-peer-skip-client-san-verification=%s", tc.experimentalPeerSkipClientSanVerification))
+				val, err := strconv.ParseBool(tc.experimentalPeerSkipClientSanVerification)
+				if err != nil {
+					t.Fatal(err)
+				}
+				yc.ExperimentalPeerSkipClientSanVerification = val
+				yc.PeerSecurityCfgFile.PeerSkipClientSanVerification = val
+			}
+
+			b, err := yaml.Marshal(&yc)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			tmpfile := mustCreateCfgFile(t, b)
+			defer os.Remove(tmpfile.Name())
+
+			cfgFromCmdLine := newConfig()
+			errFromCmdLine := cfgFromCmdLine.parse(cmdLineArgs)
+
+			cfgFromFile := newConfig()
+			errFromFile := cfgFromFile.parse([]string{fmt.Sprintf("--config-file=%s", tmpfile.Name())})
+
+			if tc.expectErr {
+				if errFromCmdLine == nil || errFromFile == nil {
+					t.Fatal("expect parse error")
+				}
+				return
+			}
+
+			if errFromCmdLine != nil || errFromFile != nil {
+				t.Fatal(err)
+			}
+
+			if cfgFromCmdLine.ec.PeerTLSInfo.SkipClientSANVerify != tc.expectedPeerSkipClientSanVerification {
+				t.Errorf("expected SkipClientSANVerify=%v, got %v",
+					tc.expectedPeerSkipClientSanVerification,
+					cfgFromCmdLine.ec.PeerTLSInfo.SkipClientSANVerify)
+			}
+
+			if cfgFromFile.ec.PeerTLSInfo.SkipClientSANVerify != tc.expectedPeerSkipClientSanVerification {
+				t.Errorf("expected SkipClientSANVerify=%v, got %v",
+					tc.expectedPeerSkipClientSanVerification,
+					cfgFromFile.ec.PeerTLSInfo.SkipClientSANVerify)
+			}
+		})
+	}
+}
diff --git a/server/etcdmain/help.go b/server/etcdmain/help.go
@@ -292,7 +292,7 @@ Experimental feature:
   --compaction-batch-limit 1000
     CompactionBatchLimit sets the maximum revisions deleted in each compaction batch.
   --experimental-peer-skip-client-san-verification 'false'
-    Skip verification of SAN field in client certificate for peer connections.
+    Skip verification of SAN field in client certificate for peer connections.Deprecated in v3.6 and will be decommissioned in v3.7. Use 'peer-skip-client-san-verification' instead.
   --experimental-watch-progress-notify-interval '10m'
     Duration of periodical watch progress notification. Deprecated in v3.6 and will be decommissioned in v3.7. Use 'watch-progress-notify-interval' instead.
   --watch-progress-notify-interval '10m'