Skip to content
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

Rewrite system account detection and hardening and create tests #621

Merged
merged 8 commits into from
Jan 27, 2023

Conversation

rndmh3ro
Copy link
Member

@DonEstefan I messed up your PR when trying to resolve merge conflicts.

Can you please either:

  • do a fource push from your local branch (system_account_hardeining_rewrite) to github to get the changes back or
  • look a this PR and check if all changes are there (I think so)

@DonEstefan
Copy link
Contributor

I'll try to find some time later this week to look into it. I know I had a hard time splitting up the initial pull request into multiple smaller requests. It makes sense that re-uniting them is just as complicated 😋

Signed-off-by: Sebastian Gumprich <sebastian.gumprich@t-systems.com>
@DonEstefan
Copy link
Contributor

DonEstefan commented Jan 23, 2023

I lost the content of the branch/pullrequest that kept the sys accounts changes separate from the other stuff. But I still have a copy of the final user_accounts.yml file, that has all the changes that I tried to split in separate pull requests (user accounts + system accounts + root accounts).

It seems like you undid some of the inital changes during merge of master branch.

From what I can see, I think by now you can remove the 5 plays below from the file. "uid_min" and "uid_max" should not be used any longer. Instead we now rely on os_auth_sys_uid_max/os_auth_uid_min/os_auth_uid_max from the vars file.

- name: Get UID_MIN from login.defs
  shell: awk '/^\s*UID_MIN\s*([0-9]*).*?$/ {print $2}' /etc/login.defs
  args:
    removes: /etc/login.defs
  register: uid_min
  check_mode: false
  changed_when: false

- name: Calculate UID_MAX from UID_MIN by substracting 1
  set_fact:
    uid_max: '{{ uid_min.stdout | int - 1 }}'
  when: uid_min.stdout|int > 0

- name: Set UID_MAX on Debian-systems if no login.defs exist
  set_fact:
    uid_max: '999'
  when:
    - ansible_facts.os_family == 'Debian'
    - uid_max is not defined

- name: Set UID_MAX on other systems if no login.defs exist
  set_fact:
    uid_max: '499'
  when: uid_max is not defined

- name: Get all system accounts
  command: awk -F'':'' '{ if ( $3 <= {{ uid_max | quote }} ) print $1}' /etc/passwd
  args:
    removes: /etc/passwd
  changed_when: false
  check_mode: false
  register: sys_accs

@DonEstefan
Copy link
Contributor

You also might want to consider splitting this:

- name: Remove shell+password for linux system accounts
  user:
    name: '{{ item }}'
    shell: '{{ os_nologin_shell_path }}'
    password: '*'
    createhome: false
  loop: "{{ system_users }}"

in 2 separate tasks, to make detection of existing password locks more reliable

- name: remove shell for linux system accounts
  user:
    name: '{{ item }}'
    shell: '{{ os_nologin_shell_path }}'
    createhome: false
  loop: "{{ system_users }}"

- name: lock passwords from linux system accounts
  user:
    name: '{{ item }}'
    password: '*'
    createhome: false
  loop: "{{ system_users }}"
  when:
    - getent_shadow[item][0] is not match("\!") # password hashes containing illegal characters like "!" are unusable already (locked)

Sorry, for texting this, but I don't have editing rights in this pull request.

@DonEstefan
Copy link
Contributor

I rebased to your current master branch and force-pushed my changes to master...DonEstefan:ansible-collection-hardening:system_account_hardeining_rewrite
I hope this helps.

Signed-off-by: Sebastian Gumprich <sebastian.gumprich@t-systems.com>
@rndmh3ro rndmh3ro changed the title Sys accounts Rewrite system account detection and hardening and create tests Jan 24, 2023
Copy link
Contributor

@schurzi schurzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great cleanup, Just the minor change in the name of the test prepare task and I also like the suggestion from @DonEstefan to split up the setting of shell and password.

molecule/os_hardening/prepare.yml Outdated Show resolved Hide resolved
Signed-off-by: Martin Schurz <Martin.Schurz@t-systems.com>
Signed-off-by: Martin Schurz <Martin.Schurz@t-systems.com>
Signed-off-by: Sebastian Gumprich <sebastian.gumprich@t-systems.com>
@rndmh3ro rndmh3ro merged commit 89138be into master Jan 27, 2023
@rndmh3ro rndmh3ro deleted the sys_accounts branch January 27, 2023 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants