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

Migrate to GitHub Native Dependabot #917

Merged
merged 3 commits into from
May 17, 2021

Conversation

jtwillis92
Copy link

@jtwillis92 jtwillis92 commented May 14, 2021

Summary of Changes

Currently our Snyk configuration is configured on the carltonsmith Snyk organization which will no longer work after @carltonsmith leaves the project. Additionally, these Snyk PRs require us to manage an unnecessary requirements.txt file in addition to our Pipfile and python dependency update PRs are not complete when they get opened since they don't update anything in our actual dependencies. Example Snyk Python Dependency PR

Additionally, we currently use the Dependabot Preview app which is being deprecated in favor of a GitHub Native Dependabot which has more features and is configured via a YAML file committed to the repo.

Rather than setting up Snyk on a new organization and in order to get ahead on the impending Dependabot migration this PR proposes an update which provides the necessary YAML config to enable the new GitHub Native version of Dependabot. The new version is capable of providing both dependency version upgrades as well as security upgrades in response to vulnerabilities, which covers the functionality provided by both Snyk and Dependabot Preview.

NOTE: We may want to explicitly disable automated PR updates on the HHS repo if it is not already, otherwise once this file gets merged in we will get Dependabot PRs to both repos, which would conflict with our git flow as they would be opened on divergent branches. This can be disabled in the security analysis settings as shown below:
disable-dependabot-security-updates

How to Test

Unfortunately due to the way Dependabot works we can't fully test this until it gets merged. On merge to raft-tdp-main a scan should begin immediately and the status checks should update shortly thereafter in the Dependency Graph

We can however, run the proposed YAML file against the Dependabot provided validator here as shown below:
Screenshot from 2021-05-14 18-06-14

Deliverable 1: Accepted Features

Performance Standard(s): At the beginning of each sprint, the Product Owner and development team will collaborate to define a set of user stories to be completed during the sprint. Acceptance criteria for each story will also be defined. The development team will deliver code and functionality to satisfy these user stories.

Acceptable Quality Level: Delivered code meets the acceptance criteria for each user story. Incomplete stories will be assessed and considered for inclusion in the next sprint.

  • Look up the acceptance criteria in the related issue; paste ACs below in checklist format.
  • Check against the criteria:
    • Project dependencies can be automatically kept up to date.
    • Security vulnerabilities in project dependencies can be automatically resolved.
    • All relevant documentation is updated to reflect this change.

As facilitator/product manager, @kniz-raft will decide if ACs are met from Raft's perspective.

Deliverable 2: Tested Code

Performance Standard(s): Code delivered under the order must have substantial test code coverage. Version-controlled HHS GitHub repository of code that comprises products that will remain in the government domain.

Acceptable Quality Level: Minimum of 90% test coverage of all code. All areas of code are meaningfully tested.

N/A changes relate only to dependency and vulnerability management

Deliverable 3: Properly Styled Code

Performance Standard(s): GSA 18F Front- End Guide

Acceptable Quality Level: 0 linting errors and 0 warnings

N/A changes relate only to dependency and vulnerability management

Deliverable 4: Accessible

Performance Standard(s): Web Content Accessibility Guidelines 2.1 AA standards

Acceptable Quality Level: 0 errors reported using an automated scanner and 0 errors reported in manual testing

N/A no UX changes

Deliverable 5: Deployed

Performance Standard(s): Code must successfully build and deploy into the staging environment.

Acceptable Quality Level: Successful build with a single command

NOTE: until we have a proper staging environment this may not be satisfiable prior to merging

N/A changes relate only to dependency and vulnerability management

Deliverable 6: Documented

Performance Standard(s): Summary of user stories completed every two weeks. All dependencies are listed and the licenses are documented. Major functionality in the software/source code is documented, including system diagram. Individual methods are documented inline in a format that permits the use of tools such as JSDoc. All non-inherited 800-53 system security controls are documented in the Open Control or OSCAL format and HHS Section 508 Product Assessment Template (PAT) are updated as appropriate.

