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

Rustls w/ aws-lc-rs on Windows requires NASM #1913

Open
Diggsey opened this issue Apr 22, 2024 · 31 comments
Open

Rustls w/ aws-lc-rs on Windows requires NASM #1913

Diggsey opened this issue Apr 22, 2024 · 31 comments

Comments

@Diggsey
Copy link

Diggsey commented Apr 22, 2024

The entire reason half the ecosystem is using rustls rather than openssl is because it "just works" without any dependencies.

Having upgraded to the latest version, I get the following error:

error: failed to run custom build command for aws-lc-sys v0.15.0
...
Missing dependency: nasm

It's very frustrating that aws-lc has been added to the default dependency list as many intermediate dependencies forget to add "default-features = false", making it in practice a hard dependency of rustls.

Please reconsider making aws-lc the default backend. The default backend should require fewer dependencies, not more. Any minor performance difference is a secondary concern.

@djc
Copy link
Member

djc commented Apr 22, 2024

Which version of aws-lc-rs do you have in your tree? AIUI aws-lc-rs 1.7 requires fewer extra dependencies than before on the most common platforms at least.

@Diggsey
Copy link
Author

Diggsey commented Apr 22, 2024

1.7

@Diggsey
Copy link
Author

Diggsey commented Apr 22, 2024

This is on 64-bit windows using MSVC toolchain, so a fairly standard platform, and NASM is not listed as a required dependency anywhere that I could see.

@ctz
Copy link
Member

ctz commented Apr 22, 2024

and NASM is not listed as a required dependency anywhere that I could see.

https://aws.github.io/aws-lc-rs/requirements/

Heard you on the frustration with additional dependencies, especially on Windows. See aws/aws-lc-rs#364 upstream for the specific NASM requirement.

@cpu
Copy link
Member

cpu commented Apr 22, 2024

AIUI aws-lc-rs 1.7 requires fewer extra dependencies than before

Indeed, but I think there's still a ways to go for Windows in particular w.r.t the nasm requirement.

Hopefully these improvements continue!

NASM is not listed as a required dependency anywhere that I could see.

The 0.23.0 release notes that brought the change to aws-lc-rs as default mentions it: "Note that this has some implications on platform support and build-time tool requirements such as cmake on all platforms and nasm on Windows."

Since then the cmake requirement has been relaxed in many cases but nasm is still req'd on Windows.

@Diggsey
Copy link
Author

Diggsey commented Apr 22, 2024

Please also consider users who are installing tools via cargo install. Now they are expected to find this build requirement from https://aws.github.io/aws-lc-rs/requirements/, download a random ZIP file from https://www.nasm.us/pub/nasm/releasebuilds/?C=M;O=D, and then go an manually configure an environment variable to point to the installed files?

This change has a huge negative impact across the whole Rust ecosystem. Please revert to using ring, or other pure rust implementation by default.

@Rigidity
Copy link

Rigidity commented Apr 22, 2024

Agreed that this is a negative change, though in the meantime for my use case at least I was able to get around this with default-features = false - maybe this will help.

@DaOneLuna

This comment has been minimized.

@Rigidity

This comment has been minimized.

@Diggsey
Copy link
Author

Diggsey commented Apr 22, 2024

I was able to get around this with default-features = false - maybe this will help.

