Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: 2.4.x
Choose a base branch
from

Conversation

ncharles
Copy link
Contributor

No description provided.

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

@g-bougard g-bougard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ha, this is something I was not aware of

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 = ();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed:)

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

use FusionInventory::Agent::Tools;

use JSON::PP;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By root or by the current user

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

Thank you for the feedback !

@ncharles ncharles force-pushed the bug_403_extend_inventory_in_rudder branch from 1e18741 to 2ecadbd Compare November 20, 2017 14:58
@ncharles ncharles force-pushed the bug_403_extend_inventory_in_rudder branch from 2ecadbd to e3721f9 Compare November 20, 2017 15:00
# Ignore non-executable file, or folders
next unless -x $script_file;

# Check that the file is not world writable
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants