Fix HTTP TLS client certificate verification #551

dliappis · 2018-08-15T16:29:38Z

Currently Rally fails with "bad certificate" when Elasticsearch
requires verification of client certificates.

Fix bug, add tests and update documentation with more
examples.

Closes #550

Currently Rally fails with "bad certificate" when Elasticsearch requires verification of client certificates. Fix bug, add tests and update documentation with more examples.

rdiez-stratio · 2018-08-16T15:02:51Z

Hi,

In the case ES is configured to require client certificate (mutual-tls) and I run rally with "verify_certs:false" (instead of true) the code will enter by this line and the client certificate and key will never be loaded in the ssl_context

Is it right? In my opinion even if rally(ES client) is not verifying ES(server) certificate, it should be able to connect using client certificate and key.

Ramón

dliappis · 2018-08-16T16:12:23Z

Hey @rdiez-stratio,

I see what you mean. Yes what you are suggesting wouldn't work with this PR. It could be implemented by setting ssl.create_default_context(ssl.Purpose.CLIENT_AUTH) and additionally loading the cert chain (context.load_cert_chain) for this special case.

Practically speaking, is it a common use case to use unverified HTTPS from Rally to ES but have ES request verification from the client? I mean, again practically speaking, the only thing required for the verification of the server is to just pass the CA crt (if the server certificate has been created using a private CA), which shouldn't be something sensitive.

Not verifying the server cert when the client initiates the connection is an insecure configuration, so that's why I am a bit ambivalent about supporting it, esp since supplying the ca is so simple, but I will think about it a bit more.

As discussed in elastic#551 (comment), decouple verification of server certs from having the ability to present client certificates. Also enhance and improve unit tests and adjust docs.

dliappis · 2018-08-17T08:28:37Z

@rdiez-stratio with the latest commit 8607190, I have tried to decouple the logic, as discussed earlier, You should be able to present the client certs, regardless of the value of verify_certs. Feedback welcome.

rdiez-stratio · 2018-08-17T11:30:08Z

@dliappis really appreciate the change. thanks.

danielmitterdorfer · 2018-08-20T10:55:45Z

docs/command_line_reference.rst

+**TLS/SSL Certificate Verification**
+
+Server certificate verification is controlled with the ``verify_certs`` boolean. To disable use ``verify_certs:false``.
+If ``verify_certs:true``, Rally will attempt to verify the certificates of Elasticsearch. If they are private certificates, you will also need to supply the private CA certificate using ``ca_certs:'/path/to/cacert.pem'``.


Suggestion: "Rally will attempt to verify the certificate provided by Elasticsearch"?

danielmitterdorfer · 2018-08-20T10:57:46Z

docs/command_line_reference.rst

+
+**TLS/SSL Examples**
+
+* Enable SSL, verify server certificates using public certificate: ``--client-options="use_ssl:true,verify_certs:true"``. Note that you don't need to set ``ca_cert`` (which defines the path to the root certificates). Rally does this automatically for you.


Should we say "public CA" here instead of "public certificate"?

danielmitterdorfer · 2018-08-20T11:03:23Z

esrally/client.py

+            client_cert = self.client_options.pop("client_cert", False)
+            client_key = self.client_options.pop("client_key", False)
+
+            if bool(client_cert) != bool(client_key):


Why are you converting to a bool here? Can't we just avoid the default value alltogether and check whether both are not None?

This is a XOR, as this section needs to execute when one of the settings has been set and the other hasn't, which is something we don't allow. It could be moved to utils/opts.py?

danielmitterdorfer · 2018-08-20T11:04:35Z

esrally/client.py

            self.logger.info("SSL support: on")
            self.client_options["scheme"] = "https"

-            self.ssl_context = create_ssl_context(cafile=self.client_options.pop("ca_certs", certifi.where()))
+            self.ssl_context = ssl.create_default_context(ssl.Purpose.CLIENT_AUTH, cafile=self.client_options.pop("ca_certs", certifi.where()))


Why is the SSL context created differently now? Is this to avoid the import on elasticsearch.connection? Also, is it correct to use the SSL purpose CLIENT_AUTH in all cases?

This is required to enable client authentication, if requested by the server.

Using ssl.create_default_context, because the one in elasticsearch-py is a simple wrapper for it, but only passes kwargs and won't pass ssl.Purpose.CLIENT_AUTH.

danielmitterdorfer · 2018-08-20T11:05:36Z

esrally/client.py

                self.ssl_context.verify_mode = ssl.CERT_NONE
+                self.ssl_context.check_hostname = False


Why is the order changed here?

This is mandatory. When specifying check_hostname the verify_mode needs to be set already otherwise:

ValueError: check_hostname needs a SSL context with either CERT_OPTIONAL or CERT_REQUIRED

danielmitterdorfer · 2018-08-20T11:08:00Z

esrally/client.py

+                self.logger.error(
+                    "Supplied client-options contain only one of client_cert/client_key. "
+                    "If your Elasticsearch setup requires client certificate verification both need to be supplied. "
+                    "See https://esrally.readthedocs.io/en/stable/command_line_reference.html?highlight=client_options#id2"


I am not sure we should use links directly here? If we do this I think we should use our DOC_LINK variable (in esrally/__init__.py). Also, shouldn't we present this to the user on the command line instead of just logging it?

We are already doing it here so I just copied this behavior.

Should it go the the exception following a few lines after then?

danielmitterdorfer · 2018-08-20T11:33:26Z

esrally/client.py

+                )
+                defined_client_ssl_option = "client_key" if client_key else "client_cert"
+                missing_client_ssl_option = "client_cert" if client_key else "client_key"
+                raise exceptions.InvalidSyntax("'{}' is missing from client-options but '{}' "


Admittedly, there are no hard definitions which exception is used for what purpose but I tend to use InvalidSyntax only for errors in files like tracks or car definitions, not so much with command line parameters. I'd suggest we use SystemSetupError.

danielmitterdorfer · 2018-08-20T12:28:27Z

esrally/client.py

+                                               "has been specified".format(defined_client_ssl_option,
+                                                                           missing_client_ssl_option))
+            elif client_cert and client_key:
+                self.logger.info("SSL client authentication: on")


Can we please also add a log statement when this feature is turned off (we do that for the other cases as well)?

We can, but I think it would be a bit weird to warn about something when no corresponding arguments have been passed. I mean this would appear when client_cert + client_key haven't been passed, so, maybe it would be too much info for the user to see that a certain feature hasn't been enabled when the arguments influencing it haven't been passed to begin with?

danielmitterdorfer · 2018-08-20T12:29:03Z

tests/client_test.py

        hosts = [{"host": "127.0.0.1", "port": 9200}]
        client_options = {
            "use_ssl": True,
            "verify_certs": True,
            "http_auth": ("user", "password")
        }
        # make a copy so we can verify later that the factory did not modify it
-        original_client_options = dict(client_options)
+        original_client_options = deepcopy(client_options)


Great catch!

danielmitterdorfer · 2018-08-20T12:30:42Z

docs/command_line_reference.rst

+
+**TLS/SSL Certificate Verification**
+
+Server certificate verification is controlled with the ``verify_certs`` boolean. To disable use ``verify_certs:false``.


Maybe mention that it is turned on by default?

Great point, indeed.

dliappis · 2018-08-21T10:32:14Z

@danielmitterdorfer I tried to address all the comments, would appreciate another pass when you have time.

danielmitterdorfer

Thanks for the iteration. I think we're almost there. I left one more comment.

danielmitterdorfer · 2018-08-21T11:20:57Z

esrally/client.py

