zeroize: Allow versions newer than 1.3 for `curve25519-dalek` #33516

ilya-bobyr · 2023-10-04T00:20:49Z

Problem

curve25519-dalek v3.2.1 has a constraint on the maximum zeroize version to be no more than 1.3.

At the same time, cargo does not want to construct a dependency graph with duplicate instances of a crate, when the first non-zero version of those instances are the same. That is, it refuses to build a workspace with both 1.3 and 1.4 versions of zeroize.

zeroize is actually backward compatible, and curve25519-dalek restriction is overly pessimistic. This package lifted this restriction in newer versions, but we still depend on older version and can not immediately update.

Summary of Changes

Use a patched version based on v3.2.1 that also removes the unnecessary constraint.

codecov · 2023-10-04T03:52:36Z

Codecov Report

Merging #33516 (1357582) into master (01f1bf2) will decrease coverage by 0.1%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #33516     +/-   ##
=========================================
- Coverage    81.8%    81.8%   -0.1%     
=========================================
  Files         806      806             
  Lines      217428   217428             
=========================================
- Hits       178058   178047     -11     
- Misses      39370    39381     +11

wei1769 · 2023-10-04T16:13:49Z

Thanks ser, you save me.
I'm thinking to upgrade ed25519-dalek and patch all breaking changes. That's a lot of work @@
This patch is clever

gregcusack

This is great! super helpful. avoids a ton of headaches! Thank you!

ilya-bobyr · 2023-10-04T22:10:29Z

Thanks ser, you save me. I'm thinking to upgrade ed25519-dalek and patch all breaking change. That's a lot of work @@ The patch is clever

People were already looking at it: #32836
The problem is that we use types provided by ed25519-dalek in the public API.
And if we update ed25519-dalek to a new major version, those types become incompatible.
It is a tricky problem to solve, unfortunately.

t-nelson · 2023-10-09T17:11:19Z

was there an attempt to work with upstream in either of these cases before vendoring? vendoring should really be a last resort

if we have to take this route, ideally put the vendored crates in in separate PRs to make the revert simple

ilya-bobyr · 2023-10-09T19:42:43Z

was there an attempt to work with upstream in either of these cases before vendoring?
vendoring should really be a last resort

Here is a thread with dalek-cryptography/curve25519-dalek:

dalek-cryptography/curve25519-dalek#452 (comment)

They do not seem to be interested in doing another release.
They are just saying we should upgrade.
I think, we would need curve25519-dalek with the zeroize constraint removed even if we upgrade, if we want to preserve backward compatibility in our public API.

if we have to take this route, ideally put the vendored crates in in separate PRs to make the revert simple

I'll split aes-gcm-siv and curve25519-dalek changes into two PRs.

ilya-bobyr · 2023-10-10T08:21:57Z

Split eas-gcm-siv changes into #33618.
I'll reuse this PR for the curve25519-dalek changes.
For now, it includes both, as two separate commits, as the second relies on the first.

gregcusack

LGTM. But ya with what trent said, I'm not sure how people feel about vendoring. This definitely solves a big headache with versioning though.....

joncinque

This looks good to me. It doesn't seem like it'll be possible upstream the change, and we'll likely need to keep dalek v1 in our public API for a long time to come, so we likely want this change when we include both dalek v2 and v1. I would get @t-nelson's approval too since he had reservations.

t-nelson · 2023-10-13T23:00:49Z

seems fine since upstream doesn't want to play ball.

This looks good to me. It doesn't seem like it'll be possible upstream the change, and we'll likely need to keep dalek v1 in our public API for a long time to come, so we likely want this change when we include both dalek v2 and v1. I would get @t-nelson's approval too since he had reservations.

given that we're going to try to take monorepo to 2.0 in 1.19, i'm hoping we'll never need to formally support v2 and our api can instead be purely based on traits that can be implemented externally

`curve25519-dalek` v3.2.1 has a constraint on the maximum `zeroize` version to be no more than 1.3. At the same time, `cargo` does not want to construct a dependency graph with duplicate instances of a crate, when the first non-zero version of those instances are the same. That is, it refuses to build a workspace with both 1.3 and 1.4 versions of `zeroize`. `zeroize` is actually backward compatible, and `curve25519-dalek` restriction is overly pessimistic. This packages lifted this restriction in newer versions, but we still depend on older version and can not immediately update.

ilya-bobyr · 2023-10-21T01:36:43Z

Moved the fork to https://github.com/solana-labs/curve25519-dalek and rebased on top of the merged #33618.
This is now good for inclusion to fully remove the zerioze constraint.

joncinque

Looks great!

gregcusack

lgtm. thank you!

