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

Bug #4916

Plugins should not be able to override core functionality.

Added by Nigel Kersten almost 4 years ago. Updated over 1 year ago.

Status:RejectedStart date:10/01/2010
Priority:LowDue date:
Assignee:-% Done:

0%

Category:plug-ins
Target version:3.0.0
Affected Puppet version: Branch:
Keywords:

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

If you take core functionality like a provider, say puppet/providers/package/apt.rb and copy it to a module libdir, and then have pluginsync on… then anyone consuming that modulepath, regardless of whether they consume this particular module, will have this plugin override the core functionality.

I think this is a problem. It really doesn’t seem like a good idea from a security point of view for plugins to be able to override an equivalent core library.

I understand that some people find this useful for patching functionality with providers. This doesn’t make it right.

They should instead be inheriting from the desired provider and specifying the provider they want.


Related issues

Duplicated by Puppet - Feature #5383: envpuppet script should allow puppet in site_ruby Duplicate 10/20/2010

History

#1 Updated by Nigel Kersten almost 4 years ago

  • Status changed from Unreviewed to Accepted
  • Assignee set to Dan Bode

#2 Updated by Nigel Kersten almost 4 years ago

  • Status changed from Accepted to Needs Decision
  • Assignee deleted (Dan Bode)

I swear I did this on a completely different ticket, not this one.

#3 Updated by Nigel Kersten almost 4 years ago

  • Assignee set to Luke Kanies

Luke, assigning to you for input, as I’m not sure who is more appropriate.

#4 Updated by donavan m almost 4 years ago

Is there a distinction being made between modules/custom/lib/puppet/ and modules/foo/lib/puppet/, or was this obviated with the .25 change? I can see the reasoning for modules/custom/lib/puppet/providers/package/apt.rb always overriding core functionality. Having modules/foo/lib/puppet/providers/package/apt.rb override the core provider seems like a bad plan.

#5 Updated by Nigel Kersten almost 4 years ago

I was under the impression the ‘custom’ module was a convention only.

#6 Updated by Luke Kanies almost 4 years ago

  • Status changed from Needs Decision to Accepted
  • Assignee deleted (Luke Kanies)

The ‘custom’ module name is definitely just a convention.

#7 Updated by eric sorenson over 3 years ago

A data point for what it’s worth: the current functionality is highly useful for sites like mine where rolling new packages sitewide is a slow-cycle process. This saved my bacon tonight to roll a hotfix to mount.rb for #6027.

#8 Updated by eric sorenson over 3 years ago

eric sorenson wrote:

A data point for what it’s worth: the current functionality is highly useful for sites like mine where rolling new packages sitewide is a slow-cycle process. This saved my bacon tonight to roll a hotfix to mount.rb for #6027.

I take it back, this is evil and a Bad Idea[tm].

#9 Updated by Nigel Kersten over 3 years ago

Care to shed more light on why this is so evil Eric? :)

#10 Updated by eric sorenson over 3 years ago

  • makes upgrading awful
  • appears to trigger a bug if you add core-override plugins and then later (perhaps in an attempt to upgrade) try to remove them

Feb 17 03:36:25db005 puppetd[12917]: (/File[/var/lib/puppet/lib/puppet/type/mount.rb]/ensure) change from absent to file failed: Could not find any content at puppet://puppet/pl ugins/puppet/type/mount.rb Feb 17 03:36:26 db005 puppetd[12917]: Could not retrieve catalog from remote server: Error 400 on SERVER: Failed to parse template puppet-client/puppet.conf.erb: Could not find value for ‘operatingsystem’ at /etc/puppetlabs/puppet-test/modules/puppet-client/manifests/config.pp:5 on node db005.mydomain.com

#11 Updated by Nigel Kersten over 3 years ago

  • Target version set to 3.x

#12 Updated by Nick Lewis over 3 years ago

  • Category set to plug-ins

#13 Updated by Nigel Kersten over 3 years ago

  • Status changed from Accepted to Rejected

I’d actually forgotten I’d filed this a while ago before I joined Puppet Labs.

We’re reversing position on this one given we’d like to be able to pluginsync more and more of the code base.

I’m going to reject it.

#14 Updated by Jacob Helwig over 3 years ago

  • Status changed from Rejected to Accepted

Nigel Kersten wrote:

I’d actually forgotten I’d filed this a while ago before I joined Puppet Labs.

We’re reversing position on this one given we’d like to be able to pluginsync more and more of the code base.

I’m going to reject it.

This shouldn’t interfere with the plan to pluginsync more and more types and providers, and probably becomes more important to be careful about what things plugins can override as we increase the frequency with which pluginsync is used to deliver things.

It’s also something that we were planning on tackling in the near future on the Open Source Team. Re-opening this, since it’s relatively high on our current backlog, and we still feel that it’s something that should be done as part of the larger plan of cleaning up the autoloader behavior in Puppet.

[Ed (Nigel)] – We all got on the same page in meat space before this update in case it looks like we’re churning without direction.

#15 Updated by Nigel Kersten over 2 years ago

  • Priority changed from Normal to Low

#16 Updated by Nigel Kersten over 2 years ago

  • Status changed from Accepted to Needs Decision
  • Assignee set to Anonymous

#17 Updated by Anonymous over 2 years ago

  • Status changed from Needs Decision to Rejected

Ultimately, Patrick and I have looked at this and are rejecting it.

I think that some part of this would be valuable, but it is not clear what the reasonable scope of that would be – and I think it would be much smaller than expected, given we want to move toward a world where more and more functionality can be supplied in modules and, consequently, on a per-environment basis.

That isn’t the heart of the rejection, though. The heart of that is that we cannot stop people futzing with “core” functionality in Ruby. It is literally impossible; any code we load can result in code being injected that overrides core functionality, and without an AI complete analysis phase we can’t tell if a seemingly innocent “type” or “provider” extension is secretly reaching out and overriding something in a core module.

It doesn’t feel worth the bother trying to stop people accidentally breaking things in that regard, at least in a way that can be expressed a as a general “protect the core” mandate. Specific parts of functionality might end up having specific protections, but we will look to that when we hit them, not as part of this general move.

#18 Updated by Anonymous over 2 years ago

  • Target version changed from 3.x to 3.0.0

#19 Updated by Anonymous over 1 year ago

  • Assignee deleted (Anonymous)

Also available in: Atom PDF