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

Vulnerability Scanning on Third Party Deps #36506

Merged
merged 225 commits into from
Nov 23, 2022
Merged

Conversation

sealesj
Copy link
Contributor

@sealesj sealesj commented Sep 29, 2022

Introduce vulnerability scanning github workflow on third party dependencies defined in the DEPS file.

Project details
The main flow of the scanning is the following:
extract third party dependencies outlined in the DEPS file
for each of those dependencies:

  • check there is a pinned commit hash value
  • use the corresponding upstream dependency url (found in DEPS file, vars section) to find a commit hash that the mirror dependency has most recently in common
  • use the above commit hash in common to query the Open Source Vulnerability database for potential vulnerabilities on the dependency
  • if a vulnerability is found, add the details to a SARIF file
    display the SARIF file report on the Flutter Engine "Security" tab
    For more details, see the design document link

Resolves b/230824334

Recipe Test:
Successful recipe run for test outlined in tests.yaml https://chromium-swarm.appspot.com/task?id=5d7f4dc5008f7810
Recipe will be added to recipes/engine which will trigger the tests defined in tests.yaml
engine/engine_lint recipe updated to ensure that the DEPS file has the correct dependency metadata -- for mirrored deps it is essential to have the upstream repo url in the DEPS file
Successful recipe test: https://chromium-swarm.appspot.com/task?id=5dccb000b2594810

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@sealesj sealesj requested review from zanderso and removed request for zanderso November 9, 2022 16:18
Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. Still hoping for answers to the open questions I had.

@sealesj
Copy link
Contributor Author

sealesj commented Nov 10, 2022

Sorry for the delay. Still hoping for answers to the open questions I had.

Apologies, I had several responses I needed to click "submit" for multiple responses

@@ -0,0 +1,56 @@
name: Third party dependency scan
Copy link
Member

Choose a reason for hiding this comment

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

What does "Notify the person who triggered it" mean? I'd prefer a notification, and not anything that could block developers, like a presubmit check. The new test added below should be the only new thing that engine team members need to pay attention to unless there's a vulnerability detected.

DEPS Show resolved Hide resolved
ci/deps_parser_tests.py Outdated Show resolved Hide resolved
}

# Eval the content.
exec(deps_content, global_scope_mirror, local_scope_mirror)
Copy link
Member

Choose a reason for hiding this comment

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

Is this resolved?

ci/deps_parser_tests.py Outdated Show resolved Hide resolved
ci/deps_parser_tests.py Outdated Show resolved Hide resolved
ci/scan_flattened_deps.py Outdated Show resolved Hide resolved
@chinmaygarde
Copy link
Member

cc @zanderso I believe this is good for another review. All outstanding questions seem to be resolved.

Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

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

This is getting close. Just a couple more comments.

ci/deps_parser.py Outdated Show resolved Hide resolved
ci/scan_flattened_deps.py Outdated Show resolved Hide resolved
@skia-gold
Copy link

Gold has detected about 1 new digest(s) on patchset 221.
View them at https://flutter-engine-gold.skia.org/cl/github/36506

Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

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

lgtm w/ optional nit

Still not entirely sure I understand how this is going to work in practice, but we can tweak it as we go.

# dep[1] contains the mirror's pinned SHA
# upstream is the origin repo
dep_name = dep[0].split('/')[-1].split('.')[0]
if UPSTREAM_PREFIX + dep_name in deps_list:
Copy link
Member

Choose a reason for hiding this comment

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

optional nit: flip the sense, do an early return, then un-indent the try: catch::

if UPSTREAM_PREFIX + dep_name not in deps_list:
  print('did not find dep: ' + dep_name)
  return {}

try:
  ...
except ...:
  ...

return {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for this recommendation! I added this early return.

@zanderso
Copy link
Member

Looks like the PR needs a rebase to pass presubs.

@skia-gold
Copy link

Gold has detected about 1 new digest(s) on patchset 222.
View them at https://flutter-engine-gold.skia.org/cl/github/36506

@sealesj sealesj added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 23, 2022
@sealesj sealesj merged commit acefe5f into flutter:main Nov 23, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 23, 2022
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Nov 23, 2022
…115952)

* 399fca706 [web] Use generic variable name for trusted url (flutter/engine#37872)

* acefe5f11 Vulnerability Scanning on Third Party Deps (flutter/engine#36506)

* 8a40e8324 Roll Dart SDK from c32f12ffbef2 to a7d1f804fa27 (1 revision) (flutter/engine#37873)
@sealesj sealesj deleted the osv-scan branch November 28, 2022 14:44
- name: setup python
uses: actions/setup-python@98f2ad02fd48d057ee3b4d4f66525b231c3e52b6
with:
python-version: '3.7.7' # install the python version needed
Copy link
Member

Choose a reason for hiding this comment

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

shogohida pushed a commit to shogohida/flutter that referenced this pull request Dec 7, 2022
…lutter#115952)

* 399fca706 [web] Use generic variable name for trusted url (flutter/engine#37872)

* acefe5f11 Vulnerability Scanning on Third Party Deps (flutter/engine#36506)

* 8a40e8324 Roll Dart SDK from c32f12ffbef2 to a7d1f804fa27 (1 revision) (flutter/engine#37873)
gspencergoog pushed a commit to gspencergoog/flutter that referenced this pull request Jan 19, 2023
…lutter#115952)

* 399fca706 [web] Use generic variable name for trusted url (flutter/engine#37872)

* acefe5f11 Vulnerability Scanning on Third Party Deps (flutter/engine#36506)

* 8a40e8324 Roll Dart SDK from c32f12ffbef2 to a7d1f804fa27 (1 revision) (flutter/engine#37873)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants