Discussion: Can probes exist without belonging to a check

[laurentsimon]

The probes (experimental) currently need to belong to a check to be run. What is users need probes that don't fit into a check? Do we create a new check? Do we put all new probes into their own check? In this case, shall we "hide" these checks from the --checks option?

I think this will eventually be a case-by-case discussion. But it would be good to have a story.

I'm currently thinking that if there's not obvious check for a new probe, we could have a fallback "special" check that contains all the probes and not exposed the check within the --checks option. This would also allow us to migrate probes to part of a public check once we think that probe(s) should be "promoted" to a public check (based on probe popularity, etc).

[github-actions]

Stale issue message

[spencerschrock]

This exists to some extent already. For the Binary-Artifact conversion (#3508), we ended up making two probes:

• freeOfAnyBinaryArtifacts
• freeOfUnverifiedBinaryArtifacts

Only freeOfUnverifiedBinaryArtifacts is hooked up, the other is disconnected from the Binary-Artifact check.

A similar situation will happen with some of the optional Pinned-Dependencies enhanced probes:

• Differentiate between commit SHA and tag SHA #2132 (comment)
• Feature: Dangerous Workflows - Imposter Commit Check #2733 (this one may fit under Dangerous-Workflows too?)

I think the bigger question will be how raw data collection will work in these cases, as sometimes they use the same data (like freeOfAnyBinaryArtifacts above), but the two Pinned-Dependencies will need to fetch more data.

[spencerschrock]

Also linking #3840, which has me thinking about this again.

There is a CLI argument, --probes, which will run some probes. This would work for people that just want the data, but wouldn't handle arbitrary scoring decisions. Something like blank-imports/custom checks may be better suited for that (#3095)

How to make it work

The initial implementation of --probes required setting up extra maps, and has fallen out of date. Ideally, this is setup in a way that keeps itself updated.

There's at least two piece of info we need:

• which raw data fields the probes need (right now, assumes 1:1 mapping)
  

  scorecard/probes/entries.go

  Lines 195 to 200 in fb3edd9

  	CheckMap = map[string]string{
  	securityPolicyPresent.Probe: "Security-Policy",
  	securityPolicyContainsLinks.Probe: "Security-Policy",
  	securityPolicyContainsVulnerabilityDisclosure.Probe: "Security-Policy",
  	securityPolicyContainsText.Probe: "Security-Policy",
  	toolRenovateInstalled.Probe: "Dependency-Update-Tool",

• which runner func the probe uses
  

  scorecard/probes/entries.go

  Lines 158 to 164 in fb3edd9

  	probeRunners = map[string]func(*checker.RawResults) ([]finding.Finding, string, error){
  	securityPolicyPresent.Probe: securityPolicyPresent.Run,
  	securityPolicyContainsLinks.Probe: securityPolicyContainsLinks.Run,
  	securityPolicyContainsVulnerabilityDisclosure.Probe: securityPolicyContainsVulnerabilityDisclosure.Run,
  	securityPolicyContainsText.Probe: securityPolicyContainsText.Run,
  	toolRenovateInstalled.Probe: toolRenovateInstalled.Run,
  	toolDependabotInstalled.Probe: toolDependabotInstalled.Run,

We can always use the same strategy from checks, which is an init() block which calls registerCheck:

scorecard/checks/all_checks.go

Lines 57 to 58 in fb3edd9

	func registerCheck(name string, fn checker.CheckFn, supportedRequestTypes []checker.RequestType) error {
	if name == "" {

If we had a registerProbe func, we could presumably have all this data registration taken care of by the probes in the various impl.go files:

func init() {
	// may not be able to use constants from checks package due to circular imports
	requiredRawData := []string{
		"Maintained", 
		"SAST",
	}
	supportedRequestTypes := []probes.RequestType{
		probes.CommitBased,
	}
	if err := registerProbe(Probe /* name */ , Run /* func */ , requiredRawData, supportedRequestTypes); err != nil {
		// this should never happen
		panic(err)
	}
}

This would also let us register supported request types (which relates to #3718).

[spencerschrock]

So I played around with this a little bit in a branch: https://github.com/spencerschrock/scorecard/tree/register-probes

the init approach would still require blank imports somewhere, as the probe packages which aren't hooked up to the evaluation code aren't loaded.

scorecard/probes/entries.go

Lines 24 to 25 in c940d9b

	"github.com/ossf/scorecard/v4/probes/contributorsFromOrgOrCompany"
	_ "github.com/ossf/scorecard/v4/probes/freeOfAnyBinaryArtifacts"

[spencerschrock]

Partially completed via #3876, but #3718 may prevent this one from being closed.