What's wrong with predicates being "booleanized"
Gustavo Narea
From time to time I get (mostly private) emails from TG users who complained
because there was no way to evaluate predicate checkers implicitly (i.e.,
without passing the environ explicitly) and now complain because, although
it's possible, I strongly discourage it.
So here I'll elaborate on the warning I put on the repoze.what-pylons docs, so
I won't have to repeat the explanation over and over and over again in
different ways, and keep receiving complaints (even from the same people)
nevertheless.
First of all, keep in mind that this has nothing to do with this functionality
being handy or not. I agree it's handy, but that's out of the scope of this
thread. My goal is _not_ to discuss if it's handy or not, but why it should be
avoided.
OK, so here I go...
The warning reads:
"This functionality was implemented by popular demand, but it’s strongly
discouraged by the author of repoze.what because it’s a monkey-patch which
brings serious side-effects when enabled:
1.- Third-party components which handle repoze.what predicates may get
erroneous values after evaluating a predicate checker (even if they don’t use
this functionality!).
2.- If another non-Pylons-based application uses the same monkey-patch and you
mount it on your application (or vice versa), the predicates used by both
application will share the same WSGI environ.
In the two scenarios above, it will lead to serious security flaws. So avoid
it by all means! Use predicate evaluators instead."
I'll start explaining side-effect #2 because I believe it'd be the most
important one for TG2 users. Then I'll explain side-effect #1.
===== Predicates booleanized in multiple repoze.what-powered apps =====
This is how *all* TG2 applications behave:
"""
# Keep in mind that this is *pseudo-code* to illustrate how things work!
...
from repoze.what.predicates import Predicate
from tg import request
...
class TurboGearsApp(object):
def __init__(self, ...):
...
Predicate.__nonzero__ = lambda self: self.is_met(request.environ)
...
def __call__(self, environ, start_response):
...
"""
Through repoze.what-pylons, TurboGears 2 applications set the .__nonzero__
attribute of the base Predicate class, so predicate checkers can be used as
booleans without passing the environ explicitly.
Then it's perfectly possible for other non-Pylons based applications to offer
this handy functionality as well if they use repoze.what, so they'd do:
"""
# Pseudo code here too.
...
from repoze.what.predicates import Predicate
from myframework import environ # <-- Assuming they use thread-locals too
...
class MyFrameworkApp(object):
"""
This doesn't "have" to come from a framework. It can be a raw WSGI app.
"""
def __init__(self, ...):
...
Predicate.__nonzero__ = lambda self: self.is_met(environ)
...
def __call__(self, environ, start_response):
...
"""
In both WSGI applications, the __init__ method will be called just once and
its object will be shared among threads.
Now, the problem arises when both applications are used together; this is, one
mounted on the other. When they're used independently one another, there's no
problem at all.
When they are mounted one on the other, Predicate.__nonzero__ will equal:
lambda self: self.is_met(myframework.environ)
or,
lambda self: self.is_met(tg.request.environ)
forever, in every thread, in every request.
How come? When MyFrameworkApp.__init__ is called first and
TurboGearsApp.__init__ is called next, Predicate.__nonzero__ takes the
following values, respectively:
lambda self: self.is_met(myframework.environ)
lambda self: self.is_met(tg.request.environ)
Likewise, when TurboGears.__init__ is called first and MyFrameworkApp.__init__
is called next, Predicate.__nonzero__ takes the following values,
respectively:
lambda self: self.is_met(tg.request.environ)
lambda self: self.is_met(myframework.environ)
It will certainly cause problems. In the best case scenario, an exception will
be raised and security won't be compromised. In the worse one, the environ
will have a default value and thus security could be compromised. Either way,
the application will be broken.
And it's a big mistake to believe that it would be solved if repoze.what
itself set Predicate.__nonzero__. It won't change anything at all. It'd be the
same module-level change, just that instead of being dynamic (like the monkey-
patch), it'd be static.
And before somebody argues "But it's not a common scenario!": So what? The
point is that it's possible. There's nothing weird in having two repoze.what
powered applications, one with TG2 and the other non-TG2 based.
===== Third party components =====
Now, this will be quite annoying for people who write plugins/routines that
deal with predicate checkers, and pretty dangerous for TG2 users.
Predicate checkers don't act as booleans, since the only officially supported
way to evaluate them is to pass the WSGI environ to the
.is_met()/.check_authorization() methods. But then we have "the TurboGears
way" to evaluate predicate checkers as plain old bool.
So, someone who wrote something that deals with predicate checkers could have
this:
if predicate:
# There's a predicate defined
else:
# There's no predicate defined (predicate is None)
But if a TurboGears app is being used, it'd actually mean:
if predicate:
# The predicate exists AND evaluates to True
else:
# There's no predicate OR it exists but is not met
Which is horribly dangerous for TG2 users. It'd be a critical security flaw.
Even I had to update repoze.what itself to take into account "the TurboGears
way" to evaluate predicates. But non-TG2 users who write stuff to deal with
predicates may not be aware of this and TG2 users who use such software will
get in a trouble... Until they find it and ask the maintainer to update the
original TG2-independent code.
=====
So, to sum up:
1.- Predicate checkers won't ever act as bool officially (i.e., with
Predicate.__nonzero__ defined by default), unless somebody comes up with a
solution to side-effect #2.
2.- TG2 users are *strongly* encouraged to disable this monkey-patch, unless
they are 500% sure that their applications *won't* *ever*:
A.- Mount or be mounted on another repoze.what-powered application which
is not based on Pylons/TG2.
B.- Use third party, TG2-independent tools which handle predicate
checkers.
Does anything need further clarification?
Cheers.
--
Gustavo Narea <xri://=Gustavo>.
| Tech blog: =Gustavo/(+blog)/tech ~ About me: =Gustavo/about |
Jorge Vargas
Thank you I finally understand your issue with it. And I do see this
as a valid concern.
Although I still think the warning at
http://code.gustavonarea.net/repoze.what-pylons/Manual/Misc.html#boolean-predicates
is over the top. And I think this explanation should go in there
somehow.
Specially be calling it a monkey-patch, it is indeed a monkey-patch
but by choice because of #1, and I won't argue with you on that. But
at least at this point in time both cases of #2 are really minor. At
least I don't know any installation that currently exposes any of
those.
Diez B. Roggisch
>> erroneous values after evaluating a predicate checker (even if they don’t use
>> this functionality!).
>> 2.- If another non-Pylons-based application uses the same monkey-patch and you
>> mount it on your application (or vice versa), the predicates used by both
>> application will share the same WSGI environ.
Then all that is needed are three functions in repoze.what,
push_environ
get_environ
pop_environ
that provide access to a thread-local, stacked object. AFAIK Paster has
something to offer for that already, but I'm not 100%sure.
If a wsgi-app gains control, it pushes it's environ. If it is left, it
pops it. The __nonzero__ uses get_environ to access it.
>> ===== Third party components =====
>>
>> Now, this will be quite annoying for people who write plugins/routines that
>> deal with predicate checkers, and pretty dangerous for TG2 users.
>>
>> Predicate checkers don't act as booleans, since the only officially supported
>> way to evaluate them is to pass the WSGI environ to the
>> .is_met()/.check_authorization() methods. But then we have "the TurboGears
>> way" to evaluate predicate checkers as plain old bool.
>>
>> So, someone who wrote something that deals with predicate checkers could have
>> this:
>> if predicate:
>> # There's a predicate defined
>> else:
>> # There's no predicate defined (predicate is None)
>>
>> But if a TurboGears app is being used, it'd actually mean:
>> if predicate:
>> # The predicate exists AND evaluates to True
>> else:
>> # There's no predicate OR it exists but is not met
>>
>> Which is horribly dangerous for TG2 users. It'd be a critical security flaw.
>>
>> Even I had to update repoze.what itself to take into account "the TurboGears
>> way" to evaluate predicates. But non-TG2 users who write stuff to deal with
>> predicates may not be aware of this and TG2 users who use such software will
>> get in a trouble... Until they find it and ask the maintainer to update the
>> original TG2-independent code.
I don't find this compelling. The whole point about this
__nonzero__-issue is ease of use for the *users*. People who do write
predicate-checkers and thus dive deeper into the functionality need to
take care a bit more - but they need to do that anyway.
And if you'd offer a function like
check_for_predicate
that is prominently advertised with the above scenario, I guess they
will get it.
>>
>> =====
>>
>> So, to sum up:
>>
>> 1.- Predicate checkers won't ever act as bool officially (i.e., with
>> Predicate.__nonzero__ defined by default), unless somebody comes up with a
>> solution to side-effect #2.
>> 2.- TG2 users are *strongly* encouraged to disable this monkey-patch, unless
>> they are 500% sure that their applications *won't* *ever*:
>> A.- Mount or be mounted on another repoze.what-powered application which
>> is not based on Pylons/TG2.
Which is the >95% usecase. IMHO you should gear towards satisfying those
users, and worry about the others *if* they happen to complain.
But then, that's your decision of course.
Diez
Diez B. Roggisch
Just a thought. You argue above with security-flaws creeping into code if
__nonzero__ is used.
I'd like to make the point for the opposite - *not* evaluating the predicate
automatically via __nonzero__ creates one.
Consider the following situation:
<div py:if="in_group('admin')"> Some super secret stuff </div>
Now without the patch, this gets evaluated to a predicate - in other words an
object which is not amongst the objects python automatically treats
as "False".
So unless you *do* provide a __nonzero__-method on Predicates that always
yields False, users are likely to create security-flaws by forgetting the
evaluator. But then the reasoning of #1 falls flat on it's face as well.
Diez
Gustavo Narea
Diez said:
> Just a thought. You argue above with security-flaws creeping into code if
> __nonzero__ is used.
>
> I'd like to make the point for the opposite - not evaluating the predicate
> automatically via __nonzero__ creates one.
>
> Consider the following situation:
>
> <div py:if="in_group('admin')"> Some super secret stuff </div>
>
> Now without the patch, this gets evaluated to a predicate - in other words
> an object which is not amongst the objects python automatically treats as
> "False".
>
> So unless you do provide a __nonzero__-method on Predicates that always
> yields False, users are likely to create security-flaws by forgetting the
> evaluator. But then the reasoning of #1 falls flat on it's face as well.
There's no bug/flaw nowhere. The repoze.what docs clearly emphasize that the
only way to evaluate predicates is by doing so explicitly with the
.is_met()/.check_authorization() methods:
http://what.repoze.org/docs/1.x/Manual/Predicates/Evaluating.html
It can only be a flaw if it's a dumb developer who doesn't RTFM and doesn't
try the code s/he writes either. And then, the bug would be the programmer
himself.
On the other hand, having a default evaluation result of False can be
dangerous equally:
"""
<div py:choose="">
<span py:when="is_anonymous()">Nothing to see here</span>
<span py:otherwise="">Secret message for registered users</span>
</div>
<div py:choose="">
<span py:when="not_anonymous()">Secret message for registered users</span>
<span py:otherwise="">Nothing to see here</span>
</div>
Diez B. Roggisch
> Hello.
>
> Diez said:
> > Just a thought. You argue above with security-flaws creeping into code if
> > __nonzero__ is used.
> >
> > I'd like to make the point for the opposite - not evaluating the
> > predicate automatically via __nonzero__ creates one.
> >
> > Consider the following situation:
> >
> > <div py:if="in_group('admin')"> Some super secret stuff </div>
> >
> > Now without the patch, this gets evaluated to a predicate - in other
> > words an object which is not amongst the objects python automatically
> > treats as "False".
> >
> > So unless you do provide a __nonzero__-method on Predicates that always
> > yields False, users are likely to create security-flaws by forgetting the
> > evaluator. But then the reasoning of #1 falls flat on it's face as well.
>
> There's no bug/flaw nowhere. The repoze.what docs clearly emphasize that
> the only way to evaluate predicates is by doing so explicitly with the
> .is_met()/.check_authorization() methods:
> http://what.repoze.org/docs/1.x/Manual/Predicates/Evaluating.html
>
> It can only be a flaw if it's a dumb developer who doesn't RTFM and doesn't
> try the code s/he writes either. And then, the bug would be the programmer
> himself.
Ahh, ok, so a programmer who does an error in the 95% use case is an ignorant
idiot, whereas a programmer that dives deep into the auth-stuff, writes
predicates handlers, diggs into WSGI-app deployment and combines apps, and
even has the need for *potentially* differing environments can't be bothered
to do so, and instead is to be protected from his or her unwitting actions?
>
> On the other hand, having a default evaluation result of False can be
> dangerous equally:
I'm well aware of that and didn't suggest that it would be a good idea to do
that, merely that security flaws can be introduced both with or without
__nonzero__, which IMHO makse a strong case for using __nonzero__ in the way
it is used by the patch.
Sorry Gustavo, but your argumentation sounds purely defensive and seems to try
and rationalize a design-decision based on personal taste. Which is all fine
and good, it's your package.
But it is apparent that a lot of people think different and want to provide an
authorization-implementation that is less JDEE-esk but more "user"-friendly,
where user is the average TG2-developer, and not some auth-junkie.
So I find it a pity that you don't consider serving that need. But again,
that's totally up to you.
Diez
Gustavo Narea
Jorge said:
> Thank you I finally understand your issue with it. And I do see this
> as a valid concern.
Cool :)
> Although I still think the warning at
> http://code.gustavonarea.net/repoze.what-pylons/Manual/Misc.html#boolean-pr
>edicates is over the top.
Well, now that I reread it, it seems like I was wearing my alarmist hat when I
wrote the last paragraph. I have to rephrase it with the hat off. ;-)
> And I think this explanation should go in there somehow.
I think you're right. But I don't want to make it longer, so I'll just link
here.
> Specially be calling it a monkey-patch, it is indeed a monkey-patch
> but by choice because of #1, and I won't argue with you on that. But
> at least at this point in time both cases of #2 are really minor. At
> least I don't know any installation that currently exposes any of
> those.
Yes, I agree. I will rephrase it all to explain that it'd be safe for those
applications which won't be in either scenario.
Thanks!
Gustavo Narea
Diez said:
> > There's no bug/flaw nowhere. The repoze.what docs clearly emphasize that
> > the only way to evaluate predicates is by doing so explicitly with the
> > .is_met()/.check_authorization() methods:
> > http://what.repoze.org/docs/1.x/Manual/Predicates/Evaluating.html
> >
> > It can only be a flaw if it's a dumb developer who doesn't RTFM and
> > doesn't try the code s/he writes either. And then, the bug would be the
> > programmer himself.
>
> Ahh, ok, so a programmer who does an error in the 95% use case is an
> ignorant idiot, whereas a programmer that dives deep into the auth-stuff,
> writes predicates handlers, diggs into WSGI-app deployment and combines
> apps, and even has the need for *potentially* differing environments can't
> be bothered to do so, and instead is to be protected from his or her
> unwitting actions?
Let me repeat what I said in other words, because it seems like I was not
clear enough:
If a programmer starts coding without having read the relevant documentation
of the technology he's using, and in addition to that, he doesn't even try
what he has written (until deployment), I'd definitely call him dump. *
Actually I wouldn't even call him "dump programmer". "non-programmer" sounds
more accurate.
Should this happen 95% of the time, I'd love it to be confirmed ASAP, so I can
stop using airplanes, my bank's web site and everything else that relies on
software! ;-)
And I've never called dumb someone with simple auth needs. I'm just trying to
get a good compromise between simple and advanced setups.
* For your information, "dumb" does not equal "*ignorant* idiot". Ignorance is
totally unrelated, but, well, I agree it's nicer to invent adjectives when one
is refuting somebody else's statements.
> > On the other hand, having a default evaluation result of False can be
> > dangerous equally:
>
> I'm well aware of that and didn't suggest that it would be a good idea to
> do that, merely that security flaws can be introduced both with or without
> __nonzero__, which IMHO makse a strong case for using __nonzero__ in the
> way it is used by the patch.
1.- That wouldn't be better or worse, it'd be the exact same thing but the
other way around.
2.- Anyway, I don't know why we're talking about this, since
py:if="is_anonymous()" would evaluate as you expect, as well as any other
predicate.
> Sorry Gustavo, but your argumentation sounds purely defensive and seems to
> try and rationalize a design-decision based on personal taste. Which is all
> fine and good, it's your package.
>
> But it is apparent that a lot of people think different and want to provide
> an authorization-implementation that is less JDEE-esk but more
> "user"-friendly, where user is the average TG2-developer, and not some
> auth-junkie.
>
> So I find it a pity that you don't consider serving that need. But again,
> that's totally up to you.
Only someone who has *no* *clue* about the development of repoze.what and its
plugins can call me that way.
That I didn't implement a backwards-incompatible proposal you made some time
ago, and that I don't like the two proposals to workaround the evaluation of
predicates, doesn't mean that repoze.what is for "the auth-junkie".
I don't want to believe those statements are BS, so please get me some proofs.
Right now I cannot think of another suggestion rejected by me, but hey, I'm
sure you'll find one (at the very least!) in these links:
http://www.google.com/search?q=site:lists.repoze.org+"Gustavo+Narea"+-checkins
http://groups.google.com/group/turbogears/search?group=turbogears&q="Gustavo+Narea"
http://groups.google.com/group/turbogears/search?group=turbogears-
trunk&q="Gustavo+Narea"
And you know, if I'm doing it that bad and I am so close-minded, and there's
someone better qualified, it can always be forked.
Cheers,