From b823bfed0ec889c1094c6a65dc2845e6bdf611bb Mon Sep 17 00:00:00 2001
From: tanvipise <56138955+tanvipise@users.noreply.github.com>
Date: Wed, 23 Oct 2024 09:13:37 -0400
Subject: [PATCH] changes to tls-mode from default to prefer and update test
 cases (#152)

* changes to tls-mode from default to prefer and update test cases

* update default in configutaion test

* Added test for prefer tls mode and made it default

* Fix typo

* Cleanup integration and unit test suites

* Update README.md to reflect 'prefer' TLS mode

* restoring previous logic

* testing

* tls-tests passing locally

* update unit test

* added commented test back

* modify test-helper.js to fix CI workflow

* address PR comments
---
 packages/v-connection-string/index.js         |  6 +--
 packages/v-connection-string/test/parse.js    |  4 +-
 packages/vertica-nodejs/README.md             |  4 +-
 packages/vertica-nodejs/lib/client.js         |  2 +-
 packages/vertica-nodejs/lib/connection.js     | 20 +++++---
 packages/vertica-nodejs/lib/defaults.js       |  2 +-
 .../integration/client/configuration-tests.js |  2 +-
 .../integration/connection/test-helper.js     |  2 +-
 .../test/integration/connection/tls-tests.js  | 50 ++++++++++++++++---
 .../test/integration/gh-issues/2307-tests.js  |  4 ++
 .../test/unit/client/configuration-tests.js   |  2 +-
 .../connection-parameters/creation-tests.js   |  4 +-
 .../environment-variable-tests.js             |  6 +--
 .../unit/connection/inbound-parser-tests.js   |  3 ++
 14 files changed, 81 insertions(+), 30 deletions(-)

diff --git a/packages/v-connection-string/index.js b/packages/v-connection-string/index.js
index 42f0b3ce..664a6c78 100644
--- a/packages/v-connection-string/index.js
+++ b/packages/v-connection-string/index.js
@@ -41,9 +41,9 @@ function parse(str) {
     }
   }
 
