On Wed, Jan 13, 2016 at 11:21:54AM +0100, Pavel Březina wrote:
On 01/12/2016 02:20 PM, Pavel Březina wrote:
>On 01/12/2016 01:53 PM, Pavel Březina wrote:
>>On 01/12/2016 01:19 PM, Sumit Bose wrote:
>>>On Mon, Jan 11, 2016 at 12:16:29PM +0100, Pavel Březina wrote:
>>>>On 01/08/2016 04:37 PM, Sumit Bose wrote:
>>>>>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
>>>>>
>>>>
>>>>Hi,
>>>>thank you for the fixes. I squashed them into the patch set and hope
>>>>you
>>>>don't mind.
>>>
>>>of course not
>>>
>>>>
>>>>I have changed search_bases a little, added a state->cur_base that
>>>>points to
>>>>the currently selected search base. This solves the -1 problem you
>>>>mentioned
>>>>and looks better in my opinion then using some magic value.
>>>
>>>good idea, I think with this you can even say
>>>
>>>+ state->cur_base = state->bases[state->base_iter++];
>>>
>>>and drop the state->base_iter++; line.
>>>
>>>>
>>>>I also moved the first change in your patch (if (num_attrs != 0)) into
>>>>ipa_sudo_filter_rules_bycmdgroups as:
>>>>
>>>> if (num_cmdgroups == 0) {
>>>> *_filter = NULL;
>>>> return EOK;
>>>> }
>>>>
>>>
>>>ok.
>>>
>>>I'm done with my testing. I basically added different kinds of sudo
>>>related IPA objects and compared what is written to the cache with the
>>>old LDAP scheme and the new IPA scheme. Except for some obvious
>>>differences like no entryUSN with the IPA scheme and different original*
>>>attributes I found only one issue.
>>>
>>>If a rule object on the server-side has no sudoHost attribute is is
>>>still read and written to the cache by the LDAP scheme:
>>>
>>>dn: name=sudorule008,cn=sudorules,cn=custom,cn=ipa.devel,cn=sysdb
>>>cn: sudorule008
>>>dataExpireTimestamp: 1452605137
>>>entryUSN: 358647
>>>name: sudorule008
>>>objectClass: sudoRule
>>>originalDN: cn=sudorule008,ou=sudoers,dc=ipa,dc=devel
>>>sudoUser: ALL
>>>distinguishedName:
>>>name=sudorule008,cn=sudorules,cn=custom,cn=ipa.devel,cn=sys
>>> db
>>>
>>>but it is not read by the IPA scheme. Does this make a difference in
>>>the later
>>>processing by the sudo responder, i.e. will sudorule008 be available on
>>>the client with the LDAP scheme?
>>>
>>>bye,
>>>Sumit
>>
>>Hi, thank you for review. Yes, we should download even rules without
>>host specified, nice catch. I'm attaching new patch set, the diff is:
>>
>>static char *
>>ipa_sudo_host_filter(TALLOC_CTX *mem_ctx,
>> struct ipa_hostinfo *host,
>> struct sdap_attr_map *map)
>>{
>> TALLOC_CTX *tmp_ctx;
>> char *filter;
>> size_t i;
>>
>> /* If realloc fails we will free all data through tmp_ctx. */
>> tmp_ctx = talloc_new(NULL);
>> if (tmp_ctx == NULL) {
>> return NULL;
>> }
>>
>>+ filter = talloc_asprintf(tmp_ctx, "(!(%s=*))",
>>+ map[IPA_AT_SUDORULE_HOST].name);
>>+ if (filter == NULL) {
>>+ goto fail;
>>+ }
>>
>>This patch set also contains four new patches, but they are quite
>>trivial. It actually allows setting full_refresh=0 and smart_refresh !=
>>0 which should be supported but actually never was.
>
>The previous tar ball got malformed. New one is attached.
I'm sending patches with fixed few Coverity issues that Sumit sent me
off-list. The diff is:
diff --git a/src/providers/ipa/ipa_dn.c b/src/providers/ipa/ipa_dn.c
index 9f2950b..c58e014 100644
--- a/src/providers/ipa/ipa_dn.c
+++ b/src/providers/ipa/ipa_dn.c
@@ -53,26 +53,26 @@ static bool check_dn(struct ldb_dn *dn,
while ((attr = va_arg(ap, const char *)) != NULL) {
val = va_arg(ap, const char *);
if (val == NULL) {
- return false;
+ goto vafail;
}
if (comp > num_comp) {
- return false;
+ goto vafail;
}
ldbattr = ldb_dn_get_component_name(dn, comp);
if (ldbattr == NULL || strcasecmp(ldbattr, attr) != 0) {
- return false;
+ goto vafail;
}
ldbval = ldb_dn_get_component_val(dn, comp);
if (ldbval == NULL) {
- return false;
+ goto vafail;
}
strval = (const char *)ldbval->data;
if (strval == NULL || strncasecmp(strval, val, ldbval->length) !=
0) {
- return false;
+ goto vafail;
}
diff --git a/src/providers/ipa/ipa_sudo_async.c
b/src/providers/ipa/ipa_sudo_async.c
index c2ee1b3..e2783d3 100644
--- a/src/providers/ipa/ipa_sudo_async.c
+++ b/src/providers/ipa/ipa_sudo_async.c
@@ -1046,7 +1046,7 @@ ipa_sudo_refresh_done(struct tevent_req *subreq)
{
struct ipa_sudo_refresh_state *state;
struct tevent_req *req;
- char *usn;
+ char *usn = NULL;
bool in_transaction = false;
errno_t sret;
int ret;
diff --git a/src/providers/ipa/ipa_sudo_conversion.c
b/src/providers/ipa/ipa_sudo_conversion.c
index 87f6462..5bba714 100644
--- a/src/providers/ipa/ipa_sudo_conversion.c
+++ b/src/providers/ipa/ipa_sudo_conversion.c
@@ -585,6 +585,7 @@ build_filter(TALLOC_CTX *mem_ctx,
filter = talloc_strdup(tmp_ctx, "");
if (filter == NULL) {
+ ret = ENOMEM;
goto done;
}
@@ -1018,7 +1019,6 @@ rules_iterator(hash_entry_t *item,
if (ctx == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
- ctx->ret = ERR_INTERNAL;
return false;
}
@@ -1066,7 +1066,6 @@ cmdgroups_iterator(hash_entry_t *item,
if (ctx == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
- ctx->ret = ERR_INTERNAL;
return false;
}
it looks like the last 2 hunks are not available in the latest tar ball.
and there are some trailing whitespaces in patch 9 at line 41.
bye,
Sumit