Bug #4342
closedSeveral issues with userManagement technique v2.0
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
Updated by François ARMAND almost 11 years ago
- Assignee set to Nicolas CHARLES
Again, thank you for the detailled bug report.
Nico, do you have any idea on that?
Updated by Nicolas CHARLES almost 11 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,
Updated by Nicolas CHARLES almost 11 years ago
- Status changed from 8 to In progress
- Priority changed from N/A to 2
Updated by Nicolas CHARLES almost 11 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 !
Updated by Alex Tkachenko almost 11 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 :)
Updated by Nicolas CHARLES almost 11 years ago
Yeah, naming convention are not excellent on this, sorry :(
Updated by Vincent MEMBRÉ almost 11 years ago
- Target version changed from 2.6.10 to 2.6.11
Updated by Nicolas CHARLES almost 11 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
Updated by Alex Tkachenko almost 11 years ago
Yes, I figured. Thank you very much for your work!
Updated by Vincent MEMBRÉ over 10 years ago
- Target version changed from 2.6.11 to 2.6.12
Updated by Vincent MEMBRÉ over 10 years ago
- Target version changed from 2.6.12 to 2.6.13
Updated by Vincent MEMBRÉ over 10 years ago
- Target version changed from 2.6.13 to 2.6.14
Updated by Jonathan CLARKE over 10 years ago
- Target version changed from 2.6.14 to 2.6.16
Updated by Jonathan CLARKE over 10 years ago
- Target version changed from 2.6.16 to 2.6.17
Updated by Nicolas PERRON over 10 years ago
- Target version changed from 2.6.17 to 2.6.18
Updated by Matthieu CERDA about 10 years ago
- Target version changed from 2.6.18 to 2.6.19
Updated by Vincent MEMBRÉ about 10 years ago
- Target version changed from 2.6.19 to 2.6.20
Updated by François ARMAND almost 10 years ago
- Status changed from In progress to Rejected
I'm closing that one to have only one ticket tracking the remaining feature: #3942