On 06/07/2011 02:11 PM, Stephen Gallagher wrote:
> On Tue, 2011-06-07 at 10:41 +0200, Jakub Hrozek wrote:
>> On 06/07/2011 04:59 AM, Stephen Gallagher wrote:
>>> While attempting to extract the SSSD's HBAC rule evaluation into a
>>> common library, I discovered several shortcomings. Some were related to
>>> extracting the library, others were related to the rule evaluation
>>> itself.
>>>
>>> Related to extracting the library, I needed for the code to be able to
>>> identify and resolve without having direct access to LDAP or the LDB.
>>> This meant I could not rely on the memberOf objects to be valid.
>>>
>>> As for the rule evaluation, I discovered that we were only caching for
>>> offline use those remote hosts that had previously attempted to log in.
>>> This meant that if the IPA server was unreachable, valid users would be
>>> unable to log in from some valid machines.
>>>
>>> After careful consideration (and a few false starts), I decided that a
>>> total rewrite was necessary. So here it is.
>>>
>>> The major changes are these:
>>>
>>> 1) We have moved to always performing a full enumeration of known IPA
>>> hosts and HBAC services. This actually results in a net reduction in
>>> round-trips to the LDAP server, which was a primary bottleneck.
>>> Additionally, this means that we also have a full understanding when
>>> offline as to whether a remote host is permitted access or not.
>>>
>>> 2) I've added a configurable timeout for hbac rule refreshes to the LDAP
>>> server. Previously, if we were online, EVERY pam_account() request would
>>> result in at least one LDAP round-trip. By default, we'll only refresh
>>> the HBAC rules every five seconds now.
>>>
>>> 3) I broke the acquisition and processing of the rules, hosts and
>>> services into separate C files, as ipa_access.c was getting to be much
>>> to large to manage.
>>>
>>> The overall view of the code (when online) is this:
>>> A request comes in, we call to LDAP to get, in order, the set of known
>>> hosts, the set of known PAM services and then finally the set of
>>> *enabled* rules with the current ipa_hostname as the targethost.
>>>
>>> And now, the patches:
>>>
>>> Patch 0001: The new standalone evaluator library. It's very simple at
>>> the moment (but it will become much more complex in the near future when
>>> timerule evaluation is added). Jakub, Rob and I agreed on the interface
>>> and Jakub is creating Python bindings for it.
>>>
>>
>> I'll rebase the bindings on top of these patches and send to the list.
>>
>>> Patch 0002: This is the meat of the new IPA access backend. All of the
>>> helper functions necessary for looking up, storing and converting the
>>> hosts, rules and services are in this patch.
>>>
>>> Patch 0003: This is not really a patch as much as it is a trash can. I
>>> split this from Patch 0004 to make the latter more reviewable. Patch
>>> 0003 contains only the deletions of the old access provider
>>> implementation.
>>>
>>> Patch 0004: Adds the new IPA access provider, taking advantage of the
>>> helper functions from Patch 0002.
>>>
>>> Patch 0005: Adds a refresh timeout to reduce round-trips to IPA while
>>> online.
>>>
>>>
>>
>> Nack, msgs2attrs_array() is not defined:
>>
>> src/providers/ipa/ipa_access.c: In function 'hbac_get_cached_rules':
>> src/providers/ipa/ipa_access.c:589:5: error: implicit declaration of
>> function 'msgs2attrs_array' [-Werror=implicit-function-declaration]
>> cc1: some warnings being treated as errors
>
>
> Whoops, I forgot I reordered some of my patches. I missed two at the
> beginning.
>
I know the patches are going to change, but at least some of the
comments below would apply to the next iteration as well.
> Patch 0001: Retrieve the group name as well as the GID during
> sysdb_initgroups() (used later for getting the user memberships of a
> user for evaluating the rules)
>
Ack provided that the patch is still valid in the PAC context - that
wasn't clear to me from the IRC discussion between you and Simo.
> Patch 0002: Add a helper function to convert a list of ldb_messages to
> sysdb_attrs.
>
Ack
> The other patches are the same as the previous email, but numbered +2.
>
Patch 3:
Nack,
in sssd.spec.in, "%defattr(-,root,root-)" is missing a coma between the
third and fourth paramater.
Is it necessary to provide an explicit Requires: on libtalloc? Usually
RPM is good in guessing the dependencies automatically. This might be
moot if you're getting rid of talloc as mentioned on IRC.
I'm curious about using size_t for loop counter in
hbac_evaluate_element() - it's certainly not wrong, but why not simply
use unsigned long?
Patch 4:
In overall, the code reads really good.
Why do you talloc_steal the items of the array sdap_get_generic_recv()
returns? The memory hierarchy seems OK to me in sdap_get_generic_*
functions.
What is the rationale of ever passing delete_subdir=false to
ipa_hbac_save_list()? When you're sure you're just priming the sysdb? I
don't see it ever used in the code.
In ipa_hbac_save_list() the, error message "IPA_UNIQUE_ID not found"
should probably say "naming attribute not found", it does not have to be
IPA unique ID.
In hbac_service_attrs_to_rule() a comment says "/* First check if this
is a specific host */". I think it should say "..specific service".
In ipa_hbac_rule_info_send(), please use IPA_HBAC_RULE instead of
hardcoding ipaHBACRule.
nitpick - in hbac_ctx_to_eval_request() the block after "if (pd->rhost
== NULL || pd->rhost[0] == '\0') {" is indented one level too deep.
A comment in hbac_eval_host_element() says "/* Find the service groups
*/", it should probably say "host groups".
Patch 5:
Ack
Patch 6:
just nitpicking, but can you add a blank line between variable
declarations and the ipa_hbac_rule_info_recv() call in hbac_sysdb_save()?
Patch 7:
Nack, the new option belongs to SSSDConfigAPI.py, too.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
