Adding support for client certificate verification in SSLAdapter (patch included)

Anonymous avatarAnonymous created an issue

Enhancement

Adding support for client SSL certificate verification to wsgiserver's SSLAdapter

Reason

SSL support is critical for modern webservers to provide secure services to users. However, there are times when applications running behind the webservers need to determine which clients are actually communicating them. While HTTP basic_auth can provide authentication, SSL provides another means to verify client identity: client certification verification.

Similar to the server providing its SSL certificate, when client verification is in use, clients must provide a certificate signed by a CA that the server recognizes in order for the client to be allowed to connect.

Changes

This patch adds another optional keyword argument to the SSLAdapter init() called ''client_CA''. ''client_CA'' is a string that contains a path to a CA certificate. When client_CA is present, the SSLAdapter knows to perform client verification using this CA. When absent, SSLAdapter behaves as before, ie with no client verification.

Bugs/Issues

While verification is performed correctly for both the ssl_pyopenssl SSLAdapter and the ssl_builtin SSLAdapter, the different implementations provide varying levels of support for SSL client environment variables that are traditionally provided by Apache's mod_ssl. See this page for details [http://www.modssl.org/docs/2.8/ssl_reference.html#ToC25 Mod_SSL Environment Variables].

ssl_pyopenssl currently provides '''no''' client environment variables due to the fact that the SSL handshake and thus the access to the client's certificate occurs at first data transfer - well after the environment variables are set by the SSLAdapter wrap() function.

ssl_builtin provides minimal environment variables. The major limiting factor is that python's builtin ssl routines only expose a small amount of information about the certificates, and then only for the client certificate. This problem will be difficult to fix if ssl_builtin must depend solely on python's ssl.

Patch

See attached Diff

Reported by nmitchell@anl.gov