+                console.println(
+                    "'{}' is missing from client-options but '{}' has been specified.\n"
+                    "If your Elasticsearch setup requires client certificate verification both need to be supplied.\n"
+                    "Read the documentation at {}//command_line_reference.html?highlight=client_options#id2\n".format(


Is the double slash intentional? Also, I'd rather use a more stable on-page anchor than #id2?

Ugh sure it's by accident. Ditched the highlight and left the pure link in 4e7f434

danielmitterdorfer

Thanks @dliappis! LGTM

rljie · 2019-09-02T09:50:44Z

我的ES是7.2.0版本，使用了xpack加密，使用下面这条命令执行esrally：
esrally --advanced-config --target-hosts=12.19.24.92:9200 --pipeline=benchmark-only --client-options="basic_auth_user:'elastic',basic_auth_password:'123456',use_ssl:true,verify_certs:true,ca_certs:'/root/config/client-ca.cer',client_cert:'/root/config/client.cer',client_key:'/root/config/client.key',timeout:120" --test-mode

报错：
[ERROR] Cannot race. Error in race control (('Could not connect to your Elasticsearch metrics store. Please check that it is running on host [https://12.19.24.92] at port [9200] or fix the configuration in [/root/.rally/rally.ini].', None))

Fix HTTP TLS client certificate verification

32f016b

Currently Rally fails with "bad certificate" when Elasticsearch requires verification of client certificates. Fix bug, add tests and update documentation with more examples.

dliappis requested a review from danielmitterdorfer August 15, 2018 16:29

dliappis added bug Something's wrong :Docs Changes to the documentation :Config Config file format changes, new properties, ... labels Aug 15, 2018

dliappis added this to the 1.0.1 milestone Aug 15, 2018

dliappis removed the :Docs Changes to the documentation label Aug 15, 2018

Decouple verification of server certs from client certs

8607190

As discussed in elastic#551 (comment), decouple verification of server certs from having the ability to present client certificates. Also enhance and improve unit tests and adjust docs.

danielmitterdorfer requested changes Aug 20, 2018

View reviewed changes

Addressing PR comments

a30296d

dliappis force-pushed the fix-verification-of-client-tls-certs branch from 5320f66 to a30296d Compare August 21, 2018 10:31

danielmitterdorfer requested changes Aug 21, 2018

View reviewed changes

Fix url in PR comments

4e7f434

danielmitterdorfer approved these changes Aug 21, 2018

View reviewed changes

dliappis merged commit 31ac8b0 into elastic:master Aug 21, 2018

dliappis deleted the fix-verification-of-client-tls-certs branch August 21, 2018 14:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix HTTP TLS client certificate verification #551

Fix HTTP TLS client certificate verification #551

dliappis commented Aug 15, 2018

rdiez-stratio commented Aug 16, 2018

dliappis commented Aug 16, 2018 •

edited

Loading

dliappis commented Aug 17, 2018

rdiez-stratio commented Aug 17, 2018

danielmitterdorfer Aug 20, 2018

dliappis Aug 20, 2018

danielmitterdorfer Aug 20, 2018

dliappis Aug 20, 2018

danielmitterdorfer Aug 20, 2018

dliappis Aug 20, 2018

danielmitterdorfer Aug 20, 2018

dliappis Aug 20, 2018

danielmitterdorfer Aug 20, 2018

dliappis Aug 20, 2018

danielmitterdorfer Aug 20, 2018

dliappis Aug 20, 2018

danielmitterdorfer Aug 20, 2018

dliappis Aug 20, 2018

danielmitterdorfer Aug 20, 2018

dliappis Aug 20, 2018

danielmitterdorfer Aug 20, 2018

danielmitterdorfer Aug 20, 2018

dliappis Aug 20, 2018

dliappis commented Aug 21, 2018

danielmitterdorfer left a comment

danielmitterdorfer Aug 21, 2018

dliappis Aug 21, 2018

danielmitterdorfer left a comment

rljie commented Sep 2, 2019


		TLS/SSL Examples

		* Enable SSL, verify server certificates using public certificate: ``--client-options="use_ssl:true,verify_certs:true"``. Note that you don't need to set ``ca_cert`` (which defines the path to the root certificates). Rally does this automatically for you.

		self.ssl_context.verify_mode = ssl.CERT_NONE
		self.ssl_context.check_hostname = False


		TLS/SSL Certificate Verification

		Server certificate verification is controlled with the ``verify_certs`` boolean. To disable use ``verify_certs:false``.

Fix HTTP TLS client certificate verification #551

Fix HTTP TLS client certificate verification #551

Conversation

dliappis commented Aug 15, 2018

rdiez-stratio commented Aug 16, 2018

dliappis commented Aug 16, 2018 • edited Loading

dliappis commented Aug 17, 2018

rdiez-stratio commented Aug 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dliappis commented Aug 21, 2018

danielmitterdorfer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielmitterdorfer left a comment

Choose a reason for hiding this comment

rljie commented Sep 2, 2019

dliappis commented Aug 16, 2018 •

edited

Loading