pamd: fixes for multiple issues #47695

shepdelacreme · 2018-10-26T22:17:42Z

SUMMARY

Provides fixes for a multitude of issues in the pamd module. This PR supersedes #47420 and includes changes from #47178

Update the regex to account for leading dashes that are allowed in the pamd config file spec.
Also created a VALID_TYPES constant to use throughout the module with all allowed types and their dash permutations.
['account', '-account', 'auth', '-auth', 'password', '-password', 'session', '-session']

For #47083
See changes from @mskymoore to fix update_rule() idempotence issue

For #47197
See changes from #47178 for add_module_arguments() idempotence and duplicate argument issue. Also cleaned up and simplified the logic and forced args_present action to fail if a complex argument (a complex argument is defined in the spec as argument within square brackets) is passed in that we can't handle with this function (users is notified to use updated action instead.

Fixes #47418
Fixes #47083
Fixes #47197

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

pamd

ANSIBLE VERSION

ansible 2.6.5
  config file = /Users/me/.ansible.cfg
  configured module search path = ['/Users/me/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /Users/me/.virtualenvs/ansible/lib/python3.6/site-packages/ansible
  executable location = /Users/me/.virtualenvs/ansible/bin/ansible
  python version = 3.6.6 (default, Jul 21 2018, 22:49:24) [GCC 4.2.1 Compatible Apple LLVM 9.1.0 (clang-902.0.39.2)]

Also tested with 2.7.0 and devel

ADDITIONAL INFORMATION

See #47418
See #47083
See #47197
See #47178

…as well

…tion output

…ion in update_rule()

…regex to capture complex args

… on complex bracketed arguments

ansibot · 2018-10-26T22:24:56Z

Hi @shepdelacreme, thank you for submitting this pull-request!

click here for bot help

shepdelacreme · 2018-10-27T00:08:13Z

cc @defionscode

juliedavila · 2018-10-27T23:31:40Z

shipit

juliedavila · 2018-10-27T23:33:51Z

@jamescassell if you have a moment to take a peek, maybe kick the tires and add a "shipit" comment, we can get this in quicker, same goes for @mskymoore

juliedavila · 2018-10-28T13:04:24Z

cc @kevensen

jamescassell · 2018-10-28T18:45:07Z

I'll take a good look a soon as I can...

jamescassell

Added a few comments. Would be good to see some integration tests. Perhaps wholesale lift/shift the pamd tasks from the various STIG roles into an integration test.

Also, would be easier to review if you use '--fixup' commits, then autosquash those to make each commit a logical change as shown on github.

lib/ansible/modules/system/pamd.py

jamescassell · 2018-10-30T09:59:31Z

lib/ansible/modules/system/pamd.py

+                    if isinstance(new_args, str):
+                        new_args = new_args.replace(" = ", "=")
+                        new_args = new_args.split(' ')
+                    if(current_rule.rule_args is not new_args):


is not vs !=? I may not be polished enough on my python...

is tests identity = tests equality. These if statements would always test true because current_rule.rule_args and new_args are two different objects. Using != tests for equality of what is contained in each variable not if the two vars are pointing to the same spot in memory.

lib/ansible/modules/system/pamd.py

jamescassell · 2018-10-30T10:08:34Z

lib/ansible/modules/system/pamd.py

+                    no_eq_new_args.add(new_arg)
+                else:
+                    pair = new_arg.split("=")
+                    new_args_d[pair[0]] = pair[1]


Does this handle the case where there may be multiple instances of = within an arg?

This code was changed in a later commit.

lib/ansible/modules/system/pamd.py

jamescassell · 2018-10-30T10:35:55Z

lib/ansible/modules/system/pamd.py

+    # Return empty list if we have no args to parse
+    if not module_arguments:
+        return []
+    elif not module_arguments[0]:


What does this check for?

When parse_module_arguments is used from within the PamdRule class to parse out the arguments in an existing rule in a pamd file it can potentially send nothing if there are no matches.

This checks for the cases where the passed in args are None, and empty list [] and the second one checks for a list with a single empty item. i.e [''] and then returns early.

looks like it checks that the first element is empty, but not that there is a single element...

The first part of the if handles module_arguments is:

an empty string

an empty list

None

The second part of the if handles module_arguments is:

a list with a single empty item ['']

I can add a check like isinstance(module_arguments, list) and len(module_arguments) == 1 and not module_arguments[0] if that is preferable.

Yes, and that would narrow it down to one ifstatement.

Yeah that's probably more defensive given UserError, it could be possible, though probably not that likely that there would be some array [None,'foo', 'bar']

I don't think it narrows it down to a single if since module_arguments can be an empty string or None and if it is we still want to return [] which the first if covers. My change for this would look like

if not module_arguments: return [] elif isinstance(module_arguments, list) and len(module_arguments) == 1 and not module_arguments[0]: return []

Ack. Looks like a good change.

I mean, you could do an or but that might break pep8 on line length, I dont care either way, the major thing for me was just being defensive about index 0 potentially being a None value AND not the only index.

Yeah my preference is for shorter/simpler if tests. I'll update this. I wonder if I can make a suggested change to myself and apply it hehe.

lib/ansible/modules/system/pamd.py

Co-Authored-By: shepdelacreme <shepdelacreme@users.noreply.github.com>

shepdelacreme · 2018-10-30T13:57:09Z

@jamescassell Thanks for the thorough review. And yes I agree integration tests here are 💯 needed. The unit tests for it are good but they didn't catch the issues the module had and I believe integration tests would have.

juliedavila · 2018-10-30T13:58:09Z

Sweet, I'll open a new issue about adding an integration test.

* pamd: fixes for multiple issues (#47695) * Providing fix for #47083 in pamd.py * Providing fix for #47197 * Fixing pep8 errors * update regex to account for leading dash and VALID_TYPES with dashes as well * use a results dictionary and clean up unnecessary items * remove unnessecary return value. action is already reported in invocation output * make naming consistent across action returns * fix comparison so it checks equality instead of identity and indentation in update_rule() * make sure file always has EOF newline * updated regex to skip spacing between path and args and add rule arg regex to capture complex args * new module argument parsing code in function and DRY changes * remove unused has_rule method on PamdService class * fix error in parse_module_arguments() * updated args_present action to make it handle key value args and fail on complex bracketed arguments * pep8 and other fixes so units still work * suggested change - make version removed 2.8 Co-Authored-By: shepdelacreme <shepdelacreme@users.noreply.github.com> * add more error proof test to if statement (cherry picked from commit ef690e9) * add changelog fragment for backport * add action return value back for backport

…8305)

* Providing fix for ansible#47083 in pamd.py * Providing fix for ansible#47197 * Fixing pep8 errors * update regex to account for leading dash and VALID_TYPES with dashes as well * use a results dictionary and clean up unnecessary items * remove unnessecary return value. action is already reported in invocation output * make naming consistent across action returns * fix comparison so it checks equality instead of identity and indentation in update_rule() * make sure file always has EOF newline * updated regex to skip spacing between path and args and add rule arg regex to capture complex args * new module argument parsing code in function and DRY changes * remove unused has_rule method on PamdService class * fix error in parse_module_arguments() * updated args_present action to make it handle key value args and fail on complex bracketed arguments * pep8 and other fixes so units still work * suggested change - make version removed 2.8 Co-Authored-By: shepdelacreme <shepdelacreme@users.noreply.github.com> * add more error proof test to if statement

…8305)

skymoore and others added 15 commits October 25, 2018 11:54

Providing fix for #47083 in pamd.py

b35a570

Providing fix for #47197

cab34f9

Fixing pep8 errors

8a3b421

update regex to account for leading dash and VALID_TYPES with dashes …

7672606

…as well

use a results dictionary and clean up unnecessary items

4c47456

remove unnessecary return value. action is already reported in invoca…

5abaa32

…tion output

make naming consistent across action returns

d71cd3d

fix comparison so it checks equality instead of identity and indentat…

d09c340

…ion in update_rule()

make sure file always has EOF newline

c0f5e05

updated regex to skip spacing between path and args and add rule arg …

44d70ce

…regex to capture complex args

new module argument parsing code in function and DRY changes

c2427a2

remove unused has_rule method on PamdService class

99aae18

fix error in parse_module_arguments()

9ad33a4

updated args_present action to make it handle key value args and fail…

c00430f

… on complex bracketed arguments

pep8 and other fixes so units still work

d386521

shepdelacreme mentioned this pull request Oct 26, 2018

pamd: allow leading dash and always end with newline #47420

Closed

shepdelacreme mentioned this pull request Oct 26, 2018

Providing fix for #47083 in pamd.py #47178

Closed

juliedavila self-assigned this Oct 27, 2018

juliedavila approved these changes Oct 27, 2018

View reviewed changes

jamescassell reviewed Oct 30, 2018

View reviewed changes

jamescassell and others added 2 commits October 30, 2018 09:29

suggested change - make version removed 2.8

a597314

Co-Authored-By: shepdelacreme <shepdelacreme@users.noreply.github.com>

add more error proof test to if statement

3eef783

juliedavila merged commit ef690e9 into ansible:devel Oct 30, 2018

jborean93 removed the needs_triage Needs a first human triage before being processed. label Nov 1, 2018

shepdelacreme deleted the pamd_fixes_multiple_issues branch November 7, 2018 16:52

This was referenced Nov 7, 2018

add changelog fragment for changes merged in #47695 #48305

Merged

Backport/2.7/47695 - pamd fixes #48307

Merged

jborean93 pushed a commit that referenced this pull request Nov 8, 2018

add changelog fragment for changes merged in #47695 (#48305)

e212861

mjmayer pushed a commit to mjmayer/ansible that referenced this pull request Nov 30, 2018

add changelog fragment for changes merged in ansible#47695 (ansible#4…

24d37b4

…8305)

Tomorrow9 pushed a commit to Tomorrow9/ansible that referenced this pull request Dec 4, 2018

add changelog fragment for changes merged in ansible#47695 (ansible#4…

bdd321e

…8305)

ansible locked and limited conversation to collaborators Jul 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pamd: fixes for multiple issues #47695

pamd: fixes for multiple issues #47695

shepdelacreme commented Oct 26, 2018

ansibot commented Oct 26, 2018

shepdelacreme commented Oct 27, 2018

juliedavila commented Oct 27, 2018

juliedavila commented Oct 27, 2018

juliedavila commented Oct 28, 2018

jamescassell commented Oct 28, 2018

jamescassell left a comment

jamescassell Oct 30, 2018

shepdelacreme Oct 30, 2018

jamescassell Oct 30, 2018

shepdelacreme Oct 30, 2018

jamescassell Oct 30, 2018

shepdelacreme Oct 30, 2018

jamescassell Oct 30, 2018

shepdelacreme Oct 30, 2018

jamescassell Oct 30, 2018

juliedavila Oct 30, 2018 •

edited

shepdelacreme Oct 30, 2018

jamescassell Oct 30, 2018

juliedavila Oct 30, 2018

shepdelacreme Oct 30, 2018

shepdelacreme commented Oct 30, 2018

juliedavila commented Oct 30, 2018

pamd: fixes for multiple issues #47695

pamd: fixes for multiple issues #47695

Conversation

shepdelacreme commented Oct 26, 2018

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

ansibot commented Oct 26, 2018

shepdelacreme commented Oct 27, 2018

juliedavila commented Oct 27, 2018

juliedavila commented Oct 27, 2018

juliedavila commented Oct 28, 2018

jamescassell commented Oct 28, 2018

jamescassell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliedavila Oct 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shepdelacreme commented Oct 30, 2018

juliedavila commented Oct 30, 2018

juliedavila Oct 30, 2018 •

edited