.github/workflows: Split advisory issues from PR workflows using `cargo-deny` #2803

pinkforest · 2022-08-06T18:46:24Z

Description

actions-rs/check-audit is not maintained

Links to any relevant issues

rust-secure-code/wg#46

Open Questions

It is currently complaining about owning_ref - we added advisory few days ago

Please review to make sure this is addressed e.g. potentially supressing / migration etc.

I've also added this into yaml:

    # Prevent sudden announcement of a new advisory from failing ci:
    continue-on-error: ${{ matrix.checks == 'advisories' }}

Note: Please review licenses

Cargo deny does license check and I had to add the ring crate specific license and Unicode unicode-ident as special cases

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
A changelog entry has been made in the appropriate crates - N/A

thomaseizinger

I am in favor of this.

thomaseizinger · 2022-08-08T15:26:25Z

deny.toml

+# https://embarkstudios.github.io/cargo-deny/checks/bans/cfg.html
+[bans]
+# Lint level for when multiple versions of the same crate are detected
+multiple-versions = "warn"


I'd love to ban this actually but I think that is gonna be a fair bit of work.

Out of curiosity, where are we at today?

I think the best we can do is be a good citizen and merge @dependabot pull requests quickly.

Out of curiosity, where are we at today?

Something like 20 duplicates I think.

I think the best we can do is be a good citizen and merge @dependabot pull requests quickly.

We can do better and regularly check for duplicate dependencies and investigate, where they come from. We can also audit, which dependencies we have and try to get rid of some. Another way is to audit for set but unused features or entire dependencies altogether.

For example, we pull in an old version of rand (0.7) via ed25519-dalek.

Several of the windows specific dependencies are also duplicates I think.

ed25519-dalek is not maintained.
and there is another concern about it which doesn't break anything but it's not good either
rustsec/advisory-db#1360
I don't think any of this stops this PR tho ?

It doesn't no, sorry for the OT.

thomaseizinger · 2022-08-08T15:31:34Z

The actual issue is already being addressed: prometheus/client_rust#78

thomaseizinger · 2022-08-08T23:02:48Z

Friendly note @pinkforest : We squash merge here and favor merges over force-pushes. Thank you :)

pinkforest · 2022-08-08T23:16:53Z

Oh that was just github's catch-up rebase, didn't want to bother with merge from main commits noise - but noted thanks :)

p.s. needs CONTRIBUTING.md 🤷‍♀️

EDIT: p.p.s. a CONTRIBUTING.md I eventually found - https://github.com/ipfs/community/blob/master/CONTRIBUTING.md#rebasing instructs push -f

mxinden

Thanks @pinkforest and @thomaseizinger for the help on this!

.github/workflows/cargo-audit.yml

deny.toml

mxinden · 2022-08-10T02:37:47Z

deny.toml

+# https://embarkstudios.github.io/cargo-deny/checks/bans/cfg.html
+[bans]
+# Lint level for when multiple versions of the same crate are detected
+multiple-versions = "warn"


Out of curiosity, where are we at today?

I think the best we can do is be a good citizen and merge @dependabot pull requests quickly.

pinkforest · 2022-08-10T02:46:26Z

multiple-versions - I didn't get any warnings but there are some weird SemVer stuff that sometimes it's impossible to deal with so it's best to leave as warn - reality of dealing with rust ecosystem.

I will send a commit around getting the separate cron-check for advisories and will separate it from the PR CI workflow

pinkforest · 2022-08-10T04:17:01Z

I just switched the PR to be different flow with deny from periodic one that uses audit

The periodic will catch any advisories and beep about them as usual and the PR / push will catch license / shady dependencies

mxinden · 2022-08-17T03:58:45Z

@pinkforest why do we need both cargo audit and cargo deny?

pinkforest · 2022-08-17T04:48:01Z

Audit is for the advisories on scanning periodically.

Deny is for the PR scanning which includes license checks.

As you can see other is on PR / push and the other one is for cron basis.

It's better to use both.

thomaseizinger · 2022-08-17T05:49:33Z

@pinkforest why do we need both cargo audit and cargo deny?

cargo deny doesn't open issues I think although we could potentially script that with the pre-installed gh CLI.

