diff --git a/remediation/workflow/permissions/permissions.go b/remediation/workflow/permissions/permissions.go index 5964c0c1f..018eea47b 100644 --- a/remediation/workflow/permissions/permissions.go +++ b/remediation/workflow/permissions/permissions.go @@ -89,7 +89,7 @@ func githubTokenInJobLevelEnv(job metadata.Job) bool { return false } -func AddWorkflowLevelPermissions(inputYaml string, addProjectComment bool) (string, error) { +func AddWorkflowLevelPermissions(inputYaml string, addProjectComment bool, addEmptyTopLevelPermissions bool) (string, error) { workflow := metadata.Workflow{} err := yaml.Unmarshal([]byte(inputYaml), &workflow) @@ -138,13 +138,20 @@ func AddWorkflowLevelPermissions(inputYaml string, addProjectComment bool) (stri spaces += " " } - if addProjectComment { - output = append(output, spaces+"permissions: # added using https://github.com/step-security/secure-repo") + if addEmptyTopLevelPermissions { + if addProjectComment { + output = append(output, spaces+"permissions: {} # added using https://github.com/step-security/secure-repo") + } else { + output = append(output, spaces+"permissions: {}") + } } else { - output = append(output, spaces+"permissions:") + if addProjectComment { + output = append(output, spaces+"permissions: # added using https://github.com/step-security/secure-repo") + } else { + output = append(output, spaces+"permissions:") + } + output = append(output, spaces+" contents: read") } - - output = append(output, spaces+" contents: read") output = append(output, "") for i := line - 1; i < len(inputLines); i++ { @@ -154,7 +161,7 @@ func AddWorkflowLevelPermissions(inputYaml string, addProjectComment bool) (stri return strings.Join(output, "\n"), nil } -func AddJobLevelPermissions(inputYaml string) (*SecureWorkflowReponse, error) { +func AddJobLevelPermissions(inputYaml string, addEmptyTopLevelPermissions bool) (*SecureWorkflowReponse, error) { workflow := metadata.Workflow{} errors := make(map[string][]string) @@ -216,7 +223,7 @@ func AddJobLevelPermissions(inputYaml string) (*SecureWorkflowReponse, error) { if strings.Compare(inputYaml, fixWorkflowPermsReponse.FinalOutput) != 0 { fixWorkflowPermsReponse.IsChanged = true - if len(perms) == 1 && strings.Contains(perms[0], contents_read) { + if len(perms) == 1 && strings.Contains(perms[0], contents_read) && !addEmptyTopLevelPermissions { // Don't add contents: read, because it will get defined at workflow level continue } else { diff --git a/remediation/workflow/permissions/permissions_test.go b/remediation/workflow/permissions/permissions_test.go index 66ccca031..993bbdcd6 100644 --- a/remediation/workflow/permissions/permissions_test.go +++ b/remediation/workflow/permissions/permissions_test.go @@ -18,6 +18,11 @@ func TestAddJobLevelPermissions(t *testing.T) { } for _, f := range files { + + if f.Name() == "empty-top-level-permissions.yml" { + continue + } + input, err := ioutil.ReadFile(path.Join(inputDirectory, f.Name())) if err != nil { @@ -26,7 +31,7 @@ func TestAddJobLevelPermissions(t *testing.T) { os.Setenv("KBFolder", "../../../knowledge-base/actions") - fixWorkflowPermsResponse, err := AddJobLevelPermissions(string(input)) + fixWorkflowPermsResponse, err := AddJobLevelPermissions(string(input), false) output := fixWorkflowPermsResponse.FinalOutput jobErrors := fixWorkflowPermsResponse.JobErrors @@ -68,6 +73,47 @@ func TestAddJobLevelPermissions(t *testing.T) { } } +func TestAddJobLevelPermissionsWithEmptyTopLevel(t *testing.T) { + const inputDirectory = "../../../testfiles/joblevelpermskb/input" + const outputDirectory = "../../../testfiles/joblevelpermskb/output" + + // Test the empty-top-level-permissions.yml file + input, err := ioutil.ReadFile(path.Join(inputDirectory, "empty-top-level-permissions.yml")) + if err != nil { + t.Fatal(err) + } + + expectedOutput, err := ioutil.ReadFile(path.Join(outputDirectory, "empty-top-level-permissions.yml")) + if err != nil { + t.Fatal(err) + } + + os.Setenv("KBFolder", "../../../knowledge-base/actions") + + // Test with addEmptyTopLevelPermissions = true + fixWorkflowPermsResponse, err := AddJobLevelPermissions(string(input), true) + if err != nil { + t.Errorf("Unexpected error with addEmptyTopLevelPermissions=true: %v", err) + } + + if fixWorkflowPermsResponse.FinalOutput != string(expectedOutput) { + t.Errorf("test failed with addEmptyTopLevelPermissions=true for empty-top-level-permissions.yml\nExpected:\n%s\n\nGot:\n%s", + string(expectedOutput), fixWorkflowPermsResponse.FinalOutput) + } + + // Test with addEmptyTopLevelPermissions = false (should skip contents: read) + fixWorkflowPermsResponse2, err2 := AddJobLevelPermissions(string(input), false) + if err2 != nil { + t.Errorf("Unexpected error with addEmptyTopLevelPermissions=false: %v", err2) + } + + // With false, contents: read should be skipped at job level + if fixWorkflowPermsResponse2.FinalOutput != string(input) { + t.Errorf("test failed with addEmptyTopLevelPermissions=false for empty-top-level-permissions.yml\nExpected:\n%s\n\nGot:\n%s", + string(input), fixWorkflowPermsResponse2.FinalOutput) + } +} + func Test_addPermissions(t *testing.T) { type args struct { inputYaml string @@ -112,6 +158,10 @@ func TestAddWorkflowLevelPermissions(t *testing.T) { continue } + if f.Name() == "empty-permissions.yml" { + continue + } + input, err := ioutil.ReadFile(path.Join(inputDirectory, f.Name())) if err != nil { @@ -125,7 +175,7 @@ func TestAddWorkflowLevelPermissions(t *testing.T) { addProjectComment = true } - output, err := AddWorkflowLevelPermissions(string(input), addProjectComment) + output, err := AddWorkflowLevelPermissions(string(input), addProjectComment, false) if err != nil { t.Errorf("Error not expected") @@ -143,3 +193,41 @@ func TestAddWorkflowLevelPermissions(t *testing.T) { } } + +func TestAddWorkflowLevelPermissionsWithEmpty(t *testing.T) { + const inputDirectory = "../../../testfiles/toplevelperms/input" + const outputDirectory = "../../../testfiles/toplevelperms/output" + + // Test the empty-permissions.yml file + input, err := ioutil.ReadFile(path.Join(inputDirectory, "empty-permissions.yml")) + if err != nil { + t.Fatal(err) + } + + expectedOutput, err := ioutil.ReadFile(path.Join(outputDirectory, "empty-permissions.yml")) + if err != nil { + t.Fatal(err) + } + + // Test with addEmptyTopLevelPermissions = true + output, err := AddWorkflowLevelPermissions(string(input), false, true) + if err != nil { + t.Errorf("Unexpected error with addEmptyTopLevelPermissions=true: %v", err) + } + + if output != string(expectedOutput) { + t.Errorf("test failed with addEmptyTopLevelPermissions=true for empty-permissions.yml\nExpected:\n%s\n\nGot:\n%s", + string(expectedOutput), output) + } + + // Test with addEmptyTopLevelPermissions = false (should add contents: read) + output2, err2 := AddWorkflowLevelPermissions(string(input), false, false) + if err2 != nil { + t.Errorf("Unexpected error with addEmptyTopLevelPermissions=false: %v", err2) + } + + // With false, should add contents: read instead of empty permissions + if !strings.Contains(output2, "contents: read") || strings.Contains(output2, "permissions: {}") { + t.Errorf("test failed with addEmptyTopLevelPermissions=false for empty-permissions.yml - should contain 'contents: read' but not 'permissions: {}'\nGot:\n%s", output2) + } +} diff --git a/remediation/workflow/secureworkflow.go b/remediation/workflow/secureworkflow.go index 114e34601..9c88674bc 100644 --- a/remediation/workflow/secureworkflow.go +++ b/remediation/workflow/secureworkflow.go @@ -22,6 +22,7 @@ func SecureWorkflow(queryStringParams map[string]string, inputYaml string, svc d pinnedActions, addedHardenRunner, addedPermissions, replacedMaintainedActions := false, false, false, false ignoreMissingKBs := false enableLogging := false + addEmptyTopLevelPermissions := false exemptedActions, pinToImmutable, maintainedActionsMap := []string{}, false, map[string]string{} if len(params) > 0 { @@ -68,6 +69,10 @@ func SecureWorkflow(queryStringParams map[string]string, inputYaml string, svc d enableLogging = true } + if queryStringParams["addEmptyTopLevelPermissions"] == "true" { + addEmptyTopLevelPermissions = true + } + if enableLogging { // Log query parameters paramsJSON, _ := json.MarshalIndent(queryStringParams, "", " ") @@ -83,7 +88,7 @@ func SecureWorkflow(queryStringParams map[string]string, inputYaml string, svc d if enableLogging { log.Printf("Adding job level permissions") } - secureWorkflowReponse, err = permissions.AddJobLevelPermissions(secureWorkflowReponse.FinalOutput) + secureWorkflowReponse, err = permissions.AddJobLevelPermissions(secureWorkflowReponse.FinalOutput, addEmptyTopLevelPermissions) secureWorkflowReponse.OriginalInput = inputYaml if err != nil { if enableLogging { @@ -95,7 +100,7 @@ func SecureWorkflow(queryStringParams map[string]string, inputYaml string, svc d if enableLogging { log.Printf("Adding workflow level permissions") } - secureWorkflowReponse.FinalOutput, err = permissions.AddWorkflowLevelPermissions(secureWorkflowReponse.FinalOutput, addProjectComment) + secureWorkflowReponse.FinalOutput, err = permissions.AddWorkflowLevelPermissions(secureWorkflowReponse.FinalOutput, addProjectComment, addEmptyTopLevelPermissions) if err != nil { if enableLogging { log.Printf("Error adding workflow level permissions: %v", err) diff --git a/remediation/workflow/secureworkflow_test.go b/remediation/workflow/secureworkflow_test.go index 6be2e663c..868d53c9d 100644 --- a/remediation/workflow/secureworkflow_test.go +++ b/remediation/workflow/secureworkflow_test.go @@ -277,3 +277,103 @@ func TestSecureWorkflow(t *testing.T) { } } + +func TestSecureWorkflowEmptyPermissions(t *testing.T) { + const inputDirectory = "../../testfiles/secureworkflow/input" + const outputDirectory = "../../testfiles/secureworkflow/output" + + httpmock.Activate() + defer httpmock.DeactivateAndReset() + + // Mock APIs for actions/checkout + httpmock.RegisterResponder("GET", "https://api.github.com/repos/actions/checkout/commits/v2", + httpmock.NewStringResponder(200, `ee0669bd1cc54295c223e0bb666b733df41de1c5`)) + + httpmock.RegisterResponder("GET", "https://api.github.com/repos/actions/checkout/git/matching-refs/tags/v2.", + httpmock.NewStringResponder(200, + `[ + { + "ref": "refs/tags/v2.7.0", + "object": { + "sha": "ee0669bd1cc54295c223e0bb666b733df41de1c5", + "type": "commit" + } + } + ]`), + ) + + // Mock APIs for actions/setup-node + httpmock.RegisterResponder("GET", "https://api.github.com/repos/actions/setup-node/commits/v1", + httpmock.NewStringResponder(200, `f1f314fca9dfce2769ece7d933488f076716723e`)) + + httpmock.RegisterResponder("GET", "https://api.github.com/repos/actions/setup-node/git/matching-refs/tags/v1.", + httpmock.NewStringResponder(200, + `[ + { + "ref": "refs/tags/v1.4.6", + "object": { + "sha": "f1f314fca9dfce2769ece7d933488f076716723e", + "type": "commit" + } + } + ]`), + ) + + // Mock APIs for step-security/harden-runner + httpmock.RegisterResponder("GET", "https://api.github.com/repos/step-security/harden-runner/commits/v2", + httpmock.NewStringResponder(200, `17d0e2bd7d51742c71671bd19fa12bdc9d40a3d6`)) + + httpmock.RegisterResponder("GET", "https://api.github.com/repos/step-security/harden-runner/git/matching-refs/tags/v2.", + httpmock.NewStringResponder(200, + `[ + { + "ref": "refs/tags/v2.8.1", + "object": { + "sha": "17d0e2bd7d51742c71671bd19fa12bdc9d40a3d6", + "type": "commit" + } + } + ]`), + ) + + var err error + var input []byte + input, err = ioutil.ReadFile(path.Join(inputDirectory, "empty-permissions.yml")) + + if err != nil { + log.Fatal(err) + } + + os.Setenv("KBFolder", "../../knowledge-base/actions") + + queryParams := make(map[string]string) + queryParams["addEmptyTopLevelPermissions"] = "true" + queryParams["addProjectComment"] = "false" + + output, err := SecureWorkflow(queryParams, string(input), &mockDynamoDBClient{}) + + if err != nil { + t.Errorf("Error not expected") + } + + expectedOutput, err := ioutil.ReadFile(path.Join(outputDirectory, "empty-permissions.yml")) + + if err != nil { + log.Fatal(err) + } + + if output.FinalOutput != string(expectedOutput) { + // Write the actual output to a file for debugging + debugFile := path.Join(outputDirectory, "empty-permissions-debug.yml") + err := ioutil.WriteFile(debugFile, []byte(output.FinalOutput), 0644) + if err != nil { + t.Logf("Failed to write debug file: %v", err) + } else { + t.Logf("Actual output written to: %s", debugFile) + } + + t.Errorf("test failed empty-permissions.yml did not match expected output\nExpected:\n%s\n\nGot:\n%s", + string(expectedOutput), output.FinalOutput) + } + +} diff --git a/testfiles/joblevelpermskb/input/empty-top-level-permissions.yml b/testfiles/joblevelpermskb/input/empty-top-level-permissions.yml new file mode 100644 index 000000000..6ea984f89 --- /dev/null +++ b/testfiles/joblevelpermskb/input/empty-top-level-permissions.yml @@ -0,0 +1,13 @@ +name: "checkout only workflow" + +on: + push: + + +jobs: + checkout-job: + runs-on: ubuntu-latest + + steps: + - name: Checkout + uses: actions/checkout@v3 \ No newline at end of file diff --git a/testfiles/joblevelpermskb/output/empty-top-level-permissions.yml b/testfiles/joblevelpermskb/output/empty-top-level-permissions.yml new file mode 100644 index 000000000..2febb7397 --- /dev/null +++ b/testfiles/joblevelpermskb/output/empty-top-level-permissions.yml @@ -0,0 +1,15 @@ +name: "checkout only workflow" + +on: + push: + + +jobs: + checkout-job: + permissions: + contents: read # for actions/checkout to fetch code + runs-on: ubuntu-latest + + steps: + - name: Checkout + uses: actions/checkout@v3 \ No newline at end of file diff --git a/testfiles/secureworkflow/input/empty-permissions.yml b/testfiles/secureworkflow/input/empty-permissions.yml new file mode 100644 index 000000000..8b0e188b6 --- /dev/null +++ b/testfiles/secureworkflow/input/empty-permissions.yml @@ -0,0 +1,12 @@ +on: + push: + +jobs: + test: + runs-on: ubuntu-latest + + steps: + - name: Checkout + uses: actions/checkout@v2 + - name: Setup Node + uses: actions/setup-node@v1 \ No newline at end of file diff --git a/testfiles/secureworkflow/output/empty-permissions.yml b/testfiles/secureworkflow/output/empty-permissions.yml new file mode 100644 index 000000000..1574d2ff4 --- /dev/null +++ b/testfiles/secureworkflow/output/empty-permissions.yml @@ -0,0 +1,21 @@ +on: + push: + +permissions: {} + +jobs: + test: + permissions: + contents: read # for actions/checkout to fetch code + runs-on: ubuntu-latest + + steps: + - name: Harden the runner (Audit all outbound calls) + uses: step-security/harden-runner@17d0e2bd7d51742c71671bd19fa12bdc9d40a3d6 # v2.8.1 + with: + egress-policy: audit + + - name: Checkout + uses: actions/checkout@ee0669bd1cc54295c223e0bb666b733df41de1c5 # v2.7.0 + - name: Setup Node + uses: actions/setup-node@f1f314fca9dfce2769ece7d933488f076716723e # v1.4.6 \ No newline at end of file diff --git a/testfiles/toplevelperms/input/empty-permissions.yml b/testfiles/toplevelperms/input/empty-permissions.yml new file mode 100644 index 000000000..0f7c2ee95 --- /dev/null +++ b/testfiles/toplevelperms/input/empty-permissions.yml @@ -0,0 +1,9 @@ +name: "simple workflow" + +jobs: + test: + runs-on: ubuntu-latest + + steps: + - name: Run test + run: echo "test" \ No newline at end of file diff --git a/testfiles/toplevelperms/output/empty-permissions.yml b/testfiles/toplevelperms/output/empty-permissions.yml new file mode 100644 index 000000000..53143ae06 --- /dev/null +++ b/testfiles/toplevelperms/output/empty-permissions.yml @@ -0,0 +1,11 @@ +name: "simple workflow" + +permissions: {} + +jobs: + test: + runs-on: ubuntu-latest + + steps: + - name: Run test + run: echo "test" \ No newline at end of file