Two-way SSL support #99

loganasherjones · 2018-03-21T23:02:55Z

This branch will implement client-side certificate support, so that users running a client-authenticated confluence server can utilize this package.

Here is what is left to be done:

Make pylint work
Add the ability to specify a certificate authority
Allow client certificates to be specified
Add support for encrypted client-side certificates
Documentation
Figure out if I can even test the XML-RPC interface with my local confluence, if so, make it work with XML-RPC interface also

Transport was not defined. I believe it should have been self.xmlrpc_transport and just got missed during a merge. Anyway, this causes the pylint job to build and means that if a user needs to fallback to xml_rpc, it won't break. Signed-off-by: Logan Jones <loganasherjones@gmail.com>

You may now specify confluence_ca_cert in the conf.py If confluence_ssl_disable is set to True, this option is ignored. Pulled out the session setup, because it will get more complicated as we support more SSL options. Signed-off-by: Logan Jones <loganasherjones@gmail.com>

Client certificates can now be provided as either a tuple or a single file. This supports unencrypted and encrypted files, however there is a catch with encrypted files because a password will be prompted if an encrypted cert is provided. Signed-off-by: Logan Jones <loganasherjones@gmail.com>

The SSLError is a subclass of ConnectionError so it needs to come before it in the except block. Before this commit the error message indicated a connection problem but if the error is an SSL error we should not fall back to the XML RPC interface. In addition, we give a more descriptive message about the underlying problem. Signed-off-by: Logan Jones <loganasherjones@gmail.com>

In order to support encrypted certificates, we need to use the TransportAdapter exposed by urllib3. If requests ever adds native support for encrypted keys then we can remove the SSLAdapter and just use the native API. see: psf/requests#2519 for more information. Signed-off-by: Logan Jones <loganasherjones@gmail.com>

loganasherjones · 2018-03-22T01:22:52Z

Okay, I'm now just seeing if I can get RPC stuff working, but I think the Rest Interface is done.

I've tested with the following:

No SSL at all, just user/pass
1-way SSL with CA defined
1-way SSL with ssl_verify_disabled set to True
2-way SSL with encrypted and unencrypted client certificates

Everything that works from the SSL perspective now also works with the XML-RPC interface. Including: * Certificate Authority Specification * Client Certificates Encrypted & Unencrypted Signed-off-by: Logan Jones <loganasherjones@gmail.com>

This commit fixes an error where if a user disabled ssl verification but then provided a client-cert, it would break because you cannot set verify_mode to CERT_NONE if check_hostname is not also disabled. So, this fixes that bug. Signed-off-by: Logan Jones <loganasherjones@gmail.com>

Signed-off-by: Logan Jones <loganasherjones@gmail.com>

This prevents a stacktrace from happening during the sphinx-build and raises a more helpful error message. Signed-off-by: Logan Jones <loganasherjones@gmail.com>

loganasherjones · 2018-03-22T03:50:22Z

Okay, I believe this is now ready to go. Short of any changes you all would like to see. I've tested this with the following:

Test Case	Rest Interface	XML-RPC Interface	Notes
No SSL	✔️	✔️	All SSL info is just ignored
1 way SSL	✔️	✔️	Valid CA provided
1 way SSL - disable CA checking	✔️	✔️	Ignores CA correctly
1 way SSL - Client certs specified	✔️	✔️	No error, just ignores client certs
2 way SSL - Encrypted Client certs	✔️	✔️	Hooray
2 way SSL - Encrypted + ignore CA	✔️	✔️	Also works, though you do get a lot of warnings from requests
2 way SSL - Encrypted Client certs broken apart	✔️	✔️	Checked that `('/path/to/cert.pem', '/path/to/key.pem')` works
2 way SSL - Encrypted Bad CA + ignore	✔️	✔️	Fails as expected
2 way SSL - All the same things + unencrypted	✔️	✔️	Hooray
2 way SSL - Invalid certificate	✔️	✔️	Fails with ssl verification error.
2 way SSL - Bad Password	✔️	✔️	No stacktrace, just an error message.

tonybaloney

Some recommended change to begin with, this absolutely needs complimentary tests as well to keep the coverage

tonybaloney · 2018-03-22T03:48:22Z

doc/configuration.rst

 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

-A boolean value to explicitly disable any verification of SSL certificates when
+A boolean value to explicitly disable verification of sever SSL certificates when


tonybaloney · 2018-03-22T06:04:35Z

