crypto-common: remove BlockSizes trait

[newpavlov]

BlockSizeUser is no longer limited by block sizes smaller than 256 bytes. BlockSizes was moved to block-buffer (see RustCrypto/utils#1455) and used in bounds only where it's needed.

Closes: #2304

[tarcieri]

Hmm, fun times with elliptic-curve and its (transitive) dependency on hmac there...

[newpavlov]

Welp, this hack does the thing, but it's surely not pretty... I wonder if we should move it to crypto-common. It would mean addition of optional block-buffer dependency for it which may be fine.

[baloo]

This is a breaking change that seems to confuse cargo in downstream crates.
I think the minimum would have been to bump the version in digest and cipher:

diff --git a/cipher/Cargo.toml b/cipher/Cargo.toml
index 4399b4fb88..a6a41be332 100644
--- a/cipher/Cargo.toml
+++ b/cipher/Cargo.toml
@@ -13,7 +13,7 @@
description = "Traits for describing block ciphers and stream ciphers"
[dependencies]
-common = { version = "0.2", package = "crypto-common" }
+common = { version = "0.2.1", package = "crypto-common" }
inout = "0.2.2"
# optional dependencies
diff --git a/digest/Cargo.toml b/digest/Cargo.toml
index bc08a55c7d..79eb264661 100644
--- a/digest/Cargo.toml
+++ b/digest/Cargo.toml
@@ -13,7 +13,7 @@
description = "Traits for cryptographic hash functions and message authentication codes"
[dependencies]
-common = { version = "0.2", package = "crypto-common" }
+common = { version = "0.2.1", package = "crypto-common" }
# optional dependencies
block-buffer = { version = "0.12", optional = true }

My worry is that we'll break chacha20 and rand 0.10 with this late change.

Repro in https://github.com/baloo/blocksizes-repro:

$ cargo check
Checking cipher v0.5.1
error[E0277]: the trait bound `BS: crypto_common::sealed::BlockSizes` is not satisfied
--> /home/arthur_gautier/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/cipher-0.5.1/src/block/ctx.rs:15:22
|
15 | type BlockSize = BS;
| ^^ the trait `crypto_common::typenum::NonZero` is not implemented for `BS`
|
= note: required for `BS` to implement `crypto_common::sealed::BlockSizes`
= note: required for `<block::ctx::BlockCtx<'_, '_, BS> as BlockSizeUser>::BlockSize` to implement `crypto_common::BlockSizes`
note: required by a bound in `crypto_common::BlockSizeUser::BlockSize`
--> /home/arthur_gautier/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/crypto-common-0.2.0/src/lib.rs:61:21
|
61 | type BlockSize: BlockSizes;
| ^^^^^^^^^^ required by this bound in `BlockSizeUser::BlockSize`
help: consider further restricting type parameter `BS` with trait `NonZero`
|
14 | impl<BS: ArraySize + crypto_common::typenum::NonZero> BlockSizeUser for BlockCtx<'_, '_, BS> {
| +++++++++++++++++++++++++++++++++

(minimum fix would be to bump crypto-common version in the dependency, re-release digest and cipher, and yank the x.x.1, but that's not going to be enough (from tests with patch.crates-io I'm running)).

[tarcieri]

@baloo have you tested it with chacha20? I thought this would only impact block ciphers

[baloo]

@tarcieri updated the original message with a reproducer.

[newpavlov]

I and @tarcieri discussed it previously in Zulip and decided to try the yanking approach. The impact should be minimal since we did not release any digest-dependent crates and the only released cipher-dependent crate is chacha20. It's used by rand, but it does not enable the cipher feature, so it should not be affected.

To trigger the breakage you need to use both crypto-common v0.2.0 and cipher v0.5.1 (or digest v0.11.1) simultaneously. I raised this possibility during the discussion, but it's is unlikely to happen in practice. If you pulled chacha20 before the new releases you will use v0.2.0/v0.5.0, running cargo update would update both versions. Adding (or updating from the previous minor version) chacha20 should pull only v0.2.1/v0.5.1 since v0.2.0/v0.5.0 are yanked.

[baloo]

If you run cargo update that works, but dependabot would only pull one dependency at a time in its default config.

This is showcased in RustCrypto/elliptic-curves#1669 and there is no dependency I can bump to fix that (I have no direct edge to cipher to bump it to 0.5.1 in elliptic-curves workspace).

[tarcieri]

In our discussions I said the version requirements should be updated to ensure compatibility.

I’m surprised it didn’t trigger a minimum-versions failure?

[newpavlov]

@baloo
Yes, I consider it included into the "minimal impact". IMO we should use Dependabot only for updating Cargo.lock and use deps.rs badges to nudge dep updates in Cargo.toml.

@tarcieri

In our discussions I said the version requirements should be updated to ensure compatibility.

You mean we should release chacha20 v0.11.0 with cipher set to v0.5.1 and yank v0.11.0? I don't think it's necessary, but you could do it if you want.

I’m surprised it didn’t trigger a minimum-versions failure?

IIUC minimum-versions do not pull yanked dependencies.

[baloo]

There might be a bug in regard to the way we override the dependencies in the workspaces (bug or lack of support for patch.crates-io).
https://github.com/RustCrypto/actions/blob/master/.github/workflows/minimal-versions.yml

[newpavlov]

Merging RustCrypto/elliptic-curves#1670 should fix the breakage after rebase.

UPD: Ah, it does not, since the crates in the repo use pre-release versions of hash crates. We should release the hash crates and update both hashes and digest.

[tarcieri]

@newpavlov I’m not a computer now, but it sounds like cipher needs a stricter requirement for crypto-common, no?

[newpavlov]

@tarcieri
I don't think it does since yanked v0.5.0 should not be pulled without a pre-existing Cargo.lock, so simple version = "0.5" should be sufficient in practice.

[newpavlov]

The problem is with already existing pre-releases of the sha3, rc5, rc6, and block mode crates crates which rely on the removed BlockSizes trait in their source code and thus are effectively broken now.

[tarcieri]

I don't think it does since yanked v0.5.0 should not be pulled without a pre-existing Cargo.lock

But several people will have pre-existing Cargo.locks with the existing crate.

If the versions are specified correctly, there should never be a breakage, even for users who have old yanked versions in their Cargo.lock.

[newpavlov]

But several people will have pre-existing Cargo.locks with the existing crate.

Their Cargo.lock should contain crypto-common v0.2.0 and cipher v0.5.0. Updating dependencies with cargo update should bump both to v0.2.1 and v0.5.1. You could trigger the breakage by specifically running cargo update -p cipher, but I think we can discard such scenarios as improbable.

The elliptic-curve breakage is caused by an incorrect automatic update from the previous release candidate and it would require a manual intervention either way because of the sha3 pre-release used in the repository.

Moreover, the only proper implementation crate release we have is chacha20. IMO any breakage in pre-release crates is not a big problem.

[baloo]

My comment was more: Cargo needs an incentive to update a dependency (currently using a yanked dependency is not enough), otherwise it will continue to use the one version it is using.

I'm not saying cargo update won't fix that, but in larger code-bases (where one of your dependency might be lagging behind/looser dependencies than they should/not so good api stability in versions) you may not be able to use that.
I myself struggle sometimes with the dependency graph of rustcrypto and I've spent a fair bit of time working on it.
I think we will open ourselves to people opening issues because of compilation failure, and unless we want to spend time on user-support, we should fix that.

I think the better approach is to yank cipher 0.5.1 and digest 0.5.1, force the crypto-common dependency to 0.2.1 (the incentive for cargo to bump), then re-release cipher and digest to 0.5.2.

[tarcieri]

I would agree they should use more specific crypto-common dependency that always works. Given the choice between that and a requirement that sometimes causes build failures, we should always pick the former

[baloo]

I've tried to reproduce it in minimal-version, but sadly cargo will not pull a yanked version.
RustCrypto/actions#57

[newpavlov]

@baloo
My position is that we can treat the yanked versions as if they did not exist in the first place because we did not cut any proper releases which depend on them apart from chacha20. In other words, crypto-common v0.2.0 should not be in users' Cargo.lock unless they use release candidates and if they do they should be ready to deal with any potential breakage (which is simple enough in this case).

This is the only reason why releasing breaking changes in 0.x.1 and yanking of 0.x.0 was considered "fine" in this case.

I think the better approach is to yank cipher 0.5.1 and digest 0.5.1, force the crypto-common dependency to 0.2.1 (the incentive for cargo to bump), then re-release cipher and digest to 0.5.2.

At this point I would prefer to just un-yank 0.x.0, yank the 0.x.1, and cut proper new breaking releases of crypto-common, cipher, and digest (and yank chacha20 v0.10.0 and release v0.10.1 which would depend on cipher v0.6.0). It was initially what I wanted to do, but @tarcieri argued for 0.x.0 yanking during the previously mentioned private discussion.

I've tried to reproduce it in minimal-version, but sadly cargo will not pull a yanked version.

What do you mean "sadly"? It's by design and why I think it's fine to skip the patch number while specifying dependencies.