-  // if the tls mode specified in the connection string is an invalid option, use the default - disable.
-  if (config.tls_mode && !['require', 'verify-ca', 'verify-full'].includes(config.tls_mode)) {
-    config.tls_mode = 'disable'
+  // if the tls mode specified in the connection string is an invalid option, use the default - prefer.
+  if (config.tls_mode && !['disable', 'require', 'verify-ca', 'verify-full'].includes(config.tls_mode)) {
+    config.tls_mode = 'prefer'
   }
   
   var auth = (result.auth || ':').split(':')
diff --git a/packages/v-connection-string/test/parse.js b/packages/v-connection-string/test/parse.js
index d56db656..d76348f8 100644
--- a/packages/v-connection-string/test/parse.js
+++ b/packages/v-connection-string/test/parse.js
@@ -247,9 +247,9 @@ describe('parse', function () {
   })
 
   it('configuration parameter tls_mode=no-verify', function () {
-    var connectionString = 'vertica:///?tls_mode=no-verify' // not a supported tls_mode, should instead default to disable
+    var connectionString = 'vertica:///?tls_mode=no-verify' // not a supported tls_mode, should instead default to prefer
     var subject = parse(connectionString)
-    subject.tls_mode.should.eql('disable')
+    subject.tls_mode.should.eql('prefer')
   })
 
   it('configuration parameter tls_mode=verify-ca', function () {
diff --git a/packages/vertica-nodejs/README.md b/packages/vertica-nodejs/README.md
index fd51edd1..aecbeddf 100644
--- a/packages/vertica-nodejs/README.md
+++ b/packages/vertica-nodejs/README.md
@@ -266,11 +266,11 @@ The `client_label` connection property is a string that sets a label for the con
 
 Current TLS Support in vertica-nodejs is limited to server modes that does not require the client to present a certificate. mTLS will be supported in a future version of vertica-nodejs. 
 
-Valid values for the `tls_mode` connection property are `disable`, `require` which will ensure the connection is encrypted, `verify-ca` which ensures the connection is encrypted and the client trusts the server certificate, and `verify-full` which ensures the connection is encrypted, the client trusts the server certificate, and the server hostname has been verified to match the provided server certificate. 
+Valid values for the `tls_mode` connection property are `disable`, `prefer`, `require` which will ensure the connection is encrypted, `verify-ca` which ensures the connection is encrypted and the client trusts the server certificate, and `verify-full` which ensures the connection is encrypted, the client trusts the server certificate, and the server hostname has been verified to match the provided server certificate. 
 
 ### TLS Connection Properties
 
-The `tls_mode` connection property is a string that determines the mode of tls the client will attempt to use. By default it is `disable`. Other valid values are described in the above section.
+The `tls_mode` connection property is a string that determines the mode of tls the client will attempt to use. By default it is `prefer`. Other valid values are described in the above section.
 
 The `tls_trusted_certs` connection property is an optional override of the trusted CA certificates. `tls_trusted_certs` is a path to the .pem file being used to override defaults. The default is based on the node.js tls module which defaults to well-known CAs curated by Mozilla.
 
diff --git a/packages/vertica-nodejs/lib/client.js b/packages/vertica-nodejs/lib/client.js
index b895f902..63d6819f 100644
--- a/packages/vertica-nodejs/lib/client.js
+++ b/packages/vertica-nodejs/lib/client.js
@@ -81,7 +81,7 @@ class Client extends EventEmitter {
     this.processID = null
     this.secretKey = null
     this.tls_config = this.connectionParameters.tls_config
-    this.tls_mode = this.connectionParameters.tls_mode || 'disable'
+    this.tls_mode = this.connectionParameters.tls_mode || 'prefer'
     this.tls_trusted_certs = this.connectionParameters.tls_trusted_certs
     this._connectionTimeoutMillis = c.connectionTimeoutMillis || 0
     this.workload = this.connectionParameters.workload
diff --git a/packages/vertica-nodejs/lib/connection.js b/packages/vertica-nodejs/lib/connection.js
index 4152bd47..6153c23e 100644
--- a/packages/vertica-nodejs/lib/connection.js
+++ b/packages/vertica-nodejs/lib/connection.js
@@ -19,6 +19,7 @@ var fs = require('fs')
 var EventEmitter = require('events').EventEmitter
 
 const { parse, serialize } = require('v-protocol')
+const { error } = require('console')
 
 const flushBuffer = serialize.flush()
 const syncBuffer = serialize.sync()
@@ -46,9 +47,9 @@ class Connection extends EventEmitter {
     this.tls_config = config.tls_config
 
     if (this.tls_config === undefined) {
-      this.tls_mode = config.tls_mode || 'disable'
-      //this.tls_client_key = config.tls_client_key
-      //this.tls_client_cert = config.tls_client_cert
+      this.tls_mode = config.tls_mode || 'prefer'
+      // this.tls_client_key = config.tls_client_key
+      // this.tls_client_cert = config.tls_client_cert
       this.tls_trusted_certs = config.tls_trusted_certs
       this.tls_host = config.tls_host
     }
@@ -100,8 +101,15 @@ class Connection extends EventEmitter {
         case 'S': // Server supports TLS connections, continue with a secure connection
           break
         case 'N': // Server does not support TLS connections
-          self.stream.end()
-          return self.emit('error', new Error('The server does not support TLS connections'))
+          if (self.tls_mode == 'prefer') {
+            self.attachListeners(self.stream)
+            self.emit('sslconnect')
+            return
+          }
+          else {
+            self.stream.end()
+            return self.emit('error', new Error('The server does not support TLS connections'))
+          }
         default:
           // Any other response byte, including 'E' (ErrorResponse) indicating a server error
           self.stream.end()
@@ -128,7 +136,7 @@ class Connection extends EventEmitter {
         // With an undefined checkServerIdentity function, we are still checking to see that the server
         // certificate is signed by the CA (default or provided).
         
-        if (self.tls_mode === 'require') { // basic TLS connection, does not verify CA certificate
+        if (self.tls_mode === 'require' || self.tls_mode === 'prefer') { // basic TLS connection, does not verify CA certificate
           tls_options.rejectUnauthorized = false
           tls_options.checkServerIdentity = (host , cert) => undefined
           if (self.tls_trusted_certs) {
diff --git a/packages/vertica-nodejs/lib/defaults.js b/packages/vertica-nodejs/lib/defaults.js
index 63a43998..c5f27f93 100644
--- a/packages/vertica-nodejs/lib/defaults.js
+++ b/packages/vertica-nodejs/lib/defaults.js
@@ -55,7 +55,7 @@ module.exports = {
   // from the pool and destroyed
   idleTimeoutMillis: 30000,
   client_encoding: '',
-  tls_mode: 'disable',
+  tls_mode: 'prefer',
   tls_key_file: undefined,
   tls_cert_file: undefined,
   options: undefined,
diff --git a/packages/vertica-nodejs/test/integration/client/configuration-tests.js b/packages/vertica-nodejs/test/integration/client/configuration-tests.js
index 7b4ae8e9..4d3652ed 100644
--- a/packages/vertica-nodejs/test/integration/client/configuration-tests.js
+++ b/packages/vertica-nodejs/test/integration/client/configuration-tests.js
@@ -24,7 +24,7 @@ suite.test('default values are used in new clients', function () {
     binary: false,
     idleTimeoutMillis: 30000,
     client_encoding: '',
-    tls_mode: 'disable',
+    tls_mode: 'prefer',
     parseInputDatesAsUTC: false,
   })
 
diff --git a/packages/vertica-nodejs/test/integration/connection/test-helper.js b/packages/vertica-nodejs/test/integration/connection/test-helper.js
index 22bc7a89..030d3f8b 100644
--- a/packages/vertica-nodejs/test/integration/connection/test-helper.js
+++ b/packages/vertica-nodejs/test/integration/connection/test-helper.js
@@ -6,7 +6,7 @@ var utils = require('../../../lib/utils')
 var connect = function (callback) {
   var username = helper.args.user
   var database = helper.args.database
-  var con = new Connection({ stream: new net.Stream() })
+  var con = new Connection({ stream: new net.Stream(), tls_mode: 'disable' })
   con.on('error', function (error) {
     console.log(error)
     throw new Error('Connection error')
diff --git a/packages/vertica-nodejs/test/integration/connection/tls-tests.js b/packages/vertica-nodejs/test/integration/connection/tls-tests.js
index d3bbde0d..3f07b27a 100644
--- a/packages/vertica-nodejs/test/integration/connection/tls-tests.js
+++ b/packages/vertica-nodejs/test/integration/connection/tls-tests.js
@@ -1,4 +1,5 @@
 'use strict'
+const { error } = require('console')
 var helper = require('./../test-helper')
 var vertica = helper.vertica
 
@@ -37,8 +38,8 @@ const client_key_path     =  __dirname + '/../../tls/client_key.pem'
 // all connections from the client, the caveat being that for try_verify and verify_ca it's possible
 // for the connection to be plaintext if the client doesn't present valid credentials. 
 suite.test('vertica tls - disable mode - all', function () {
-  var client = new vertica.Client() // 'disable' by default, so no need to pass in that option
-  assert.equal(client.tls_mode, vertica.defaults.tls_mode)
+  var client = new vertica.Client({tls_mode: 'disable'})
+  assert.equal(client.tls_mode, 'disable')
   client.connect(err => {
     if (err) {
       // shouldn't fail to connect
@@ -58,6 +59,36 @@ suite.test('vertica tls - disable mode - all', function () {
   })
 })
 
+// Test case for tls_mode = 'prefer' as default
+// The client will attempt to establish a TLS connection if the server supports it.
+// If the server does not support TLS, the client will still connect using a plaintext connection.
+// This test verifies that in 'prefer' mode, the client connects successfully.
+suite.test('vertica tls - prefer mode', function () {
+  var client = new vertica.Client() // 'prefer' by default, so no need to pass in that option
+  assert.equal(client.tls_mode, vertica.defaults.tls_mode)
+  client.connect(err => {
+    if (err) {
+      // shouldn't fail to connect
+      assert(false)      
+    }
+    // If connection succeeds, check for TLS connection
+    client.query("SELECT mode FROM tls_configurations where name = 'server' LIMIT 1", (err, res) => {
+      if (err) {
+        console.log(err)
+        assert(false)
+      }
+      // Assert only if server supports TLS
+      if (['ENABLE', 'TRY_VERIFY', 'VERIFY_CA', 'VERIFY_FULL'].includes(res.rows[0].mode)) {
+        assert(client.connection.stream.constructor.name.toString(), "TLSSocket")
+      }
+      else {
+        assert.notEqual(client.connection.stream.constructor.name.toString(), "TLSSocket")
+      }
+      client.end()
+    })
+  })
+})
+
 // Test case for tls_mode = 'require'
 // The server will not accept all connections from the client with the client in 'require' mode. The server
 // will reject a connection in DISABLE mode for obvious reasons (client requiring TLS + server disallowing TLS)
@@ -96,7 +127,8 @@ suite.test('vertica tls - verify-ca - no tls_cert_file specified', function () {
     if (err) {
       assert(err.message.includes("verify-ca mode requires setting tls_trusted_certs property") // we didn't set the property, this is ok
           || err.message.includes("SSL alert number 40") // VERIFY_CA mode, this is ok
-          || err.message.includes("The server does not support TLS connections")) // DISABLE mode, this is ok
+          || err.message.includes("The server does not support TLS connections") // DISABLE mode, this is ok
+          || err.message.includes("unable to verify the first certificate"))
     }
     client.end()
   })
@@ -115,7 +147,8 @@ suite.test('vertica tls - verify-ca - valid server certificate', function () {
   client.connect(err => {
     if (err) {
       assert(err.message.includes("SSL alert number 40") // VERIFY_CA mode, this is ok
-          || err.message.includes("The server does not support TLS connections")) // DISABLE mode, this is ok
+          || err.message.includes("The server does not support TLS connections") // DISABLE mode, this is ok
+          || err.message.includes("unable to verify the first certificate"))
       return
     }
     assert.equal(client.connection.stream.constructor.name.toString(), "TLSSocket")
@@ -141,7 +174,8 @@ suite.test('vertica tls - verify-full - no tls_cert_file specified', function ()
     if (err) {
       assert(err.message.includes("verify-ca mode requires setting tls_trusted_certs property") // we didn't set the property, this is ok
           || err.message.includes("SSL alert number 40") // VERIFY_CA mode, this is ok
-          || err.message.includes("The server does not support TLS connections")) // DISABLE mode, this is ok
+          || err.message.includes("The server does not support TLS connections") // DISABLE mode, this is ok
+          || err.message.includes("unable to verify the first certificate"))
     }
     client.end()
   })
@@ -159,7 +193,8 @@ suite.test('vertica tls - verify-full - valid server certificate', function () {
   client.connect(err => {
     if (err) {
       assert(err.message.includes("SSL alert number 40") // VERIFY_CA mode, this is ok
-          || err.message.includes("The server does not support TLS connections")) // DISABLE mode, this is ok
+          || err.message.includes("The server does not support TLS connections") // DISABLE mode, this is ok
+          || err.message.includes("unable to verify the first certificate"))
       return
     }
     assert.equal(client.connection.stream.constructor.name.toString(), "TLSSocket")
@@ -183,7 +218,8 @@ suite.test('vertica tls - tls_config feature', function() {
   client.connect(err => {
     if (err) {
       assert(err.message.includes("SSL alert number 40") // VERIFY_CA mode, this is ok
-          || err.message.includes("The server does not support TLS connections")) // DISABLE mode, this is ok
+          || err.message.includes("The server does not support TLS connections") // DISABLE mode, this is ok
+          || err.message.includes("unable to verify the first certificate"))
       return
     }
     // this is how we can tell we actually used tls_config and created a tls socket
diff --git a/packages/vertica-nodejs/test/integration/gh-issues/2307-tests.js b/packages/vertica-nodejs/test/integration/gh-issues/2307-tests.js
index 7adf1ffd..7e5ab309 100644
--- a/packages/vertica-nodejs/test/integration/gh-issues/2307-tests.js
+++ b/packages/vertica-nodejs/test/integration/gh-issues/2307-tests.js
@@ -1,3 +1,7 @@
+//Should we remove this file as its a redundant test for TLS mode?
+//All TLS test suites for various TLS modes can be found at vertica-nodejs/test/integration/connection/tls-tests.js
+
+
 'use strict'
 
 const vertica = require('../../../lib')
diff --git a/packages/vertica-nodejs/test/unit/client/configuration-tests.js b/packages/vertica-nodejs/test/unit/client/configuration-tests.js
index 47eb7487..3796fdb7 100644
--- a/packages/vertica-nodejs/test/unit/client/configuration-tests.js
+++ b/packages/vertica-nodejs/test/unit/client/configuration-tests.js
@@ -12,7 +12,7 @@ test('client settings', function () {
     assert.equal(client.user, pguser)
     assert.equal(client.database, pgdatabase)
     assert.equal(client.port, pgport)
-    assert.equal(client.tls_mode, 'disable')
+    assert.equal(client.tls_mode, 'prefer')
   })
 
   test('custom', function () {
diff --git a/packages/vertica-nodejs/test/unit/connection-parameters/creation-tests.js b/packages/vertica-nodejs/test/unit/connection-parameters/creation-tests.js
index 51b4b7dd..29b1d07f 100644
--- a/packages/vertica-nodejs/test/unit/connection-parameters/creation-tests.js
+++ b/packages/vertica-nodejs/test/unit/connection-parameters/creation-tests.js
@@ -102,10 +102,10 @@ suite.test('ConnectionParameters initializing from config and config.connectionS
   })
   var subject4 = new ConnectionParameters({
     connectionString: 'vertica://test@host/db?tls_mode=require', // connection string has preference
-    tls_mode: 'disable',
+    tls_mode: 'require',
   })
 
-  assert.equal(subject1.tls_mode, 'disable')
+  assert.equal(subject1.tls_mode, 'prefer')
   assert.equal(subject2.tls_mode, 'require')
   assert.equal(subject3.tls_mode, 'require')
   assert.equal(subject4.tls_mode, 'require')
diff --git a/packages/vertica-nodejs/test/unit/connection-parameters/environment-variable-tests.js b/packages/vertica-nodejs/test/unit/connection-parameters/environment-variable-tests.js
index 52802a5d..bea3b30f 100644
--- a/packages/vertica-nodejs/test/unit/connection-parameters/environment-variable-tests.js
+++ b/packages/vertica-nodejs/test/unit/connection-parameters/environment-variable-tests.js
@@ -83,17 +83,17 @@ suite.test('connection string parsing - tls_mode', function () {
 
   string = 'vertica://brian:pw@boom:381/lala'
   subject = new ConnectionParameters(string)
-  assert.equal(subject.tls_mode, 'disable')
+  assert.equal(subject.tls_mode, 'prefer')
 
   string = 'vertica://brian:pw@boom:381/lala?tls_mode=verify-ca'
   subject = new ConnectionParameters(string)
   assert.equal(subject.tls_mode, 'verify-ca')
 })
 
-suite.test('tls mode is disable by default', function () {
+suite.test('tls mode is prefer by default', function () {
   clearEnv()
   var subject = new ConnectionParameters()
-  assert.equal(subject.tls_mode, 'disable')
+  assert.equal(subject.tls_mode, 'prefer')
 })
 
 // restore process.env
diff --git a/packages/vertica-nodejs/test/unit/connection/inbound-parser-tests.js b/packages/vertica-nodejs/test/unit/connection/inbound-parser-tests.js
index 3bf52099..83402c4e 100644
--- a/packages/vertica-nodejs/test/unit/connection/inbound-parser-tests.js
+++ b/packages/vertica-nodejs/test/unit/connection/inbound-parser-tests.js
@@ -121,6 +121,7 @@ var testForMessage = function (buffer, expectedMessage) {
     var stream = new MemoryStream()
     var client = new Connection({
       stream: stream,
+      tls_mode: 'disable'
     })
     client.connect()
 
@@ -379,6 +380,7 @@ test('split buffer, single message parsing', function () {
   var stream = new MemoryStream()
   var client = new Connection({
     stream: stream,
+    tls_mode: 'disable'
   })
   client.connect()
   var message = null
@@ -437,6 +439,7 @@ test('split buffer, multiple message parsing', function () {
   var stream = new MemoryStream()
   var client = new Connection({
     stream: stream,
+    tls_mode: 'disable'
   })
   client.connect()
   client.on('message', function (msg) {