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

Expose the cargo crate feature: vendored-openssl. #99

Merged
merged 2 commits into from
Apr 17, 2020

Conversation

anderejd
Copy link
Contributor

@anderejd anderejd commented Apr 8, 2020

Fixes: #97

Is this an acceptable solution? For macOS this seems like the right choice to me. If this PR passes the CI build I'm fine with using the vendored-openssl feature for all platforms.

@anderejd anderejd changed the title Using te cargo crate feature vendored-openssl. Use the cargo crate feature vendored-openssl. Apr 8, 2020
@anderejd
Copy link
Contributor Author

anderejd commented Apr 8, 2020

If this PR is merged then #69 will be even more important since this PR increases compile times quite a bit.

@Shnatsel
Copy link

Shnatsel commented Apr 8, 2020

I am generally averse to bundling OpenSSL because security vulnerabilities in it are quite common, and using a vendored version leaves the user without security updates.

@anderejd
Copy link
Contributor Author

anderejd commented Apr 8, 2020

Good point, is this true for macOS as well?

Homebrew is the most common way to install openssl as far as I know on macOS and that feels to me like a security issue as well. I don't know anything about the security track record for Homebrew, but I personally do not trust it and do not want it in my system.

I suspect that the official cargo build installed through rustup is using vendored openssl as well, since that executable of cargo works, but I can't compile it on macOS without the vendored-openssl feature enabled. Do you know if the official cargo is also built using the vendored-openssl feature?

@anderejd
Copy link
Contributor Author

anderejd commented Apr 8, 2020

Maybe we need to wait for this to land on stable before trying to use different default features for different targets: rust-lang/cargo#7914 I'm not sure.

@anderejd
Copy link
Contributor Author

anderejd commented Apr 8, 2020

Would adding the new feature, but not any default feature and documenting the two ways of installing/building be an acceptable solution?

Adding something like these examples to the readme:

Try to find and use a system installed OpenSSL:
cargo install cargo-geiger

Build and statically link OpenSSL:
cargo install cargo-geiger --features cargo-vendored-openssl

@tarcieri
Copy link
Collaborator

tarcieri commented Apr 8, 2020

FWIW, we added a similar feature to the rustsec crate (off-by-default) specifically to address macOS (without Homebrew): https://github.com/RustSec/rustsec-crate/pull/130

Also added documentation in README.md about installing with and without this
feature.
@anderejd
Copy link
Contributor Author

anderejd commented Apr 8, 2020

I renamed the feature to vendored-openssl, the same name as the feature in cargo and the same as in rustsec. The feature is now off by default.

@anderejd anderejd changed the title Use the cargo crate feature vendored-openssl. Expose the cargo crate feature: vendored-openssl. Apr 8, 2020
@anderejd
Copy link
Contributor Author

@Shnatsel Are your concerns addressed by the optional cargo feature flag?

@Shnatsel
Copy link

Yes, it is addressed.

If Cargo itself uses bundled OpenSSL, it's OK to do so by default in cargo-geiger as well. However, ideally I'd prefer to have both controlled by an off-by-default feature.

@tarcieri
Copy link
Collaborator

As far as I know it's off-by-default for both, and mainly useful for Macs

@anderejd
Copy link
Contributor Author

The new feature added in this PR is now off-by-default, macOS users without Homebrew will need to follow the readme and use the custom install command that enables the new cargo feature.

I'm not sure about how the official cargo distribution does it, but that cargo executable runs on my macOS without any Homebrew or custom OpenSSL in the system.

I will merge this later today and publish a new version to crates.io.

@anderejd anderejd merged commit c84a5f1 into geiger-rs:master Apr 17, 2020
@HenkPoley
Copy link

On Debian & Ubuntu you have the fight between two versions of libssl{-dev,1.0-dev}, for the past two years... For example meaning you can't have PHP(-dev) and NPM installed at the same time.

https://bugs.launchpad.net/ubuntu/+source/nodejs/+bug/1794589

Statically linked OpenSSL is useful for that too. 🎉🤦‍♂️ (Thanks for implementing)

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

Successfully merging this pull request may close these issues.

Fail to link libssh on cargo install
4 participants