From b52eaab0c54307607216bf2765dd01660f153b07 Mon Sep 17 00:00:00 2001 From: Joseph Flinn Date: Fri, 12 Jan 2024 10:22:38 -0800 Subject: [PATCH] Update with linter findings for RuleStepUsesApproved --- .../src/rules/job_environment_prefix.py | 2 +- lint-workflow/src/rules/name_capitalized.py | 4 +- lint-workflow/src/rules/name_exists.py | 4 +- lint-workflow/src/rules/pinned_job_runner.py | 18 ++++++++- lint-workflow/src/rules/step_approved.py | 28 +++++++------ lint-workflow/src/rules/step_pinned.py | 2 +- .../tests/rules/test_pinned_job_runner.py | 24 +++++------ .../tests/rules/test_step_approved.py | 40 +++++++++---------- 8 files changed, 67 insertions(+), 55 deletions(-) diff --git a/lint-workflow/src/rules/job_environment_prefix.py b/lint-workflow/src/rules/job_environment_prefix.py index 036e5c04..e631ce2e 100644 --- a/lint-workflow/src/rules/job_environment_prefix.py +++ b/lint-workflow/src/rules/job_environment_prefix.py @@ -1,10 +1,10 @@ """A Rule to enforce prefixes environment variables.""" from typing import Union, Tuple, List -from ..rule import Rule from ..models.job import Job from ..models.workflow import Workflow from ..models.step import Step +from ..rule import Rule from ..utils import LintLevels, Settings diff --git a/lint-workflow/src/rules/name_capitalized.py b/lint-workflow/src/rules/name_capitalized.py index 84b4d948..4e24bbfc 100644 --- a/lint-workflow/src/rules/name_capitalized.py +++ b/lint-workflow/src/rules/name_capitalized.py @@ -1,10 +1,10 @@ """A Rule to enforce all 'name' values start with a capital letter.""" -from typing import Union, Tuple +from typing import Tuple, Union -from ..rule import Rule from ..models.workflow import Workflow from ..models.job import Job from ..models.step import Step +from ..rule import Rule from ..utils import LintLevels, Settings diff --git a/lint-workflow/src/rules/name_exists.py b/lint-workflow/src/rules/name_exists.py index e3a8920d..021218d3 100644 --- a/lint-workflow/src/rules/name_exists.py +++ b/lint-workflow/src/rules/name_exists.py @@ -1,10 +1,10 @@ """A Rule to enforce that a 'name' exists.""" -from typing import Union, Tuple +from typing import Tuple, Union -from ..rule import Rule from ..models.workflow import Workflow from ..models.job import Job from ..models.step import Step +from ..rule import Rule from ..utils import LintLevels, Settings diff --git a/lint-workflow/src/rules/pinned_job_runner.py b/lint-workflow/src/rules/pinned_job_runner.py index d2897f48..135baea2 100644 --- a/lint-workflow/src/rules/pinned_job_runner.py +++ b/lint-workflow/src/rules/pinned_job_runner.py @@ -1,14 +1,28 @@ -from typing import Union, Tuple +"""A Rule to enforce pinning runners to a specific OS version.""" +from typing import List, Tuple, Union -from ..rule import Rule from ..models.job import Job from ..models.workflow import Workflow from ..models.step import Step +from ..rule import Rule from ..utils import LintLevels, Settings class RuleJobRunnerVersionPinned(Rule): + """Rule to enforce pinned Runner OS versions. + + `*-latest` versions updating without knowing has broken all of our worklfows + in the past. To avoid this and prevent a single event from breaking the majority of + our pipelines, we pin the versions. + """ def __init__(self, settings: Settings = None) -> None: + """Constructor for RuleJobRunnerVersionPinned to override Rule class. + + Args: + settings: + A Settings object that contains any default, overriden, or custom settings + required anywhere in the application. + """ self.message = "Workflow runner must be pinned" self.on_fail: LintLevels = LintLevels.ERROR self.compatibility: List[Union[Workflow, Job, Step]] = [Job] diff --git a/lint-workflow/src/rules/step_approved.py b/lint-workflow/src/rules/step_approved.py index b184059b..276ea1ac 100644 --- a/lint-workflow/src/rules/step_approved.py +++ b/lint-workflow/src/rules/step_approved.py @@ -1,15 +1,22 @@ -from typing import Union, Tuple +"""A Rule to enforce the use of a list of pre-approved Actions.""" +from typing import List, Tuple, Union -from ..rule import Rule from ..models.job import Job from ..models.workflow import Workflow from ..models.step import Step +from ..rule import Rule from ..utils import LintLevels, Settings class RuleStepUsesApproved(Rule): + """Rule to enforce that all Actions have been pre-approved. + + To limit the surface area of a supply chain attack in our pipelines, all Actions + are required to pass a security review and be added to the pre-approved list to + check against. + """ def __init__(self, settings: Settings = None) -> None: - self.message = f"error" + self.message = "error" self.on_fail: LintLevels = LintLevels.WARNING self.compatibility: List[Union[Workflow, Job, Step]] = [Step] self.settings: Settings = settings @@ -69,19 +76,14 @@ def fn(self, obj: Step) -> Tuple[bool, str]: if self.skip(obj): return True, "" - path, hash = obj.uses.split("@") - # Actions in bitwarden/gh-actions are auto-approved - if not path in self.settings.approved_actions: - return ( - False, - ( - f"New Action detected: {path}\n" - "For security purposes, actions must be reviewed and on the pre-approved list" - ), + if obj.uses and not obj.uses_path in self.settings.approved_actions: + return False, ( + f"New Action detected: {obj.uses_path}\nFor security purposes, " + "actions must be reviewed and on the pre-approved list" ) - action = self.settings.approved_actions[path] + action = self.settings.approved_actions[obj.uses_path] if obj.uses_version != action.version or obj.uses_ref != action.sha: return False, ( diff --git a/lint-workflow/src/rules/step_pinned.py b/lint-workflow/src/rules/step_pinned.py index 2f49cfd7..a2a1efd9 100644 --- a/lint-workflow/src/rules/step_pinned.py +++ b/lint-workflow/src/rules/step_pinned.py @@ -1,9 +1,9 @@ from typing import Union, Tuple -from ..rule import Rule from ..models.job import Job from ..models.workflow import Workflow from ..models.step import Step +from ..rule import Rule from ..utils import LintLevels, Settings diff --git a/lint-workflow/tests/rules/test_pinned_job_runner.py b/lint-workflow/tests/rules/test_pinned_job_runner.py index 59d0b93e..e3facfad 100644 --- a/lint-workflow/tests/rules/test_pinned_job_runner.py +++ b/lint-workflow/tests/rules/test_pinned_job_runner.py @@ -1,18 +1,16 @@ +"""Test src/rules/pinned_job_runner.py.""" import pytest from ruamel.yaml import YAML -from ..conftest import FIXTURE_DIR -from ..context import src - from src.load import WorkflowBuilder from src.rules.pinned_job_runner import RuleJobRunnerVersionPinned yaml = YAML() -@pytest.fixture -def correct_runner(): +@pytest.fixture(name="correct_runner") +def fixture_correct_runner(): workflow = """\ --- on: @@ -27,8 +25,8 @@ def correct_runner(): return WorkflowBuilder.build(workflow=yaml.load(workflow), from_file=False) -@pytest.fixture -def incorrect_runner(): +@pytest.fixture(name="incorrect_runner") +def fixture_incorrect_runner(): workflow = """\ --- on: @@ -43,16 +41,16 @@ def incorrect_runner(): return WorkflowBuilder.build(workflow=yaml.load(workflow), from_file=False) -@pytest.fixture -def rule(): +@pytest.fixture(name="rule") +def fixture_rule(): return RuleJobRunnerVersionPinned() def test_rule_on_correct_runner(rule, correct_runner): - result, message = rule.fn(correct_runner.jobs["job-key"]) - assert result == True + result, _ = rule.fn(correct_runner.jobs["job-key"]) + assert result is True def test_rule_on_incorrect_runner(rule, incorrect_runner): - result, message = rule.fn(incorrect_runner.jobs["job-key"]) - assert result == False + result, _ = rule.fn(incorrect_runner.jobs["job-key"]) + assert result is False diff --git a/lint-workflow/tests/rules/test_step_approved.py b/lint-workflow/tests/rules/test_step_approved.py index 2c3ba039..c343fecd 100644 --- a/lint-workflow/tests/rules/test_step_approved.py +++ b/lint-workflow/tests/rules/test_step_approved.py @@ -1,10 +1,8 @@ +"""Test src/rules/step_approved.py.""" import pytest from ruamel.yaml import YAML -from ..conftest import FIXTURE_DIR -from ..context import src - from src.load import WorkflowBuilder from src.rules.step_approved import RuleStepUsesApproved from src.utils import Settings @@ -13,8 +11,8 @@ yaml = YAML() -@pytest.fixture -def settings(): +@pytest.fixture(name="settings") +def fixture_settings(): return Settings( approved_actions={ "actions/checkout": { @@ -31,8 +29,8 @@ def settings(): ) -@pytest.fixture -def correct_workflow(): +@pytest.fixture(name="correct_workflow") +def fixture_correct_workflow(): workflow = """\ --- on: @@ -57,8 +55,8 @@ def correct_workflow(): return WorkflowBuilder.build(workflow=yaml.load(workflow), from_file=False) -@pytest.fixture -def incorrect_workflow(): +@pytest.fixture(name="incorrect_workflow") +def fixture_incorrect_workflow(): workflow = """\ --- on: @@ -77,32 +75,32 @@ def incorrect_workflow(): return WorkflowBuilder.build(workflow=yaml.load(workflow), from_file=False) -@pytest.fixture -def rule(settings): +@pytest.fixture(name="rule") +def fixture_rule(settings): return RuleStepUsesApproved(settings=settings) def test_rule_on_correct_workflow(rule, correct_workflow): - result, message = rule.fn(correct_workflow.jobs["job-key"].steps[0]) - assert result == True + result, _ = rule.fn(correct_workflow.jobs["job-key"].steps[0]) + assert result is True - result, message = rule.fn(correct_workflow.jobs["job-key"].steps[1]) - assert result == True + result, _ = rule.fn(correct_workflow.jobs["job-key"].steps[1]) + assert result is True - result, message = rule.fn(correct_workflow.jobs["job-key"].steps[2]) - assert result == True + result, _ = rule.fn(correct_workflow.jobs["job-key"].steps[2]) + assert result is True - result, message = rule.fn(correct_workflow.jobs["job-key"].steps[3]) - assert result == True + result, _ = rule.fn(correct_workflow.jobs["job-key"].steps[3]) + assert result is True def test_rule_on_incorrect_workflow(rule, incorrect_workflow): result, message = rule.fn(incorrect_workflow.jobs["job-key"].steps[0]) - assert result == False + assert result is False assert "New Action detected" in message result, message = rule.fn(incorrect_workflow.jobs["job-key"].steps[1]) - assert result == False + assert result is False assert "Action is out of date" in message