Allow partial override of the `ubtu22cis_sshd` struct

rostskadat commented 7 months ago

Overall Review of Changes: My use case is simple: When configuring SSHD, I need to set AllowUsers to a specific value, while leaving the rest of the configuration for SSHD as it is. More specifically I want to keep using the default ciphers (macs, kex_algorithms, etc.) as recommended by CIS. I was only able to do that if I defined in my own variable the whole structure ubtu22cis_sshd as defined in defaults/main.yml which is really cumbersome. It would be more user friendly if I was able to overwrite just on specific settings in my variable and use the recommended values for the rest of the settings, like this:

---
- name: ubuntu22-base
  hosts: all
  become: yes
  vars:
    # SSHD configuration, only overriding the allow_users, and keep the rest to its recommended values.
    ubtu22cis_sshd:
      allow_users: "{{ansible_user}}"
  roles:
    - mindpointgroup.ubuntu22_cis

Issue Fixes: NA

Enhancements: NA

How has this been tested?: Tested on a pristine VirtualBox VM straight from installation.

ansible [core 2.15.6]
  config file = /etc/ansible/ansible.cfg
  configured module search path = ['~/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python3/dist-packages/ansible
  ansible collection location = ~/.ansible/collections:/usr/share/ansible/collections
  executable location = /usr/bin/ansible
  python version = 3.10.12 (main, Jun 11 2023, 05:26:28) [GCC 11.4.0] (/usr/bin/python3)
  jinja version = 3.0.3
  libyaml = True

rostskadat commented 7 months ago

pre-commit.ci autofix

rostskadat commented 7 months ago

BTW, this whole PR might be simply solved by making sure that the AllowUsers contains not only vagrant and ubuntu, but also {{ansible_user}}

georgenalen commented 7 months ago

Hey @rostskadat, Thank you for opening a PR and helping with the project, community input/support/help is always greatly appreciated and encouraged. I'm looking at this PR and I might need some more explanation of the issue it is solving. You mention the issue is with defining the allow users and keeping the rest of the sshd variables set as they are in defaults/main.yml. The allow users variable is its own variable, ubtu22cis_sshd. allow_users. If you use the extra vars pretty anywhere you can set variables that will override the default value(s) we have in defaults/main.yml. So you can set ubtu22cis_sshd. allow_users to whatever you need it to be in -e at run time, host/group vars in inventory (setting on inventory is the preferred way), extra vars in templates/projects in AAP/Tower/AWX, etc. to have that newly defined value used where the allow users are applied. The defaults/main.yml is generally not intended to be modified, the goal is to modify the values by overriding via extra var areas. Since this adds a fair bit of complexity to the var values I want to make sure I fully understand the situation being addressed.

The other issues is can you remove the .editorconfig file?

rostskadat commented 7 months ago

Hi @georgenalen ,

Thanks for your reply. The idea behind the PR was to be able to override just one part of the ubtu22cis_sshd structure (in this case the allow_users attribute) leaving the rest as it is defined in the role. However I've had trouble doing that, and it might be to my limited experience with all the different ways to set facts in Ansible... From your comment I understand that it should be possible to override just one of the ubtu22cis_sshd attributes leaving the rest to their default value. Allow me to boil down the playbook to its minimal form:

---
- hosts: all
  vars:
    ubtu22cis_sshd: 
      allow_users: "ubuntu"
      other_key: "default_value"
  tasks:
    - debug:
        msg: "{{ubtu22cis_sshd}}"

with the following inventory

---
all:
  hosts:
    target:
      ubtu22cis_sshd.allow_users: admin

When running this I still get ubtu22cis_sshd.allow_users=ubuntu
When running with the -e ubtu22cis_sshd.allow_users=admin on the command line I still get ubtu22cis_sshd.allow_users=ubuntu

My question is whether I understood correctly your suggestion.

As for the .editorconfig, it has been removed.

joshavant commented 6 months ago

Just wanted to add a +1 for this exact scenario and PR solution.

In my project, I'm customizing UBUNTU22-CIS using the following pattern:

role_vars/ubuntu22_cis.yml

---
ubtu22cis_purge_apt: true
ubtu22cis_time_sync_tool: "chrony"
# ...

Playbook: some_server.yml

---
- hosts: target
  become: yes
  vars_files:
    - role_vars/ubuntu22_cis.yml
  roles:
    - role: MindPointGroup.ubuntu22_cis
      tags:
        - level1-server
        - level2-server

(This does NOT work if, e.g., I add ubtu22cis_sshd.log_level = "VERBOSE" to role_vars/ubuntu22_cis.yml, as I believe you were suggesting in your comment here.)

Otherwise, I'm customizing many other, directly assigned variables using this pattern and it's working great for my needs.

But because ubtu22cis_sshd is a dictionary structure instead of individual variables, I'm unable to use this pattern for it.

I took a look at the PR and it would seem to solve my needs here, too. If there's anything I can do to help this PR along, I'd be glad to lend the help!

EDIT - I have also encountered the same issue with the ubtu22cis_auditd dictionary.

uk-bolly commented 6 months ago

hi @rostskadat

Thank you for taking the time to raise this PR and for the feedback you have participated in. While it is possible to override a nested variable it requires you to add all of the nested options or to run just that one control with a tag, which doesnt really work or scaleable.

I am happy to take this PR although we need to resolve a couple of issues.

The DCO is missing, it is not gpg signed commited on one the previous commits so it failing
The pipeline is failing, it appears for some reason to be picking up older code for the prelim step, could you possible see if you update your devel branch with the latest to see if that resolves the issue.

Alternatively, i can take the work and add the updates for the audit sections also as mentioned by @joshavant and give credits once updated?

Many thanks

uk-bolly

rostskadat commented 6 months ago

Hello @uk-bolly,

Thanks for your feedback. I fixed both problems. Please let me know if there is anything missing.

Regards

uk-bolly commented 5 months ago

hi @rostskadat

Thank you for the quick turnaround. All looks good in the pipeline, the final commit is gpg signed, but it is still not happy with the signing. You will see that the merge is still blocked as not all commits are signed.

Many thanks

uk-bolly

rostskadat commented 5 months ago

Hello @uk-bolly

Sorry to bother you about that, but now I'm confused on how to proceed. Allow me to give you a summary of the situation. As you mentioned the last commit (aa21b5b3 a merge commit) was not signed off. I tried to rebase the history of my rostskadat:devel branch by signing off the last commit with git rebase --signoff HEAD~1. However, suddenly, all commit between HEAD of what I suspect is the ansible-lockdown:devel branch and the commit I was trying to sign off (aa21b5b3) have been signed off. Which might be more sign off than I might want to do.

I'm not really sure how to proceed and I would really appreciate if you could give me an indication on how to solve this issue.

Regards

joshavant commented 5 months ago

@rostskadat

Just taking a guess here. git commit --amend --no-edit -S <commit hash> should allow you to amend a signature to a previous commit.

Since there's only 10 commits in this PR, maybe it wouldn't be too difficult to iterate individually through all 10 commit hashes using the above command to sign them?

joshavant commented 5 months ago

Also, just a heads up that it seems like your key might be expired (note the blue 'Expired' bubble beneath the key hash).

You may need to address that prior to actually signing the commits.

rostskadat commented 5 months ago

@uk-bolly @joshavant sorry for the late answer. Just a quick question to check that I understood properly what you are requesting. You want the merge commit (aa21b5b38bc13da89e0d88941e6df2f8250163c9) to also be signed off by me, correct?

joshavant commented 5 months ago

On a recent PR of my own, it seemed like the CI system requires all of the following:

GPG signed commits
Sign-off commits

rostskadat commented 5 months ago

@joshavant unless I'm mistaken the commit is a merge commit from the Github Merge UI process, and therefore signed by Github's own key. Please do correct me if I'm mistaken...

joshavant commented 5 months ago

@uk-bolly is probably the best person to lead this conversation, as they're the maintainer of the CI system.

uk-bolly commented 2 months ago

hi @rostskadat and @joshavant

Thank you for your patience on this, Probably due to the number of changes that have taken place since this was raised i maybe best for me to add to a new branch to get this into production. Credits will be given but should get around any possible issues that now may arise. Apologies it has taken as long as it has, client requests take priority so we have been focusing on those first. I hope to get this through into devel later today once i've tested with all the new commits.

Many thanks

uk-bolly

uk-bolly commented 11 hours ago

hi @rostskadat and @joshavant

Thank you for the time and conversations on this thread, this was merged a while back and i assume it is as expected. I will therefore close this PR (Note that v2.0.0 has been released to subscribers and will be available in the next few of months). Many thanks once again

uk-bolly

ansible-lockdown / UBUNTU22-CIS

Allow partial override of the `ubtu22cis_sshd` struct #175