Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 13 commits into from
Oct 23, 2024
6 changes: 3 additions & 3 deletions packages/v-connection-string/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(':')
Expand Down
4 changes: 2 additions & 2 deletions packages/v-connection-string/test/parse.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down
4 changes: 2 additions & 2 deletions packages/vertica-nodejs/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
2 changes: 1 addition & 1 deletion packages/vertica-nodejs/lib/client.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
38 changes: 32 additions & 6 deletions packages/vertica-nodejs/lib/connection.js
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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
tanvipise marked this conversation as resolved.
Show resolved Hide resolved
this.tls_client_cert = config.tls_client_cert
this.tls_trusted_certs = config.tls_trusted_certs
this.tls_host = config.tls_host
}
Expand Down Expand Up @@ -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')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the management of our connection state events, 'sslconnect' is the proper event to emit. This may seem odd because we are not actually connecting with SSL wrapped socket in this case, however the sslconnect event just symbolizes that we completed our attempt at an ssl connection.

So this code and this emission of 'sslconnect' is correct. Just wanted to add some context

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()
Expand All @@ -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) {
Expand All @@ -146,6 +154,24 @@ class Connection extends EventEmitter {
return self.emit('error', err)
}
}
// else if (self.tls_mode === 'prefer') { // basic TLS connection, does not verify CA certificate
tanvipise marked this conversation as resolved.
Show resolved Hide resolved
// tls_options.rejectUnauthorized = false
// tls_options.checkServerIdentity = (host , cert) => undefined
// if (self.tls_trusted_certs) {
// tls_options.ca = fs.readFileSync(self.tls_trusted_certs).toString()
// }
// /*if (self.tls_client_cert) {// the client won't know whether or not this is required, depends on server mode
// tls_options.cert = fs.readFileSync(self.tls_client_cert).toString()
// }
// if (self.tls_client_key) {
// tls_options.key = fs.readFileSync(self.tls_client_key).toString()
// }*/
// try {
// self.stream = tls.connect(tls_options);
// } catch (err) {
// return self.emit('error', err)
// }
// }
else if (self.tls_mode === 'verify-ca') { //verify that the server certificate is signed by a trusted CA
try {
tls_options.rejectUnauthorized = true
Expand Down
2 changes: 1 addition & 1 deletion packages/vertica-nodejs/lib/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
})

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down
50 changes: 43 additions & 7 deletions packages/vertica-nodejs/test/integration/connection/tls-tests.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
'use strict'
const { error } = require('console')
var helper = require('./../test-helper')
var vertica = helper.vertica

Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand Down Expand Up @@ -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()
})
Expand All @@ -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")
Expand All @@ -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()
})
Expand All @@ -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")
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
//Should we remove this file as its a redundant test for TLS mode?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is reasonable to move this test for bad credentials to tls-tests.js.

Additionally, I would be partial to not only moving this test into tls-tests.js, but also moving tls-tests.js into mochatest/integration/connection (currently only client directory exists so we would add connection).

This is surely not any sort of requirement for this work so if you would prefer to leave things as they are, that is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can open up another PR later for moving the tests and doing some more cleanup.

//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')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ require('./test-helper')
const BufferList = require('../../buffer-list')
var Connection = require('../../../lib/connection')
var buffers = require('../../test-buffers')
const { tls_mode } = require('../../../lib/defaults')
tanvipise marked this conversation as resolved.
Show resolved Hide resolved
var PARSE = function (buffer) {
return new Parser(buffer).parse()
}
Expand Down Expand Up @@ -121,6 +122,7 @@ var testForMessage = function (buffer, expectedMessage) {
var stream = new MemoryStream()
var client = new Connection({
stream: stream,
tls_mode: 'disable'
})
client.connect()

Expand Down Expand Up @@ -379,6 +381,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
Expand Down Expand Up @@ -437,6 +440,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) {
Expand Down