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 a cargo deny workflow #89

Merged
merged 11 commits into from
Nov 17, 2022
Merged

Add a cargo deny workflow #89

merged 11 commits into from
Nov 17, 2022

Conversation

kayabaNerve
Copy link
Member

Also trims out a pointless submodule checkout (we have none).

Relevant to #81 and #82.

@kayabaNerve kayabaNerve added documentation Improvements or additions to documentation improvement This could be better labels Aug 17, 2022
Copy link
Collaborator

@TheArchitect108 TheArchitect108 left a comment

Choose a reason for hiding this comment

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

I think we need to ADD this TOML in the dockerfile for Serai so that it copies the deny and for other services as well. Maybe concern here is only running the action workflow, but we want it to trigger for local builds too.
ADD deny.toml .

db-path = "~/.cargo/advisory-db"
db-urls = ["https://github.com/rustsec/advisory-db"]

vulnerability = "deny"
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should add a security threshold so this doesn't trigger on every little thing. Medium?

Threshold for security vulnerabilities, any vulnerability with a CVSS score lower than the range specified will be ignored. Note that ignored advisories will still output a note when they are encountered.

  • None - CVSS Score 0.0
  • Low - CVSS Score 0.1 - 3.9
  • Medium - CVSS Score 4.0 - 6.9
  • High - CVSS Score 7.0 - 8.9
  • Critical - CVSS Score 9.0 - 10.0
    severity-threshold =

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not against this but I'd rather have the heads up so we can review it ourselves and then make a new commit explicitly allowing it. I will note the CI doesn't even run advisory checks right now for this reason, but I currently lean towards having it check everything and doing the aforementioned solution, having given it a second thought. Will reach out off GitHub to discuss.

deny.toml Show resolved Hide resolved
"RUSTSEC-2020-0071", # https://github.com/chronotope/chrono/issues/602
]

[licenses]
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we want private = { ignore = true }?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. That skips the entire tree for every unpublished crate. That means we wouldn't review our crates, nor Substrate, nor Substrate's depends.

deny.toml Show resolved Hide resolved
@TheArchitect108
Copy link
Collaborator

Also this does not do deep scanning and relies on the author to be honest about licensing. If the lib uses embedded libs, we also rely on the author to do the appropriate thing for supplying the license. May not be relevant at this point, but if we want deeper scanning in the future we should look at https://github.com/EmbarkStudios/cargo-about.

@kayabaNerve
Copy link
Member Author

kayabaNerve commented Nov 16, 2022

Placing this in the Dockerfile isn't helpful, unless the Dockerfile then runs cargo deny. I don't feel a need to have the Dockerfile run cargo deny just as it shouldn't run cargo test.

We have clarifications available in cargo deny, and there's some level of LICENSE detection, but you're right it can misrepresented. I'm not sure cargo about does anything deeper. It seems to just generate a complete report/citation list.

While this may bring down an unrelated commit, we can manually review, before creating a followup commit allowing it. If it's critical, then this did its job.
@kayabaNerve kayabaNerve merged commit 56574f2 into develop Nov 17, 2022
@kayabaNerve kayabaNerve deleted the deny branch November 17, 2022 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation improvement This could be better
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants