✨ Included documentation for Active check #347

naveensrinivasan · 2021-04-17T13:05:12Z

Please check if the PR fulfills these requirements

Tests for the changes have been added (for bug fixes / features)
PR title follows the guidelines defined in https://github.com/ossf/scorecard/blob/main/CONTRIBUTING.md#pr-process

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Feature
What is the current behavior? (You can also link to an open issue here)
The check results didn't provide a description and remediation.
What is the new behavior (if this is a feature change)?
- Included the documentation for Active check
- The building block for SARIF Integration with CI/CD and GitHub Code Scanning Results #193
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
None
Other information:
None

github-actions · 2021-04-17T13:10:55Z

Integration tests success for 73e6676695e692677e328e01143b62927a33f24d

inferno-chromium · 2021-04-17T18:16:38Z

Some things i noticed

json file is not the source of truth, both checks.json and checks.md are generated files from checks.yaml, so i think program should use checks.yaml directly and we shouldn't modify json file here (like indent can be fixed in auto-generation logic).
i dont think we want to add long description and remediation in output, since it will inflate latest.json significantly. i think the point was for checks.json would be read by some utility directly alongwith latest.json. however, I do see the point of showing this info when tool is run in standalone fashion like one repo only, so need to see how to show it only for that usecase.

i would let @azeemshaikh38 @oliverchang and @naveensrinivasan to sync here and discuss a solution here, just added my thoughts above.

naveensrinivasan · 2021-04-17T18:41:21Z

Some things i noticed

json file is not the source of truth, both checks.json and checks.md are generated files from checks.yaml, so i think program should use checks.yaml directly and we shouldn't modify json file here (like indent can be fixed in auto-generation logic).

i dont think we want to add long description and remediation in output, since it will inflate latest.json significantly. i think the point was for checks.json would be read by some utility directly alongwith latest.json. however, I do see the point of showing this info when tool is run in standalone fashion like one repo only, so need to see how to show it only for that usecase.

i would let @azeemshaikh38 @oliverchang and @naveensrinivasan to sync here and discuss a solution here, just added my thoughts above.

I agree , I will change the implementation to use the YAML and will add HelpURL which points to the GitHub instead of remediation.

naveensrinivasan · 2021-04-17T20:10:38Z

Some things i noticed

json file is not the source of truth, both checks.json and checks.md are generated files from checks.yaml, so i think program should use checks.yaml directly and we shouldn't modify json file here (like indent can be fixed in auto-generation logic).

i dont think we want to add long description and remediation in output, since it will inflate latest.json significantly. i think the point was for checks.json would be read by some utility directly alongwith latest.json. however, I do see the point of showing this info when tool is run in standalone fashion like one repo only, so need to see how to show it only for that usecase.

i would let @azeemshaikh38 @oliverchang and @naveensrinivasan to sync here and discuss a solution here, just added my thoughts above.

I agree , I will change the implementation to use the YAML and will add HelpURL which points to the GitHub instead of remediation.

Updated the implementation to utilize the YAML instead of the json

github-actions · 2021-04-17T20:11:22Z

Integration tests failure for 14d6deeb3beb1d7ef53a34ea3d29d78828c14847

* Included the documentation for Active check * Embed the checks.yaml into the binary

github-actions · 2021-04-17T20:20:01Z

Integration tests success for 9214dd3dc18519a8a26929ce54d2f1437ef0017a

azeemshaikh38

If this change is attempting to solve the "upload a SARIF file to GitHub" problem, should we first figure out the design? IMO, we should hold off on this PR before we have a consensus on how the design for the bigger problem would look like.

azeemshaikh38 · 2021-04-18T06:20:03Z

checker/check_result.go

@@ -23,6 +23,8 @@ type CheckResult struct {
 	Details     []string
 	ShouldRetry bool  `json:"-"`
 	Error       error `json:"-"`
+	Description string


Let's not bloat CheckResult with Description and HelpURL. We should have a separate struct for these which other packages can access as/when needed.

The result is where the Help documentation needs to have the necessary stuff and this is what is being used in output.

Where else do you think this could be used?

How do you see this as bloat?

Also, what would be your suggestion?

azeemshaikh38 · 2021-04-18T06:24:33Z

checks/documentation.go

@@ -0,0 +1,90 @@
+// Copyright 2020 Security Scorecard Authors


Change filename -> check_documentation.go

It is already within the checks package. Adding prefix doesn't add any value and also _ aren't common within standard go naming though there are few exceptions.

azeemshaikh38 · 2021-04-18T06:24:36Z

checks/documentation.go

+}
+
+// HelpDocumentation provides the documentation for the checks.
+type HelpDocumentation struct {


We can expose this struct to other packages to get Description/HelpURL.

azeemshaikh38 · 2021-04-18T06:24:43Z

checks/documentation.go

+}
+
+// ActiveHelpDocumentation provides the help documentation for ActiveHelpDocumentation check.
+func ActiveHelpDocumentation() (HelpDocumentation, error) {


Everytime a *HelpDocumentation is called, we will read the YAML file, unmarshal it and then send the result back. Instead, could we only unmarshal the first time, and have a var which saves this in memory so that the next time these fn calls happen, we only read from the in-memory var?

The idea behind this is to avoid a global state. This performance is negligible. IMO avoiding state is cleaner design.

naveensrinivasan · 2021-04-18T14:59:43Z

If this change is attempting to solve the "upload a SARIF file to GitHub" problem, should we first figure out the design? IMO, we should hold off on this PR before we have a consensus on how the design for the bigger problem would look like.

It is not just for SARIF, anyone who uses the tool would like to know details and steps to address these issues #63

oliverchang · 2021-04-19T02:24:12Z

checks/documentation.go

+
+type documentation struct {
+	Checks struct {
+		SecurityPolicy struct {


This is a lot of repetition and makes maintenance difficult. Can we marshal these into a map[string]CheckDescription or something similar?

There is also a con with that approach, we lose the type-safety and strings are all over the codebase.

We would have to define a constant for the string and use it which beats the purpose.

oliverchang · 2021-04-19T02:41:06Z

cmd/root.go

+			Name:        r.Name,
+			Pass:        r.Pass,
+			Confidence:  r.Confidence,
+			Description: r.Description,


I also don't think we should include these long strings in these results by default. It will blow up the latest.json and cause a lot of duplication.

If we need this for another use case, we should figure out another way to do this.

I can think of a flag that either dumps the help information in the json or not.

Do you have any other suggestions?

Me and Oliver synced over this today. This is what we thought - #63 wants to address the problem that if Scorecard tool fails, the user should know "why the check failed", "how confident are we about this result" and "how to fix it". This PR won't solve that issue. A simple example - if the check failed due to too many retries, we'd simply be outputting a generic description and helpURL. This does not help our case in making the tool more user friendly.

So here's my suggestion -

Let's update all of our checks to return error_codes on failure. Each unique case of failure should have a unique error_code. These can simply be appended to []details of CheckResult for now.

We should then update the YAML to add details/remediation corresponding to these unique failures.

Update cmd/root.go to include the remediation string from YAML if any of the checks fail.

What do you think? Also adding @oliverchang @inferno-chromium for feedback/opinions.

Sounds like a plan, yes giving actual remediation instead of everything is a nice idea. and in yes in tool run, it does not make sense to give everything, (although it can say at end, for more information on check, please see doc at ).

Sounds like a good plan to me too. Raising more relevant errors messages seems way more useful. @naveensrinivasan WDYT?

SG.

Shall we also provide additional information about the checks we make in the command line output (when --show-details is provided)? I personally find it useful - even if somehow redundant with the YAML document file. Here's an example output (YES implicitly means it's a positive check outcome):

$ scorecard <> --show-details RESULTS ------- Repo: some-repo Active: Fail Confidence: 100% Commits in last 90 days greater than 2: NO (1) Branch-Protection: Fail Confidence: 60% AllowForcePushes disabled: YES AllowDeletions disabled: YES EnforceAdmins enabled: YES RequireLinearHistory enabled: YES RequiredStatusChecks enabled: YES but has no contexts RequiredPullRequestReviews enabled: YES but with only 1 reviews DismissStaleReviews enabled: YES DismissalRestrictions enabled: NO RequireCodeOwnerReviews enabled: YES

This is just a first attempt and be changed, of course.

On a related note, I'm not sure the term Confidence makes sense in all cases. In some cases it does (say, if we find a commit with no PR we have high confidence it's not branch-protected). In other case, confidence seems to mean 'degree of protection' (Branch-Protection above)

naveensrinivasan self-assigned this Apr 17, 2021

naveensrinivasan requested review from azeemshaikh38 and inferno-chromium April 17, 2021 13:05

naveensrinivasan linked an issue Apr 17, 2021 that may be closed by this pull request

The score card should indicate when checks don't support a particular language/ecosystem #74

Open

naveensrinivasan removed a link to an issue Apr 17, 2021

The score card should indicate when checks don't support a particular language/ecosystem #74

Open

naveensrinivasan linked an issue Apr 17, 2021 that may be closed by this pull request

Feature - Provide Remediation in results #304

Open

11 tasks

inferno-chromium requested a review from oliverchang April 17, 2021 17:43

naveensrinivasan force-pushed the feat/checks-documentation branch from 73e6676 to 14d6dee Compare April 17, 2021 20:09

✨ Included documentation for Active check

9214dd3

* Included the documentation for Active check * Embed the checks.yaml into the binary

naveensrinivasan force-pushed the feat/checks-documentation branch from 14d6dee to 9214dd3 Compare April 17, 2021 20:13

naveensrinivasan linked an issue Apr 17, 2021 that may be closed by this pull request

Integration with CI/CD and GitHub Code Scanning Results #193

Closed

azeemshaikh38 reviewed Apr 18, 2021

View reviewed changes

oliverchang reviewed Apr 19, 2021

View reviewed changes

inferno-chromium mentioned this pull request Apr 20, 2021

Feature: Scorecard output should be human readable HTML #342

Closed

laurentsimon mentioned this pull request May 14, 2021

✨ Add checks for workflow action pinning #451

Closed

2 tasks

azeemshaikh38 mentioned this pull request May 14, 2021

Scorecard should provide details on fuzzing coverage in fuzzing check #78

Open

azeemshaikh38 mentioned this pull request May 26, 2021

Project: Scorecard UX #511

Closed

naveensrinivasan closed this Jul 13, 2021

naveensrinivasan deleted the feat/checks-documentation branch July 13, 2021 17:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Included documentation for Active check #347

✨ Included documentation for Active check #347

naveensrinivasan commented Apr 17, 2021 •

edited

Loading

github-actions bot commented Apr 17, 2021

inferno-chromium commented Apr 17, 2021

naveensrinivasan commented Apr 17, 2021

naveensrinivasan commented Apr 17, 2021

github-actions bot commented Apr 17, 2021

github-actions bot commented Apr 17, 2021

azeemshaikh38 left a comment

azeemshaikh38 Apr 18, 2021

naveensrinivasan Apr 18, 2021

azeemshaikh38 Apr 18, 2021

naveensrinivasan Apr 18, 2021

azeemshaikh38 Apr 18, 2021

azeemshaikh38 Apr 18, 2021

naveensrinivasan Apr 18, 2021

naveensrinivasan commented Apr 18, 2021

oliverchang Apr 19, 2021

naveensrinivasan Apr 19, 2021

oliverchang Apr 19, 2021

naveensrinivasan Apr 19, 2021

azeemshaikh38 Apr 20, 2021 •

edited

Loading

inferno-chromium Apr 20, 2021

oliverchang Apr 21, 2021

laurentsimon Apr 21, 2021 •

edited

Loading

		@@ -0,0 +1,90 @@
		// Copyright 2020 Security Scorecard Authors

✨ Included documentation for Active check #347

✨ Included documentation for Active check #347

Conversation

naveensrinivasan commented Apr 17, 2021 • edited Loading

github-actions bot commented Apr 17, 2021

inferno-chromium commented Apr 17, 2021

naveensrinivasan commented Apr 17, 2021

naveensrinivasan commented Apr 17, 2021

github-actions bot commented Apr 17, 2021

github-actions bot commented Apr 17, 2021

azeemshaikh38 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naveensrinivasan commented Apr 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azeemshaikh38 Apr 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurentsimon Apr 21, 2021 • edited Loading

Choose a reason for hiding this comment

naveensrinivasan commented Apr 17, 2021 •

edited

Loading

azeemshaikh38 Apr 20, 2021 •

edited

Loading

laurentsimon Apr 21, 2021 •

edited

Loading