Fixes #403: Extend Rudder specific inventory with client side data #404

ncharles · 2017-11-14T13:52:38Z

No description provided.

g-bougard

Hi @ncharles
thank you for the PR.
I'm suspicious about the fact you want to run as root or admin any script found in a rudder dedicated folder.
I would love to see a safer check while checking we should run that scripts or a safer way of running them.

g-bougard · 2017-11-15T13:07:59Z

lib/FusionInventory/Agent/Task/Inventory/Generic/Rudder.pm

+    my $custom_properties;
+    if (-d "$custom_properties_dir") {
+        my @custom_propertes_list = ();
+        opendir(DIR, $custom_properties_dir); # or die $!;


You should remove the not useful comment at the end of line here.

ha, i've had a remark on this one that I should catch the error. Should I catch, or will it be correctly handled by calling code ?

To be fully safe, you can initialize @ordered_script_list to empty list, run an eval where you want to populate the list, you should even move the closedir(DIR) from l.99 in the eval. Then if something goes wrong, the list will be kept empty or even partially filled. If the list remains empty, the while will do nothing and no custom property will be inventoried, but the Rudder module will still finish and inventory other informations.

g-bougard · 2017-11-15T13:12:53Z

lib/FusionInventory/Agent/Task/Inventory/Generic/Rudder.pm

+                my $properties = qx($script_file);
+                my $exit_code = $? >> 8;
+                if ($exit_code > 0) {
+                    $logger->error("Script $script_file failed to run properly, with exit code $exit_code");


By convention, don't assume $logger is defined while it was passed in %params. So add a if $logger at the end of line before ;

ha, this is something I was not aware of

g-bougard · 2017-11-15T13:13:30Z

lib/FusionInventory/Agent/Task/Inventory/Generic/Rudder.pm

+                    push @custom_propertes_list, $coder->encode($propertiesData);
+                };
+                if ($@) {
+                    $logger->error("Script $script_file didn't return valid JSON entry, error is:$@");


The same here, don't assume $logger is defined

g-bougard · 2017-11-15T13:15:43Z

lib/FusionInventory/Agent/Task/Inventory/Generic/Rudder.pm

+    my $custom_properties_dir = ($OSNAME eq 'MSWin32') ? 'C:\Program Files\Rudder\local\hooks.d' : '/var/rudder/local/hooks.d';
+    my $custom_properties;
+    if (-d "$custom_properties_dir") {
+        my @custom_propertes_list = ();


I think you missed an i to say properties in @custom_propertes_list

g-bougard · 2017-11-15T13:20:11Z

lib/FusionInventory/Agent/Task/Inventory/Generic/Rudder.pm

@@ -7,6 +7,8 @@ use English qw(-no_match_vars);

 use FusionInventory::Agent::Tools;

+use JSON::PP;


Even if JSON::PP is still necesarry for some FusionInventory agent modules, I would advise you to better depend on UNIVERSAL::require and add a JSON::PP->require() later in the related-eval. This will delay the load until it is really needed and even will save some memory usage while no custom script is found.

i mimicked what was in Inventory/Virtualization/Docker.pm and HTTP/Client/Fusion.pm
i'll try the proposed solution

g-bougard · 2017-11-15T13:32:34Z

lib/FusionInventory/Agent/Task/Inventory/Generic/Rudder.pm

+                next if ($file =~ m/^\./);
+                # Ignore non-executable file, or folders
+                next unless -x $script_file;
+                my $properties = qx($script_file);


I know you want here to not use runCommand() API as it won't fail if script fails as you asked us about that on IRC. But it is important then to at least add a debug2 level logging just before as is doing runCommand() so we can debug easily something is started from here and what.
Another point, does this scripts need to be run as root user ? This can look like a security hole if somebody other than root gains the privilege to write a script there.

I can add the logger; however i'm not sure which user could be used to run the scripts, except root (or current admin user).
Plus, if someone can access as root to /var/rudder, we have other issues on our hands. @peckpeck do you have any advices on this ?

today it is run by root, this means we have to make sure that this directory is not writable by anyone except root.
For users not having rudder, /var is writable only by root, so this is not a security problem for them

Okay, then I'll be okay if you add a simple check to see if scripts is owned and executable by root. You should then log a warning and not start the script if it is not owned and runnable by admin.

More we add safe controls, more we can be sure nothing goes wrong for users.

Is your remarks also valid for win32 platform ? This is the most sensible...

yes, c:\Program Files\Rudder is also owned and readable only by Administrator

By root or by the current user

if it is not runable, it cannot be run by the command, so the check is not interesting.
If you want a better check, you can check that when you are root, it is not writable by anyone else.

ncharles · 2017-11-15T13:50:10Z

Thank you for the feedback !

…nt side data

ncharles · 2017-11-20T15:01:19Z

lib/FusionInventory/Agent/Task/Inventory/Generic/Rudder.pm

+                # Ignore non-executable file, or folders
+                next unless -x $script_file;
+
+                # Check that the file is not world writable


here, I failed to check if file was owned by root (or its Windows equivalent) - as there can be several admin
how would you do that ? @peckpeck @g-bougard

I used
my $stats = stat($script_file);
my $owner = $stats->uid;
my $currentUser = $<;

file must be owned by root or current user
if (($owner != 0) && ($owner != $currentUser)) {
$logger->error("Skipping script $script_file as it is not owned by root nor current user (owner is $owner)") if $logger;
next;
}
but this fails miserably on Windows - it always believe the owner of the file is 0

ncharles · 2017-11-21T14:13:48Z

lib/FusionInventory/Agent/Task/Inventory/Generic/Rudder.pm

+                my $permissions = stat($script_file);
+                my $retMode = $permissions->mode;
+                $retMode = $retMode & 0777;
+                if (($retMode & 002) || ($retMode & 020)) {


this fails on windows - even if file is writable only by admin, it gets 777 :(

ncharles force-pushed the bug_403_extend_inventory_in_rudder branch from e13d549 to 1e18741 Compare November 15, 2017 08:50

g-bougard reviewed Nov 15, 2017

View reviewed changes

g-bougard added the Inventory-task label Nov 15, 2017

ncharles force-pushed the bug_403_extend_inventory_in_rudder branch from 1e18741 to 2ecadbd Compare November 20, 2017 14:58

Fixes fusioninventory#403: Extend Rudder specific inventory with clie…

e3721f9

…nt side data

ncharles force-pushed the bug_403_extend_inventory_in_rudder branch from 2ecadbd to e3721f9 Compare November 20, 2017 15:00

ncharles commented Nov 20, 2017

View reviewed changes

ncharles commented Nov 21, 2017

View reviewed changes

g-bougard mentioned this pull request Nov 22, 2017

Win32 file security check API needed #409

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #403: Extend Rudder specific inventory with client side data #404

Fixes #403: Extend Rudder specific inventory with client side data #404

ncharles commented Nov 14, 2017

g-bougard left a comment

g-bougard Nov 15, 2017

ncharles Nov 15, 2017

g-bougard Nov 15, 2017

g-bougard Nov 15, 2017

ncharles Nov 15, 2017

g-bougard Nov 15, 2017

g-bougard Nov 15, 2017

ncharles Nov 20, 2017

g-bougard Nov 15, 2017

ncharles Nov 15, 2017

g-bougard Nov 15, 2017

ncharles Nov 15, 2017

peckpeck Nov 15, 2017

g-bougard Nov 15, 2017

ncharles Nov 15, 2017

peckpeck Nov 15, 2017

peckpeck Nov 15, 2017

ncharles commented Nov 15, 2017

ncharles Nov 20, 2017

ncharles Nov 21, 2017

ncharles Nov 21, 2017

		@@ -7,6 +7,8 @@ use English qw(-no_match_vars);

		use FusionInventory::Agent::Tools;

		use JSON::PP;

Fixes #403: Extend Rudder specific inventory with client side data #404

Are you sure you want to change the base?

Fixes #403: Extend Rudder specific inventory with client side data #404

Conversation

ncharles commented Nov 14, 2017

g-bougard 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

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

ncharles commented Nov 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment