Skip to content
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion docs/basic-config.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,10 @@ const client = new Client({
|`boolean`, `'proto'`, `'constructor'` - By the default the client will protect you against prototype poisoning attacks. Read https://web.archive.org/web/20200319091159/https://hueniverse.com/square-brackets-are-the-enemy-ff5b9fd8a3e8?gi=184a27ee2a08[this article] to learn more. If needed you can disable prototype poisoning protection entirely or one of the two checks. Read the `secure-json-parse` https://github.com/fastify/secure-json-parse[documentation] to learn more. +
_Default:_ `false`

|`caFingerprint`
Comment thread
delvedor marked this conversation as resolved.
|`string` - If configured, verify the supplied CA certificate fingerprint. +
_Default:_ `null`

|===

[discrete]
Expand All @@ -276,4 +280,3 @@ const client = new Client({
disablePrototypePoisoningProtection: true
})
----

22 changes: 22 additions & 0 deletions docs/connecting.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,28 @@ const client = new Client({
})
----

[discrete]
[[auth-ca-fingerprint]]
==== CA fingerprint

To improve the security of your connection to Elasticsearch, you can provide
Comment thread
delvedor marked this conversation as resolved.
Outdated
a `caFingerprint` option, which will verify the supplied certificate authority fingerprint.

[source,js]
----
const { Client } = require('@elastic/elasticsearch')
const client = new Client({
node: 'https://example.com'
auth: { ... },
// the fingerprint (SHA256) of the CA certificate that is used to sign the certificate that the Elasticsearch node presents for TLS.
caFingerprint: '20:0D:CA:FA:76:...',
ssl: {
// might be required if it's a self-signed certificate

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Most CA certificates are self signed. Well root CAs are, intermediate aren't ( are signed by the root ) but this is not very uncommon

rejectUnauthorized: false
}
})
----


[discrete]
[[client-usage]]
Expand Down
1 change: 1 addition & 0 deletions index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ interface ClientOptions {
password?: string;
};
disablePrototypePoisoningProtection?: boolean | 'proto' | 'constructor';
caFingerprint?: string;
}

declare class Client {
Expand Down
19 changes: 19 additions & 0 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ class Client extends ESAPI {
suggestCompression: false,
compression: false,
ssl: null,
caFingerprint: null,
agent: null,
headers: {},
nodeFilter: null,
Expand All @@ -116,6 +117,10 @@ class Client extends ESAPI {
disablePrototypePoisoningProtection: false
}, opts)

if (options.caFingerprint !== null && isHttpConnection(opts.node || opts.nodes)) {
Comment thread
delvedor marked this conversation as resolved.
throw new ConfigurationError('You can\'t configure the caFingerprint with a http connection')
}

if (process.env.ELASTIC_CLIENT_APIVERSIONING === 'true') {
options.headers = Object.assign({ accept: 'application/vnd.elasticsearch+json; compatible-with=7' }, options.headers)
}
Expand Down Expand Up @@ -146,6 +151,7 @@ class Client extends ESAPI {
Connection: options.Connection,
auth: options.auth,
emit: this[kEventEmitter].emit.bind(this[kEventEmitter]),
caFingerprint: options.caFingerprint,
sniffEnabled: options.sniffInterval !== false ||
options.sniffOnStart !== false ||
options.sniffOnConnectionFault !== false
Expand Down Expand Up @@ -315,6 +321,19 @@ function getAuth (node) {
}
}

function isHttpConnection (node) {
if (Array.isArray(node)) {
for (const n of node) {
if (new URL(n).protocol === 'http:') {
return true
}
}
return false
Comment thread
delvedor marked this conversation as resolved.
Outdated
} else {
return new URL(node).protocol === 'http:'
}
}

const events = {
RESPONSE: 'response',
REQUEST: 'request',
Expand Down
1 change: 1 addition & 0 deletions lib/Connection.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ export interface ConnectionOptions {
roles?: ConnectionRoles;
auth?: BasicAuth | ApiKeyAuth;
proxy?: string | URL;
caFingerprint?: string;
}

interface ConnectionRoles {
Expand Down
39 changes: 39 additions & 0 deletions lib/Connection.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ class Connection {
this.headers = prepareHeaders(opts.headers, opts.auth)
this.deadCount = 0
this.resurrectTimeout = 0
this.caFingerprint = opts.caFingerprint

this._openRequests = 0
this._status = opts.status || Connection.statuses.ALIVE
Expand Down Expand Up @@ -123,10 +124,28 @@ class Connection {
callback(new RequestAbortedError(), null)
}

const onSocket = socket => {
/* istanbul ignore else */
if (!socket.isSessionReused()) {
Comment thread
delvedor marked this conversation as resolved.
socket.once('secureConnect', () => {
// Check if fingerprint matches
/* istanbul ignore else */
if (this.caFingerprint !== getIssuerCertificate(socket).fingerprint256) {
onError(new Error('Fingerprint does not match'))
Comment thread
delvedor marked this conversation as resolved.
Outdated
request.once('error', () => {}) // we need to catch the request aborted error
return request.abort()
}
})
}
}

request.on('response', onResponse)
request.on('timeout', onTimeout)
request.on('error', onError)
request.on('abort', onAbort)
if (this.caFingerprint != null) {
request.on('socket', onSocket)
}

// Disables the Nagle algorithm
request.setNoDelay(true)
Expand All @@ -152,6 +171,7 @@ class Connection {
request.removeListener('timeout', onTimeout)
request.removeListener('error', onError)
request.removeListener('abort', onAbort)
request.removeListener('socket', onSocket)
cleanedListeners = true
}
}
Expand Down Expand Up @@ -340,5 +360,24 @@ function prepareHeaders (headers = {}, auth) {
return headers
}

function getIssuerCertificate (socket) {
let certificate = socket.getPeerCertificate(true)
while (certificate && Object.keys(certificate).length > 0) {
/* istanbul ignore else */
if (certificate.issuerCertificate !== undefined) {
// For self-signed certificates, `issuerCertificate` may be a circular reference.
/* istanbul ignore else */
if (certificate.fingerprint256 === certificate.issuerCertificate.fingerprint256) {
break
}
/* istanbul ignore next */
certificate = certificate.issuerCertificate
} else {
break

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

when issuerCertificate is undefined, the certificate is invalid or malformed, let's not try to use it. Even if we don't care about the standards, then this here is the peer certificate, and not it's issuer's.

@azasypkin azasypkin Aug 2, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

when issuerCertificate is undefined, the certificate is invalid or malformed, let's not try to use it.

Hmm, it seems that missing issuerCertificate in certificate is one thing and missing issuerCertificate property in a certificate parsed by NodeJS is a different thing. If host just doesn't return a full chain, I guess, we'll have an empty issuerCertificate property (or if we pass false to socket.getPeerCertificate).

If ES is configured to not return full chain, do we treat it as a misconfiguration @jkakavas or you meant something else?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

missing issuerCertificate in certificate is one thing and missing issuerCertificate property in a certificate parsed by NodeJS is a different thing

Correct. I focused on the former I guess :/ Still though, we're in a method called getIssuerCertificate and we shouldn't return a certificate just because we couldn't get it's issuer. If ES doesn't return the full chain, then caFingerprint does not make much sense to be set.

@azasypkin azasypkin Aug 2, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Still though, we're in a method called getIssuerCertificate and we shouldn't return a certificate just because we couldn't get it's issuer.

Yep

If ES doesn't return the full chain, then caFingerprint does not make much sense to be set.

Would it make sense to rename it to rootCAFingerprint then? Because we may still have a number intermediate CA certificates in the chain even though root CA certificate isn't there.

And just for my own education, if I issue a Let's Encrypt certificate to use with my ES instance, I still have to configure ES with the full chain, even though root CA certificate is issued by the well-known public CA, right?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Would it make sense to rename it to rootCAFingerprint then? Because we may still have a number intermediate CA certificates in the chain even though root CA certificate isn't there.

Possibly, it makes it clearer I think but we can handle this with documentation also.

And just for my own education, if I issue a Let's Encrypt certificate to use with my ES instance, I still have to configure ES with the full chain, even though root CA certificate is issued by the well-known public CA, right?

Well, we don't support the enrollment process for arbitrary configurations but if you'd want to use let's encrypt and use caFingerprint in a way in your es client, then yes ,you'd need to do that.

Generically, it would make more sense for a client to support leaf certificate pinning ( as in I want to trust this server certificate only ). The idea behind us using CA certificate pinning for the enrollment process is that it allows you to get a single enrollment token that might contain many addresses for all ES nodes in a cluster and you can try/selct any of them and validate the connection with a single fingerprint ( as opposed to one per node )

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, but I had an impression that caFingerprint we've introduced here is not specific to enrollment process otherwise we should have named it differently I guess.

Generically, it would make more sense for a client to support leaf certificate pinning ( as in I want to trust this server certificate only ).

That's actually what I was alluding to initially, pinning to the leaf (this specific server) or intermediate CA (this specific group of servers) seems to be a perfectly valid use case. My concern was that ES seems to not explicitly require full chains and moreover works fine with non-complete chains. That'd be enough for the aforementioned use case, but it won't work if we treat certificate with null/undefined issuerCertificate as invalid (the one that NodeJS parses, aka if (certificate.issuerCertificate == null) return null).

Anyway, PR is merged, so let's if it ever becomes a problem 🤷‍♂️

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

That's actually what I was alluding to initially, pinning to the leaf (this specific server) or intermediate CA (this specific group of servers) seems to be a perfectly valid use case.

Good point, I'd be 👍 This would cover security on by default enrollment process, and more generic cases at the same time. Maybe we can do in a followup ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe we can do in a followup ?

Yeah, I'd 👍 too.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm exactly having this issue. Returned certificate only has issuer not issuerCertificate. It results in error: Unhandled Rejection at: Promise [object Promise] reason ConnectionError: Invalid or malformed certificate. This is in contrast with Python client where ssl_assert_fingerprint parameter works as expected with self-signed certificate that elastic generates by default.

}
}
return certificate
}

module.exports = Connection
module.exports.internals = { prepareHeaders }
3 changes: 3 additions & 0 deletions lib/pool/BaseConnectionPool.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ class BaseConnectionPool {
this._ssl = opts.ssl
this._agent = opts.agent
this._proxy = opts.proxy || null
this._caFingerprint = opts.caFingerprint || null
}

getConnection () {
Expand Down Expand Up @@ -72,6 +73,8 @@ class BaseConnectionPool {
if (opts.agent == null) opts.agent = this._agent
/* istanbul ignore else */
if (opts.proxy == null) opts.proxy = this._proxy
/* istanbul ignore else */
if (opts.caFingerprint == null) opts.caFingerprint = this._caFingerprint

const connection = new this.Connection(opts)

Expand Down
1 change: 1 addition & 0 deletions lib/pool/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ interface BaseConnectionPoolOptions {
auth?: BasicAuth | ApiKeyAuth;
emit: (event: string | symbol, ...args: any[]) => boolean;
Connection: typeof Connection;
caFingerprint?: string;
}

interface ConnectionPoolOptions extends BaseConnectionPoolOptions {
Expand Down
1 change: 1 addition & 0 deletions test/acceptance/sniff.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ test('Should update the connection pool', t => {
t.same(hosts[i], {
url: new URL(nodes[id].url),
id: id,
caFingerprint: null,
roles: {
master: true,
data: true,
Expand Down
86 changes: 86 additions & 0 deletions test/unit/client.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ const intoStream = require('into-stream')
const { ConnectionPool, Transport, Connection, errors } = require('../../index')
const { CloudConnectionPool } = require('../../lib/pool')
const { Client, buildServer } = require('../utils')

let clientVersion = require('../../package.json').version
if (clientVersion.includes('-')) {
clientVersion = clientVersion.slice(0, clientVersion.indexOf('-')) + 'p'
Expand Down Expand Up @@ -1498,3 +1499,88 @@ test('Bearer auth', t => {
})
})
})

test('Check server fingerprint (success)', t => {
t.plan(1)

function handler (req, res) {
res.end('ok')
}

buildServer(handler, { secure: true }, ({ port, caFingerprint }, server) => {
const client = new Client({
node: `https://localhost:${port}`,
caFingerprint
})

client.info((err, res) => {
t.error(err)
server.stop()
})
})
})

test('Check server fingerprint (failure)', t => {
t.plan(2)

function handler (req, res) {
res.end('ok')
}

buildServer(handler, { secure: true }, ({ port }, server) => {
const client = new Client({
node: `https://localhost:${port}`,
caFingerprint: 'FO:OB:AR'
})

client.info((err, res) => {
t.ok(err instanceof errors.ConnectionError)
t.equal(err.message, 'Fingerprint does not match')
server.stop()
})
})
})

test('caFingerprint can\'t be configured over http / 1', t => {
t.plan(2)

try {
new Client({ // eslint-disable-line
node: 'http://localhost:9200',
caFingerprint: 'FO:OB:AR'
})
t.fail('shuld throw')
} catch (err) {
t.ok(err instanceof errors.ConfigurationError)
t.equal(err.message, 'You can\'t configure the caFingerprint with a http connection')
}
})

test('caFingerprint can\'t be configured over http / 2', t => {
t.plan(2)

try {
new Client({ // eslint-disable-line
nodes: ['http://localhost:9200'],
Comment thread
thomheymann marked this conversation as resolved.
caFingerprint: 'FO:OB:AR'
})
t.fail('shuld throw')
Comment thread
delvedor marked this conversation as resolved.
Outdated
} catch (err) {
t.ok(err instanceof errors.ConfigurationError)
t.equal(err.message, 'You can\'t configure the caFingerprint with a http connection')
}
})

test('caFingerprint can\'t be configured over http / 3', t => {
t.plan(1)

try {
new Client({ // eslint-disable-line
nodes: ['https://localhost:9200'],
caFingerprint: 'FO:OB:AR'
})
t.pass('should not throw')
} catch (err) {
t.fail('shuld not throw')
}
})
57 changes: 56 additions & 1 deletion test/unit/connection.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ const hpagent = require('hpagent')
const intoStream = require('into-stream')
const { buildServer } = require('../utils')
const Connection = require('../../lib/Connection')
const { TimeoutError, ConfigurationError, RequestAbortedError } = require('../../lib/errors')
const { TimeoutError, ConfigurationError, RequestAbortedError, ConnectionError } = require('../../lib/errors')

test('Basic (http)', t => {
t.plan(4)
Expand Down Expand Up @@ -947,3 +947,58 @@ test('Abort with a slow body', t => {

setImmediate(() => request.abort())
})

test('Check server fingerprint (success)', t => {
t.plan(2)

function handler (req, res) {
res.end('ok')
}

buildServer(handler, { secure: true }, ({ port, caFingerprint }, server) => {
const connection = new Connection({
url: new URL(`https://localhost:${port}`),
caFingerprint
})

connection.request({
path: '/hello',
method: 'GET'
}, (err, res) => {
t.error(err)

let payload = ''
res.setEncoding('utf8')
res.on('data', chunk => { payload += chunk })
res.on('error', err => t.fail(err))
res.on('end', () => {
t.equal(payload, 'ok')
server.stop()
})
})
})
})

test('Check server fingerprint (failure)', t => {
t.plan(2)

function handler (req, res) {
res.end('ok')
}

buildServer(handler, { secure: true }, ({ port }, server) => {
const connection = new Connection({
url: new URL(`https://localhost:${port}`),
caFingerprint: 'FO:OB:AR'
})

connection.request({
path: '/hello',
method: 'GET'
}, (err, res) => {
t.ok(err instanceof ConnectionError)
t.equal(err.message, 'Fingerprint does not match')
server.stop()
})
})
})
Loading