git: Always use OpenSSL on win32 #3554

HybridEidolon · 2024-04-21T18:10:43Z

By default, libssh2 uses Windows Cryptography Next Generation when targeting win32. The wincng backend does not support ED25519, which is a widely-used algorithm among git hosting services, and in some cases may be the only option on remotes in certain configurations.

This change adds the feature openssl-on-win32, enabling the same one in libssh2-sys, and enables it in the CI release flow, ensuring that release builds are able to communicate with hosts using ED25519.

Fixes #3322

Checklist

If applicable:

I have updated CHANGELOG.md
I have updated the documentation (README.md, docs/, demos/)
(N/A) I have updated the config schema (cli/src/config-schema.json)
(N/A) I have added tests to cover my changes

google-cla · 2024-04-21T18:10:47Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

HybridEidolon · 2024-04-21T18:12:34Z

I updated my CLA signature.

martinvonz

Thanks! Hopefully someone who uses Windows can review and test this. If we don't hear from anyone in a few days, I can add a rubberstamp to get this in.

FYI, there's also draft PR #3191 for switching from libssh2 to OpenSSH.

CHANGELOG.md

gulbanana · 2024-04-22T08:43:40Z

CHANGELOG.md

@@ -64,6 +69,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
  literals. This means that `snapshot.max-new-file-size="1"` and
  `snapshot.max-new-file-size=1` are now equivalent.

+* ED25519 host keys are now supported correctly when connecting to Git+SSH hosts


I'm hopeful that this will actually fix several other git ssh issues, but I guess we shouldn't get ahead of ourselves :)

gulbanana · 2024-04-22T08:45:02Z

Cargo.toml

@@ -60,6 +60,7 @@ indoc = "2.0.4"
 insta = { version = "1.38.0", features = ["filters"] }
 itertools = "0.12.1"
 libc = { version = "0.2.153" }
+libssh2-sys = { version = "0.3.0" }


This dependency could be made Windows-only by moving it to a [target.'cfg(windows)'.dependencies] section.

This isn't in the dependencies key; it's in the workspace.dependencies key. I tried target.'cfg(windows)'.workspace.dependencies and it said it was not allowed.

cli/Cargo.toml

jhechtf · 2024-04-22T19:20:39Z

I use windows, let me pull this down and give it a look.

EDIT: Build fails for me, is there something more I need to do?

steveklabnik · 2024-04-22T20:45:28Z

This change necessitates Windows users building jj to install openssl via vcpkg for the target triple x64-windows-static-md when building for MSVC, or otherwise have OpenSSL available at build time.

FWIW, this can be a huge pain. I really, really would prefer not to use openssl unless I have to, and I don't need the features in this PR. I think it might be nicer to have the option to use openssl if you want these extra features for some extra pain in building, but that's just my own personal opinion :)

khionu · 2024-04-22T23:52:55Z

Maybe this could be a feature flag?

HybridEidolon · 2024-04-23T00:34:02Z

Maybe this could be a feature flag?

It is technically already one, but enabled during release builds in CI.

This change necessitates Windows users building jj to install openssl via vcpkg for the target triple x64-windows-static-md when building for MSVC, or otherwise have OpenSSL available at build time.

FWIW, this can be a huge pain. ...

I hadn't actually updated the PR text yet; it now uses the vendored-openssl feature that's already turned on for release builds for Windows and macOS, so it's not disruptive and vcpkg is only needed if you specifically enable openssl-on-win32 and disable vendored-openssl.

To my knowledge, OpenSSL is only used for the cryptographic primitives in libssh2 and not for TLS relative stuff.

jhechtf · 2024-04-27T21:43:25Z

Clarifying question for me; would this allow me to use jj git push on windows now? I can't do it because my ID key for github is an ed2559 key so i have to manually do git push and it's a bit annoying.

HybridEidolon · 2024-04-28T04:25:19Z

Clarifying question for me; would this allow me to use jj git push on windows now? I can't do it because my ID key for github is an ed2559 key so i have to manually do git push and it's a bit annoying.

Yes. The issue goes both for your own key and the host's, I believe.

Zoybean · 2024-05-07T05:09:31Z

I'm on windows, tried using this but I can't get openssl-sys to compile. It needs the openssl libs and headers to be available, but I think I only have the libs as part of my git-for-windows install.

(looks like github ate my last comment. this was 2nd attempt)

HybridEidolon · 2024-05-08T01:44:30Z

I'm on windows, tried using this but I can't get openssl-sys to compile. It needs the openssl libs and headers to be available, but I think I only have the libs as part of my git-for-windows install.

You can use the vendored-openssl feature to compile it statically. This is normally turned on in CI for release builds already.

farnoy · 2024-05-08T10:31:21Z

I tried testing it with $ cargo install --locked --bin jj --git https://github.com/HybridEidolon/jj --branch push-pklwzruzswky jj-cli --features vendored-openssl,openssl-on-win32 --force but it fails on vcpkg when compiling openssl-sys. I thought this was supposed to work without having openssl installed on the system?

HybridEidolon · 2024-05-08T19:54:27Z

but it fails on vcpkg when compiling openssl-sys. I thought this was supposed to work without having openssl installed on the system?

It is; the vendored-openssl feature is never supposed to invoke vcpkg. Per the openssl crate documentation,

If the vendored Cargo feature is enabled, the openssl-src crate will be used to compile and statically link to a copy of OpenSSL. The build process requires a C compiler, perl (and perl-core), and make.

Zoybean · 2024-05-09T08:37:45Z

the vendored-openssl feature is never supposed to invoke vcpkg. Per the openssl crate documentation,

Not sure if having the log helps, but Installing the latest version (c59e79d) with: cargo install --path cli --locked --features packaging,openssl-on-win32,vendored-openssl, I get the following:

error: failed to run custom build command for `openssl-sys v0.9.101`

Caused by:
  process didn't exit successfully: `C:\..redacted..\jj\target\release\build\openssl-sys-88af3904954fd47d\build-script-main` (exit code: 101)
  --- stdout
  cargo:rerun-if-env-changed=X86_64_PC_WINDOWS_MSVC_OPENSSL_LIB_DIR
  X86_64_PC_WINDOWS_MSVC_OPENSSL_LIB_DIR unset
  cargo:rerun-if-env-changed=OPENSSL_LIB_DIR
  OPENSSL_LIB_DIR unset
  cargo:rerun-if-env-changed=X86_64_PC_WINDOWS_MSVC_OPENSSL_INCLUDE_DIR
  X86_64_PC_WINDOWS_MSVC_OPENSSL_INCLUDE_DIR unset
  cargo:rerun-if-env-changed=OPENSSL_INCLUDE_DIR
  OPENSSL_INCLUDE_DIR unset
  cargo:rerun-if-env-changed=X86_64_PC_WINDOWS_MSVC_OPENSSL_DIR
  X86_64_PC_WINDOWS_MSVC_OPENSSL_DIR unset
  cargo:rerun-if-env-changed=OPENSSL_DIR
  OPENSSL_DIR unset
  note: vcpkg did not find openssl: Could not find Vcpkg tree: No vcpkg installation found. Set the VCPKG_ROOT environment variable or run 'vcpkg integrate install'

  --- stderr
  thread 'main' panicked at C:\..redacted..\.cargo\registry\src\index.crates.io-6f17d22bba15001f\openssl-sys-0.9.101\build\find_normal.rs:190:5:


  Could not find directory of OpenSSL installation, and this `-sys` crate cannot
  proceed without this knowledge. If OpenSSL is installed and this crate had
  trouble finding it,  you can set the `OPENSSL_DIR` environment variable for the
  compilation process.

  Make sure you also have the development packages of openssl installed.
  For example, `libssl-dev` on Ubuntu or `openssl-devel` on Fedora.

  If you're in a situation where you think the directory *should* be found
  automatically, please open a bug at https://github.com/sfackler/rust-openssl
  and include information about your system as well as this message.

  $HOST = x86_64-pc-windows-msvc
  $TARGET = x86_64-pc-windows-msvc
  openssl-sys = 0.9.101


  It looks like you're compiling for MSVC but we couldn't detect an OpenSSL
  installation. If there isn't one installed then you can try the rust-openssl
  README for more information about how to download precompiled binaries of
  OpenSSL:

  https://github.com/sfackler/rust-openssl#windows


  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

HybridEidolon · 2024-05-10T18:13:35Z

Not sure if having the log helps, but Installing the latest version (c59e79d) with: cargo install --path cli --locked --features packaging,openssl-on-win32,vendored-openssl, I get the following:

Could you try building it after checking out the commit with cargo build instead? I'm not sure what is happening here, but I am building for MSVC locally in a clean context and I don't see this at all.

