Various TLS changes

[Res260]

See #192.

• Enable TLS 1.2 support between MITM and RDP Server
• Lock pyopenssl version to avoid breaking changes
• Disable TLS 1.3 for both client MITM and server MITM to avoid "cannot decrypt after the fact" scenarios.

I believe the current code has probably been copied without thinking about this argument, but it limits the TLS versions that PyRDP can use for TLS.

For context: for the TLS 1.3 disable: #116

I have tested on my machine, but further investigation is required. If something is not clear hmu.

Fixes #192. Workaround for #116.

[Res260]

After further reading:

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=784153

SSLv23_METHOD is the wildcard tls method and will use any available TLS version in OpenSSL. This probably should stay like this for CLIENT --> MITM. Thoughts @obilodeau ? I'll add a comment because this constant name is pure garbage.

[Res260]

Alright ready for review now

[Res260]

Small change: Lock PyOpenSSL version to avoid breaking changes ( https://www.pyopenssl.org/en/stable/changelog.html ) (despite https://www.pyopenssl.org/en/stable/backward-compatibility.html )

[Res260]

Last change (i, too, gotta sleep): Disable TLS 1.3 (see main comment for context).

[alxbl]

I don't think we should close #116 by disabling support for TLS 1.3. There will come a time where it will become necessary. I agree with disabling TLS 1.3 negotiation while we don't properly support it, but I don't feel that's a fix, only a workaround until we can handle 1.3 properly.

Other than not closing #116, this looks good to me.

[Res260]

Yeah I wasnt sure what was the best approach, I thought we should open a new issue for TLS 1.3 support which would be an enhancement issue instead of a bug-ish issue. I can remove the fixes statement though no problem

Edit I see you already edited the comment haha

[Res260]

@obilodeau do you want to review or I can merge?

[obilodeau]

@Res260 reviewing now

[obilodeau]

Looking good. Minor comments. I'll let you react or change then I'll merge.

Also, from now on, we should collect pcaps for TLS issues and add them as integration tests.

[Res260]

Also, from now on, we should collect pcaps for TLS issues and add them as integration tests.

This might require more work than it seems, as the current integration test purposely avoids TLS testing (because neither me nor @xshill used TLS with scapy).

[obilodeau]

This might require more work than it seems, as the current integration test purposely avoids TLS testing (because neither me nor @xshill used TLS with scapy).

Fair point. I missed that. Well, I'll try if we ever have a regression in the future and give up eventually then 😉. Let's not track that in an issue so we don't feel bad about ourselves 😄

[Res260]

Just thought about it, should this go in the changelogs?

[obilodeau]

Just thought about it, should this go in the changelogs?

I'm doing changelog updates after I merge in a separate push straight into master. This reduces the amount of conflict handling that we have to do.

It's not a problem if you provide it but it's not a problem if you don't. I've been using that process with a lot of success over at asciidoctor-reveal.js.