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.

Sign up for GitHub

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

Jump to bottom

Use Snyk as data provider #860

Merged
merged 12 commits into from
Aug 17, 2022
Merged

Use Snyk as data provider #860

merged 12 commits into from
Aug 17, 2022

Conversation

ManjunathMS35
Copy link
Contributor

@ManjunathMS35 ManjunathMS35 commented Aug 12, 2022 •
edited by sourabhsparkala
Loading

Fixes #717

Changes:

  • Added Snyk as data provider basically to improve the Golang project rating
  • Reduced the sub score weightage for the Dependendabot if the GitHub was used for development from 6.0 to 3.0
  • With the current change the rating for the project https://github.com/gofiber/fiber has changed from 4.78 to 4.99

ToDo:

  • If the GitHub is used as a development environment is considered in both Dependabot and Snyk due to which both can increase the score value if both tools are used in a single project. Check if this feature can be extracted separately or modify the current implement to consider if the GitHub is used for development form all data providers once and not multiple times.

sourabhsparkala
sourabhsparkala reviewed Aug 15, 2022
View reviewed changes
src/main/java/com/sap/oss/phosphor/fosstars/data/github/PackageManagement.java Outdated
@@ -83,6 +86,7 @@ public class PackageManagement extends CachedSingleFeatureGitHubDataProvider<Pac
".vcxproj"::equals, ".fsproj"::equals, "packages.config"::equals);
register(RUBYGEMS, "Gemfile.lock"::equals, "Gemfile"::equals, ".gemspec"::endsWith);
register(COMPOSER, "composer.json"::equals, "composer.lock"::equals);
register(GOMODULES, "go.mod"::equals);
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 the only form of Go Module possible for Package Management.

Maybe is there a .lock file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In addition to go.mod, the go command maintains a file named go.sum containing the expected cryptographic hashes of the content of specific module versions. Do we need to check this as well?

sourabhsparkala
sourabhsparkala reviewed Aug 15, 2022
View reviewed changes
src/test/java/com/sap/oss/phosphor/fosstars/advice/oss/SnykAdvisorTest.java
Rating rating = RatingRepository.INSTANCE.rating(OssSecurityRating.class);
ValueSet values = new ValueHashSet();
values.update(allUnknown(rating.score().allFeatures()));
values.update(USES_SNYK.value(false));
Copy link
Member

Choose a reason for hiding this comment

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

If this is false, shouldn't the Advisory list contain an advice for Snyk?

And anyways the above test method already tests this scenario, what does this test method actually do?

sourabhsparkala
sourabhsparkala reviewed Aug 15, 2022
View reviewed changes
src/main/java/com/sap/oss/phosphor/fosstars/data/github/UsesSnyk.java
private static boolean isSnyk(String name) {
return name != null && name.toLowerCase().contains(SNYK_PATTERN);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I believe most of the methods given here seems to be from UsesDependebot. Could you please provide an abstract class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

sourabhsparkala
sourabhsparkala reviewed Aug 15, 2022
View reviewed changes
src/main/java/com/sap/oss/phosphor/fosstars/model/feature/oss/OssFeatures.java Outdated
* <p>Shows if a project uses Snyk.</p>
* <p><a href="https://snyk.io/">Snyk</a> offers
* i) Static Application Security Testing (SAST) amd
* i) Static Application Security Testing (SAST) amd
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* i) Static Application Security Testing (SAST) amd

sourabhsparkala
sourabhsparkala reviewed Aug 15, 2022
View reviewed changes
src/main/java/com/sap/oss/phosphor/fosstars/model/feature/oss/OssFeatures.java Outdated
Comment on lines 175 to 182
* <p>Shows if a project uses Snyk.</p>
* <p><a href="https://snyk.io/">Snyk</a> offers
* i) Static Application Security Testing (SAST) amd
* i) Static Application Security Testing (SAST) amd
* ii) Automatic dependency updates
* In particular for automatic dependency updates,
* when Snyk finds a vulnerability in dependencies,
* it opens a pull request to update the vulnerable dependency to the safe version.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Could you format this a bit better? It looks very congested.

Copy link
Member

Choose a reason for hiding this comment

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

*<ul>
*   <li></li> 
*</ul>

sourabhsparkala
sourabhsparkala reviewed Aug 15, 2022
View reviewed changes
src/main/java/com/sap/oss/phosphor/fosstars/model/feature/oss/OssFeatures.java Outdated
* it opens a pull request to update the vulnerable dependency to the safe version.</p>
*/
public static final Feature<Boolean> USES_SNYK
= new BooleanFeature("If a project uses Snyk");
Copy link
Member

Choose a reason for hiding this comment

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

I guess here indentation seems to be a miss

sourabhsparkala
sourabhsparkala reviewed Aug 15, 2022
View reviewed changes
src/main/java/com/sap/oss/phosphor/fosstars/model/feature/oss/OssFeatures.java Outdated
* @see <a href="https://snyk.io/">Snyk</a>
*/
public static final BooleanFeature HAS_OPEN_PULL_REQUEST_FROM_SNYK
= new BooleanFeature("If a project has open pull requests from Snyk");
Copy link
Member

Choose a reason for hiding this comment

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

Indentation

Copy link
Member

Choose a reason for hiding this comment

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

This feature seems completely useless, where is it used in SnykScore?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is a good feature to increase the case in Snyk Score

sourabhsparkala
sourabhsparkala reviewed Aug 15, 2022
View reviewed changes
src/test/java/com/sap/oss/phosphor/fosstars/model/score/oss/SnykDependencyScanScoreTest.java Outdated

import com.sap.oss.phosphor.fosstars.model.Confidence;
import com.sap.oss.phosphor.fosstars.model.Score;
import com.sap.oss.phosphor.fosstars.model.math.DoubleInterval;
Copy link
Member

Choose a reason for hiding this comment

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

Remove this import

sourabhsparkala
sourabhsparkala reviewed Aug 15, 2022
View reviewed changes
src/test/resources/com/sap/oss/phosphor/fosstars/model/score/oss/DependabotScoreTestVectors.yml Outdated
Comment on lines 107 to 110
from: 1.0
openLeft: false
negativeInfinity: false
to: 9.0
to: 3.0
Copy link
Member

@sourabhsparkala sourabhsparkala Aug 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This cannot be changed like this, As the Okay rating comes only between range 5 - 8 maybe a good range

sourabhsparkala
sourabhsparkala reviewed Aug 15, 2022
View reviewed changes
...sources/com/sap/oss/phosphor/fosstars/model/score/oss/SnykDependencyScanScoreTestVectors.yml Outdated
Comment on lines 106 to 110
type: "DoubleInterval"
from: 1.0
openLeft: false
negativeInfinity: false
to: 3.0
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong range for Okay rating

sourabhsparkala
sourabhsparkala reviewed Aug 15, 2022
View reviewed changes
...sources/com/sap/oss/phosphor/fosstars/model/score/oss/SnykDependencyScanScoreTestVectors.yml
openRight: false
positiveInfinity: false
expectedLabel: null
alias: "very_good"
Copy link
Member

Choose a reason for hiding this comment

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

Please add additional test vector for Bad and moderate rating range as well

sourabhsparkala
sourabhsparkala reviewed Aug 15, 2022
View reviewed changes
...osphor/fosstars/model/score/oss/VulnerabilityDiscoveryAndSecurityTestingScoreTestVectors.yml Outdated
Comment on lines 3 to 7
- type: "BooleanValue"
feature:
type: "BooleanFeature"
name: "If a project uses Snyk"
flag: false
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this default value is not required, as you are already putting in unknown values

sourabhsparkala
sourabhsparkala reviewed Aug 15, 2022
View reviewed changes
...rces/com/sap/oss/phosphor/fosstars/model/rating/oss/OssArtifactSecurityRatingTestVectors.yml Outdated
Comment on lines 651 to 655
- type: "BooleanValue"
feature:
type: "BooleanFeature"
name: "If a project uses Snyk"
flag: false
Copy link
Member

Choose a reason for hiding this comment

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

As a default is already set, maybe this part is not required.

ManjunathMS35 and others added 4 commits August 17, 2022 09:08
@ManjunathMS35 @sourabhsparkala
Update src/test/resources/com/sap/oss/phosphor/fosstars/model/score/o…
c9873f7 
…ss/DependabotScoreTestVectors.yml

Co-authored-by: Sourabh Sarvotham Parkala <[email protected]>
@ManjunathMS35 @sourabhsparkala
Update src/test/resources/com/sap/oss/phosphor/fosstars/model/score/o…
ff843d8 
…ss/DependabotScoreTestVectors.yml

Co-authored-by: Sourabh Sarvotham Parkala <[email protected]>
@ManjunathMS35 @sourabhsparkala
Update src/test/resources/com/sap/oss/phosphor/fosstars/model/score/o…
4a381d6 
…ss/SnykDependencyScanScoreTestVectors.yml

Co-authored-by: Sourabh Sarvotham Parkala <[email protected]>
@ManjunathMS35 @sourabhsparkala
Update src/test/resources/com/sap/oss/phosphor/fosstars/model/score/o…
a1427c0 
…ss/SnykDependencyScanScoreTestVectors.yml

Co-authored-by: Sourabh Sarvotham Parkala <[email protected]>
sourabhsparkala
sourabhsparkala approved these changes Aug 17, 2022
View reviewed changes
Copy link
Member

@sourabhsparkala sourabhsparkala left a comment

Choose a reason for hiding this comment

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

👍

@sourabhsparkala sourabhsparkala added the enhancement New feature or request label Aug 17, 2022
@sourabhsparkala sourabhsparkala added this to the 1.10.0 milestone Aug 17, 2022
@ManjunathMS35 ManjunathMS35 merged commit 02f66d6 into SAP:master Aug 17, 2022
sourabhsparkala pushed a commit to sourabhsparkala/fosstars-rating-core that referenced this pull request Sep 1, 2022
@ManjunathMS35 @sourabhsparkala
Use Snyk as data provider (SAP#860)
2f63b5a 
Fixes SAP#717
sourabhsparkala pushed a commit to sourabhsparkala/fosstars-rating-core that referenced this pull request Sep 26, 2022
@ManjunathMS35 @sourabhsparkala
Use Snyk as data provider (SAP#860)
bee4b6f 
Fixes SAP#717
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sourabhsparkala sourabhsparkala sourabhsparkala approved these changes

@Sachpat Sachpat Awaiting requested review from Sachpat

Assignees

@ManjunathMS35 ManjunathMS35

Labels
enhancement New feature or request
Projects
None yet
Milestone
1.10.0
Development

Successfully merging this pull request may close these issues.

Snyk used in GitHub projects
2 participants
@ManjunathMS35 @sourabhsparkala