CSUB-386: Enable cargo audit in CI

[atodorov]

Description of proposed changes:

Practical tips for PR review & merge:

• All GitHub Actions report PASS
• Newly added code/functions have unit tests
  • Coverage tools report all newly added lines as covered
  • The positive scenario is exercised
  • Negative scenarios are exercised, e.g. assert on all possible errors
  • Assert on events triggered if applicable
  • Assert on changes made to storage if applicable
• Modified behavior/functions - try to make sure above test items are covered
• Integration tests are added if applicable/needed

[codecov-commenter]

Codecov Report

❗ No coverage uploaded for pull request base (dev@f1d3463). Click here to learn what that means.
The diff coverage is n/a.

@@          Coverage Diff           @@
##             dev     #880   +/-   ##
======================================
  Coverage       ?   77.62%           
======================================
  Files          ?       75           
  Lines          ?    11383           
  Branches       ?        0           
======================================
  Hits           ?     8836           
  Misses         ?     2547           
  Partials       ?        0           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[github-actions]

For full LLVM coverage report click here!

[AdaJane]

The report being output by the audit points to the time library as being the only one with a real "problem" (segFault). It looks like this is depended on by some of the runtime related crates.

If that library is being used inside the runtime, it's not a security concern because of the wasmtime handles segFaults (terminates the process and 0s the memory).

One question I have from looking at this and the the main cargo.toml, "Does dependabot do anything about explicit version dependencies?".

I would assume, for example, that this dependency:
tracing = { version = "5.0.0", default-features = false, branch = "polkadot-v0.9.32", git = "https://github.com/paritytech/substrate.git", package = "sp-tracing" }
would not be automatically updated because it both depends on an explicit and exact version, and it comes from a GitHub repo.

Still digging, but any insights are appreciated.

[atodorov]

I would assume, for example, that this dependency:
tracing = { version = "5.0.0", default-features = false, branch = "polkadot-v0.9.32", git = "https://github.com/paritytech/substrate.git", package = "sp-tracing" }
would not be automatically updated

That's correct. Dependabot will not do anything about it.

[WilfredTA]

Referring to this issue in the chrono repository. Projects that have referenced that issue appear to have fixed the issue in one of a few different ways:

1. Update the time dependency to a version that patches this vulnerability
2. Disable chrono default features, which disables the oldtime feature, which prevents the dependency on time crate
3. Wait for chrono 0.5 upstream, which removes this dependency.

However, maintainers of chrono have stated that chrono does not use the vulnerable API in time crate.

Unfortunately, this would not fix the problem... because it appears that chrono has the same vulnerability in its own source code that the time crate has, as discussed here. This vulnerability is described in RUSTSEC-2020-0159. The PR that introduced the 2020-0159 security advisory states that this has impacted at least one project running in production.

We need to look at the execution paths & conditions to determine whether this is a concern for us. Unfortunately, it appears that some of the dependencies that utilize chrono are also no longer maintained, so we wouldn't expect these fixes to come upstream. Let's determine how our third party libraries use chrono and if their usage leads to erroneous behavior.

The fact that multiple core dependencies are no longer maintained brings up another issue around how large our "trusted base" is, given the amount of dependencies the project has.

Let's take a little more time to analyze the short term risks of the failures reported by cargo-audit as well as the long term consequences of the deps we are using.

[AdaJane]

Referring to this issue in the chrono repository. Projects that have referenced that issue appear to have fixed the issue in one of a few different ways:

The fact that multiple core dependencies are no longer maintained brings up another issue around how large our "trusted base" is, given the amount of dependencies the project has.

Let's take a little more time to analyze the short term risks of the failures reported by cargo-audit as well as the long term consequences of the deps we are using.

The time dependency here looks like it's being pulled in by Substrate related crates. There are newer versions of these crates available now on crates.io (as opposed the github they're currently being pulled from), but several of the newer crates introduce new names for existing types as well as moving types we're using to new modules. There would be a bit of effort involved in migrating to the latest Substrate crates, but it may help with some of our dependency concerns here.

The downside is that by upgrading the substrate crates we go from ~900 dependencies to just over 1100, so not the ideal direction.

[AdaJane]

I'm not crazy about ignoring all indirect dependencies, but I think this is still a nice addition to the pipeline.

Edit: After clarification, everything makes sense

[nathanwhit]

A couple questions/comments