Add SChannel TLS 1.3 support #8419

wyattoday · 2022-02-09T16:39:07Z

Add SChannel TLS 1.3 support to curl / libcurl. This is a rework of the previous SChannel TLS 1.3 PR, built on curl-main, and finishing the remaining TODO.

This adds TLS 1.3 support for versions of Windows that support it at runtime. No-need for compile-time shenanigans.

Also, added a new define to disable optional code: DISABLE_SCHANNEL_CLIENT_CERT (reduces compiled code size if that feature is not needed in the vein of tinycurl).

SChannel currently only supports the following ciphers:

TLS_AES_256_GCM_SHA384
TLS_CHACHA20_POLY1305_SHA256
TLS_AES_128_GCM_SHA256

The following are not currently supported (but also not negotiated):

TLS_AES_128_CCM_8_SHA256
TLS_AES_128_CCM_SHA256

And TLS_CHACHA20_POLY1305_SHA256 is supported, but disabled by default in Windows (it can be enabled).

In addition to all the code required to add this, I've also modified the "SSL Ciphers" and "CURLOPT_TLS13_CIPHERS explained" docs to add the new options and document when the functionality was introduced.

lib/vtls/schannel.c

bagder · 2022-02-10T10:00:37Z

@jay @MarcelRaad please give this PR a glance!

lib/vtls/schannel.h

MarcelRaad · 2022-02-10T16:10:16Z

lib/vtls/schannel.c

+        break;
+      }
+      else { /* Windows 10 and older */
+        failf(data, "schannel: TLS 1.3 not supported on Windows prior to 11");


Hmm, could this be moved to where an actual error occurs? I like the clear error message, but if someone actually wants to use TLS 1.3 on Windows 10 or whatever other OS, why prevent them from doing so?

Well, SChannel is inherently tied to the version of Windows. And Windows 11 / Windows 2022 is when TLS 1.3 was introduced as a finished feature (yes, there were buggy registry-enabled versions in the later Windows 10 builds).

So, if a user explicitly requests TLS 1.3 on a version of Windows that doesn't support it then they will get an error ASAP.

However, the max TLS version will be set correctly at runtime (if not set by the user). See: https://github.com/curl/curl/pull/8419/files#diff-518f418eae7110255d54ce052e3f17412cfc2289fd2c31cded6fec437ee58333R201

If you're talking about Windows emulators, then that's a whole other bag of worms. But if they're emulating correctly (accurately) then they should have the exact same behavior as Windows.

lib/vtls/schannel.c

MarcelRaad · 2022-02-15T20:53:54Z

Looks good to me now except for the build break with Visual Studio 2008 (see AppVeyor), and I'd feel more comfortable if we had our MinGW CI on Azure Pipelines back up before merging.

jay · 2022-02-15T21:03:11Z

Looks good to me now except for the build break

We need to investigate why it's necessary to set pDisabledCrypto, that does not make sense to me. It seems as though the MS TLS 1.3 support is half baked if it requires explicitly disabling certain TLS 1.3 algorithms.

bagder · 2022-03-11T08:47:51Z

"This branch cannot be rebased due to conflicts"

Please rebase (and feel free to squash into fewer commits) and force-push to this branch.

wyattoday · 2022-03-11T11:25:38Z

@bagder Merged with upstream "master". Should be good to go.

bagder · 2022-03-11T12:04:55Z

We rebase and do a clean and "straight" commit history when we merge. So it needs to rebase without conflict.

wyattoday · 2022-03-11T14:03:46Z

Ok.

I gotta be honest, at our company we merge PRs with full history, so I have little experience with the git magic in re-writing the history. And the git docs are as clear as mud on this point.

So, if I have time at the end of today I'll work on it. Otherwise I'll work on it sometime next week.

jay · 2022-03-12T22:13:11Z

We need to investigate why it's necessary to set pDisabledCrypto, that does not make sense to me. It seems as though the MS TLS 1.3 support is half baked if it requires explicitly disabling certain TLS 1.3 algorithms.

I still think one of us familiar with Windows should investigate this before landing. Basically we need to know how Windows 10 and 11 TLS 1.3 behaves without explicitly disabling any ciphers.

gvollant · 2022-03-13T17:42:59Z

Microsoft distribute curl.exe with SChannel with Windows 10, pehaps they can help curl team !

wyattoday · 2022-03-22T11:03:47Z

@bagder @jay Should I do the work of squashing the commits so it can be merged? Or should I wait for more reviews by other users?

I'm getting mixed signals.

Does this work by adding TLS 1.3 to curl with SChannel? Yes. (We're using it in production right now).

Does this use all the publicly available documentation and implement it based on that? Also, yes.

Is the SChannel API a black-box with shitty public documentation? Also, yes.

jay · 2022-03-22T17:38:36Z

Please squash. If you run into squash errors that look time consuming then try working from a temporary branch based off of upstream/master.

git fetch upstream
git checkout -b temp upstream/master
git merge --squash schannel-ng
git commit # consolidate commit messages into one
git checkout schannel-ng
git reset --hard temp
git branch -D temp
git push -f origin

I'm assuming in this example that remotes upstream => curl/curl and origin => wyattoday/curl

wpodgorski · 2022-04-20T09:23:13Z

@wyattoday @jay any update on this :)?

gvollant · 2022-05-09T09:36:24Z

This will be wonderfull if this feature can be included in next version @wyattoday

wyattoday · 2022-05-28T10:52:24Z

Sorry, been super busy. I’ll get to this end of next week.

Andrei-Popov · 2022-06-04T02:44:32Z

We need to investigate why it's necessary to set pDisabledCrypto, that does not make sense to me. It seems as though the MS TLS 1.3 support is half baked if it requires explicitly disabling certain TLS 1.3 algorithms.

I own schannel. There is no need to disable algorithms. Unsupported cipher suites, etc., obviously, won't get negotiated.

wyattoday · 2022-06-04T02:59:16Z

@Andrei-Popov

I own schannel.

You work at MS?

There is no need to disable algorithms. Unsupported cipher suites, etc., obviously, won't get negotiated.

Do you happen to have a link (or a reference to any public documentation) that says as much?

The docs I’ve read are all public (obviously — I don’t work at MS) and suggest the opposite. But I would happy to be proven wrong (because it makes more sense for the way I’ve implemented it right now to be the wrong way).

It’s not that I don’t believe you, it’s just that it would better to have canonical sources that can be referred to rather than (a) guesses (b) referencing other source code examples {what I did here} or (c) lore passed down in mailing lists and issues trackers.

wyattoday · 2022-07-19T19:01:12Z

I fixed 2 bugs in the code since I said it was ready to merge.

Added the missing SSLSUPP_TLS13_CIPHERSUITES flag so curl knows you can pass in those ciphers.
Loop over all possible TLS 1.3 ciphers (rather than just the first 3) when passing in explicit ciphers.

Again, looks good to my eyes. It's being used in production here. It's ready for additional review and/or merging.

MarcelRaad · 2022-07-19T20:37:55Z

The Visual Studio 2008 and classic MinGW compilation failures are a problem. There were fixes for them in #8927.

wyattoday · 2022-07-20T16:29:48Z

@MarcelRaad Fixed the VS2008 / MingW issues. Passed all the compilation and relevant tests (there seems to be bugs in CI configuration on some tests unrelated to any changes in this PR).

Just squashed it into a single commit to be ready for merge with "main".

Any other comments or review are welcomed. Ready for merge otherwise.

…TLS13_CIPHERS` option).

wyattoday · 2022-07-22T14:48:59Z

Just updated the docs as well. So, if no further comments, this is ready to merge as-is without requiring any additional work.

The only thing to be aware of is that in docs/libcurl/opts/CURLOPT_TLS13_CIPHERS.3 I said it was "Added in 7.85.0 for SChannel.". This presumes this PR is merged for that release.

If that's not that case then that will have to be changed.

ttc0419 · 2022-07-28T03:12:14Z

@wyattoday @bagder Any comments for this merge request? Pretty excited to have this feature in git bash.

gvollant · 2022-07-30T07:31:57Z

For me, this PR is correct

MarcelRaad

I'm still not sure about the Windows version check if the user has explicitly requested TLS 1.3. I don't have a strong opinion about that though. Other than that, it looks good to me now!

jay · 2022-08-02T17:58:33Z

LGTM. CI failures are unrelated and being addressed in other PRs. Thanks!

gvollant · 2022-08-20T21:08:27Z

You work at MS?

Yes.

@Andrei-Popov can you ask people who package the c:\Windows\System32\curl.exe build packaged with Windows 11/2022 update it to curl 7.85 when it'll be released?

regards

The comment and PR history doesn't tell more or mention mingw version, so take a chance on this and assume it affected old mingw. My fresh copy of mingw-w64 does have an `ARRAYSIZE()` macro defined by Windows headers. Originally added in 8beff43 curl#8419

wyattoday mentioned this pull request Feb 9, 2022

Add SChannel TLS 1.3 support #7784

Closed

bagder added TLS Windows Windows-specific labels Feb 9, 2022

bagder reviewed Feb 10, 2022

View reviewed changes

lib/vtls/schannel.c Outdated Show resolved Hide resolved

lib/vtls/schannel.c Outdated Show resolved Hide resolved

lib/vtls/schannel.c Outdated Show resolved Hide resolved

lib/vtls/schannel.c Outdated Show resolved Hide resolved

MarcelRaad reviewed Feb 10, 2022

View reviewed changes

lib/vtls/schannel.h Outdated Show resolved Hide resolved

MarcelRaad reviewed Feb 10, 2022

View reviewed changes

lib/vtls/schannel.h Outdated Show resolved Hide resolved

MarcelRaad reviewed Feb 10, 2022

View reviewed changes

lib/vtls/schannel.c Outdated Show resolved Hide resolved

MarcelRaad added the feature-window A merge of this requires an open feature window label Feb 15, 2022

curl blocked Cmo1976 Mar 11, 2022

curl deleted a comment from Cmo1976 Mar 11, 2022

gvollant added a commit to gvollant/curl that referenced this pull request May 27, 2022

merged commit from Add SChannel TLS 1.3 support curl#8419 of wyattoday

e8f6ba3

This was referenced May 27, 2022

rebased Add SChannel TLS 1.3 support #8419 wyattoday/curl#1

Closed

merged commit from Add SChannel TLS 1.3 support #8419 of wyattoday #8927

Closed

wyattoday force-pushed the schannel-ng branch 2 times, most recently from 1dfa735 to ec17f79 Compare July 20, 2022 16:28

Add TLS 1.3 support for SChannel (including support for the `CURLOPT_…

e0d0e6a

…TLS13_CIPHERS` option).

wyattoday force-pushed the schannel-ng branch from ec17f79 to e0d0e6a Compare July 22, 2022 14:45

bagder requested review from jay and MarcelRaad August 2, 2022 14:44

MarcelRaad approved these changes Aug 2, 2022

View reviewed changes

jay approved these changes Aug 2, 2022

View reviewed changes

jay closed this in 8beff43 Aug 2, 2022

This was referenced Sep 6, 2022

Received HTTP/0.9 when not allowed with http2 feature alexcrichton/curl-rust#460

Closed

Windows nghttp2 + schannel + tls 1.3 error: Received HTTP/0.9 when not allowed #9451

Closed

bradking mentioned this pull request Nov 9, 2022

Windows schannel error SEC_E_DECRYPT_FAILURE with tls 1.3 support #9879

Closed

wyattoday mentioned this pull request Jun 8, 2023

Update tls13-ciphers.d to include Schannel. #11271

Closed

jjolidon mentioned this pull request Mar 15, 2024

curl/schannel error "SEC_E_ALGORITHM_MISMATCH " getting box from private registry hashicorp/vagrant#13365

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SChannel TLS 1.3 support #8419

Add SChannel TLS 1.3 support #8419

wyattoday commented Feb 9, 2022 •

edited

bagder commented Feb 10, 2022

MarcelRaad Feb 10, 2022

wyattoday Feb 10, 2022 •

edited

MarcelRaad commented Feb 15, 2022 •

edited

jay commented Feb 15, 2022

bagder commented Mar 11, 2022

wyattoday commented Mar 11, 2022 •

edited

bagder commented Mar 11, 2022

wyattoday commented Mar 11, 2022

jay commented Mar 12, 2022

gvollant commented Mar 13, 2022

wyattoday commented Mar 22, 2022 •

edited

jay commented Mar 22, 2022

wpodgorski commented Apr 20, 2022

gvollant commented May 9, 2022

wyattoday commented May 28, 2022

Andrei-Popov commented Jun 4, 2022

wyattoday commented Jun 4, 2022 •

edited

wyattoday commented Jul 19, 2022

MarcelRaad commented Jul 19, 2022

wyattoday commented Jul 20, 2022 •

edited

wyattoday commented Jul 22, 2022

ttc0419 commented Jul 28, 2022

gvollant commented Jul 30, 2022

MarcelRaad left a comment

jay commented Aug 2, 2022

gvollant commented Aug 20, 2022

Add SChannel TLS 1.3 support #8419

Add SChannel TLS 1.3 support #8419

Conversation

wyattoday commented Feb 9, 2022 • edited

bagder commented Feb 10, 2022

MarcelRaad Feb 10, 2022

Choose a reason for hiding this comment

wyattoday Feb 10, 2022 • edited

Choose a reason for hiding this comment

MarcelRaad commented Feb 15, 2022 • edited

jay commented Feb 15, 2022

bagder commented Mar 11, 2022

wyattoday commented Mar 11, 2022 • edited

bagder commented Mar 11, 2022

wyattoday commented Mar 11, 2022

jay commented Mar 12, 2022

gvollant commented Mar 13, 2022

wyattoday commented Mar 22, 2022 • edited

jay commented Mar 22, 2022

wpodgorski commented Apr 20, 2022

gvollant commented May 9, 2022

wyattoday commented May 28, 2022

Andrei-Popov commented Jun 4, 2022

wyattoday commented Jun 4, 2022 • edited

wyattoday commented Jul 19, 2022

MarcelRaad commented Jul 19, 2022

wyattoday commented Jul 20, 2022 • edited

wyattoday commented Jul 22, 2022

ttc0419 commented Jul 28, 2022

gvollant commented Jul 30, 2022

MarcelRaad left a comment

Choose a reason for hiding this comment

jay commented Aug 2, 2022

gvollant commented Aug 20, 2022

wyattoday commented Feb 9, 2022 •

edited

wyattoday Feb 10, 2022 •

edited

MarcelRaad commented Feb 15, 2022 •

edited

wyattoday commented Mar 11, 2022 •

edited

wyattoday commented Mar 22, 2022 •

edited

wyattoday commented Jun 4, 2022 •

edited

wyattoday commented Jul 20, 2022 •

edited