Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(40)

#16487: Allow ssl certificates to be speficfied from memory rather than files.

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 10 months ago by sweskman
Modified:
1 year ago
Reviewers:
lists, senthil, jgehrcke
CC:
jcea, orsenthil, AntoinePitrou, krisvale, haypo, giampaolo.rodola, christian.heimes, njs, asvetlov, Jan-Philip Gehrcke, brandon-rhodes, dstufft, martius
Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Patch Set 3 #

Patch Set 4 #

Total comments: 11
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Doc/library/ssl.rst View 1 2 3 2 chunks +10 lines, -6 lines 0 comments Download
Lib/ssl.py View 1 2 3 2 chunks +46 lines, -0 lines 5 comments Download
Lib/test/test_ssl.py View 1 2 3 6 chunks +216 lines, -56 lines 0 comments Download
Modules/_ssl.c View 1 2 3 3 chunks +202 lines, -6 lines 6 comments Download
Modules/clinic/_ssl.c.h View 1 2 3 3 chunks +46 lines, -10 lines 0 comments Download

Messages

Total messages: 3
christian.heimes
Good work! I have left some feedback. https://bugs.python.org/review/16487/diff/18126/Lib/ssl.py File Lib/ssl.py (right): https://bugs.python.org/review/16487/diff/18126/Lib/ssl.py#newcode446 Lib/ssl.py:446: certfile_path, keyfile_path, ...
1 year ago #1
orsenthil
http://bugs.python.org/review/16487/diff/18126/Lib/ssl.py File Lib/ssl.py (right): http://bugs.python.org/review/16487/diff/18126/Lib/ssl.py#newcode455 Lib/ssl.py:455: certdata = certdata.encode('ascii') On 2016/09/09 10:13:54, christian.heimes wrote: > ...
1 year ago #2
Jan-Philip Gehrcke
1 year ago #3
Thanks for your kind feedback, Christian and Senthil!

I have responded to every point. Would appreciate your quick feedback again
(should really be quick) before submitting an amended patch.

https://bugs.python.org/review/16487/diff/18126/Lib/ssl.py
File Lib/ssl.py (right):

https://bugs.python.org/review/16487/diff/18126/Lib/ssl.py#newcode446
Lib/ssl.py:446: certfile_path, keyfile_path, password)
On 2016/09/09 10:13:54, christian.heimes wrote:
> Small comment: it's more secure to let OpenSSL load the file. It takes more
care
> to clean up behind itself. Python may leaf remnants of private key material
> memory. We don't care about clearing memory after use.

Understood, but I am not sure what to do with this comment :-). 

In that specific code section where you added this comment, the file path(s)
is/are handed over to OpenSSL's SSL_CTX_use_certificate_chain_file(), as we did
before.

https://bugs.python.org/review/16487/diff/18126/Lib/ssl.py#newcode455
Lib/ssl.py:455: certdata = certdata.encode('ascii')
On 2016/09/09 10:13:54, christian.heimes wrote:
> Why encode('ascii')? A PEM file may contain additional comments that are not
> ASCII. OpenSSL should ignore all text that is not surrounded by BEGIN/END
> markers.

Good point. When writing this I in fact assumed that a PEM file contains
exclusively PEM data. I will follow Senthil's suggestion to use encode('utf-8',
'ignore').

https://bugs.python.org/review/16487/diff/18126/Modules/_ssl.c
File Modules/_ssl.c (right):

https://bugs.python.org/review/16487/diff/18126/Modules/_ssl.c#newcode2909
Modules/_ssl.c:2909: usecert = PEM_read_bio_X509_AUX(bio, NULL, pwcb,
pwcb_data);
On 2016/09/09 10:13:54, christian.heimes wrote:
> PEM_read_bio_X509_AUX() looks wrong. The AUX variant also loads additional
> information from TRUSTED CERTIFICATE. A trusted cert can add trust parameters
> and override CA signatures.
> 
> See https://www.openssl.org/docs/manmaster/apps/x509.html "Trust settings".

Well, I feel like sitting between a hard place and a stone here.

I initially tried to build up the logic using OpenSSL documentation. Only to
frequently run into situations where it was clear that the docs are not precise
(did not answer the question that I had) or even wrong. So I looked at existing
code. As I wrote in my lengthy comment in
http://bugs.python.org/issue16487#msg272585 "to me the safest bet seems to keep
the memory/BIO-based load_cert_chain() implementation as close as possible to
use_certificate_chain_file() in OpenSSL 1.1.0"

So I have literally copy/pasted the entire thing from the OpenSSL code and then
carefully adjusted, trying to ensure that the underlying method is equivalent in
both cases.

This is the implementation of use_certificate_chain_file in OpenSSL 1.1.0:

https://fossies.org/dox/openssl-1.1.0/ssl__rsa_8c_source.html#l00585

In line 615 you see that it uses `PEM_read_bio_X509_AUX()` just the way how I do
here.

So, even if the docs (and you) are right, I am not sure if we should deviate
from what OpenSSL is doing itself.

https://bugs.python.org/review/16487/diff/18126/Modules/_ssl.c#newcode2920
Modules/_ssl.c:2920: r = SSL_CTX_clear_chain_certs(ctx);
On 2016/09/09 10:13:54, christian.heimes wrote:
> I think this removes all existing chains. AFAIK OpenSSL can have a RSA and
ECDSA
> chain in the same context.

Same argumentation as above, this is done here:

https://fossies.org/dox/openssl-1.1.0/ssl__rsa_8c_source.html#l00640

https://bugs.python.org/review/16487/diff/18126/Modules/_ssl.c#newcode2927
Modules/_ssl.c:2927: cacert = PEM_read_bio_X509(bio, NULL, pwcb, pwcb_data);
On 2016/09/09 10:13:54, christian.heimes wrote:
> What happens if BIO contains no valid cert at all? I think usecert fails
before.
> Can you add a comment that explains how you ensure that BIO must contain at
> least one cert?

What you say is correct, the `SSL_CTX_use_certificate(ctx, usecert);` will error
out above. This is also covered by the corresponding unit tests (watch out for
`EMPTYCERT`).

Will add this comment!
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7