…33930) Fixes commit a099c7a Author: Illia Bobyr <[email protected]> Date: Mon Oct 23 12:19:59 2023 -0700 zeroize: Allow versions newer than 1.3 for `curve25519-dalek` (#33516) Use correct commit hash from `solana-labs/curve25519-dalek.git`.

…-labs#33516) `curve25519-dalek` v3.2.1 has a constraint on the maximum `zeroize` version to be no more than 1.3. At the same time, `cargo` does not want to construct a dependency graph with duplicate instances of a crate, when the first non-zero version of those instances are the same. That is, it refuses to build a workspace with both 1.3 and 1.4 versions of `zeroize`. `zeroize` is actually backward compatible, and `curve25519-dalek` restriction is overly pessimistic. These packages lifted this restriction in newer versions, but we still depend on older version and can not immediately update.

…olana-labs#33930) Fixes commit a099c7a Author: Illia Bobyr <[email protected]> Date: Mon Oct 23 12:19:59 2023 -0700 zeroize: Allow versions newer than 1.3 for `curve25519-dalek` (solana-labs#33516) Use correct commit hash from `solana-labs/curve25519-dalek.git`.

…-labs#33516) `curve25519-dalek` v3.2.1 has a constraint on the maximum `zeroize` version to be no more than 1.3. At the same time, `cargo` does not want to construct a dependency graph with duplicate instances of a crate, when the first non-zero version of those instances are the same. That is, it refuses to build a workspace with both 1.3 and 1.4 versions of `zeroize`. `zeroize` is actually backward compatible, and `curve25519-dalek` restriction is overly pessimistic. These packages lifted this restriction in newer versions, but we still depend on older version and can not immediately update.

…olana-labs#33930) Fixes commit a099c7a Author: Illia Bobyr <[email protected]> Date: Mon Oct 23 12:19:59 2023 -0700 zeroize: Allow versions newer than 1.3 for `curve25519-dalek` (solana-labs#33516) Use correct commit hash from `solana-labs/curve25519-dalek.git`.

ilya-bobyr marked this pull request as ready for review October 4, 2023 16:00

ilya-bobyr requested a review from gregcusack October 4, 2023 16:01

gregcusack previously approved these changes Oct 4, 2023

View reviewed changes

ilya-bobyr requested review from t-nelson and joncinque October 4, 2023 22:03

ilya-bobyr dismissed gregcusack’s stale review via bfc0132 October 10, 2023 08:18

ilya-bobyr changed the title ~~zeroize: Allow versions newer than 1.3~~ zeroize: Allow versions newer than 1.3 for curve25519-dalek Oct 10, 2023

gregcusack self-requested a review October 13, 2023 16:41

gregcusack previously approved these changes Oct 13, 2023

View reviewed changes

joncinque previously approved these changes Oct 13, 2023

View reviewed changes

ilya-bobyr dismissed stale reviews from joncinque and gregcusack via 1357582 October 21, 2023 01:35

ilya-bobyr requested review from joncinque and gregcusack October 21, 2023 01:37

joncinque approved these changes Oct 23, 2023

View reviewed changes

gregcusack approved these changes Oct 23, 2023

View reviewed changes

ilya-bobyr merged commit a099c7a into solana-labs:master Oct 23, 2023
28 checks passed

ilya-bobyr deleted the pr/remove-zeroize-1.3-hard-limit branch October 23, 2023 19:20

This was referenced Oct 25, 2023

zeroize: Apply zeroize patch to all repository crates. #33853

Closed

Fixup: zeroize: Allow versions newer than 1.3 for curve25519-dalek #33930

Merged

willhickey mentioned this pull request Mar 28, 2024

v1.18 commits - please ignore anza-xyz/agave#475

Closed

ilya-bobyr mentioned this pull request Jul 23, 2024

bump curve25519-dalek from 3.2.1 to 4.1.3 anza-xyz/agave#1693

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zeroize: Allow versions newer than 1.3 for `curve25519-dalek` #33516

zeroize: Allow versions newer than 1.3 for `curve25519-dalek` #33516

ilya-bobyr commented Oct 4, 2023 •

edited

Loading

codecov bot commented Oct 4, 2023 •

edited

Loading

wei1769 commented Oct 4, 2023 •

edited

Loading

gregcusack left a comment

ilya-bobyr commented Oct 4, 2023

t-nelson commented Oct 9, 2023

ilya-bobyr commented Oct 9, 2023

ilya-bobyr commented Oct 10, 2023

gregcusack left a comment

joncinque left a comment

t-nelson commented Oct 13, 2023

ilya-bobyr commented Oct 21, 2023

joncinque left a comment

gregcusack left a comment

zeroize: Allow versions newer than 1.3 for curve25519-dalek #33516

zeroize: Allow versions newer than 1.3 for curve25519-dalek #33516

Conversation

ilya-bobyr commented Oct 4, 2023 • edited Loading

Problem

Summary of Changes

codecov bot commented Oct 4, 2023 • edited Loading

Codecov Report

wei1769 commented Oct 4, 2023 • edited Loading

gregcusack left a comment

Choose a reason for hiding this comment

ilya-bobyr commented Oct 4, 2023

t-nelson commented Oct 9, 2023

ilya-bobyr commented Oct 9, 2023

ilya-bobyr commented Oct 10, 2023

gregcusack left a comment

Choose a reason for hiding this comment

joncinque left a comment

Choose a reason for hiding this comment

t-nelson commented Oct 13, 2023

ilya-bobyr commented Oct 21, 2023

joncinque left a comment

Choose a reason for hiding this comment

gregcusack left a comment

Choose a reason for hiding this comment

zeroize: Allow versions newer than 1.3 for `curve25519-dalek` #33516

zeroize: Allow versions newer than 1.3 for `curve25519-dalek` #33516

ilya-bobyr commented Oct 4, 2023 •

edited

Loading

codecov bot commented Oct 4, 2023 •

edited

Loading

wei1769 commented Oct 4, 2023 •

edited

Loading