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

Add cargo audit CI #9182

Merged
merged 3 commits into from
Feb 14, 2024
Merged

Add cargo audit CI #9182

merged 3 commits into from
Feb 14, 2024

Conversation

ongchi
Copy link
Contributor

@ongchi ongchi commented Feb 9, 2024

Which issue does this PR close?

Closes #9116 .

Rationale for this change

What changes are included in this PR?

Add cargo audit CI.

Are these changes tested?

Are there any user-facing changes?

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

Thanks @ongchi
how much longer the CI build is with the new security task?

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @ongchi -- this is a good start but I don't think it quite works yet

runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: rustsec/[email protected]
Copy link
Contributor

Choose a reason for hiding this comment

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

Apache policy only allows actions from certain other repos and rustsec isn't one.

I tested this action via a PR that also updated datafusion-cli/Cargo.lock: #9185

This check fails
https://github.com/apache/arrow-datafusion/actions/runs/7849603823

Annotations
1 error
Error: .github#L1
rustsec/[email protected] is not allowed to be used in apache/arrow-datafusion. Actions in this workflow must be: within a repository owned by apache, created by GitHub, verified in the GitHub Marketplace, or matching the following: */*@[a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9]+, AdoptOpenJDK/install-jdk@*, JamesIves/github-pages-deploy-action@5dc1d5a192aeb5ab5b7d5a77b7d36aea4a7f5c92, TobKed/label-when-approved-action@*, actions-cool/issues-helper@*, actions-rs/*, al-cheb/configure-pagefile-action@*, amannn/action-semantic-pull-request@*, apache/*, burrunan/gradle-cache-action@*, bytedeco/javacpp-presets/.github/actions/*, chromaui/action@*, codecov/codecov-action@*, conda-incubator/setup-miniconda@*, container-tools/kind-action@*, container-tools/microshift-action@*, dawidd6/action-download-artifact@*, delaguardo/setup-graalvm@*, docker://jekyll/jekyll:*, docker://pandoc/core:2.9, eps1lon/actions-label-merge-conflict@*, gaurav-nelson/github-action-markdown-link-chec...

Maybe we can just install/run cargo audit directly 🤔 Or maybe https://embarkstudios.github.io/cargo-deny/ which is what we use in InfluxDb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was misunderstanding before, so it's about the policy.

Not sure if the action from Embark Studios is on the allowlist, so I'm going to install cargo-audit from cargo in the workflow at this moment.

@ongchi ongchi marked this pull request as draft February 10, 2024 01:47
@ongchi
Copy link
Contributor Author

ongchi commented Feb 12, 2024

Thanks @ongchi how much longer the CI build is with the new security task?

It takes a couple of minutes to install cargo-audit and download the advisory db while installing from Cargo. Instead of that, use rustsec/audit-check, which takes no time for this step.

The audit process takes about 10 seconds to complete.

@alamb
Copy link
Contributor

alamb commented Feb 12, 2024

👍 looks good to me -- the audit check job takes 12s to run: https://github.com/apache/arrow-datafusion/actions/runs/7877518929/job/21493850998?pr=9205

Perhaps we can simply add this to the normal CI runs on every PR rather than only on main?

@ongchi
Copy link
Contributor Author

ongchi commented Feb 13, 2024

The GitHub CI runner VM does not recreate each time it runs, the installation time of cargo-audit could be ignored in most cases. It's would be viable to include this action on every PR.

@ongchi ongchi marked this pull request as ready for review February 13, 2024 03:26
Copy link
Contributor

@Jefffrey Jefffrey left a comment

Choose a reason for hiding this comment

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

Thanks for this 👍

Comment on lines +26 to +28
paths:
- "**/Cargo.toml"
- "**/Cargo.lock"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we instead have it run each main push? e.g. in case an existing dependency gets flagged

Copy link
Contributor

@alamb alamb Feb 13, 2024

Choose a reason for hiding this comment

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

This is what @ongchi had in his initial PR (only un on main)

I think it was better to run on each PR because:

  1. It takes 12 seconds to run
  2. If it fails, this will block PRs merging and force us to fix it quickly rather than waiting for someone to notice mai is broken. If we are going to have a CI check I think we should invest the effort to keep it green

However, if others agree it would be better to run only on main, I would be fine with that too

@alamb
Copy link
Contributor

alamb commented Feb 13, 2024

The GitHub CI runner VM does not recreate each time it runs, the installation time of cargo-audit could be ignored in most cases. It's would be viable to include this action on every PR.

When I looked at the log of
https://github.com/apache/arrow-datafusion/actions/runs/7877518929/job/21493850998?pr=9205

It appears to show that cargo-audit is already installed (so "reinstalling" it is just a no op)

Run cargo install cargo-audit
    Updating crates.io index
 Downloading crates ...
  Downloaded cargo-audit v0.19.0
     Ignored package `cargo-audit v0.19.0` is already installed, use --force to override

@alamb alamb merged commit ac919d5 into apache:main Feb 14, 2024
23 checks passed
@alamb
Copy link
Contributor

alamb commented Feb 14, 2024

Thanks again @ongchi

@ongchi ongchi deleted the cargo-audit-ci branch February 15, 2024 11:11
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 this pull request may close these issues.

ci: run cargo audit
4 participants