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

✨ Add policy file #1002

Merged
merged 14 commits into from
Sep 22, 2021
Merged

✨ Add policy file #1002

merged 14 commits into from
Sep 22, 2021

Conversation

laurentsimon
Copy link
Contributor

  • Please check if the PR fulfills these requirements
  • 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)
    no policy support

  • What is the new behavior (if this is a feature change)?
    policy file loaded - no effect yet

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
    no

  • Other information:

@laurentsimon laurentsimon marked this pull request as draft September 11, 2021 01:07
@naveensrinivasan
Copy link
Member

Sorry, I am still not clear with this feature. What does the policy file do?

@laurentsimon
Copy link
Contributor Author

Sorry, I am still not clear with this feature. What does the policy file do?

PR still in Draft mode. TL:DR: the policy file will allow users to specify a minimum score for each check in order to customize the results and get a Pass/Fail per check.

The first use case is for SARIF in GitHub action: to allow users to define a minimum score; and be alerted if it's violated. Over time, we expect users to increase the minimum score to improve their security posture.

@naveensrinivasan
Copy link
Member

Sorry, I am still not clear with this feature. What does the policy file do?

PR still in Draft mode. TL:DR: the policy file will allow users to specify a minimum score for each check in order to customize the results and get a Pass/Fail per check.

The first use case is for SARIF in GitHub action: to allow users to define a minimum score; and be alerted if it's violated. Over time, we expect users to increase the minimum score to improve their security posture.

Thanks

//nolint:govet
type CheckPolicy struct {
Score int `yaml:"score"`
Mode string `yaml:"mode"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add a Risk to this structure later. I will use the default doc risk or the policy risk if a policy is provided.
Main blocker here is doc link: to accommodate for dynamic risk, we need a proper doc web page and pass the risk as GET params. I'll add this to the doc soon.

policy/policy.go Outdated Show resolved Hide resolved
policy/policy.go Outdated Show resolved Hide resolved
policy/policy.go Outdated Show resolved Hide resolved
policy/policy.go Outdated Show resolved Hide resolved
policy/policy.go Outdated Show resolved Hide resolved
policy/policy.go Outdated Show resolved Hide resolved
policy/policy.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
@laurentsimon
Copy link
Contributor Author

@azeemsgoogle I've addressed the comments. Can you take a second look?

@laurentsimon laurentsimon force-pushed the feat/enforce branch 2 times, most recently from d01cf2d to 25af561 Compare September 22, 2021 00:32
cmd/root.go Outdated Show resolved Hide resolved
)

func readPolicy() *spol.ScorecardPolicy {
if policyFile != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for checking if the file is empty. Its already done on caller side.

cmd/root.go Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
ENFORCED = 1;
}

Mode mode = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: a bool named enforced will do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what if we add more modes later? I think a bool is compatible with an int which is compatible with enum, so bool -> enum conversion would work if we ever need to add other modes. So I think it's safe to use bool. Agreed? It does not hurt to keep enum, though.

Copy link
Contributor

@azeemshaikh38 azeemshaikh38 left a comment

Choose a reason for hiding this comment

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

LGTM with comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants