Skip to content
This repository has been archived by the owner on Jun 9, 2022. It is now read-only.

Rust support #121

Merged
merged 5 commits into from
Sep 16, 2019
Merged

Rust support #121

merged 5 commits into from
Sep 16, 2019

Conversation

bekh6ex
Copy link
Contributor

@bekh6ex bekh6ex commented Jul 10, 2019

Description

Add new module that wraps cargo audit for checking dependency versions in Cargo.lock against known vulnerabilities.

Fixes #100

Type of change

  • New feature (non-breaking change which adds functionality)

Toolchain

  • Other (Rust)

How Has This Been Tested?

  • Unit tests in /lib/modules/rust-cargoaudit/__tests__/cargoaudit-unit.js
  • Using Docker image on test sample directories
  • Using locally installed Rust toolchain on test sample directories

Test Configuration:

  • Toolchain: cargo-audit 0.6.1
  • SDK (incl. version): rustc/cargo version 1.33.0 and 1.36.0
  • OS version: MacOS
  • Relevant links (e.g. a proof-of-concept repo to test-drive the changes): project directories /lib/modules/rust-cargoaudit/__tests__/sample/default and /lib/modules/rust-cargoaudit/__tests__/sample/no-lock-file

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

mitigation: `Update "${adv.package}" crate to one of the following versions: ${patchedVersionsText}`
}
})
.reduce((results, v) => results.critical(v), new ModuleResults(key))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cargo audit report does not contain any information about severity.
I used critical as IMO it makes sense as a default value in this context.
Any opinions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe high might be enough, but that’s just semantics. If one wants to ignore it, they can do so via .hawkeyerc. Let’s wait for the corresponding ticket to be resolved and then revisit this point: rustsec/advisory-db#20

if (!fs.existsSync(path.join(fm.target, 'Cargo.lock'))) {
await exec.command('cargo generate-lockfile', { cwd: fm.target })
lockFileWasGeneratedByTheModule = true
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the package is library then Cargo.lock should not be present and cargo audit will fail.
IMO it makes sense to generate lock file and remove it after processing to make it useful for library owners as well.
Opinions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am divided on this: On the one hand, we should avoid “polluting” the target folder, on the other hand this is really useful. This might lead to some UID and GID issues when running a CI, but this can be easily mitigated with the proper docker run command. Unfortunately cargo generate-lockfile gives us no option as to where the lockfile should be created, otherwise I’d have put it into a temporary folder.
I’d say we go this route until someone complains :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There won't be any pollution because we delete it later, but there might be an issue with access to create lock file.

I’d say we go this route until someone complains :)

Totally agree 😄

Dockerfile Outdated
@@ -71,6 +71,11 @@ RUN cd /usr/local/bin && \
wget --quiet https://get.sensiolabs.org/security-checker.phar && \
chmod +x security-checker.phar

RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y
ENV PATH=/root/.cargo/bin:$PATH
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The /root part confuses me, but apparently $HOME is not accessible in Dockerfile. Couldn't find a way around it

Copy link
Collaborator

Choose a reason for hiding this comment

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

what about /usr/local/opt/?

Copy link
Collaborator

@felixhammerl felixhammerl left a comment

Choose a reason for hiding this comment

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

Let’s move the install directory away from /root, otherwise looks good.

Dockerfile Outdated
@@ -71,6 +71,11 @@ RUN cd /usr/local/bin && \
wget --quiet https://get.sensiolabs.org/security-checker.phar && \
chmod +x security-checker.phar

RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y
ENV PATH=/root/.cargo/bin:$PATH
Copy link
Collaborator

Choose a reason for hiding this comment

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

what about /usr/local/opt/?

@bekh6ex
Copy link
Contributor Author

bekh6ex commented Jul 24, 2019

@felixhammerl Done

@felixhammerl felixhammerl merged commit 55a60c8 into hawkeyesec:master Sep 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add rust dependency check with cargo-audit
2 participants