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.

Sign up for GitHub

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

Jump to bottom

dependency crates-index with vendored-openssl feature fails with an SSL error #272

Closed
tonowak opened this issue Jan 8, 2023 · 20 comments
Closed

dependency crates-index with vendored-openssl feature fails with an SSL error #272

tonowak opened this issue Jan 8, 2023 · 20 comments
Labels
C-bug Category: doesn't meet expectations

Comments

@tonowak
Copy link
Collaborator

tonowak commented Jan 8, 2023

Steps to reproduce the bug with the above code

cargo semver-checks check-release

Actual Behaviour

Updating index
Error: SSL error: unknown error; class=Ssl (16)

Caused by:
SSL error: unknown error; class=Ssl (16)

Expected Behaviour

It should work.

Generated System Information

Software version

cargo-semver-checks 0.12.1

Operating system

Linux 6.1.1-arch1-1

Command-line

/home/tonowak/.cargo/bin/cargo-semver-checks semver-checks --bugreport

cargo nightly version

> cargo +nightly -V
cargo 1.68.0-nightly (c994a4a63 2022-12-18)

Compile time information

  • Profile: release
  • Target triple: x86_64-unknown-linux-gnu
  • Family: unix
  • OS: linux
  • Architecture: x86_64
  • Pointer width: 64
  • Endian: little
  • CPU features: fxsr,llvm14-builtins-abi,sse,sse2
  • Host: x86_64-unknown-linux-gnu

Additional Context

I've asked people and tried other machines and it seems to not work only on my laptop.

The issues happens exactly here:

cargo-semver-checks/src/baseline.rs

Line 187 in f6b41f4

