Better TLS certificate chain support

[Tremolo4]

This PR adds support for fullchain.pem files, i.e., where the main/leaf server certificate is included in the chain file.
Additionally, chain.pem or fullchain.pem files with more than one intermediate certificate are now supported.

The PEM splitting function might look a bit hacky, but it should be perfectly safe and doesn't require any dependencies.

I've changed the last-edit-check to use the most recent timestamp out of all the four .pem files.

Hope this is useful! (it is for me ;))

[Et0h]

Hi @Tremolo4, your code looks like a clever way of circumventing the limitations of Twisted. However, when it comes to encryption our philosophy is to avoid custom implementations as much as possible.

We do not have the resources and expertise to review or maintain a custom implementation. For all we know there was a reason why Twisted did things the way they did, or the specific implementation might work fine with the current version of Twisted but might be problematic in a future version.

As such, our view is that if the issue raised in #242 is to be resolved then it would have to be resolved upstream by Twisted. You can try working with the Twisted team directly on this, but it is up to them which development efforts they wish to prioritise.

Until it is fixed upstream our position is that the matter is closed. However, we recognise that some users will experience the same issue as you, so if you can provide guidance on how people can manually split the fullchain.pem file then we would be happy to add it to the guide at https://github.com/Syncplay/syncplay/wiki/TLS-support

[Tremolo4]

That's fair, thanks for the response. In general I completely agree with your philosophy about leaving encryption to competent implementations as much as possible.

However, I personally think that my PR doesn't interfere with that philosophy, since we're still fully relying on library code to guarantee the security:

• Actually parsing the certificates is done by PySSL/OpenSSL: it will throw an error if, for example, we pass it an invalid certificate.
• If the server is missing a certificate, for example due to a programming mistake on my side, the client will refuse connecting to the server.

But I guess it's a matter of interpretation and you have more leverage here :D

Just one more point: Currently, it is impossible to use TLS with syncplay if your certificate requires more than one intermediate certificate (i.e., chain.pem contains multiple certs). And that is due to syncplay code only: The twisted API does provide that functionality (list of certs), but syncplay never passes more than one in the list. (The problem is that PySSL's load_certificate only parses one certificate at a time).
Edit: That's why I don't think the twisted API is going to improve in this regard.

Anyway, thanks a lot for your work on syncplay and thanks for taking the time to discuss this!
Feel free to just ignore this if the decision is final.

[albertosottile]

Just one more point: Currently, it is impossible to use TLS with syncplay if your certificate requires more than one intermediate certificate (i.e., chain.pem contains multiple certs).

Thanks for raising this issue. While we suspected this condition, were not fully aware of it and we will try to address this in the future.

And that is due to syncplay code only: The twisted API does provide that functionality (list of certs), but syncplay never passes more than one in the list. (The problem is that PySSL's load_certificate only parses one certificate at a time).

I tend to disagree with the first statement. As you said, the issue is that pyOpenSSL.crypto.load_certificate handles only the first certificate in a passed .pem file, as reported here. Hence, I do not see this as an issue caused by syncplay code only, since we are still delegating certificate handling to dedicated libraries.

A potential solution to the latter issue and to the issue originally described by this PR could be to use another external library (e.g. pem - GitHub) to handle the certificates instead of pyOpenSSL directly.

While this approach seems appealing in principle, I remember well that implementing TLS for Syncplay required a lot of trial and error, a lot of failed attempts, and a lot of compromises between supported features and reliability. Hence, for the moment I would tend to maintain the current TLS code as much as possible, while perhaps experimenting privately with pem, then upgrade to a new logic/library only once that's been extensively tested.

[Tremolo4]

I tend to disagree with the first statement. As you said, the issue is that pyOpenSSL.crypto.load_certificate handles only the first certificate in a passed .pem file, as reported here. Hence, I do not see this as an issue caused by syncplay code only, since we are still delegating certificate handling to dedicated libraries.

You're completely right, I stand corrected. Let's say the two APIs (PySSL and twisted) are not fully compatible / are incomplete for the desired use case.

When looking into how to fix this situation, I saw hynek's pem library as well. But I thought it would be overkill to use it, because the only thing we ever need out of the chain.pem files are the -----BEGIN CERTIFICATE-----...-----END CERTIFICATE----- blocks -- as implemented in the PR. I was faced with deciding between including another external library or writing 20-odd lines of code and opted for the latter :P . But I'm fairly certain that it would work equally well with that library.

Edit (continuation):

While this approach seems appealing in principle, I remember well that implementing TLS for Syncplay required a lot of trial and error, a lot of failed attempts, and a lot of compromises between supported features and reliability.

I definitely believe that.

Hence, for the moment I would tend to maintain the current TLS code as much as possible, while perhaps experimenting privately with pem, then upgrade to a new logic/library only once that's been extensively tested.

From my point of view, all the complex parts of the TLS code are already working perfectly. The certificate chain handling from this PR just a tiny bonus part from a technical point of view -- trivial in comparison to what is already there. I can't see it turning out to be hard to maintain.

[albertosottile]

@Tremolo4 Syncplay now ships pem, as we used it to mitigate #554. Feel free to reconsider this PR basing on this library, if you are still interested in this feature.