add ansible-lint rules to ensure no static secrets and empty defaults are present in the code by evgeni · Pull Request #526 · theforeman/foremanctl

evgeni · 2026-05-28T06:36:48Z

Why are you introducing these changes? (Problem description, related links)

#523 (review)

What are the changes introduced in this pull request?

add a var-defaults[no-empty] check that ensures no defaults are empty
add a var-secrets[no-static] check that ensure we don't use static secrets
fix violations of those checks (partially via Generate passwords instead of using hard-coded ones #524 )

How to test this pull request

Steps to reproduce:

Checklist

Tests added/updated (if applicable)
Documentation updated (if applicable)

archanaserver · 2026-05-28T09:28:27Z

+    def test_empty_defaults_are_flagged(
+        config_options: Options,
+        app: object,
+    ) -> None:
+        """Null and empty string defaults produce match errors."""
+        rules = RulesCollection(app=app, options=config_options)
+        rules.register(EmptyDefaultsRule())
+        results = Runner(
+            Lintable("tests/fixtures/ansible-lint/roles/test_empty_defaults"),
+            rules=rules,
+        ).run()
+        empty_results = [r for r in results if r.rule.id == EmptyDefaultsRule.id]
+        assert len(empty_results) == 4
+        for result in empty_results:
+            assert result.tag == "var-defaults[no-empty]"


but isn't this only happy path testing and this but doesn't explicitly verify that edge cases are correctly NOT flagged.

which edge cases? tests/fixtures/ansible-lint/roles/test_empty_defaults/defaults/main.yml contains 9 variables, and the assertion is that we flag 4 of them as "empty" (the ones that are either null or ""), but others (e.g. false and []) are accepted

Okay, I see your point with that current cases. 🙌

archanaserver · 2026-05-28T09:28:33Z

+            return results
+
+        for key, value in meta_data.items():
+            if value is None or value == "":


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?

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"

archanaserver

Dropped comments just to clarify some thought on this.

ehelms · 2026-05-28T14:38:35Z

This is from me testing out a review skill I wrote:

empty_defaults.py and no_static_secrets.py — redundant YAML parse

.ansible-lint-rules/empty_defaults.py:36 — parse_yaml_from_file(str(file.path)) re-parses a file that file.data already holds. The rule already guards against empty data with if not file.data, so meta_data = dict(file.data) is sufficient. Same pattern in no_static_secrets.py:51.

no_static_secrets rule: None/empty secret values slip through

.ansible-lint-rules/no_static_secrets.py:50 — The rule only matches isinstance(value, str) and not has_jinja(value). A null value for a secret variable (e.g., some_password:) is not flagged. Role defaults will catch it via var-defaults[no-empty], but non-role vars files with a null secret variable are silently ignored. Low risk given how foremanctl uses secrets, but worth noting.

evgeni · 2026-05-29T07:01:38Z

empty_defaults.py and no_static_secrets.py — redundant YAML parse

.ansible-lint-rules/empty_defaults.py:36 — parse_yaml_from_file(str(file.path)) re-parses a file that file.data already holds. The rule already guards against empty data with if not file.data, so meta_data = dict(file.data) is sufficient. Same pattern in no_static_secrets.py:51.

Lintable.data uses parse_yaml_linenumbers, not parse_yaml_from_file, and while the tests do pass after the change to file.data, I am not versed enough in ansible-lint internals and would follow the existing core rules like var-naming which use parse_yaml_from_file explicitly

evgeni · 2026-05-29T08:01:48Z

-candlepin_database_ssl_cert:
-candlepin_database_ssl_key:


love finding unused vars :)

evgeni · 2026-05-29T08:07:07Z

+                    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]",


this uses id[sub-id] style, as I've used var-naming rule as a base, and this has multiple sub-tags.
after using the two rules in this PR (and also flagging certain vars as noqa for them), I think this is wrong.

We should either:

Have a single var-content rule file with multiple tags (var-content[no-empty-defaults], var-content[no-static-secrets])

Have dedicated named rules like no-empty-defaults and no-static-secrets

I slightly lean towards the dedicated naming, but eager to hear what you think.

Dedicated looks good to me

evgeni · 2026-05-29T08:56:37Z

no_static_secrets rule: None/empty secret values slip through

.ansible-lint-rules/no_static_secrets.py:50 — The rule only matches isinstance(value, str) and not has_jinja(value). A null value for a secret variable (e.g., some_password:) is not flagged. Role defaults will catch it via var-defaults[no-empty], but non-role vars files with a null secret variable are silently ignored. Low risk given how foremanctl uses secrets, but worth noting.

I guess null is static by definition, but do we really need to care?
OTOH, adding it to the linter doesn't cost much, so 🤷‍♀️

pr-processor Bot added the Not yet reviewed label May 28, 2026

evgeni mentioned this pull request May 28, 2026

Force FDW defaults to use undef for things that need inputs #523

Merged

2 tasks

add a var-defaults[no-empty] check that ensures no defaults are empty

93be9bb

evgeni force-pushed the lint-empty-defaults branch from 8ed56d7 to 93be9bb Compare May 28, 2026 06:44

evgeni commented May 28, 2026

View reviewed changes

Comment thread .ansible-lint-rules/empty_defaults.py Outdated

pr-processor Bot removed the Not yet reviewed label May 28, 2026

archanaserver reviewed May 28, 2026

View reviewed changes

add no-static-secrets ansible-lint rule

64545cf

evgeni mentioned this pull request May 28, 2026

Generate passwords instead of using hard-coded ones #524

Open

2 tasks

fix static default passwords in iop roles

719feb8

ehelms mentioned this pull request May 28, 2026

Set explicit volume mount permissions #375

Open

evgeni added 2 commits May 28, 2026 14:57

fix static default password in postgresql role

78f0805

allow empty ca paths

1b054ec

evgeni added 2 commits May 29, 2026 08:42

Generate passwords instead of using hard-coded ones

a9de1aa

mark ca_key_password as no-qa for static secret -- it's a path

7fe7be0

fix new lint rules in foreman_development role

c452cdd

evgeni force-pushed the lint-empty-defaults branch 3 times, most recently from b950c1f to f6a48ab Compare May 29, 2026 07:58

evgeni commented May 29, 2026

View reviewed changes

evgeni changed the title ~~add a var-defaults[no-empty] check that ensures no defaults are empty~~ add ansible-lint rules to ensure no static secrets and empty defaults are present in the code May 29, 2026

move tests to the normal location, so they get executed

7b0048c

evgeni force-pushed the lint-empty-defaults branch from f6a48ab to 7b0048c Compare May 29, 2026 08:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add ansible-lint rules to ensure no static secrets and empty defaults are present in the code#526

add ansible-lint rules to ensure no static secrets and empty defaults are present in the code#526
evgeni wants to merge 9 commits into
masterfrom
lint-empty-defaults

evgeni commented May 28, 2026 •

edited

Loading

Uh oh!

Uh oh!

archanaserver May 28, 2026

Uh oh!

evgeni May 28, 2026

Uh oh!

archanaserver May 29, 2026

Uh oh!

archanaserver May 28, 2026

Uh oh!

evgeni May 28, 2026

Uh oh!

archanaserver left a comment

Uh oh!

ehelms commented May 28, 2026

Uh oh!

evgeni commented May 29, 2026

Uh oh!

evgeni May 29, 2026

Uh oh!

evgeni May 29, 2026

Uh oh!

ehelms May 29, 2026

Uh oh!

evgeni commented May 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		candlepin_database_ssl_cert:
		candlepin_database_ssl_key:

Conversation

evgeni commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are you introducing these changes? (Problem description, related links)

What are the changes introduced in this pull request?

How to test this pull request

Checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

archanaserver left a comment

Choose a reason for hiding this comment

Uh oh!

ehelms commented May 28, 2026

Uh oh!

evgeni commented May 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evgeni commented May 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

evgeni commented May 28, 2026 •

edited

Loading