libbpf-rs icon indicating copy to clipboard operation
libbpf-rs copied to clipboard

Cargo audit should be added to the github workflow

Open mimullin-bbry opened this issue 3 years ago • 4 comments

PR https://github.com/libbpf/libbpf-rs/pull/186 will give the libbpf-rs project a clean run of "cargo audit"

The project may wish to utilize cargo audit in the github workflow.

Using cargo audit, will give users of libbpf-rs piece of mind that the project isn't pulling in any potential vulnerabilities.

Cargo audit be run on a schedule (weekly/daily?) along with the regular PR request cycle in order to catch new vulnerabilities introduced by dependencies.

mimullin-bbry avatar Feb 12 '22 06:02 mimullin-bbry

https://github.com/actions-rs/audit-check could probably do the job.

You can also use the schedule trigger in GitHub actions to run it as a cron.

I can PR something if that helps?

loshz avatar Feb 18 '22 05:02 loshz

Change #190 is certainly something that I, a user of libbpf-rs, would appreciate.

However, adding cargo audit as a scheduled event will add a maintenance burden upon the project maintainers. I think it's a small burden w.r.t. scheduled events; but it's not my call to make.

To follow up #190, do you know if there is a way to add cargo audit to the CI pipeline, but make it such that it doesn't "fail" a PR build? It would be good for reviewers of code to see if any newly added dependencies pull in potential security violations, but shouldn't interrupt project momentum for already known, but as-yet unaddressed violations.

mimullin-bbry avatar Feb 19 '22 18:02 mimullin-bbry

Apologies if I jumped the gun by creating the PR.

However, adding cargo audit as a scheduled event will add a maintenance burden upon the project maintainers. I think it's a small burden w.r.t. scheduled events; but it's not my call to make.

If a scheduled event fails, it will only update the new shield in the README. But I do see how this could be added overhead to maintainers.

To follow up https://github.com/libbpf/libbpf-rs/pull/190, do you know if there is a way to add cargo audit to the CI pipeline, but make it such that it doesn't "fail" a PR build?

Unfortunately, that isn't possible right now. There is an open feature request to add it, but as of right now, it would fail the PR status checks.

loshz avatar Feb 19 '22 19:02 loshz

I wanted to touch base on this again as I've been having good success with cargo-deny recently, and it addresses all of the above issues we encountered before. In fact, cargo-deny is now the official RustSec frontend.

We could use the default cargo-deny config and set the levels to "warn" as to not fail, and run a GitHub actions job like this:

name: audit

on:
  push:
    branches:
      - main
  pull_request:
    branches:
      - main

jobs:
  audit:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v2
      - uses: actions-rs/toolchain@v1
        with:
          toolchain: 1.60
      - run: cargo install --locked cargo-deny
      - run: cargo deny check

I can PR something to show it working if you think this would be useful.

loshz avatar May 14 '22 05:05 loshz

I can PR something to show it working if you think this would be useful.

I think that would be great. I am not familiar with either cargo-audit or cargo-deny and I think it's hard to gauge for us how much of a maintenance impact it will have. So if you have something we could evaluate, potentially merge and get some experience under our belt, and then reassess if we want to keep it or not.

That being said, I don't exactly know what benefit it will provide if it's based on Cargo.lock. Currently we are on a path to having Cargo.lock act more as a baseline for the minimal versions we support, as opposed to updating it whenever a new dependency version pops up. So there may be some conflict of intent here. We may have to switch to using something like cargo +nightly -Z minimal-versions update to go about that long term.

danielocfb avatar Jan 13 '23 22:01 danielocfb

Having spent the past 6 months using my original suggestion, I'd recommend a different approach going forward.

It's probably more efficient to run this as a period job (weekly, etc.) instead of on every PR as it can take a while to complete due to the nature of it having to recursively compile and analyze each dependency.

Something like this would probably do the job:

on:
  schedule:
    - cron: '0 0 * * 0' # At 00:00 on Sunday

jobs:
  audit:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
      - uses: dtolnay/rust-toolchain@stable # Latest stable Rust version

      # Install and run
      - run: cargo install --locked cargo-deny
      - run: cargo deny check

loshz avatar Jan 13 '23 23:01 loshz