Zoybean · 2024-05-11T10:08:43Z

building cli at c59e79d:

cargo build --locked --features packaging,openssl-on-win32,vendored-openssl fails
cargo build --locked --features openssl-on-win32,vendored-openssl fails
cargo build --locked --features openssl-on-win32 fails
cargo build --locked and no features succeeds of course

All the failures had what appeared to be the same error message, though I didn't bother to run them through diff.
(I forget why I added the packaging feature in the first place but it looks like it doesn't affect this error)

HybridEidolon · 2024-05-11T18:04:25Z

Oh boy, building openssl-sys with the vendored feature only seems to work under msys2 shell on Windows.

Huh...

By default, libssh2 uses Windows Cryptography Next Generation when targeting win32. The wincng backend does not support ED25519, which is a widely-used algorithm among git hosting services, and in some cases may be the only option on remotes in certain configurations. This change adds the feature `openssl-on-win32`, enabling the same one in `libssh2-sys`, and enables it in the CI release flow, ensuring that release builds are able to communicate with hosts using ED25519. Fixes jj-vcs#3322

gulbanana · 2024-05-12T13:56:28Z

Aha. I was also using msys2 - the version that comes with Git for Windows as "Git Bash".

Zoybean · 2024-05-13T01:14:52Z

If it helps, I've been building under nushell, but I get the same failure when running in git bash. I don't have an explicit msys2 install.

HybridEidolon · 2024-05-14T20:41:43Z

I guess I could alter this to not run in CI release packaging, since it does work, it just requires a specific build environment? All it does is forward the feature down to the libssh-sys crate ultimately. I do find it's the only way to make jj useful in my specific use case.

jhechtf · 2024-05-14T22:19:54Z

I would definitely like for jj git push to work on Windows for me, but AFAIK the only way to install it is from source, and I can never seem to get this pr to build.

ErichDonGubler · 2024-05-29T17:39:40Z

I was able to build on Windows 11 locally without using MSYS2 using these commands (in Nushell/sh-ish syntax, rather than cmd or PowerShell, but it should work in cmd equivalents, too):

# Get a copy of `vcpkg.exe`.
$ gh repo clone microsoft/vcpkg $VCPKG_SRC_DIR
<snip>
$ mkdir $VCPKG_BIN_DIR
$ cd $VCPKG_BIN_DIR
$ $VCPKG_SRC_DIR\bootstrap-vcpkg.bat # downloads `vcpkg.exe`
<snip>

# Install OpenSSL.
$ .\vcpkg.exe install openssl:x64-windows-static-md
<snip>

# Test a build!
$ VCPKG_ROOT=$VCPKG_BIN_DIR cargo run --release --features openssl-on-win32 -- --version
<snip>
jj 0.17.1-89fc5743d112c49d60d15e1c8684dfa9e3d242e4

…so this build seems reproducible, just not without some expertise.

We may be able to use vcpkg in CI with an action like https://github.com/marketplace/actions/run-vcpkg.

ErichDonGubler · 2024-05-29T17:43:10Z

Hmm, I get this seemingly incorrect hint while testing jj git push using the build I mentioned in my previous comment:

Hint: Jujutsu uses libssh2, which doesn't respect ~/.ssh/config. Does `ssh -F /dev/null` to the host work?

I say "incorrect" because this build/PR actually uses OpenSSH under the hood, right?

Full output, for the curious.

I had tried to push to this PR, which I expected to fail with a permissions error:

VCPKG_ROOT=E:\vcpkg cargo run --release --features openssl-on-win32 -- git push
    Finished `release` profile [optimized] target(s) in 0.33s
     Running `target\release\jj.exe git push`
Branch changes to push to origin:
  Add branch push-pklwzruzswky to 89fc5743d112
Error: ERROR: Permission to martinvonz/jj.git denied to ErichDonGubler.
; class=Ssh (23); code=Eof (-20)
Hint: Jujutsu uses libssh2, which doesn't respect ~/.ssh/config. Does `ssh -F /dev/null` to the host work?
error: process didn't exit successfully: `target\release\jj.exe git push` (exit code: 1)

HybridEidolon force-pushed the push-pklwzruzswky branch from 9359703 to c6a5c8e Compare April 21, 2024 22:58

martinvonz reviewed Apr 21, 2024

View reviewed changes

HybridEidolon force-pushed the push-pklwzruzswky branch from c6a5c8e to 811bf5d Compare April 21, 2024 23:30

HybridEidolon mentioned this pull request Apr 21, 2024

SSH: Use OpenSSH instead of libssh2 for authentication with Git hosts #3191

Draft

4 tasks

gulbanana approved these changes Apr 22, 2024

View reviewed changes

HybridEidolon force-pushed the push-pklwzruzswky branch 4 times, most recently from fb9ef1c to 0c15b40 Compare April 27, 2024 21:33

HybridEidolon force-pushed the push-pklwzruzswky branch from 0c15b40 to 6efb2e4 Compare May 2, 2024 00:58

HybridEidolon force-pushed the push-pklwzruzswky branch from 6efb2e4 to c59e79d Compare May 8, 2024 02:03

HybridEidolon force-pushed the push-pklwzruzswky branch from c59e79d to 89fc574 Compare May 12, 2024 06:22

thoughtpolice mentioned this pull request May 15, 2024

EXPERIMENT: build jujutsu with buck2 #1997

Draft

6 tasks

ilyagr added the 🪟Windows label Jun 9, 2024

miscoined mentioned this pull request Nov 8, 2024

Unable to connect to Sourcehut over SSH on Windows with JJ #4802

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

git: Always use OpenSSL on win32 #3554

git: Always use OpenSSL on win32 #3554

HybridEidolon commented Apr 21, 2024 •

edited

Loading

google-cla bot commented Apr 21, 2024

HybridEidolon commented Apr 21, 2024

martinvonz left a comment

gulbanana Apr 22, 2024

gulbanana Apr 22, 2024

HybridEidolon May 8, 2024

jhechtf commented Apr 22, 2024 •

edited

Loading

steveklabnik commented Apr 22, 2024

khionu commented Apr 22, 2024

HybridEidolon commented Apr 23, 2024

jhechtf commented Apr 27, 2024

HybridEidolon commented Apr 28, 2024

Zoybean commented May 7, 2024

HybridEidolon commented May 8, 2024

farnoy commented May 8, 2024

HybridEidolon commented May 8, 2024

Zoybean commented May 9, 2024

HybridEidolon commented May 10, 2024

Zoybean commented May 11, 2024

HybridEidolon commented May 11, 2024

gulbanana commented May 12, 2024

Zoybean commented May 13, 2024

HybridEidolon commented May 14, 2024

jhechtf commented May 14, 2024 •

edited

Loading

ErichDonGubler commented May 29, 2024 •

edited

Loading

ErichDonGubler commented May 29, 2024 •

edited

Loading

git: Always use OpenSSL on win32 #3554

Are you sure you want to change the base?

git: Always use OpenSSL on win32 #3554

Conversation

HybridEidolon commented Apr 21, 2024 • edited Loading

Checklist

google-cla bot commented Apr 21, 2024

HybridEidolon commented Apr 21, 2024

martinvonz left a comment

Choose a reason for hiding this comment

gulbanana Apr 22, 2024

Choose a reason for hiding this comment

gulbanana Apr 22, 2024

Choose a reason for hiding this comment

HybridEidolon May 8, 2024

Choose a reason for hiding this comment

jhechtf commented Apr 22, 2024 • edited Loading

steveklabnik commented Apr 22, 2024

khionu commented Apr 22, 2024

HybridEidolon commented Apr 23, 2024

jhechtf commented Apr 27, 2024

HybridEidolon commented Apr 28, 2024

Zoybean commented May 7, 2024

HybridEidolon commented May 8, 2024

farnoy commented May 8, 2024

HybridEidolon commented May 8, 2024

Zoybean commented May 9, 2024

HybridEidolon commented May 10, 2024

Zoybean commented May 11, 2024

HybridEidolon commented May 11, 2024

gulbanana commented May 12, 2024

Zoybean commented May 13, 2024

HybridEidolon commented May 14, 2024

jhechtf commented May 14, 2024 • edited Loading

ErichDonGubler commented May 29, 2024 • edited Loading

ErichDonGubler commented May 29, 2024 • edited Loading

HybridEidolon commented Apr 21, 2024 •

edited

Loading

jhechtf commented Apr 22, 2024 •

edited

Loading

jhechtf commented May 14, 2024 •

edited

Loading

ErichDonGubler commented May 29, 2024 •

edited

Loading

ErichDonGubler commented May 29, 2024 •

edited

Loading