feat: auditd - new role - initial commit#1
feat: auditd - new role - initial commit#1richm wants to merge 1 commit intolinux-system-roles:mainfrom
Conversation
Reviewer's GuideAdds a new linux-system-roles.auditd Ansible role that manages auditd.conf and custom audit rules, with full CI, validation, rpm-ostree support, documentation, and tests for argument sanity, rules purging, and integration behavior. Sequence diagram for auditd rules purge and retention behaviorsequenceDiagram
actor Admin
participant AnsiblePlay
participant Role_auditd as Role_auditd
participant Tasks_main as Tasks_main_yml
participant Tasks_purge as Tasks_purge_rules_yml
participant Template_custom as Template_custom_rules_j2
participant Handler_augenrules as Handler_Run_augenrules
Admin->>AnsiblePlay: Run play with auditd_purge_rules=true
AnsiblePlay->>Role_auditd: Apply linux_system_roles.auditd
Role_auditd->>Tasks_main: Execute tasks/main.yml
Note over Tasks_main: Earlier tasks validate vars, set facts,
Note over Tasks_main: install packages, create rules.d
alt auditd_purge_rules and auditd_manage_rules are true
Tasks_main->>Tasks_purge: Include purge_rules.yml
Tasks_purge->>Tasks_purge: Find files in rules.d
Tasks_purge->>Tasks_purge: Stat custom.rules
opt custom.rules exists
Tasks_purge->>Tasks_purge: Slurp custom.rules content
Tasks_purge->>Template_custom: Lookup rendered custom.rules
Template_custom-->>Tasks_purge: Rendered rules text
Tasks_purge->>Tasks_purge: Compute expected_sig from rendered body
Tasks_purge->>Tasks_purge: Compute current_sig from file body
alt Signatures equal
Tasks_purge->>Tasks_purge: Set __auditd_purge_keep_custom_rules=true
else Signatures differ
Tasks_purge->>Tasks_purge: Set __auditd_purge_keep_custom_rules=false
end
end
loop For each file in rules.d
alt File is custom.rules and __auditd_purge_keep_custom_rules is true
Tasks_purge-->>Tasks_purge: Skip deletion
else Delete file
Tasks_purge->>Tasks_purge: Remove file
Tasks_purge->>Handler_augenrules: notify Run_augenrules
end
end
else auditd_purge_rules is false or manage_rules is false
Tasks_main-->>Tasks_main: Skip purge_rules.yml
end
Note over Tasks_main,Template_custom: Later, if auditd_manage_rules and not __auditd_purge_keep_custom_rules
Note over Tasks_main,Template_custom: template custom.rules.j2 overwrites custom.rules and notifies Run_augenrules
Handler_augenrules-->>Handler_augenrules: Run augenrules
Handler_augenrules->>Handler_augenrules: Run augenrules --load when allowed
Handler_augenrules-->>Admin: Audit rules updated on target
Class diagram for linux_system_roles_auditd role structureclassDiagram
class Role_linux_system_roles_auditd {
+defaults_main_yml
+vars_main_yml
+tasks_main_yml
+handlers_main_yml
+templates_auditd_conf_j2
+templates_custom_rules_j2
+meta_main_yml
+meta_argument_specs_yml
}
class Defaults_main_yml {
+bool auditd_local_events
+bool auditd_write_logs
+string auditd_log_file
+string auditd_log_format
+string auditd_log_group
+string auditd_flush
+int auditd_freq
+int auditd_num_logs
+string auditd_dispatcher
+string auditd_name_format
+string auditd_name
+string auditd_disp_qos
+int auditd_max_log_file
+string auditd_max_log_file_action
+string auditd_max_log_file_action_exe
+string auditd_space_left
+string auditd_space_left_action
+string auditd_space_left_action_exe
+string auditd_action_mail_acct
+bool auditd_verify_email
+string auditd_admin_space_left
+string auditd_admin_space_left_action
+string auditd_admin_space_left_action_exe
+string auditd_disk_full_action
+string auditd_disk_full_action_exe
+string auditd_disk_error_action
+string auditd_disk_error_action_exe
+int auditd_priority_boost
+int auditd_tcp_listen_port
+int auditd_tcp_listen_queue
+int auditd_tcp_max_per_addr
+bool auditd_use_libwrap
+string auditd_tcp_client_ports
+int auditd_tcp_client_max_idle
+string auditd_transport
+bool auditd_enable_krb5
+string auditd_krb5_principal
+string auditd_krb5_key_file
+bool auditd_distribute_network
+int auditd_q_depth
+string auditd_overflow_action
+int auditd_max_restarts
+string auditd_plugin_dir
+int auditd_end_of_event_timeout
+string auditd_report_interval
+int auditd_buffer_size
+int auditd_fail_mode
+int auditd_maximum_rate
+int auditd_enable_flag
+bool auditd_manage_rules
+bool auditd_purge_rules
+bool auditd_start_service
+string auditd_default_arch
+list auditd_rules
}
class Vars_main_yml {
+map _auditd_packages_map
+string auditd_config_directory
+string auditd_config_file
+string auditd_service
+map _auditd_permissions
+list __auditd_required_facts
+list __auditd_required_facts_subsets
+list __auditd_rh_distros
+list __auditd_rh_distros_fedora
+bool __auditd_is_rh_distro
+bool __auditd_is_rh_distro_fedora
}
class Tasks_main_yml {
+task Validate_role_parameters
+task Include_set_vars
+task Install_audit_packages
+task Deploy_auditd_configuration
+task Ensure_rulesd_directory
+task Purge_rulesd_when_requested
+task Deploy_custom_audit_rules
+task Start_and_enable_auditd_service
}
class Tasks_set_vars_yml {
+task Ensure_required_facts
+task Detect_ostree
+task Include_platform_vars_files
+task Resolve_package_names
+bool __auditd_is_ostree
+list __auditd_required_facts_subsets
+list __auditd_packages
}
class Tasks_assert_yml {
+assert auditd_num_logs_range
+assert auditd_freq_range
+assert_incremental_flush_requires_freq
+assert_priority_boost_range
+assert_q_depth_range
+assert_max_restarts_range
+assert_tcp_ports_ranges
+assert_tcp_client_ports_format
+assert_space_left_action_not_halt
+assert_disk_full_action_not_email
+assert_disk_error_action_not_email_rotate
+assert_exec_actions_have_paths
+assert_name_when_user_format
+assert_auditd_rules_structure
+assert_syscall_rule_keys
+assert_file_rule_permissions
}
class Tasks_purge_rules_yml {
+task Discover_rulesd_files
+task Stat_custom_rules
+task Slurp_custom_rules
+task Compute_expected_signature
+task Compute_current_signature
+task Decide_keep_custom_rules
+task Remove_rulesd_files
+bool __auditd_purge_keep_custom_rules
}
class Handlers_main_yml {
+handler Run_augenrules
+handler Load_audit_rules
+handler Restart_auditd
}
class Template_auditd_conf_j2 {
+macro auditd_yesno
+render auditd_conf_from_variables
}
class Template_custom_rules_j2 {
+render_global_settings
+render_file_watch_rules
+render_syscall_rules
+render_generic_rules
+use _auditd_permissions
}
class Meta_main_yml {
+string namespace
+string role_name
+string author
+string description
+string company
+string license
+string min_ansible_version
+list platforms
+list galaxy_tags
}
Role_linux_system_roles_auditd *-- Defaults_main_yml
Role_linux_system_roles_auditd *-- Vars_main_yml
Role_linux_system_roles_auditd *-- Tasks_main_yml
Role_linux_system_roles_auditd *-- Handlers_main_yml
Role_linux_system_roles_auditd *-- Template_auditd_conf_j2
Role_linux_system_roles_auditd *-- Template_custom_rules_j2
Role_linux_system_roles_auditd *-- Meta_main_yml
Tasks_main_yml --> Tasks_set_vars_yml
Tasks_main_yml --> Tasks_assert_yml
Tasks_main_yml --> Tasks_purge_rules_yml
Tasks_main_yml --> Handlers_main_yml
Tasks_purge_rules_yml --> Template_custom_rules_j2
Template_custom_rules_j2 --> Vars_main_yml
Template_auditd_conf_j2 --> Defaults_main_yml
Tasks_assert_yml --> Vars_main_yml
Tasks_set_vars_yml --> Vars_main_yml
Flow diagram for get_ostree_data_sh package resolution and outputflowchart TD
A[Start get_ostree_data_sh
args category pkgtype distro_ver format] --> B{Args valid?}
B -- no --> Z[Exit with_usage]
B -- yes --> C[Set category
Set pkgtypes
Parse distro and version]
C --> D{category == packages}
D -- no --> Z2[Unsupported_category
exit]
D -- yes --> E[Call get_packages
with ostree_dir]
subgraph get_packages_recursive
E --> F[For each pkgtype in pkgtypes]
F --> G[For each suffix in
"" -distro -distro-major -distro-ver]
G --> H[Check packages-pkgtype-suffix txt]
H --> I{File exists?}
I -- yes --> J[Output package lines]
I -- no --> K[Skip]
F --> L[Check roles-pkgtype txt]
L --> M{roles file exists?}
M -- no --> N[Skip roles]
M -- yes --> O[Read role names]
O --> P[For each role
call get_rolepath]
P --> Q{rolepath found?}
Q -- no --> R[Print error
exit 2]
Q -- yes --> S[Recurse get_packages
on rolepath]
J --> T[Collect all packages]
S --> T
T --> U[Sort -u packages]
end
U --> V{format}
V -- json --> W1[format_packages_json]
V -- yaml --> W2[format_packages_yaml]
V -- raw --> W3[format_packages_raw]
V -- toml --> W4[format_packages_toml]
W1 --> X[Print JSON list]
W2 --> X
W3 --> X
W4 --> X
X --> Y[End]
subgraph get_rolepath_resolution
GR1[Given ostree_dir and role] --> GR2[Check rolesdir under parent collection
role_path role ostree]
GR2 --> GR3{Exists?}
GR3 -- yes --> GR7[Return rolesdir]
GR3 -- no --> GR4[Check legacy role dirs
*-system-roles role
.ostree]
GR4 --> GR5{Exists?}
GR5 -- yes --> GR7
GR5 -- no --> GR6[Search ANSIBLE_COLLECTIONS_PATH
for role .ostree]
GR6 --> GR8{Found?}
GR8 -- yes --> GR7
GR8 -- no --> GR9[Print error
exit 2]
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 6 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path=".github/workflows/build_docs.yml" line_range="99-103" />
<code_context>
+ name: README.html
+ path: ${{ env.RELEASE_VERSION }}/README.html
+
+ - name: Commit changes
+ run: |
+ git config --global user.name "${{ github.actor }}"
+ git config --global user.email "${{ github.actor }}@users.noreply.github.com"
+ git add ${{ env.RELEASE_VERSION }}/README.html docs/index.html
+ git commit -m "Update README.html for ${{ env.RELEASE_VERSION }}"
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard adding docs/index.html so the workflow does not fail when RELEASE_VERSION != 'latest'
`docs/index.html` is only created when `RELEASE_VERSION == 'latest'`, but `git add ... docs/index.html` always runs. For tag-based releases, this file won’t exist and the `git add` will fail. Please either guard `git add docs/index.html` with `RELEASE_VERSION == 'latest'` or move it to a separate, conditional `git add` that matches the copy step’s condition.
</issue_to_address>
### Comment 2
<location path=".github/workflows/tft.yml" line_range="119-120" />
<code_context>
+ ansible_version: "2.17"
+ runs-on: ubuntu-latest
+ env:
+ ARTIFACTS_DIR_NAME: "tf_${{ github.event.repository.name }}-${{ github.event.issue.number }}_\
+ ${{ matrix.platform }}-${{ matrix.ansible_version }}_\
+ ${{ needs.prepare_vars.outputs.datetime }}/artifacts"
+ ARTIFACT_TARGET_DIR: /srv/pub/alt/${{ vars.SR_LSR_USER }}/logs
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid referencing a non-existent datetime output from prepare_vars in ARTIFACTS_DIR_NAME
`ARTIFACTS_DIR_NAME` uses `${{ needs.prepare_vars.outputs.datetime }}`, but `prepare_vars` doesn’t define a `datetime` output. That will leave an empty segment in the path and diverges from the `Set variables with DATETIME` step, which recomputes `DATETIME` and artifact paths. Please either expose `DATETIME` as an output from `prepare_vars` and use it here, or remove this interpolation and rely on the value computed in `set_vars` only.
Suggested implementation:
```
env:
ARTIFACT_TARGET_DIR: /srv/pub/alt/${{ vars.SR_LSR_USER }}/logs
```
If subsequent steps rely on `ARTIFACTS_DIR_NAME` as a job-level environment variable, they should be updated to either:
1. Use the value computed in `set_vars` (e.g., by echoing it to `$GITHUB_OUTPUT` in `set_vars` and then referencing `steps.set_vars.outputs.ARTIFACTS_DIR_NAME`), or
2. Recompute the same value locally in their own `run` sections, mirroring the logic from `set_vars`.
This will ensure there is a single source of truth for the artifacts directory name and no dependency on a non-existent `prepare_vars.outputs.datetime`.
</issue_to_address>
### Comment 3
<location path=".ostree/get_ostree_data.sh" line_range="132" />
<code_context>
+ exit 1
+fi
+
+"get_$category" "$ostree_dir" | "format_${category}_$format"
</code_context>
<issue_to_address>
**suggestion:** Validate category/format values before invoking helper functions to avoid confusing runtime errors
Right now the script assumes any `category`/`format` is valid and directly invokes `get_$category` and `format_${category}_$format`. If a caller passes an unsupported value (e.g., a typo), this will just produce a `command not found` error. Please add explicit validation (e.g., a case statement) for allowed categories and formats, and exit with a clear, actionable error when an invalid value is given.
</issue_to_address>
### Comment 4
<location path="contributing.md" line_range="14-15" />
<code_context>
+* Basic git and github information
+* How to create git commits and submit pull requests
+
+**Bugs and needed implementations** are listed on
+[Github Issues](https://github.com/linux-system-roles/auditd/issues).
+Issues labeled with
+[**help wanted**](https://github.com/linux-system-roles/auditd/issues?q=is%3Aissue+is%3Aopen+label%3A%22help+wanted%22)
</code_context>
<issue_to_address>
**nitpick (typo):** Use the correct "GitHub" capitalization in the link text.
Please update the link text to "GitHub Issues" to match the official capitalization and keep the terminology consistent.
Suggested implementation:
```
* Basic git and GitHub information
```
```
[GitHub Issues](https://github.com/linux-system-roles/auditd/issues).
```
```
**Code** is managed on [GitHub](https://github.com/linux-system-roles/auditd), using
```
</issue_to_address>
### Comment 5
<location path="plans/README-plans.md" line_range="3" />
<code_context>
+# Introduction CI Testing Plans
+
+Linux System Roles CI runs [tmt](https://tmt.readthedocs.io/en/stable/index.html) test plans in [Testing farm](https://docs.testing-farm.io/Testing%20Farm/0.1/index.html) with the [tft.yml](https://github.com/linux-system-roles/auditd/blob/main/.github/workflows/tft.yml) GitHub workflow.
+
+The `plans/test_playbooks_parallel.fmf` plan is a test plan that runs test playbooks in parallel on multiple managed nodes.
</code_context>
<issue_to_address>
**nitpick (typo):** Align "Testing farm" capitalization with the official "Testing Farm" name.
Please update the link text here to "Testing Farm" to match the official name used elsewhere in the docs.
```suggestion
Linux System Roles CI runs [tmt](https://tmt.readthedocs.io/en/stable/index.html) test plans in [Testing Farm](https://docs.testing-farm.io/Testing%20Farm/0.1/index.html) with the [tft.yml](https://github.com/linux-system-roles/auditd/blob/main/.github/workflows/tft.yml) GitHub workflow.
```
</issue_to_address>
### Comment 6
<location path="plans/README-plans.md" line_range="20" />
<code_context>
+3. For the given role and the given PR, runs the general test from [test.sh](https://github.com/linux-system-roles/tft-tests/blob/main/tests/general/test.sh).
+
+The [tft.yml](https://github.com/linux-system-roles/auditd/blob/main/.github/workflows/tft.yml) workflow runs the above plan and uploads the results to our Fedora storage for public access.
+This workflow uses Testing Farm's Github Action [Schedule tests on Testing Farm](https://github.com/marketplace/actions/schedule-tests-on-testing-farm).
+
+## Running Tests
</code_context>
<issue_to_address>
**nitpick (typo):** Use the proper "GitHub Action" capitalization.
Please update "Github Action" to "GitHub Action" in this sentence for correct branding and consistency with the official name.
```suggestion
This workflow uses Testing Farm's GitHub Action [Schedule tests on Testing Farm](https://github.com/marketplace/actions/schedule-tests-on-testing-farm).
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| - name: Commit changes | ||
| run: | | ||
| git config --global user.name "${{ github.actor }}" | ||
| git config --global user.email "${{ github.actor }}@users.noreply.github.com" | ||
| git add ${{ env.RELEASE_VERSION }}/README.html docs/index.html |
There was a problem hiding this comment.
issue (bug_risk): Guard adding docs/index.html so the workflow does not fail when RELEASE_VERSION != 'latest'
docs/index.html is only created when RELEASE_VERSION == 'latest', but git add ... docs/index.html always runs. For tag-based releases, this file won’t exist and the git add will fail. Please either guard git add docs/index.html with RELEASE_VERSION == 'latest' or move it to a separate, conditional git add that matches the copy step’s condition.
| ARTIFACTS_DIR_NAME: "tf_${{ github.event.repository.name }}-${{ github.event.issue.number }}_\ | ||
| ${{ matrix.platform }}-${{ matrix.ansible_version }}_\ |
There was a problem hiding this comment.
suggestion (bug_risk): Avoid referencing a non-existent datetime output from prepare_vars in ARTIFACTS_DIR_NAME
ARTIFACTS_DIR_NAME uses ${{ needs.prepare_vars.outputs.datetime }}, but prepare_vars doesn’t define a datetime output. That will leave an empty segment in the path and diverges from the Set variables with DATETIME step, which recomputes DATETIME and artifact paths. Please either expose DATETIME as an output from prepare_vars and use it here, or remove this interpolation and rely on the value computed in set_vars only.
Suggested implementation:
env:
ARTIFACT_TARGET_DIR: /srv/pub/alt/${{ vars.SR_LSR_USER }}/logs
If subsequent steps rely on ARTIFACTS_DIR_NAME as a job-level environment variable, they should be updated to either:
- Use the value computed in
set_vars(e.g., by echoing it to$GITHUB_OUTPUTinset_varsand then referencingsteps.set_vars.outputs.ARTIFACTS_DIR_NAME), or - Recompute the same value locally in their own
runsections, mirroring the logic fromset_vars.
This will ensure there is a single source of truth for the artifacts directory name and no dependency on a non-existent prepare_vars.outputs.datetime.
| exit 1 | ||
| fi | ||
|
|
||
| "get_$category" "$ostree_dir" | "format_${category}_$format" |
There was a problem hiding this comment.
suggestion: Validate category/format values before invoking helper functions to avoid confusing runtime errors
Right now the script assumes any category/format is valid and directly invokes get_$category and format_${category}_$format. If a caller passes an unsupported value (e.g., a typo), this will just produce a command not found error. Please add explicit validation (e.g., a case statement) for allowed categories and formats, and exit with a clear, actionable error when an invalid value is given.
| **Bugs and needed implementations** are listed on | ||
| [Github Issues](https://github.com/linux-system-roles/auditd/issues). |
There was a problem hiding this comment.
nitpick (typo): Use the correct "GitHub" capitalization in the link text.
Please update the link text to "GitHub Issues" to match the official capitalization and keep the terminology consistent.
Suggested implementation:
* Basic git and GitHub information
[GitHub Issues](https://github.com/linux-system-roles/auditd/issues).
**Code** is managed on [GitHub](https://github.com/linux-system-roles/auditd), using
| @@ -0,0 +1,55 @@ | |||
| # Introduction CI Testing Plans | |||
|
|
|||
| Linux System Roles CI runs [tmt](https://tmt.readthedocs.io/en/stable/index.html) test plans in [Testing farm](https://docs.testing-farm.io/Testing%20Farm/0.1/index.html) with the [tft.yml](https://github.com/linux-system-roles/auditd/blob/main/.github/workflows/tft.yml) GitHub workflow. | |||
There was a problem hiding this comment.
nitpick (typo): Align "Testing farm" capitalization with the official "Testing Farm" name.
Please update the link text here to "Testing Farm" to match the official name used elsewhere in the docs.
| Linux System Roles CI runs [tmt](https://tmt.readthedocs.io/en/stable/index.html) test plans in [Testing farm](https://docs.testing-farm.io/Testing%20Farm/0.1/index.html) with the [tft.yml](https://github.com/linux-system-roles/auditd/blob/main/.github/workflows/tft.yml) GitHub workflow. | |
| Linux System Roles CI runs [tmt](https://tmt.readthedocs.io/en/stable/index.html) test plans in [Testing Farm](https://docs.testing-farm.io/Testing%20Farm/0.1/index.html) with the [tft.yml](https://github.com/linux-system-roles/auditd/blob/main/.github/workflows/tft.yml) GitHub workflow. |
| 3. For the given role and the given PR, runs the general test from [test.sh](https://github.com/linux-system-roles/tft-tests/blob/main/tests/general/test.sh). | ||
|
|
||
| The [tft.yml](https://github.com/linux-system-roles/auditd/blob/main/.github/workflows/tft.yml) workflow runs the above plan and uploads the results to our Fedora storage for public access. | ||
| This workflow uses Testing Farm's Github Action [Schedule tests on Testing Farm](https://github.com/marketplace/actions/schedule-tests-on-testing-farm). |
There was a problem hiding this comment.
nitpick (typo): Use the proper "GitHub Action" capitalization.
Please update "Github Action" to "GitHub Action" in this sentence for correct branding and consistency with the official name.
| This workflow uses Testing Farm's Github Action [Schedule tests on Testing Farm](https://github.com/marketplace/actions/schedule-tests-on-testing-farm). | |
| This workflow uses Testing Farm's GitHub Action [Schedule tests on Testing Farm](https://github.com/marketplace/actions/schedule-tests-on-testing-farm). |
|
[citest] |
spetrosi
left a comment
spetrosi
left a comment
There was a problem hiding this comment.
Looks very good. Did you mention where this role was taken from?
| # SPDX-License-Identifier: MIT | ||
| --- | ||
| - name: Validate role parameters | ||
| ansible.builtin.import_tasks: |
There was a problem hiding this comment.
Why import not include? Please add a comment if there is a reason.
There was a problem hiding this comment.
Not sure - this is copied from ansible-role-auditd
| - name: Validate role parameters | ||
| ansible.builtin.import_tasks: | ||
| file: assert.yml | ||
| run_once: true |
There was a problem hiding this comment.
Different hosts might have different variables. Makes sense to me to run this without delegate_to and run_once, so run as a regular task against all hosts.
There was a problem hiding this comment.
Yeah - not sure - this is copied from ansible-role-auditd
There was a problem hiding this comment.
I suggest calling this file assert_role_vars for clarity.
| ansible.builtin.assert: | ||
| that: | ||
| - auditd_priority_boost | int >= 0 | ||
| - auditd_priority_boost | int <= 2147483647 |
There was a problem hiding this comment.
Some assert tasks that check int ranges do not have failed_msg, but I guess this is pretty self-explanatory and doesn't require a dedicated message.
There was a problem hiding this comment.
The default message should be informative enough
| {% endif %} | ||
| {% endfor %} | ||
| {% endif %} |
There was a problem hiding this comment.
Here again might need some -%} otherwise jinja adds many newlines for each item in the loop. Try running it with more auditd_rules and look at the resulting file.
There was a problem hiding this comment.
hm - the code looks good as-is - here is the output when there are multiple filters in the for loop:
-w /etc/hosts -p rw -k lsr_auditd_integration
-w /etc/group -F auid>=1000 -F guid>=1000 -F name=root -F name=bin -F name=daemon -F name=sys -F name=adm -F name=lp -F name=sync -F name=shutdown -F name=halt -p r -k lsr_auditd_group_filtered
-a always,exit -F arch=b64 -S adjtimex -k lsr_auditd_syscall_adjtimex
| by `meta/argument_specs.yml` and `tasks/assert.yml` using the same limits as the | ||
| audit-userspace parsers (for example `num_logs` ≤ 999). | ||
|
|
||
| ### auditd_local_events |
There was a problem hiding this comment.
It would be clearer and more redable to place Type and Default value for each variable on a separate line. Now each var's description is one long line that's hard to process.
A not for future, maybe when we introduce argument_spec in all roles, we can make it the source of truth, and generate variables section in README from info in argument_spec.
There was a problem hiding this comment.
It would be clearer and more redable to place Type and Default value for each variable on a separate line. Now each var's description is one long line that's hard to process.
ok
A not for future, maybe when we introduce argument_spec in all roles, we can make it the source of truth, and generate variables section in README from info in argument_spec.
Yes, good idea
In the README.md - "This role is heavily based on ansible-role-auditd" |
richm
force-pushed
the
initial_version
branch
3 times, most recently
from
930725f to
8862d20
Compare
|
[citest] |
| that: | ||
| - auditd_freq | int >= 0 | ||
| - auditd_freq | int <= 2147483647 | ||
| fail_msg: auditd_freq must be between 0 and INT_MAX |
There was a problem hiding this comment.
https://docs.ansible.com/projects/ansible/latest/dev_guide/developing_program_flow_modules.html#argument-spec
this link documents the use of the following keywords, we maybe we can make use of them to replace these checks.
mutually_exclusive:
required_together:
required_one_of:
required_if:
required_by:
There was a problem hiding this comment.
I tried this - I tried adding:
required_if:
- [auditd_max_log_file_action, exec, [auditd_max_log_file_action_exe], true]My editor flags this as Property required_if is not allowed.yaml-schema: Entry Point(513)
At runtime - it appears this is ignored - ansible doesn't issue a warning or an error - but it doesn't work - in tests_invalid_input.yml the implicit argument validation is not hit, it falls through to the explicit check Assert exec companion paths when action is exec
Looking at the role argument spec page - https://docs.ansible.com/projects/ansible/latest/playbook_guide/playbooks_reuse_roles.html#specification-format - it doesn't list those options, nor does it say that you can also use the module argument specs
So I would say we can't do this and we are limited to whatever is specified at https://docs.ansible.com/projects/ansible/latest/playbook_guide/playbooks_reuse_roles.html#specification-format
|
Right now the role defaults come from clear_config() in auditd-config.c, but users get Some of the defaults that don't match:
The shipped config uses uppercase keywords ( |
Are these values different on different versions of EL and Fedora? We are targeting EL8 and later. Are the values on EL8 different than EL9 and EL10 and Fedora? Are they different on different minor versions e.g. between EL10.0 and EL10.2?
I agree, but it may be difficult depending on how different the values are between major and minor versions of the OS.
|
Looking at the code - https://github.com/linux-audit/audit-userspace/blob/master/src/auditd-config.c#L1677-L1699 - setting if |
|
[citest] |
|
Next round of findings, this time focused on the rules-level settings:
|
| # Each block runs include_role expecting failure; rescue records that outcome. | ||
| - name: Verify invalid parameters are rejected | ||
| hosts: all | ||
| gather_facts: true |
There was a problem hiding this comment.
| gather_facts: true | |
| gather_facts: false |
What should be the default value for
Make the default value
Ok. I also see from the auditctl man page that a rule can have multiple keynames? Also, when to use
The man pages for audit.rules and auditctl strongly suggest adding
Alternately - we get rid of Should we allow the user to specify multiple arch in a rule? e.g. - action: always
filter: exit
arch: [b32, b64]
....which will generate both the b32 and b64 version of the rule? Also - when to use
OK - see above? Additional questions: -a action,list (or list,action)
-w, -F path=, -F dir=, -q, -p, -F perm=, -t
-SLooks like this should be a list instead of a single value: - action: always
filter: exit
syscall: [accept,connect]and that can be written either as -F exe=Should we make this a separate key |
Q1: --backlog_wait_time default and omit valueThe default should be Q2: auditd_maximum_rate default and 0 means omitDo we really need to provide defaults for all of these rule-level settings? The shipped rules don't set Q3: keyname — -k vs -F key=, multiple keys
Q4: auditd_default_arch — how to handle archI'm not aware of any other option for specifying arch besides Q5: -a action,list questionsYes, we should validate action and list values — the valid actions are Q6: -w, -F path=, -F dir=, -q, -p, -F perm=, -tThe role should use the syscall form exclusively — Q7: -S syscall — list vs single valueYes, Q8: -F exe= — separate key or filterKeep it in the filter list. In the shipped rules, |
ok - I guess we'll have to use
Every parameter that can be set by the user must be declared in defaults/main.yml and must have some sort of default value. However, the default can be
We can't use
Yes, it will just take some additional validation logic.
ok - I don't think I will add validation for total keyname lengths < 256 unless that is a bad failure case.
ok.
ok. I'll add a check for valid action and list. I will leave the naming for
ok I assume that having a single rule with both
ok
ok |
We discussed setting to null vs setting to an empty line. And you and Peter were for the empty line, and I eventually like empty line more than null. For lists and dicts empty would be [] and {}. |
Ok |
|
Thinking more about this, empty strings can be ambiguous in the cases when empty value is a proper value too. In leapp it was about Satellite Orgs that cannot be empty, so empty definitely means omit. But there may be cases when empty string value might be a value different from omit. |
|
Consider an option where empty string is a real value. I use the role to set the value to |
I think
We use |
I guess this is the same as using |
I guess not - looks like some of them can begin with |
|
[citest] |
|
@spetrosi @Cropi I guess start with the README.md to see the rule changes. I'm still not happy with the rule keywords - I'm using I got the rule key/value combinations using the feedback from @Cropi and reading the audit.rules and auditctl man pages. |
|
[citest] |
Generally, yes except for the legacy
Yes.
Almost. Syscall rules can also use io_uring as a filter (e.g. -a always,io_uring ...). io_uring is relatively new enhancement in the kernel, it's not present in rhel8.
If a user specifies a path/dir rule with explicit syscalls (-S), omitting perm is fine and doesn't need a warning. Btw, adding perm=w internally maps to ~20 syscalls different syscalls. and checking that it would be internally turned into |
Not sure what you mean - we generate the entire custom.rules file every time, in the order given by the user. Is there a case where we would generate a file like this?
Ok. I might need to add a conditional for that for EL8.
ok - so allow specifying path/dir with either perm or syscall with no warning |
I keep forgetting this is not a wrapper for
|
|
[citest] |
|
[citest] |
Cropi
left a comment
Cropi
left a comment
There was a problem hiding this comment.
In assert_role_vars.yml, the check for tcp_max_per_addr allows zero, but auditd itself rejects anything below 1 — so the role would happily write a config that auditd refuses to load. The lower bound should be 1.
Also, auditd_enable_flag, auditd_fail_mode, and auditd_buffer_size go straight into the rules file without any range validation. The first two only make sense as 0, 1, or 2, and buffer size needs to be positive.
These are small changes though, can be addressed in a follow-up.
Done. |
|
[citest] |
richm
force-pushed
the
initial_version
branch
2 times, most recently
from
2386b0e to
1186a27
Compare
|
[citest] |
|
[citest_bad] |
| - auditd_freq | int <= 2147483647 | ||
| fail_msg: auditd_freq must be between 0 and INT_MAX | ||
|
|
||
| - name: Assert incremental flush requires non-zero freq (sanity_check) |
There was a problem hiding this comment.
You can indeed replace sanity here
This is the initial commit of the new role auditd. Signed-off-by: Rich Megginson <rmeggins@redhat.com>
|
[citest] |
3 participants
This is the initial commit of the new role auditd.
Signed-off-by: Rich Megginson rmeggins@redhat.com
Summary by Sourcery
Introduce a new linux-system-roles.auditd Ansible role to manage auditd configuration and rules with comprehensive CI, docs, and testing support.
New Features:
Enhancements:
CI:
Documentation:
Tests:
Chores: