Introduce CI workflow running cargo audit
#2861
Conversation
|
Note Reviews PausedUse the following commands to manage reviews:
WalkthroughA new security audit workflow named "Security Audit" has been introduced to enhance the project's security posture. This workflow, set to run on a schedule, triggers security audits using the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
tnull
force-pushed
the
2024-01-introduce-cargo-audit
branch
from
e6ea9ce
to
c55415f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- .github/workflows/audit.yml (1 hunks)
Additional comments: 1
.github/workflows/audit.yml (1)
- 1-16: The workflow configuration for the security audit looks well-structured and aligns with the PR objectives. It's set to trigger on a daily schedule and upon changes to
Cargo.tomlorCargo.lockfiles, which is a comprehensive approach for dependency auditing. The use ofactions/checkout@v3andrustsec/[email protected]is appropriate for the tasks at hand. However, it's important to ensure that therustsec/[email protected]action is the latest or most stable version available for use, to leverage the latest features and fixes.
tnull
force-pushed
the
2024-01-introduce-cargo-audit
branch
2 times, most recently
from
8ac7efe
to
5aabad8
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2861 +/- ##
==========================================
+ Coverage 89.14% 90.62% +1.47%
==========================================
Files 116 116
Lines 93205 103103 +9898
Branches 93205 103103 +9898
==========================================
+ Hits 83089 93438 +10349
+ Misses 7583 7286 -297
+ Partials 2533 2379 -154 ☔ View full report in Codecov by Sentry. |
| runs-on: ${{ matrix.platform }} | ||
| steps: | ||
| - uses: actions/checkout@v3 | ||
| - uses: rustsec/[email protected] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes. I think this is the behavior should be a good middleground, and essentially exactly what we want?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Alright, I now realized that for I did so by including some prepended commits:
|
tnull
force-pushed
the
2024-01-introduce-cargo-audit
branch
from
d52761d
to
cbd12ca
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
Cargo.tomlis excluded by:!**/*.tomllightning-transaction-sync/Cargo.tomlis excluded by:!**/*.toml
Files selected for processing (3)
- .github/workflows/audit.yml (1 hunks)
- ci/ci-tests.sh (2 hunks)
- lightning-transaction-sync/tests/integration_tests.rs (1 hunks)
Files skipped from review due to trivial changes (2)
- ci/ci-tests.sh
- lightning-transaction-sync/tests/integration_tests.rs
Files skipped from review as they are similar to previous changes (1)
- .github/workflows/audit.yml
tnull
force-pushed
the
2024-01-introduce-cargo-audit
branch
2 times, most recently
from
d90e224
to
c398978
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (6)
Cargo.tomlis excluded by:!**/*.tomlbench/Cargo.tomlis excluded by:!**/*.tomllightning-persister/Cargo.tomlis excluded by:!**/*.tomllightning-rapid-gossip-sync/Cargo.tomlis excluded by:!**/*.tomllightning-transaction-sync/Cargo.tomlis excluded by:!**/*.tomllightning/Cargo.tomlis excluded by:!**/*.toml
Files selected for processing (3)
- .github/workflows/audit.yml (1 hunks)
- ci/ci-tests.sh (2 hunks)
- lightning-transaction-sync/tests/integration_tests.rs (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- .github/workflows/audit.yml
- ci/ci-tests.sh
- lightning-transaction-sync/tests/integration_tests.rs
|
Now also bumping |
tnull
force-pushed
the
2024-01-introduce-cargo-audit
branch
from
c398978
to
6f5ec95
Compare
Thinking about it again, we might actually want to leave this to another PR in order to test that the auditing cron job is actually working as expected? |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (6)
Cargo.tomlis excluded by:!**/*.tomlbench/Cargo.tomlis excluded by:!**/*.tomllightning-persister/Cargo.tomlis excluded by:!**/*.tomllightning-rapid-gossip-sync/Cargo.tomlis excluded by:!**/*.tomllightning-transaction-sync/Cargo.tomlis excluded by:!**/*.tomllightning/Cargo.tomlis excluded by:!**/*.toml
Files selected for processing (3)
- .github/workflows/audit.yml (1 hunks)
- ci/ci-tests.sh (2 hunks)
- lightning-transaction-sync/tests/integration_tests.rs (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- .github/workflows/audit.yml
- ci/ci-tests.sh
- lightning-transaction-sync/tests/integration_tests.rs
tnull
force-pushed
the
2024-01-introduce-cargo-audit
branch
from
ef45086
to
c0767fe
Compare
|
@coderabbitai pause |
|
Now also introduced this to LDK Node. Took a few interations to get everything right, but now it works. Here's a sample output of a workflow run over there: https://github.com/lightningdevkit/ldk-node/actions/runs/7740496634/job/21105571089 (Note we'll get a nice green badge in README if it passes, lol). |
| @@ -10,10 +10,10 @@ members = [ | |||
| "lightning-background-processor", | |||
| "lightning-rapid-gossip-sync", | |||
| "lightning-custom-message", | |||
| "lightning-transaction-sync", | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
.. as the `electrsd` crate doesn't support it. While we previously did so in our CI script, we now also `cfg`-gate the tests and dependencies for easier handling.
.. so it's actually included in the audit.
.. since a version with fixed MSRV was released by now.
tnull
force-pushed
the
2024-01-introduce-cargo-audit
branch
from
c0767fe
to
017d051
Compare
|
LGTM, feel free to squash. |
In order to continuously monitor our dependencies for security vulnerabilities, we introduce a new CI job that will use `cargo audit` to check for any known vulnerabilities. This job is run on a daily schedule. For each new advisory, a new issue will be created.
tnull
force-pushed
the
2024-01-introduce-cargo-audit
branch
from
017d051
to
fd705c7
Compare
Squashed fixups without further changes. |
It worked: #2896 |
In order to continuously monitor our dependencies for security vulnerabilities, we introduce a new CI job that will use
cargo auditto check for any known vulnerabilities.This job is run on a daily schedule. For each new RUSTSEC advisory found a new issue will be created.