kanidm / webauthn-rs

An implementation of webauthn components for Rustlang servers
Mozilla Public License 2.0
488 stars 80 forks source link

Update outdated OpenSSL vendoring doc #416

Closed WesleyAC closed 7 months ago

WesleyAC commented 7 months ago

The upstream openssl-src crate now supports v3, so this works correctly.

Fixes #

Firstyear commented 7 months ago

@micolous should check this too IMO.

WesleyAC commented 7 months ago

That makes sense about feature unification. I still think the docs should be changed, although if "you can use" is not sufficient for you, I'm not sure exactly what you want — I would hope that people using this library understand their constraints well enough to know whether vendoring OpenSSL is a good choice for them.

micolous commented 7 months ago

Also, in order to make this work "as expected", we will need to bump our version of the openssl dependency to one which vendors against OpenSSL 3.x.

While this isn't necessary to allow vendoring, I've written https://github.com/kanidm/webauthn-rs/pull/418 which will explicitly fail the build on OpenSSL 1.x (vendored or not) for all of webauthn-rs (not just webauthn-authenticator-rs).

I still think the docs should be changed, although if "you can use" is not sufficient for you, I'm not sure exactly what you want — I would hope that people using this library understand their constraints well enough to know whether vendoring OpenSSL is a good choice for them.

We're a big fan of making it hard to shoot yourself in the foot, and vendoring through feature flags is a great way to shoot yourself in the foot – and worse is that it's not immediately obvious. 😄

While vendoring has some legitimate use cases, they need to be carefully managed, especially for a security-critical library like OpenSSL, which also sits outside the Rust ecosystem.

When vendoring for redistributable binaries, it is disingenuous to suggest that simply "enabling" a feature flag to get vendoring is the end of the story - when you start shipping others' software as dependencies of your own, you need to manage the software supply chain. Handling that properly means you're either:

By contrast, a vendored feature flag gives no control over which version of OpenSSL ships. You'll just get "a" version. Who knows if it's up to date.

This brings me to the other major use case I've seen for vendoring – it's for developers who haven't set up their build environment properly, and they were unable (or unwilling) to actually fix the issue properly, so that technical debt has been outsourced to the maintainers of those bindings.

While I acknowledge that in OpenSSL's case it isn't the easiest thing to get going (especially for a novice), that's really a problem for rust-openssl's build scripts and documentation - and it's something I've been sending PRs upstream for. 😄

Unfortunately, this also means the priority for a vendored feature flag is to make sure that those developers have a "working" build, even if that means shipping outdated and insecure libraries. As an example, the openssl crate was blocked on moving to OpenSSL 3 due to developers like that.

At the end of the day, these are all issues for the openssl crate to sort out, and vendoring (through feature flags or proper other means) doesn't require code changes from us to support. While there is a lot of documentation about the openssl crate here, it's a stop-gap until PRs get accepted upstream to document things better and resolve issues.

There are wider issues with vendoring in the Rust ecosystem (especially silent auto-vendoring), but thankfully webauthn-rs doesn't have to deal with any of those directly. 😄

PS: Bringing in that flag to webauthn-rs means we'll need to double up on our CI targets to check that vendoring always works correctly with all the different feature configurations. I think we have enough targets already... 😅

Firstyear commented 7 months ago

As a shorter response, I think that vendoring is up to the leaf-binary, not a library. As @micolous has pointed out, you don't need webauthn-rs to have a vendoring flag due to https://doc.rust-lang.org/cargo/reference/features.html#feature-unification . So I don't think we need to add our own vendor flag when there are already ways for you to enable this externally.

The bar to clear here is "how is webauthn-rs having a vendor flag, superior to cargo's existing feature unification options".

WesleyAC commented 7 months ago

I've changed this PR to simply remove the outdated comment in OpenSSL.md about the vendored version of OpenSSL that the Rust openssl crate provides.

Personally, I think it'd be good to add a note in that file that the vendored feature is a option that is available to people, albeit one that should be considered carefully. I can write something about that up if you are open to including it, otherwise I'll leave this PR as is.

Firstyear commented 7 months ago

I'm okay with a readme/doc note about the topic

WesleyAC commented 7 months ago

Updated this PR with a note about the minimum required openssl crate version to get v3, as well as a note to consider whether vendoring OpenSSL is the right choice.

micolous commented 7 months ago

Yup, that seems reasonable, thanks!