Two-Way SSL Publishing Support

[loganasherjones]

Thanks for the library, I plan on using it quite regularly.

Our confluence is backed by a two-way SSL authentication mechanism. If at all possible, I'd like to add support for specifying client certificates. My initial thought was to add this as configuration values. Something like confluence_client_certificate and confluence_ca_file.

Would you all be open to accepting Pull Requests if I can make this work? If so, is there anything you need me to consider? I would assume this needs to get added and supported to the Rest Interface as well as the RPC interface.

[tonybaloney]

Would you all be open to accepting Pull Requests if I can make this work?

Yes please!

This would potentially need to be implemented twice (see related HTTPS proxy feature), since this library supports the Confluence REST API and the older XML RPC API.

See this block of code for the connection instantiation logic you would need to update https://github.com/tonybaloney/sphinxcontrib-confluencebuilder/blob/master/sphinxcontrib/confluencebuilder/publisher.py#L72-L131

@jdknight thoughts?

[tonybaloney]

I would build the REST workflow first btw, and raise that as a PR, especially if you don't have access to an XMLRPC endpoint to test on

[jdknight]

Looks like a good enhancement request to me. Here's some points (that @loganasherjones may already seen):

• Adding your configuration option to __init__.py (as you've suggested, confluence_ca_file is fine, but use what you think is best).
• The REST API (I feel) is required to be supported. The REST portion should be easier for you, just need to tweak the self.session.verify assignment. Just make sure you do not remove support for the confluence_disable_ssl_validation option.
• The XML-RPC transport, while ideally supported as well, could be skipped if you really can't test it (as @tonybaloney has already mentioned; but you get bonus points if you include it 😄). You'd want to add a method into ConfluenceTransport which can pass in the certificate file (in a fashion similar to disable_ssl_verification). All you'd have to do then is build a proper HTTPS context which get's forwarded into httplib.HTTPSConnection. While in theory it should work, I've never tried building a non-default context before (it could be easy, it could be not).
• Ensure you add documentation for you're newly introduced option.
• Please sign-of your work for the commits you introduce.
• If you want, you can append AUTHORS when you make your PR, or we can add you to the list later.

On a related note, I'd hope if a proposed PR is made for this change we can schedule it for next release at the ealiest (so I don't have to re-test the upcoming release cycle #95).

(I wonder if we should make a CONTRIBUTING document to outline some of these points)

Edit: fixed a bad statement

[tonybaloney]

Oh, I assume by "two-way SSL authentication" you mean client-side SSL certificates.

There are 2 ways to do this, the first is to pass a cert kwarg to the request (or the session settings) consisting of a tuple with the ca and the key http://requests.readthedocs.io/en/master/user/advanced/#client-side-certificates

The other mechanism is a signed adapter, which gives you a bit more control. Here's one I've written before https://github.com/apache/libcloud/blob/trunk/libcloud/http.py#L41-L52

"Just make sure you remove support for the confluence_disable_ssl_validation option." > this actually stops it from working. Trust me :-D

[loganasherjones]

Thanks for the quick feedback!

By two-way SSL authentication, I do indeed mean client-side SSL certificates. For the Rest interface, I was thinking about using a signed adapter because I'm pretty sure you can get it to work with an encrypted client-side certificate which is something that requests doesn't support out of the box, but that I would like to see supported.

I haven't done much with the xmlrpclib, though I have configured SSL contexts in many different libraries. I might give it a shot, but my first attempt will probably just be the Rest interface. Since I'm not sure how readily I can test an RPC interface.

I'm not sure what you mean by "Just make sure you remove support for the confluence_disable_ssl_validation option." Do you mean, setting verify to False on the session object while using a client certificate doesn't work?

I will make sure there is documentation and to sign off on the commits.

[jdknight]

Have you tested your PR without a client cert set?

I just was testing something and I got the following error:

...publisher.py", line 477, in __init__
    self._certfile, self._keyfile = None
TypeError: 'NoneType' object is not iterable

I did a crude change to make it work:

        if client_cert:
            self._certfile, self._keyfile = client_cert
        else:
            self._certfile = None
            self._keyfile = None

That being said, curious if you want to re-add a new PR with a related fix? If not, I'll probably submit something tomorrow or this upcoming weekend.

[loganasherjones]

Ahh! Sorry, yeah I had tested it before all the refactorings. I'll go through all my tests again to make sure it works again. PR coming shortly.

[loganasherjones]

Interesting... So, it's failing if it falls back to XML RPC based communications. So unless you were testing that specifically, another error occurred with the REST based request. Was this intended?

[jdknight]

@loganasherjones, yes; by default, this extension will attempt to fallback on XML-RPC if REST fails (unless explicitly configured not to). This was due to this extension originally being XML-RPC only and providing a relaxed transition to any existing users. In time, I assume we'll remove the automatic fallback (when XML-RPC is officially removed from a most recent stable variant of Confluence; and possibly remove XML-RPC support if all official supported variants remove XML-RPC support).

To confirm, I was testing a XML-RPC-based validation test.

[loganasherjones]

Okay cool. I wanted to confirm that you expected to be using the RPC interface and there wasn't another bug with the ReST SSL stuff (because I couldn't replicate it). PR is up and ready for review #101

[jdknight]

With PR's #99 and #101 in, I'll mark this issue as closed.