sphinxcontrib/confluencebuilder/config.py

 """)

+            if c.confluence_ca_cert:
+                if (not os.path.isfile(c.confluence_ca_cert) and


you don't need parenthesis on this line

Sorry, I'm used to the google style-guide. Would you prefer to see a backslash?

tonybaloney · 2018-03-22T06:05:37Z

sphinxcontrib/confluencebuilder/config.py

+proper file path.
+""")
+
+            if c.confluence_client_cert:


good idea, I like this as an API +1

tonybaloney · 2018-03-22T06:06:40Z

sphinxcontrib/confluencebuilder/config.py

+                    if not os.path.isfile(cert_file):
+                        errState = True
+                        if log:
+                            ConfluenceLogger.error(


Why not raise in this condition instead of having a special error condition and logic flow? If the error is non-recoverable then you should raise the error

I didn’t raise an error for consistencies sake. None of the above error conditions actually raise an error, they just set the errorState to True which in turn raises an error. I assumed this was what Sphinx wanted.

I’d be happy to change it, but I think I should also change all the other error conditions in this file as well.

I think what was submitted is correct (with respect to setting the error flag and logging the error). This is to allow multiple configuration items to be verified (and output at once) before a configuration exception (ConfluenceConfigurationError) is thrown (in the parent class).

tonybaloney · 2018-03-22T06:08:40Z

sphinxcontrib/confluencebuilder/publisher.py

        self.https = (self.scheme == 'https')
        self.proxy = None
        self.timeout = timeout
+        if isinstance(client_cert, tuple) and len(client_cert) >= 2:


this conditional is confusing. Recommend checking for len of client_cert once and once only, refactor this to validate that client_cert is tuple and break into a conditional statement or using a ternary operator.

I did think about this. Do you have a recommended place to format the client cert?

My initial thought was during config validation. But I didn’t think it fit well there from an immutability standpoint.

tonybaloney · 2018-03-22T06:10:18Z

sphinxcontrib/confluencebuilder/publisher.py

+                self._certfile = client_cert[0]
+                self._keyfile = None
+            else:
+                self._certfile = client_cert[0]


I would use native Python unpacking for this, eg. self._certfile, self._keyfile = client_cert

tonybaloney · 2018-03-22T06:10:49Z

sphinxcontrib/confluencebuilder/publisher.py

-                if self.disable_ssl_validation:
-                    import ssl
-                    context = ssl._create_unverified_context()
+                context = self._setup_ssl_context()


there is still a condition where user would want a non-verified SSL connection surely?

This is still supported. If they specify to disable ssl support, we can still return a valid ssl context with check_hostname set to false and the verify_mode set to ssl.CERT_NONE.

tonybaloney · 2018-03-22T06:11:30Z

sphinxcontrib/confluencebuilder/rest.py

+                self._certfile = cert[0]
+                self._keyfile = None
+            else:
+                self._certfile = cert[0]


Use native unpacking

tonybaloney · 2018-03-22T06:13:38Z

@loganasherjones thanks for the contribution! I've made some recommendations about code fixes and logic conditions. This needs unit tests before we can merge it, look at the existing test suite and leverage the Tox runtime to see how things are working now..

loganasherjones

Thanks for the feedback. When I get some time I will write the tests and fix the issues raised.

loganasherjones · 2018-03-22T11:36:09Z

sphinxcontrib/confluencebuilder/config.py

+                    if not os.path.isfile(cert_file):
+                        errState = True
+                        if log:
+                            ConfluenceLogger.error(


I didn’t raise an error for consistencies sake. None of the above error conditions actually raise an error, they just set the errorState to True which in turn raises an error. I assumed this was what Sphinx wanted.

I’d be happy to change it, but I think I should also change all the other error conditions in this file as well.

loganasherjones · 2018-03-22T11:39:57Z

sphinxcontrib/confluencebuilder/publisher.py

        self.https = (self.scheme == 'https')
        self.proxy = None
        self.timeout = timeout
+        if isinstance(client_cert, tuple) and len(client_cert) >= 2:


I did think about this. Do you have a recommended place to format the client cert?

My initial thought was during config validation. But I didn’t think it fit well there from an immutability standpoint.

loganasherjones · 2018-03-22T11:43:40Z

sphinxcontrib/confluencebuilder/publisher.py

-                if self.disable_ssl_validation:
-                    import ssl
-                    context = ssl._create_unverified_context()
+                context = self._setup_ssl_context()


This is still supported. If they specify to disable ssl support, we can still return a valid ssl context with check_hostname set to false and the verify_mode set to ssl.CERT_NONE.

1. Fixed typo for 'server.' 2. Made publisher tests available to be run with client certs. tested on my local confluence, and they ran successfully. 3. Changed the client_cert loading during config validation time. I'm not sold on how I did this because I'm editing the config during the validation step, but it means I can remove a lot of error checking in other places (like the rest client and xml rpc client). 4. Wrote unit tests for the configuration validation changes. Signed-off-by: Logan Jones <loganasherjones@gmail.com>

loganasherjones · 2018-03-23T01:16:11Z

Okay, I've updated with tests for the config portion.

I still have an outstanding question on the parenthesis on line 127 of config. I'm happy to change it to match whatever style you want, if you want a backslash or can recommend a better way, I can change it.

I'm happy to answer more questions in regards to disabling the SSL validation during XML RPC publishing if you have questions.

As for tests for the rest/rpc publishing portion. I added the ability to test these things in the test_publisher.py, however it looks like this script is meant to be used more for integration testing? So, I ran the tests against my local confluence, in No-SSL/1-way SSL/2-way SSL and they all worked as expected. I believe testing these changes on the jdknight confluence page requires configuration changes to the server, but in theory, it could be done. I looked for unit tests for the rest client or publisher, but didn't see any. I'm not sure if you wanted me to create these or if just adding support in the integration tests was enough.

Hopefully this is better!

jdknight

@loganasherjones, thanks! I'm liking this PR so far.

I have noted at least two (or three) minor changes. I put in a slew of optional comments, which you can ignore if you desire (just me being nitpicky).

jdknight · 2018-03-23T00:38:40Z

AUTHORS

 James Knight <james.d.knight@live.com>
 John Teasdale <teasdale@uber.com>
 Thomas Malcher <malcher@student.tugraz.at>
+Logan Jones <loganasherjones@gmail.com>


(optional) I'd prefer if the list of authors was sorted in a lexicographical fashion.

jdknight · 2018-03-23T00:41:09Z

doc/configuration.rst


    confluence_timeout = 10

+confluence_ca_cert


(optional) I'd prefer if the provided configuration options sorted in a lexicographical fashion (even if it separates them) in whatever category they are placed (the category you've chosen works for me).

jdknight · 2018-03-23T00:49:44Z

doc/configuration.rst

+
+Provide a CA certificate to use for server cert authentication. Can either be a 
+file or a path. If it is a path, the directory must have been processed using 
+the c_rehash utility supplied with OpenSSL. By default, verification is turned 


While this directory requirement appears to exists based of requests, does it also apply to the ssl library (when using XML-RPC)? I would suggest, instead of outlining the conditions imposed by the network libraries, just point to their API instead (i.e. indicate that it takes a file or OpenSSL-compatible CA directory and link to both the requests API and ssl API for more information).

jdknight · 2018-03-23T00:54:21Z

doc/configuration.rst

+
+.. code-block:: python
+
+    confluence_ca_cert = '/path/to/ca.crt'


(optional) I'd would be nice to use a more platform independent path in the example (even though this example matches what requests uses). If you like what it's currently set to, that's fine.

jdknight · 2018-03-23T00:57:37Z

sphinxcontrib/confluencebuilder/__init__.py

    app.add_config_value('confluence_publish_prefix', None, False)
    """Timeout for network-related calls (publishing)."""
    app.add_config_value('confluence_timeout', None, False)
+    """File/path to Certificate Authority"""


(optional) I'd prefer if the provided configuration options sorted in a lexicographical fashion (even if it separates them). I would not be opposed if you want to introduce a sub-section (like the documentation) and have category:

"""(advanced configuration - publishing)""" <your-new-options>

jdknight · 2018-03-23T01:15:11Z

sphinxcontrib/confluencebuilder/rest.py

+from .exceptions import ConfluenceCertificateError
 from .std.confluence import API_REST_BIND_PATH

+class SSLAdapter(HTTPAdapter):


(optional) While I know PEP 8 (and maybe Google's Python guidelines) promote "CapWords" style, I'd prefer the convention SslAdapter. If you want to leave it as is or if @tonybaloney prefers the "CapWords" style, that's fine by me.

jdknight · 2018-03-23T01:15:56Z

sphinxcontrib/confluencebuilder/rest.py

+                                    keyfile=self._keyfile,
+                                    password=self._password)
+        except ssl.SSLError as ex:
+            ConfluenceCertificateError(ex)


Isn't this missing raise?

Thanks! Nice catch.

jdknight · 2018-03-23T01:17:11Z

sphinxcontrib/confluencebuilder/rest.py

+        kwargs['ssl_context'] = context
+        return super(SSLAdapter, self).init_poolmanager(*args, **kwargs)
+
+


(trivial) No need for two lines.

jdknight · 2018-03-23T01:20:00Z

test/test_publisher.py

+        self._conf('confluence_server_url',       'CB_URL', DEFAULT_TEST_URL)
+        self._conf('confluence_server_user',      'CB_USR', DEFAULT_TEST_USER)
+        self._conf('confluence_space_name',       'CB_SPC', DEFAULT_TEST_SPACE)
+        self._conf('confluence_ca_cert',          'CB_CA')


(note) Heads up that the test_publisher.py class is about to be re-written. You can leave these changes in if you'd like, but just note these changes may or may not exist in the future (if it matters to you).

jdknight · 2018-03-23T01:23:02Z

test/unit-tests/common/test_config.py

+        builder.config.confluence_publish = False
+        builder.config.confluence_client_cert = None
+        builder.config.confluence_server_url = None
+        builder.config.confluence_space_name = None


Missing trailing line?

1. Fixed lexographical ordering problems. 2. Fixed whitespacing issues. 3. Fixed missing raise. 4. Refactored ca_cert path checking. 5. Updated docs to link to relevant ssl/requests sections. Signed-off-by: Logan Jones <loganasherjones@gmail.com>

loganasherjones · 2018-03-23T01:57:17Z

Okie dokie. I think feedback from both @tonybaloney and @jdknight have been implemented. Let me know if there is anything else!

jdknight · 2018-03-23T02:01:28Z

@loganasherjones, much appreciated. Everything looks good for me (unless I'm overlooking something 😅). I'll let @tonybaloney drive it from here, unless he states otherwise.

Edit:
I also mentioned before that I wanted to hold off this commit till next milestone. I actually don't mind this getting pushed in ahead of time. I'll sync some stuff and begin my validation tests.

tonybaloney · 2018-03-23T02:02:35Z

Yep, it's a 👍 from me.

Great work. Merging

loganasherjones · 2018-03-23T02:03:08Z

Thanks for all the help! Looking forward to the next release :)

loganasherjones added 4 commits March 21, 2018 18:52

loganasherjones force-pushed the issue/97 branch from c3cbc02 to 8173491 Compare March 22, 2018 00:32

loganasherjones force-pushed the issue/97 branch from 6510304 to 17d0536 Compare March 22, 2018 01:16

loganasherjones added 3 commits March 21, 2018 23:24

Added two-way SSL support for XMLRPC.

59712fe

Everything that works from the SSL perspective now also works with the XML-RPC interface. Including: * Certificate Authority Specification * Client Certificates Encrypted & Unencrypted Signed-off-by: Logan Jones <loganasherjones@gmail.com>

Added myself to AUTHORS file.

6c2238f

Signed-off-by: Logan Jones <loganasherjones@gmail.com>

loganasherjones changed the title ~~[WIP] Two-way SSL support~~ Two-way SSL support Mar 22, 2018

Cleaned up error for password mismatches.

e766780

This prevents a stacktrace from happening during the sphinx-build and raises a more helpful error message. Signed-off-by: Logan Jones <loganasherjones@gmail.com>

tonybaloney requested changes Mar 22, 2018

View reviewed changes

tonybaloney requested a review from jdknight March 22, 2018 06:13

loganasherjones commented Mar 22, 2018

View reviewed changes

jdknight requested changes Mar 23, 2018

View reviewed changes

Implemented feedback from @jdknight

5f083e0

1. Fixed lexographical ordering problems. 2. Fixed whitespacing issues. 3. Fixed missing raise. 4. Refactored ca_cert path checking. 5. Updated docs to link to relevant ssl/requests sections. Signed-off-by: Logan Jones <loganasherjones@gmail.com>

jdknight approved these changes Mar 23, 2018

View reviewed changes

tonybaloney approved these changes Mar 23, 2018

View reviewed changes

tonybaloney merged commit 0c47b40 into sphinx-contrib:master Mar 23, 2018

jdknight mentioned this pull request Mar 30, 2018

Two-Way SSL Publishing Support #97

Closed


		.. code-block:: python

		confluence_ca_cert = '/path/to/ca.crt'

		kwargs['ssl_context'] = context
		return super(SSLAdapter, self).init_poolmanager(args, *kwargs)

Two-way SSL support #99

Two-way SSL support #99

Uh oh!

Conversation

loganasherjones commented Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

loganasherjones commented Mar 22, 2018

Uh oh!

loganasherjones commented Mar 22, 2018

Uh oh!

tonybaloney left a comment

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonybaloney commented Mar 22, 2018

Uh oh!

loganasherjones left a comment

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!

loganasherjones commented Mar 23, 2018

Uh oh!

jdknight left a comment

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!

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!

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!

loganasherjones commented Mar 23, 2018

Uh oh!

loganasherjones commented Mar 21, 2018 •

edited

Loading

jdknight commented Mar 23, 2018 •

edited

Loading