ansible-lockdown / UBUNTU22-CIS

Ansible role for Ubuntu22 CIS Baseline
https://ansible-lockdown.readthedocs.io/en/latest/
MIT License
181 stars 80 forks source link

Enabling the user to chose between complain and enforce mode in 1.6.1.3. #94

Closed bgro closed 11 months ago

bgro commented 1 year ago

Overall Review of Changes: Control 1.6.1.3 mandates to Ensure all AppArmor Profiles are in enforce or complain mode. However, the corresponding task only allows the role to set every profile to enforce mode -- the existing toggle in defaults/main.yml disables the tasks rather than switching between enforce and complain mode.

This fix changes the implementation such that complain or enforce mode can be chosen.

Issue Fixes:

bgro commented 1 year ago

Hold on, I overlooked the dependency with 1.6.1.4.

uk-bolly commented 1 year ago

Hi @bgro

Superb PR and work thank you so much. I am currently testing locally, however i have noticed it will fail yamllint checks. The default is set to 4 spaces (being consistent also helps the ansible-galaxy score). I have run it to feedback. If you could amend accordingly that would be excellent.

tasks/prelim.yml
  4:5       error    wrong indentation: expected 6 but found 4  (indentation)
  8:1       error    too many blank lines (2 > 1)  (empty-lines)

tasks/section_1/cis_1.6.x.yml
  69:11     error    wrong indentation: expected 12 but found 10  (indentation)
  111:11    error    wrong indentation: expected 12 but found 10  (indentation)
  114:11    error    wrong indentation: expected 12 but found 10  (indentation)
  117:11    error    wrong indentation: expected 12 but found 10  (indentation)
  120:11    error    wrong indentation: expected 12 but found 10  (indentation)
  154:1     error    too many blank lines (1 > 0)  (empty-lines)

many thanks

uk-bolly

uk-bolly commented 1 year ago

hi @bgro

To get around enforce always benig set even on level 2 could i suggest that

- ubtu22cis_apparmor_mode == "enforce"

is added to line 98, that would get around it forcing something not requested in error.

Again great work.

Thanks

uk-bolly

bgro commented 1 year ago

Hi @uk-bolly,

I forgot about linting, sorry!

Regarding your suggestion to add

- ubtu22cis_apparmor_mode == "enforce"

I think I understand that you want this, because the control can really be invasive and break stuff, and therefore you want an extra variable to disable or at least defang rule 1.6.1.4, yes?

That would lead to either nothing being done, if 1.6.1.3 is not part of what needs to be executed (e.g., wenn only executing L2 rules) or 1.6.1.3 being executed instead...

I think that is not really ideal. Probably the easiest would be make 1.6.1.4 to also use ubtu22cis_apparmor_mode for implementing, but not for checking (and document accordingly, i.e., have

ansible.builtin.shell: aa-{{ubtu22_cis_apparmor_mode}} /etc/apparmor.d/*

in line 80.

What do you think about that?

Update: Ah, there are, of course no AUDITs performed by this rule, the other stuff is about idempotency. OK, how about making 1.6.1.3 almost the same as 1.6.1.4, but in case complain is chosen, add a warning regarding non-compliance with 1.6.1.4?

uk-bolly commented 12 months ago

Hi @uk-bolly,

I forgot about linting, sorry!

Regarding your suggestion to add

- ubtu22cis_apparmor_mode == "enforce"

I think I understand that you want this, because the control can really be invasive and break stuff, and therefore you want an extra variable to disable or at least defang rule 1.6.1.4, yes?

That would lead to either nothing being done, if 1.6.1.3 is not part of what needs to be executed (e.g., wenn only executing L2 rules) or 1.6.1.3 being executed instead...

I think that is not really ideal. Probably the easiest would be make 1.6.1.4 to also use ubtu22cis_apparmor_mode for implementing, but not for checking (and document accordingly, i.e., have

ansible.builtin.shell: aa-{{ubtu22_cis_apparmor_mode}} /etc/apparmor.d/*

in line 80.

What do you think about that?

Update: Ah, there are, of course no AUDITs performed by this rule, the other stuff is about idempotency. OK, how about making 1.6.1.3 almost the same as 1.6.1.4, but in case complain is chosen, add a warning regarding non-compliance with 1.6.1.4?

hi @bgro

Been looking and thinking about this one for a while. Ideally we should only do what the control asks for. Like the rest of the tasks they should only run if required. i am happy to skip 1.6.1.3 if 1.6.1.4 is set and running. We should set a warning though if only level 1 is run. This could get a little more complicated still. Let me discuss this with the team and see what othewr ideas we can come up with. Many thanks

uk-bolly

uk-bolly commented 11 months ago

hi @bgro

Thank you for this PR as always. I can see it is still failing on pre-commit checks, if you are able to resolve this would like to get this merged.

Many thanks

uk-bolly

bgro commented 11 months ago

HI @uk-bolly, I made a new PR https://github.com/ansible-lockdown/UBUNTU22-CIS/pull/148 (I was not quite sure about updating the old one with rebase and everything). The syntax issues are gone, but the pipeline now terminates with an error for what seems like some infrastructure issue. What do I need to do to re-run the pipeline?

uk-bolly commented 11 months ago

hi @bgro

i think we are all sorted now? I believe this PR can be closed now?

many thanks once again for all of your help.

uk-bolly

uk-bolly commented 11 months ago

closing as replaced by #148