On Fri, Jan 08, 2016 at 11:27:41AM +0100, Pavel Březina wrote:
On 01/07/2016 02:01 PM, Sumit Bose wrote:
>On Tue, Jan 05, 2016 at 11:41:45AM +0100, Pavel Březina wrote:
>>On 12/18/2015 02:36 PM, Pavel Březina wrote:
>>>Hello list,
>>>the support of IPA sudo schema is almost complete. The only thing that
>>>remains is to finish smart refresh implementation and one patch to
>>>reduce code duplication between LDAP and IPA implementation. Then I need
>>>to run some tests but I don't expect much troubles here since I tested
>>>it a lot during development. I'll finish all of this after my Christmas
>>>vacation.
>>>
>>>The patches are probably too big to be sent as an attachment, so until I
>>>complete the last two patches, you can check it on my git repo, branch
>>>sudo [1]. I don't really expect anyone to review them during Christmas
>>>break, but I thought it's a good thing to present if in case someone
>>>will get really bored from all the candies and family visits :-)
>>>
>>>Happy reviewing.
>>>
>>>[1]
https://fedorapeople.org/cgit/pbrezina/public_git/sssd.git/log/?h=sudo
>>
>>Final patches are attached. Happy reviewing.
>>
>
>Here are my first comments:
>
>0001:
>timeout == 0 is checked twice
Fixed.
>+
>+ for (state->map_num_attrs = 0;
>+ state->map[state->map_num_attrs].opt_name != NULL;
>
>wouldn't is be safer to check if map is not NULL before deferencinf it?
Check added.
>Is there a reason for incrementing base_iter at the end of
>sdap_search_bases_next_base(), I think it would be more clear to increment it
>in sdap_search_bases_done() immediately before calling
>sdap_search_bases_next_base() again?
It depends on the point of view, I guess. This way the iteration logic is
all in one place which is better in my opinion.
hm, I see your point. What about initializing base_iter to some magic
value in sdap_search_bases_send and doing something like
state->base_iter = (state->base_iter == MAGIC) ? 0 : ++state->base_iter;
then state->base_iter always points to the current base and you do not
need this odd
base = state->bases[state->base_iter - 1]
in sdap_search_bases_done(). But that's only cosmetic and personal
preference, feel free to ignore it.
>0005:
>
>+int sysdb_compare_usn(const char *a, const char *b)
>+{
>+ if (a == NULL) {
>+ return -1;
>+ }
>+
>+ if (b == NULL) {
>+ return 1;
>+ }
>+
>+ /* less digits means lower number */
>
>this is only true as long as a and b do not start with 0. I would recommend to
>add a
> if (*a != '0' && *b != '0') {
>
I added a cycle to remove leading zeros.
>+ if (strlen(a) < strlen(b)) {
>+ return -1;
>+ }
>+
>+ /* more digits means bigger number */
>+ if (strlen(a) > strlen(b)) {
>+ return 1;
>+ }
>+
>+ /* now we can compare digits since alphabetical order is the same
>+ * as numeric order */
>+ return strcmp(a, b);
>+}
>+errno_t sysdb_get_highest_usn(TALLOC_CTX *mem_ctx,
>+ struct sysdb_attrs **attrs,
>+ size_t num_attrs,
>+ char **_usn)
>+{
>+ const char *highest = NULL;
>+ const char *current = NULL;
>+ char *usn;
>+ errno_t ret;
>+ size_t i;
>+
>+ if (num_attrs == 0 || attrs == NULL) {
>
>you can save a few lines since highest == NULL with:
> goto done;
Done.
>+#define ALL_FILTER "(" SYSDB_OBJECTCLASS "=" SYSDB_SUDO_CACHE_OC
")"
>+
>
>I would prefer a more specific name like e.g. SUDO_ALL_FILTER.
Done.
>0008:
>>@@ -516,6 +516,8 @@ static void sdap_sudo_refresh_done(struct tevent_req
*subreq)
>> tevent_req_error(req, ret);
>> }
>> return;
>>+ } else if (ret != EOK) {
>>+ tevent_req_error(req, ret);
>
>Isn't a return missing here?
Yes, fixed.
>0009:
>
>+static bool check_dn(struct ldb_dn *dn,
>+ const char *rdn_attr,
>+ va_list in_ap)
>+{
>+ const struct ldb_val *ldbval;
>+ const char *strval;
>+ const char *ldbattr;
>+ const char *attr;
>+ const char *val;
>+ va_list ap;
>+ int num_comp;
>+ int comp;
>+
>+ /* check RDN attribute */
>+ ldbattr = ldb_dn_get_rdn_name(dn);
>+ if (ldbattr == NULL || strcasecmp(ldbattr, rdn_attr) != 0) {
>+ return false;
>+ }
>+
>+ /* Check DN components. First we check if all attr=value pairs match input.
>+ * Then we check that the next attribute is a domain component.
>+ */
>+
>+ comp = 1;
>+ num_comp = ldb_dn_get_comp_num(dn);
>+
>+ va_copy(ap, in_ap);
>+ while ((attr = va_arg(ap, const char *)) != NULL) {
>+ val = va_arg(ap, const char *);
>+ if (val == NULL) {
>+ return false;
>+ }
>+
>+ if (comp > num_comp) {
>+ return false;
>+ }
>+
>+ ldbattr = ldb_dn_get_component_name(dn, comp);
>+ if (ldbattr == NULL || strcasecmp(ldbattr, attr) != 0) {
>+ return false;
>+ }
>+
>+ ldbval = ldb_dn_get_component_val(dn, comp);
>+ if (ldbval == NULL) {
>+ return false;
>+ }
>+
>+ strval = (const char *)ldbval->data;
>+ if (strval == NULL || strncasecmp(strval, val, ldbval->length) != 0) {
>+ return false;
>+ }
>+
>+ comp++;
>+ }
>+ va_end(ap);
>+
>+ ldbattr = ldb_dn_get_component_name(dn, comp);
>+ if (ldbattr == NULL || strcmp(ldbattr, "dc") != 0) {
>
>If I see it correctly the "dc" is the only thing specific to IPA (and it
would
>work for AD as well). I would recommend to make the name of the domain
>component a parameter of check_dn() and not put it in ipa/ipa_dn.c but
>ldap/sdap_dn.c. This would avoid in the next patch to include a file from the
>IPA provider in libsss_ldap_common_la_SOURCES.
Even though I don't like having it included in ldap_common.la, I think it is
a lesser evil. You can't use it in LDAP and AD, since you can't expect any
specific dn format there, or can you?
Also having "dc" value a parameter and making the function more general
would make the check and function call much more complicated since we would
have to check for a correct ending (i.e. dc=example,dc=com or whatever is
set) and not just for a presence of dc. But we can work on that if you want,
I just think it is unnecessary for the given usage.
ok, it makes it at least obvious that some IPA details are leaking in
the general LDAP/SDAP code because of sdap_nested_group_get_ipa_user()
and stays there as a reminder to fix it.
>
>
>+ return false;
>+ }
>+
>+ return true;
>+}
>
>
>That's all for now, more will come later.
Thank you, patches are attached.
I started testing and found an issues when using the compat version from
ou=sudoers,dc=ipa,dc=devel and some issue if there are no command groups
on the server. Please find the fixes I used in attached patch.
bye,
Sumit