Re: [389-devel] PAM plugin vs post op processing. New return code proposal...
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
>