thomaseizinger · 2022-08-17T05:57:16Z

I just switched the PR to be different flow with deny from periodic one that uses audit

The periodic will catch any advisories and beep about them as usual and the PR / push will catch license / shady dependencies

Thanks!

Can we split this PR into two then? Changing cargo audit to only run on a schedule and using cargo deny are two different things IMO.

I think the latter is pretty uncontroversial whereas the former may need a bit of discussion.

I am okay with moving away from a PR-based audit with cargo audit. We will still get the open issue and can prioritise work to fix them whilst being able to merge other PRs.

For cargo deny, we could remove the continue-on-error config and just not mark it as mandatory for merge.

pinkforest · 2022-08-17T05:59:23Z

I am not changing your existing cargo audit except modifying it to exclude PR pushes which is being switched to cargo deny.

So I don't think there is much to gain to separate PRs at this point.

Happy to take the continue-on-error out.

mxinden

One comment. Otherwise looks good to me. In the future, agreed with Thomas that separate pull requests are easier.

.github/workflows/cargo-deny-pr.yml

Signed-off-by: pinkforest <[email protected]>

mxinden · 2022-08-25T08:59:59Z

.github/workflows/cargo-deny-pr.yml

+      matrix:
+        checks:
+          - advisories
+          - bans licenses sources


Given that we no longer treat advisories special, is the matrix still needed? Also do we need to list the commands any longer at all? Wouldn't a cargo deny check suffice to run them all?

I personally prefer to be explicit as if there is change in the future you can gradually adopt more check features.

If the preference is to be implicit non-exhaustive then we can change this but prepare for surprise(s) if the tooling changes.

Also I personally prefer matrix to separate the workflow streams so I can catch / view errors in context e.g. I want to have different mindset when dealing with licenses vs advisories - plus it's faster as silver lining.

If the preference is to bundle them to same flow then that can be certainly changed.

Let me know and I can change.

I'd like to keep it separate actually so we can temporarily ignore a failing advisory check for example. It also allows us to mark some of them as required via GH for merging and leave others optional.

.github/workflows/cargo-deny-pr.yml

thomaseizinger · 2022-08-29T12:27:48Z

@mxinden @pinkforest I tried to summarize what this PR does in a new title. Improvements welcome!

Signed-off-by: pinkforest <[email protected]>

thomaseizinger

LGTM! Thanks for the contribution and sorry for the delay in getting this reviewed1

thomaseizinger · 2022-08-29T13:10:51Z

@mxinden Feel free to merge if you also agree with the changes.

mxinden

🙏

pinkforest mentioned this pull request Aug 6, 2022

Address RUSTSEC-2021-0124 GothenburgBitFactory/taskwarrior#2830

Closed

thomaseizinger approved these changes Aug 8, 2022

View reviewed changes

Switch to cargo-deny GH action

526ff8d

pinkforest force-pushed the feat/switch/deny branch from 46eac3a to 526ff8d Compare August 8, 2022 15:57

pinkforest mentioned this pull request Aug 8, 2022

CONTRIBUTING.md #2808

Closed

mxinden reviewed Aug 10, 2022

View reviewed changes

Separate PR deny and Periodic audit flows

e01935a

pinkforest added 3 commits August 13, 2022 19:00

Merge branch 'master' into feat/switch/deny

bcf69a8

Merge branch 'master' into feat/switch/deny

db7c84d

Merge branch 'master' into feat/switch/deny

5108516

mxinden reviewed Aug 23, 2022

View reviewed changes

.github/workflows/cargo-deny-pr.yml Outdated Show resolved Hide resolved

Fail on PR upon advisory

035a2a8

Signed-off-by: pinkforest <[email protected]>

mxinden reviewed Aug 25, 2022

View reviewed changes

thomaseizinger reviewed Aug 28, 2022

View reviewed changes

.github/workflows/cargo-deny-pr.yml Outdated Show resolved Hide resolved

thomaseizinger changed the title ~~Switch to cargo-deny GH action~~ .github/workflows: Split advisory issues from PR workflows using cargo-deny Aug 29, 2022

Remove .lock

75b9e42

Signed-off-by: pinkforest <[email protected]>

thomaseizinger mentioned this pull request Aug 29, 2022

Consider tracking Cargo.lock in Git #2853

Closed

thomaseizinger approved these changes Aug 29, 2022

View reviewed changes

mxinden approved these changes Aug 30, 2022

View reviewed changes

Merge branch 'master' into feat/switch/deny

86986ba

mxinden merged commit f16561c into libp2p:master Aug 30, 2022

pinkforest deleted the feat/switch/deny branch August 30, 2022 09:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

.github/workflows: Split advisory issues from PR workflows using `cargo-deny` #2803

.github/workflows: Split advisory issues from PR workflows using `cargo-deny` #2803

pinkforest commented Aug 6, 2022 •

edited

Loading

thomaseizinger left a comment

thomaseizinger Aug 8, 2022

mxinden Aug 10, 2022

thomaseizinger Aug 17, 2022

pinkforest Aug 17, 2022 •

edited

Loading

thomaseizinger Aug 28, 2022

thomaseizinger commented Aug 8, 2022

thomaseizinger commented Aug 8, 2022

pinkforest commented Aug 8, 2022 •

edited

Loading

mxinden left a comment

mxinden Aug 10, 2022

pinkforest commented Aug 10, 2022

pinkforest commented Aug 10, 2022 •

edited

Loading

mxinden commented Aug 17, 2022

pinkforest commented Aug 17, 2022

thomaseizinger commented Aug 17, 2022 •

edited

Loading

thomaseizinger commented Aug 17, 2022

pinkforest commented Aug 17, 2022

mxinden left a comment

mxinden Aug 25, 2022

pinkforest Aug 27, 2022 •

edited

Loading

thomaseizinger Aug 28, 2022

thomaseizinger commented Aug 29, 2022

thomaseizinger left a comment

thomaseizinger commented Aug 29, 2022

mxinden left a comment

.github/workflows: Split advisory issues from PR workflows using cargo-deny #2803

.github/workflows: Split advisory issues from PR workflows using cargo-deny #2803

Conversation

pinkforest commented Aug 6, 2022 • edited Loading

Description

Links to any relevant issues

Open Questions

Note: Please review licenses

Change checklist

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger Aug 8, 2022

Choose a reason for hiding this comment

mxinden Aug 10, 2022

Choose a reason for hiding this comment

thomaseizinger Aug 17, 2022

Choose a reason for hiding this comment

pinkforest Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

thomaseizinger Aug 28, 2022

Choose a reason for hiding this comment

thomaseizinger commented Aug 8, 2022

thomaseizinger commented Aug 8, 2022

pinkforest commented Aug 8, 2022 • edited Loading

mxinden left a comment

Choose a reason for hiding this comment

mxinden Aug 10, 2022

Choose a reason for hiding this comment

pinkforest commented Aug 10, 2022

pinkforest commented Aug 10, 2022 • edited Loading

mxinden commented Aug 17, 2022

pinkforest commented Aug 17, 2022

thomaseizinger commented Aug 17, 2022 • edited Loading

thomaseizinger commented Aug 17, 2022

pinkforest commented Aug 17, 2022

mxinden left a comment

Choose a reason for hiding this comment

mxinden Aug 25, 2022

Choose a reason for hiding this comment

pinkforest Aug 27, 2022 • edited Loading

Choose a reason for hiding this comment

thomaseizinger Aug 28, 2022

Choose a reason for hiding this comment

thomaseizinger commented Aug 29, 2022

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger commented Aug 29, 2022

mxinden left a comment

Choose a reason for hiding this comment

.github/workflows: Split advisory issues from PR workflows using `cargo-deny` #2803

.github/workflows: Split advisory issues from PR workflows using `cargo-deny` #2803

pinkforest commented Aug 6, 2022 •

edited

Loading

pinkforest Aug 17, 2022 •

edited

Loading

pinkforest commented Aug 8, 2022 •

edited

Loading

pinkforest commented Aug 10, 2022 •

edited

Loading

thomaseizinger commented Aug 17, 2022 •

edited

Loading

pinkforest Aug 27, 2022 •

edited

Loading