ctx.load_verify_locations(cadata)

[tiran]

BPO	18138
Nosy	@jcea, @pitrou, @tiran, @ezio-melotti
Files	sslctx_add_cert.patch sslctx_add_cert2.patch sslctx_add_cert4.patch sslctx_add_cert5.patch ssl_cadata.patch ssl_cadata2.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/tiran'
closed_at = <Date 2014-06-19.22:43:31.639>
created_at = <Date 2013-06-05.01:50:33.770>
labels = ['extension-modules', 'type-feature']
title = 'ctx.load_verify_locations(cadata)'
updated_at = <Date 2014-06-19.22:43:31.638>
user = 'https://github.com/tiran'

bugs.python.org fields:

activity = <Date 2014-06-19.22:43:31.638>
actor = 'ezio.melotti'
assignee = 'christian.heimes'
closed = True
closed_date = <Date 2014-06-19.22:43:31.639>
closer = 'ezio.melotti'
components = ['Extension Modules']
creation = <Date 2013-06-05.01:50:33.770>
creator = 'christian.heimes'
dependencies = []
files = ['30466', '30519', '30641', '30643', '32731', '32737']
hgrepos = []
issue_num = 18138
keywords = ['patch']
message_count = 12.0
messages = ['190637', '190872', '191409', '191411', '191419', '191420', '191421', '203522', '203541', '203564', '203565', '212973']
nosy_count = 5.0
nosy_names = ['jcea', 'pitrou', 'christian.heimes', 'ezio.melotti', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue18138'
versions = ['Python 3.4']

[tiran]

The patch implements an add_cert(pem_or_der_data) method for the ssl.SSLContext() object. On success the method adds a trusted CA cert to the context's internal cert store. The CA certificate can either be an ASCII unicode string (PEM format) or buffer object (DER / ASN1 format).

The patch also implements a get_cert_count() method for debugging. I'm going to remove that function eventually as it doesn't give correct answers when the object table contains CRLs, too. A correct implementation might be useful to verify set_default_verify_paths().

I've split up the functions so I can re-use _add_cert() in my upcoming patch for an interface to crypt32.dll on Windows.

[tiran]

New patch:

• rename function to add_ca_cert()
• only accept CA certs, no other certs
• raise an error if extra data is found after cert (e.g. two certs). PEM_read_bio_X509() silently ignores extra data
• fixes from Ezio's code review
• documentation

[tiran]

Here is a simplified version of the C function. It uses y* or es# "ascii" to parse the argument.

The check for trailing data ensures that the user gets an error message if she tries to load a PEM string with multiple certs. She might expect that add_ca_cert(pem) loads all PEM certs from the string while in fact PEM_read_bio_X509() only loads the first cert. The new patch make the check optional.

I still need to find a good name for the option, though...

[pitrou]

The check for trailing data ensures that the user gets an error
message if she tries to load a PEM string with multiple certs. She
might expect that add_ca_cert(pem) loads all PEM certs from the
string while in fact PEM_read_bio_X509() only loads the first cert.

I don't think it is useful. Just make the behaviour well-documented.
(there is no security risk in loading too few CA certs)

[tiran]

I'm pondering about the error case "cert already in hash table". There should be a way to distinguish the error from other errors. I see three ways to handle the case:

1. introduce SSLCertInStoreError exeption
2. ignore the error and do nothing
3. ignore the error and return True if a cert was added or False if the cert is already in the store

I like 3).

[pitrou]

Le mardi 18 juin 2013 à 17:30 +0000, Christian Heimes a écrit :

Christian Heimes added the comment:

I'm pondering about the error case "cert already in hash table". There
should be a way to distinguish the error from other errors.

I don't know if you've seen it, but SSLError has "library" and "reason"
attributes (they are little known). See SSLErrorTests.

I see three ways to handle the case:

1. introduce SSLCertInStoreError exeption
2. ignore the error and do nothing
3. ignore the error and return True if a cert was added or False if
   the cert is already in the store

I like 3).

Yes, sounds reasonable.

[tiran]

Yes, I have seen them. In fact OpenSSL has library, function and reason.

if ((ERR_GET_LIB(errcode) == ERR_LIB_X509) &&
(ERR_GET_REASON(errcode) == X509_R_CERT_ALREADY_IN_HASH_TABLE)) {}

I'm going for 3)

[tiran]

I think the patch in bpo-16487 does too many things at once. The new patch is a draft for a new patch that adds SSLContext.load_verify_locations(cadata) to the SSL module. cadata can be a bunch of PEM encoded certs (ASCII) or DER encoded certs (bytes-like). The patch may contain bugs as I haven't verified all error paths yet.

[tiran]

Final patch

[python-dev]

New changeset 234e3c8dc52f by Christian Heimes in branch 'default':
Issue bpo-18138: Implement cadata argument of SSLContext.load_verify_location()
http://hg.python.org/cpython/rev/234e3c8dc52f

[tiran]

Memo to me: update whatsnew

[python-dev]

New changeset 8e3b3b4a90fb by R David Murray in branch 'default':
whatsnew: SSLcontext.load_verify_locations cadata argument (bpo-18138)
http://hg.python.org/cpython/rev/8e3b3b4a90fb