From 37d08a6c567cd2bb8b788ae024ff5a2cc425d46d Mon Sep 17 00:00:00 2001 From: Ivan Kozlovic Date: Fri, 12 Jul 2019 12:10:28 -0600 Subject: [PATCH] [FIXED] Allow TLS InsecureSkipVerify again This has an effect only on connections created by the server, so routes and gateways (explicit and implicit). Make sure that an explicit warning is printed if the insecure property is set, but otherwise allow it. Resolves #1062 Signed-off-by: Ivan Kozlovic --- server/gateway.go | 24 +++++++++++++++++------ server/route.go | 2 +- server/server.go | 5 ----- server/server_test.go | 41 ++++++++++++++++++++++++++++------------ test/cluster_tls_test.go | 2 +- 5 files changed, 49 insertions(+), 25 deletions(-) diff --git a/server/gateway.go b/server/gateway.go index d17279fe..1c7996de 100644 --- a/server/gateway.go +++ b/server/gateway.go @@ -46,6 +46,9 @@ var ( gatewaySolicitDelay = int64(defaultSolicitGatewaysDelay) ) +// Warning when user configures gateway TLS insecure +const gatewayTLSInsecureWarning = "TLS certificate chain and hostname of solicited gateways will not be verified, do not use in production!" + // SetGatewaysSolicitDelay sets the initial delay before gateways // connections are initiated. // Used by tests. @@ -231,9 +234,6 @@ func validateGatewayOptions(o *Options) error { if o.Gateway.Port == 0 { return fmt.Errorf("gateway %q has no port specified (select -1 for random port)", o.Gateway.Name) } - if o.Gateway.TLSConfig != nil && o.Gateway.TLSConfig.InsecureSkipVerify { - return fmt.Errorf("tls InsecureSkipVerify not supported for gateway") - } for i, g := range o.Gateway.Gateways { if g.Name == "" { return fmt.Errorf("gateway in the list %d has no name", i) @@ -241,9 +241,6 @@ func validateGatewayOptions(o *Options) error { if len(g.URLs) == 0 { return fmt.Errorf("gateway %q has no URL", g.Name) } - if g.TLSConfig != nil && g.TLSConfig.InsecureSkipVerify { - return fmt.Errorf("tls InsecureSkipVerify not supported for remote gateway %q", g.Name) - } } return nil } @@ -434,6 +431,21 @@ func (s *Server) gatewayAcceptLoop(ch chan struct{}) { } // Setup state that can enable shutdown s.gatewayListener = l + // Warn if insecure is configured in gateway{} or any remoteGateway{} + if tlsReq { + warn := opts.Gateway.TLSConfig.InsecureSkipVerify + if !warn { + for _, g := range opts.Gateway.Gateways { + if g.TLSConfig != nil && g.TLSConfig.InsecureSkipVerify { + warn = true + break + } + } + } + if warn { + s.Warnf(gatewayTLSInsecureWarning) + } + } s.mu.Unlock() // Let them know we are up diff --git a/server/route.go b/server/route.go index b775f5ef..f67f4331 100644 --- a/server/route.go +++ b/server/route.go @@ -97,7 +97,7 @@ const ( const sendRouteSubsInGoRoutineThreshold = 1024 * 1024 // 1MB // Warning when user configures cluster TLS insecure -const clusterTLSInsecureWarning = "TLS Hostname verification disabled, system will not verify identity of the solicited route" +const clusterTLSInsecureWarning = "TLS certificate chain and hostname of solicited routes will not be verified, do not use in production!" // Can be changed for tests var routeConnectDelay = DEFAULT_ROUTE_CONNECT diff --git a/server/server.go b/server/server.go index 36cd6091..90b6b518 100644 --- a/server/server.go +++ b/server/server.go @@ -309,11 +309,6 @@ func NewServer(opts *Options) (*Server, error) { } func validateOptions(o *Options) error { - // For now, InsecureSkipVerify is supported only for Cluster. - // So fail if it was set to client and or gateways. - if o.TLSConfig != nil && o.TLSConfig.InsecureSkipVerify { - return fmt.Errorf("tls InsecureSkipVerify not supported for client connections") - } // Check that the trust configuration is correct. if err := validateTrustedOperators(o); err != nil { return err diff --git a/server/server_test.go b/server/server_test.go index 2540f83d..ae0a71b6 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -1088,16 +1088,31 @@ func TestClientWriteLoopStall(t *testing.T) { } } -func TestInsecureSkipVerifyNotSupportedForClientAndGateways(t *testing.T) { - checkServerFails := func(t *testing.T, o *Options) { +func TestInsecureSkipVerifyWarning(t *testing.T) { + checkServerFails := func(t *testing.T, o *Options, expectedWarn string) { t.Helper() s, err := NewServer(o) - if s != nil { - s.Shutdown() + if err != nil { + t.Fatalf("Error on new server: %v", err) } - if err == nil || !strings.Contains(err.Error(), "not supported") { - t.Fatalf("Expected error about not supported feature, got %v", err) + l := &captureWarnLogger{warn: make(chan string, 1)} + s.SetLogger(l, false, false) + wg := sync.WaitGroup{} + wg.Add(1) + go func() { + s.Start() + wg.Done() + }() + select { + case w := <-l.warn: + if !strings.Contains(w, expectedWarn) { + t.Fatalf("Expected warning %q, got %q", expectedWarn, w) + } + case <-time.After(2 * time.Second): + t.Fatalf("Did not get any warning") } + s.Shutdown() + wg.Wait() } tc := &TLSConfigOpts{} @@ -1111,20 +1126,22 @@ func TestInsecureSkipVerifyNotSupportedForClientAndGateways(t *testing.T) { if err != nil { t.Fatalf("Error generating tls config: %v", err) } - o.TLSConfig = config - checkServerFails(t, o) + o.Cluster.Port = -1 + o.Cluster.TLSConfig = config + checkServerFails(t, o, clusterTLSInsecureWarning) // Get a clone that we will use for the Gateway TLS setting gwConfig := config.Clone() - // Remove the setting - o.TLSConfig.InsecureSkipVerify = false + // Remove the route setting + o.Cluster.Port = 0 + o.Cluster.TLSConfig = nil // Configure GW o.Gateway.Name = "A" o.Gateway.Host = "127.0.0.1" o.Gateway.Port = -1 o.Gateway.TLSConfig = gwConfig - checkServerFails(t, o) + checkServerFails(t, o, gatewayTLSInsecureWarning) // Get a config for the remote gateway rgwConfig := gwConfig.Clone() @@ -1139,7 +1156,7 @@ func TestInsecureSkipVerifyNotSupportedForClientAndGateways(t *testing.T) { TLSConfig: rgwConfig, }, } - checkServerFails(t, o) + checkServerFails(t, o, gatewayTLSInsecureWarning) } func TestConnectErrorReports(t *testing.T) { diff --git a/test/cluster_tls_test.go b/test/cluster_tls_test.go index b46fb3a6..fe9f554b 100644 --- a/test/cluster_tls_test.go +++ b/test/cluster_tls_test.go @@ -89,7 +89,7 @@ type captureClusterTLSInsecureLogger struct { func (c *captureClusterTLSInsecureLogger) Warnf(format string, v ...interface{}) { msg := fmt.Sprintf(format, v...) - if strings.Contains(msg, "verification disabled") { + if strings.Contains(msg, "solicited routes will not be verified") { select { case c.ch <- struct{}{}: default: