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

Update outdated OpenSSL vendoring doc #416

Merged
merged 1 commit into from
Feb 25, 2024
Merged

Conversation

WesleyAC
Copy link
Contributor

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

Fixes #

  • cargo test has been run and passes
  • documentation has been updated with relevant examples (if relevant)

Copy link
Member

@yaleman yaleman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather the feature be called openssl-vendored just from a naming convention thing... <target>-<modifier>.

@Firstyear
Copy link
Member

@micolous should check this too IMO.

Copy link
Collaborator

@micolous micolous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no reason to include this in webauthn-rs, because of Cargo feature unification.

If you would like to use vendoring, then your binary target should add a dependency on openssl with that feature.

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.

OpenSSL vendoring has a lot of caveats (on technical, certification and licensing grounds), so saying "you can use it" is not sufficient.

@WesleyAC
Copy link
Contributor Author

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
Copy link
Collaborator

micolous commented Feb 12, 2024

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 #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:

  • Depending on OpenSSL provided by a package manager (ie: Linux distributions), and possibly including that in a container image (containers have their own risks and issues, but dependency management tooling is maturing there – whereas doing that for openssl-sys requires bespoke tooling).

    When you vendor libraries into an Linux distribution, you're abandoning their supported environment to run from upstream. At that point, what's the point of that Linux distribution... especially if you're paying for an enterprise support agreement. 😄

  • Distributing your own OpenSSL binaries with your software, and managing the OpenSSL update process yourself (typically on macOS and Windows).

    Then you know for certain "we're shipping OpenSSL x.x.x", and you can check the OpenSSL project to see when you need to update.

    Larger organisations tend to have their own central versions of these libraries patched in as part of their build process, so that they can keep all of their software's third-party dependencies in sync and up to date.

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
Copy link
Member

Firstyear commented Feb 12, 2024

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
Copy link
Contributor Author

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
Copy link
Member

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

@WesleyAC
Copy link
Contributor Author

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 micolous changed the title Allow using vendored OpenSSL. Update outdated OpenSSL vendoring doc Feb 25, 2024
@micolous
Copy link
Collaborator

Yup, that seems reasonable, thanks!

@micolous micolous merged commit 9f1d995 into kanidm:master Feb 25, 2024
30 of 33 checks passed
arthurgleckler pushed a commit to arthurgleckler/webauthn-rs that referenced this pull request Apr 4, 2024
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.

4 participants