Support CA fingerprint validation by delvedor · Pull Request #1499 · elastic/elasticsearch-js

delvedor · 2021-07-22T12:27:35Z

As titled.

Usage:

const client = new Client({
  node: 'https://localhost:9200',
  caFingerprint: 'string'
})

TODO:

More unit test
Docs
~~Integration test with Elasticsearch~~

delvedor · 2021-07-22T12:28:33Z

lib/Connection.js

+          if (socket.authorized === false) {
+            onError(new Error(socket.authorizationError))
+            request.once('error', () => {}) // we need to catch the request aborted error
+            return request.abort()
+          }


I’m not entirely sure this is needed, as this check would catch the self-signed certificate.
One of the test is failing because of this check, if we remove it, everything works as expected.

I suspect it's not needed since we're going to validate a fingerprint of the top-most CA certificate, that will most-likely be a self-signed one (from the tech spec The fingerprint (SHA256) of the CA certificate that is used to sign the certificate that the Elasticsearch node presents for TLS. ). @jkakavas is my understanding correct?

@jkakavas is my understanding correct?

Yes. We elected to use the CA certificate for the pinning as it originally felt like it provided for more options. It will be a self signed CA certificate that Elasticsearch generates on startup for security on by default but in general, it is common/fine/expected that CA certificates ( the root ones ) are self-signed

thomheymann

I haven't tested this locally yet but added a few comments based on the code.

One general question I have is how we can avoid the "self signed certificate in certificate chain" error nodejs will throw during interactive setup when connecting to the (at that point) untrusted cert. We can't ask users to set NODE_TLS_REJECT_UNAUTHORIZED=0 and setting ssl.verificationMode=none in the elasticsearch client doesn't seem to prevent it.

lib/Connection.js

test/utils/buildServer.js

lib/Connection.js

azasypkin · 2021-07-26T09:59:55Z

lib/Connection.js

    this.headers = prepareHeaders(opts.headers, opts.auth)
    this.deadCount = 0
    this.resurrectTimeout = 0
+    this.certFingerprint = opts.certFingerprint


question: should we name it caCertFingerprint? I assume we're planning to validate fingerprint of the CA certificate, not the leaf one.

Good call, I would say that caFingerprint is the best, so we avoid the double "certificate" repetition :)

Technically we don't repeat (or rather repeat, but legitimately) since it's Certificate Authority certificate fingerprint, but either works for me 🙂

azasypkin · 2021-07-26T10:03:33Z

lib/Connection.js

+          if (socket.authorized === false) {
+            onError(new Error(socket.authorizationError))
+            request.once('error', () => {}) // we need to catch the request aborted error
+            return request.abort()
+          }


I suspect it's not needed since we're going to validate a fingerprint of the top-most CA certificate, that will most-likely be a self-signed one (from the tech spec The fingerprint (SHA256) of the CA certificate that is used to sign the certificate that the Elasticsearch node presents for TLS. ). @jkakavas is my understanding correct?

lib/Connection.js

docs/connecting.asciidoc

sethmlarson

LGTM!

thomheymann

I've tested this locally and the fingerprint is correctly verified. 🥳

However, this only works when running node with NODE_TLS_REJECT_UNAUTHORIZED=0 flag. Otherwise we don't get to the verification step since node throws an error for self signed certificates.

Asking users to run Kibana with that flag is not a viable option. Appreciate that this is node throwing an error but could we set rejectUnauthorized to false when caFingerprint option is set on the client to avoid this? Is there another option you can think of?

$ node test.js

ERROR ConnectionError: self signed certificate in certificate chain
    at ClientRequest.onError (/Users/thom/Repositories/elasticsearch-js/lib/Connection.js:116:16)
    at ClientRequest.emit (events.js:375:28)
    at TLSSocket.socketErrorListener (_http_client.js:475:9)
    at TLSSocket.emit (events.js:375:28)
    at emitErrorNT (internal/streams/destroy.js:106:8)
    at emitErrorCloseNT (internal/streams/destroy.js:74:3)
    at processTicksAndRejections (internal/process/task_queues.js:82:21)

delvedor · 2021-07-27T09:00:52Z

Appreciate that this is node throwing an error but could we set rejectUnauthorized to false when caFingerprint option is set on the client to avoid this? Is there another option you can think of?

I'm fine to setting rejectUnauthorized by default when caFingerprint is configured, I'll do it :)

thomheymann

As discussed offline, let's leave this as opt in and let the consumer decide whether to disable rejectUnauthorized or not for now.

Otherwise LGTM! Thanks for implementing this so swiftly. 🙏

azasypkin

LGTM from Kibana standpoint, thanks! Just one issue we discussed over Slack: handle/test the case when caFingerprint is provided, but ES client tries to connect to HTTP address.

docs/basic-config.asciidoc

Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>

test/unit/client.test.js

azasypkin

Still LGTM!

index.js

docs/basic-config.asciidoc

Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>

jkakavas · 2021-07-28T20:29:34Z

As discussed offline, let's leave this as opt in and let the consumer decide whether to disable rejectUnauthorized or not for now.

Just validating my understanding that Kibana will set this so that the users won't need to configure something extra. Can this be set per connection and only set it for the initial call to the enroll API ? We wouldn't want to set this globally off for Kibana

jkakavas

Added just a few comments, thanks for tackling this so quickly!

docs/basic-config.asciidoc

jkakavas · 2021-07-28T20:39:53Z

docs/connecting.asciidoc

+  // 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


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

index.js

docs/connecting.asciidoc

jkakavas · 2021-07-28T20:59:16Z

lib/Connection.js

+      /* istanbul ignore next */
+      certificate = certificate.issuerCertificate
+    } else {
+      break


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.

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?

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.

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?

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 )

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 🤷‍♂️

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 ?

Maybe we can do in a followup ?

Yeah, I'd 👍 too.

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.

lib/Connection.js

Co-authored-by: Ioannis Kakavas <ioannis@elastic.co>

thomheymann · 2021-07-29T09:00:51Z

Just validating my understanding that Kibana will set this so that the users won't need to configure something extra. Can this be set per connection and only set it for the initial call to the enroll API ? We wouldn't want to set this globally off for Kibana

Correct, Kibana will disable rejectUnauthorized for the enrolment call so that we can connect to Elasticsearch until we have established trust. During that call we verify the CA using the fingerprint so we can still guarantee that we're speaking to the correct node.

All other requests to Elasticsearch will keep that security check enabled.

jkakavas

LGTM with my latest comments addressed, I don't think this needs another round. Thanks Tomas!

lib/Connection.js

docs/connecting.asciidoc

test/unit/client.test.js

Co-authored-by: Ioannis Kakavas <ioannis@elastic.co>

github-actions · 2021-08-02T09:20:47Z

The backport to 7.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-7.x 7.x
# Navigate to the new working tree
cd .worktrees/backport-7.x
# Create a new branch
git switch --create backport-1499-to-7.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick --mainline 1 2d1505eb2b7ad05387ca8ba8a6874f580a126ecd
# Push it to GitHub
git push --set-upstream origin backport-1499-to-7.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-7.x

Then, create a pull request where the base branch is 7.x and the compare/head branch is backport-1499-to-7.x.

Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com> Co-authored-by: Ioannis Kakavas <ioannis@elastic.co>

delvedor added 2 commits July 22, 2021 14:24

WIP: support CA fingerprint validation

793018d

Updated test

8f60fa0

delvedor added semver-minor backport 7.x labels Jul 22, 2021

delvedor requested review from azasypkin, jkakavas and sethmlarson July 22, 2021 12:27

delvedor commented Jul 22, 2021

View reviewed changes

Merge branch 'master' into ca-fingerprint

407a140

thomheymann reviewed Jul 26, 2021

View reviewed changes

lib/Connection.js Outdated Show resolved Hide resolved

test/utils/buildServer.js Outdated Show resolved Hide resolved

test/utils/buildServer.js Show resolved Hide resolved

azasypkin reviewed Jul 26, 2021

View reviewed changes

lib/Connection.js Show resolved Hide resolved

delvedor added 2 commits July 26, 2021 16:19

Address feedback

ad3b90b

Updated test

d4844f6

delvedor requested review from azasypkin and thomheymann July 26, 2021 14:20

delvedor added 2 commits July 26, 2021 16:35

Moar test

b0f786f

Updated docs

ebbbee8

sethmlarson reviewed Jul 26, 2021

View reviewed changes

docs/connecting.asciidoc Outdated Show resolved Hide resolved

sethmlarson approved these changes Jul 26, 2021

