Introduce CI workflow running cargo audit#2861

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

Merged
TheBlueMatt 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 numberDiff line numberDiff 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/audit-check@v1.4.1

@TheBlueMattTheBlueMattJan 30, 2024

Copy link
Copy Markdown
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?

@tnulltnullJan 31, 2024

Copy link
Copy Markdown
ContributorAuthor

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.

@TheBlueMattTheBlueMattJan 31, 2024

Copy link
Copy Markdown
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.

@tnulltnullFeb 1, 2024•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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?

@TheBlueMattTheBlueMattFeb 1, 2024

Copy link
Copy Markdown
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.

@tnulltnullFeb 1, 2024•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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.

@TheBlueMattTheBlueMattFeb 5, 2024

Copy link
Copy Markdown
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 }}
Comment thread
tnull marked this conversation as resolved.
2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -10,10 +10,10 @@ members = [
"lightning-background-processor",
"lightning-rapid-gossip-sync",
"lightning-custom-message",
"lightning-transaction-sync",

@TheBlueMattTheBlueMattFeb 5, 2024

Copy link
Copy Markdown
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.

@tnulltnullFeb 6, 2024

Copy link
Copy Markdown
ContributorAuthor

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.

@TheBlueMattTheBlueMattFeb 6, 2024•
edited
Loading

Copy link
Copy Markdown
Collaborator

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.

@tnulltnullFeb 7, 2024•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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.

@TheBlueMattTheBlueMattFeb 8, 2024

Copy link
Copy Markdown
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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Introduce CI workflow running cargo audit#2861

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

 Merged

TheBlueMatt
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 numberDiff line numberDiff 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/audit-check@v1.4.1
@TheBlueMattTheBlueMattJan 30, 2024

Copy link


Copy Markdown

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?
@tnulltnullJan 31, 2024

Copy link


Copy Markdown

ContributorAuthor
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.
@TheBlueMattTheBlueMattJan 31, 2024

Copy link


Copy Markdown

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.
@tnulltnullFeb 1, 2024•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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?
@TheBlueMattTheBlueMattFeb 1, 2024

Copy link


Copy Markdown

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.
@tnulltnullFeb 1, 2024•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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.
@TheBlueMattTheBlueMattFeb 5, 2024

Copy link


Copy Markdown

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 }}
Comment thread
tnull marked this conversation as resolved.


2 changes: 1 addition & 1 deletion
Cargo.toml

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -10,10 +10,10 @@ members = [
 "lightning-background-processor",
 "lightning-rapid-gossip-sync",
 "lightning-custom-message",
 "lightning-transaction-sync",
@TheBlueMattTheBlueMattFeb 5, 2024

Copy link


Copy Markdown

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.
@tnulltnullFeb 6, 2024

Copy link


Copy Markdown

ContributorAuthor
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.
@TheBlueMattTheBlueMattFeb 6, 2024•

edited

Loading

Copy link


Copy Markdown

Collaborator
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.
@tnulltnullFeb 7, 2024•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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.
@TheBlueMattTheBlueMattFeb 8, 2024

Copy link


Copy Markdown

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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Introduce CI workflow running cargo audit#2861

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

 Merged

TheBlueMatt
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 numberDiff line numberDiff 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/audit-check@v1.4.1
@TheBlueMattTheBlueMattJan 30, 2024

Copy link


Copy Markdown

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?
@tnulltnullJan 31, 2024

Copy link


Copy Markdown

ContributorAuthor
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.
@TheBlueMattTheBlueMattJan 31, 2024

Copy link


Copy Markdown

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.
@tnulltnullFeb 1, 2024•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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?
@TheBlueMattTheBlueMattFeb 1, 2024

Copy link


Copy Markdown

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.
@tnulltnullFeb 1, 2024•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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.
@TheBlueMattTheBlueMattFeb 5, 2024

Copy link


Copy Markdown

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 }}
Comment thread
tnull marked this conversation as resolved.