while need_retry(index.update())? {

The tool works after removing vendored-openssl from the list of features.

Further inspection of the bug requires going into crates-index code and debugging it there. I'll do that in some free time -- this issue is a tracking issue for the bug.
@tonowak tonowak added the C-bug Category: doesn't meet expectations label Jan 8, 2023
@obi1kenobi
Copy link
Owner

obi1kenobi commented Jan 8, 2023 via email

@tonowak
Copy link
Collaborator Author

tonowak commented Jan 8, 2023

Oh, that might be a good idea. From what I remember, vendored SSL is used, because there was some problem in the CI and using it was the easiest solution, right?

@obi1kenobi
Copy link
Owner

obi1kenobi commented Jan 8, 2023 via email

@tonowak
Copy link
Collaborator Author

tonowak commented Jan 8, 2023

That won't fix the underlying problem and it won't prevent an user from encountering the same bug, but I'm happy with such a solution.

@obi1kenobi
Copy link
Owner

obi1kenobi commented Jan 8, 2023 via email

@tonowak tonowak mentioned this issue Jan 9, 2023
Open
@tonowak tonowak mentioned this issue Jan 17, 2023
Open
@staniewzki
Copy link
Collaborator

I have also just run into this exact error

@tonowak
Copy link
Collaborator Author

tonowak commented Jan 22, 2023 •
edited
Loading

Interesting. Probably you haven't changed your local build setup nor the Rust version, so it might be related with the packages in the distribution. Now that there are two of us with this issue, there is a high chance that other users of the tool will also stumble on this bug -- that's bad news.

Edit: oh wait, it was the non-vendored version that used the openssl already present in the system. I don't know anymore how to reproduce this bug.

@wcampbell0x2a
Copy link

I'm also having this issue on Arch Linux:

$ uname -r -s
Linux 6.1.7-arch1-1
$ cargo --version
cargo 1.68.0-nightly (a5d47a725 2023-01-16)
$ cargo semver-checks check-release
    Updating index
Error: the server did not provide a certificate; class=Ssl (16)

Caused by:
    the server did not provide a certificate; class=Ssl (16)
$ cargo semver-checks check-release --version
cargo-semver-checks-check-release 0.16.1

@obi1kenobi
Copy link
Owner

@epage have you by any chance seen this sort of error before? Based on the printout, this is happening somewhere in git2 via crates-index and based on @tonowak's tests it seems to be related to whether OpenSSL is vendored in or not. The error message blames the server, but "GitHub's servers have misconfigured SSL" seems like a way too bold a claim with the evidence we have so far. Any advice?

@epage
Copy link
Collaborator

epage commented Jan 24, 2023

THis is all with the latest git2, right?

If so, then it is likely related to actually checking SSL certificates. My first lines of investigation would be whether crates-index is fetching using https or ssh and the second is if people running into this issue remapped their remotes to turn crates-index https fetches into ssh fetches.

@wcampbell0x2a
Copy link

I have used the following, but it's currently commented out:

$ bat ~/.cargo/config --plain
#[registry]
#crates-io.protocol = "sparse"

Looking at the connection using wireshark, the client is sending a TCP RST after trying to start a TLS session.

@obi1kenobi
Copy link
Owner

cargo-semver-checks 0.16.1 has latest git2. I think remapping remotes to turn https into ssh fetches will fail with a different symptom and error message — we have an open issue for it: #295

@wcampbell0x2a does the client receive the "server hello" part of the TLS handshake, or does it RST before that? If there's a "server hello," can you check what cert is included / whether it's valid?

@epage
Copy link
Collaborator

epage commented Jan 24, 2023

Last I knew, crates-index did not support the sparse registry protocol.

@wcampbell0x2a
Copy link

2023-01-23-224354_1261x500_scrot

@obi1kenobi
Copy link
Owner

Fascinating, thanks for sharing it!

If it's no trouble, I'd love to see the contents of the "Change Cipher Spec" and "Application Data" packets going in both directions between the "Server Hello" and the RST. This looks likely to be some sort of incompatibility between the vendored OpenSSL and the GitHub server, and I'd love to give the upstream maintainers as much data as possible in the bug report.

If it's easier for you, feel free to attach the packet dump file corresponding to the screenshot — I don't mind digging into it on my own.

Again, very much appreciate your help in diagnosing this unfortunate issue.

@tonowak tonowak mentioned this issue Jan 25, 2023
Merged
@obi1kenobi obi1kenobi mentioned this issue Jan 28, 2023
Open
@tonowak
Copy link
Collaborator Author

tonowak commented Jan 30, 2023

I cannot reproduce this issue anymore -- probably updating the packages in my Arch solved the issue. Nevertheless #324 has been merged and released, thus it is now possible to use the openssl installed on the system (instead of the vendored one) through cargo install cargo-semver-checks --locked --no-default-features. @wcampbell0x2a can you still reproduce this issue / does this temporary fix work for you?

@tonowak
Copy link
Collaborator Author

tonowak commented Jan 31, 2023

Actually, this bug is no longer present since #318 has been merged. It seems that using a newer version of git2 fixed the issue -- I've confirmed this by running the tool on the commit just before #318 has been merged and just after. I'm closing the issue :)

@tonowak tonowak closed this as completed Jan 31, 2023
@wcampbell0x2a
Copy link

I cannot reproduce this issue anymore -- probably updating the packages in my Arch solved the issue. Nevertheless #324 has been merged and released, thus it is now possible to use the openssl installed on the system (instead of the vendored one) through cargo install cargo-semver-checks --locked --no-default-features. @wcampbell0x2a can you still reproduce this issue / does this temporary fix work for you?

Can confirm this works, thanks for the fix

@obi1kenobi
Copy link
Owner

Thanks for confirming and for your patience while we got to the bottom of this!

@tonowak
Copy link
Collaborator Author

tonowak commented Feb 2, 2023

Actually, this bug is no longer present since #318 has been merged. It seems that using a newer version of git2 fixed the issue -- I've confirmed this by running the tool on the commit just before #318 has been merged and just after. I'm closing the issue :)

Huh, somehow the issue is still present on my (other) machine. At least now I can also confirm that the feature works.

@DianaNites DianaNites mentioned this issue Jul 14, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-bug Category: doesn't meet expectations
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@epage @obi1kenobi @wcampbell0x2a @staniewzki @tonowak