Acceptable Quality Level: Combination of manual review and automated testing, if available

  • If this PR introduces backend code, is that code documented both inline and overall?
  • If this PR introduces frontend code, is that code documented both inline and overall?
  • If this PR introduces dependencies, are their licenses documented?

Deliverable 7: Secure

Performance Standard(s): Open Web Application Security Project (OWASP) Application Security Verification Standard 3.0

Acceptable Quality Level: Code submitted must be free of medium- and high-level static and dynamic security vulnerabilities

  • Does the OWASP Scan pass on CircleCI?
  • Do manual code review and manual testing detect any security issues?

dependencies:
"@types/graphlib" "^2"

"@snyk/cocoapods-lockfile-parser@3.6.2":
Copy link
Author

Choose a reason for hiding this comment

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

Snyk was installing a ton of packages we didn't actually need. Dependabot can now handle all of this within Github natively without needing to affect our dev dependencies - this may help speed up local builds as an added bonus.

@@ -44,7 +44,6 @@
"test": "react-scripts test",
"test:cov": "react-scripts test --coverage --watchAll",
"test:ci": "CI=1 react-scripts test --coverage",
"snyk": "snyk test",
Copy link
Author

Choose a reason for hiding this comment

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

This was only used for running snyk checks locally if needed.

@@ -1,69 +0,0 @@
# Snyk (https://snyk.io) policy file, patches or ignores known vulnerabilities.
Copy link
Author

Choose a reason for hiding this comment

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

These ignores are all expired even if we choose to continue using Snyk on a different organization instead of accepting this PR's changes.

@@ -1,53 +0,0 @@
-i https://pypi.org/simple
Copy link
Author

Choose a reason for hiding this comment

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

Dependabot can manage Python dependencies using the Pipfile so we no longer need to maintain this file. (It's out of date anyway - note this closed vulnerability, in our Pipfile.lock version 3.3.2 of cryptography is currently in use)

## Dependabot Security Analysis

Dependabot status badges have not yet been updated to work with GitHub Native Dependabot per [this open issue](https://github.com/dependabot/dependabot-core/issues/1912). In lieu of these badges links are provided to the Dependabot alerts page on raft-tech and the Security Advisories page on HHS.
Copy link
Author

Choose a reason for hiding this comment

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

Just want to point this out - the badges aren't currently working correctly for GitHub Native Dependabot so they were not used in this PR.

@jtwillis92 jtwillis92 added raft review This issue is ready for raft review dependencies Pull requests that update a dependency file documentation security labels May 14, 2021
Copy link

@jorgegonzalez jorgegonzalez left a comment

Choose a reason for hiding this comment

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

LGTM, dropping Snyk was long overdue in my opinion.

@jtwillis92 jtwillis92 added QASP Review and removed raft review This issue is ready for raft review labels May 17, 2021
@jtwillis92 jtwillis92 requested a review from ADPennington May 17, 2021 19:11
Co-authored-by: Alex P.  <63075587+ADPennington@users.noreply.github.com>
Copy link
Collaborator

@ADPennington ADPennington left a comment

Choose a reason for hiding this comment

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

👍🏾 👍🏾 confirming that dependabot alerts disabled in hhs repo. given that there is a snyk upgrade ready to merge (#886) , let's hold off on the merge to test if merging this one first will detect the need for dependency upgrade.

ACs:

  • Project dependencies can be automatically kept up to date.
  • Security vulnerabilities in project dependencies can be automatically resolved.
  • All relevant documentation is updated to reflect this change.

Code changes:

  • If this PR introduces backend code, is that code documented both inline and overall?
  • If this PR introduces frontend code, is that code documented both inline and overall?

Security:

  • Does the OWASP Scan pass on CircleCI?
  • Do manual code review and manual testing detect any security issues?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file documentation Ready to Merge security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants