Rewrite user creation to prevent quoting bug (fixes #173) #176
Conversation
| @@ -39,8 +39,8 @@ | |||
| } | |||
|
|
|||
| $cmd = $privpass ? { | |||
| undef => "createUser ${title} ${authtype} \\\"${authpass}\\\"", | |||
| default => "createUser ${title} ${authtype} \\\"${authpass}\\\" ${privtype} \\\"${privpass}\\\"" | |||
| undef => "createUser ${title} ${authtype} \"${authpass}\"", | |||
There was a problem hiding this comment.
Do we actually need double quotes around those strings? Can't we use single quotes?
There was a problem hiding this comment.
Maybe. I will do some tests when I get back into office.
There was a problem hiding this comment.
It seems that single quotes would also work.
Maybe the quoting should be reviewed. Quotes are only required when the password contains spaces. On the other hand passwords may also include quotes which then need to be escaped (not currently implemented). We could drop the quotes in Puppet and forget about escaping if we would just prohibit passwords with spaces.
| # | ||
| $command = "service ${service_name} stop ; sleep 5" | ||
|
|
||
| exec { "stop-${service_name}": |
There was a problem hiding this comment.
Why is this even an exec resource? Can't we refresh the service?
There was a problem hiding this comment.
No, probably not. It seems to be working the following way: The demon writes the internal state (users with hashed passwords, number of restarts, ...) to the config in /var/lib/snmp/snmpd.conf when it terminates. It doesn't update the file, it replaces it. So you loose everything you add to the file when the daemon is still running. Therefore you need to stop the daemon before you add the new users.
Unfortunately you can't use one service resource to terminate a service and another one to start it. If they refer to the same service name then Puppet will throw a duplicate declaration error. So I believe we will have to keep the exec to stop the daemon. I don't like it either but I don't have any better idea.
There was a problem hiding this comment.
I'm looking to make some changes to this module to handle v3 engine IDs and other things. I've noticed that /var/lib/net-snmp is being used for the configuration. Why not use /etc/snmp instead? The files under /var/lib/net-snmp say to not edit manually and would make the code for this a lot cleaner.
There was a problem hiding this comment.
I've noticed that /var/lib/net-snmp is being used for the configuration. Why not use /etc/snmp instead? The files under /var/lib/net-snmp say to not edit manually and would make the code for this a lot cleaner.
@hanej smoeding explains well. If you need to ready again, for instance have a look to:
https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/html-single/system_administrators_guide/#sect-System_Monitoring_Tools-Net-SNMP-Configuring
There was a problem hiding this comment.
Even that documentation says to edit the config files under /etc/snmp. I’ll create a PR with my changes.
There was a problem hiding this comment.
See the official manpage at http://www.net-snmp.org/docs/man/snmpd.examples.html
Remember that these createUser directives should be defined in the /var/net-snmp/snmpd.conf file, rather than the usual location.
|
@smoeding do you need to make some more work about this PR ? Are you waiting a review ? |
|
@Dan33l As far as I am concerned I am done with this PR. |
|
@smoeding sorry but with the last merge, this PR needs now a rebase. Can you do it ? and i'll merge it. |
This change replaces the exec resource to update the config file with the SNMPv3 user name with a file_line resource.
|
No need to apologize! Your are doing a good job to bring the project forward! |
|
@smoeding Thanks! |
|
|
||
| Data type: `String` | ||
|
|
||
| The authentication type to calculate. This must either be |
There was a problem hiding this comment.
There might maybe be a time when this might be handy outside of the module. But currently I would think it is safe to keep it private to the module.
Pull Request (PR) description
This change replaces the
execresource to update the config file with the SNMPv3 user name with afile_lineresource. This moves more logic from theexecresource to Puppet types. It should handle special characters better and make the code more robust.Unfortunately the
execresource can't be replaced completely (see comment in #173).With this code the service is also stopped only once if multiple users are to be created. This speeds up processing.
This Pull Request (PR) fixes the following issues
Fixes #173