Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cargo audit should be added to the github workflow #187

Closed
mimullin-bbry opened this issue Feb 12, 2022 · 6 comments · Fixed by #339
Closed

Cargo audit should be added to the github workflow #187

mimullin-bbry opened this issue Feb 12, 2022 · 6 comments · Fixed by #339
Assignees

Comments

@mimullin-bbry
Copy link
Contributor

PR #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.

@loshz
Copy link
Contributor

loshz commented Feb 18, 2022

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?

@mimullin-bbry
Copy link
Contributor Author

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.

@loshz
Copy link
Contributor

loshz commented Feb 19, 2022

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 #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
Copy link
Contributor

loshz commented May 14, 2022

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.

@danielocfb
Copy link
Collaborator

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.

@loshz
Copy link
Contributor

loshz commented Jan 13, 2023

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

@danielocfb danielocfb self-assigned this Jan 19, 2023
insearchoflosttime pushed a commit that referenced this issue Jan 21, 2023
…ndencies

Now that we have decoupled our minimum supported Rust version checks
from `Cargo.lock` [0], we can easily keep dependency versions
up-to-date. Doing so will make sure that we always release with the most
recent dependencies, which may include bug and security fixes. It will
also help with enabling cargo audit/deny [2], depending on what we audit.

This change enables Dependabot for doing so. It will scan for updated
packages and open pull requests. I've tried it out on my fork [1] and it
worked fine.

[0] #318
[1] https://github.com/danielocfb/libbpf-rs/pull/2
[2] #187

Signed-off-by: Daniel Müller <deso@posteo.net>
insearchoflosttime pushed a commit that referenced this issue Jan 27, 2023
This change enables cargo-deny to run every week to check for advisories
and license violations, among other things. Doing so will make us aware
of problems potential problems in a timely fashion and give users peace
of mind [0].

[0] #187 (comment)

Closes: #187
Signed-off-by: Daniel Müller <deso@posteo.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants