Add support for TLS v1.3 #86
Conversation
0c55af6 to
8607472
Compare
|
Okay, now it is ready to review :) |
There was a problem hiding this comment.
Thanks a lot for this very useful improvement.
Very good job an extending the unit tests so they also test that this actually does handle TLS v1.3 (I assume it does so :-)).
Please address my comments though.
If you don't have the time or don't want to make these changes yourself then I'll be happy to do so myself when time permits. Your contribution is much appreciated no matter what.
|
There are conflicting codes. But the code in mainline doesn't seem to pass CI. Do I need to wait for the mainline code to be fixed first? |
As far as I can tell the errors were related to a problem with the Windows images used by all Github CI actions, not just this repository, so you should be able to fix the conflicts and rebase without any issues now. |
Unit test works fine. But coverage and coverity task failed :( |
The coverage and coverity jobs require access tokens which are only available on my github repository. I assume you could generate your own accounts on codecov and coverity if you really wanted to, but that'd hardly be worth it. Just ignore that for now and please focus on addressing the issues I've pointed out instead. The coverage and coverity jobs are only being run on the master branch anyway so they won't stop this PR from being merged. |
Okay, it;s ready for review |
There was a problem hiding this comment.
Sorry for being late with this review.
It looks very good. Please address my last comments and then I think this could be merged.
I also made some changes to the master branch so the coverity and coverage jobs should no runner long on forks of this repository. You made me aware of that issue so thanks a lot for that as well. You might want to rebase your fork on this repos master branch.
| DWORD cTlsParameters; | ||
| PTLS_PARAMETERS pTlsParameters; | ||
| } SCH_CREDENTIALS, * PSCH_CREDENTIALS; | ||
|
|
There was a problem hiding this comment.
You mentioned that these structs only needed to be defined when building on some older versions of Windows.
Could you please move them to an appropriately named header file and then only include that file if we are building on a version of Windows where they are needed?
There was a problem hiding this comment.
Already moved to sspi_compat.hpp
|
Looks great. Only one minor comment left then I think should be ready to merge and I should probably also make a new release with this. Thanks a lot. |
| #define WINTLS_DETAIL_SSPI_COMPAT_HPP | ||
|
|
||
| // for SCH_CREDENTIALS | ||
| #define SCHANNEL_USE_BLACKLISTS |
There was a problem hiding this comment.
To play nicer with clients I think this should be guarded by an ifdef similar to what is done with SECURITY_WIN32 and UNICODE in sspi_types.h so it keeps being either defined or not after this header has been included, eg. guard it with WINTLS_SCHANNEL_USE_BLACKLISTS and undefine it if it wasn't already defined.
|
Seems like I forgot to actually submit my final comment. After that I think we should merge this. Thanks. |
TLS v1.3 is available on windows 10.0.17763 and above version.
tls_extension currently only supports the supported_versions extension.
For compatibility purposes, it is not possible to directly check the version in handshake to determine if TLSv1.3 is currently in use. The best practice in the RFC is to check the supported_versions extension to see if it supports TLSv1.3.
|
Great and thanks a lot. Sorry for the slow response time. |
TLS v1.3 is available on windows 10.0.17763 and above version. #83
The associated unittest needs further modification, I'll upload it later.