Project

General

Profile

Actions

Bug #4342

closed

Several issues with userManagement technique v2.0

Added by Alex Tkachenko over 10 years ago. Updated about 9 years ago.

Status:
Rejected
Priority:
2
Category:
Techniques
Target version:
Severity:
UX impact:
User visibility:
Effort required:
Priority:
Name check:
Fix check:
Regression:

Description

While trying to use the userManagement technique, I have discovered several problems.

First, if the user is configured for check-only, the result of the check is Unknown most of the time. I was able to track it down to the use of library provided warn_only body, which includes ifelapsed => 60. My understanding is that the report line is produced not sooner than 60 minutes apart, as a result, 5-minute-spaced checks do not get proper status, even if everything is ok.

This could be addressed by replacing the library body with the one which does not include ifelapsed clause:

body action warn_immediately
{
  action_policy => "warn";
}

After this was fixed, I have noted that the password check is also reported as unknown, even if the empty one is configured.

First - the empty password detection (same for the full name empty check) - both use isvariable function, but the variables are written into the policy as empty strings, which means that they are still defined, so the use of isvariable just does not make sense. I think it is better to replace them with strcmp-based checks, i.e.:

"usermanagement_user_pwempty_${usergroup_user_index}" expression => strcmp("${usergroup_user_password[${usergroup_user_index}]}","");
"usermanagement_user_nameempty_${usergroup_user_index}" expression => strcmp("${usergroup_user_fullname[${usergroup_user_index}]}","");

However, this won't be sufficient, as there is no reaction generated if the password is empty, so I augmented the reports with this case:

## Password check has not been requested (password is empty)
"@@userGroupManagement@@result_success@@${usergroup_directive_id[${usergroup_user_index}]}@@Password@@${usergroup_user_login[${usergroup_user_index}]}@@${g.execRun}##${g.uuid}@#The user ${usergroup_user_login[${usergroup_user_index}]} ( ${repname[${usergroup_user_index}]} ) password check was not requested (supplied password is empty)" 
  ifvarclass => "usermanagement_user_pwempty_${usergroup_user_index}";

One could also replace the ifvarclass with usermanagement_user_checkpres_${usergroup_user_index}, if we agree that user presence check does not include password check.
But we do check for the full name (why? why not for more important other fields instead?) so for consistency reasons I tried to do the same for the password.
I had to add the following changes:

files:
...
"/etc/shadow" 
  create     => "false",
  edit_line  => set_user_field("${usergroup_user_login[${usergroup_user_index}]}", 2, "${usergroup_user_password[${usergroup_user_index}]}"),
  classes    => kept_if_else("usermanagement_user_password_ok_${usergroup_user_index}", "usermanagement_user_password_repaired_${usergroup_user_index}", "usermanagement_user_password_failed_${usergroup_user_index}"),
  action     => warn_immediately,
  ifvarclass => "usermanagement_user_checkpres_${usergroup_user_index}.!usermanagement_user_pwempty_${usergroup_user_index}";

...

reports:
...
## Could not be changed (Error)
"@@userGroupManagement@@result_error@@${usergroup_directive_id[${usergroup_user_index}]}@@Password@@${usergroup_user_login[${usergroup_user_index}]}@@${g.execRun}##${g.uuid}@#The user ${usergroup_user_login[${usergroup_user_index}]} ( ${repname[${usergroup_user_index}]} ) password could NOT be changed !" 
  ifvarclass => "usermanagement_user_password_failed_${usergroup_user_index}.!usermanagement_user_checkpres_${usergroup_user_index}";
...
## Password does not match for check-only user (Error)
"@@userGroupManagement@@result_error@@${usergroup_directive_id[${usergroup_user_index}]}@@Password@@${usergroup_user_login[${usergroup_user_index}]}@@${g.execRun}##${g.uuid}@#The user ${usergroup_user_login[${usergroup_user_index}]} ( ${repname[${usergroup_user_index}]} ) password does NOT match !" 
  ifvarclass => "usermanagement_user_password_failed_${usergroup_user_index}.usermanagement_user_checkpres_${usergroup_user_index}";

## Password match for check-only (Success)
"@@userGroupManagement@@result_success@@${usergroup_directive_id[${usergroup_user_index}]}@@Password@@${usergroup_user_login[${usergroup_user_index}]}@@${g.execRun}##${g.uuid}@#The user ${usergroup_user_login[${usergroup_user_index}]} ( ${repname[${usergroup_user_index}]} ) password is OK" 
  ifvarclass => "usermanagement_user_exists_${usergroup_user_index}.usermanagement_user_checkpres_${usergroup_user_index}.!usermanagement_user_password_failed_${usergroup_user_index}";

I have attached the resulting .st file for your convenience.

But this is not it :)

If I want to ensure that the system accounts do NOT have a valid password, I need to enter '*' or '!!' in the password field - currently only hashed values are allowed. Please consider adding Plain to the list to allow that.

And the last - if account happens to be defined in the network database (i.e. LDAP) the result is always 'Unknown'. userexists detects the account as present, but file operations would obviously fail (useradd would report an error too). I do not know how to address this properly, but I have used something like

  classes:
    "local_user_defined" 
      expression    => isgreaterthan(countlinesmatching("^$(user):.*", "$(shadow_db)"),0);

before, although introducing this into the technique may make things even more complicated. But it would be nice to have this reported :)

Hope this helps.
Thank you!


Files

userManagement.st (22 KB) userManagement.st Alex Tkachenko, 2014-01-10 00:19

Related issues 3 (0 open3 closed)

Related to Rudder - Bug #4346: In 'userManagement' Technique, the full name is checked only every 60 minutes, resulting in unknown reportsReleasedJonathan CLARKE2014-01-13Actions
Related to Rudder - Bug #4347: Incorrect detection of empty password/name in 'userManagement' Technique when several user are to be managedReleasedJonathan CLARKE2014-01-13Actions
Related to Rudder - User story #3942: Technique "password" field type enhancement: add option to set prehashed passwordRejectedFrançois ARMAND2013-09-19Actions
Actions #1

Updated by François ARMAND over 10 years ago

  • Assignee set to Nicolas CHARLES

Again, thank you for the detailled bug report.

Nico, do you have any idea on that?

Actions #2

Updated by Nicolas CHARLES over 10 years ago

  • Category set to Techniques
  • Status changed from New to 8
  • Target version set to 2.6.10

Thank you very much for the bug report and the very detailled explanations and proposed solution.
I'll be working on this ASAP.

Regards,

Actions #3

Updated by Nicolas CHARLES over 10 years ago

  • Status changed from 8 to In progress
  • Priority changed from N/A to 2
Actions #4

Updated by Nicolas CHARLES over 10 years ago

Alex,

We need to use isvariable to check if the variable is there, because when there is only one empty entry the variable is not written (that's an awful legacy part, i'm sorry about this)
But indeed, when there are several values, we need to compare with strcmp. So the comparision should be based on both; we need to fix this on both 1.0 and 2.0 version of the technique

For the warn policy, we can use the WarnOnly body of the rudder_lib

I really like what you've done for the password check, thank you very much.

For the last point, we have plein password policy (or pre-hashed), but somehow it is not within this technique :( ( see http://www.rudder-project.org/foswiki/Development/TechniqueXML#Available_Hash_format )

I'll be creating tickets for each of these bugs.

Many thanks !

Actions #5

Updated by Alex Tkachenko over 10 years ago

Sure, please use whatever library modules you've got already available. It is just a matter of confusing naming - warn_only vs WarnOnly - go figure that one is doing it immediately and the other - one hour apart :)

Actions #6

Updated by Nicolas CHARLES over 10 years ago

Yeah, naming convention are not excellent on this, sorry :(

Actions #7

Updated by Vincent MEMBRÉ over 10 years ago

  • Target version changed from 2.6.10 to 2.6.11
Actions #8

Updated by Nicolas CHARLES about 10 years ago

Alex,

as you see, i've open one ticket per issue (to have unitary tracking of what is happening), and so far two are fixed and released.

thank you

Actions #9

Updated by Alex Tkachenko about 10 years ago

Yes, I figured. Thank you very much for your work!

Actions #10

Updated by Vincent MEMBRÉ about 10 years ago

  • Target version changed from 2.6.11 to 2.6.12
Actions #11

Updated by Vincent MEMBRÉ about 10 years ago

  • Target version changed from 2.6.12 to 2.6.13
Actions #12

Updated by Vincent MEMBRÉ almost 10 years ago

  • Target version changed from 2.6.13 to 2.6.14
Actions #13

Updated by Jonathan CLARKE almost 10 years ago

  • Target version changed from 2.6.14 to 2.6.16
Actions #14

Updated by Jonathan CLARKE almost 10 years ago

  • Target version changed from 2.6.16 to 2.6.17
Actions #15

Updated by Nicolas PERRON over 9 years ago

  • Target version changed from 2.6.17 to 2.6.18
Actions #16

Updated by Matthieu CERDA over 9 years ago

  • Target version changed from 2.6.18 to 2.6.19
Actions #17

Updated by Vincent MEMBRÉ over 9 years ago

  • Target version changed from 2.6.19 to 2.6.20
Actions #18

Updated by François ARMAND about 9 years ago

  • Status changed from In progress to Rejected

I'm closing that one to have only one ticket tracking the remaining feature: #3942

Actions

Also available in: Atom PDF