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

fix(s2n-quic-crypto): require new enough version of zeroize #1609

Merged
merged 2 commits into from
Mar 1, 2023

Conversation

flub
Copy link
Contributor

@flub flub commented Jan 21, 2023

Description of changes:

This crate uses the ZeroizeOnDrop trait which was only introduced in the 1.5 series of zeroize. Earlier versions of this series were yanked, so 1.5.3 is the oldest version that can be used.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@toidiu
Copy link
Contributor

toidiu commented Jan 21, 2023

Thanks for the PR!

This crate uses the ZeroizeOnDrop trait which was only introduced in
the 1.5 series of zeroize.
@flub
Copy link
Contributor Author

flub commented Jan 22, 2023

I've updated this to depend on 1.5 instead of 1.5.3. The way cargo upgrades work it would otherwise not allow upgrades to 1.6 which would not be desirable for a library. Cargo will skip the yanked versions for new dependencies anyway.

@toidiu
Copy link
Contributor

toidiu commented Jan 22, 2023

1.2.3 := >=1.2.3, <2.0.0
1.2 := >=1.2.0, <2.0.0
1.5.3 was fine actually.

But looking at cargo yank docs, even, 1 wont be an issue since new crates will use the latest and skip the yanked versions.

https://doc.rust-lang.org/cargo/commands/cargo-yank.html

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

@toidiu
Copy link
Contributor

toidiu commented Jan 23, 2023

Cargo will skip the yanked versions for new dependencies anyway.

Do we need to update the dependency then?

@flub
Copy link
Contributor Author

flub commented Jan 23, 2023

1.2.3 := >=1.2.3, <2.0.0 1.2 := >=1.2.0, <2.0.0 1.5.3 was fine actually.

Oh that's good to know, I must have misremembered this.

But looking at cargo yank docs, even, 1 wont be an issue since new crates will use the latest and skip the yanked versions.

This was not the case for me, I stumbled upon this because on a new small test project it picked too old a version for some reason. As long as it is not specified to be >=1.5 there are scenarios where cargo will allow earlier versions, which will break.

Do we need to update the dependency then?

I believe so, though I don't mind whether that's 1.5 or 1.5.3 (because I don't think that part matters).

@toidiu
Copy link
Contributor

toidiu commented Jan 27, 2023

This was not the case for me, I stumbled upon this because on a new small test project it picked too old a version for some reason. As long as it is not specified to be >=1.5 there are scenarios where cargo will allow earlier versions, which will break.

Are you sure you didn't have a Cargo.lock file? If its reproducible then its a Cargo bug and you should report that since zeroize is not the only dependency which can yank crates (also in the future) and we certainly dont want to be managing yank versions manually.

@flub
Copy link
Contributor Author

flub commented Jan 27, 2023

This was not the case for me, I stumbled upon this because on a new small test project it picked too old a version for some reason. As long as it is not specified to be >=1.5 there are scenarios where cargo will allow earlier versions, which will break.

Are you sure you didn't have a Cargo.lock file? If its reproducible then its a Cargo bug and you should report that since zeroize is not the only dependency which can yank crates (also in the future) and we certainly dont want to be managing yank versions manually.

So maybe there was a bug that needs reporting to cargo as I did not understand why it chose this version. Though that might also indicate I was doing something wrong.

However it does not change what s2n-quic needs to specify as dependency requirement. It needs zeroize >= 1.5 since it uses an API first introduced there. So that's what it should depend on.

The yanking of some 1.5 versions are not part of the issue being addressed here, I apologise for mixing that in when trying to figure out what version to depend on. We can expect cargo to deal with yanking.

camshaft
camshaft previously approved these changes Jan 27, 2023
@camshaft camshaft enabled auto-merge (squash) January 27, 2023 15:27
@camshaft camshaft disabled auto-merge March 1, 2023 02:10
@camshaft camshaft merged commit dc9e0fa into aws:main Mar 1, 2023
@flub flub deleted the flub/zeroize-version branch March 1, 2023 19:52
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.

3 participants