View reviewed changes

Improve example

b7b0545

delvedor marked this pull request as ready for review July 26, 2021 14:56

delvedor added 2 commits July 26, 2021 17:03

Merge branch 'master' into ca-fingerprint

dd08d7e

Typo

d0c1040

thomheymann reviewed Jul 27, 2021

View reviewed changes

thomheymann approved these changes Jul 27, 2021

View reviewed changes

azasypkin approved these changes Jul 27, 2021

View reviewed changes

docs/basic-config.asciidoc Outdated Show resolved Hide resolved

Update docs/basic-config.asciidoc

a5c59db

Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>

delvedor added 2 commits July 27, 2021 14:11

Throw if caFingerprint is configured with http

ff279cc

Updated test

9e1babd

delvedor requested a review from azasypkin July 27, 2021 12:12

delvedor mentioned this pull request Jul 27, 2021

Support CA fingerprint validation elastic/elastic-transport-js#16

Merged

1 task

thomheymann reviewed Jul 27, 2021

View reviewed changes

test/unit/client.test.js Show resolved Hide resolved

thomheymann mentioned this pull request Jul 27, 2021

Interactive setup mode elastic/kibana#106881

Merged

9 tasks

azasypkin approved these changes Jul 28, 2021

View reviewed changes

index.js Outdated Show resolved Hide resolved

docs/basic-config.asciidoc Show resolved Hide resolved

delvedor and others added 2 commits July 28, 2021 16:10

Update index.js

fcf947d

Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>

Updated docs

1cc3281

jkakavas reviewed Jul 28, 2021

View reviewed changes

delvedor and others added 3 commits July 29, 2021 10:25

Update docs/basic-config.asciidoc

33f90b0

Co-authored-by: Ioannis Kakavas <ioannis@elastic.co>

Update docs/connecting.asciidoc

1b69efe

Co-authored-by: Ioannis Kakavas <ioannis@elastic.co>

Update lib/Connection.js

5f85ded

Co-authored-by: Ioannis Kakavas <ioannis@elastic.co>

delvedor added 2 commits July 30, 2021 12:48

Improve getIssuerCertificate utility

a6a6dee

Updated test

d9192e9

delvedor requested a review from jkakavas July 30, 2021 10:48

Nit

208d316

jkakavas approved these changes Jul 30, 2021

View reviewed changes

lib/Connection.js Outdated Show resolved Hide resolved

docs/connecting.asciidoc Outdated Show resolved Hide resolved

test/unit/client.test.js Outdated Show resolved Hide resolved

delvedor and others added 3 commits July 30, 2021 15:45

Update docs/connecting.asciidoc

fecd15b

Co-authored-by: Ioannis Kakavas <ioannis@elastic.co>

Update test/unit/client.test.js

5886718

Co-authored-by: Ioannis Kakavas <ioannis@elastic.co>

Improve getIssuerCertificate utility

b4ea6aa

delvedor merged commit 2d1505e into master Aug 2, 2021

delvedor deleted the ca-fingerprint branch August 2, 2021 09:20

delvedor added a commit that referenced this pull request Aug 2, 2021

Support CA fingerprint validation (#1499)

a48ebc9

Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com> Co-authored-by: Ioannis Kakavas <ioannis@elastic.co>

azasypkin mentioned this pull request Aug 5, 2021

Disable Product check in @elastic/elasticsearch-js elastic/kibana#107642

Merged

1 task

Conversation

delvedor commented Jul 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkakavas Jul 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomheymann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin Jul 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sethmlarson left a comment

Choose a reason for hiding this comment

Uh oh!

thomheymann left a comment

Choose a reason for hiding this comment

Uh oh!

delvedor commented Jul 27, 2021

Uh oh!

thomheymann left a comment

Choose a reason for hiding this comment

Uh oh!

azasypkin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

azasypkin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jkakavas commented Jul 28, 2021

Uh oh!

jkakavas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin Aug 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin Aug 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

delvedor commented Jul 22, 2021 •

edited

Loading

jkakavas Jul 28, 2021 •

edited

Loading

azasypkin Jul 26, 2021 •

edited

Loading

azasypkin Aug 2, 2021 •

edited

Loading

azasypkin Aug 2, 2021 •

edited

Loading

thomheymann commented Jul 29, 2021 •

edited

Loading