Unfortunately that doesn't help when intermediate dependencies don't specify this which is the case for eg. ureq and others. (Unsurprising since this crate didn't used to have default features afaik)

@algesten
Copy link

FWIW, I don't really know how to deal with this situation in ureq. I absolutely need to support windows users and rustls has so far been brilliant in how little dependencies and complexity it introduced.

In ureq we also want as few dependencies as possible, however rustls-webpki still seem to use ring as its default provider, which means I either need to override the default feature flags or accept that I get both ring and aws-lc-rs as deps. I can obviously solve that with feature flags, but as a library author, I prefer not to do that, since every feature flag I require on ureq's deps takes away some choice for the user of ureq.

For now I hope I can just stall on upgrading rustls until the dust settles, but so far life seemed simpler when it was just ring…

@djc
Copy link
Member

djc commented Apr 23, 2024

Why do you have a direct dependency on rustls-webpki? In either case, it seems like you can simply use default-features = false for your rustls-webpki dependency. For that matter, you could declare both your rustls and rustls-webpki dependency with default-features = false and then add back the crypto provider you prefer (sounds like it might be ring).

@algesten
Copy link

Why do you have a direct dependency on rustls-webpki?

LOL. I don't know! Just realize it seems to work without it 🤦 … Thanks!

For that matter, you could declare both your rustls and rustls-webpki dependency with default-features = false and then add back the crypto provider you prefer (sounds like it might be ring).

Sure, I can do that, but that also means I take a stand for/against some default choices rustls has made for me, and you are the experts on this. From my perspective FIPS is an enterprise compliance requirement – the kind of entities that can afford to spend more time tinkering with their builds.

I understand aws-lc-rs is required for FIPS, and there is a small performance gain, but are there more reasons rustls decided to change the default? Maybe there's a discussion somewhere I missed?

@djc
Copy link
Member

djc commented Apr 23, 2024

I understand aws-lc-rs is required for FIPS, and there is a small performance gain, but are there more reasons rustls decided to change the default? Maybe there's a discussion somewhere I missed?

We have found aws-lc-rs has a number of features that ring does not support, and upstream is generally more responsive/moving faster. Things like P-521 support, RSA keygen support, better support for SEC-1 keys, support for the PQC KYBER key exchange mechanism, support for HPKE (which is a prerequisite for Encrypted ClientHello support) are now supported in aws-lc-rs but not in ring (even though there have been long-standing feature requests). We've also seen fairly rapid support in the build system requirement from aws-lc-rs (and note that ring effectively ships pre-built binary code for Windows to avoid these issues -- which has other problems).

Due to this issue we've been having a discussion about how to better communicate the advantages of the aws-lc-rs choice, and this is something we expect to work on soon, but please assume the choice is well-motivated.

That said, the more cumbersome build process is a downside for the time being. IMO for your minimal-dependencies use case it is fine to stick with ring for the time being -- and I don't think you're the only library making that choice.

@algesten
Copy link

@djc thank you for elaborating!

but please assume the choice is well-motivated.

I apologize if I came across as questioning your motives. That was not my intent.

That said, the more cumbersome build process is a downside for the time being. IMO for your minimal-dependencies use case it is fine to stick with ring for the time being -- and I don't think you're the only library making that choice.

Sounds good! I'll make a PR to that today. Thank you!

@Diggsey
Copy link
Author

Diggsey commented Apr 23, 2024

We have found aws-lc-rs has a number of features that ring does not support

That's great as a motivation for users to enable the aws-lc feature in order to get access to the new functionality, but IMO it's not a rationale for making aws-lc required by default.

We've also seen fairly rapid support in the build system requirement from aws-lc-rs

That's great, and when the build system requirements are as low as ring, then it might make sense to switch.

(and note that ring effectively ships pre-built binary code for Windows to avoid these issues -- which has other problems).

It's fairly normal to ship binary code on Windows. The source can still be reviewed and the build can be reproduced, so I don't see a huge difference from a security perspective (and most people aren't reviewing the source code before running cargo install anyway...)

@cpu cpu changed the title Rustls no longer builds without additional dependencies Rustls w/ aws-lc-rs on Windows requires NASM Apr 23, 2024
@cpu
Copy link
Member

cpu commented Apr 23, 2024

but IMO it's not a rationale for making aws-lc required by default.

Diggsey: It sounds like you're prioritizing minimal build time requirements as the most important factor in a cryptography provider dependency. That's reasonable and we hear you, but I don't think you've made a case for why that should be the default prioritization for everyone.

As @djc outlined there are several concurrent factors that make aws-lc-rs appealing as a default even though for Windows users it requires an additional build dependency. The fact that several classes of users all have their own justifiable prioritizations (build reqs, performance, post-quantum ciphersuites, FIPS mode, national ciphersuites, a pure Rust impl (note: not ring), etc) is why the backend is now selectable. There's no one provider that could make everyone happy.

I think it's expected there will be some bumps in the road short-term that make it harder to opt-in to using *ring* and not aws-lc-rs than it should be, such as with ureq, but these are problems that can and are being solved. I think the best path forward is to make that choice ergonomic. Concurrently the challenges from build tool requirements will continue to get better with time.

since this crate didn't used to have default features afaik

This crate has had default features for a very long time. At least back to 0.10.0.

@Diggsey
Copy link
Author

Diggsey commented Apr 23, 2024

It sounds like you're prioritizing minimal build time requirements as the most important factor in a cryptography provider dependency. That's reasonable and we hear you, but I don't think you've made a case for why that should be the default prioritization for everyone.

Rustls is not just any cryptography provider - we already had the openssl crate which worked just fine. What made rustls special was precisely the minimal build dependencies.

but I don't think you've made a case for why that should be the default prioritization for everyone.

It seems pretty clear to me: opting into aws-lc is easy, but opting out is essentially impossible when it's provided under a default feature. Therefore the question is not whether to prioritise build requirements vs say performance, but whether to prioritize giving people a choice vs not giving them the choice.

@djc
Copy link
Member

djc commented Apr 24, 2024

It sounds like you're prioritizing minimal build time requirements as the most important factor in a cryptography provider dependency. That's reasonable and we hear you, but I don't think you've made a case for why that should be the default prioritization for everyone.

Rustls is not just any cryptography provider - we already had the openssl crate which worked just fine. What made rustls special was precisely the minimal build dependencies.

I hardly think that's the only thing (or even necessarily most important) thing that makes rustls an attractive option.

but I don't think you've made a case for why that should be the default prioritization for everyone.

It seems pretty clear to me: opting into aws-lc is easy, but opting out is essentially impossible when it's provided under a default feature. Therefore the question is not whether to prioritise build requirements vs say performance, but whether to prioritize giving people a choice vs not giving them the choice.

It's not "essentially impossible" -- get the intermediate libraries to fix their feature usage, and/or advocate for/contribute to Cargo efforts to improve general handling of the feature situation.

@Diggsey
Copy link
Author

Diggsey commented Apr 24, 2024

I hardly think that's the only thing (or even necessarily most important) thing that makes rustls an attractive option.

Have you surveyed your users? I promise you, the only thing most people care about is having a TLS library that just works.

To quote someone else, who phrases it better than I can:

I always thought Rustls ambition was to provide a "pure Rust" TLS implementation. The original README said "Rustls is a modern TLS library written in Rust". However, I clearly read things into that which was never stated.

With ring, it was of course never pure (it's a mix of asm/C/Rust) and from my limited understanding of crypto, many of the optimizations required for modern crypto primitives are hand written assembly, which will take a long time, if ever, to replace with any Rust equivalent (or at least the asm! macro). However I used to believe Rustls ambition was to aim towards a pure Rust TLS library.

This is the way a lot of people feel, myself included, and the way rustls has been portrayed.

Also

It's not "essentially impossible" -- get the intermediate libraries to fix their feature usage, and/or advocate for/contribute to Cargo efforts to improve general handling of the feature situation.

As a user, those things are not in my control. hence "essentially impossible". Sure, I can go and fork every intermediate crate to fix the feature usage - is that practical? No.

@cpu
Copy link
Member

cpu commented Apr 24, 2024

If your goal is a true "pure Rust" TLS implementation top-to-bottom then it seems as though the recent work has improved Rustls for you. Unlike before when the cryptography provider was not a choice you can now build rustls without aws-lc-rs or *ring* so that a cryptography provider like rustls-rustcrypto can be used. Unlike *ring*, aws-lc-rs, BoringSSL, or any other OpenSSL derived cryptography libraries (they all share the same common heredity) it includes only Rust code. A world where ring is the implied default does not meet the goal of pure Rust and so you will still require intermediate dependencies adjust to the "no default" world so you can slot in your pure Rust choice. I continue to believe helping those intermediate deps. make this ergonomic is the best solution both for you, and our other users.

In general it seems like this is becoming an unproductive conversation. I would characterize your attitude as rather combative from the outset and the goal posts keep being moved as we explain our decisions. It's clear you're unhappy with the decision and you've made the reasons why understood. Perhaps we should call that a conclusion and let this thread lie unless there are new perspectives to weigh?

@Diggsey
Copy link
Author

Diggsey commented Apr 24, 2024

If your goal is a true "pure Rust" TLS implementation top-to-bottom

That's not my goal, I'm saying that the name "rustls" has given users the (apparantly false) impression that this library was striving to provide a TLS implementation with fewer external dependencies that what we had before with openssl, and that this is the main reason this library ever became popular with users.

I would characterize your attitude as rather combative from the outset

Yes, I'm upset that yet again someone has decided to break the world for no reason (the new features would work perfectly well under a feature flag) and expects everyone else to fix it.

the goal posts keep being moved as we explain our decisions

I don't think I've moved the goalposts at all? Prior versions of this crate just worked with a standard Rust install, new versions don't. That's bad.

Perhaps we should call that a conclusion and let this thread lie unless there are new perspectives to weigh?

Sure, at least until the maintainers of all 728 direct dependencies of this crate - https://crates.io/crates/rustls/reverse_dependencies - start receiving complaints from all the users of those crates asking why the new version no longer builds correctly. Or when the person who's not even a Rust programmer tries to cargo install their favourite tool and now gets an error because of something called nasm? or cmake?

@gimbling-away
Copy link

gimbling-away commented Apr 25, 2024

Why use nasm specifically? Major compiler toolchains already include an assembler (gcc, clang, msvc with masm64) why can't that be used to prevent an external dependency?

Or even more portabley, using inline asm?

@djc
Copy link
Member

djc commented Apr 25, 2024

@gimbling-away this is a question for the aws-lc-rs folks, please follow-up with them upstream:

aws/aws-lc-rs#364
aws/aws-lc#1477

@Diggsey

This comment was marked as off-topic.

@izolyomi
Copy link

izolyomi commented May 2, 2024

I've also just hit this issue trying to cargo install tools depending on rustls. I fully agree that requiring extra binaries installed for a default build is bad choice in general.

@clairewood
Copy link

Here's what fixed the nasm errors for me, in case this helps anyone else:

[dependencies]
ring = "0.17.8"
rustls = { version = "0.22.4", default-features = false }
webpki = "0.22.4"
webpki-roots = "0.26.1"

@goenning
Copy link

goenning commented May 9, 2024

@clairewood I think you fixed it because on version 0.22.4 aws-lc is not used

I have this on my cargo.toml:

rustls = { version = "=0.23.5", default-features = false, features = ["ring"]}

However, aws-lc-rs is still present on the lock file.

[[package]]
name = "rustls"
version = "0.23.5"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "afabcee0551bd1aa3e18e5adbf2c0544722014b899adb31bd186ec638d3da97e"
dependencies = [
 "aws-lc-rs",
 "log",
 "once_cell",
 "ring",
 "rustls-pki-types",
 "rustls-webpki",
 "subtle",
 "zeroize",
]

Anyone know why?

@djc
Copy link
Member

djc commented May 9, 2024

Anyone know why?

Other crates in your dependency graph might be pulling in rustls with default features? Or maybe rust-lang/cargo#10801?

@justsmth
Copy link

This is an issue that is being discussed, but we want to be cautious.

Ring seems to get around this requirement by pregenerating/embedding the NASM object files into the crate:

❯ cargo download ring > ring.tar.gz
INFO: cargo-download v0.1.2
INFO: Latest version of crate ring=* is 0.17.8
INFO: Crate `ring==0.17.8` downloaded successfully

❯ tar ztf ring.tar.gz | egrep '\.o$'
ring-0.17.8/pregenerated/aesni-gcm-x86_64-nasm.o
ring-0.17.8/pregenerated/aesni-x86-win32n.o
ring-0.17.8/pregenerated/aesni-x86_64-nasm.o
ring-0.17.8/pregenerated/chacha-x86-win32n.o
ring-0.17.8/pregenerated/chacha-x86_64-nasm.o
ring-0.17.8/pregenerated/chacha20_poly1305_x86_64-nasm.o
ring-0.17.8/pregenerated/ghash-x86-win32n.o
ring-0.17.8/pregenerated/ghash-x86_64-nasm.o
ring-0.17.8/pregenerated/p256-x86_64-asm-nasm.o
ring-0.17.8/pregenerated/sha256-x86_64-nasm.o
ring-0.17.8/pregenerated/sha512-x86_64-nasm.o
ring-0.17.8/pregenerated/vpaes-x86-win32n.o
ring-0.17.8/pregenerated/vpaes-x86_64-nasm.o
ring-0.17.8/pregenerated/x86-mont-win32n.o
ring-0.17.8/pregenerated/x86_64-mont-nasm.o
ring-0.17.8/pregenerated/x86_64-mont5-nasm.o

@decorahensisire

This comment has been minimized.

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

No branches or pull requests