New patches attached with some significant changes. After much
discussion, we decided that, rather than implement a facility for
identifying "unresolved groups" for the user, we would instead perform a
group lookup during the initial pull-down of the HBAC rules. This way,
we know that all groups that are relevant to HBAC rules are available on
the system. So if a user is a member of a group that is not resolvable,
we don't have to worry that it might match a DENY rule.
On Fri, 2011-06-10 at 21:31 +0200, Jakub Hrozek wrote:
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.
This patch is unchanged.
> Patch 0002: Add a helper function to convert a list of
ldb_messages to
> sysdb_attrs.
>
Ack
This patch is unchanged.
> 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.
Good catch. Fixed.
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've removed the requirement on talloc in the evaluator, so it's also
been removed from the spec file. (But you're right, the explicit
Requires: would have been unnecessary).
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?
It's just a habit I've gotten into when dealing with loops based on an
array index.
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.
That was in the old code and I didn't want to rock the boat, but you're
right. That's wasteful. If the hierarchy is ever wrong here it should be
a bug. Fixed.
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.
ipa_hbac_save_list() was copied verbatim from hbac_save_list() (Removed
in Patch 5). I didn't change it at all. I'd rather leave it in for the
future. At some point we'll probably want to optimize this lookup to use
LastUSN to just get the delta of changes to the rules, hosts and
services, rather than enumerating them fully.
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.
Fixed.
In hbac_service_attrs_to_rule() a comment says "/* First check if this
is a specific host */". I think it should say "..specific service".
Copy-paste error. Fixed.
In ipa_hbac_rule_info_send(), please use IPA_HBAC_RULE instead of
hardcoding ipaHBACRule.
Fixed.
nitpick - in hbac_ctx_to_eval_request() the block after "if
(pd->rhost
== NULL || pd->rhost[0] == '\0') {" is indented one level too deep.
Fixed.
A comment in hbac_eval_host_element() says "/* Find the service
groups
*/", it should probably say "host groups".
Copy-paste error. Fixed.
Patch 5:
Ack
Patch is unchanged.
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()?
This code was rewritten in patch 8. It should be fine now.
Patch 7:
Nack, the new option belongs to SSSDConfigAPI.py, too.
Added.
New Patches:
I added these as new patches to make the review easier.
Patch 0008: Adds a new routine to update any expired or missing group
entries in the cache that are relevant to the HBAC rules.
I'm "cheating" a little bit here for a significant performance increase.
Because the originalDN attribute is a DN, it cannot be made into a
search filter normally, so in order to look these up it would require a
series of BASE searches. Then I'd need to gather the results and write a
new cache update routine.
What I did instead was abuse the fact that IPA has a well-known
construct for usergroups in its tree. I parse the originalDN to see if
it's a group and extract the group name. I then use this to construct a
single search request with all of the missing or expired groups and send
it through sdap_get_groups_send(), which has the advantage of already
being designed to handle multiple (and recursive) groups and cluster
them for a single transaction to save them.
Admittedly, this will need to be modified if we ever opt to support the
HBAC provider against non-IPA LDAP servers, but for now the performance
gains are enormous.
Patch 0009: While fixing these, I realized that if there were no
applicable rules for the system, it was returning PAM_SYSTEM_ERR. It
makes more sense to return PAM_PERM_DENIED and purge any existing rules
from the cache.