Skip to content

Commit 005fba8

Browse files
varunsh-codervarunsh-coder
authored
Merge pull request #2524 from step-security/add-empty-toplevel-permissions
Add addEmptyTopLevelPermissions parameter for workflow permissions
2 parents 7b9f651 + b072f04 commit 005fba8

File tree

10 files changed

+293
-12
lines changed

10 files changed

+293
-12
lines changed

‎remediation/workflow/permissions/permissions.go

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ func githubTokenInJobLevelEnv(job metadata.Job) bool {
8989
return false
9090
}
9191

92-
func AddWorkflowLevelPermissions(inputYaml string, addProjectComment bool) (string, error) {
92+
func AddWorkflowLevelPermissions(inputYaml string, addProjectComment bool, addEmptyTopLevelPermissions bool) (string, error) {
9393
workflow := metadata.Workflow{}
9494

9595
err := yaml.Unmarshal([]byte(inputYaml), &workflow)
@@ -138,13 +138,20 @@ func AddWorkflowLevelPermissions(inputYaml string, addProjectComment bool) (stri
138138
spaces += " "
139139
}
140140

141-
if addProjectComment {
142-
output = append(output, spaces+"permissions: # added using https://github.com/step-security/secure-repo")
141+
if addEmptyTopLevelPermissions {
142+
if addProjectComment {
143+
output = append(output, spaces+"permissions: {} # added using https://github.com/step-security/secure-repo")
144+
} else {
145+
output = append(output, spaces+"permissions: {}")
146+
}
143147
} else {
144-
output = append(output, spaces+"permissions:")
148+
if addProjectComment {
149+
output = append(output, spaces+"permissions: # added using https://github.com/step-security/secure-repo")
150+
} else {
151+
output = append(output, spaces+"permissions:")
152+
}
153+
output = append(output, spaces+" contents: read")
145154
}
146-
147-
output = append(output, spaces+" contents: read")
148155
output = append(output, "")
149156

150157
for i := line - 1; i < len(inputLines); i++ {
@@ -154,7 +161,7 @@ func AddWorkflowLevelPermissions(inputYaml string, addProjectComment bool) (stri
154161
return strings.Join(output, "\n"), nil
155162
}
156163

157-
func AddJobLevelPermissions(inputYaml string) (*SecureWorkflowReponse, error) {
164+
func AddJobLevelPermissions(inputYaml string, addEmptyTopLevelPermissions bool) (*SecureWorkflowReponse, error) {
158165

159166
workflow := metadata.Workflow{}
160167
errors := make(map[string][]string)
@@ -216,7 +223,7 @@ func AddJobLevelPermissions(inputYaml string) (*SecureWorkflowReponse, error) {
216223
if strings.Compare(inputYaml, fixWorkflowPermsReponse.FinalOutput) != 0 {
217224
fixWorkflowPermsReponse.IsChanged = true
218225

219-
if len(perms) == 1 && strings.Contains(perms[0], contents_read) {
226+
if len(perms) == 1 && strings.Contains(perms[0], contents_read) && !addEmptyTopLevelPermissions {
220227
// Don't add contents: read, because it will get defined at workflow level
221228
continue
222229
} else {

‎remediation/workflow/permissions/permissions_test.go

Lines changed: 90 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,11 @@ func TestAddJobLevelPermissions(t *testing.T) {
1818
}
1919

2020
for _, f := range files {
21+
22+
if f.Name() == "empty-top-level-permissions.yml" {
23+
continue
24+
}
25+
2126
input, err := ioutil.ReadFile(path.Join(inputDirectory, f.Name()))
2227

2328
if err != nil {
@@ -26,7 +31,7 @@ func TestAddJobLevelPermissions(t *testing.T) {
2631

2732
os.Setenv("KBFolder", "../../../knowledge-base/actions")
2833

29-
fixWorkflowPermsResponse, err := AddJobLevelPermissions(string(input))
34+
fixWorkflowPermsResponse, err := AddJobLevelPermissions(string(input), false)
3035
output := fixWorkflowPermsResponse.FinalOutput
3136
jobErrors := fixWorkflowPermsResponse.JobErrors
3237

@@ -68,6 +73,47 @@ func TestAddJobLevelPermissions(t *testing.T) {
6873
}
6974
}
7075

76+
func TestAddJobLevelPermissionsWithEmptyTopLevel(t *testing.T) {
77+
const inputDirectory = "../../../testfiles/joblevelpermskb/input"
78+
const outputDirectory = "../../../testfiles/joblevelpermskb/output"
79+
80+
// Test the empty-top-level-permissions.yml file
81+
input, err := ioutil.ReadFile(path.Join(inputDirectory, "empty-top-level-permissions.yml"))
82+
if err != nil {
83+
t.Fatal(err)
84+
}
85+
86+
expectedOutput, err := ioutil.ReadFile(path.Join(outputDirectory, "empty-top-level-permissions.yml"))
87+
if err != nil {
88+
t.Fatal(err)
89+
}
90+
91+
os.Setenv("KBFolder", "../../../knowledge-base/actions")
92+
93+
// Test with addEmptyTopLevelPermissions = true
94+
fixWorkflowPermsResponse, err := AddJobLevelPermissions(string(input), true)
95+
if err != nil {
96+
t.Errorf("Unexpected error with addEmptyTopLevelPermissions=true: %v", err)
97+
}
98+
99+
if fixWorkflowPermsResponse.FinalOutput != string(expectedOutput) {
100+
t.Errorf("test failed with addEmptyTopLevelPermissions=true for empty-top-level-permissions.yml\nExpected:\n%s\n\nGot:\n%s",
101+
string(expectedOutput), fixWorkflowPermsResponse.FinalOutput)
102+
}
103+
104+
// Test with addEmptyTopLevelPermissions = false (should skip contents: read)
105+
fixWorkflowPermsResponse2, err2 := AddJobLevelPermissions(string(input), false)
106+
if err2 != nil {
107+
t.Errorf("Unexpected error with addEmptyTopLevelPermissions=false: %v", err2)
108+
}
109+
110+
// With false, contents: read should be skipped at job level
111+
if fixWorkflowPermsResponse2.FinalOutput != string(input) {
112+
t.Errorf("test failed with addEmptyTopLevelPermissions=false for empty-top-level-permissions.yml\nExpected:\n%s\n\nGot:\n%s",
113+
string(input), fixWorkflowPermsResponse2.FinalOutput)
114+
}
115+
}
116+
71117
func Test_addPermissions(t *testing.T) {
72118
type args struct {
73119
inputYaml string
@@ -112,6 +158,10 @@ func TestAddWorkflowLevelPermissions(t *testing.T) {
112158
continue
113159
}
114160

161+
if f.Name() == "empty-permissions.yml" {
162+
continue
163+
}
164+
115165
input, err := ioutil.ReadFile(path.Join(inputDirectory, f.Name()))
116166

117167
if err != nil {
@@ -125,7 +175,7 @@ func TestAddWorkflowLevelPermissions(t *testing.T) {
125175
addProjectComment = true
126176
}
127177

128-
output, err := AddWorkflowLevelPermissions(string(input), addProjectComment)
178+
output, err := AddWorkflowLevelPermissions(string(input), addProjectComment, false)
129179

130180
if err != nil {
131181
t.Errorf("Error not expected")
@@ -143,3 +193,41 @@ func TestAddWorkflowLevelPermissions(t *testing.T) {
143193
}
144194

145195
}
196+
197+
func TestAddWorkflowLevelPermissionsWithEmpty(t *testing.T) {
198+
const inputDirectory = "../../../testfiles/toplevelperms/input"
199+
const outputDirectory = "../../../testfiles/toplevelperms/output"
200+
201+
// Test the empty-permissions.yml file
202+
input, err := ioutil.ReadFile(path.Join(inputDirectory, "empty-permissions.yml"))
203+
if err != nil {
204+
t.Fatal(err)
205+
}
206+
207+
expectedOutput, err := ioutil.ReadFile(path.Join(outputDirectory, "empty-permissions.yml"))
208+
if err != nil {
209+
t.Fatal(err)
210+
}
211+
212+
// Test with addEmptyTopLevelPermissions = true
213+
output, err := AddWorkflowLevelPermissions(string(input), false, true)
214+
if err != nil {
215+
t.Errorf("Unexpected error with addEmptyTopLevelPermissions=true: %v", err)
216+
}
217+
218+
if output != string(expectedOutput) {
219+
t.Errorf("test failed with addEmptyTopLevelPermissions=true for empty-permissions.yml\nExpected:\n%s\n\nGot:\n%s",
220+
string(expectedOutput), output)
221+
}
222+
223+
// Test with addEmptyTopLevelPermissions = false (should add contents: read)
224+
output2, err2 := AddWorkflowLevelPermissions(string(input), false, false)
225+
if err2 != nil {
226+
t.Errorf("Unexpected error with addEmptyTopLevelPermissions=false: %v", err2)
227+
}
228+
229+
// With false, should add contents: read instead of empty permissions
230+
if !strings.Contains(output2, "contents: read") || strings.Contains(output2, "permissions: {}") {
231+
t.Errorf("test failed with addEmptyTopLevelPermissions=false for empty-permissions.yml - should contain 'contents: read' but not 'permissions: {}'\nGot:\n%s", output2)
232+
}
233+
}

‎remediation/workflow/secureworkflow.go

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ func SecureWorkflow(queryStringParams map[string]string, inputYaml string, svc d
2222
pinnedActions, addedHardenRunner, addedPermissions, replacedMaintainedActions := false, false, false, false
2323
ignoreMissingKBs := false
2424
enableLogging := false
25+
addEmptyTopLevelPermissions := false
2526
exemptedActions, pinToImmutable, maintainedActionsMap := []string{}, false, map[string]string{}
2627

2728
if len(params) > 0 {
@@ -68,6 +69,10 @@ func SecureWorkflow(queryStringParams map[string]string, inputYaml string, svc d
6869
enableLogging = true
6970
}
7071

72+
if queryStringParams["addEmptyTopLevelPermissions"] == "true" {
73+
addEmptyTopLevelPermissions = true
74+
}
75+
7176
if enableLogging {
7277
// Log query parameters
7378
paramsJSON, _ := json.MarshalIndent(queryStringParams, "", " ")
@@ -83,7 +88,7 @@ func SecureWorkflow(queryStringParams map[string]string, inputYaml string, svc d
8388
if enableLogging {
8489
log.Printf("Adding job level permissions")
8590
}
86-
secureWorkflowReponse, err = permissions.AddJobLevelPermissions(secureWorkflowReponse.FinalOutput)
91+
secureWorkflowReponse, err = permissions.AddJobLevelPermissions(secureWorkflowReponse.FinalOutput, addEmptyTopLevelPermissions)
8792
secureWorkflowReponse.OriginalInput = inputYaml
8893
if err != nil {
8994
if enableLogging {
@@ -95,7 +100,7 @@ func SecureWorkflow(queryStringParams map[string]string, inputYaml string, svc d
95100
if enableLogging {
96101
log.Printf("Adding workflow level permissions")
97102
}
98-
secureWorkflowReponse.FinalOutput, err = permissions.AddWorkflowLevelPermissions(secureWorkflowReponse.FinalOutput, addProjectComment)
103+
secureWorkflowReponse.FinalOutput, err = permissions.AddWorkflowLevelPermissions(secureWorkflowReponse.FinalOutput, addProjectComment, addEmptyTopLevelPermissions)
99104
if err != nil {
100105
if enableLogging {
101106
log.Printf("Error adding workflow level permissions: %v", err)

‎remediation/workflow/secureworkflow_test.go

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -277,3 +277,103 @@ func TestSecureWorkflow(t *testing.T) {
277277

278278
}
279279
}
280+
281+
func TestSecureWorkflowEmptyPermissions(t *testing.T) {
282+
const inputDirectory = "../../testfiles/secureworkflow/input"
283+
const outputDirectory = "../../testfiles/secureworkflow/output"
284+
285+
httpmock.Activate()
286+
defer httpmock.DeactivateAndReset()
287+
288+
// Mock APIs for actions/checkout
289+
httpmock.RegisterResponder("GET", "https://api.github.com/repos/actions/checkout/commits/v2",
290+
httpmock.NewStringResponder(200, `ee0669bd1cc54295c223e0bb666b733df41de1c5`))
291+
292+
httpmock.RegisterResponder("GET", "https://api.github.com/repos/actions/checkout/git/matching-refs/tags/v2.",
293+
httpmock.NewStringResponder(200,
294+
`[
295+
{
296+
"ref": "refs/tags/v2.7.0",
297+
"object": {
298+
"sha": "ee0669bd1cc54295c223e0bb666b733df41de1c5",
299+
"type": "commit"
300+
}
301+
}
302+
]`),
303+
)
304+
305+
// Mock APIs for actions/setup-node
306+
httpmock.RegisterResponder("GET", "https://api.github.com/repos/actions/setup-node/commits/v1",
307+
httpmock.NewStringResponder(200, `f1f314fca9dfce2769ece7d933488f076716723e`))
308+
309+
httpmock.RegisterResponder("GET", "https://api.github.com/repos/actions/setup-node/git/matching-refs/tags/v1.",
310+
httpmock.NewStringResponder(200,
311+
`[
312+
{
313+
"ref": "refs/tags/v1.4.6",
314+
"object": {
315+
"sha": "f1f314fca9dfce2769ece7d933488f076716723e",
316+
"type": "commit"
317+
}
318+
}
319+
]`),
320+
)
321+
322+
// Mock APIs for step-security/harden-runner
323+
httpmock.RegisterResponder("GET", "https://api.github.com/repos/step-security/harden-runner/commits/v2",
324+
httpmock.NewStringResponder(200, `17d0e2bd7d51742c71671bd19fa12bdc9d40a3d6`))
325+
326+
httpmock.RegisterResponder("GET", "https://api.github.com/repos/step-security/harden-runner/git/matching-refs/tags/v2.",
327+
httpmock.NewStringResponder(200,
328+
`[
329+
{
330+
"ref": "refs/tags/v2.8.1",
331+
"object": {
332+
"sha": "17d0e2bd7d51742c71671bd19fa12bdc9d40a3d6",
333+
"type": "commit"
334+
}
335+
}
336+
]`),
337+
)
338+
339+
var err error
340+
var input []byte
341+
input, err = ioutil.ReadFile(path.Join(inputDirectory, "empty-permissions.yml"))
342+
343+
if err != nil {
344+
log.Fatal(err)
345+
}
346+
347+
os.Setenv("KBFolder", "../../knowledge-base/actions")
348+
349+
queryParams := make(map[string]string)
350+
queryParams["addEmptyTopLevelPermissions"] = "true"
351+
queryParams["addProjectComment"] = "false"
352+
353+
output, err := SecureWorkflow(queryParams, string(input), &mockDynamoDBClient{})
354+
355+
if err != nil {
356+
t.Errorf("Error not expected")
357+
}
358+
359+
expectedOutput, err := ioutil.ReadFile(path.Join(outputDirectory, "empty-permissions.yml"))
360+
361+
if err != nil {
362+
log.Fatal(err)
363+
}
364+
365+
if output.FinalOutput != string(expectedOutput) {
366+
// Write the actual output to a file for debugging
367+
debugFile := path.Join(outputDirectory, "empty-permissions-debug.yml")
368+
err := ioutil.WriteFile(debugFile, []byte(output.FinalOutput), 0644)
369+
if err != nil {
370+
t.Logf("Failed to write debug file: %v", err)
371+
} else {
372+
t.Logf("Actual output written to: %s", debugFile)
373+
}
374+
375+
t.Errorf("test failed empty-permissions.yml did not match expected output\nExpected:\n%s\n\nGot:\n%s",
376+
string(expectedOutput), output.FinalOutput)
377+
}
378+
379+
}

‎testfiles/joblevelpermskb/input/empty-top-level-permissions.yml

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
name: "checkout only workflow"
2+
3+
on:
4+
push:
5+
6+
7+
jobs:
8+
checkout-job:
9+
runs-on: ubuntu-latest
10+
11+
steps:
12+
- name: Checkout
13+
uses: actions/checkout@v3

‎testfiles/joblevelpermskb/output/empty-top-level-permissions.yml

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
name: "checkout only workflow"
2+
3+
on:
4+
push:
5+
6+
7+
jobs:
8+
checkout-job:
9+
permissions:
10+
contents: read # for actions/checkout to fetch code
11+
runs-on: ubuntu-latest
12+
13+
steps:
14+
- name: Checkout
15+
uses: actions/checkout@v3

‎testfiles/secureworkflow/input/empty-permissions.yml

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
on:
2+
push:
3+
4+
jobs:
5+
test:
6+
runs-on: ubuntu-latest
7+
8+
steps:
9+
- name: Checkout
10+
uses: actions/checkout@v2
11+
- name: Setup Node
12+
uses: actions/setup-node@v1

‎testfiles/secureworkflow/output/empty-permissions.yml

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
on:
2+
push:
3+
4+
permissions: {}
5+
6+
jobs:
7+
test:
8+
permissions:
9+
contents: read # for actions/checkout to fetch code
10+
runs-on: ubuntu-latest
11+
12+
steps:
13+
- name: Harden the runner (Audit all outbound calls)
14+
uses: step-security/harden-runner@17d0e2bd7d51742c71671bd19fa12bdc9d40a3d6 # v2.8.1
15+
with:
16+
egress-policy: audit
17+
18+
- name: Checkout
19+
uses: actions/checkout@ee0669bd1cc54295c223e0bb666b733df41de1c5 # v2.7.0
20+
- name: Setup Node
21+
uses: actions/setup-node@f1f314fca9dfce2769ece7d933488f076716723e # v1.4.6

0 commit comments

Comments
 (0)