PAM plugin vs post op processing. New return code proposal...

[Mark Reynolds]

Hi Everyone,

There is an issue with the PAM plugin, that when it performs a 
successful bind we actually return error 1 to plugins_call_func(), which 
essentially causes the abort of the all plugin processing:  the rest of 
pre-op, the backend call, and all of post-op.  PAM has completed the 
bind and already returned the result, so it returns 1 to stop the DS 
from doing the rest of bind op.  Makes sense...

However, with the Account Policy plugin, when tracking the "last bind 
time", binding thru PAM won't update the entry, even though the bind was 
successful.  This is because the successful PAM bind essentially aborted 
all the pre-op and post-op plugins.  I feel that we should still call 
the post op plugins in this scenario.  The pre-op plugins should still 
be aborted, because the operation was already completed - there's 
nothing to reject at that point.

So to get around plugins like this, I am proposing a new plugin pre-op 
return code(either use 1, or -2).  This return code implies that the 
operation was fully completed, and that we should also process the post 
op plugins - but do not send the operation to the backend.

So to the recap, the new return code says the operation was fully 
completed by the plugin, but we still want to process just the post-op 
plugins.

I know this will impact documentation, there might be unforeseen issues, 
and this is also a rare situation(only PAM plugin seems to behave like 
this).  Saying that, I still think its the valid solution to this type 
of problem.  It could also allow future plugins for be more 
versatile/robust.

Please let me know your thoughts.

Thanks,
Mark

[Rich Megginson]

On 02/01/2012 02:16 PM, Mark Reynolds wrote:
>  Hi Everyone,
>
>  There is an issue with the PAM plugin, that when it performs a 
>  successful bind we actually return error 1 to plugins_call_func(), 
>  which essentially causes the abort of the all plugin processing:  the 
>  rest of pre-op, the backend call, and all of post-op.  PAM has 
>  completed the bind and already returned the result, so it returns 1 to 
>  stop the DS from doing the rest of bind op.  Makes sense... 
Where is the code that
calls the preop?
>  However, with the Account Policy plugin, when tracking the "last bind 
>  time", binding thru PAM won't update the entry, even though the bind 
>  was successful.  This is because the successful PAM bind essentially 
>  aborted all the pre-op and post-op plugins.  I feel that we should 
>  still call the post op plugins in this scenario.  The pre-op plugins 
>  should still be aborted, because the operation was already completed - 
>  there's nothing to reject at that point.
>
>  So to get around plugins like this, I am proposing a new plugin pre-op 
>  return code(either use 1, or -2).  This return code implies that the 
>  operation was fully completed, and that we should also process the 
>  post op plugins - but do not send the operation to the backend.
>
>  So to the recap, the new return code says the operation was fully 
>  completed by the plugin, but we still want to process just the post-op 
>  plugins.
>
>  I know this will impact documentation, there might be unforeseen 
>  issues, and this is also a rare situation(only PAM plugin seems to 
>  behave like this).  Saying that, I still think its the valid solution 
>  to this type of problem.  It could also allow future plugins for be 
>  more versatile/robust.
>
>  Please let me know your thoughts. 
I don't think we need a new return code - I
just think we need to

diff --git a/ldap/servers/slapd/bind.c b/ldap/servers/slapd/bind.c
index 1d860b6..bd7df3d 100644
--- a/ldap/servers/slapd/bind.c
+++ b/ldap/servers/slapd/bind.c
@@ -796,6 +796,10 @@ do_bind( Slapi_PBlock *pb )

              slapi_pblock_set( pb, SLAPI_PLUGIN_OPRETURN, &rc );
              plugin_call_plugins( pb, SLAPI_PLUGIN_POST_BIND_FN );
+        } else {
+            /* even if the pre-op plugins returned an error, we still need
+               to call the post-op plugins */
+            plugin_call_plugins( pb, SLAPI_PLUGIN_POST_BIND_FN );
          }
      } else {
          send_ldap_result( pb, LDAP_UNWILLING_TO_PERFORM, NULL,

Looking at bind.c - there is a lot of special case code that makes sure 
to call the POST_BIND plugins in various error cases.  This seems like 
just another error case that we should handle by calling the POST_BIND 
plugins.
>  Thanks,
>  Mark
>
>
>
>
>
>  -- 
>  389-devel mailing list
>  389-devel(a)lists.fedoraproject.org
>  https://admin.fedoraproject.org/mailman/listinfo/389-devel

[Mark Reynolds]

On 02/01/2012 04:28 PM, Rich Megginson wrote:
>  On 02/01/2012 02:16 PM, Mark Reynolds wrote:
> > Hi Everyone,
> >
> > There is an issue with the PAM plugin, that when it performs a 
> > successful bind we actually return error 1 to plugins_call_func(), 
> > which essentially causes the abort of the all plugin processing:  the 
> > rest of pre-op, the backend call, and all of post-op.  PAM has 
> > completed the bind and already returned the result, so it returns 1 
> > to stop the DS from doing the rest of bind op.  Makes sense...
>  Where is the code that calls the preop?
> >
> > However, with the Account Policy plugin, when tracking the "last bind 
> > time", binding thru PAM won't update the entry, even though the bind 
> > was successful.  This is because the successful PAM bind essentially 
> > aborted all the pre-op and post-op plugins.  I feel that we should 
> > still call the post op plugins in this scenario.  The pre-op plugins 
> > should still be aborted, because the operation was already completed 
> > - there's nothing to reject at that point.
> >
> > So to get around plugins like this, I am proposing a new plugin 
> > pre-op return code(either use 1, or -2).  This return code implies 
> > that the operation was fully completed, and that we should also 
> > process the post op plugins - but do not send the operation to the 
> > backend.
> >
> > So to the recap, the new return code says the operation was fully 
> > completed by the plugin, but we still want to process just the 
> > post-op plugins.
> >
> > I know this will impact documentation, there might be unforeseen 
> > issues, and this is also a rare situation(only PAM plugin seems to 
> > behave like this).  Saying that, I still think its the valid solution 
> > to this type of problem.  It could also allow future plugins for be 
> > more versatile/robust.
> >
> > Please let me know your thoughts.
>  I don't think we need a new return code - I just think we need to
>
>  diff --git a/ldap/servers/slapd/bind.c b/ldap/servers/slapd/bind.c
>  index 1d860b6..bd7df3d 100644
>  --- a/ldap/servers/slapd/bind.c
>  +++ b/ldap/servers/slapd/bind.c
>  @@ -796,6 +796,10 @@ do_bind( Slapi_PBlock *pb )
>
>               slapi_pblock_set( pb, SLAPI_PLUGIN_OPRETURN, &rc );
>               plugin_call_plugins( pb, SLAPI_PLUGIN_POST_BIND_FN );
>  +        } else {
>  +            /* even if the pre-op plugins returned an error, we still 
>  need
>  +               to call the post-op plugins */
>  +            plugin_call_plugins( pb, SLAPI_PLUGIN_POST_BIND_FN );
>           }
>       } else {
>           send_ldap_result( pb, LDAP_UNWILLING_TO_PERFORM, NULL, 
I thought about
this, but I thought we needed a new code because isn't 
it by design: if pre-op fails, it should all fail?  PAM really isn't 
failing, it just doesn't have a better mechanism to handle its behaviour.

The above code would work fine, so maybe this could be a discussion for 
a later time.  I was just looking for a more "global" solution.  So, I 
will use this approach for now so I can get the bug resolved.  If 
there's time in the future maybe we can come back to this.  Like I said 
it's not something that is needed, but it would allow for more creative 
plugins :-)

Thanks,
Mark
>  Looking at bind.c - there is a lot of special case code that makes 
>  sure to call the POST_BIND plugins in various error cases.  This seems 
>  like just another error case that we should handle by calling the 
>  POST_BIND plugins.
> >
> > Thanks,
> > Mark
> >
> >
> >
> >
> >
> > -- 
> > 389-devel mailing list
> > 389-devel(a)lists.fedoraproject.org
> > https://admin.fedoraproject.org/mailman/listinfo/389-devel
>

[Rich Megginson]

On 02/01/2012 02:48 PM, Mark Reynolds wrote:
>  On 02/01/2012 04:28 PM, Rich Megginson wrote:
> > On 02/01/2012 02:16 PM, Mark Reynolds wrote:
> >> Hi Everyone,
> >>
> >> There is an issue with the PAM plugin, that when it performs a 
> >> successful bind we actually return error 1 to plugins_call_func(), 
> >> which essentially causes the abort of the all plugin processing:  
> >> the rest of pre-op, the backend call, and all of post-op.  PAM has 
> >> completed the bind and already returned the result, so it returns 1 
> >> to stop the DS from doing the rest of bind op.  Makes sense...
> > Where is the code that calls the preop?
> >>
> >> However, with the Account Policy plugin, when tracking the "last 
> >> bind time", binding thru PAM won't update the entry, even though the 
> >> bind was successful.  This is because the successful PAM bind 
> >> essentially aborted all the pre-op and post-op plugins.  I feel that 
> >> we should still call the post op plugins in this scenario.  The 
> >> pre-op plugins should still be aborted, because the operation was 
> >> already completed - there's nothing to reject at that point.
> >>
> >> So to get around plugins like this, I am proposing a new plugin 
> >> pre-op return code(either use 1, or -2).  This return code implies 
> >> that the operation was fully completed, and that we should also 
> >> process the post op plugins - but do not send the operation to the 
> >> backend.
> >>
> >> So to the recap, the new return code says the operation was fully 
> >> completed by the plugin, but we still want to process just the 
> >> post-op plugins.
> >>
> >> I know this will impact documentation, there might be unforeseen 
> >> issues, and this is also a rare situation(only PAM plugin seems to 
> >> behave like this).  Saying that, I still think its the valid 
> >> solution to this type of problem.  It could also allow future 
> >> plugins for be more versatile/robust.
> >>
> >> Please let me know your thoughts.
> > I don't think we need a new return code - I just think we need to
> >
> > diff --git a/ldap/servers/slapd/bind.c b/ldap/servers/slapd/bind.c
> > index 1d860b6..bd7df3d 100644
> > --- a/ldap/servers/slapd/bind.c
> > +++ b/ldap/servers/slapd/bind.c
> > @@ -796,6 +796,10 @@ do_bind( Slapi_PBlock *pb )
> >
> >              slapi_pblock_set( pb, SLAPI_PLUGIN_OPRETURN, &rc );
> >              plugin_call_plugins( pb, SLAPI_PLUGIN_POST_BIND_FN );
> > +        } else {
> > +            /* even if the pre-op plugins returned an error, we 
> > still need
> > +               to call the post-op plugins */
> > +            plugin_call_plugins( pb, SLAPI_PLUGIN_POST_BIND_FN );
> >          }
> >      } else {
> >          send_ldap_result( pb, LDAP_UNWILLING_TO_PERFORM, NULL,
>  I thought about this, but I thought we needed a new code because isn't 
>  it by design: if pre-op fails, it should all fail? 
I don't think so.  In every
other case in bind.c, when something fails, 
the POST_BIND functions are called.  I don't see why this particular 
case should be any different.  IMHO a POST_BIND plugin will always want 
to know if the bind succeeded or failed for any reason.  Note that IPA 
has a POST_BIND plugin - ipa_lockout - which will want to be called in 
this case.

>  PAM really isn't failing, it just doesn't have a better
> mechanism to 
>  handle its behaviour.
>
>  The above code would work fine, so maybe this could be a discussion 
>  for a later time.  I was just looking for a more "global" solution.  
>  So, I will use this approach for now so I can get the bug resolved.  
>  If there's time in the future maybe we can come back to this.  Like I 
>  said it's not something that is needed, but it would allow for more 
>  creative plugins :-)
>
>  Thanks,
>  Mark
> >
> > Looking at bind.c - there is a lot of special case code that makes 
> > sure to call the POST_BIND plugins in various error cases.  This 
> > seems like just another error case that we should handle by calling 
> > the POST_BIND plugins.
> >>
> >> Thanks,
> >> Mark
> >>
> >>
> >>
> >>
> >>
> >> -- 
> >> 389-devel mailing list
> >> 389-devel(a)lists.fedoraproject.org
> >> https://admin.fedoraproject.org/mailman/listinfo/389-devel
> >
>