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 support for rustsec advisories #62

Merged
merged 38 commits into from
Dec 18, 2019
Merged

Add support for rustsec advisories #62

merged 38 commits into from
Dec 18, 2019

Conversation

Jake-Shadle
Copy link
Member

@Jake-Shadle Jake-Shadle commented Dec 16, 2019

This PR adds a new advisories check and config section, allowing cargo-deny to fetch an advisory database (default https://github.com/RustSec/advisory-db) and check for security vulnerabilities, unmaintained crates, and security notices.

Resolves: #18
Resolves: #23
Resolves: #44
Resolves: #56

@Jake-Shadle Jake-Shadle marked this pull request as ready for review December 18, 2019 10:51
@Jake-Shadle
Copy link
Member Author

Jake-Shadle commented Dec 18, 2019

@repi not really looking for more than a sanity check on the general interface and config additions and changes, which can be mostly seen from the README.md and CHANGELOG.md changes I made.

@repi
Copy link
Contributor

repi commented Dec 18, 2019

@Jake-Shadle ok will take a look

CHANGELOG.md Outdated
- [PR#62](https://github.com/EmbarkStudios/cargo-deny/pull/62) Fixed [#56](https://github.com/EmbarkStudios/cargo-deny/issues/56), the `[metadata]` section in `Cargo.lock` is now gone in nightly to improve merging, the previous reporting mechanism that required this section has been reworked.

### Changed
- Renamed the `<which>` options for `cargo deny check <which>` from `ban` => `bans` and `license` => `licenses`
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a pretty big breaking change if someone is running it in CI, but not sure if we can highlight it more. And we are not at 1.0 yet

Copy link
Member Author

Choose a reason for hiding this comment

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

Yah, this just made it more consistent with the configuration names and everything else. But I can instead make it have both the old and new and instead mark them as deprecated.

Copy link
Contributor

Choose a reason for hiding this comment

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

That would be cleaner, and doesn't hurt anything right now. And good to get into the practice of backwards compatibility / deprecation.

@@ -17,17 +17,16 @@ track of certain things, especially as a project evolves over time, which is wha

Copy link
Contributor

Choose a reason for hiding this comment

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

Also mention security advisories and unmaintained crates up here on the top level? Think it would be great to get a good overview

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, definitely!

README.md Outdated
Comment on lines 257 to 263
#### The `db-path` field

Path to the local copy of advisory database's git repo (default: ~/.cargo/advisory-db)

#### The `db-url` field

URL to the advisory database's git repo (default: https://github.com/RustSec/advisory-db)
Copy link
Contributor

Choose a reason for hiding this comment

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

When reading this was a bit unclear to me if these are required, optional or why/when one would want or need to set them

Copy link
Member Author

Choose a reason for hiding this comment

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

Yah I should mark them as optional, I just supported it because cargo audit also supports it. And you could have private internal databases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Make sense!

Copy link
Contributor

@repi repi left a comment

Choose a reason for hiding this comment

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

Added a few comments inline, but looks good!

@Jake-Shadle Jake-Shadle merged commit c58888e into master Dec 18, 2019
@Jake-Shadle Jake-Shadle deleted the advisory-db branch December 18, 2019 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants