✨ Improved Security Policy Check

[shissam]

What kind of change does this PR introduce?

Improvement on the Security Policy check which goes beyond the simply existence of a recognized Security Policy filename, and examines the contents of the found Security Policy filename seeking hits on key elements of a community security policy (linked content, emails, time frames and specifics about disclosures or vulnerability reporting)

• PR title follows the guidelines defined in our pull request documentation

What is the current behavior?

Security Policy will score either 0 (zero) if no file is found in the repo or the org's repo, or will award a maximum score of 10 (ten) simply on the existence of the file and a length greater than 0 (zero) - regardless if there is anything in it.

What is the new behavior (if this is a feature change)?**

If a Security Policy file does not exist or is less that 1 byte in length, the behavior of scorecard is unchanged. Otherwise:

#1: found one linked (email/http) content: score += 3
    rationale: someone to collaborate with or link to
    information (strong for community)

#2: more than one unique (email/http) linked content found: score += 3
    rationale: if more than one link, even stronger for the community

#3: more bytes than the sum of the length of all the linked content found: score += 3
    rationale: there appears to be information and context around those links
    no credit if there is just a link to a site or an email address (those given above)

#4: found whole number(s) and or match(es) to "Disclos" and or "Vuln": score += 1
    rationale: works towards the intent of the security policy file
    regarding whom to contact about vuls and disclosures and timing
    e.g., we'll disclose, report a vulnerabily, 30 days, etc.
    looking for at least 2 hits

• Tests for the changes have been added (for bug fixes/features)

Which issue(s) this PR fixes

Fixes #2137

Special notes for your reviewer

Does this PR introduce a user-facing change?

The fact that Security-Policy scores will now be gradiated (and not just 0 or 10) is user facing, in this PR, there is an update to checks.md proposed.

For user-facing changes, please add a concise, human-readable release note to
the release-note

(In particular, describe what changes users might need to make in their
application as a result of this pull request.)

This check examines the contents of the security policy file awarding points for those policies that express vulnerability process(es), disclosure timelines, and have links (e.g., URL(s) and email(s) to support the users.

[olivekl]

Thanks for this! The documentation in checks.md is autogenerated. Please don't manually update checks.md. Instead, make the changes in checks.yaml and run make generate-docs.

[shissam]

Thanks for this! The documentation in checks.md is autogenerated. Please don't manually update checks.md. Instead, make the changes in checks.yaml and run make generate-docs.

hi, thanks, I see that now, sorry I missed that. I have also seen that make unit-test (locally) fails just like the integration test in the workflow in this PR. In the CONTRIBUTING.md for scorecard, there is a comment on make e2e (which for me (locally) reports make: Nothing to be done for 'e2e'. There was nothing in that guide about make unit-test.

So, I have more work to do

• fix the checks.yaml and do make generate-docs
• update the unit tests for Security-Policy to support GetFileContents and revise the test to reflect new potential scores in the mock site (i.e., pass make unit-test locally).

given this "depth" of additional work I need to complete, how should this PR proceed? Should I delete the PR and re-fork and re-PR after I get those two things fixed? I am not sure what is the best way to proceed. If this PR is kept, how to I submit those changes (like checks.yaml) into this PR? (I am a n00b to this overall process.

[github-actions]

Integration tests success for
[dcdee96]
(https://github.com/ossf/scorecard/actions/runs/2922344086)

[codecov]

Codecov Report

Merging #2195 (2668737) into main (d67fcba) will decrease coverage by 0.31%.
The diff coverage is 30.76%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2195      +/-   ##
==========================================
- Coverage   40.77%   40.46%   -0.32%     
==========================================
  Files         113      113              
  Lines        8919     9117     +198     
==========================================
+ Hits         3637     3689      +52     
- Misses       5020     5152     +132     
- Partials      262      276      +14     

[laurentsimon]

@shissam feel free to include the update of checks.yaml / checks.md + unit tests in this PR. That's what we usally do. Thanks again for your contribution

[laurentsimon]

Thanks for the PR. I've suggested some changes:

1. Rename the structure in

   scorecard/checker/raw_result.go

   Lines 141 to 144 in 32d6ba2

   	type SecurityPolicyData struct {
   	// Files contains a list of files.
   	Files []File
   	}

2. Gather all the information needed
3. Parse that data, add calls to Warn/Info and compute the score as you do right now.

Let me know if this does not make sense. I understand there's a lot to unpack.

[github-actions]

Integration tests success for
[a3fa5de]
(https://github.com/ossf/scorecard/actions/runs/2940927701)

[github-actions]

Integration tests success for
[1a8efab]
(https://github.com/ossf/scorecard/actions/runs/2958686435)

[laurentsimon]

Thanks. I've added comments. Let me know if they make sense or not.
Sorry for the back and forth. You're basically working on the heart of raw results, and they are not well polished. We can hop on a call if you want.

Thanks

[shissam]

Hi-there is definitely more dependencies here than I was aware. I can make these changes as you indicated and revise some of the resolved comments. Telemetry on the hits for emails, URLs etc is easy enough, I’ll just have to treat the contents line by line (for line numbers and offsets) As I mentioned on slack, I’ll be out a few weeks and will work these comments when I return. Thanks for your patience and insights

…

On Sep 1, 2022, at 1:56 PM, laurentsimon ***@***.***> wrote:  @laurentsimon commented on this pull request. Thanks. I've added comments. Let me know if they make sense or not. Sorry for the back and forth. You're basically working on the heart of raw results, and they are not well polished. We can hop on a call if you want. Thanks In checker/raw_result.go: > @@ -136,11 +136,28 @@ type VulnerabilitiesData struct { Vulnerabilities []clients.Vulnerability } +type SecurityPolicyInformationType string + +const ( + // forms of security policy hints being evaluated. + SecurityPolicyInformationTypeEmail SecurityPolicyInformationType = "emailAddress" + SecurityPolicyInformationTypeLink SecurityPolicyInformationType = "httpLink" + SecurityPolicyInformationTypeText SecurityPolicyInformationType = "vuln & disclosure text" keep this camelcase, without spaces. This will ultimately be used in a field "name". In checker/raw_result.go: > @@ -136,11 +136,28 @@ type VulnerabilitiesData struct { Vulnerabilities []clients.Vulnerability } +type SecurityPolicyInformationType string + +const ( + // forms of security policy hints being evaluated. + SecurityPolicyInformationTypeEmail SecurityPolicyInformationType = "emailAddress" + SecurityPolicyInformationTypeLink SecurityPolicyInformationType = "httpLink" + SecurityPolicyInformationTypeText SecurityPolicyInformationType = "vuln & disclosure text" +) + +type SecurityPolicyInformation struct { + InformationType SecurityPolicyInformationType + InformationValue []string how will you keep track of lines each of the InformationValue were found? In checks/evaluation/security_policy.go: > @@ -19,6 +19,81 @@ import ( sce "github.com/ossf/scorecard/v4/errors" ) +func scoreSecurityCriteria(f checker.File, info []checker.SecurityPolicyInformation, dl checker.DetailLogger) int { + var urls, emails, discvuls, linkedContentLen, score int + // for Security-Policy, EndOffset denotes the EndOffset is typically used to identify the end of the relevant file where a Snippet value was found. In your case, Snippet would be an email. address, for example. In checks/raw/security_policy.go: > @@ -29,23 +30,38 @@ import ( ) type securityPolicyFilesWithURI struct { - uri string - files []checker.File + info []checker.SecurityPolicyInformation + uri string + file checker.File You may have to make fields fields public, so that we can access them from pkg/ folder. In pkg/json_raw_results.go: > @@ -609,11 +609,9 @@ func (r *jsonScorecardRawResult) addBinaryArtifactRawResults(ba *checker.BinaryA //nolint:unparam func (r *jsonScorecardRawResult) addSecurityPolicyRawResults(sp *checker.SecurityPolicyData) error { r.Results.SecurityPolicies = []jsonFile{} - for _, v := range sp.Files { - r.Results.SecurityPolicies = append(r.Results.SecurityPolicies, jsonFile{ - Path: v.Path, - }) - } + r.Results.SecurityPolicies = append(r.Results.SecurityPolicies, jsonFile{ Ideally information about SecurityPolicyInformationTypeEmail, etc should be reported here. This way someone who fetches results from the REST API (once the raw results are exposed) will be able to parse and create their own policies based on what kind of information was found. One suggestion it to create a jsonSecurityFile: type jsonSecurityFile struct{ jsonFile Type string --> this will take a string like SecurityPolicyInformationTypeEmail, etc } Later we can "fix" the jsonFile structure to contain a list of Snippet/lines. @raghavkaul fyi — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

[github-actions]

Stale pull request message

[shissam]

I’ll return to this PR when I return from leave

…

On Sep 11, 2022, at 7:15 PM, github-actions[bot] ***@***.***> wrote:  Stale pull request message — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

[github-actions]

Stale pull request message

[spencerschrock]

understood - I think - with 2 Failed after reporting FAIL! - Interrupted by User I see other errors around lines 3771 where it reports Expect(err).Should(BeNil()) for fuzzing_test.go:38 are these also because of the interrupted API?

Yep. The interrupted by user is from the 30m timeout stoping the other tests.

Assuming so, then, how does the job/integration test get restarted or triggered to rerun?

Basically any change to your branch, a new commit, merge, rebase etc. Or a maintainer can manually re-run jobs.

[github-actions]

Integration tests success for
[e0739b4]
(https://github.com/ossf/scorecard/actions/runs/3380088683)

[shissam]

understood - I think - with 2 Failed after reporting FAIL! - Interrupted by User I see other errors around lines 3771 where it reports Expect(err).Should(BeNil()) for fuzzing_test.go:38 are these also because of the interrupted API?

Yep. The interrupted by user is from the 30m timeout stoping the other tests.

Assuming so, then, how does the job/integration test get restarted or triggered to rerun?

Basically any change to your branch, a new commit, merge, rebase etc. Or a maintainer can manually re-run jobs.

changes for the workflow were successful - thanks!

[github-actions]

Integration tests success for
[c46a581]
(https://github.com/ossf/scorecard/actions/runs/3390840177)

[github-actions]

Integration tests success for
[2668737]
(https://github.com/ossf/scorecard/actions/runs/3396904408)

[spencerschrock]

Thanks @shissam !