2 changes: 1 addition & 1 deletion
Cargo.toml

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -10,10 +10,10 @@ members = [
 "lightning-background-processor",
 "lightning-rapid-gossip-sync",
 "lightning-custom-message",
 "lightning-transaction-sync",
@TheBlueMattTheBlueMattFeb 5, 2024

Copy link


Copy Markdown

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.
@tnulltnullFeb 6, 2024

Copy link


Copy Markdown

ContributorAuthor
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.
@TheBlueMattTheBlueMattFeb 6, 2024•

edited

Loading

Copy link


Copy Markdown

Collaborator
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.
@tnulltnullFeb 7, 2024•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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.
@TheBlueMattTheBlueMattFeb 8, 2024

Copy link


Copy Markdown

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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Introduce CI workflow running cargo audit#2861

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

 Merged

TheBlueMatt
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 numberDiff line numberDiff 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/audit-check@v1.4.1
@TheBlueMattTheBlueMattJan 30, 2024

Copy link


Copy Markdown

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?
@tnulltnullJan 31, 2024

Copy link


Copy Markdown

ContributorAuthor
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.
@TheBlueMattTheBlueMattJan 31, 2024

Copy link


Copy Markdown

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.
@tnulltnullFeb 1, 2024•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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?
@TheBlueMattTheBlueMattFeb 1, 2024

Copy link


Copy Markdown

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.
@tnulltnullFeb 1, 2024•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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.
@TheBlueMattTheBlueMattFeb 5, 2024

Copy link


Copy Markdown

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 }}
Comment thread
tnull marked this conversation as resolved.


2 changes: 1 addition & 1 deletion
Cargo.toml

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -10,10 +10,10 @@ members = [
 "lightning-background-processor",
 "lightning-rapid-gossip-sync",
 "lightning-custom-message",
 "lightning-transaction-sync",
@TheBlueMattTheBlueMattFeb 5, 2024

Copy link


Copy Markdown

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.
@tnulltnullFeb 6, 2024

Copy link


Copy Markdown

ContributorAuthor
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.
@TheBlueMattTheBlueMattFeb 6, 2024•

edited

Loading

Copy link


Copy Markdown

Collaborator
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.
@tnulltnullFeb 7, 2024•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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.
@TheBlueMattTheBlueMattFeb 8, 2024

Copy link


Copy Markdown

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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Introduce CI workflow running cargo audit#2861

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

 Merged

TheBlueMatt
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 numberDiff line numberDiff 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/audit-check@v1.4.1
@TheBlueMattTheBlueMattJan 30, 2024

Copy link


Copy Markdown

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?
@tnulltnullJan 31, 2024

Copy link


Copy Markdown

ContributorAuthor
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.
@TheBlueMattTheBlueMattJan 31, 2024

Copy link


Copy Markdown

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.
@tnulltnullFeb 1, 2024•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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?
@TheBlueMattTheBlueMattFeb 1, 2024

Copy link


Copy Markdown

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.
@tnulltnullFeb 1, 2024•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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.
@TheBlueMattTheBlueMattFeb 5, 2024

Copy link


Copy Markdown

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 }}
Comment thread
tnull marked this conversation as resolved.


2 changes: 1 addition & 1 deletion
Cargo.toml

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -10,10 +10,10 @@ members = [
 "lightning-background-processor",
 "lightning-rapid-gossip-sync",
 "lightning-custom-message",
 "lightning-transaction-sync",
@TheBlueMattTheBlueMattFeb 5, 2024

Copy link


Copy Markdown

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.
@tnulltnullFeb 6, 2024

Copy link


Copy Markdown

ContributorAuthor
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.
@TheBlueMattTheBlueMattFeb 6, 2024•

edited

Loading

Copy link


Copy Markdown

Collaborator
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.
@tnulltnullFeb 7, 2024•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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.
@TheBlueMattTheBlueMattFeb 8, 2024

Copy link


Copy Markdown

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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Introduce CI workflow running cargo audit#2861

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

 Merged

TheBlueMatt
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 numberDiff line numberDiff 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/audit-check@v1.4.1
@TheBlueMattTheBlueMattJan 30, 2024

Copy link


Copy Markdown

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?
@tnulltnullJan 31, 2024

Copy link


Copy Markdown

ContributorAuthor
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.
@TheBlueMattTheBlueMattJan 31, 2024

Copy link


Copy Markdown

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.
@tnulltnullFeb 1, 2024•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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?
@TheBlueMattTheBlueMattFeb 1, 2024

Copy link


Copy Markdown

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.
@tnulltnullFeb 1, 2024•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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.
@TheBlueMattTheBlueMattFeb 5, 2024

Copy link


Copy Markdown

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 }}
Comment thread
tnull marked this conversation as resolved.


2 changes: 1 addition & 1 deletion
Cargo.toml

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -10,10 +10,10 @@ members = [
 "lightning-background-processor",
 "lightning-rapid-gossip-sync",
 "lightning-custom-message",
 "lightning-transaction-sync",
@TheBlueMattTheBlueMattFeb 5, 2024

Copy link


Copy Markdown

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.
@tnulltnullFeb 6, 2024

Copy link


Copy Markdown

ContributorAuthor
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.
@TheBlueMattTheBlueMattFeb 6, 2024•

edited

Loading

Copy link


Copy Markdown

Collaborator
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.
@tnulltnullFeb 7, 2024•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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.
@TheBlueMattTheBlueMattFeb 8, 2024

Copy link


Copy Markdown

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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Introduce CI workflow running cargo audit#2861

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

 Merged

TheBlueMatt
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 numberDiff line numberDiff 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/audit-check@v1.4.1
@TheBlueMattTheBlueMattJan 30, 2024

Copy link


Copy Markdown

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?
@tnulltnullJan 31, 2024

Copy link


Copy Markdown

ContributorAuthor
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.
@TheBlueMattTheBlueMattJan 31, 2024

Copy link


Copy Markdown

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.
@tnulltnullFeb 1, 2024•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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?
@TheBlueMattTheBlueMattFeb 1, 2024

Copy link


Copy Markdown

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.
@tnulltnullFeb 1, 2024•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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.
@TheBlueMattTheBlueMattFeb 5, 2024

Copy link


Copy Markdown

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 }}
Comment thread
tnull marked this conversation as resolved.


2 changes: 1 addition & 1 deletion
Cargo.toml

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -10,10 +10,10 @@ members = [
 "lightning-background-processor",
 "lightning-rapid-gossip-sync",
 "lightning-custom-message",
 "lightning-transaction-sync",
@TheBlueMattTheBlueMattFeb 5, 2024

Copy link


Copy Markdown

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.
@tnulltnullFeb 6, 2024

Copy link


Copy Markdown

ContributorAuthor
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.
@TheBlueMattTheBlueMattFeb 6, 2024•

edited

Loading

Copy link


Copy Markdown

Collaborator
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.
@tnulltnullFeb 7, 2024•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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.
@TheBlueMattTheBlueMattFeb 8, 2024

Copy link


Copy Markdown

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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Introduce CI workflow running cargo audit#2861

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

 Merged

TheBlueMatt
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 numberDiff line numberDiff 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/audit-check@v1.4.1
@TheBlueMattTheBlueMattJan 30, 2024

Copy link


Copy Markdown

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?
@tnulltnullJan 31, 2024

Copy link


Copy Markdown

ContributorAuthor
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.
@TheBlueMattTheBlueMattJan 31, 2024

Copy link


Copy Markdown

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.
@tnulltnullFeb 1, 2024•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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?
@TheBlueMattTheBlueMattFeb 1, 2024

Copy link


Copy Markdown

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.
@tnulltnullFeb 1, 2024•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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.
@TheBlueMattTheBlueMattFeb 5, 2024

Copy link


Copy Markdown

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 }}
Comment thread
tnull marked this conversation as resolved.


2 changes: 1 addition & 1 deletion
Cargo.toml

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -10,10 +10,10 @@ members = [
 "lightning-background-processor",
 "lightning-rapid-gossip-sync",
 "lightning-custom-message",
 "lightning-transaction-sync",
@TheBlueMattTheBlueMattFeb 5, 2024

Copy link


Copy Markdown

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.
@tnulltnullFeb 6, 2024

Copy link


Copy Markdown

ContributorAuthor
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.
@TheBlueMattTheBlueMattFeb 6, 2024•

edited

Loading

Copy link


Copy Markdown

Collaborator
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.
@tnulltnullFeb 7, 2024•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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.
@TheBlueMattTheBlueMattFeb 8, 2024

Copy link


Copy Markdown

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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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