From 35801c9f4a48e65a7f3591b0d1f9b079ae03b41c Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Wed, 23 Jun 2021 18:09:14 +0800 Subject: [PATCH 1/8] client,server: better err msg when PD endpoint missing certificate --- cmd/client.go | 10 +++++++--- cmd/server.go | 16 +++++++++++++--- cmd/server_test.go | 7 +++++++ 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/cmd/client.go b/cmd/client.go index 62972e0cb97..05713a662c9 100644 --- a/cmd/client.go +++ b/cmd/client.go @@ -138,11 +138,15 @@ func newCliCommand() *cobra.Command { return errors.Annotate(err, "fail to validate TLS settings") } if tlsConfig != nil { - if strings.Contains(cliPdAddr, "http://") { + if strings.Contains(cliPdAddr, HTTP) { return errors.New("PD endpoint scheme should be https") } - } else if !strings.Contains(cliPdAddr, "http://") { - return errors.New("PD endpoint scheme should be http") + } else { + if strings.Contains(cliPdAddr, HTTPS) { + return errors.New("PD endpoint scheme is https, please provide certificate") + } else if !strings.Contains(cliPdAddr, HTTP) { + return errors.New("PD endpoint scheme should be http") + } } grpcTLSOption, err := credential.ToGRPCDialOption() if err != nil { diff --git a/cmd/server.go b/cmd/server.go index ed9fc2a128d..e1d9566f031 100644 --- a/cmd/server.go +++ b/cmd/server.go @@ -35,6 +35,12 @@ import ( "go.uber.org/zap" ) +// Endpoint schemes. +const ( + HTTP = "http://" + HTTPS = "https://" +) + var ( serverPdAddr string serverConfigFilePath string @@ -204,11 +210,15 @@ func loadAndVerifyServerConfig(cmd *cobra.Command) (*config.ServerConfig, error) } for _, ep := range strings.Split(serverPdAddr, ",") { if conf.Security.IsTLSEnabled() { - if strings.Index(ep, "http://") == 0 { + if strings.Index(ep, HTTP) == 0 { return nil, cerror.ErrInvalidServerOption.GenWithStack("PD endpoint scheme should be https") } - } else if strings.Index(ep, "http://") != 0 { - return nil, cerror.ErrInvalidServerOption.GenWithStack("PD endpoint scheme should be http") + } else { + if strings.Index(ep, HTTPS) == 0 { + return nil, errors.New("PD endpoint scheme is https, please provide certificate") + } else if strings.Index(ep, HTTP) != 0 { + return nil, cerror.ErrInvalidServerOption.GenWithStack("PD endpoint scheme should be http") + } } } diff --git a/cmd/server_test.go b/cmd/server_test.go index c3c444b2908..acb42491b0c 100644 --- a/cmd/server_test.go +++ b/cmd/server_test.go @@ -90,6 +90,13 @@ func (s *serverSuite) TestLoadAndVerifyServerConfig(c *check.C) { _, err = loadAndVerifyServerConfig(cmd) c.Assert(err, check.ErrorMatches, ".*PD endpoint scheme should be http.*") + // test missing certificate + cmd = new(cobra.Command) + initServerCmd(cmd) + c.Assert(cmd.ParseFlags([]string{"--pd=https://aa"}), check.IsNil) + _, err = loadAndVerifyServerConfig(cmd) + c.Assert(err, check.ErrorMatches, ".*PD endpoint scheme is https, please provide certificate.*") + // test undefined flag cmd = new(cobra.Command) initServerCmd(cmd) From 4e39edacdeb048232d3f5cc9c82b91947485adb8 Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Wed, 23 Jun 2021 19:04:18 +0800 Subject: [PATCH 2/8] refactor: better code --- cmd/client.go | 6 +++--- cmd/server.go | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/cmd/client.go b/cmd/client.go index 05713a662c9..a7c95e04974 100644 --- a/cmd/client.go +++ b/cmd/client.go @@ -138,13 +138,13 @@ func newCliCommand() *cobra.Command { return errors.Annotate(err, "fail to validate TLS settings") } if tlsConfig != nil { - if strings.Contains(cliPdAddr, HTTP) { + if strings.HasPrefix(cliPdAddr, HTTP) { return errors.New("PD endpoint scheme should be https") } } else { - if strings.Contains(cliPdAddr, HTTPS) { + if strings.HasPrefix(cliPdAddr, HTTPS) { return errors.New("PD endpoint scheme is https, please provide certificate") - } else if !strings.Contains(cliPdAddr, HTTP) { + } else if !strings.HasPrefix(cliPdAddr, HTTP) { return errors.New("PD endpoint scheme should be http") } } diff --git a/cmd/server.go b/cmd/server.go index e1d9566f031..ea3cb5a810e 100644 --- a/cmd/server.go +++ b/cmd/server.go @@ -210,13 +210,13 @@ func loadAndVerifyServerConfig(cmd *cobra.Command) (*config.ServerConfig, error) } for _, ep := range strings.Split(serverPdAddr, ",") { if conf.Security.IsTLSEnabled() { - if strings.Index(ep, HTTP) == 0 { + if strings.HasPrefix(ep, HTTP) { return nil, cerror.ErrInvalidServerOption.GenWithStack("PD endpoint scheme should be https") } } else { - if strings.Index(ep, HTTPS) == 0 { + if strings.HasPrefix(ep, HTTPS) { return nil, errors.New("PD endpoint scheme is https, please provide certificate") - } else if strings.Index(ep, HTTP) != 0 { + } else if !strings.HasPrefix(ep, HTTP) { return nil, cerror.ErrInvalidServerOption.GenWithStack("PD endpoint scheme should be http") } } From 7746d8b8d4a7138e4462f9f10145554b0d8cfb94 Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Mon, 28 Jun 2021 19:20:05 +0800 Subject: [PATCH 3/8] Use ErrInvalidServerOption err --- cmd/server.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/server.go b/cmd/server.go index ea3cb5a810e..ac1b5648a72 100644 --- a/cmd/server.go +++ b/cmd/server.go @@ -215,7 +215,7 @@ func loadAndVerifyServerConfig(cmd *cobra.Command) (*config.ServerConfig, error) } } else { if strings.HasPrefix(ep, HTTPS) { - return nil, errors.New("PD endpoint scheme is https, please provide certificate") + return nil, cerror.ErrInvalidServerOption.GenWithStack("PD endpoint scheme is https, please provide certificate") } else if !strings.HasPrefix(ep, HTTP) { return nil, cerror.ErrInvalidServerOption.GenWithStack("PD endpoint scheme should be http") } From 14f7a4ac9cf820f7d53e71750af85be2c5b6f9c3 Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Mon, 28 Jun 2021 19:50:25 +0800 Subject: [PATCH 4/8] Parse URL first --- cmd/client.go | 12 ++++++++---- cmd/server.go | 16 ++++++++++------ cmd/server_test.go | 11 +++++++++-- 3 files changed, 27 insertions(+), 12 deletions(-) diff --git a/cmd/client.go b/cmd/client.go index a7c95e04974..f837aabad4c 100644 --- a/cmd/client.go +++ b/cmd/client.go @@ -17,6 +17,7 @@ import ( "context" "fmt" "io" + "net/url" "os" "strings" "time" @@ -137,15 +138,18 @@ func newCliCommand() *cobra.Command { if err != nil { return errors.Annotate(err, "fail to validate TLS settings") } + u, err := url.Parse(cliPdAddr) + if err != nil || (u.Scheme != HTTP && u.Scheme != HTTPS) || u.Host == "" { + return errors.New("PD endpoint should be a valid http or https URL") + } + if tlsConfig != nil { - if strings.HasPrefix(cliPdAddr, HTTP) { + if u.Scheme == HTTP { return errors.New("PD endpoint scheme should be https") } } else { - if strings.HasPrefix(cliPdAddr, HTTPS) { + if u.Scheme == HTTPS { return errors.New("PD endpoint scheme is https, please provide certificate") - } else if !strings.HasPrefix(cliPdAddr, HTTP) { - return errors.New("PD endpoint scheme should be http") } } grpcTLSOption, err := credential.ToGRPCDialOption() diff --git a/cmd/server.go b/cmd/server.go index ac1b5648a72..0b3cf1f4743 100644 --- a/cmd/server.go +++ b/cmd/server.go @@ -15,6 +15,7 @@ package cmd import ( "context" + "net/url" "strings" "time" @@ -37,8 +38,8 @@ import ( // Endpoint schemes. const ( - HTTP = "http://" - HTTPS = "https://" + HTTP = "http" + HTTPS = "https" ) var ( @@ -209,15 +210,18 @@ func loadAndVerifyServerConfig(cmd *cobra.Command) (*config.ServerConfig, error) return nil, cerror.ErrInvalidServerOption.GenWithStack("empty PD address") } for _, ep := range strings.Split(serverPdAddr, ",") { + u, err := url.Parse(ep) + if err != nil || (u.Scheme != HTTP && u.Scheme != HTTPS) || u.Host == "" { + return nil, cerror.ErrInvalidServerOption.GenWithStack("PD endpoint should be a valid http or https URL") + } + if conf.Security.IsTLSEnabled() { - if strings.HasPrefix(ep, HTTP) { + if u.Scheme == HTTP { return nil, cerror.ErrInvalidServerOption.GenWithStack("PD endpoint scheme should be https") } } else { - if strings.HasPrefix(ep, HTTPS) { + if u.Scheme == HTTPS { return nil, cerror.ErrInvalidServerOption.GenWithStack("PD endpoint scheme is https, please provide certificate") - } else if !strings.HasPrefix(ep, HTTP) { - return nil, cerror.ErrInvalidServerOption.GenWithStack("PD endpoint scheme should be http") } } } diff --git a/cmd/server_test.go b/cmd/server_test.go index acb42491b0c..bffe1ac0abe 100644 --- a/cmd/server_test.go +++ b/cmd/server_test.go @@ -88,9 +88,16 @@ func (s *serverSuite) TestLoadAndVerifyServerConfig(c *check.C) { initServerCmd(cmd) c.Assert(cmd.ParseFlags([]string{"--pd=aa"}), check.IsNil) _, err = loadAndVerifyServerConfig(cmd) - c.Assert(err, check.ErrorMatches, ".*PD endpoint scheme should be http.*") + c.Assert(err, check.ErrorMatches, ".*PD endpoint should be a valid http or https URL.*") - // test missing certificate + // test invalid PD address + cmd = new(cobra.Command) + initServerCmd(cmd) + c.Assert(cmd.ParseFlags([]string{"--pd=http://"}), check.IsNil) + _, err = loadAndVerifyServerConfig(cmd) + c.Assert(err, check.ErrorMatches, ".*PD endpoint should be a valid http or https URL.*") + + // test missing certificate(without host) cmd = new(cobra.Command) initServerCmd(cmd) c.Assert(cmd.ParseFlags([]string{"--pd=https://aa"}), check.IsNil) From bcfb8c674eed89a6c961cc8ba8b9981c35ab313b Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Mon, 28 Jun 2021 19:51:15 +0800 Subject: [PATCH 5/8] Fix typo --- cmd/server_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/server_test.go b/cmd/server_test.go index bffe1ac0abe..0cfaa158938 100644 --- a/cmd/server_test.go +++ b/cmd/server_test.go @@ -90,14 +90,14 @@ func (s *serverSuite) TestLoadAndVerifyServerConfig(c *check.C) { _, err = loadAndVerifyServerConfig(cmd) c.Assert(err, check.ErrorMatches, ".*PD endpoint should be a valid http or https URL.*") - // test invalid PD address + // test invalid PD address(without host) cmd = new(cobra.Command) initServerCmd(cmd) c.Assert(cmd.ParseFlags([]string{"--pd=http://"}), check.IsNil) _, err = loadAndVerifyServerConfig(cmd) c.Assert(err, check.ErrorMatches, ".*PD endpoint should be a valid http or https URL.*") - // test missing certificate(without host) + // test missing certificate cmd = new(cobra.Command) initServerCmd(cmd) c.Assert(cmd.ParseFlags([]string{"--pd=https://aa"}), check.IsNil) From fc4ad54b5486e19b7d9e0499f260d12dccaa5a27 Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Mon, 28 Jun 2021 20:00:56 +0800 Subject: [PATCH 6/8] Better code --- cmd/client.go | 6 +++++- cmd/server.go | 5 ++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/cmd/client.go b/cmd/client.go index f837aabad4c..3df1cdce3c4 100644 --- a/cmd/client.go +++ b/cmd/client.go @@ -138,8 +138,12 @@ func newCliCommand() *cobra.Command { if err != nil { return errors.Annotate(err, "fail to validate TLS settings") } + u, err := url.Parse(cliPdAddr) - if err != nil || (u.Scheme != HTTP && u.Scheme != HTTPS) || u.Host == "" { + if err != nil { + return errors.Annotate(err, "parse PD endpoint") + } + if (u.Scheme != HTTP && u.Scheme != HTTPS) || u.Host == "" { return errors.New("PD endpoint should be a valid http or https URL") } diff --git a/cmd/server.go b/cmd/server.go index 0b3cf1f4743..d6b89ef78b4 100644 --- a/cmd/server.go +++ b/cmd/server.go @@ -211,7 +211,10 @@ func loadAndVerifyServerConfig(cmd *cobra.Command) (*config.ServerConfig, error) } for _, ep := range strings.Split(serverPdAddr, ",") { u, err := url.Parse(ep) - if err != nil || (u.Scheme != HTTP && u.Scheme != HTTPS) || u.Host == "" { + if err != nil { + return nil, errors.Annotate(err, "parse PD endpoint") + } + if (u.Scheme != HTTP && u.Scheme != HTTPS) || u.Host == "" { return nil, cerror.ErrInvalidServerOption.GenWithStack("PD endpoint should be a valid http or https URL") } From 2d52fa8ac16dc4f037d8f0cc4380b5e1b7174fdd Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Mon, 28 Jun 2021 21:45:14 +0800 Subject: [PATCH 7/8] Better code --- cmd/client.go | 18 ++---------------- cmd/server.go | 25 ++----------------------- cmd/util.go | 29 +++++++++++++++++++++++++++++ cmd/util_test.go | 36 ++++++++++++++++++++++++++++++++++++ 4 files changed, 69 insertions(+), 39 deletions(-) diff --git a/cmd/client.go b/cmd/client.go index 3df1cdce3c4..cbaaa39a3f1 100644 --- a/cmd/client.go +++ b/cmd/client.go @@ -17,7 +17,6 @@ import ( "context" "fmt" "io" - "net/url" "os" "strings" "time" @@ -139,23 +138,10 @@ func newCliCommand() *cobra.Command { return errors.Annotate(err, "fail to validate TLS settings") } - u, err := url.Parse(cliPdAddr) - if err != nil { - return errors.Annotate(err, "parse PD endpoint") - } - if (u.Scheme != HTTP && u.Scheme != HTTPS) || u.Host == "" { - return errors.New("PD endpoint should be a valid http or https URL") + if err := verifyPdEndpoint(cliPdAddr, tlsConfig != nil); err != nil { + return errors.Trace(err) } - if tlsConfig != nil { - if u.Scheme == HTTP { - return errors.New("PD endpoint scheme should be https") - } - } else { - if u.Scheme == HTTPS { - return errors.New("PD endpoint scheme is https, please provide certificate") - } - } grpcTLSOption, err := credential.ToGRPCDialOption() if err != nil { return errors.Annotate(err, "fail to validate TLS settings") diff --git a/cmd/server.go b/cmd/server.go index d6b89ef78b4..b57076273f2 100644 --- a/cmd/server.go +++ b/cmd/server.go @@ -15,7 +15,6 @@ package cmd import ( "context" - "net/url" "strings" "time" @@ -36,12 +35,6 @@ import ( "go.uber.org/zap" ) -// Endpoint schemes. -const ( - HTTP = "http" - HTTPS = "https" -) - var ( serverPdAddr string serverConfigFilePath string @@ -210,22 +203,8 @@ func loadAndVerifyServerConfig(cmd *cobra.Command) (*config.ServerConfig, error) return nil, cerror.ErrInvalidServerOption.GenWithStack("empty PD address") } for _, ep := range strings.Split(serverPdAddr, ",") { - u, err := url.Parse(ep) - if err != nil { - return nil, errors.Annotate(err, "parse PD endpoint") - } - if (u.Scheme != HTTP && u.Scheme != HTTPS) || u.Host == "" { - return nil, cerror.ErrInvalidServerOption.GenWithStack("PD endpoint should be a valid http or https URL") - } - - if conf.Security.IsTLSEnabled() { - if u.Scheme == HTTP { - return nil, cerror.ErrInvalidServerOption.GenWithStack("PD endpoint scheme should be https") - } - } else { - if u.Scheme == HTTPS { - return nil, cerror.ErrInvalidServerOption.GenWithStack("PD endpoint scheme is https, please provide certificate") - } + if err := verifyPdEndpoint(ep, conf.Security.IsTLSEnabled()); err != nil { + return nil, cerror.ErrInvalidServerOption.Wrap(err).GenWithStackByCause() } } diff --git a/cmd/util.go b/cmd/util.go index 63d7581d462..0738e0e7918 100644 --- a/cmd/util.go +++ b/cmd/util.go @@ -60,6 +60,12 @@ var errOwnerNotFound = liberrors.New("owner not found") var tsGapWarnning int64 = 86400 * 1000 // 1 day in milliseconds +// Endpoint schemes. +const ( + HTTP = "http" + HTTPS = "https" +) + func addSecurityFlags(flags *pflag.FlagSet, isServer bool) { flags.StringVar(&caPath, "ca", "", "CA certificate path for TLS connection") flags.StringVar(&certPath, "cert", "", "Certificate path for TLS connection") @@ -365,3 +371,26 @@ func confirmLargeDataGap(ctx context.Context, cmd *cobra.Command, startTs uint64 } return nil } + +// verifyPdEndpoint verifies whether the pd endpoint is a valid http or https URL. +// The certificate is required when using https. +func verifyPdEndpoint(pdEndpoint string, useTLS bool) error { + u, err := url.Parse(pdEndpoint) + if err != nil { + return errors.Annotate(err, "parse PD endpoint") + } + if (u.Scheme != HTTP && u.Scheme != HTTPS) || u.Host == "" { + return errors.New("PD endpoint should be a valid http or https URL") + } + + if useTLS { + if u.Scheme == HTTP { + return errors.New("PD endpoint scheme should be https") + } + } else { + if u.Scheme == HTTPS { + return errors.New("PD endpoint scheme is https, please provide certificate") + } + } + return nil +} diff --git a/cmd/util_test.go b/cmd/util_test.go index 703ecb6819d..605197b5382 100644 --- a/cmd/util_test.go +++ b/cmd/util_test.go @@ -55,3 +55,39 @@ func (s *utilsSuite) TestProxyFields(c *check.C) { } } } + +func (s *utilsSuite) TestVerifyPdEndpoint(c *check.C) { + // empty URL. + url := "" + c.Assert(verifyPdEndpoint(url, false), check.ErrorMatches, ".*PD endpoint should be a valid http or https URL.*") + + // invalid URL. + url = "\n hi" + c.Assert(verifyPdEndpoint(url, false), check.ErrorMatches, ".*invalid control character in URL.*") + + // http URL without host. + url = "http://" + c.Assert(verifyPdEndpoint(url, false), check.ErrorMatches, ".*PD endpoint should be a valid http or https URL.*") + + // https URL without host. + url = "https://" + c.Assert(verifyPdEndpoint(url, false), check.ErrorMatches, ".*PD endpoint should be a valid http or https URL.*") + + // postgres scheme. + url = "postgres://postgres@localhost/cargo_registry" + c.Assert(verifyPdEndpoint(url, false), check.ErrorMatches, ".*PD endpoint should be a valid http or https URL.*") + + // https scheme without TLS. + url = "https://aa" + c.Assert(verifyPdEndpoint(url, false), check.ErrorMatches, ".*PD endpoint scheme is https, please provide certificate.*") + + // http scheme with TLS. + url = "http://aa" + c.Assert(verifyPdEndpoint(url, true), check.ErrorMatches, ".*PD endpoint scheme should be https.*") + + // valid http URL. + c.Assert(verifyPdEndpoint("http://aa", false), check.IsNil) + + // valid https URL with TLS. + c.Assert(verifyPdEndpoint("https://aa", true), check.IsNil) +} From 53c0ce5c86a3472262e92cb8186ef193ec175736 Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Mon, 28 Jun 2021 21:51:08 +0800 Subject: [PATCH 8/8] Fix check --- cmd/util_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cmd/util_test.go b/cmd/util_test.go index 605197b5382..09514314187 100644 --- a/cmd/util_test.go +++ b/cmd/util_test.go @@ -57,6 +57,7 @@ func (s *utilsSuite) TestProxyFields(c *check.C) { } func (s *utilsSuite) TestVerifyPdEndpoint(c *check.C) { + defer testleak.AfterTest(c)() // empty URL. url := "" c.Assert(verifyPdEndpoint(url, false), check.ErrorMatches, ".*PD endpoint should be a valid http or https URL.*")