On Thu, Sep 25, 2014 at 01:46:00PM +0200, Sumit Bose wrote: > On Wed, Sep 24, 2014 at 03:23:54PM +0200, Jakub Hrozek wrote: > > On Tue, Sep 23, 2014 at 05:11:01PM +0200, Sumit Bose wrote: > > > Hi, > > > > > > this patch should fix https://fedorahosted.org/freeipa/ticket/4031 and > > > with the corresponding SSSD part it would be possible to get the full > > > list of group memberships with the id command even for user who didn't > > > log in before. > > > > > > bye, > > > Sumit > > > > So far I only read the patch, no testing was done yet (I'm installing a > > separate VM where I'll keep this new plugin for easy comparison and > > backwards-compatibility testing) > > Thank you for the review, please see comments below. > > > > > First, there are some Coverity warnings: > > > > Error: USE_AFTER_FREE (CWE-825): > > freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:242: > > alias: Assigning: "groups" = "new_groups". Now both point to the same > > storage. > > freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:246: > > freed_arg: "free(void *)" frees "groups". > > freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:252: > > use_after_free: Using freed pointer "groups". > > fixed > > > > > Error: CONSTANT_EXPRESSION_RESULT (CWE-398): > > freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:596: > > missing_parentheses: "!id_type != SSS_ID_TYPE_GID" is always true > > regardless of the values of its operands. Did you intend to either negate > > the entire comparison expression, in which case parentheses would be > > required around the entire comparison expression to force that > > interpretation, or negate the sense of the comparison (that is, use '==' > > rather than '!=')? This occurs as the logical second operand of '||'. > > fixed > > > > > Error: DEADCODE (CWE-561): > > freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:594: > > cond_cannot_single: Condition "request_type == 1U", taking false branch. > > Now the value of "request_type" cannot be equal to 1. > > freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:594: > > cond_cannot_set: Condition "request_type == 3U", taking false branch. Now > > the value of "request_type" cannot be equal to any of {1, 3}. > > freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:606: > > cannot_set: At condition "request_type == 1U", the value of "request_type" > > cannot be equal to any of {1, 3}. > > freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:606: > > dead_error_condition: The condition "request_type == 1U" cannot be true. > > freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:607: > > dead_error_line: Execution cannot reach this statement "ret = > > pack_ber_sid(sid_str,...". > > I thik this is a result of the CONSTANT_EXPRESSION_RESULT issue, since I > fixed it this warning should be gone as well. > > > > > See some comments inline. > > > > > From 23ff38cdea85995b211e73f474bcb4b0d7fb8039 Mon Sep 17 00:00:00 2001 > > > From: Sumit Bose <sb...@redhat.com> > > > Date: Tue, 23 Sep 2014 15:55:43 +0200 > > > Subject: [PATCH] extdom: add support for new version > > > > > > Currently the extdom plugin is basically used to translate SIDs of AD > > > users and groups to names and POSIX IDs. > > > > > > With this patch a new version is added which will return the full member > > > list for groups and the full list of group memberships for a user. > > > Additionally the gecos field, the home directory and the login shell of a > > > user are returned and an optional list of key-value pairs which > > > currently will contain the SID of the requested object if available. > > > > > > https://fedorahosted.org/freeipa/ticket/4031 > > > --- > > > .../ipa-extdom-extop/ipa_extdom.h | 29 +- > > > .../ipa-extdom-extop/ipa_extdom_common.c | 850 > > > +++++++++++++++------ > > > .../ipa-extdom-extop/ipa_extdom_extop.c | 28 +- > > > 3 files changed, 640 insertions(+), 267 deletions(-) > > > > > > diff --git a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h > > > b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h > > > index > > > 5f834a047a579104cd2589ce417c580c1c5388d3..548ee74f561c474854c049726c4c3e71da5cbbea > > > 100644 > > > --- a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h > > > +++ b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h > > > @@ -64,6 +64,7 @@ > > > #include <sss_nss_idmap.h> > > > > > > #define EXOP_EXTDOM_OID "2.16.840.1.113730.3.8.10.4" > > > +#define EXOP_EXTDOM_V2_OID "2.16.840.1.113730.3.8.10.4.1" > > > > It's a bit odd that this control is called V1 in the SSSD tree and V2 in > > the IPA tree. It's not wrong, just strange maybe. > > you are right, I renamed the versions here. > > > > > > > > > -int handle_request(struct ipa_extdom_ctx *ctx, struct extdom_req *req, > > > - struct extdom_res **res) > > > +int check_request(struct extdom_req *req, enum extdom_version version) > > > +{ > > > + if (version == EXTDOM_V1) { > > > + if (req->request_type == REQ_FULL_WITH_GROUPS) { > > > + return LDAP_PROTOCOL_ERROR; > > > + } > > > + } > > > > Any particular reason why these conditions are nested and not and-ed ? > > Did you expect more under the EXTDOM_V1 condition? > > I'm not expecting them, but who knows :-) I think this way it is more > clear that we are testing features of a specific version here. > > > > > > + > > > + return LDAP_SUCCESS; > > > +} > > > + > > > +static int get_buffer(size_t *_buf_len, char **_buf) > > > { > > > - int ret; > > > - char *domain_name = NULL; > > > - char *sid_str = NULL; > > > - size_t buf_len; > > > - char *buf = NULL; > > > long pw_max; > > > long gr_max; > > > - struct pwd_grp pg_data; > > > - struct passwd *pwd_result = NULL; > > > - struct group *grp_result = NULL; > > > - enum sss_id_type id_type; > > > - char *fq_name = NULL; > > > - char *sep; > > > - > > > + size_t buf_len; > > > + char *buf; > > > > > > pw_max = sysconf(_SC_GETPW_R_SIZE_MAX); > > > gr_max = sysconf(_SC_GETGR_R_SIZE_MAX); > > > @@ -211,302 +212,655 @@ int handle_request(struct ipa_extdom_ctx *ctx, > > > struct extdom_req *req, > > > return LDAP_OPERATIONS_ERROR; > > > } > > > > > > - switch (req->input_type) { > > > - case INP_POSIX_UID: > > > - if (req->request_type == REQ_SIMPLE) { > > > - ret = sss_nss_getsidbyid(req->data.posix_uid.uid, &sid_str, > > > - &id_type); > > > + *_buf_len = buf_len; > > > + *_buf = buf; > > > + > > > + return LDAP_SUCCESS; > > > +} > > > + > > > +static int get_user_grouplist(const char *name, gid_t gid, > > > + size_t *_ngroups, gid_t **_groups ) > > > +{ > > > + int ret; > > > + int ngroups; > > > + gid_t *groups; > > > + gid_t *new_groups; > > > + > > > + ngroups = 128; > > > > I was wondering whether to use _SC_NGROUPS_MAX or NGROUPS_MAX here, but > > I guess you're right it's very unlikely that a user will be a member of > > more than 128 groups so we'd just clinge to more memory than needed.. > > > > > + groups = malloc(ngroups * sizeof(gid_t)); > > > + if (groups == NULL) { > > > + return LDAP_OPERATIONS_ERROR; > > > + } > > > + > > > + ret = getgrouplist(name, gid, groups, &ngroups); > > > + if (ret == -1) { > > > + new_groups = realloc(groups, ngroups); > > > + if (new_groups == NULL) { > > > + free(groups); > > > + return LDAP_OPERATIONS_ERROR; > > > + } > > > + groups = new_groups; > > > + > > > + ret = getgrouplist(name, gid, groups, &ngroups); > > > + if (ret == -1) { > > > + free(groups); > > > + ret = LDAP_OPERATIONS_ERROR; > > > + } > > > + } > > > + > > > + *_ngroups = ngroups; > > > + *_groups = groups; > > > + > > > + return LDAP_SUCCESS; > > > +} > > > + > > > +static int pack_ber_sid(const char *sid, struct berval **berval) > > > +{ > > > + BerElement *ber = NULL; > > > + int ret; > > > + > > > + ber = ber_alloc_t( LBER_USE_DER ); > > > + if (ber == NULL) { > > > + return LDAP_OPERATIONS_ERROR; > > > + } > > > + > > > + ret = ber_printf(ber,"{es}", RESP_SID, sid); > > > + if (ret == -1) { > > > + ber_free(ber, 1); > > > + return LDAP_OPERATIONS_ERROR; > > > + } > > > + > > > + ret = ber_flatten(ber, berval); > > > + ber_free(ber, 1); > > > + if (ret == -1) { > > > + return LDAP_OPERATIONS_ERROR; > > > + } > > > + > > > + return LDAP_SUCCESS; > > > +} > > > + > > > +#define SSSD_SYSDB_SID_STR "objectSIDString" > > > + > > > +static int pack_ber_user(const char *domain_name, const char *user_name, > > > + uid_t uid, gid_t gid, > > > + const char *gecos, const char *homedir, > > > + const char *shell, const char *sid_str, > > > + struct berval **berval) > > > +{ > > > + BerElement *ber = NULL; > > > + int ret; > > > + enum response_types response_type; > > > + size_t ngroups; > > > + gid_t *groups = NULL; > > > + size_t buf_len; > > > + char *buf = NULL; > > > + struct group grp; > > > + struct group *grp_result; > > > + size_t c; > > > + char *locat; > > > + char *short_user_name = NULL; > > > + const char *single_value_string_array[] = {NULL, NULL}; > > > + > > > + if (gecos == NULL && homedir == NULL && shell == NULL) { > > > + response_type = RESP_USER; > > > + } else { > > > + response_type = RESP_USER_GROUPLIST; > > > + } > > > + > > > + short_user_name = strdup(user_name); > > > + if ((locat = strchr(short_user_name, SSSD_DOMAIN_SEPARATOR)) != > > > NULL) { > > > > Some functions in the code use strchr to fund the at-sign, some use > > strrch. Could we standardize on one or the other? Do you expect some > > usernames with an at-sign in them? > > I think the 'rr' version was just a typo, I changed it to 'r'. > > > > > > + if (strcasecmp(locat+1, domain_name) == 0 ) { > > > + locat[0] = '\0'; > > > } else { > > > - id_type = SSS_ID_TYPE_UID; > > > - ret = getpwuid_r(req->data.posix_uid.uid, &pg_data.data.pwd, > > > buf, > > > - buf_len, &pwd_result); > > > + ret = LDAP_NO_SUCH_OBJECT; > > > + goto done; > > > } > > > + } > > > > > > - domain_name = strdup(req->data.posix_uid.domain_name); > > > - break; > > > - case INP_POSIX_GID: > > > - if (req->request_type == REQ_SIMPLE) { > > > - ret = sss_nss_getsidbyid(req->data.posix_uid.uid, &sid_str, > > > - &id_type); > > > + ber = ber_alloc_t( LBER_USE_DER ); > > > + if (ber == NULL) { > > > + return LDAP_OPERATIONS_ERROR; > > > + } > > > + > > > + ret = ber_printf(ber,"{e{ssii", response_type, domain_name, > > > short_user_name, > > > + uid, gid); > > > + if (ret == -1) { > > > + ret = LDAP_OPERATIONS_ERROR; > > > + goto done; > > > + } > > > + > > > + if (response_type == RESP_USER_GROUPLIST) { > > > + ret = get_user_grouplist(user_name, gid, &ngroups, &groups); > > > + if (ret != LDAP_SUCCESS) { > > > + goto done; > > > + } > > > + > > > + ret = get_buffer(&buf_len, &buf); > > > + if (ret != LDAP_SUCCESS) { > > > + goto done; > > > + } > > > + > > > + ret = ber_printf(ber,"sss", gecos, homedir, shell); > > > + if (ret == -1) { > > > + ret = LDAP_OPERATIONS_ERROR; > > > + goto done; > > > + } > > > + > > > + ret = ber_printf(ber,"{"); > > > + if (ret == -1) { > > > + ret = LDAP_OPERATIONS_ERROR; > > > + goto done; > > > + } > > > + > > > + for (c = 0; c < ngroups; c++) { > > > + ret = getgrgid_r(groups[c], &grp, buf, buf_len, &grp_result); > > > + if (ret != 0) { > > > + ret = LDAP_OPERATIONS_ERROR; > > > + goto done; > > > + } > > > + if (grp_result == NULL) { > > > + ret = LDAP_NO_SUCH_OBJECT; > > > + goto done; > > > > I wanted to check if you think it's better to continue or fail here. Did > > you opt for failing because you were afraid of missing some deny access > > checks in case we couldn't resolv a group? > > I think there is a disconnect if getgrouplist() returns a GID that > cannot be resolved so I prefer an error in this case. > > > > > > + } > > > + > > > + ret = ber_printf(ber, "s", grp.gr_name); > > > + if (ret == -1) { > > > + ret = LDAP_OPERATIONS_ERROR; > > > + goto done; > > > + } > > > + } > > > + > > > + ret = ber_printf(ber,"}"); > > > + if (ret == -1) { > > > + ret = LDAP_OPERATIONS_ERROR; > > > + goto done; > > > + } > > > + > > > + single_value_string_array[0] = sid_str; > > > + ret = ber_printf(ber,"{{s{v}}}", SSSD_SYSDB_SID_STR, > > > + single_value_string_array); > > > + if (ret == -1) { > > > + ret = LDAP_OPERATIONS_ERROR; > > > + goto done; > > > + } > > > + } > > > + > > > + ret = ber_printf(ber,"}}"); > > > + if (ret == -1) { > > > + ret = LDAP_OPERATIONS_ERROR; > > > + goto done; > > > + } > > > + > > > + ret = ber_flatten(ber, berval); > > > + if (ret == -1) { > > > + ret = LDAP_OPERATIONS_ERROR; > > > + goto done; > > > + } > > > + > > > + ret = LDAP_SUCCESS; > > > +done: > > > + free(short_user_name); > > > + free(groups); > > > + free(buf); > > > + ber_free(ber, 1); > > > + return ret; > > > +} > > > + > > > +static int pack_ber_group(const char *domain_name, const char > > > *group_name, > > > + gid_t gid, char **members, const char *sid_str, > > > + struct berval **berval) > > > +{ > > > + BerElement *ber = NULL; > > > + int ret; > > > + size_t c; > > > + char *locat; > > > + char *short_group_name = NULL; > > > + const char *single_value_string_array[] = {NULL, NULL}; > > > + > > > + short_group_name = strdup(group_name); > > > + if ((locat = strchr(short_group_name, SSSD_DOMAIN_SEPARATOR)) != > > > NULL) { > > > + if (strcasecmp(locat+1, domain_name) == 0 ) { > > > + locat[0] = '\0'; > > > } else { > > > - id_type = SSS_ID_TYPE_GID; > > > - ret = getgrgid_r(req->data.posix_gid.gid, &pg_data.data.grp, > > > buf, > > > - buf_len, &grp_result); > > > + ret = LDAP_NO_SUCH_OBJECT; > > > + goto done; > > > } > > > + } > > > > > > - domain_name = strdup(req->data.posix_gid.domain_name); > > > - break; > > > - case INP_SID: > > > - ret = sss_nss_getnamebysid(req->data.sid, &fq_name, &id_type); > > > - if (ret != 0) { > > > + ber = ber_alloc_t( LBER_USE_DER ); > > > + if (ber == NULL) { > > > + return LDAP_OPERATIONS_ERROR; > > > + } > > > + > > > + ret = ber_printf(ber,"{e{ssi", members == NULL ? RESP_GROUP > > > + : RESP_GROUP_MEMBERS, > > > > Each pack_ber_group is called like this: > > 718 if (request_type == REQ_FULL) { > > 719 ret = pack_ber_group(domain_name, grp.gr_name, grp.gr_gid, > > 720 NULL, NULL, berval); > > 721 } else { > > 722 ret = pack_ber_group(domain_name, grp.gr_name, grp.gr_gid, > > 723 grp.gr_mem, sid, berval); > > 724 } > > > > And then you guess the request_type again based on the parameter > > values. Isn't it safer to add the request type parameter avoid the if-else > > switch in the callers? Or were you trying to be on the safe side to avoid > > checking the validity members array in the pack_ber_group function and have > > the array set to NULL by the caller? > > You are right, the if-block is odd. Instead of the request_type I added > the response_type to the argument list of pack_ber_user() and > pack_ber_group() which I think is more natural because it is the > response that is packed. > > > > > The rest of the file looks to me, just the same "issue" with guessing the > > request type is repeated. > > > > New version attached. > > bye, > Sumit
Hi, Jakub found another issue which is fixed with this new version. bye, Sumit _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
