New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require client certificates #52

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@dpeschman
Contributor

dpeschman commented Oct 13, 2017

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    Feature

  • What is the related issue number (starting with #)
    cherrypy/cherrypy#1001 (CherryPy) — partial implementation

  • What is the current behavior? (You can also link to an open issue here)
    Using the built-in SSL adapter, client certificates are neither required nor validated with a CA. Client certificate fields (subject, issuer) are not passed downstream via WSGI environment.

  • What is the new behavior (if this is a feature change)?
    New parameter verify_mode on BuiltinSSLAdapter can be one of none/optional/required to specify whether client certificates are required. These values map to Python's verify_mode values [1]. The default is "none" for backwards compatibility.

Subject and issuer of the client certificate are passed downstream via WSGI environment. For instance, certificate subject is passed down as SSL_CLIENT_S_DN_* fields.

  • Other information:
    This patch was inspired by, and much of the code derived from, the patch in CherryPy cherrypy/cherrypy#1001. That change includes additional features, such as hostname verification and parallel support for pyOpenSSL, which are omitted here.

There will be a forthcoming PR to cherrypy with an ssl_verify_mode on server adapter to pass along to the SSL adapter.

I plan to incorporate the tests from CherryPy cherrypy/cherrypy#1001, but I expect most of them belong in CherryPy. I'm happy to add tests here for these changes, though, if you'd like.

CONTRIBUTING.rst has a note to ensure you set up pre-commit utility correctly and TravisCI tests pass, but it is not clear to me how to do this, and your guidance is appreciated.

[1] https://docs.python.org/3.5/library/ssl.html?highlight=ssl#constants


This change is Reviewable

@codecov

This comment has been minimized.

codecov bot commented Oct 13, 2017

Codecov Report

Merging #52 into master will increase coverage by 0.61%.
The diff coverage is 98.05%.

@@            Coverage Diff            @@
##           master     #52      +/-   ##
=========================================
+ Coverage   66.08%   66.7%   +0.61%     
=========================================
  Files          15      16       +1     
  Lines        2748    2850     +102     
=========================================
+ Hits         1816    1901      +85     
- Misses        932     949      +17

@cherrypy cherrypy deleted a comment from mention-bot Oct 15, 2017

@webknjaz webknjaz requested review from webknjaz and jaraco Oct 15, 2017

@webknjaz

This comment has been minimized.

Member

webknjaz commented Oct 15, 2017

@dpeschman Thank you for your contribution!

CONTRIBUTING.rst has a note to ensure you set up pre-commit utility correctly and TravisCI tests pass, but it is not clear to me how to do this, and your guidance is appreciated.

Actually, just pip install pre-commit and then pre-commit install before committing of pre-commit run --all-files to run it manually.
CI already runs that, so you can look up errors from logs there. Installing it manually is just for convenience, so that you could catch errors early :)

@webknjaz

This comment has been minimized.

Member

webknjaz commented Oct 15, 2017

Just a friendly tip: here's how to refer other repo's issue — cherrypy/cherrypy#1001. JFYI, i've fixed it in your post

