Opened 5 years ago

Closed 4 years ago

#6499 enhancement closed fixed (fixed)

Teach chain certificates to SSL server endpoints string syntax

Reported by: Hynek Schlawack Owned by: Tom Prince
Priority: normal Milestone:
Component: core Keywords:
Cc: Hynek Schlawack, Glyph, zooko@… Branch: branches/ssl-endpoint-string-chain-certificate-6499
branch-diff, diff-cov, branch-cov, buildbot
Author: tomprince

Description

Now we have chain certificates, twisted.internet.endpoints._parseSSL should be able to handle them too.

Attachments (8)

6499-chains-for-strports.diff (9.1 KB) - added by Hynek Schlawack 5 years ago.
6499-chains-for-strports-v2.diff (9.4 KB) - added by Hynek Schlawack 5 years ago.
Topfile + grammar.
6499-chains-for-strports-v3.diff (11.5 KB) - added by Hynek Schlawack 5 years ago.
Add endpoints documentation.
6499-chains-for-strports-v4.diff (10.8 KB) - added by Hynek Schlawack 4 years ago.
Updates as per zooko’s & exarkun’s feedback.
6499-chains-for-strports-v5.diff (10.8 KB) - added by Hynek Schlawack 4 years ago.
Fix outdated comment.
fix-ci-errors.patch (1.5 KB) - added by Hynek Schlawack 4 years ago.
fix-python3-and-ssl.patch (1.6 KB) - added by Hynek Schlawack 4 years ago.
Fix Python 3.3 & pyOpenSSL regression.
fix-tests-for-windows.patch (600 bytes) - added by Hynek Schlawack 4 years ago.

Download all attachments as: .zip

Change History (35)

comment:1 Changed 5 years ago by Jean-Paul Calderone

As part of this, it would be great to add an example demonstrating the use of chain certificates to the SSL howto.

comment:2 Changed 5 years ago by Glyph

Cc: Glyph added

comment:3 Changed 5 years ago by Hynek Schlawack

So while the code here is rather trivial, testing it properly is rather daunting (like everything SSL-related :-/).

Ideally we’d add a full fake CA to the testing code base so we have both more keys and certificates to play with but also to be able to generate new ones on demand. For example I need a file with multiple certificates right now. The strports code on the other hand uses one certificate file for everything – and doesn’t even bother to check whether the file is correctly imported (cf source:trunk/twisted/internet/test/test_endpoints.py#L1798 ).

Any opinions on how to proceed here? I’m stuck here until this gets resolved but I don’t feel like press ahead without consultation.

comment:4 Changed 5 years ago by Jean-Paul Calderone

Try not to be stuck for too long. If you need to generate 3 or for keys and certificates for some new tests and check them in, fine. Maybe it's not the totally ideal solution, but it lets you make progress so it's not all bad. Afterwards, if you want to make a pass over all the certificate material in Twisted's test suite and organize it a little bit, that wouldn't hurt either.

I do want to recommend against generating new certificates during test runs. It's slow. I prefer fast tests and pre-generated crypto objects.

Changed 5 years ago by Hynek Schlawack

comment:5 Changed 5 years ago by Hynek Schlawack

Keywords: review added
Owner: set to Glyph

First regarding the SSL howto: I haven’t done anything yet since it’s using Certificate.options() which is a very limited API not allowing chain files. I’m afraid we’ll need to close #6361 first – or rewrite lots of the howto.

As for test data, I decided to simply concatenate the existing certificates which gave me nice verification possibilities. I also needed a file that definitely doesn’t contain any certificates, so I went for __init__.py. If you want me to add some lorem ipsum instead I can do that too…

If you want to play with it, I’ve added a second example in https://gist.github.com/hynek/4095c70615a223ecc1df that uses strports.

As per his mail on the mailing list, I’m assigning the review to glyph but I’m sure he’ll allow others to look into the code too. ;)

Changed 5 years ago by Hynek Schlawack

Topfile + grammar.

comment:6 Changed 5 years ago by Itamar Turner-Trauring

The endpoints howto should document this syntax in the SSL servers section, especially since chain certs are basically a necessity.

Changed 5 years ago by Hynek Schlawack

Add endpoints documentation.

comment:7 Changed 5 years ago by Hynek Schlawack

FTR, I’ve opened a ticket at #6513 for client chain file string syntax.

comment:8 Changed 5 years ago by zooko

Cc: zooko@… added
Owner: changed from Glyph to zooko
Status: newassigned

comment:9 Changed 5 years ago by zooko

Keywords: review removed
Owner: changed from zooko to Hynek Schlawack
Status: assignednew

This is my review of attachment:6499-chains-for-strports-v3.diff .

Replace "trustworthy" with "root" in "from a trustworthy CA to the one that signed you certificate" ("trustworthy" is not the technical term for this, and is also inaccurate). Replace "you" with "your". While you're at it, replace "the chain of trust" with "the chain".

Also in the docstring, replace "trustworthy" with "root" and remove "of trust".

Why is it "notACertQuotedPathName" instead of "escapedNotACertPathName", like the others? (The others are "escapedChainPathName", "escapedCAsPathName", and "escapedPEMPathName".)

It would make me feel slightly better if the test for "are there no certs here?" was moved down one line to be a test of chainCertificates instead of a test of matches. I can't come up with a failure scenario which would justify this, but I just feel better checking that the fully-parsed objects are present rather than checking that the half-parsed strings are present. (Note that this half-parsing represented by the re.findall is the first parsing of certs in Twisted or pyOpenSSL (excluding test code). Hopefully also the last.)

Okay, otherwise this patch looks good to me! It is good that it includes a test of the exception raised when the file doesn't contain any certs. Thank you for writing this patch! I really need this fix. ☺

comment:10 Changed 5 years ago by Jean-Paul Calderone

Thanks hynek, zooko.

In addition to the comments above, please consider:

  1. doc/core/howto/endpoints.xhtml
    1. Reflowing prose makes for super difficult reviews (presumably due to shortcomings of our tools). :( Some people have suggested that one sentence per line in prose is a good compromise. Since the text here is already word-wrapped, I would suggest just making edits *without* rewrapping it.
    2. Instead of "certKey, privateKey, extraCertChain and sslmethod." please write "certKey, privateKey, extraCertChain, and sslmethod."
  2. twisted/internet/endpoints.py
    1. The type documentation for the new parameter, extraCertChain, has two issues.
      1. It should be L{} instead of C{} (I know the nearby parameters use C{}, they date from an earlier standard).
      2. It should not be str now that str has become ambiguous. All new documentation (and eventually all old documentation) should be written using bytes or unicode. There may be some exceptions where str is still appropriate, but you can be pretty sure they won't involve user input.
  3. twisted/internet/test/test_endpoints.py
    1. Files not containing certificates are cheap. self.mktemp() nearly gives you one, even. I think using __init__.py for notACertPath is a little too clever. Particular for a file with unit tests for SSL code, including certificate handling, who's to say there will never be a certificate in this file?
    2. I'd like to see the new chain-related code and assertions in ServerStringTests.test_ssl moved into new test methods. The fewer assertions in a test method, the better (at least, until you get to 1).
  4. twisted/topfiles/6499.feature
    1. Maybe we'll never be free of the curse of strports... but we can try. This branch is concerned with "string endpoint descriptions" - twisted.internet.endpoints.serverFromString, not twisted.application.strports. I suggest describing the feature as an improvement to "SSL server endpoint string descriptions" rather than "strports".

I like that zooko pointed out that this is the first instance of PEM parsing code anywhere in Twisted (or even pyOpenSSL). That makes me somewhat uneasy. It would be cool if we could find an API in OpenSSL to do this for us (but a cursory investigation hasn't revealed such an API to me). I also briefly worried about the ".+?" in the regular expression, but I can't see how it could cause a real world problem.

Thanks again.

Changed 4 years ago by Hynek Schlawack

Updates as per zooko’s & exarkun’s feedback.

comment:11 Changed 4 years ago by Hynek Schlawack

Keywords: review added
Owner: Hynek Schlawack deleted

I hope I have incorporated all feedback both in word and spirit.

As for the docs:I have unwrapped lines that have been touched anyway, I hope that’s the right compromise and a good strep towards getting it like that everywhere eventually.

Changed 4 years ago by Hynek Schlawack

Fix outdated comment.

comment:12 Changed 4 years ago by Hynek Schlawack

Since I seem to have triggered some error in trac before ( http://glui.me/?i=3vnrs4xp6z2df4j/2013-05-27_at_10.10.26.png/ ) and apparently no notifies went out, I hereby ping it once more that I’ve updated the patch according to the feedback.

comment:13 Changed 4 years ago by Tom Prince

Author: tomprince
Branch: branches/ssl-endpoint-string-chain-certificate-6499

(In [38479]) Branching to ssl-endpoint-string-chain-certificate-6499.

comment:14 Changed 4 years ago by Tom Prince

(In [38480]) Apply 6499-chains-for-strports-v5.diff from hynek.

Refs: #6499

comment:15 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Hynek Schlawack

I started to look at this, but the last patch, which I applied to a branch doesn't appear to have all the code, and I'm not sure what else should be applied there. Can you attach a patch against the branch that has everything?

Also, there is a python3 build failure and pydoctor error (see #6318 for some hints on C{} vs. L{}).

Changed 4 years ago by Hynek Schlawack

Attachment: fix-ci-errors.patch added

comment:16 Changed 4 years ago by Hynek Schlawack

Keywords: review added
Owner: changed from Hynek Schlawack to Tom Prince

I’ve addressed the build errors.

comment:17 Changed 4 years ago by Tom Prince

(In [38803]) Apply fix-ci-errors.patch from hynek.

Refs: #6499

comment:18 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: changed from Tom Prince to Hynek Schlawack
  1. Regarding parsing chain cert files, from a cursory perusal of openssl code, it appears that the way to read multiple certificates from a file is likely just to try and read them consecutively from the same bio. So please file tickets against pyopenssl and twisted to be able to support that.
    • Until that happens, I think the solution here is a reasonable compromise.
  2. Rather than creating a new in-tree file, it would be better to create the file on the fly (see #6535). But certainly use pregenerated data.
    • Since this is a minor point, and the existing tests use existing files, I'll merge as is, but please file a ticket for cleaning this all up.

Please merge and file the appropriate tickets.

comment:19 Changed 4 years ago by Hynek Schlawack

Not sure if you meant me with “please merge” and if so I’m flattered but I can’t. :)

As for your ticket requests:

  1. I’m not 100% sure what the contents of the tickets should be. “Add function to parse files with multiple certificates”? Is that something that belongs into Twisted at all?
  2. #6578, I will tackle that soonish.

comment:20 Changed 4 years ago by ashfall

Resolution: fixed
Status: newclosed

(In [38813]) Merge ssl-endpoint-string-chain-certificate-6499: Making SSL server endpoint string descriptions support the specification of chain certificates.

Author: hynek Reviewers: zooko, exarkun, tomprince Fixes: #6499

comment:21 Changed 4 years ago by Tom Prince

Resolution: fixed
Status: closedreopened

(In [38817]) Revert r38813: python 3.3 regression.

Reopens: #6499

Traceback (most recent call last):
  File "admin/run-python3-tests", line 30, in <module>
    unittest.main(module=None, argv=["run-python3-tests", "-v"] + testModules)
  File "/usr/lib/python3.3/unittest/main.py", line 124, in __init__
    self.parseArgs(argv)
  File "/usr/lib/python3.3/unittest/main.py", line 168, in parseArgs
    self.createTests()
  File "/usr/lib/python3.3/unittest/main.py", line 175, in createTests
    self.module)
  File "/usr/lib/python3.3/unittest/loader.py", line 137, in loadTestsFromNames
    suites = [self.loadTestsFromName(name, module) for name in names]
  File "/usr/lib/python3.3/unittest/loader.py", line 137, in <listcomp>
    suites = [self.loadTestsFromName(name, module) for name in names]
  File "/usr/lib/python3.3/unittest/loader.py", line 96, in loadTestsFromName
    module = __import__('.'.join(parts_copy))
  File "/var/lib/buildbot/buildarea/python-3.3-tests/Twisted/twisted/internet/test/test_endpoints.py", line 1780, in <module>
    class ServerStringTests(unittest.TestCase):
  File "/var/lib/buildbot/buildarea/python-3.3-tests/Twisted/twisted/internet/test/test_endpoints.py", line 1845, in ServerStringTests
    % (escapedPEMPathName,))
NameError: name 'escapedPEMPathName' is not defined

Changed 4 years ago by Hynek Schlawack

Attachment: fix-python3-and-ssl.patch added

Fix Python 3.3 & pyOpenSSL regression.

comment:22 Changed 4 years ago by Hynek Schlawack

Keywords: review added
Owner: Hynek Schlawack deleted
Status: reopenednew

This is embarrassing, I was under the impression that we don’t test Python 3.3 with SSL. Since the SSL-code is riddled with these path-names, I thought it’s enough to protect them like that. Oh well, there’s obviously more to it.

I made the code less clever and it passes regardless of the presence of PyOpenSSL on Python 3.3.

I’m sorry everyone.

comment:23 Changed 4 years ago by radix

I've applied the patch to the branch, so the branch should now be up to date.

Changed 4 years ago by Hynek Schlawack

Attachment: fix-tests-for-windows.patch added

comment:24 Changed 4 years ago by Hynek Schlawack

The error message of the XP buildbot is a bit cryptic but since the empty temporary file was unquoted, I’ll presume it’s the reason for it. Applying and buildbot-nudging would be appreciated to go for sure. :)

comment:25 Changed 4 years ago by Hynek Schlawack

For the record, yes the previous patch fixed the SSL error in the XP buildbot.

The Python 3.3 errors that caused the revert have been addressed too.

Therefore this branch is ready for final review/merge again.

comment:26 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Tom Prince
Status: newassigned

The buildbots look happy, and it is good not having double interpolation on the string.

I'll merge this.

comment:27 Changed 4 years ago by Tom Prince

Resolution: fixed
Status: assignedclosed

(In [38882]) Merge ssl-endpoint-string-chain-certificate-6499: Make SSL server endpoint string descriptions support the specification of chain certificates.

Author: hynek Reviewers: zooko, exarkun, tomprince Fixes: #6499

Note: See TracTickets for help on using tickets.