-
Notifications
You must be signed in to change notification settings - Fork 33
add ansible-lint rules to ensure no static secrets and empty defaults are present in the code #526
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
base: master
Are you sure you want to change the base?
Changes from all commits
93be9bb
64545cf
719feb8
78f0805
1b054ec
a9de1aa
7fe7be0
c452cdd
7b0048c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| """Implementation of var-defaults rule.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from typing import TYPE_CHECKING | ||
|
|
||
| from ansiblelint.rules import AnsibleLintRule | ||
| from ansiblelint.utils import parse_yaml_from_file | ||
|
|
||
| if TYPE_CHECKING: | ||
| from ansiblelint.errors import MatchError | ||
| from ansiblelint.file_utils import Lintable | ||
|
|
||
|
|
||
| class EmptyDefaultsRule(AnsibleLintRule): | ||
| """Role default variables should not have empty values.""" | ||
|
|
||
| id = "var-defaults" | ||
| severity = "HIGH" | ||
| tags = ["idiom"] | ||
| version_added = "custom" | ||
|
|
||
| _ids = { | ||
| "var-defaults[no-empty]": "Role default variables must not be null or empty strings.", | ||
| } | ||
|
|
||
| def matchyaml(self, file: Lintable) -> list[MatchError]: | ||
| """Return matches for empty defaults in role defaults files.""" | ||
| results: list[MatchError] = [] | ||
|
|
||
| if str(file.kind) != "vars" or not file.data: | ||
| return results | ||
|
|
||
| if not file.role or "defaults" not in file.path.parts: | ||
| return results | ||
|
|
||
| meta_data = parse_yaml_from_file(str(file.path)) | ||
| if not isinstance(meta_data, dict): | ||
| return results | ||
|
|
||
| for key, value in meta_data.items(): | ||
| if value is None or value == "": | ||
| results.append( | ||
| self.create_matcherror( | ||
| message=f"Role default variable '{key}' has an empty value. Use `undef(hint='…')` to indicate defaults that need to be overriden.", | ||
| filename=file, | ||
| tag="var-defaults[no-empty]", | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this uses We should either:
I slightly lean towards the dedicated naming, but eager to hear what you think.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dedicated looks good to me |
||
| data=key, | ||
| ), | ||
| ) | ||
|
|
||
| return results | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| """Implementation of var-secrets rule.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from typing import TYPE_CHECKING | ||
|
|
||
| from ansiblelint.rules import AnsibleLintRule | ||
| from ansiblelint.text import has_jinja | ||
| from ansiblelint.utils import parse_yaml_from_file | ||
|
|
||
| if TYPE_CHECKING: | ||
| from ansiblelint.errors import MatchError | ||
| from ansiblelint.file_utils import Lintable | ||
|
|
||
| SECRET_SUFFIXES = ( | ||
| "_password", | ||
| "_passwd", | ||
| "_secret", | ||
| "_token", | ||
| ) | ||
|
|
||
|
|
||
| class NoStaticSecretsRule(AnsibleLintRule): | ||
| """Variables that look like secrets must not have static default values.""" | ||
|
|
||
| id = "var-secrets" | ||
| severity = "HIGH" | ||
| tags = ["security"] | ||
| version_added = "custom" | ||
|
|
||
| _ids = { | ||
| "var-secrets[no-static]": "Secret variables must use Jinja expressions, not static strings.", | ||
| } | ||
|
|
||
| @staticmethod | ||
| def _looks_like_secret(name: str) -> bool: | ||
| return any(name.endswith(suffix) for suffix in SECRET_SUFFIXES) | ||
|
|
||
| def matchyaml(self, file: Lintable) -> list[MatchError]: | ||
| """Flag secret-looking variables with static string values.""" | ||
| results: list[MatchError] = [] | ||
|
|
||
| if str(file.kind) != "vars" or not file.data: | ||
| return results | ||
|
|
||
| meta_data = parse_yaml_from_file(str(file.path)) | ||
| if not isinstance(meta_data, dict): | ||
| return results | ||
|
|
||
| for key, value in meta_data.items(): | ||
| if not self._looks_like_secret(str(key)): | ||
| continue | ||
| if isinstance(value, str) and not has_jinja(value): | ||
| results.append( | ||
| self.create_matcherror( | ||
| message=f"Secret variable '{key}' has a static value. Use a Jinja expression instead.", | ||
| filename=file, | ||
| tag="var-secrets[no-static]", | ||
| data=key, | ||
| ), | ||
| ) | ||
|
|
||
| return results |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,5 @@ candlepin_database_host: localhost | |
| candlepin_database_port: 5432 | ||
| candlepin_database_ssl: false | ||
| candlepin_database_ssl_mode: disable | ||
| candlepin_database_ssl_ca: | ||
| candlepin_database_ssl_ca: # noqa: var-defaults[no-empty] | ||
| candlepin_database_ssl_ca_path: /etc/candlepin/certs/db-ca.crt | ||
| candlepin_database_ssl_cert: | ||
| candlepin_database_ssl_key: | ||
|
Comment on lines
-25
to
-26
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. love finding unused vars :) |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| """Makes ansible-lint pytest fixtures available for lint rule tests.""" | ||
|
|
||
| import pytest | ||
| from ansiblelint.file_utils import Lintable | ||
| from ansiblelint.rules import RulesCollection | ||
| from ansiblelint.runner import Runner | ||
| from ansiblelint.testing.fixtures import * # noqa: F403 | ||
|
|
||
| CUSTOM_RULESDIR = ".ansible-lint-rules" | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def custom_rules(config_options, app): # noqa: F811 | ||
| """Return a RulesCollection loaded from .ansible-lint-rules/.""" | ||
| from ansiblelint.rules import RulesCollection | ||
|
|
||
| return RulesCollection( | ||
| app=app, | ||
| rulesdirs=[CUSTOM_RULESDIR], | ||
| options=config_options, | ||
| ) | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def ansible_lint_runner(request, custom_rules: RulesCollection) -> list: | ||
| path = request.param[0] | ||
| rule_id = request.param[1] | ||
| results = Runner(Lintable(path), rules=custom_rules).run() | ||
| return [r for r in results if r.rule.id == rule_id] |
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.
So just to clarify, The test fixture has passed
test_empty_defaults_plugins: []which is not flagged right. Shouldn't empty lists([])also be checked in that case? Or is empty list a valid default?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.
I've kept
[]as allowed, as my trail of thought was: "this is usually used for defining additions (e.g. plugins), so an empty list is ok"