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

v1.18: Use updated branch for curve25519-dalek #1939

Merged
merged 3 commits into from
Jul 2, 2024

Conversation

steviez
Copy link

@steviez steviez commented Jul 1, 2024

Problem

RUSTSEC-2024-0344 was announced so update to a branch that contains the commits that were created in response to the advisory.

We must do this manually as the v1.18 branch is built against curve25519-dalek 3.2.1; this is not the latest major release and the maintainers have chosen not to push changes to their older release branches

Summary of Changes

Update to a branch that contains the zeroize commit, as well as the new security advisory commits

RUSTSEC-2024-0344 was announced so update to a branch that contains the
commits that were created in response to the advisory.

We must do this manually as the v1.18 branch is built against
curve25519-dalek 3.2.1; this is not the latest major release and the
maintainers have chosen not to push changes to their older release
branches
@steviez steviez requested a review from a team as a code owner July 1, 2024 15:34
ci/do-audit.sh Outdated
Comment on lines 37 to 40
# curve25519-dalek
# Patches to address the advisory have been pulled into a fork of the repo.
# See Cargo.toml for more information
--ignore RUSTSEC-2024-0344
Copy link
Author

@steviez steviez Jul 1, 2024

Choose a reason for hiding this comment

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

Someone please sanity check me here that it is ok / proper to ignore the advisory since we pulled the commits in ourselves

ci/do-audit.sh Outdated Show resolved Hide resolved
@steviez
Copy link
Author

steviez commented Jul 1, 2024

Also, FWIW, the change to ignore the advisory in CI is present in v2.0; it landed before we cut the branch:
https://github.com/anza-xyz/agave/commits/v2.0/ci/do-audit.sh

Technically, master is currently building against a version of ed25519-dalek that does NOT have the commits to address the security advisory. That is, master is built against this branch:

agave/Cargo.toml

Lines 533 to 535 in b97fa99

[patch.crates-io.curve25519-dalek]
git = "https://github.com/anza-xyz/curve25519-dalek.git"
rev = "b500cdc2a920cd5bff9e2dd974d7b97349d61464"

which corresponds to this branch:
https://github.com/anza-xyz/curve25519-dalek/tree/3.2.1-unpin-zeroize

This PR branch is proposing to build v1.18 against this curve25519-dalek branch:
https://github.com/anza-xyz/curve25519-dalek/commits/3.2.1-fix-audit/

The branch here cherry-picked two additional commits, the commits that address the security advisory that we're ignoring

@willhickey
Copy link

Approved for merging over red CI. It's just the downstream projects check that's failing. See #1960 and #releng discussion for context

@steviez steviez merged commit a67e681 into anza-xyz:v1.18 Jul 2, 2024
26 of 36 checks passed
@steviez steviez deleted the v1.18_dalek branch July 2, 2024 16:11
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.

5 participants