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

fix(deps): Replace openssl with rustls in tests and experimental features #7047

Merged
merged 7 commits into from
Jun 26, 2023

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Jun 22, 2023

Motivation

OpenSSL continues to have memory safety vulnerabilities:
GHSA-xcf7-rvmh-g6q4

zebrad isn't impacted by this one in production, because openssl is only a dependency of our tests and experimental features. But it could accidentally become a production dependency when we stabilise features or add new dependencies.

Reference

GitHub Actions ternary expressions:
https://docs.github.com/en/actions/learn-github-actions/expressions#example

Complex Code or Requirements

--all-features activates the openssl dependency, because it's an optional dependency of some of Zebra's dependencies. So I changed the CI check to ignore banned crates for --all-features. (And ignore missing dependencies for default features.)

Solution

  • Ban openssl as a Zebra dependency
  • Make CI check that openssl is not used in default production builds and production Docker images

Related fixes:

  • Add a default-release-binaries feature to zebrad and use it in Docker files and production/prod test workflows
  • Ignore expected warnings for default features and default-release-binaries
  • Ban wildcard ("any version") dependencies, except in tests

Review

This is a low priority security fix.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

Follow Up Work

It would be good to automate keeping the duplicate dependency exception list up to date.

@teor2345 teor2345 added C-bug Category: This is a bug A-dependencies Area: Dependency file updates P-Low ❄️ C-security Category: Security issues A-network Area: Network protocol updates or fixes I-memory-safety Vulnerable code in Zebra or dependencies labels Jun 22, 2023
@teor2345 teor2345 requested a review from a team as a code owner June 22, 2023 02:53
@teor2345 teor2345 self-assigned this Jun 22, 2023
@teor2345 teor2345 requested review from a team as code owners June 22, 2023 02:53
@teor2345 teor2345 requested review from gustavovalverde and oxarbitrage and removed request for a team June 22, 2023 02:53
@github-actions github-actions bot added C-feature Category: New features C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels Jun 22, 2023
@codecov
Copy link

codecov bot commented Jun 22, 2023

Codecov Report

Merging #7047 (1a09fc9) into main (0e0ee8d) will increase coverage by 0.00%.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7047   +/-   ##
=======================================
  Coverage   77.43%   77.44%           
=======================================
  Files         310      310           
  Lines       41694    41694           
=======================================
+ Hits        32287    32289    +2     
+ Misses       9407     9405    -2     

Copy link
Member

@gustavovalverde gustavovalverde left a comment

Choose a reason for hiding this comment

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

I don't think I'm following up why we would require a new feature name (default-docker) shouldn't default sentry have the same behavior? It might be confusing as why Docker needs a "special" flag.

@teor2345
Copy link
Contributor Author

I don't think I'm following up why we would require a new feature name (default-docker) shouldn't default sentry have the same behavior? It might be confusing as why Docker needs a "special" flag.

At the moment we're repeating the sentry feature in about 4 different places in our test and production release CI workflows. And this PR adds a 5th location that's security-sensitive. (Banning OpenSSL in released binaries.)

So if we change our set of default features in released binaries, we'll need to remember to update all those locations. But we'll probably forget one!

Would it be clearer if I renamed it to something like:

  • default-release-binaries
  • zf-published-binaries
  • zf-binary-releases

We should be using the same features for all our released binaries. Even if we're only releasing Docker binaries at the moment.

@gustavovalverde
Copy link
Member

Understood!

I think both of this options should be good:

  • default-release-binaries
  • zf-binary-releases

I'd vote for the former: default-release-binaries

@teor2345
Copy link
Contributor Author

Done, and merge conflicts resolved.

@teor2345 teor2345 requested review from gustavovalverde and removed request for a team June 26, 2023 00:52
Copy link
Member

@gustavovalverde gustavovalverde left a comment

Choose a reason for hiding this comment

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

LGTM!

mergify bot added a commit that referenced this pull request Jun 26, 2023
@mergify mergify bot merged commit 76a7ff4 into main Jun 26, 2023
@mergify mergify bot deleted the no-openssl branch June 26, 2023 05:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependencies Area: Dependency file updates A-network Area: Network protocol updates or fixes C-bug Category: This is a bug C-feature Category: New features C-security Category: Security issues C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG I-memory-safety Vulnerable code in Zebra or dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants