Merge pull request #141 from appoptics/AO-15694-remove-skipverify-option

AO-15694: Remove skip verify option
solarwinds · Feb 26, 2020 · 8e29e46 · 8e29e46
2 parents 5f366a3 + cb73f9b
commit 8e29e46
Show file tree

Hide file tree

Showing 5 changed files with 37 additions and 76 deletions.
diff --git a/v1/ao/internal/config/config.go b/v1/ao/internal/config/config.go
@@ -49,7 +49,6 @@ const (
 	envAppOpticsSampleRate          = "APPOPTICS_SAMPLE_RATE"
 	envAppOpticsPrependDomain       = "APPOPTICS_PREPEND_DOMAIN"
 	envAppOpticsHostnameAlias       = "APPOPTICS_HOSTNAME_ALIAS"
-	envAppOpticsInsecureSkipVerify  = "APPOPTICS_INSECURE_SKIP_VERIFY"
 	envAppOpticsHistogramPrecision  = "APPOPTICS_HISTOGRAM_PRECISION"
 	envAppOpticsEventsFlushInterval = "APPOPTICS_EVENTS_FLUSH_INTERVAL"
 	envAppOpticsMaxReqBytes         = "APPOPTICS_MAX_REQUEST_BYTES"
@@ -93,9 +92,6 @@ type Config struct {
 	// The alias of the hostname
 	HostAlias string `yaml:"HostAlias,omitempty" env:"APPOPTICS_HOSTNAME_ALIAS"`
 
-	// Whether to skip verification of hostname
-	SkipVerify bool `yaml:"SkipVerify,omitempty" env:"APPOPTICS_INSECURE_SKIP_VERIFY" default:"true"`
-
 	// The precision of the histogram
 	Precision int `yaml:"Precision,omitempty" env:"APPOPTICS_HISTOGRAM_PRECISION" default:"2"`
 
@@ -778,13 +774,6 @@ func (c *Config) GetHostAlias() string {
 	return c.HostAlias
 }
 
-// GetSkipVerify returns the config of skipping hostname verification
-func (c *Config) GetSkipVerify() bool {
-	c.RLock()
-	defer c.RUnlock()
-	return c.SkipVerify
-}
-
 // GetPrecision returns the histogram precision
 func (c *Config) GetPrecision() int {
 	c.RLock()

diff --git a/v1/ao/internal/config/config_test.go b/v1/ao/internal/config/config_test.go
@@ -57,15 +57,13 @@ func TestLoadConfig(t *testing.T) {
 
 	os.Setenv(envAppOpticsServiceKey, key1)
 	os.Setenv(envAppOpticsHostnameAlias, "test")
-	os.Setenv(envAppOpticsInsecureSkipVerify, "false")
 	os.Setenv(envAppOpticsTrustedPath, "test.crt")
 	os.Setenv(envAppOpticsCollectorUDP, "hello.udp")
 	os.Setenv(envAppOpticsDisabled, "invalidValue")
 
 	c.Load()
 	assert.Equal(t, ToServiceKey(key1), c.GetServiceKey())
 	assert.Equal(t, "test", c.GetHostAlias())
-	assert.Equal(t, false, c.GetSkipVerify())
 	assert.Equal(t, "test.crt", filepath.Base(c.GetTrustedPath()))
 	assert.Equal(t, "hello.udp", c.GetCollectorUDP())
 	assert.Equal(t, false, c.GetDisabled())
@@ -130,7 +128,6 @@ func TestConfigInit(t *testing.T) {
 		},
 		PrependDomain: false,
 		HostAlias:     "",
-		SkipVerify:    true,
 		Precision:     2,
 		ReporterProperties: &ReporterOptions{
 			EventFlushInterval:      2,
@@ -186,7 +183,7 @@ func TestEnvsLoading(t *testing.T) {
 		"APPOPTICS_SAMPLE_RATE=1000",
 		"APPOPTICS_PREPEND_DOMAIN=true",
 		"APPOPTICS_HOSTNAME_ALIAS=alias",
-		"APPOPTICS_INSECURE_SKIP_VERIFY=true",
+
 		"APPOPTICS_HISTOGRAM_PRECISION=4",
 		"APPOPTICS_EVENTS_FLUSH_INTERVAL=4",
 		"APPOPTICS_MAX_REQUEST_BYTES=4096000",
@@ -214,7 +211,6 @@ func TestEnvsLoading(t *testing.T) {
 		},
 		PrependDomain: true,
 		HostAlias:     "alias",
-		SkipVerify:    true,
 		Precision:     2 * 2,
 		ReporterProperties: &ReporterOptions{
 			EventFlushInterval:      2 * 2,
@@ -259,7 +255,6 @@ func TestYamlConfig(t *testing.T) {
 		},
 		PrependDomain: true,
 		HostAlias:     "yaml-alias",
-		SkipVerify:    true,
 		Precision:     2 * 3,
 		ReporterProperties: &ReporterOptions{
 			EventFlushInterval:      2 * 3,
@@ -312,7 +307,6 @@ func TestYamlConfig(t *testing.T) {
 		"APPOPTICS_SAMPLE_RATE=1000",
 		"APPOPTICS_PREPEND_DOMAIN=true",
 		"APPOPTICS_HOSTNAME_ALIAS=alias",
-		"APPOPTICS_INSECURE_SKIP_VERIFY=true",
 		"APPOPTICS_HISTOGRAM_PRECISION=4",
 		"APPOPTICS_EVENTS_FLUSH_INTERVAL=4",
 		"APPOPTICS_MAX_REQUEST_BYTES=4096000",
@@ -337,7 +331,6 @@ func TestYamlConfig(t *testing.T) {
 		},
 		PrependDomain: true,
 		HostAlias:     "alias",
-		SkipVerify:    true,
 		Precision:     2 * 2,
 		ReporterProperties: &ReporterOptions{
 			EventFlushInterval:      2 * 2,
@@ -444,7 +437,6 @@ func TestInvalidConfig(t *testing.T) {
 		},
 		PrependDomain: true,
 		HostAlias:     "alias",
-		SkipVerify:    true,
 		Precision:     2 * 2,
 		ReporterProperties: &ReporterOptions{
 			EventFlushInterval:      2 * 2,

diff --git a/v1/ao/internal/config/wrappers.go b/v1/ao/internal/config/wrappers.go
@@ -36,9 +36,6 @@ var GetPrependDomain = conf.GetPrependDomain
 // GetHostAlias is a wrapper to the method of the global config
 var GetHostAlias = conf.GetHostAlias
 
-// GetSkipVerify is a wrapper to the method of the global config
-var GetSkipVerify = conf.GetSkipVerify
-
 // GetPrecision is a wrapper to the method of the global config
 var GetPrecision = conf.GetPrecision
 

diff --git a/v1/ao/internal/reporter/reporter_grpc.go b/v1/ao/internal/reporter/reporter_grpc.go
@@ -94,16 +94,15 @@ const (
 
 // everything needed for a GRPC connection
 type grpcConnection struct {
-	name               string                         // connection name
-	client             collector.TraceCollectorClient // GRPC client instance
-	connection         *grpc.ClientConn               // GRPC connection object
-	address            string                         // collector address
-	certificate        []byte                         // collector certificate
-	pingTicker         *time.Timer                    // timer for keep alive pings in seconds
-	pingTickerLock     sync.Mutex                     // lock to ensure sequential access of pingTicker
-	lock               sync.RWMutex                   // lock to ensure sequential access (in case of connection loss)
-	queueStats         *metrics.EventQueueStats       // queue stats (reset on each metrics report cycle)
-	insecureSkipVerify bool
+	name           string                         // connection name
+	client         collector.TraceCollectorClient // GRPC client instance
+	connection     *grpc.ClientConn               // GRPC connection object
+	address        string                         // collector address
+	certificate    []byte                         // collector certificate
+	pingTicker     *time.Timer                    // timer for keep alive pings in seconds
+	pingTickerLock sync.Mutex                     // lock to ensure sequential access of pingTicker
+	lock           sync.RWMutex                   // lock to ensure sequential access (in case of connection loss)
+	queueStats     *metrics.EventQueueStats       // queue stats (reset on each metrics report cycle)
 
 	proxy            string
 	proxyTLSCertPath string
@@ -133,13 +132,6 @@ func WithCert(cert []byte) GrpcConnOpt {
 	}
 }
 
-// WithSkipVerify returns a function that sets the insecureSkipVerify option
-func WithSkipVerify(skip bool) GrpcConnOpt {
-	return func(c *grpcConnection) {
-		c.insecureSkipVerify = skip
-	}
-}
-
 // WithProxy assign the proxy url to the gRPC connection
 func WithProxy(proxy string) GrpcConnOpt {
 	return func(c *grpcConnection) {
@@ -177,16 +169,15 @@ func WithBackoff(b Backoff) GrpcConnOpt {
 
 func newGrpcConnection(name string, target string, opts ...GrpcConnOpt) (*grpcConnection, error) {
 	gc := &grpcConnection{
-		name:               name,
-		client:             nil,
-		connection:         nil,
-		address:            target,
-		certificate:        []byte(grpcCertDefault),
-		queueStats:         &metrics.EventQueueStats{},
-		insecureSkipVerify: true,
-		backoff:            DefaultBackoff,
-		Dialer:             &DefaultDialer{},
-		flushed:            make(chan struct{}),
+		name:        name,
+		client:      nil,
+		connection:  nil,
+		address:     target,
+		certificate: []byte(grpcCertDefault),
+		queueStats:  &metrics.EventQueueStats{},
+		backoff:     DefaultBackoff,
+		Dialer:      &DefaultDialer{},
+		flushed:     make(chan struct{}),
 	}
 
 	for _, opt := range opts {
@@ -292,7 +283,6 @@ func newGRPCReporter() reporter {
 		opts = append(opts, WithCert(cert))
 	}
 
-	opts = append(opts, WithSkipVerify(config.GetSkipVerify()))
 	opts = append(opts, WithMaxReqBytes(config.ReporterOpts().GetMaxReqBytes()))
 
 	if proxy := getProxy(); proxy != "" {
@@ -506,11 +496,10 @@ func (c *grpcConnection) connect() error {
 	}
 	// create a new connection object for this client
 	conn, err := c.Dial(DialParams{
-		InsecureSkipVerify: c.insecureSkipVerify,
-		Certificate:        c.certificate,
-		Address:            c.address,
-		Proxy:              c.proxy,
-		ProxyCertPath:      c.proxyTLSCertPath,
+		Certificate:   c.certificate,
+		Address:       c.address,
+		Proxy:         c.proxy,
+		ProxyCertPath: c.proxyTLSCertPath,
 	})
 	if err != nil {
 		return errors.Wrap(err, "failed to connect to target")
@@ -1258,11 +1247,10 @@ type Dialer interface {
 }
 
 type DialParams struct {
-	InsecureSkipVerify bool
-	Certificate        []byte
-	Address            string
-	Proxy              string
-	ProxyCertPath      string
+	Certificate   []byte
+	Address       string
+	Proxy         string
+	ProxyCertPath string
 }
 
 // DefaultDialer implements the Dialer interface to provide the default dialing
@@ -1285,14 +1273,10 @@ func (d *DefaultDialer) Dial(p DialParams) (*grpc.ClientConn, error) {
 	}
 
 	tlsConfig := &tls.Config{
-		ServerName:         serverName,
-		RootCAs:            certPool,
-		InsecureSkipVerify: p.InsecureSkipVerify,
-	}
-	// turn off server certificate verification for Go < 1.8
-	if !utils.IsHigherOrEqualGoVersion("go1.8") {
-		tlsConfig.InsecureSkipVerify = true
+		ServerName: serverName,
+		RootCAs:    certPool,
 	}
+
 	creds := credentials.NewTLS(tlsConfig)
 
 	opts := []grpc.DialOption{
@@ -1329,7 +1313,7 @@ func newGRPCProxyDialer(p DialParams) func(context.Context, string) (net.Conn, e
 			caCertPool.AppendCertsFromPEM(cert)
 
 			// No mutual TLS for now
-			tlsConfig := tls.Config{RootCAs: caCertPool, InsecureSkipVerify: true}
+			tlsConfig := tls.Config{RootCAs: caCertPool}
 			conn, err = tls.Dial("tcp", proxy.Host, &tlsConfig)
 			if err != nil {
 				return nil, errors.Wrap(err, "failed to dial the https proxy")

diff --git a/v1/ao/internal/reporter/reporter_test.go b/v1/ao/internal/reporter/reporter_test.go
@@ -429,13 +429,12 @@ func TestInvokeRPC(t *testing.T) {
 	}()
 
 	c := &grpcConnection{
-		name:               "events channel",
-		client:             nil,
-		connection:         nil,
-		address:            "test-addr",
-		certificate:        []byte(grpcCertDefault),
-		queueStats:         &metrics.EventQueueStats{},
-		insecureSkipVerify: true,
+		name:        "events channel",
+		client:      nil,
+		connection:  nil,
+		address:     "test-addr",
+		certificate: []byte(grpcCertDefault),
+		queueStats:  &metrics.EventQueueStats{},
 		backoff: func(retries int, wait func(d time.Duration)) error {
 			if retries > grpcMaxRetries {
 				return errGiveUpAfterRetries