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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

ilya-bobyr
Copy link
Contributor

@ilya-bobyr ilya-bobyr commented Oct 25, 2023

Similar to the changes in the main workspace Cargo.toml, we want to
remove constraints for zeroize in the programs/sbf and other
workspace crates.

See

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)

and

commit 01f1bf2
Author: Illia Bobyr [email protected]
Date: Fri Oct 20 18:20:51 2023 -0700

zeroize: Allow versions newer than 1.3 for aes-gcm-siv (#33618)

Similar to the changes in the main workspace `Cargo.toml`, we want to
remove constraints for `zeroize` in the `programs/sbf` and other
workspace crates.

See

  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)

and

  commit 01f1bf2
  Author: Illia Bobyr <[email protected]>
  Date:   Fri Oct 20 18:20:51 2023 -0700

      zeroize: Allow versions newer than 1.3 for `aes-gcm-siv` (#33618)
@ilya-bobyr ilya-bobyr changed the title zeroize: Apply Cargo.toml patch in programs/sbf/Cargo.toml zeroize: Apply zeroize patch to all repository crates. Oct 25, 2023
@codecov
Copy link

codecov bot commented Oct 25, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.9%. Comparing base (9ffbe2a) to head (50c886a).
Report is 855 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #33853   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         809      809           
  Lines      217717   217717           
=======================================
+ Hits       178345   178379   +34     
+ Misses      39372    39338   -34     

@ilya-bobyr
Copy link
Contributor Author

Applying this PR makes the CI green for the whole repo, not just the main workspace: https://buildkite.com/solana-labs/solana/builds/103656.
So, hopefully, it will make it possible to have green CI when we include a newer version of zeroize in the k8s client repo.

@tareknaser
Copy link

Hello @ilya-bobyr ,

Currently, I'm working with a crate that demands curve25519-dalek version greater than 4, and due to the existing dependency issue, it's impossible to use this crate alongside Solana crates.
This related issue (see #26688) shows that others have faced the same problem.
The changes proposed in this PR are a much-needed solution to enable the use of both crates simultaneously.

Can you please share if there are plans to merge it in the near future?

Thank you!

@ilya-bobyr
Copy link
Contributor Author

Hello @ilya-bobyr ,

Currently, I'm working with a crate that demands curve25519-dalek version greater than 4, and due to the existing dependency issue, it's impossible to use this crate alongside Solana crates. This related issue (see #26688) shows that others have faced the same problem. The changes proposed in this PR are a much-needed solution to enable the use of both crates simultaneously.

Can you please share if there are plans to merge it in the near future?

Thank you!

I think I will merge this PR in the next couple of days.
Does it allow you to build your crate, one of the solana crates and curve25519-dalek version 4 all together?

Long term, it solana should certainly upgrade to a newer version of both curve25519-dalek and aes-gcm-siv. And remove these patches.

@tareknaser
Copy link

This PR indeed resolves the zeroize pinning issue.
However, it's worth mentioning that the patch for aes-gcm-siv still introduces a constraint on the subtle crate, limiting it to (>=2, <2.5).
In my situation, this constraint still presents some difficulties.
I'd suggest this patch instead:

[patch.crates-io.aes-gcm-siv]
git = "https://github.com/RustCrypto/AEADs"
rev = "e1e35e0c4f4943da0a99ceb8477c421dcfae2c33"

# the whole repository with a newer version of `zeroize` than 1.3.
[patch.crates-io.aes-gcm-siv]
git = "https://github.com/RustCrypto/AEADs"
rev = "6105d7a5591aefa646a95d12b5e8d3f55a9214ef"
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there is a newer commit that removes the upper bound in the issue raised and discussed here. The updated commit here. I know the compatibility issue was around needing >1.4 and this commit removes the <1.5. But may be worth keeping if versioning requirements change in the future.

@palinko91
Copy link

I've tried this patch and solved my zeroize conflicts. Thank you for the solution.

@github-actions github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Dec 1, 2023
@github-actions github-actions bot closed this Dec 8, 2023
@ronanyeah
Copy link

@gregcusack @joncinque Should this be re-opened? These dependencies have been causing problems in Rust projects for a long time now. #26688

@ilya-bobyr ilya-bobyr removed the stale [bot only] Added to stale content; results in auto-close after a week. label Dec 8, 2023
@ilya-bobyr
Copy link
Contributor Author

Sorry, I dropped the ball on this.
Got distracted with other work.
Need to look into the proposed newer versions, to see if it is safe to use them.

@ilya-bobyr ilya-bobyr reopened this Dec 8, 2023
@github-actions github-actions bot added stale [bot only] Added to stale content; results in auto-close after a week. and removed stale [bot only] Added to stale content; results in auto-close after a week. labels Dec 25, 2023
@github-actions github-actions bot added stale [bot only] Added to stale content; results in auto-close after a week. and removed stale [bot only] Added to stale content; results in auto-close after a week. labels Jan 9, 2024
@github-actions github-actions bot added stale [bot only] Added to stale content; results in auto-close after a week. and removed stale [bot only] Added to stale content; results in auto-close after a week. labels Jan 25, 2024
@github-actions github-actions bot added stale [bot only] Added to stale content; results in auto-close after a week. and removed stale [bot only] Added to stale content; results in auto-close after a week. labels Feb 12, 2024
@github-actions github-actions bot added stale [bot only] Added to stale content; results in auto-close after a week. and removed stale [bot only] Added to stale content; results in auto-close after a week. labels Feb 28, 2024
@willhickey
Copy link
Contributor

This repository is no longer in use. Please re-open this pull request in the agave repo: https://github.com/anza-xyz/agave

@willhickey willhickey closed this Mar 3, 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.

6 participants