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
From 62a3c79d7923dceb2c92c1b2d31388afd744a8ac Mon Sep 17 00:00:00
2001
From: Sumit Bose <sbose(a)redhat.com>
Date: Wed, 10 Feb 2016 14:59:06 +0100
Subject: [PATCH 4/8] AD: process PAC during initgroups request
If there is a recently attached PAC blob in the cached user entry the
PAC data is used to update the group memberships data of the user. If
there is no PAC attached or if it is too old the other configured
methods will be used.
---
Makefile.am | 20 ++
src/providers/ad/ad_id.c | 52 ++-
src/providers/ad/ad_pac.c | 666 ++++++++++++++++++++++++++++++++++++++
src/providers/ad/ad_pac.h | 82 +++++
src/providers/ad/ad_pac_common.c | 86 +++++
src/tests/cmocka/test_ad_common.c | 346 +++++++++++++++++++-
6 files changed, 1243 insertions(+), 9 deletions(-)
create mode 100644 src/providers/ad/ad_pac.c
create mode 100644 src/providers/ad/ad_pac.h
create mode 100644 src/providers/ad/ad_pac_common.c
I assume (I didn't try to bisect) this patch broke a build
with disabled pac responder and missing header file gen_ndr/krb5pac.h
LS