Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#6258 enhancement closed duplicate (duplicate)

Allow usage of chain files in SSL

Reported by: Hynek Schlawack Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch:
Author:

Description

Since chain certificates seem to be very useful and plainly necessary in many cases, I think it would be appropriate for Twisted to support them directly.

Context.use_certificate_chain_file can handle both single as well as chained certificates as long as they are PEM encoded. This is also how for example nginx handles them.

Therefore I’m proposing the attached patch which is also backward compatible for non-PEM certificates by falling back to use_certificate_file if use_certificate_chain_file failed.

I worked through all dev guides I found but I’m sure I forgot something, so sorry in advance.

Attachments (2)

use-chain-file-api.patch (4.8 KB) - added by Hynek Schlawack 5 years ago.
use-chain-file-api-v2.patch (5.5 KB) - added by Hynek Schlawack 5 years ago.
I forgot the news file in the first version.

Download all attachments as: .zip

Change History (9)

Changed 5 years ago by Hynek Schlawack

Attachment: use-chain-file-api.patch added

Changed 5 years ago by Hynek Schlawack

Attachment: use-chain-file-api-v2.patch added

I forgot the news file in the first version.

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

Keywords: review removed
Resolution: duplicate
Status: newclosed

Thanks.

This seems like a duplicate of #2061. It's hard to say for sure, because the ticket doesn't explain why it would be useful to have the implementation call use_certificate_chain_file, it just claims it should.

The behavior described by point b in the description of #2061 is exactly the behavior provided by calling use_certificate_chain_file, so that sounds duplicate-y to me. Please re-open if this is a mistake. Thanks again.

comment:2 Changed 5 years ago by Glyph

hynek,

Just in case it wasn't clear from exarkun's comment; you should have a look at the discussion on #2061, see if that informs your patch, and re-submit it to that ticket if you think it addresses the problem as described there.

A quick glance at said patch indicates that it at least requires a positive-path test for this functionality though - the only thing it tests is an error condition!

-glyph

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

"Positive path test" means that it needs a test that actually loads a chain cert from a file.

comment:4 in reply to:  3 Changed 5 years ago by Glyph

Replying to itamar:

"Positive path test" means that it needs a test that actually loads a chain cert from a file.

No - that would be "integration test". "Positive path test" (huh, I guess this term is not well-known?) just means that it needs a test which asserts that the appropriate API was called and that the correct thing happens when said API does not raise an error. Right now only the error case, where the API raises an exception, ("negative path") is being tested.

comment:5 Changed 5 years ago by Glyph

Not that having an integration test would be bad either, mind you. Setting up an SSL context is not particularly expensive or difficult, so it could be manipulated and inspected without having a fake.

comment:6 Changed 5 years ago by Hynek Schlawack

My search-fu failed apparently. :( While the tickets starts out differently, it mutates in this issue indeed. Sorry.

comment:7 in reply to:  6 Changed 5 years ago by Glyph

Replying to hynek:

My search-fu failed apparently. :( While the tickets starts out differently, it mutates in this issue indeed. Sorry.

No need to apologize; searching for a duplicate ticket is tricky, and I'm just glad you tried. Thanks for submitting the patch, and I hope you'll follow up on the other ticket :).

Note: See TracTickets for help on using tickets.