Comments (16)

  1. Anonymous

    I ran into a problem with this patch in the tick() method of the HTTPServer class in wsgiserver/init.py

    The HTTPServer class has a list of socket errors which it "ignores", meaning that these errors do not cause the entire server to shut down. Any other type of socket error causes CherryPy to exit, because the server believes that something has gone wrong with the socket it uses to accept incoming connections. It uses the global "socket_errors_to_ignore" list to track these recoverable errors.

    If a client connects with no certificate or a bad certificate when using the builtin ssl module, then an ssl.SSLError exception is raised. This is a subclass of socket.error, with an error number value of 1, which is EPERM. Unfortunately, EPERM is not currently on the socket_errors_to_ignore list, so a single bad client can cause the entire server to exit.

    I've added EPERM to this list and submitted an updated patch.

  2. Anonymous

    I've extended the improvements further, mostly by adding hostname checking to both the "builtin" and "pyopenssl" adapters. I also added a few more checks for edge cases, such as an "except TypeError" catch for http://bugs.python.org/issue9729

    My currently project really needs client certificate checking to be configurable, so I added "server.ssl_client_CA" and "server.ssl_client_check" options. The "ssl_client_CA" is exactly what it sounds like, and "ssl_client_check" must be one of "ignore", "optional", or "required" (and is "ignore" by default). These settings cause client certificate checking to use the following options in the "builtin" and "pyopenssl" adapters, respectively: - "ignore" denotes either ssl.CERT_NONE or OpenSSL.SSL.VERIFY_NONE - "optional" denotes either ssl.CERT_OPTIONAL or OpenSSL.SSL.VERIFY_PEER - "required" denotes either ssl.CERT_REQUIRED or OpenSSL.SSL.VERIFY_PEER | OpenSSL.SSL.VERIFY_FAIL_IF_NO_PEER_CERT

    Because these configuration options introduce a lot of combinations of settings, I've also attached a test_ssl.py file with some helper files in the same directory which contain 50 TestCase subclasses, each with 2 tests for clients with and without certificates. Each TestCase checks a different combination of settings, so that every possible combination is covered.

    For example, there's a TestCase for the following case: - server has a valid certificate - client has a certificate signed properly by the CA, but with the wrong hostname - server is using the pyopenssl module - server has its "server.ssl_client_check" option set to "optional"

    This TestCase would verify that a client connecting without a certificate would succeed, while the client connecting with the wrong-hostname certificate would fail.

    If anyone has any questions about this ticket, feel free to email me. Also let me know if there's anything else I can add which would make it more likely for the ticket to get accepted. I'd especially like to know if there's anything I can do to get this ticket accepted before the CherryPy 3.2.0 release, though I imagine that's unlikely since a release candidate has already been put together.

  3. Anonymous

    I've improved client certificate checking in the following ways:

    • wildcard hostnames (e.g. *.example.com) are now supported
    • IP addresses in the common name field are handled appropriately
    • hostname checking can be turned on and off by the server.ssl_client_check_host config option

    I did this because it turns out that my project needs to be able to support client certificates both with and without hostname checking. I'll upload the patch after posting this comment.

    I also added a lot more unit tests, so we're testing every possible combination of settings with a total of 460 unit tests in cherrypy/tests/test_ssl.py, e.g.

    • all tests are run for both the "builtin" and "pyopenssl" adapters
    • all combinations of configuration settings
    • connections are tested for connections both with and without client certificates to ensure that the connection either succeeds or fails appropriately
    • server binding to "localhost", "127.0.0.1", and "0.0.0.0" (this matters because it affects the client's certificate validation, which affects the server's error handling for failed connections)
    • client connecting to either "localhost" "127.0.0.1" (this REALLY matters, because it affects what address the server gets when calling sock.getpeername())
    • client certificates signed to "localhost", "127.0.0.1", and "*.localhost"
    • bad client and server certificates (e.g. wrong hostname, signed by the wrong CA, etc), making sure that failures happen when expected but that they don't bring down the entire server

    Anyway, I don't know if my design will be considered acceptable, but at least the implementation is thoroughly tested. Again, if there's anything I can do to help this patch get accepted into the trunk, just let me know.

  4. Anonymous

    Apologies for the incorrect attachment summary; that was a bad autocomplete.

    The latest patch includes a sensible check in our test_ssl.py unit tests for whether "localhost" is an IPv4 or IPv6 address, since this affects whether it's worth trying to connect to the server at "127.0.0.1", which we want to do if possible to test all possible ways to for certificate checks to succeed or fail.

    The actual wsgiserver code is unchanged in this latest patch (v5) compared to the previous version (v4).

  5. Anonymous

    Would it be possible to get client enviroment variable information from pyopenssl, if we would do the handshake explictly? There is a function in pyopenssl to do the handshake... Could it be added to wrap() ?

  6. Michael Walsh

    The pull requests source repo appears to have been deleted. What is the status of this ticket? Does a patch exist? Has the patch been accepted, if so where?

  7. Chris St. Pierre

    Sorry, that's my fault -- still quite new to Hg, and it scares and confuses me at times. The patch in the ticket applied fairly cleanly and passed tests without any modifications -- I just opened a pull request to hopefully speed the process along, since this ticket was fairly old. If you want me to open another pull request I can do that.

  8. Michael Walsh

    I'll try the patch. If you can open another pull request that would be great. I'd really appreciate it. I plan to use this for an example with my rules engine for to create an example for an article I am writing. I tried Twisted, but what should work hasn't been working for me.

  9. Nathan Kinder

    I'm working on updating the patch for this issue. Aside from rebasing, I found that some minor changes are needed in the new tests to clear out the SSL adapter to prevent later tests from failing. I should have a new pull request ready for this soon.

  10. Log in to comment
Tip: Filter by directory path e.g. /media app.js to search for public/media/app.js.
Tip: Use camelCasing e.g. ProjME to search for ProjectModifiedEvent.java.
Tip: Filter by extension type e.g. /repo .js to search for all .js files in the /repo directory.
Tip: Separate your search with spaces e.g. /ssh pom.xml to search for src/ssh/pom.xml.
Tip: Use ↑ and ↓ arrow keys to navigate and return to view the file.
Tip: You can also navigate files with Ctrl+j (next) and Ctrl+k (previous) and view the file with Ctrl+o.
Tip: You can also navigate files with Alt+j (next) and Alt+k (previous) and view the file with Alt+o.