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.

Sign up for GitHub

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

Jump to bottom

Introduce CI workflow running cargo audit #2861

Merged
merged 4 commits into from
Feb 13, 2024
Merged

Introduce CI workflow running cargo audit #2861

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a386ba5
Disable `lightning-transaction-sync` integration tests on Windows
tnull Feb 1, 2024
9ac42ed
Move `lightning-transaction-sync` to main workspace
tnull Feb 1, 2024
7299fe6
Drop `reqwest` pin
tnull Feb 1, 2024
fd705c7
Introduce CI workflow running `cargo audit`
tnull Jan 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions .github/workflows/audit.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
name: Security Audit
on:
workflow_dispatch:
schedule:
- cron: '0 0 * * *'

jobs:
audit:
runs-on: ubuntu-latest
permissions:
issues: write
checks: write
steps:
- uses: actions/checkout@v3
- uses: rustsec/[email protected]
Copy link
Collaborator

Choose a reason for hiding this comment

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

How does this categorize a dependency where multiple minor versions exist? eg if we depend on tokio 1.0 and 1.0.0 has a security vuln, but 1.0.1 does not, will we get lots of noise or will it miss it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So cargo audit generates a Cargo.lock and compares that to the [versions] field in the RUSTSEC advisory format (see https://github.com/rustsec/advisory-db?tab=readme-ov-file#advisory-format).

So for example, if I'd revert 348db3b and run cargo update tokio --precise "1.14.1" && cargo audit it would yield:

Crate:     tokio
Version:   1.14.1
Title:     reject_remote_clients Configuration corruption
Date:      2023-01-04
ID:        RUSTSEC-2023-0001
URL:       https://rustsec.org/advisories/RUSTSEC-2023-0001
Solution:  Upgrade to >=1.18.4, <1.19.0 OR >=1.20.3, <1.21.0 OR >=1.23.1
Dependency tree:
tokio 1.14.1
├── lightning-net-tokio 0.0.121
├── lightning-block-sync 0.0.121
└── lightning-background-processor 0.0.121

If I'm not pinning tokio, it would check against 1.35 and wouldn't yield this vulnerability.

Moreover, the audit-check job has an ignore field via which we could tell it to ignore individual advisories if we wanted to.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, so if, for example, we ship a binary release with tokio 1.14.1 then this job won't complain because 1.14.2 exists and fixes the bug (or whatever) and this job always generates a fresh Cargo.lock, causing it to just silently test against 1.14.2 once it exists.

Copy link
Contributor Author

@tnull tnull Feb 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. I think this is the behavior should be a good middleground, and essentially exactly what we want?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think its great for the Rust side of the house, but we'll need to build something separate for bindings to validate against our released binaries and get notifications when those need bumping.

Copy link
Contributor Author

@tnull tnull Feb 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Good point, indeed cargo audit also supports auditing binaries (see https://github.com/RustSec/rustsec/tree/main/cargo-audit#cargo-audit-bin-subcommand), however, this is probably an extra step and shouldn't happen in this PR I believe. We could even consider making this a manual step whenever we get notified of an advisory by this CI job. At that point we could manually go back and audit our last X releases to see which ones are affected. To that end, we may want to compile our release binaries with cargo auditable, which should make the output more accurate I believe.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For that it seems simpler to just check in or otherwise keep a Cargo.lock somewhere which describes the dependencies we shipped in the latest (2?) binary releases, no? We could check them in upstream here so that this job automatically checks them, or we chould just leave it.

with:
token: ${{ secrets.GITHUB_TOKEN }}
tnull marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ members = [
"lightning-background-processor",
"lightning-rapid-gossip-sync",
"lightning-custom-message",
"lightning-transaction-sync",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure we want to do this yet. We'd previously avoided it because the dependency tree in tx sync is kinda big/annoying, and has a large potential to break MSRV.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm confused by this: a big part why we moved to a 1.63 MSRV was exactly to be able to do this. As discussed in #2681, we chose 1.63 as it easily covers the lightning-transaction-sync dependencies, even with some buffer. We dropped all unnecessary dev dependencies at this point and are in the progress of further reducing the dependency tree. We also agreed that it will be exposed in bindings as first-class module after we figure out an auditing strategy for its dependencies, which is exactly what we do in this PR. We need to move it to the workspace exactly so that it will be covered by the audit job.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt Feb 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, so regarding bindings and the discussion at #2861 (comment), above, I figured we'd go with a different strategy for bindings auditing, but if we want to check in a bindings-release-Cargo.lock here then that does change things a bit. Also worth pointing out that bindings can happily be built against something not in-workspace, the bindings don't care at all, the only workspace-not-in-workspace difference is really how annoying it is for devs to deal with when a dependency breaks MSRV, thus I figured it really wasn't a big deal, as much as moving it in-workspace would be nice.

Copy link
Contributor Author

@tnull tnull Feb 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, so regarding bindings and the discussion at #2861 (comment), above, I figured we'd go with a different strategy for bindings auditing, but if we want to check in a bindings-release-Cargo.lock here then that does change things a bit.

Not exactly sure what changes? We can still use this CI job as is to check our Rust dependencies and then figure out a way how to keep some Cargo.locks around and check them periodically? Arguably, it might even make sense to do the latter in ldk-c-bindings or ldk-swift/ldk-garbagecollected on the 'finished' bindings?

Also worth pointing out that bindings can happily be built against something not in-workspace, the bindings don't care at all, the only workspace-not-in-workspace difference is really how annoying it is for devs to deal with when a dependency breaks MSRV, thus I figured it really wasn't a big deal, as much as moving it in-workspace would be nice.

Mh, sure, we can still include it in bindings, but we'd at least need to introduce a separate audit run just for lightning-transaction-sync. So I don't really understand why it's worth maintaining more and more workarounds/special casing for lightning-transaction-sync instead of just adding it to the workspace. If the concern is really just that devs need to pin four instead of two packages to build the whole workspace with MSRV, we could provide a simple script for that, although ci-tests.sh already does all the right things to allow local testing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not exactly sure what changes? We can still use this CI job as is to check our Rust dependencies and then figure out a way how to keep some Cargo.locks around and check them periodically? Arguably, it might even make sense to do the latter in ldk-c-bindings or ldk-swift/ldk-garbagecollected on the 'finished' bindings?

Nothing, it just seemed to me like one of the reasons to have it all in one workspace is to use one set of Cargo.locks to test multiple binaries we've shipped across several crates at once, but if we're doing that downstream in the various bindings repos there's less overhead to just running cargo audit twice and let it build its own Cargo.locks. I'm fine moving it in workspace, just worried its gonna be annoying for developers running cargo test and seeing it fail due to MSRV changes.

]

exclude = [
"lightning-transaction-sync",
"no-std-check",
"msrv-no-dev-deps-check",
"bench",
Expand Down
3 changes: 2 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@ Rust-Lightning
[![Crate](https://img.shields.io/crates/v/lightning.svg?logo=rust)](https://crates.io/crates/lightning)
[![Documentation](https://img.shields.io/static/v1?logo=read-the-docs&label=docs.rs&message=lightning&color=informational)](https://docs.rs/lightning/)
[![Safety Dance](https://img.shields.io/badge/unsafe-forbidden-success.svg)](https://github.com/rust-secure-code/safety-dance/)
[![Security Audit](https://github.com/lightningdevkit/rust-lightning/actions/workflows/audit.yml/badge.svg)](https://github.com/lightningdevkit/rust-lightning/actions/workflows/audit.yml)

[LDK](https://lightningdevkit.org)/`rust-lightning` is a highly performant and flexible
[LDK](https://lightningdevkit.org)/`rust-lightning` is a highly performant and flexible
implementation of the Lightning Network protocol.

The primary crate, `lightning`, is runtime-agnostic. Data persistence, chain interactions,
Expand Down
30 changes: 14 additions & 16 deletions ci/ci-tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,14 @@ PIN_RELEASE_DEPS # pin the release dependencies in our main workspace
# The addr2line v0.21 crate (a dependency of `backtrace` starting with 0.3.69) relies on rustc 1.65
[ "$RUSTC_MINOR_VERSION" -lt 65 ] && cargo update -p backtrace --precise "0.3.68" --verbose

# Starting with version 0.5.9 (there is no .6-.8), the `home` crate has an MSRV of rustc 1.70.0.
[ "$RUSTC_MINOR_VERSION" -lt 70 ] && cargo update -p home --precise "0.5.5" --verbose

export RUST_BACKTRACE=1

# Build `lightning-transaction-sync` in no_download mode.
export RUSTFLAGS="$RUSTFLAGS --cfg no_download"

echo -e "\n\nBuilding and testing all workspace crates..."
cargo test --verbose --color always
cargo check --verbose --color always
Expand All @@ -85,24 +91,16 @@ if [[ "$HOST_PLATFORM" != *windows* ]]; then
echo -e "\n\nBuilding and testing Transaction Sync Clients with features"
pushd lightning-transaction-sync

# reqwest 0.11.21 had a regression that broke its 1.63.0 MSRV
[ "$RUSTC_MINOR_VERSION" -lt 65 ] && cargo update -p reqwest --precise "0.11.20" --verbose
# Starting with version 1.10.0, the `regex` crate has an MSRV of rustc 1.65.0.
[ "$RUSTC_MINOR_VERSION" -lt 65 ] && cargo update -p regex --precise "1.9.6" --verbose
# Starting with version 0.5.9 (there is no .6-.8), the `home` crate has an MSRV of rustc 1.70.0.
[ "$RUSTC_MINOR_VERSION" -lt 70 ] && cargo update -p home --precise "0.5.5" --verbose

DOWNLOAD_ELECTRS_AND_BITCOIND

RUSTFLAGS="$RUSTFLAGS --cfg no_download" cargo test --verbose --color always --features esplora-blocking
RUSTFLAGS="$RUSTFLAGS --cfg no_download" cargo check --verbose --color always --features esplora-blocking
RUSTFLAGS="$RUSTFLAGS --cfg no_download" cargo test --verbose --color always --features esplora-async
RUSTFLAGS="$RUSTFLAGS --cfg no_download" cargo check --verbose --color always --features esplora-async
RUSTFLAGS="$RUSTFLAGS --cfg no_download" cargo test --verbose --color always --features esplora-async-https
RUSTFLAGS="$RUSTFLAGS --cfg no_download" cargo check --verbose --color always --features esplora-async-https
RUSTFLAGS="$RUSTFLAGS --cfg no_download" cargo test --verbose --color always --features electrum
RUSTFLAGS="$RUSTFLAGS --cfg no_download" cargo check --verbose --color always --features electrum

cargo test --verbose --color always --features esplora-blocking
cargo check --verbose --color always --features esplora-blocking
cargo test --verbose --color always --features esplora-async
cargo check --verbose --color always --features esplora-async
cargo test --verbose --color always --features esplora-async-https
cargo check --verbose --color always --features esplora-async-https
cargo test --verbose --color always --features electrum
cargo check --verbose --color always --features electrum
popd
fi

Expand Down
4 changes: 2 additions & 2 deletions lightning-transaction-sync/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ electrum-client = { version = "0.18.0", optional = true }
lightning = { version = "0.0.121", path = "../lightning", default-features = false, features = ["std", "_test_utils"] }
tokio = { version = "1.35.0", features = ["full"] }

[target.'cfg(not(no_download))'.dev-dependencies]
[target.'cfg(all(not(target_os = "windows"), not(no_download)))'.dev-dependencies]
electrsd = { version = "0.26.0", default-features = false, features = ["legacy", "esplora_a33e97e1", "bitcoind_25_0"] }

[target.'cfg(no_download)'.dev-dependencies]
[target.'cfg(all(not(target_os = "windows"), no_download))'.dev-dependencies]
electrsd = { version = "0.26.0", default-features = false, features = ["legacy"] }
2 changes: 1 addition & 1 deletion lightning-transaction-sync/tests/integration_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#![cfg(any(feature = "esplora-blocking", feature = "esplora-async", feature = "electrum"))]
#![cfg(all(not(target_os = "windows"), any(feature = "esplora-blocking", feature = "esplora-async", feature = "electrum")))]

#[cfg(any(feature = "esplora-blocking", feature = "esplora-async"))]
use lightning_transaction_sync::EsploraSyncClient;
Expand Down
Loading