open-pr: avoid using system curl (because it can't do TLS v1.3) #24

dscho · 2023-02-02T20:49:44Z

The curl executable in C:\Windows\system32 exclusively uses Secure Channel as TLS backend. This is usually fine, except when one needs to talk to a TLS v1.3-only host and one is stuck with Windows prior to Windows 11 (where Secure Channel simply does not support TLS v1.3).

This happens to be exactly the case that caused the workflow run to fail that wanted to open a PR for OpenSSH v9.2p1.

Let's just use our own MINGW curl.exe instead.

dscho · 2023-02-02T22:02:11Z

This is the successful run that opened git-for-windows/MSYS2-packages#80: https://github.com/git-for-windows/git-for-windows-automation/actions/runs/4079002007/jobs/7029893303

rimrul · 2023-02-03T06:51:24Z

The commit message of baa5ab4 is a little inacurate. The windows-latest images are currently Windows Server 2022, where schannel does support TLS 1.3 by default, but the system curl is v7.83.1 and curl only introduced support for TLS 1.3 via the schannel backend in v7.85.0

.github/workflows/open-pr.yml

rimrul · 2023-02-03T09:42:03Z

.github/workflows/open-pr.yml

+        env:
+          cache-name: cache-g4w-sdk
+        with:
+          path: .sdk


When does the cache action save the cache?

It's the post checkout step that is implied by actions/cache.

That's not ideal. It's caching the .sdk directory after we already cloned the target repo, which could cause collisions when opening PRs for multiple packages in the same repo. Maybe we should use actions/cache/restore and actions/cache/save to explicitely create the cache after the sparse checkout?

Valid point.

I changed it. To prove that it works, I ran a dry-run that did not find a cached artifact and successfully cached it, followed by a dry-run that found the cache and successfully avoided re-checking out and re-caching it.

That `curl` version uses Secure Channel internally, which supports TLS v1.3 only since version 7.85.0, and this support requires Windows 11 (see https://curl.se/changes.html#7_85_0). While we do run the workflow in `windows-latest` hosted runners, which means Windows Server 2022, the Server equivalent to Windows 11, the `curl.exe` in that Windows version seems to be stuck at v7.83.1. That is a problem because we seem to be unable to access OpenSSH's CloudFlare CDN via anything below TLS v1.3, and this GitHub workflow runs in Windows 10 agents. The symptom looks like this: ==> Retrieving sources... -> Downloading openssh-9.2_P1.tar.gz... % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0 0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0 curl: (35) schannel: next InitializeSecurityContext failed: SEC_E_UNSUPPORTED_FUNCTION (0x80090302) - The function requested is not supported ==> ERROR: Failure while downloading https://cloudflare.cdn.openbsd.org/pub/OpenBSD/OpenSSH/portable/openssh-9.2_P1.tar.gz Aborting... ==> ERROR: Failed to generate new checksums Error: Process completed with exit code 1. So let's use Git's own `curl.exe` (which can use OpenSSL, which in turn has no problems speaking TLS v1.3). We do insist on using `/mingw64/bin/curl.exe` because `/mingw64/bin/libcurl-4.dll` is already included in the `minimal` SDK artifact (to allow `git fetch`/`git push` via HTTPS), whereas if we were to use `/usr/bin/curl.exe` would require at least `/usr/bin/msys-curl-4.dll` (and who knows what other dependencies) that are not included in the `minimal` SDK. Signed-off-by: Johannes Schindelin <[email protected]>

We actually need something different than the `minimal` SDK (which is used in git/git's CI builds to build Git and run its test suite): we need `updpkgsums` and friends. So far, we abused the Action to initialize the minimal SDK and then called `curl` plenty a time to download the remaining files. It would appear that that downloading part ran afoul of rate limits at least in one instance. Let's just use a partial clone to begin with. Signed-off-by: Johannes Schindelin <[email protected]>

dscho · 2023-02-03T13:31:05Z

@rimrul thank you for your excellent review! I've fixed the issues you pointed out:

range-diff

1: baa5ab4 ! 1: c6fb563 open-pr: avoid using C:\Windows\system32\curl.exe

@@ Metadata
  ## Commit message ##
     open-pr: avoid using C:\Windows\system32\curl.exe
 
-    That `curl` version uses Secure Channel internally, which is limited to
-    TLS v1.2 on Windows versions before Windows 11.
+    That `curl` version uses Secure Channel internally, which supports TLS
+    v1.3 only since version 7.85.0, and this support requires Windows 11
+    (see https://curl.se/changes.html#7_85_0). While we do run the workflow
+    in `windows-latest` hosted runners, which means Windows Server 2022, the
+    Server equivalent to Windows 11, the `curl.exe` in that Windows version
+    seems to be stuck at v7.83.1.
 
     That is a problem because we seem to be unable to access OpenSSH's
     CloudFlare CDN via anything below TLS v1.3, and this GitHub workflow
@@ Commit message
     So let's use Git's own `curl.exe` (which can use OpenSSL, which in turn
     has no problems speaking TLS v1.3).
 
+    We do insist on using `/mingw64/bin/curl.exe` because
+    `/mingw64/bin/libcurl-4.dll` is already included in the `minimal` SDK
+    artifact (to allow `git fetch`/`git push` via HTTPS), whereas if we were
+    to use `/usr/bin/curl.exe` would require at least
+    `/usr/bin/msys-curl-4.dll` (and who knows what other dependencies) that
+    are not included in the `minimal` SDK.
+
     Signed-off-by: Johannes Schindelin <[email protected]>
 
  ## .github/workflows/open-pr.yml ##

2: f76696f = 2: 6bcba04 open-pr: avoid using the setup-git-for-windows-sdk Action

3: 31f9a18 ! 3: 3b65b81 open-pr: cache the git-sdk-64 subset

@@ Commit message
     source archive, and something in `update-scripts/` needs to be added or
     to be adjusted.
 
+    The "cache git-sdk-64 subset" step is modeled after
+    https://github.com/actions/cache/#example-cache-workflow and caches the
+    git-sdk-64 subset in a `post` Action if there was no cache hit.
+
     Signed-off-by: Johannes Schindelin <[email protected]>
 
  ## .github/workflows/open-pr.yml ##
@@ .github/workflows/open-pr.yml: jobs:
 -            https://github.com/git-for-windows/git-sdk-64 .tmp
 +            https://github.com/git-for-windows/git-sdk-64 .tmp &&
 +          echo "rev=$(git -C .tmp rev-parse HEAD)" >>$GITHUB_OUTPUT
-+      - name: Cache git-sdk-64 subset
++      - name: cache git-sdk-64 subset
 +        id: cache-g4w-sdk
 +        uses: actions/cache@v3
 +        env:

4: 8ec1887 = 4: a858612 open-pr: special-case OpenSSH's version strings

Caching this instead of re-checking out all the time is a big time saver. In particular when something failed such as downloading the source archive, and something in `update-scripts/` needs to be added or to be adjusted. Instead of using the `actions/cache` Action as-is, we use the `restore` and `save` sub-Actions for tighter control (i.e. to avoid caching the artifact when the clone failed due to problems outside of our control such as network glitches). Signed-off-by: Johannes Schindelin <[email protected]>

The release announcments use a different version string than the file name; Let's automagically transform the former into the latter. Signed-off-by: Johannes Schindelin <[email protected]>

dscho · 2023-02-06T14:48:58Z

@rimrul could you give this another review?

rimrul · 2023-02-06T14:57:45Z

LGTM

dscho force-pushed the dont-use-system-curl branch 5 times, most recently from dad8ede to f76696f Compare February 2, 2023 21:35

dscho marked this pull request as ready for review February 2, 2023 22:01

dscho requested a review from rimrul February 2, 2023 22:01

dscho self-assigned this Feb 2, 2023

rimrul reviewed Feb 3, 2023

View reviewed changes

dscho added 2 commits February 3, 2023 14:22

dscho force-pushed the dont-use-system-curl branch from 8ec1887 to a858612 Compare February 3, 2023 13:29

dscho added 2 commits February 3, 2023 22:05

open-pr: special-case OpenSSH's version strings

e077d68

The release announcments use a different version string than the file name; Let's automagically transform the former into the latter. Signed-off-by: Johannes Schindelin <[email protected]>

dscho force-pushed the dont-use-system-curl branch 2 times, most recently from f4ef67e to e077d68 Compare February 3, 2023 21:19

dscho merged commit c701662 into main Feb 6, 2023

dscho deleted the dont-use-system-curl branch February 6, 2023 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

open-pr: avoid using system curl (because it can't do TLS v1.3) #24

open-pr: avoid using system curl (because it can't do TLS v1.3) #24

Uh oh!

dscho commented Feb 2, 2023 •

edited

Loading

Uh oh!

dscho commented Feb 2, 2023

Uh oh!

rimrul commented Feb 3, 2023

Uh oh!

Uh oh!

rimrul Feb 3, 2023

Uh oh!

dscho Feb 3, 2023

Uh oh!

rimrul Feb 3, 2023

Uh oh!

dscho Feb 3, 2023 •

edited

Loading

Uh oh!

dscho commented Feb 3, 2023

Uh oh!

dscho commented Feb 6, 2023

Uh oh!

rimrul commented Feb 6, 2023

Uh oh!

Uh oh!

open-pr: avoid using system curl (because it can't do TLS v1.3) #24

open-pr: avoid using system curl (because it can't do TLS v1.3) #24

Uh oh!

Conversation

dscho commented Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dscho commented Feb 2, 2023

Uh oh!

rimrul commented Feb 3, 2023

Uh oh!

Uh oh!

rimrul Feb 3, 2023

Choose a reason for hiding this comment

Uh oh!

dscho Feb 3, 2023

Choose a reason for hiding this comment

Uh oh!

rimrul Feb 3, 2023

Choose a reason for hiding this comment

Uh oh!

dscho Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dscho commented Feb 3, 2023

Uh oh!

dscho commented Feb 6, 2023

Uh oh!

rimrul commented Feb 6, 2023

Uh oh!

Uh oh!

dscho commented Feb 2, 2023 •

edited

Loading

dscho Feb 3, 2023 •

edited

Loading