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

feat(rust): Update cargo deny check and deny.toml config #344

Closed
wants to merge 8 commits into from

Conversation

Mr-Leshiy
Copy link
Contributor

@Mr-Leshiy Mr-Leshiy commented Oct 31, 2024

Description

  • Enabled cargo deny RUSTSEC vulnerability check
  • Changed deny.toml configuration file check from strict to non-strict mode (means that the file could be different from the golden one, in places which are not specified by the golden config).
  • Made advisories.ignore and source.allow-git fields flexible and configurable on the rust project side itself and not strictly defined by cat-ci golden deny.toml config file

@Mr-Leshiy Mr-Leshiy self-assigned this Oct 31, 2024
@Mr-Leshiy Mr-Leshiy added the review me PR is ready for review label Oct 31, 2024
@Mr-Leshiy Mr-Leshiy marked this pull request as ready for review October 31, 2024 15:56
@@ -90,7 +90,7 @@ def main():
results.add(
vendor_files_check.toml_diff_check("/stdcfgs/clippy.toml", "clippy.toml")
)
results.add(vendor_files_check.toml_diff_check("/stdcfgs/deny.toml", "deny.toml"))
results.add(vendor_files_check.toml_diff_check("/stdcfgs/deny.toml", "deny.toml", strict=False))
Copy link
Collaborator

Choose a reason for hiding this comment

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

No, i want strict checking of cargo-deny config. Projects should not be able to modify it to suit themselves.

@@ -104,7 +104,7 @@ def main():
results.add(exec_manager.cli_run("cargo machete", name="Unused Dependencies Check"))
# Check if we have any supply chain issues with dependencies.
results.add(
exec_manager.cli_run("cargo deny check --exclude-dev -W vulnerability", name="Supply Chain Issues Check")
exec_manager.cli_run("cargo deny check --exclude-dev", name="Supply Chain Issues Check")
Copy link
Collaborator

Choose a reason for hiding this comment

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

All the reasons for ignoreing the RUST-SEC vulnerabilites as errors remain,
A single change to the rustsec database can make our code broken and give us no viable way to resolve it.
I am not filling the cargo-deny config with tons of "We can't fix this vulnerability" exclusions. Its just wrong.
If rust let us actually have some meaningful way to fix vulnerabiliteis, that would be one thing, but its just a timebomb otherwise waiting to break CI outside of our control.

@stevenj
Copy link
Collaborator

stevenj commented Nov 1, 2024

I would need to know a compelling reason why we must do this before I could approve it?

@Mr-Leshiy
Copy link
Contributor Author

@stevenj
My general idea here is to enable RUST-SEC vulnerability detection and make advisories. ignore (which is actually a RUST-SEC ignores) and allow-git fields configurable for project.
I dont have a strong position about enabling RUST-SEC vulnerability detection, it just looks not problem if we will have a possibility to easily add exceptions. From what we can see now, we need to have about 1-5 exceptions per Rust project.

With such change it slightly reduces the headache to always modify in all our rust projects these fields if it is an issue only for one repository. So with this patch it will be possible just to add an exception for particular rust package.

Also we are not actually loosing quality in general, because with this non-strict check other fields which are present in the golden config earthly/rust/stdcfgs/deny.toml are strictly verifiable (like license configs etc.), non-strict check just dont care about fields which are not present inside the golden config.

@stevenj
Copy link
Collaborator

stevenj commented Nov 1, 2024

We don't use a lot of git projects, and they are already listed. And I already said I don't want us to continually have to add ignores to the RUST-SEC section, and with this cahnge across multiple repos. its pointless if we have no way of dealing with them properly.

This change could require us to add ignores to multiple projects/repos because of a single RUST-SEC vulnerability that emerges. And we still have no way of properly correcting them, because rust doesn't allow us to force transitive dependencies to a fixed version..

@Mr-Leshiy
Copy link
Contributor Author

@stevenj Got it.
So I will close this PR then.

@Mr-Leshiy Mr-Leshiy closed this Nov 1, 2024
@Mr-Leshiy Mr-Leshiy deleted the feat/cargo-deny-check branch November 1, 2024 09:27
@Mr-Leshiy Mr-Leshiy restored the feat/cargo-deny-check branch November 1, 2024 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review me PR is ready for review
Projects
Status: 🔬 Ready For QA
Development

Successfully merging this pull request may close these issues.

2 participants