-
Notifications
You must be signed in to change notification settings - Fork 485
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
✨ Give low importance to github-owned actions #906
Conversation
checks/pinned_dependencies.go
Outdated
for jobName, job := range workflow.Jobs { | ||
if len(job.Name) > 0 { | ||
jobName = job.Name | ||
} | ||
for _, step := range job.Steps { | ||
if len(step.Uses) > 0 { | ||
// check whether we have github related action |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: start comments with capital letter and ends with full dot.
checks/pinned_dependencies.go
Outdated
for jobName, job := range workflow.Jobs { | ||
if len(job.Name) > 0 { | ||
jobName = job.Name | ||
} | ||
for _, step := range job.Steps { | ||
if len(step.Uses) > 0 { | ||
// check whether we have github related action | ||
// either action/ or github/ | ||
ghaction := regexp.MustCompile(`(actions/)`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need a regex or can we simplify by using strings.HasPrefix()
?
checks/pinned_dependencies.go
Outdated
for jobName, job := range workflow.Jobs { | ||
if len(job.Name) > 0 { | ||
jobName = job.Name | ||
} | ||
for _, step := range job.Steps { | ||
if len(step.Uses) > 0 { | ||
// check whether we have github related action | ||
// either action/ or github/ | ||
ghaction := regexp.MustCompile(`(actions/)`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create a dedicated function, maybe isGitHubOwnedAction()
@@ -534,7 +547,7 @@ func validateGitHubActionWorkflow(pathfn string, content []byte, | |||
} | |||
} | |||
|
|||
addPinnedResult(pdata, ret) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the dependency check is the most complicated to work with. I think what you need is the following:
- create new structure to store workflow pinned results, something like
type worklowPinningResult struct {
thirdParties pinnedResult
gitHubOwned pinnedResult
}
- Create a function
addPinnedWorkflowResult()
to use for this case (here) - You'll need a
dataAsWorkflowResultPointer()
to replacedataAsResultPointer()
for this use case (here) - Update
createReturnForIsGitHubActionsWorkflowPinned
to calculate the score as we discussed earlier.
Let me know if this clarifies things.
Thanks again for taking the time to contribute!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @laurentsimon for the comments and pointer on what to do.
I've updated the PR based from my understanding after reading your comments. Kindly give me your feedback 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
once you've updated your code, add some unit tests to verify your code does what we expect it to do.
checks/pinned_dependencies.go
Outdated
@@ -52,6 +51,13 @@ type gitHubActionWorkflowConfig struct { | |||
Name string `yaml:"name"` | |||
} | |||
|
|||
// Structure to host information about pinned github | |||
// or third party dependencies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: end comment with full stop.
checks/pinned_dependencies.go
Outdated
@@ -153,6 +159,16 @@ func dataAsResultPointer(data FileCbData) *pinnedResult { | |||
return pdata | |||
} | |||
|
|||
func dataAsWorkflowPointer(data FileCbData) *worklowPinningResult { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitL rename to dataAsWorkflowResultPointer
checks/pinned_dependencies.go
Outdated
func createReturnForIsGitHubActionsWorkflowPinned(r pinnedResult, dl checker.DetailLogger, err error) (int, error) { | ||
return createReturnValues(r, | ||
func createReturnForIsGitHubActionsWorkflowPinned(r worklowPinningResult, dl checker.DetailLogger, err error) (int, error) { | ||
return createReturnValues(r.gitHubOwned, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need a new function to return the values. The function needs to look at both gitHubOwned
and thirdParties
to calculate the score.
checks/pinned_dependencies.go
Outdated
|
||
if !CheckFileContainsCommands(content, "#") { | ||
addPinnedResult(pdata, true) | ||
addPinnedResult(&pdata.gitHubOwned, true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need a new function addWorkflowPinnedResult
that takes * worklowPinningResult
as input. You can access and set gitHubOwned
and thirdParties
accordingly inside that function.
checks/pinned_dependencies.go
Outdated
mgithub := ghgithub.Match([]byte(step.Uses)) | ||
|
||
|
||
if (!maction) && (!mgithub) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is where you should call addWorkflowPinnedResult
to set gitHubOwned
and/or thirdParties
Thank you @laurentsimon I've updated the code based on your feedback but I'm still uncertain about the following:
does this mean that the
does this mean that the
Apology for going back and forth as I'm still trying to nail the logic down and understand it properly. Once I get the logic correct will add more test cases and fix any broken fix test cases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're in the right direction. Add the unit tests and I'll review everything.
Note: I'm OOO from 01-Sept to 07-Sept so expect some delays for me to LGTM the PR.
Sorry about that!
checks/pinned_dependencies.go
Outdated
return checker.InconclusiveResultScore, err | ||
} | ||
|
||
score := 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initialize to checker.MinResultScore
instead of 0
checks/pinned_dependencies.go
Outdated
switch r.gitHubOwned { | ||
case pinned: | ||
score += 2 | ||
case notPinned: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this case not needed since checker.MinResultScore
is 0. I think we can simplify the switch
and use a single if
as:
if r.gitHubOwned != notPinned {
score += 2
}
his will also take care of the case when r.gitHubOwned
is pinnedUndefined
which means we found no GitHub-owned actions in the workflow.
checks/pinned_dependencies.go
Outdated
score += checker.MinResultScore | ||
} | ||
|
||
switch r.thirdParties { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above.
checks/pinned_dependencies.go
Outdated
maction := strings.HasPrefix(step.Uses, "actions/") | ||
mgithub := strings.HasPrefix(step.Uses, "github/") | ||
|
||
if (match) && ((maction) || (mgithub)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can simplify addWorkflowPinnedResult
by adding 1 additionalbool
: isGitHub
. The function can then internally call addPinnedResult()
. Something like:
githubOwned := isGitHubOwnedAction(step.Uses)
addWorkflowPinnedResult(pdata, match, githubOwned)
func addWorkflowPinnedResult(r * worklowPinningResult, to, isGitHub bool) {
if isGitHub {
addPinnedResult(&r.gitHubOwned, to)
} else {
addPinnedResult(&r.thirdParties, to)
}
checks/pinned_dependencies.go
Outdated
return true, nil | ||
} | ||
|
||
func addWorkflowPinnedResult(w *pinnedResult, to bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment above to simplify
35fe3bc
to
552949c
Compare
@laurentsimon PR has been modified per your suggestion and test has been added. Thank you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the hard work, this is great!
I think the next PR will be perfect and we can merge. I'm OOO until 07 Sept so expect some delay for my last LGTM. Sorry about that.
checks/pinned_dependencies.go
Outdated
score += 8 | ||
} | ||
|
||
if r.gitHubOwned == pinned { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move the Info()
inside the if r.gitHubOwned != notPinned
.
Also add a call to Info()
inside the if r.thirdParties != notPinned
.
Use a different message for the call to Call()
, and use Info3()
as in https://github.com/ossf/scorecard/blob/main/checks/permissions.go#L66. Say
Info3(&checker.LogMessage{
Path: PATH2FILE, Type: checker.FileTypeSource, Offset: LINE_NO, Snippet: SNIPPET,
Text: fmt.Sprintf("%s %s"), "GitHub-owned", infoMsg,
})
Info3(&checker.LogMessage{
Path: PATH2FILE, Type: checker.FileTypeSource, Offset: LINE_NO, Snippet: SNIPPET,
Text: fmt.Sprintf("%s %s"), "Third-party", infoMsg,
})
If you're not sure how to get the line numbers and snippet, omit them and just add a //TODO: set Snippet and line numbers.
Last point: call createReturnValuesForGitHubActionsWorkflowPinned()
with a generic message "actions are pinned"
checks/pinned_dependencies.go
Outdated
return createReturnValues(r, | ||
func createReturnForIsGitHubActionsWorkflowPinned(r worklowPinningResult, dl checker.DetailLogger, | ||
err error) (int, error) { | ||
return createReturnValuesForGitHubActionsWorkflowPinned(r, | ||
"GitHub actions are pinned", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update this message as I said in comment above.
@@ -0,0 +1,48 @@ | |||
# Copyright 2021 Security Scorecard Authors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this file be renamed to workflow-github-pinned.yaml
since it only contains pinned GitHub-owned actions?
@@ -0,0 +1,48 @@ | |||
# Copyright 2021 Security Scorecard Authors | |||
# | |||
# Licensed under the Apache License, Version 2.0 (the "License"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add additional unit tests:
- mix of pinned and non-pinned GitHub actions. score should be 8
- mixed of pinned and non-pinned non-GitHub dependencies. score should be 2
- mixed of the above. score should be 0
- all github and 3p actions pinned. score should be 10. For this one, you can simply update an existing workflow instead of creating a new file. Or you can create a new file: up to you.
@inferno-chromium please LGTM and merge this PR once the changes above have been made. Otherwise it will have to wait until I'm back from OOO on 08 Sept. |
63b9117
to
d007f73
Compare
@inferno-chromium PR has been updated based on @laurentsimon feedback. Thanks. The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I Left a single comment in the unit test. Just address it and update your branch.
I'll check what''s wrong with the protobuf issue.
the protobuf problem in the check may be due to your branch being out-of-date. The problem was solved recently and hopefully will go away when you update your fork. |
* Different calculation between github and non-github actions * Add test case for different kind of github and non-github action * Modify existing test as score calculation has changed
#802
feature update
close Give low importance to github-owned actions #802