On 03/14/2016 12:12 PM, Sumit Bose wrote:
Hi,
this patch-set should solve
https://fedorahosted.org/sssd/ticket/2158 by
moving the main processing of the PAC data to the backends. The PAC
responder now only has to send one request to the backend to look up the
user based on its SID if the user's SID was not found in the cache. I
used the new cache_req here so the patches depend on Pavel's latest
cache_req patches which are currently under review.
Since new IPA server can provided group-membership data to IPA clients
via the extdom plugin the most important test-cases are currently
against older IPA servers e.g. version 3.3.3 provided by RHEL/CentOS 7.0
(7.1 already has a newer version with enhanced extdom plugin).
bye,
Sumit
Hi,
I didn't test the patches yet, but I have some comments on patch "AD:
process PAC during initgroups request".
#define PAC_TIMEOUT 300 /* 5 minutes, should this be configurable? */
Five minutes is quite a long time. Why did you choose this value?
errno_t check_if_pac_is_available(TALLOC_CTX *mem_ctx,
struct sss_domain_info *dom,
struct be_acct_req *ar,
struct ldb_message **_msg)
{
const char *user_attrs[] = { SYSDB_NAME, SYSDB_OBJECTCLASS,
SYSDB_PAC_BLOB, SYSDB_PAC_BLOB_ADD_TIME,
NULL };
struct ldb_message *msg;
struct ldb_result *res;
struct ldb_message_element *el;
uint64_t age;
time_t now;
int ret;
char *user_name;
if (dom == NULL || ar == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "Missing arguments.\n");
return EINVAL;
}
if (ar->extra_value && strcmp(ar->extra_value, EXTRA_NAME_IS_UPN) == 0)
{
ret = sysdb_search_user_by_upn(mem_ctx, dom, ar->filter_value,
user_attrs, &msg);
} else {
switch (ar->filter_type) {
case BE_FILTER_SECID:
ret = sysdb_search_user_by_sid_str(mem_ctx, dom, ar->filter_value,
user_attrs, &msg);
break;
case BE_FILTER_UUID:
ret = sysdb_search_object_by_uuid(mem_ctx, dom, ar->filter_value,
user_attrs, &res);
if (ret == EOK) {
if (res->count == 1) {
msg = res->msgs[0];
} else {
DEBUG(SSSDBG_CRIT_FAILURE,
"Search by UUID returned multiple results.\n");
return EINVAL;
}
}
break;
case BE_FILTER_NAME:
user_name = sss_get_domain_name(mem_ctx, ar->filter_value, dom);
if (user_name == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "sss_get_domain_name failed.\n");
ret = EINVAL;
} else {
ret = sysdb_search_user_by_name(mem_ctx, dom, user_name,
user_attrs, &msg);
}
break;
default:
DEBUG(SSSDBG_OP_FAILURE, "Unsupported filter type [%d].\n",
ar->filter_type);
return EINVAL;
}
}
if (ret != EOK) {
if (ret == ENOENT) {
DEBUG(SSSDBG_TRACE_ALL, "No user found with filter [%s].\n",
ar->filter_value);
} else {
DEBUG(SSSDBG_OP_FAILURE,
"Looking up user in cache with filter [%s] failed.\n",
ar->filter_value);
}
return ret;
}
The code above would work well as a separate function that returns you
ldb_message based on filter_type and filter_value.
el = ldb_msg_find_element(msg, SYSDB_PAC_BLOB);
if (el == NULL) {
DEBUG(SSSDBG_TRACE_ALL, "No PAC available.\n");
talloc_free(msg);
return ENOENT;
}
age = ldb_msg_find_attr_as_uint64(msg, SYSDB_PAC_BLOB_ADD_TIME, 0);
now = time(NULL);
if (age < now && (now - age) > PAC_TIMEOUT) {
DEBUG(SSSDBG_TRACE_FUNC, "PAC avaiable but too old.\n");
talloc_free(msg);
return ENOENT;
}
if (_msg != NULL) {
*_msg = msg;
}
return EOK;
}
errno_t ad_get_sids_from_pac(TALLOC_CTX *mem_ctx,
struct sss_idmap_ctx *idmap_ctx,
struct PAC_LOGON_INFO *logon_info,
char **_user_sid_str,
char **_primary_group_sid_str,
size_t *_num_sids,
char *** _sid_list)
errno_t ad_get_pac_data_from_user_entry(TALLOC_CTX *mem_ctx,
struct ldb_message *msg,
struct sss_idmap_ctx *idmap_ctx,
char **username,
char **user_sid,
char **primary_group_sid,
size_t *num_sids,
char ***group_sids)
Can you use tmp_ctx in those functions please? It would simplify the
first one and you are potentially leaking 'username' in the second.
And finally CI failed on Debian (ld error in tests)
http://sssd-ci.duckdns.org/logs/job/39/29/debian_testing/ci-build-debug/c...