Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SChannel TLS 1.3 support #8419

Closed
wants to merge 1 commit into from
Closed

Conversation

wyattoday
Copy link
Contributor

@wyattoday wyattoday commented Feb 9, 2022

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.

@bagder bagder added TLS Windows Windows-specific labels Feb 9, 2022
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
@bagder
Copy link
Member

bagder commented Feb 10, 2022

@jay @MarcelRaad please give this PR a glance!

lib/vtls/schannel.h Outdated Show resolved Hide resolved
lib/vtls/schannel.h Outdated Show resolved Hide resolved
break;
}
else { /* Windows 10 and older */
failf(data, "schannel: TLS 1.3 not supported on Windows prior to 11");
Copy link
Member

@MarcelRaad MarcelRaad Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@wyattoday wyattoday Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
@MarcelRaad
Copy link
Member

MarcelRaad commented Feb 15, 2022

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
Copy link
Member

jay commented Feb 15, 2022

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
Copy link
Member

bagder commented Mar 11, 2022

"This branch cannot be rebased due to conflicts"

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

@curl curl deleted a comment from Cmo1976 Mar 11, 2022
@wyattoday
Copy link
Contributor Author

wyattoday commented Mar 11, 2022

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

@bagder
Copy link
Member

bagder commented Mar 11, 2022

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

@wyattoday
Copy link
Contributor Author

wyattoday commented Mar 11, 2022

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
Copy link
Member

jay commented Mar 12, 2022

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
Copy link
Contributor

gvollant commented Mar 13, 2022

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

@wyattoday
Copy link
Contributor Author

wyattoday commented Mar 22, 2022

@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
Copy link
Member

jay commented Mar 22, 2022

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
Copy link

wpodgorski commented Apr 20, 2022

@wyattoday @jay any update on this :)?

@gvollant
Copy link
Contributor

gvollant commented May 9, 2022

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

@wyattoday
Copy link
Contributor Author

wyattoday commented May 28, 2022

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

@Andrei-Popov
Copy link

Andrei-Popov commented Jun 4, 2022

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
Copy link
Contributor Author

wyattoday commented Jun 4, 2022

@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.

@gvollant
Copy link
Contributor

gvollant commented Jul 8, 2022

But otherwise, sounds good. I'll rework the code to not disable any "by default" and instead let the user select which they want to be disabled (regardless of whether they're enabled in SChannel or not).

Now, the feature windows is open. Do you have time for doing this modification?
regards

@wyattoday
Copy link
Contributor Author

wyattoday commented Jul 8, 2022

I've made the changes locally. I'll push them as a single commit after we've tested thoroughly on Monday or Tuesday.

@wyattoday
Copy link
Contributor Author

wyattoday commented Jul 14, 2022

I've pushed all the previous work, plus the changes based on new information from @Andrei-Popov . Everything is up-to-date with latest commits from "main". And all my work is squished into 1 commit.

We've been using it & testing it internally for the past few days.

Let me know if there are any other comments or issues.

Edit: of course there are style errors. I'll fix them and push them tomorrow. Please let me know if there are any other changes recommended besides the style errors found by CI.

@bagder
Copy link
Member

bagder commented Jul 14, 2022

./vtls/schannel.c:812:16: warning: if with space (SPACEBEFOREPAREN)
         else if (strcmp("TLS_AES_128_CCM_8_SHA256", tmp) == 0)
                ^
./vtls/schannel.c:814:16: warning: if with space (SPACEBEFOREPAREN)
         else if (strcmp("TLS_AES_128_CCM_SHA256", tmp) == 0)
                ^

Also: we typically write those checks like if(!strcmp())...

Should the checks be done case sensitive or insensitive?

@wyattoday
Copy link
Contributor Author

wyattoday commented Jul 15, 2022

Also: we typically write those checks like if(!strcmp())...

OK, I can change that.

Should the checks be done case sensitive or insensitive?

Other ciphers in the SChannel code are done case sensitive, so I was just following the precedent.

@wyattoday
Copy link
Contributor Author

wyattoday commented Jul 15, 2022

Pushed the code change recommended by @bagder, and fixed the CI-style issue found yesterday.

The only CI failures are configuration related (FreeBSD 12.3/13.2, "Windows 64-bit shared/release Schannel/SSPI/WinIDN/libssh2") or because the correct headers were not installed with a very old compiler ("curl (windows msys v1_mingw_schannel)").

To my eyes it looks ready to merge. Let me know if anyone has any comments. I can set aside some time today and Monday for additional modifications based on review.

@wyattoday
Copy link
Contributor Author

wyattoday commented Jul 19, 2022

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

  1. Added the missing SSLSUPP_TLS13_CIPHERSUITES flag so curl knows you can pass in those ciphers.
  2. 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
Copy link
Member

MarcelRaad commented Jul 19, 2022

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

@wyattoday wyattoday force-pushed the schannel-ng branch 2 times, most recently from 1dfa735 to ec17f79 Compare Jul 20, 2022
@wyattoday
Copy link
Contributor Author

wyattoday commented Jul 20, 2022

@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.

@wyattoday
Copy link
Contributor Author

wyattoday commented Jul 22, 2022

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.

@Galaxy0419
Copy link

Galaxy0419 commented Jul 28, 2022

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

@gvollant
Copy link
Contributor

gvollant commented Jul 30, 2022

For me, this PR is correct

@bagder bagder requested review from jay and MarcelRaad Aug 2, 2022
Copy link
Member

@MarcelRaad MarcelRaad left a comment

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
jay approved these changes Aug 2, 2022
@jay
Copy link
Member

jay commented Aug 2, 2022

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

@jay jay closed this in 8beff43 Aug 2, 2022
@gvollant
Copy link
Contributor

gvollant commented Aug 20, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants