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

fix(R5.4.3). Correct regexes so that they match #98

Conversation

raabf
Copy link
Contributor

@raabf raabf commented Sep 25, 2023

Overall Review of Changes:
The core is that I improved a regex which do not match:

'^(password\s*\[success=1 default=ignore\] pam_unix.so)(.*)(remember=([0-9]{1,})|)(.*$)'

to

'^(?P<begin>[^\S\n]*password[^\S\n]+.*pam_unix.so[^\S\n]+)(?P<remember>(?P<before>.+?)remember=[0-9]+[^\S\n]?)?(?P<after>.*)$'

Issue Fixes:
N/A

Enhancements:

  • The previous regex requires exactly one space between
    default=ignore] and pam_unix.so which on a default OS installation never matches, is now fixed
  • The .* in (.*)(remember=([0-9]{1,})|) of the old regex is greedy, which means that
    everything after it never matches.
  • I name the groups now which is easier than the numbers.
  • I took care that when inserting a non-existing remember=, before and after at least one space is inserted.
  • The same time I make sure that not on every run, an additional
    space is added on replacement, so that the line is not endlessly
    growing.
  • The ansible.builtin.shell: grep 'password.*pam_unix.so' /etc/pam.d/common-password do not require
    the [success=1 default=ignore] but the lineinfile regexes did,
    which would mean that the grep-regex match but not later lineinfile-regexes not ⇒
    I updated it, so that no one requires the [success=1 default=ignore], but
    it will still preserve that part.
  • If you wonder about [^\S\n], this is the shortest way to describe what in other regex-languages is known as \h (horizontal whitespace). Compared to \s this helps especially when there are only empty matches after it.

How has this been tested?:

Some manual runs of the changed task which inserted or kept remember= option correctly, and

Matching Examples to see regex in action: https://regex101.com/r/Kuxcwj

- The previous regex requires exactly *one* space between
    `default=ignore]` and `pam_unix.so` which on a default OS installetion never matches, is now fixed
- The `.*` in `(.*)(remember=([0-9]{1,})|)` was greedy, which means that
    everything after it never matches
- I name the groups now which is easier than the numbers
- I took care that when inserting a non-existing `remember=` before and after it is at least one space.
- A the same time I make sure that *not* on every run, an additonal
   space is added on replacement, so that the line is *not* endlessly
   growing.
- The `ansible.builtin.shell: grep 'password.*pam_unix.so' /etc/pam.d/common-password` do not require
    the `[success=1 default=ignore]` but the lineinfile regexs did,
    which would mean that the grep-regex match but not later lineinfile-regexes not ⇒
    I updated it, so that no one requires the `[success=1 default=ignore]`
    still prserves it.

Signed-off-by: Fabian Raab <[email protected]>
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Congrats on opening your first pull request and thank you for taking the time to help improve Ansible-Lockdown!
Please join in the conversation happening on the Discord Server as well.

Copy link
Member

@uk-bolly uk-bolly left a comment

Choose a reason for hiding this comment

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

Nice tidy up and improvements
thank you

@uk-bolly uk-bolly merged commit 083f9f7 into ansible-lockdown:devel Sep 25, 2023
3 checks passed
@raabf raabf deleted the siemens/ubuntu22/r5_4_3-pam_d-password_reuse_limited branch September 25, 2023 15:44
@uk-bolly uk-bolly mentioned this pull request Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants