Components in permission_policies should be always loaded

[Mitar]

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.

[Remy Blank]

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?

[Christian Boos]

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: ;-) )

[Remy Blank]

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.

[Mitar]

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.

[dkg@…]

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?

[Remy Blank]

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.

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

[Ryan J Ollos <ryan.j.ollos@…>]

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.

[Steven R. Loomis <srl@…>]

patch against milestone:0.12.3 implementing MandatoryOrderedExtensionsOption

[Ryan J Ollos]

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] action_controllers​, [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.

[Mitar]

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.

[Ryan J Ollos]

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

[Mitar]

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

[Ryan J Ollos]

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.

[Ryan J Ollos]

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

[Jun Omae]

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.

[Ryan J Ollos]

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.

[Ryan J Ollos]

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

[Ryan J Ollos]

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.

[Ryan J Ollos]

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

[Mitar]

Great! Thanks!

[Ryan J Ollos]

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