Skip to content

OSV export #366

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

Merged
merged 152 commits into from
Jun 28, 2021
Merged

OSV export #366

merged 152 commits into from
Jun 28, 2021

Conversation

Shnatsel
Copy link
Member

@Shnatsel Shnatsel commented May 18, 2021

This PR implements export from RustSec TOML format to the JSON-based format defined by https://github.com/google/osv

The implementation is feature-complete. The generated JSON has been successfully imported by OSV. See here for what the exported data looks like.

Summary of the changes:

  • Implemented conversion from Cargo-like selectors such as "> 2.0.0", "<= 2.5" or "^1.5" to version ranges, see OsvRange struct. This is required for OSV export.
  • Migrated our version matching logic to using OSV version ranges. This fixes a long-standing bug around handling of pre-release versions.
  • Added conversion from RustSec Advisory to data structures isomorphic to OSV JSON, which can be serialized with serde_json.
  • Implemented retrieval of file modification time from git history, which is required for OSV's modified field; see GitModificationTimes struct.
  • Added withdrawn field to RustSec advisory format, which must be set to a date. This is required for exporting to OSV. Updated the linter to require this field to be present if yanked = true. This is a breaking change under semantic versioning and caused a version bump.
  • Now that we're breaking the API anyway, changed the Versions struct to validate range consistency on creation, so we don't have to check for inconsistent ranges in every single function dealing with them.
  • Added rustsec-admin osv CLI command for exporting the RustSec TOML to OSV JSON.

Remaining merge blockers:

  • None

Remaining blockers for deploying continuous export to OSV:

@Shnatsel
Copy link
Member Author

Let me know if you'd prefer to keep the PR in draft or close it to avoid the cost of running CI on every commit.

@tarcieri
Copy link
Member

It's fine to keep it open

Shnatsel added 24 commits June 4, 2021 14:34
Merge commit 'fb90dae1947a2607a30de774a4da7e503e0d6669' into osv
… representation and an implementation detail
@Shnatsel Shnatsel marked this pull request as ready for review June 23, 2021 17:58
let commit_id = commit_id?;
let commit = repo.find_commit(commit_id)?;
// Ignore merge commits (2+ parents) because that's what 'git whatchanged' does.
// Ignore commit with 0 parents (initial commit) because there's nothing to diff against
Copy link
Contributor

Choose a reason for hiding this comment

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

Would that also mean that if a file is touched only in the initial commit it would be missing entirely? Could that be relevant to the generated advisory output in the form of a missing advisory?
Asked differently, would it hurt to diff against an empty tree (None) in that case to also fetch these paths just in case?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the general case you're probably right, but this is not relevant for the advisory database which was initialized with a README, and which has been touched since.

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'll add this in a follow-up PR so that the code is future-proof, e.g. in case we decide to flatten the repo history. Thanks for bringing this up!

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've opened #440 to address this. Thanks again!

@@ -44,6 +45,7 @@ fix = ["cargo-edit"]
git = ["crates-index", "git2", "home", "humantime", "humantime-serde"]
dependency-tree = ["cargo-lock/dependency-tree"]
vendored-openssl = ["git2/vendored-openssl"]
osv-I-know-this-is-unstable = ["git", "chrono"]
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps simply osv suffices here, and you can leave a comment regarding stability?

Copy link
Member Author

Choose a reason for hiding this comment

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

The format itself is not stabilized. I explicitly wanted to mark anything exporting to not-yet-finalized format as unstable, because it's not really exporting to OSV, it's exporting to a pre-release of OSV.

Also, renaming it to osv later will make it clear if e.g. some dependent code has been updated to the stable version or not. Not just the stable version of the code, but also the format.

Copy link
Member

@tarcieri tarcieri left a comment

Choose a reason for hiding this comment

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

A few small nits and minor suggestions, otherwise this seems fine

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.

Pre-release handling regression
3 participants