The Puppet Labs Issue Tracker has Moved: https://tickets.puppetlabs.com

Feature #6865

Bug #15472: rewrite puppet agent

Proposed patch to add support to puppetd agent extra command line args

Added by Mohamed Lrhazi about 3 years ago. Updated over 1 year ago.

Status:ClosedStart date:03/28/2011
Priority:NormalDue date:
Assignee:R.I. Pienaar% Done:

0%

Category:-
Target version:-
Keywords: Affected mCollective version:
Branch:

We've Moved!

Ticket tracking is now hosted in JIRA: https://tickets.puppetlabs.com

This issue is currently not available for export. If you are experiencing the issue described below, please file a new ticket in JIRA. Once a new ticket has been created, please add a link to it that points back to this Redmine ticket.


Description

I am suggesting a patch for puppetd agent, which allows the user to pass arbitrary command line arguments to be passed to puppetd.

Please the forked here, branch name: puppetd_options

https://github.com/lrhazi/mcollective-plugins

Thanks.

History

#1 Updated by R.I. Pienaar about 3 years ago

  • Project changed from MCollective to MCollective Plugins

#2 Updated by R.I. Pienaar about 3 years ago

  • Status changed from Unreviewed to Accepted
  • Assignee set to R.I. Pienaar

#3 Updated by Mohamed Lrhazi about 3 years ago

This change actually makes mc-puppetd very dangerous… In our setup, we have all machines configured to run with noop=true, set to a specific environment and set to speak to a production puppetmaster. machines are setup so we can also make them speak to a puppet-test server, when need be.

Changes to the production puppetmaster are carefully reviewed, and so are changes to the modules in those specific ‘production environments’.

With this feature in mc-puppetd, one could bypass all the restrictions and force all machines to run against the test puppetmaster, requesting an untested environment:

mc-puppetd —options “—environment my_beta_env —server puppet-test.dom.ain —no-noop” runall 5

I think “—options” is still very useful, and we will use it in our production env… Just thought I’d point that danger out. I was thinking about maybe adding logic to mc-puppetd to refuse to run in certain circumstances.. say if number of affected hosts is above a threshold and the —environment or —server options are included.

#4 Updated by Mohamed Lrhazi about 3 years ago

There is a bug in the ddl, cause mc-puppetd to fail if —options is not supplied.

@@ -31,7 +31,7 @@

     :prompt      => "Puppetd options",
     :description => "Extra command line arguments to puppetd",
     :type        => :string,
  •  :validation  => '^.+$',
    
  •  :validation  => '^.*$',
     :optional    => true,
     :maxlength    => 256
    

#5 Updated by R.I. Pienaar about 3 years ago

  • Status changed from Accepted to Needs Decision

I am putting this to ‘needs decision’ as I am still undecided how I feel about this patch, as demonstrated on the list recently it can expose some unexpected behavior that is sub optimal for the user.

#6 Updated by Mohamed Lrhazi about 3 years ago

Would a compromise be to add a notification/confirmation promopt when a problematic option is passed? maybe only —no-daemonize and —waitforcert would be an issue?

#7 Updated by R.I. Pienaar about 3 years ago

Mohamed Lrhazi wrote:

Would a compromise be to add a notification/confirmation promopt when a problematic option is passed? maybe only —no-daemonize and —waitforcert would be an issue?

How would you define a problematic argument? I mean this could be seen as a HUGE security risk, imagine you give someone non root shell level access to a server and access to mcollective. Safe right? Nope, cos they can start puppetd as root via mcollective passing into it a custom manifest dir where they can run any .pp files it finds in random directories.

The work to determine whats safe and not would be epic, every config option in puppet.conf is also a command line option and these change frequently as puppet add more features, we’d forever be playing catchup.

It really is the same concerns I have with a plugin that can run random shell commands, just not the mcollective way of doing things.

#8 Updated by Mohamed Lrhazi about 3 years ago

By “problematic” I was simply thinking in terms of mcollective expecting commands to finish in under the default 3 seconds timeout. I kind of forgo about other implications….

Ok.. How about a config parameter that the user can use to set exactly what options are allowed? “allowed_options”, which would default to what we consider to be safe, maybe just “noop” “no-noop”, and then leave it up to the user to decide beyond that what risks they are willing to take?

#9 Updated by R.I. Pienaar about 3 years ago

That would probably be good, best would be to just add boolean options to enable noop if thats what you want though. Then the API interface is strictly defined and ppl know what to expect

#10 Updated by Mohamed Lrhazi about 3 years ago

am not sure I understand. Are you saying you would be willing to accept —options, if it came with an “allowed_options” config setting? or you would rather see two additional boolean options only, —noop and no-noop?

In our site, I’d say that we cannot live without noop/no-noop, and would be happy to have the option of additional “options” if we ever needed them someday: —server/—tags/—environment…

mcollective can affect systems that are unreachable via SSH, say due to LDAP issue, like we had recently after some ssl certs were updated on the LDAP servers… so it could act as an “out-of-band” path to access and fix some problem that would otherwise require console access to each and every system… So, there might be cases where the ability to run puppetd in an “unexpected” way, might be useful.

Thanks a lot.

#11 Updated by R.I. Pienaar about 3 years ago

Mohamed Lrhazi wrote:

am not sure I understand. Are you saying you would be willing to accept —options, if it came with an “allowed_options” config setting? or you would rather see two additional boolean options only, —noop and no-noop?

I am saying with a whitelist of option – not a blacklist – I think its safe, but first price would be passable boolean options.

mcollective can affect systems that are unreachable via SSH, say due to LDAP issue, like we had recently after some ssl certs were updated on the LDAP servers… so it could act as an “out-of-band” path to access and fix some problem that would otherwise require console access to each and every system… So, there might be cases where the ability to run puppetd in an “unexpected” way, might be useful.

that’s true, if you want to hack on the whitelist as described I think that will address my concerns

#12 Updated by Mohamed Lrhazi about 3 years ago

Please consider the updated version (branch puppetd_options): https://github.com/lrhazi/mcollective-plugins

Have not heavily tested the security point of view, but tried to think about it a bit. Should be easy to improve/fix bugs, by updating the regexes…

Thanks a lot.

#13 Updated by R.I. Pienaar about 3 years ago

  • Status changed from Needs Decision to Code Insufficient

Thanks for this,

Sorry to be a pain, but your commit puts the whitelist on the client side in the mc-puppetd script. This doesnt actually provide any security it just means for that single script there’s a roadblock, someone can still use mco rpc or just in ruby to call the agent and bypass your restrictions.

the security – the whitelist – has to be in the agent to be effective.

#14 Updated by Mohamed Lrhazi about 3 years ago

Nah, my bad. Will work on this tonight. Thanks.

#15 Updated by Mohamed Lrhazi about 3 years ago

I moved the puppetd options checks from clients to server side. Please check my latest commit https://github.com/lrhazi/mcollective-plugins/tree/puppetd_options

#16 Updated by Mohamed Lrhazi about 3 years ago

I still think that safer would be to bypass the shell altogether by using IO.popen. I am not comfortable yet with Ruby, nor with mcollective, to try and code that.

http://ruby-doc.org/core/classes/IO.html#M000880

cmd:
"-"                                      : fork
commandline                              : command line string which is passed to a shell
[env, cmdname, arg1, ..., opts]          : command name and zero or more arguments (no shell)
[env, [cmdname, argv0], arg1, ..., opts] : command name, argv[0] and zero or more arguments (no shell)
(env and opts are optional.)

#17 Updated by Mohamed Lrhazi about 3 years ago

Overriding the default white-list from the plugin options does not seem to work. I will update this ticket when I resolved it. Thanks.

#18 Updated by Mohamed Lrhazi about 3 years ago

I think I figured it out. It seems this is wrong:

plugin.puppetd.options_whitelist = “—noop,—no-noop,—tags,—environment”

While this is correct:

plugin.puppetd.options_whitelist = —noop,—no-noop,—tags,—environment

#19 Updated by R.I. Pienaar over 1 year ago

  • Parent task set to #15472

#20 Updated by R.I. Pienaar over 1 year ago

  • Status changed from Code Insufficient to Closed

While the new puppet agent does not support arbitrary arguments it does support noop, no-noop, tags, environment and custom server:port selection.

resolving

Also available in: Atom PDF