Edgewall Software
Modify

Opened 13 years ago

Closed 11 years ago

Last modified 10 years ago

#10285 closed defect (fixed)

Components in permission_policies should be always loaded

Reported by: Mitar Owned by: Ryan J Ollos
Priority: normal Milestone: 1.0.2
Component: general Version: 0.12.1
Severity: normal Keywords: permission, exception
Cc: mmitar@…, leho@…, dkg@…, ethan.jucovy@…, Steffen Hoffmann, Jun Omae Branch:
Release Notes:

Loading of all permission_policies is enforced by raising an exception if any of the named Components are not found. request_filters, action_controllers and any plugin configuration options utilizing OrderedExtensionOption will raise an exception if one of the named Components is not found.

API Changes:

OrderedExtensionsOption raises a ConfigurationError if one of the named implementations of the interface is not found.

Internal Changes:

Description

Trac should enforce loading of all components in permission_policies and restrain from starting if this is not possible. Otherwise it is possible that some component does not load (because of some system upgrade or any other Python error) and some security enforcing is turned off. This could have grave security implications.

More about this topic in this ticket.

Attachments (2)

patch10285_trac0.12.3.patch (3.5 KB ) - added by Steven R. Loomis <srl@…> 11 years ago.
patch against milestone:0.12.3 implementing MandatoryOrderedExtensionsOption
ConfigurationError.png (22.2 KB ) - added by Ryan J Ollos 11 years ago.

Download all attachments as: .zip

Change History (28)

comment:1 by lkraav <leho@…>, 13 years ago

Cc: leho@… added

comment:2 by Remy Blank, 13 years ago

Keywords: permission added
Milestone: next-minor-0.12.x

We could extend OrderedExtensionOption to do that (or create a MandatoryOrderedExtensionOption subclass). Sounds like a good idea.

OTOH, a whitelist is inherently safer than the blacklist currently implemented in SensitiveTicketsPlugin. Trac supports both policies, and actually, if a permission isn't granted explicitly by an IPermissionPolicy, it's denied. Maybe the plugin should also be modified to use a blacklist?

comment:3 by Ryan J Ollos <ryano@…>, 13 years ago

Cc: ryano@… added

comment:4 by Christian Boos, 13 years ago

Also have a look at the discussion in #5535, where similar concerns about security when permission policies couldn't be loaded were raised.

([OT] where's the (-) Remove this change button? I was about to remove ryano@ from the CC: ;-) )

in reply to:  4 comment:5 by Remy Blank, 13 years ago

Replying to cboos:

([OT] where's the (-) Remove this change button? I was about to remove ryano@ from the CC: ;-) )

Yes, I had the same experience with comment:1. The thing is, it's more difficult to implement than I initially thought, and I haven't had time to dive in lately. Still on my list, though.

in reply to:  2 comment:6 by Mitar, 13 years ago

Replying to rblank:

We could extend OrderedExtensionOption to do that (or create a MandatoryOrderedExtensionOption subclass). Sounds like a good idea.

This really sounds as a good idea.

BTW, why is required Component flag/field not checked anywhere, it seems? I saw it in some official Trac components being set to True.

Trac supports both policies, and actually, if a permission isn't granted explicitly by an IPermissionPolicy, it's denied. Maybe the plugin should also be modified to use a blacklist?

Yes, but the idea is that you augment the existing permission policy components (like official one for tickets). Otherwise you have to replace whole chain with your own permission policy. I believe MandatoryOrderedExtensionOption is a very simple solution which would make things easy to develop with predicting results. So good work/effect ratio in my opinion.

comment:7 by dkg@…, 12 years ago

Cc: dkg@… added

I'm the new maintainer for the SensitiveTicketsPlugin. For this plugin to be able to deny access to certain tickets, I would very much like to see a way to force trac to fail closed if the plugin isn't properly loaded.

I don't know enough about trac internals to advocate for any of the proposed mechanisms above, but i agree that one of them needs to be available for plugins like SensitiveTickets.

If such a mechanism already exists, i'd be happy to learn about it and would use it.

If it doesn't exist, what can i do to help it along?

in reply to:  7 comment:8 by Remy Blank, 12 years ago

Replying to dkg@…:

If it doesn't exist, what can i do to help it along?

It currently doesn't. The best idea so far is still comment:2: enhance OrderedExtensionOption with an optional boolean argument require_all, defaulting to False, and when True, have the option check that all specified components actually exist. This is probably going to require a few changes to ExtensionPoint as well.

comment:9 by Ethan Jucovy <ethan.jucovy@…>, 12 years ago

Cc: ethan.jucovy@… added

comment:10 by fd766@…, 11 years ago

Is there a work around for this problem? I disabled then re-enabled the plugin, now it doesn't work and client is not happy to say the least.

in reply to:  10 comment:11 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

Replying to fd766@…:

Is there a work around for this problem? I disabled then re-enabled the plugin, now it doesn't work and client is not happy to say the least.

I can't make sense of your question in the context of this thread. Please post your question to the MailingList.

comment:12 by Steffen Hoffmann, 11 years ago

Cc: Steffen Hoffmann added

by Steven R. Loomis <srl@…>, 11 years ago

Attachment: patch10285_trac0.12.3.patch added

by Ryan J Ollos, 11 years ago

Attachment: ConfigurationError.png added

comment:13 by Ryan J Ollos, 11 years ago

Cc: ryano@… removed

In #11272, I've stumbled into an implementation of what Rémy suggested in comment:2, modifying OrderedExtensionsOption to raise a ConfigurationError if it can't find the named implementation of the interface. Since ExtensionOption raises an AttributeError when the specified implementation of the interface can't be found, I couldn't see a reason that OrderedExtensionOption should behave differently. However, if we want to preserve the existing behavior for the sake of maintaining consistent behavior of the API, then adding a require parameter seems like a good solution.

In Trac, OrderedExtensionOption is used for: [ticket] workflow, [trac] permission_policies and [trac] request_filters. My starting point for #11272 was trying to catch errors in the configuration in order to make it easier for users to work with AuthzPermissionPolicy, and for the workflow and request_filters options as well I can't see a reason why Trac shouldn't raise an error if one of the extensions isn't found. Silently failing to apply the extension just seems likely to make typos in the option value more difficult to debug, and lead to situations like the one discussed in this ticket where Trac may not be applying the permission policy, request filter or workflow controller that the user is expecting.

The result per all of the testing I've done done is that if the component providing named implementation of the interface is not found, everyone is effectively locked out of Trac through the Web interface:

I'm not entirely confident of the changes, so I appreciate any review and testing that others can provide, as well as challenges to the assumptions I've made about how Trac should behave. The changes can be found in rjollos.git:t11272.

Version 0, edited 11 years ago by Ryan J Ollos (next)

comment:14 by Mitar, 11 years ago

Thanks. I think that there is no real existing behavior which we want to maintain. If some Trac installation has an invalid entry somewhere in OrderedExtensionOption, they will find very fast and if they didn't notice until now, they can then simply remove the option and Trac will work as it was working before the update.

comment:15 by Ryan J Ollos, 11 years ago

While working on #11244, I had the thought that we might want to list the implementations of the interface in the error dialog to help the user in resolving the error. For example,

Cannot find implementations(s) … The enabled implementations of "IPermissionPolicy" are "ReadOnlyWikiPolicy", "LegacyAttachmentPolicy", "PrivateWikiPolicy", "DefaultPermissionPolicy".

comment:16 by Mitar, 11 years ago

Please don't leak internal configuration to a end-user. This can go to the error log.

comment:17 by Ryan J Ollos, 11 years ago

I'll be committing the other changes for #11272 with the exception of [db6a244b/rjollos.git], and will return to continue working on the ticket eventually.

comment:18 by Ryan J Ollos, 11 years ago

Milestone: next-minor-0.12.x1.0.2
Owner: set to Ryan J Ollos
Status: newassigned

The OrderedExtensionsOption is not a Component, so it doesn't have an instance of the logger or the Environment object. Most likely, this is why the ExtensionOption class took the approach of raising an AttributeError that has internal configuration data in the message: tags/trac-1.0.1/trac/config.py@:693-697#L679.

I'm considering some ways to work around this. If we added a log_messsage attribute to the TracError class, the log_message could be added where the exception is raised, and the message could be logged at the places where the exception is trapped in RequestDispatcher.dispatch, such as tags/trac-1.0.1/trac/web/main.py@:256#L251.

comment:19 by Jun Omae, 11 years ago

Cc: Jun Omae added

The changes seem good to me except the following.

  • I think ExtensionOption.__get__ should consistently raise ConfigurationError instead of AttributeError in the same manner with ChoiceOption and OrderedExtensionsOption if settings are wrong.
  • The message of a exception from ExtensionOption and OrderedExtensionsOption should be translated.

comment:20 by Ryan J Ollos, 11 years ago

Changes have been prepared in rjollos.git:t10285, incorporating suggestions from comment:19. If [f8a7f571/jomae.git] from #11284 is committed before this change, then the implementation of assertIsInstance can be removed.

I'm more convinced now that leaking of internal information is a general problem with the config module, so unless there are ideas on how to fix that now, I'll raise a separate ticket for dealing with that issue latter on, possibly pursuing the idea from comment:18. While it is true that OrderedExtensionsOption now shows configuration data to the end users when an exception is raised, this is something that ExtensionOption and other classes are already doing.

in reply to:  20 comment:21 by Ryan J Ollos, 11 years ago

Replying to rjollos:

If [f8a7f571/jomae.git] from #11284 is committed before this change, then the implementation of assertIsInstance can be removed.

Or, I suppose I can just use self.assertTrue(isinstance(...) like in [12054].

comment:22 by Ryan J Ollos, 11 years ago

I must not have run all of the tests previously, because I found just now that several tests were failing with errors such as:

======================================================================
ERROR: test_require (trac.tests.perm.PermissionCacheTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/user/Workspace/t10285/teo-rjollos.git/trac/tests/perm.py", line 183, in test_require
    self.perm.require('TEST_MODIFY')
  File "/home/user/Workspace/t10285/teo-rjollos.git/trac/perm.py", line 578, in require
    if not self._has_permission(action, resource):
  File "/home/user/Workspace/t10285/teo-rjollos.git/trac/perm.py", line 570, in _has_permission
    check_permission(action, perm.username, resource, perm)
  File "/home/user/Workspace/t10285/teo-rjollos.git/trac/perm.py", line 460, in check_permission
    for policy in self.policies:
  File "/home/user/Workspace/t10285/teo-rjollos.git/trac/config.py", line 771, in __get__
    section=self.section, name=self.name))
ConfigurationError: Cannot find implementation(s) of the "IPermissionPolicy" interface named "LegacyAttachmentPolicy".  Please update the option trac.permission_policies in trac.ini.

I've fixed the issues by enabling the components and setting trac.permissions_policies where necessary.

The latest changes can be found in rjollos.git:t10285.2, including the change described in comment:21.

comment:23 by Peter Suter, 11 years ago

Keywords: exception added

comment:24 by Ryan J Ollos, 11 years ago

API Changes: modified (diff)
Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed to 1.0-stable in [12066:12068] and merged to trunk in [12069].

comment:25 by Mitar, 11 years ago

Great! Thanks!

comment:26 by Ryan J Ollos, 10 years ago

See comment:4:ticket:11748 for related discussion about permissions "failing closed".

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.