Downgrade serde

[Jarema]

Serde v1.0.172 introduced precompiled serde derive macros, which in many use cases, is an unacceptable risk.

Until the situation is resolved, nats client will stick to the last serde version without the precompiled, nonreproducible blob built outside of visible and auditable processes.

More info: serde-rs/serde#2538

Signed-off-by: Tomasz Pietrek [email protected]

[n1ghtmare]

LGTM!

[TroyKomodo]

This does not actually downgrade serde, if you want to fix the serde version use the = syntax in cargo.toml version

https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html

serde = { version = "=1.0.171", features = ["derive"] }

Alternatively as a library you should be providing a range of supported versions

serde = { version = ">=1.0.160, >1.0.171", features = ["derive"] }

I dont know what features you use, so i just specified 1.0.160 in reality you should pick this for whatever fits best.

[paolobarbolini]

I think there are a few things we can learn from this PR.

a. Requiring an higher semver version than necessary

Every now and then a PR like #1065 is made to this repository in order to bump the patch versions of dependencies in Cargo.toml.
I feel like there are a few pros and cons to this:

Pros

1. If the older version of the dependency had a security vulnerability, or a serious bug, bumping the minimum supported semver version forces downstream users to upgrade those dependencies when upgrading the parent dependency. This is what tools like https://deps.rs and various RustSec checkers (I hope I'm not mistaking) encourage.
2. The project doesn't have to deal with issues of support and compatibility with older versions of the dependency. This is especially useful when the project has a big amount of indirect dependencies and there's no time to chase every violation of -Z minimal-versions.
3. We force users who are not aware of the existence of cargo update, or are lazy, to upgrade their dependencies. While this sounds like a good idea I don't think this should be the parent dependency responsibility, so strike this point. If someone wants to use tokio 1.0.1 (2.5 year old version) and they don't encounter any of the vulnerabilities or serious bugs that have been discovered and fixed over time let them do it.

Cons

1. Users of your crate are forced to upgrade dependencies which they could have left untouched. I feel like this is important, and I think makes sense to discuss in this serde context where issues of security and other policies come up. If we want to give users a chance to audit the dependencies they depend on (both direct and indirect), and not simply download untrusted code from the internet (or as some want to call it: remote code execution), we shouldn't force a faster indirect dependency upgrade pace than necessary.
2. If a bug arises in the version preceding the minimum supported semver version users don't have the option to downgrade the dependency except by forking your crate. This is extremely annoying. Want to talk about reproducible builds or sometimes license requirements? Well it's very annoying if in order to achieve it I have to keep around forked repositories because the crates.io release contained some silly unnecessary stuff that broke my build.

My preference, and what most projects seem to be kind of following, is requiring the widest semver range that gets the crate to compile, with the following exceptions:

• the older version has a security vulnerability
• the older version has a bug that affects my crate
• the older version breaks compiling with -Z minimal-versions

I watch many repositories just out of curiosity. Sometimes external contributors send PRs to bump dependencies patch versions. Most of the time I saw this happening was because the submitter didn't understand how semver worked, or didn't know of the existence of cargo update. In all these cases the PR was rejected and the usage of cargo update was explained.

In short, without the patch version bump part of #1065, this PR wouldn't need to exist.

b. Capping the semver version

While I agree with downgrading serde in order to not use versions that have binary artifacts in the crates.io release, I also kind of hate dependencies that add a cap to the maximum allowed semver version of their dependencies, so I disagree with the solution provided by @TroyKomodo.

This is because while cargo can handle duplicate dependency versions of the same crate, for example you can have a project that requires both serde 1 and serde 0.9, you can't have a project with both serde = "1, <1.0.172" and serde = "1.0.172" requirements for example. In this case the build will fail and the only fix will be to fork one of the dependencies in order to widen their semver range.

This has already caused issues in the past with other crates. For example some RustCrypto crates used to cap the maximum version of their dependencies in order to use versions that conformed to their MSRV requirements. This caused build failures in some projects when other dependencies required an higher minor version of that same crate.

Because of this, I feel like this move only ends up annoying users at the end. cargo would be a giant pain to use if this was done more often.

[paolobarbolini]

time-rs/time#611 might be interesting too.

[Jarema]

My initial idea, as a first, least intrusive step, was for async-nats to not bump serde resolution in cargo dependency tree to > 1.0.172.

Then, as a possible follow-up PR (after seeing to what solution community gravitates to), use a range. That PR would add a CI check that runs -Z minimal-versions (which we should have anyway).

EDIT: Those are excellent points @paolobarbolini :)

[Jarema]

closing in favor #1094

Thank you all for your insights and opinions. It helped us approach the situation without friction for async-nats users.