@@ -17,12 +17,14 @@ class Adapter(object):
"""
@abstractmethod
def __init__(self, certificate, private_key, certificate_chain=None, ciphers=None):
def __init__(self, certificate, private_key, certificate_chain=None, ciphers=None,
verify_mode=None):

This comment has been minimized.

@webknjaz

webknjaz Oct 15, 2017

Member

You don't have to have 80 chars wrap: this project's linters are configured to allow up to 120-chars lines. We don't work in 80-char terminals nowadays :)

'required': ssl.CERT_REQUIRED,
}
def get_verify_mode_constant(self, spec):

This comment has been minimized.

@webknjaz

webknjaz Oct 15, 2017

Member

I'd prefer pythonic way implementing @property setter/getter.

@webknjaz

This comment has been minimized.

Member

webknjaz commented Oct 15, 2017

@dpeschman I didn't review the PR in detail, but I've posted a few comments, so that we could proceed with discussion/reaction :)

@dpeschman

This comment has been minimized.

Contributor

dpeschman commented Oct 16, 2017

@webknjaz Thanks for the review and the tips n' tricks!
I've implemented the changes you suggested.

_verify_mode = None
@property
def verify_mode(self):

This comment has been minimized.

@webknjaz

webknjaz Oct 17, 2017

Member

I think, we should keep any method declarations below __init__

if client_cert:
for cert_key, env_var in self.CERT_KEY_TO_ENV.items():
ssl_environ.update(
self.env_dn_dict(

This comment has been minimized.

@webknjaz

webknjaz Oct 17, 2017

Member

Plz do this a bit more flat for the sake of readability

if value not in self.VERIFY_MODES:
raise ValueError('Invaid verify_mode "%s"; must be one of %s' % (
value, self.VERIFY_MODES.keys()))
self._verify_mode = value

This comment has been minimized.

@webknjaz

webknjaz Oct 17, 2017

Member

please transform 'none' string to None here, so that if self.verify_mode: check would evaluate stable result

@@ -117,8 +150,56 @@ def get_environ(self, sock):
# SSL_VERSION_INTERFACE string The mod_ssl program version
# SSL_VERSION_LIBRARY string The OpenSSL program version
}
client_cert = sock.getpeercert()

This comment has been minimized.

@webknjaz

webknjaz Oct 17, 2017

Member

I'd add if self.verify_mod check in order to avoid unnecessary syscall when this feature is disabled.

return ssl_environ
CERT_KEY_TO_ENV = {

This comment has been minimized.

@webknjaz

webknjaz Oct 17, 2017

Member

Please keep class properties at the top, above method declarations

for rdn in cert_value:
for attr_name, val in rdn:
attr_code = self.CERT_KEY_TO_LDAP_CODE.get(attr_name)
if attr_code:

This comment has been minimized.

@webknjaz

webknjaz Oct 17, 2017

Member

Doing try/except rather then if/else is more pythonic way.

attr_code = self.CERT_KEY_TO_LDAP_CODE.get(attr_name)
if attr_code:
env['%s_%s' % (env_prefix, attr_code)] = val
# dn.append("%s=%s" % (attr_code, val))

This comment has been minimized.

@webknjaz

webknjaz Oct 17, 2017

Member

do you still need this comment?

@webknjaz

This comment has been minimized.

Member

webknjaz commented Oct 17, 2017

Additionally, I'd really appreciate submitting tests, cause SSL is fairly unstable part of Cheroot.

@webknjaz

This comment has been minimized.

Member

webknjaz commented Nov 14, 2017

This pull request introduces 1 alert - view on lgtm.com

new alerts:

  • 1 for Comparison using is when operands support eq

Comment posted by lgtm.com

@jaraco

This comment has been minimized.

Member

jaraco commented Nov 20, 2017

ensure you set up pre-commit utility correctly and TravisCI tests pass

What I do is run tox -e pre-commit.

@@ -76,7 +123,8 @@ def wrap(self, sock):
server_side=True, certfile=self.certificate,
keyfile=self.private_key,
ssl_version=ssl.PROTOCOL_SSLv23,
ca_certs=self.certificate_chain)
ca_certs=self.certificate_chain,

This comment has been minimized.

@jaraco

jaraco Nov 20, 2017

Member

I'm pretty sure based on other code I've seen that ssl.wrap_socket is the legacy way to handle SSL and that context.wrap_socket is the preferred way to handle it. The whole reason the concept of the context was added was to avoid scattering SSL parameters as attributes of every class that touches sockets. Instead, the context object encapsulates these parameters.

If that's true (and I'm not certain it is), any support for SSL cert validation should be provided first on self.context and then only if absolutely necessary, also provide legacy support. Perhaps the context object is sufficient to satisfy all modern deployments.

In fact, I'd be tempted to say that the legacy technique for wrapping a socket should soon be deprecated and removed in favor of the context model.

Still, I don't think that eliminates the value of this PR. This PR could indeed adapt itself to demonstrate and test the context-based SSL certificatate validation support, but at the same time avoid the added complexity of additional pass-through parameters (with interpretation).

What do you think of that approach?

This comment has been minimized.

@webknjaz

This comment has been minimized.

@jaraco

jaraco Nov 23, 2017

Member

I'd be tempted to say that the legacy technique for wrapping a socket should soon be deprecated and removed in favor of the context model.

I have so many things on my mind that I hadn't connected that only a few days prior, I'd done just that in e4be9d6 (pending review in #60).

return context
@ddt.ddt

This comment has been minimized.

@jaraco

jaraco Nov 20, 2017

Member

I like the use of ddt, and if weren't married to unittest by way of webtest and could use pytest fixtures instead, I'd prefer those. But I'm glad to see these work here.

This comment has been minimized.

@webknjaz

webknjaz Feb 3, 2018

Member

And we fully support using pytest fixtures now :)

This comment has been minimized.

@webknjaz
@webknjaz

This comment has been minimized.

Member

webknjaz commented Dec 2, 2017

@dpeschman this branch is out of sync, rebase and conflicts resolution is required

@dpeschman

This comment has been minimized.

Contributor

dpeschman commented Dec 2, 2017

@jaraco Providing my own SSL context would be a superior implementation. A comment in the code led me to believe ssl_context was only used for PyOpenSSL. I’ll try providing my own context directly next time I iterate on this project.

@dpeschman

This comment has been minimized.

Contributor

dpeschman commented Dec 2, 2017

@webknjaz Roger that.

@stale

This comment has been minimized.

stale bot commented Jan 31, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added stale and removed stale labels Jan 31, 2018

@dpeschman

This comment has been minimized.

Contributor

dpeschman commented Feb 1, 2018

Rebase onto latest master; no substantial changes.

@dpeschman dpeschman force-pushed the dpeschman:ssl-verify-mode branch from a43587c to 0800e51 Feb 1, 2018

@dpeschman dpeschman force-pushed the dpeschman:ssl-verify-mode branch from 0800e51 to d8af237 Feb 2, 2018

@dpeschman

This comment has been minimized.

Contributor

dpeschman commented Feb 2, 2018

@jaraco @webknjaz I got rid of the new SSL Adapter parameter. This change now consists only of passing client certificate fields through to the WSGI environment, and unit testing of HTTPS with various SSL verify modes.

@@ -43,6 +43,21 @@ class BuiltinSSLAdapter(Adapter):
ciphers = None
"""The ciphers list of SSL."""
CERT_KEY_TO_ENV = {
'subject': 'SSL_CLIENT_S_DN',
'issuer': 'SSL_CLIENT_I_DN'

This comment has been minimized.

@webknjaz

webknjaz Feb 3, 2018

Member

Could you plz add a trailing comma?

def getPage(self, *args, **kw):
"""Fetch the page."""
return super().getPage(self.script_name, *args,

This comment has been minimized.

@webknjaz
from cheroot.ssl.builtin import BuiltinSSLAdapter
from cheroot.test import helper
import cheroot
import ddt

This comment has been minimized.

@webknjaz

webknjaz Feb 3, 2018

Member

@dpeschman we now rely on pytest, which means no need for ddt anymore

This comment has been minimized.

@jaraco

jaraco Aug 29, 2018

Member

Since this PR was originally drafted on unittest-based tests, I'd be inclined to accept this change, rather than asking the contributor to chase our changes.

This comment has been minimized.

@webknjaz

webknjaz Aug 29, 2018

Member

I think I could try rewriting this on top of https://github.com/cherrypy/cheroot/tree/experiment/ssl-tests (ref #95). And we also need solid design change of makefile module (ref #6).
Anyway, it's currently unmergable because of conflicts and syntax is sometimes incompatible with Python 2. (see my super() comment)

P.S. We could probably drop some of hardcoded certificates in favor of trustme, which seems to be providing a very smooth experience for TLS testing with fake trusted CA.

This comment has been minimized.

@jaraco

jaraco Aug 29, 2018

Member

I've resolved the conflicts and Python 2 syntax issues. Happy to see iterations on the code to eliminate ddt and remove unittest constructs.

@@ -0,0 +1,32 @@
-----BEGIN CERTIFICATE-----

This comment has been minimized.

@webknjaz

webknjaz Feb 3, 2018

Member

I'd like to see those ssl stubs generated (and cleaned up) by pytest fixtures rather then be part of the repo.

This comment has been minimized.

@webknjaz

webknjaz Feb 4, 2018

Member

(this way it would be like self-documentation for using this feature as well)

env = {}
for rdn in cert_value:
for attr_name, val in rdn:
attr_code = self.CERT_KEY_TO_LDAP_CODE.get(attr_name)

This comment has been minimized.

@webknjaz

webknjaz Feb 3, 2018

Member

I'd prefer try/except approach over if check. It's easier to ask for forgiveness, then for permission (which is considered more pythonic).

@jaraco

This comment has been minimized.

Member

jaraco commented Aug 29, 2018

It looks to me like this effort was abandoned, perhaps out of frustration? In my estimation, the main request that needed to be addressed was the Python 2.7 compatibility.

@jaraco

jaraco approved these changes Aug 29, 2018

@jaraco

This comment has been minimized.

Member

jaraco commented Aug 29, 2018

I see the pre-commit failing with:

Verifying PEP257 Compliance..............................................Failed
hookid: pydocstyle
cheroot/test/test_https.py:45 in public method `setup_server`:
        D401: First line should be in imperative mood; try rephrasing (found 'Setup')
cheroot/test/test_https.py:56 in public method `setUp`:
        D403: First word of the first line should be properly capitalized ('Testcase', not 'TestCase')

jaraco added a commit that referenced this pull request Aug 29, 2018

@jaraco jaraco closed this Aug 29, 2018

@jaraco jaraco referenced this pull request Aug 29, 2018

Merged

Require Client Certificates #108

jaraco added a commit that referenced this pull request Aug 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment