On Tue, Mar 15, 2016 at 04:50:48PM +0100, Lukas Slebodnik wrote:
On (15/03/16 10:04), Pavel Březina wrote:
>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?
I made this value configurable now but so far kept the default of 5 minutes.
Since the PAC is renewed during every authentication I think the value
is ok, but fell free to suggest a short timeout.
>
>>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.
done
>
>>
>> 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.
done
>
>And finally CI failed on Debian (ld error in tests)
>http://sssd-ci.duckdns.org/logs/job/39/29/debian_testing/ci-build-debug/ci-make-tests.log
"ld error" means linker error in test_search_bases :-)
undefined reference to symbol 'idmap_error_string@(a)SSS_IDMAP_0.4'
//var/lib/jenkins/workspace/ci/label/debian_testing/ci-build-debug/.libs/libsss_idmap.so.0:
error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status
fixed
New version attached.
bye,
Sumit
LS
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org